Merge branch...

Merge branch '6877-geo-the-storage-config-in-the-other-section-is-broken-in-the-geo-nodes-admin-page' into 'master'

Resolve "Geo - The "Storage config" in the "Other section" is broken in the Geo nodes admin page"

Closes #6877 and #4851

See merge request gitlab-org/gitlab-ee!6608

db/schema.rb

@@ -1155,6 +1155,7 @@ ActiveRecord::Schema.define(version: 20180722103201) do
     t.integer "wikis_checksummed_count"
     t.integer "wikis_checksum_failed_count"
     t.integer "wikis_checksum_mismatch_count"
+    t.binary "storage_configuration_digest"
   end
 
   add_index "geo_node_statuses", ["geo_node_id"], name: "index_geo_node_statuses_on_geo_node_id", unique: true, using: :btree

ee/app/models/geo_node_status.rb

 class GeoNodeStatus < ActiveRecord::Base
+  include ShaAttribute
+
   belongs_to :geo_node
 
   delegate :selective_sync_type, to: :geo_node
@@ -18,6 +20,8 @@ class GeoNodeStatus < ActiveRecord::Base
                 :hashed_storage_migrated_max_id, :hashed_storage_attachments_max_id,
                 :repositories_checked_count, :repositories_checked_failed_count
 
+  sha_attribute :storage_configuration_digest
+
   # Be sure to keep this consistent with Prometheus naming conventions
   PROMETHEUS_METRICS = {
     db_replication_lag_seconds: 'Database replication lag (seconds)',
@@ -155,6 +159,7 @@ class GeoNodeStatus < ActiveRecord::Base
     self.attachments_count = attachments_finder.count_syncable
     self.last_successful_status_check_at = Time.now
     self.storage_shards = StorageShard.all
+    self.storage_configuration_digest = StorageShard.build_digest
 
     self.version = Gitlab::VERSION
     self.revision = Gitlab.revision
@@ -330,13 +335,11 @@ class GeoNodeStatus < ActiveRecord::Base
     calc_percentage(replication_slots_count, replication_slots_used_count)
   end
 
-  # This method only is useful when the storage shard information is loaded
-  # from a remote node via JSON.
   def storage_shards_match?
-    return unless Gitlab::Geo.primary?
-    return unless current_shards && primary_shards
+    return true if geo_node.primary?
+    return false unless storage_configuration_digest && primary_storage_digest
 
-    shards_match?(current_shards, primary_shards)
+    storage_configuration_digest == primary_storage_digest
   end
 
   def [](key)
@@ -345,28 +348,8 @@ class GeoNodeStatus < ActiveRecord::Base
 
   private
 
-  def current_shards
-    serialize_storage_shards(storage_shards)
-  end
-
-  def primary_shards
-    serialize_storage_shards(StorageShard.all)
-  end
-
-  def serialize_storage_shards(shards)
-    StorageShardSerializer.new.represent(shards).as_json
-  end
-
-  def shards_match?(first, second)
-    names_match?(first, second)
-  end
-
-  def names_match?(first, second)
-    extract_names(first) == extract_names(second)
-  end
-
-  def extract_names(shards)
-    shards.map { |shard| shard['name'] }.sort
+  def primary_storage_digest
+    @primary_storage_digest ||= Gitlab::Geo.primary_node.find_or_build_status.storage_configuration_digest
   end
 
   def attachments_finder

ee/app/models/storage_shard.rb

+require 'digest'
 # This is an in-memory structure only. The repository storage configuration is
 # in gitlab.yml and not in the database. This model makes it easier to work
 # with the configuration.
@@ -25,4 +26,9 @@ class StorageShard
   def self.allowed_params
     %i(name).freeze
   end
+
+  def self.build_digest
+    names = Settings.repositories.storages.keys.sort
+    Digest::SHA1.hexdigest(names.join)
+  end
 end

ee/changelogs/unreleased/6877-geo-the-storage-config-in-the-other-section-is-broken-in-the-geo-nodes-admin-page.yml

+---
+title: "[Geo] Fix the Storage config parameter in Geo nodes admin page"
+merge_request:
+author:
+type: fixed

ee/db/migrate/20180719161844_add_storage_configuration_digest.rb

+class AddStorageConfigurationDigest < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :geo_node_statuses, :storage_configuration_digest, :binary
+  end
+end

ee/lib/ee/api/entities.rb

@@ -367,9 +367,7 @@ module EE
           status.storage_shards.present?
         end
 
-        expose :storage_shards_match?, as: :storage_shards_match, if: ->(status, options) do
-          ::Gitlab::Geo.primary? && status.storage_shards.present?
-        end
+        expose :storage_shards_match?, as: :storage_shards_match
 
         expose :_links do
           expose :self do |geo_node_status|

ee/spec/lib/ee/api/entities/geo_node_status_spec.rb

@@ -133,6 +133,6 @@ describe EE::API::Entities::GeoNodeStatus, :postgresql do
     end
 
     it { is_expected.to have_key(:storage_shards) }
-    it { is_expected.not_to have_key(:storage_shards_match) }
+    it { is_expected.to have_key(:storage_shards_match) }
   end
 end

ee/spec/models/geo_node_status_spec.rb

@@ -874,30 +874,32 @@ describe GeoNodeStatus, :geo do
   end
 
   describe '#storage_shards_match?' do
-    before do
+    it 'returns false if no shard data is available for secondary' do
       stub_primary_node
+      stub_current_geo_node(secondary)
+
+      status = create(:geo_node_status, geo_node: secondary, storage_configuration_digest: 'bc11119c101846c20367fff34ce9fffa9b05aab8')
+
+      expect(status.storage_shards_match?).to be false
     end
 
-    set(:status) { create(:geo_node_status) }
-    let(:data) { GeoNodeStatusSerializer.new.represent(status).as_json }
-    let(:result) { described_class.from_json(data) }
+    it 'returns true even if no shard data is available for secondary' do
+      stub_secondary_node
+      stub_current_geo_node(primary)
 
-    it 'returns nil if no shard data is available' do
-      data.delete('storage_shards')
+      status = create(:geo_node_status, geo_node: primary, storage_configuration_digest: 'bc11119c101846c20367fff34ce9fffa9b05aab8')
 
-      expect(result.storage_shards_match?).to be nil
+      expect(status.storage_shards_match?).to be true
     end
 
     it 'returns false if the storage shards do not match' do
-      data['storage_shards'].first['name'] = 'broken-shard'
-
-      expect(result.storage_shards_match?).to be false
-    end
+      stub_primary_node
+      stub_current_geo_node(secondary)
+      create(:geo_node_status, geo_node: primary, storage_configuration_digest: 'aea7849c10b886c202676ff34ce9fdf0940567b8')
 
-    it 'returns true if the storage shards match in different order' do
-      status.storage_shards.shuffle!
+      status = create(:geo_node_status, geo_node: secondary, storage_configuration_digest: 'bc11119c101846c20367fff34ce9fffa9b05aab8')
 
-      expect(result.storage_shards_match?).to be true
+      expect(status.storage_shards_match?).to be false
     end
   end
 

ee/spec/models/storage_shard_spec.rb

 require 'spec_helper'
 
 describe StorageShard do
-  describe '.current_shards' do
+  describe '.all' do
     it 'returns an array of StorageShard objects' do
       shards = described_class.all
 
@@ -9,4 +9,10 @@ describe StorageShard do
       expect(shards.map(&:name)).to match_array(Settings.repositories.storages.keys)
     end
   end
+
+  describe '.build_digest' do
+    it 'returns SHA1 digest for the current configuration' do
+      expect(described_class.build_digest).to eq('aea7849c10b886c202676ff34ce9fdf0940567b8')
+    end
+  end
 end