Merge branch 'sh-api-shard-config' into 'master'

Check if shard configuration is same across Geo nodes

Closes #3243

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

app/assets/javascripts/geo_nodes.js

@@ -14,7 +14,8 @@ const unhealthyIcon = 'fa-times';
 const unknownIcon = 'fa-times';
 const notAvailable = 'Not Available';
 const versionMismatch = 'Does not match the primary node version';
-const versionMismatchClass = 'geo-node-version-mismatch';
+const nodeMismatchClass = 'geo-node-mismatch';
+const storageMismatch = 'Does not match the primary storage configuration';
 
 class GeoNodeStatus {
   constructor(el) {
@@ -34,6 +35,7 @@ class GeoNodeStatus {
     this.$health = $('.js-health-message', this.$status.parent());
     this.$version = $('.js-gitlab-version', this.$status);
     this.$secondaryVersion = $('.js-secondary-version', this.$status);
+    this.$secondaryStorage = $('.js-secondary-storage-shards', this.$status);
     this.endpoint = this.$el.data('status-url');
     this.$advancedStatus = $('.js-advanced-geo-node-status-toggler', this.$status.parent());
     this.$advancedStatus.on('click', GeoNodeStatus.toggleShowAdvancedStatus.bind(this));
@@ -204,13 +206,23 @@ class GeoNodeStatus {
 
     if (!this.primaryVersion || (this.primaryVersion === status.version
       && this.primaryRevision === status.revision)) {
-      this.$secondaryVersion.removeClass(`${versionMismatchClass}`);
+      this.$secondaryVersion.removeClass(`${nodeMismatchClass}`);
       this.$secondaryVersion.text(`${status.version} (${status.revision})`);
     } else {
-      this.$secondaryVersion.addClass(`${versionMismatchClass}`);
+      this.$secondaryVersion.addClass(`${nodeMismatchClass}`);
       this.$secondaryVersion.text(`${status.version} (${status.revision}) - ${versionMismatch}`);
     }
 
+    if (status.storage_shards_match == null) {
+      this.$secondaryStorage.text('UNKNOWN');
+    } else if (status.storage_shards_match) {
+      this.$secondaryStorage.removeClass(`${nodeMismatchClass}`);
+      this.$secondaryStorage.text('OK');
+    } else {
+      this.$secondaryStorage.addClass(`${nodeMismatchClass}`);
+      this.$secondaryStorage.text(storageMismatch);
+    }
+
     if (status.repositories_count > 0) {
       const repositoriesStats = GeoNodeStatus.getSyncStatistics({
         syncedCount: status.repositories_synced_count,

app/assets/stylesheets/pages/geo_nodes.scss

@@ -127,7 +127,7 @@
   }
 }
 
-.geo-node-version-mismatch {
+.geo-node-mismatch {
   color: $gl-danger;
 }
 

app/models/storage_shard.rb

+# 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.
+class StorageShard
+  include ActiveModel::Model
+
+  attr_accessor :name, :path
+
+  validates :name, presence: true
+  validates :path, presence: true
+
+  # Generates an array of StorageShard objects from the currrent storage
+  # configuration using the gitlab.yml array of key/value pairs:
+  #
+  # {"default"=>{"path"=>"/home/git/repositories", ...}
+  #
+  # The key is the shard name, and the values are the parameters for that shard.
+  def self.all
+    Settings.repositories.storages.map do |name, params|
+      config = params.symbolize_keys.merge(name: name)
+      config.slice!(*allowed_params)
+      StorageShard.new(config)
+    end
+  end
+
+  def self.allowed_params
+    %i(name path).freeze
+  end
+end

changelogs/unreleased-ee/sh-api-shard-config.yml

+---
+title: Check if shard configuration is same across Geo nodes
+merge_request:
+author:
+type: added

ee/app/models/geo_node_status.rb

@@ -4,6 +4,7 @@ class GeoNodeStatus < ActiveRecord::Base
   # Whether we were successful in reaching this node
   attr_accessor :success, :version, :revision
   attr_writer :health_status
+  attr_accessor :storage_shards
 
   # Be sure to keep this consistent with Prometheus naming conventions
   PROMETHEUS_METRICS = {
@@ -49,12 +50,12 @@ class GeoNodeStatus < ActiveRecord::Base
   def self.from_json(json_data)
     json_data.slice!(*allowed_params)
 
-    GeoNodeStatus.new(json_data)
+    GeoNodeStatus.new(HashWithIndifferentAccess.new(json_data))
   end
 
   def self.allowed_params
     excluded_params = %w(id created_at updated_at).freeze
-    extra_params = %w(success health health_status last_event_timestamp cursor_last_event_timestamp version revision).freeze
+    extra_params = %w(success health health_status last_event_timestamp cursor_last_event_timestamp version revision storage_shards).freeze
     self.column_names - excluded_params + extra_params
   end
 
@@ -74,6 +75,7 @@ class GeoNodeStatus < ActiveRecord::Base
     self.lfs_objects_count = lfs_objects_finder.count_lfs_objects
     self.attachments_count = attachments_finder.count_attachments
     self.last_successful_status_check_at = Time.now
+    self.storage_shards = StorageShard.all
 
     if Gitlab::Geo.primary?
       self.replication_slots_count = geo_node.replication_slots_count
@@ -152,12 +154,40 @@ 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?
+
+    shards_match?(current_shards, primary_shards)
+  end
+
   def [](key)
     public_send(key) # rubocop:disable GitlabSecurity/PublicSend
   end
 
   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)
+    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

@@ -59,6 +59,21 @@ class GeoNodeStatusEntity < Grape::Entity
 
   expose :namespaces, using: NamespaceEntity
 
+  # We load GeoNodeStatus data in two ways:
+  #
+  # 1. Directly by asking a Geo node via an API call
+  # 2. Via cached state in the database
+  #
+  # We don't yet cached the state of the shard information in the database, so if
+  # we don't have this information omit from the serialization entirely.
+  expose :storage_shards, using: StorageShardEntity, if: ->(status, options) do
+    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
+
   private
 
   def namespaces

ee/app/serializers/storage_shard_entity.rb

+class StorageShardEntity < Grape::Entity
+  expose :name, :path
+end

ee/app/serializers/storage_shard_serializer.rb

+class StorageShardSerializer < BaseSerializer
+  entity StorageShardEntity
+end

ee/app/views/admin/geo_nodes/index.html.haml

@@ -15,6 +15,7 @@
   %strong exact order
   they appear.
 
+- current_primary = Gitlab::Geo.primary?
 - if @nodes.any?
   .panel.panel-default
     .panel-heading
@@ -46,6 +47,13 @@
                       GitLab version:
                   %td
                     .node-info.prepend-top-5.prepend-left-5.js-secondary-version
+                - if current_primary
+                  %tr
+                    %td
+                      .help-block
+                        Storage config:
+                        %td
+                          .node-info.prepend-top-5.prepend-left-5.js-secondary-storage-shards
                 - if node.enabled?
                   %tr
                     %td

spec/ee/spec/models/geo_node_status_spec.rb

@@ -396,18 +396,49 @@ describe GeoNodeStatus, :geo do
     it_behaves_like 'timestamp parameters', :cursor_last_event_timestamp, :cursor_last_event_date
   end
 
+  describe '#storage_shards' do
+    it "returns the current node's shard config" do
+      expect(subject[:storage_shards].as_json).to eq(StorageShard.all.as_json)
+    end
+  end
+
   describe '#from_json' do
     it 'returns a new GeoNodeStatus excluding parameters' do
       status = create(:geo_node_status)
 
-      data = status.as_json
-      data[:id] = 10000
+      data = GeoNodeStatusSerializer.new.represent(status).as_json
+      data['id'] = 10000
 
-      result = GeoNodeStatus.from_json(data)
+      result = described_class.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 { stub_primary_node }
+
+    it 'returns false if the storage shards do not match' do
+      status = create(:geo_node_status)
+      data = GeoNodeStatusSerializer.new.represent(status).as_json
+      data['storage_shards'].first['name'] = 'broken-shard'
+
+      result = described_class.from_json(data)
+
+      expect(result.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 = GeoNodeStatusSerializer.new.represent(status).as_json
+      result = described_class.from_json(data)
+
+      expect(result.storage_shards_match?).to be true
     end
   end
 end

spec/ee/spec/serializers/geo_node_status_entity_spec.rb

 require 'spec_helper'
 
 describe GeoNodeStatusEntity, :postgresql do
+  include ::EE::GeoHelpers
+
   let(:geo_node_status) { build(:geo_node_status) }
   let(:entity) { described_class.new(geo_node_status, request: double) }
   let(:error) { 'Could not connect to Geo database' }
 
   subject { entity.as_json }
 
+  before { stub_primary_node }
+
   it { is_expected.to have_key(:geo_node_id) }
   it { is_expected.to have_key(:healthy) }
   it { is_expected.to have_key(:health) }
@@ -32,6 +36,8 @@ describe GeoNodeStatusEntity, :postgresql do
   it { is_expected.to have_key(:replication_slots_max_retained_wal_bytes) }
   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
@@ -127,4 +133,21 @@ describe GeoNodeStatusEntity, :postgresql do
       expect(subject[:namespaces].first[:path]).to eq(namespace.path)
     end
   end
+
+  describe '#storage_shards' do
+    it 'returns the config' do
+      shards = StorageShard.all
+
+      expect(subject[:storage_shards].count).to eq(shards.count)
+      expect(subject[:storage_shards].first[:name]).to eq(shards.first.name)
+      expect(subject[:storage_shards].first[:path]).to eq(shards.first.path)
+    end
+  end
+
+  context 'secondary Geo node' do
+    before { stub_secondary_node }
+
+    it { is_expected.to have_key(:storage_shards) }
+    it { is_expected.not_to have_key(:storage_shards_match) }
+  end
 end

spec/ee/support/geo_helpers.rb

@@ -4,5 +4,14 @@ module EE
       allow(::Gitlab::Geo).to receive(:current_node).and_return(node)
       allow(node).to receive(:current?).and_return(true) unless node.nil?
     end
+
+    def stub_primary_node
+      allow(::Gitlab::Geo).to receive(:primary?).and_return(true)
+    end
+
+    def stub_secondary_node
+      allow(::Gitlab::Geo).to receive(:primary?).and_return(false)
+      allow(::Gitlab::Geo).to receive(:secondary?).and_return(true)
+    end
   end
 end

spec/factories/geo_node_statuses.rb

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

spec/fixtures/api/schemas/geo_node_status.json

@@ -64,6 +64,8 @@
     "cursor_last_event_timestamp": { "type": ["integer", "null"] },
     "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"] }
   },

spec/models/storage_shard_spec.rb

+require 'spec_helper'
+
+describe StorageShard do
+  describe '.current_shards' do
+    it 'returns an array of StorageShard objects' do
+      shards = described_class.all
+
+      expect(shards.count).to eq(Settings.repositories.storages.count)
+      expect(shards.map(&:name)).to match_array(Settings.repositories.storages.keys)
+      expect(shards.map(&:path)).to match_array(Settings.repositories.storages.values.map(&:path))
+    end
+  end
+end