Improve implementation and testing of storage shard matching

ee/app/models/geo_node_status.rb

@@ -154,10 +154,12 @@ 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?
 
-    storage_shards.as_json == StorageShardSerializer.new.represent(StorageShard.all).as_json
+    shards_match?(storage_shards.as_json, current_shards.as_json)
   end
 
   def [](key)
@@ -166,6 +168,18 @@ class GeoNodeStatus < ActiveRecord::Base
 
   private
 
+  def current_shards
+    StorageShardSerializer.new.represent(StorageShard.all)
+  end
+
+  def shards_match?(first, second)
+    sort_by_name(first) == sort_by_name(second)
+  end
+
+  def sort_by_name(shards)
+    shards.sort_by { |shard| shard['name'] }
+  end
+
   def attachments_finder
     @attachments_finder ||= Geo::AttachmentRegistryFinder.new(current_node: geo_node)
   end

ee/app/serializers/geo_node_status_entity.rb

@@ -70,7 +70,7 @@ class GeoNodeStatusEntity < Grape::Entity
   end
 
   expose :storage_shards_match?, as: :storage_shards_match, if: -> (status, options) do
-    status.storage_shards_match.present?
+    Gitlab::Geo.primary? && status.storage_shards.present?
   end
 
   private

spec/ee/spec/models/geo_node_status_spec.rb

@@ -406,16 +406,41 @@ describe GeoNodeStatus, :geo do
     it 'returns a new GeoNodeStatus excluding parameters' do
       status = create(:geo_node_status)
 
-      data = status.as_json
+      data = GeoNodeStatusSerializer.new.represent(status).as_json
       data['id'] = 10000
-      data['storage_shards'] = Settings.repositories.storages
 
       result = GeoNodeStatus.from_json(data)
 
       expect(result.id).to be_nil
       expect(result.attachments_count).to eq(status.attachments_count)
-      expect(result.cursor_last_event_date).to eq(status.cursor_last_event_date)
+      expect(result.cursor_last_event_date).to eq(Time.at(status.cursor_last_event_timestamp))
       expect(result.storage_shards.count).to eq(Settings.repositories.storages.count)
     end
   end
+
+  describe '#storage_shards_match?' do
+    before do
+      allow(Gitlab::Geo).to receive(:primary?).and_return(true)
+    end
+
+    it 'returns false if the storage shards do not match' do
+      status = create(:geo_node_status)
+      status.storage_shards.first['name'] = 'broken-shard'
+      data = status.as_json
+
+      result = GeoNodeStatus.from_json(data)
+
+      expect(status.storage_shards_match?).to be false
+    end
+
+    it 'returns true if the storage shards match in different order' do
+      status = create(:geo_node_status)
+
+      status.storage_shards.shuffle!
+      data = status.as_json
+      result = GeoNodeStatus.from_json(data)
+
+      expect(status.storage_shards_match?).to be true
+    end
+  end
 end

spec/ee/spec/serializers/geo_node_status_entity_spec.rb

@@ -33,6 +33,7 @@ describe GeoNodeStatusEntity, :postgresql do
   it { is_expected.to have_key(:last_successful_status_check_timestamp) }
   it { is_expected.to have_key(:namespaces) }
   it { is_expected.to have_key(:storage_shards) }
+  it { is_expected.to have_key(:storage_shards_match) }
 
   describe '#healthy' do
     context 'when node is healthy' do

spec/factories/geo_node_statuses.rb

@@ -2,7 +2,7 @@ FactoryBot.define do
   factory :geo_node_status do
     sequence(:id)
     geo_node
-    storage_shards { StorageShard.all }
+    storage_shards { StorageShardSerializer.new.represent(StorageShard.all) }
 
     trait :healthy do
       health nil

spec/fixtures/api/schemas/geo_node_status.json

@@ -63,6 +63,7 @@
     "last_successful_status_check_timestamp": { "type": ["integer", "null"] },
     "namespaces": { "type": "array" },
     "storage_shards": { "type": "array" },
+    "storage_shards_match": { "type": "boolean" },
     "version": { "type": ["string"] },
     "revision": { "type": ["string"] }
   },