Merge branch 'mk/fix-synchronization-disabled' into 'master'

Geo: Fix synchronization disabled on primary UI

Closes #217643

See merge request gitlab-org/gitlab!33760

ee/app/models/geo_node_status.rb

@@ -8,13 +8,10 @@ class GeoNodeStatus < ApplicationRecord
   delegate :selective_sync_type, to: :geo_node
 
   after_initialize :initialize_feature_flags
+  before_save :coerce_status_field_values, if: :status_changed?
 
   attr_accessor :storage_shards
 
-  attr_accessor :attachments_replication_enabled, :lfs_objects_replication_enabled, :job_artifacts_replication_enabled,
-                :container_repositories_replication_enabled, :design_repositories_replication_enabled, :repositories_replication_enabled,
-                :repository_verification_enabled
-
   # Prometheus metrics, no need to store them in the database
   attr_accessor :event_log_max_id, :repository_created_max_id, :repository_updated_max_id,
                 :repository_deleted_max_id, :repository_renamed_max_id, :repositories_changed_max_id,
@@ -35,16 +32,21 @@ class GeoNodeStatus < ApplicationRecord
   alias_attribute :cursor_last_event_timestamp, :cursor_last_event_date_timestamp
 
   RESOURCE_STATUS_FIELDS = %w(
+    repository_verification_enabled
+    repositories_replication_enabled
     repositories_synced_count
     repositories_failed_count
+    lfs_objects_replication_enabled
     lfs_objects_count
     lfs_objects_synced_count
     lfs_objects_failed_count
+    attachments_replication_enabled
     attachments_count
     attachments_synced_count
     attachments_failed_count
     wikis_synced_count
     wikis_failed_count
+    job_artifacts_replication_enabled
     job_artifacts_count
     job_artifacts_synced_count
     job_artifacts_failed_count
@@ -64,10 +66,12 @@ class GeoNodeStatus < ApplicationRecord
     repositories_retrying_verification_count
     wikis_retrying_verification_count
     projects_count
+    container_repositories_replication_enabled
     container_repositories_count
     container_repositories_synced_count
     container_repositories_failed_count
     container_repositories_registry_count
+    design_repositories_replication_enabled
     design_repositories_count
     design_repositories_synced_count
     design_repositories_failed_count
@@ -81,6 +85,7 @@ class GeoNodeStatus < ApplicationRecord
   # Be sure to keep this consistent with Prometheus naming conventions
   PROMETHEUS_METRICS = {
     db_replication_lag_seconds: 'Database replication lag (seconds)',
+    repository_verification_enabled: 'Boolean denoting if verification is enabled for Repositories',
     repositories_replication_enabled: 'Boolean denoting if replication is enabled for Repositories',
     repositories_count: 'Total number of repositories available on primary',
     repositories_synced_count: 'Number of repositories synced on secondary',
@@ -162,25 +167,23 @@ class GeoNodeStatus < ApplicationRecord
   def self.alternative_status_store_accessor(attr_names)
     attr_names.each do |attr_name|
       define_method(attr_name) do
-        status[attr_name].nil? ? read_attribute(attr_name) : status[attr_name].to_i
+        val = status[attr_name]
+
+        # Backwards-compatible line for when the status was written by an
+        # earlier release without the `status` field
+        val ||= read_attribute(attr_name)
+
+        convert_status_value(attr_name, val)
       end
 
       define_method("#{attr_name}=") do |val|
-        status[attr_name] = val.nil? ? nil : val.to_i
+        val = convert_status_value(attr_name, val)
 
-        return unless self.class.column_names.include?(attr_name)
-
-        write_attribute(attr_name, val)
+        status[attr_name] = val
       end
     end
   end
 
-  # We migrated from attributes stored in individual columns to
-  # a single JSONB hash. To create accessors method for every fields in hash we could use
-  # Rails embeded store_accessor but in this case we won't have smooth update procedure.
-  # The method "alternative_status_store_accessor" does actually the same that store_accessor would
-  # but it uses the old fashioned fields in case the new ones are not set yet. We also casting the type into integer when
-  # we set the value to preserve the old behaviour.
   alternative_status_store_accessor RESOURCE_STATUS_FIELDS
 
   def self.current_node_status
@@ -232,14 +235,47 @@ class GeoNodeStatus < ApplicationRecord
     self.column_names - EXCLUDED_PARAMS + EXTRA_PARAMS
   end
 
+  # Helps make alternative_status_store_accessor act more like regular Rails
+  # attributes. Request params values are always strings, but when saved as
+  # attributes of a model, they are converted to the appropriate types. We could
+  # manually map a specified type to each attribute, but for now, the type can
+  # be easily inferred by the attribute name.
+  #
+  # If you add a new status attribute that does not look like existing
+  # attributes, then you'll get an error until you handle it in the cases below.
+  #
+  # @param [String] attr_name the status key
+  # @param [String, Integer, Boolean] val being assigned or retrieved
+  # @return [String, Integer, Boolean] converted value based on attr_name
+  def convert_status_value(attr_name, val)
+    return if val.nil?
+
+    case attr_name
+    when /_count\z/ then val.to_i
+    when /_enabled\z/ then val.to_s == 'true'
+    else raise "Unhandled status attribute name format \"#{attr_name}\""
+    end
+  end
+
+  # Leverages attribute reader methods written by
+  # alternative_status_store_accessor to convert string values to integers and
+  # booleans if necessary.
+  def coerce_status_field_values
+    status_attrs = status.slice(*RESOURCE_STATUS_FIELDS)
+    self.assign_attributes(status_attrs)
+  end
+
   def initialize_feature_flags
-    self.attachments_replication_enabled = Geo::UploadRegistry.replication_enabled?
-    self.container_repositories_replication_enabled = Geo::ContainerRepositoryRegistry.replication_enabled?
-    self.design_repositories_replication_enabled = Geo::DesignRegistry.replication_enabled?
-    self.job_artifacts_replication_enabled = Geo::JobArtifactRegistry.replication_enabled?
-    self.lfs_objects_replication_enabled = Geo::LfsObjectRegistry.replication_enabled?
-    self.repositories_replication_enabled = Geo::ProjectRegistry.replication_enabled?
     self.repository_verification_enabled = Gitlab::Geo.repository_verification_enabled?
+
+    if Gitlab::Geo.secondary?
+      self.attachments_replication_enabled = Geo::UploadRegistry.replication_enabled?
+      self.container_repositories_replication_enabled = Geo::ContainerRepositoryRegistry.replication_enabled?
+      self.design_repositories_replication_enabled = Geo::DesignRegistry.replication_enabled?
+      self.job_artifacts_replication_enabled = Geo::JobArtifactRegistry.replication_enabled?
+      self.lfs_objects_replication_enabled = Geo::LfsObjectRegistry.replication_enabled?
+      self.repositories_replication_enabled = Geo::ProjectRegistry.replication_enabled?
+    end
   end
 
   def update_cache!

ee/app/services/geo/node_status_request_service.rb

@@ -17,7 +17,13 @@ module Geo
     end
 
     def payload(status)
-      status.attributes.except('id')
+      # RESOURCE_STATUS_FIELDS is excluded since that data would be duplicated
+      # in the payload as top-level attributes as well attributes nested in the
+      # new status field. We can remove this exclusion when we remove those
+      # deprecated columns from the geo_node_statuses table.
+      excluded_keys = GeoNodeStatus::RESOURCE_STATUS_FIELDS + ['id']
+
+      status.attributes.except(*excluded_keys)
     end
   end
 end

ee/changelogs/unreleased/mk-fix-synchronization-disabled.yml

+---
+title: 'Geo: Fix synchronization disabled on primary UI'
+merge_request: 33760
+author:
+type: fixed

ee/lib/api/geo.rb

@@ -7,9 +7,13 @@ module API
     resource :geo do
       helpers do
         def sanitized_node_status_params
-          allowed_attributes = GeoNodeStatus.attribute_names - ['id']
-          valid_attributes = params.keys & allowed_attributes
-          params.slice(*valid_attributes)
+          valid_attributes = GeoNodeStatus.attribute_names - GeoNodeStatus::RESOURCE_STATUS_FIELDS - ['id']
+          sanitized_params = params.slice(*valid_attributes)
+
+          # sanitize status field
+          sanitized_params['status'] = sanitized_params['status'].slice(*GeoNodeStatus::RESOURCE_STATUS_FIELDS) if sanitized_params['status']
+
+          sanitized_params
         end
 
         # Check if a Geo request is legit or fail the flow

ee/spec/factories/geo_node_statuses.rb

@@ -20,6 +20,7 @@ FactoryBot.define do
       job_artifacts_synced_count { 577 }
       job_artifacts_synced_missing_on_primary_count { 91 }
       container_repositories_count { 400 }
+      container_repositories_registry_count { 203 }
       container_repositories_failed_count { 3 }
       container_repositories_synced_count { 200 }
       design_repositories_count { 400 }
@@ -55,6 +56,13 @@ FactoryBot.define do
       last_successful_status_check_timestamp { 2.minutes.ago }
       version { Gitlab::VERSION }
       revision { Gitlab.revision }
+      attachments_replication_enabled { true }
+      container_repositories_replication_enabled { false }
+      design_repositories_replication_enabled { true }
+      job_artifacts_replication_enabled { false }
+      lfs_objects_replication_enabled { true }
+      repositories_replication_enabled { true }
+      repository_verification_enabled { true }
     end
 
     trait :unhealthy do

ee/spec/fixtures/api/schemas/public_api/v4/geo_node_status.json

@@ -81,40 +81,40 @@
     "health": { "type": ["string", "null"] },
     "health_status": { "type": "string" },
     "missing_oauth_application": { "type": "boolean" },
-    "attachments_replication_enabled": { "type": "boolean" },
+    "attachments_replication_enabled": { "type": ["boolean", "null"] },
     "attachments_count": { "type": "integer" },
     "attachments_failed_count": { "type": ["integer", "null"] },
     "attachments_synced_count": { "type": ["integer", "null"] },
     "attachments_synced_missing_on_primary_count": { "type": ["integer", "null"] },
     "attachments_synced_in_percentage": { "type": "string" },
     "db_replication_lag_seconds": { "type": ["integer", "null"] },
-    "lfs_objects_replication_enabled": { "type": "boolean" },
+    "lfs_objects_replication_enabled": { "type": ["boolean", "null"] },
     "lfs_objects_count": { "type": "integer" },
     "lfs_objects_failed_count": { "type": ["integer", "null"] },
     "lfs_objects_synced_count": { "type": ["integer", "null"] },
     "lfs_objects_synced_missing_on_primary_count": { "type": ["integer", "null"] },
     "lfs_objects_synced_in_percentage": { "type": "string" },
-    "job_artifacts_replication_enabled": { "type": "boolean" },
+    "job_artifacts_replication_enabled": { "type": ["boolean", "null"] },
     "job_artifacts_count": { "type": "integer" },
     "job_artifacts_failed_count": { "type": ["integer", "null"] },
     "job_artifacts_synced_count": { "type": ["integer", "null"] },
     "job_artifacts_synced_missing_on_primary_count": { "type": ["integer", "null"] },
     "job_artifacts_synced_in_percentage": { "type": "string" },
-    "container_repositories_replication_enabled": { "type": "boolean" },
+    "container_repositories_replication_enabled": { "type": ["boolean", "null"] },
     "container_repositories_count": { "type": "integer" },
     "container_repositories_failed_count": { "type": ["integer", "null"] },
     "container_repositories_synced_count": { "type": ["integer", "null"] },
     "container_repositories_synced_in_percentage": { "type": "string" },
     "container_repositories_registry_count": { "type": ["integer", "null"] },
-    "design_repositories_replication_enabled": { "type": "boolean" },
+    "design_repositories_replication_enabled": { "type": ["boolean", "null"] },
     "design_repositories_count": { "type": "integer" },
     "design_repositories_failed_count": { "type": ["integer", "null"] },
     "design_repositories_synced_count": { "type": ["integer", "null"] },
     "design_repositories_synced_in_percentage": { "type": "string" },
-    "repositories_replication_enabled": { "type": "boolean" },
+    "repositories_replication_enabled": { "type": ["boolean", "null"] },
     "projects_count": { "type": "integer" },
     "repositories_failed_count": { "type": ["integer", "null"] },
-    "repository_verification_enabled": { "type": "boolean" },
+    "repository_verification_enabled": { "type": ["boolean", "null"] },
     "repositories_synced_count": { "type": ["integer", "null"] },
     "repositories_synced_in_percentage": { "type": "string" },
     "wikis_failed_count": { "type": ["integer", "null"] },

ee/spec/models/geo_node_status_spec.rb

@@ -34,13 +34,14 @@ RSpec.describe GeoNodeStatus, :geo, :geo_fdw do
 
   describe '#update_cache!' do
     it 'writes a cache' do
+      status = described_class.new
+
       rails_cache = double
       allow(Rails).to receive(:cache).and_return(rails_cache)
-      allow(rails_cache).to receive(:fetch).with('flipper:persisted_names', expires_in: 1.minute).and_return([described_class.cache_key])
 
       expect(rails_cache).to receive(:write).with(described_class.cache_key, kind_of(Hash))
 
-      described_class.new.update_cache!
+      status.update_cache!
     end
   end
 
@@ -1326,18 +1327,17 @@ RSpec.describe GeoNodeStatus, :geo, :geo_fdw do
 
     context 'backward compatibility when counters stored in separate columns' do
       describe '#projects_count' do
-        it 'counts the number of projects' do
+        it 'returns data from the deprecated field if it is not defined in the status field' do
           subject.write_attribute(:projects_count, 10)
           subject.status = {}
 
           expect(subject.projects_count).to eq 10
         end
 
-        it 'sets data in both ways, deprecated and the new one' do
+        it 'sets data in the new status field' do
           subject.projects_count = 10
 
           expect(subject.projects_count).to eq 10
-          expect(subject.read_attribute(:projects_count)).to eq 10
         end
 
         it 'uses column counters when calculates percents using attr_in_percentage' do
@@ -1357,5 +1357,13 @@ RSpec.describe GeoNodeStatus, :geo, :geo_fdw do
         expect(subject.projects_count).to eq 10
       end
     end
+
+    context 'status booleans are converted into booleans' do
+      it 'returns boolean value' do
+        subject.status = { "repositories_replication_enabled" => "true" }
+
+        expect(subject.repositories_replication_enabled).to eq true
+      end
+    end
   end
 end

ee/spec/requests/api/geo_spec.rb

@@ -336,29 +336,6 @@ RSpec.describe API::Geo do
         geo_node_id: secondary_node.id,
         status_message: nil,
         db_replication_lag_seconds: 0,
-        projects_count: 10,
-        repositories_synced_count: 1,
-        repositories_failed_count: 2,
-        wikis_synced_count: 2,
-        wikis_failed_count: 3,
-        lfs_objects_count: 100,
-        lfs_objects_synced_count: 50,
-        lfs_objects_failed_count: 12,
-        lfs_objects_synced_missing_on_primary_count: 4,
-        job_artifacts_count: 100,
-        job_artifacts_synced_count: 50,
-        job_artifacts_failed_count: 12,
-        job_artifacts_synced_missing_on_primary_count: 5,
-        container_repositories_count: 100,
-        container_repositories_synced_count: 50,
-        container_repositories_failed_count: 12,
-        design_repositories_count: 100,
-        design_repositories_synced_count: 50,
-        design_repositories_failed_count: 12,
-        attachments_count: 30,
-        attachments_synced_count: 30,
-        attachments_failed_count: 25,
-        attachments_synced_missing_on_primary_count: 6,
         last_event_id: 2,
         last_event_date: Time.now.utc,
         cursor_last_event_id: 1,
@@ -368,7 +345,39 @@ RSpec.describe API::Geo do
         repository_updated_max_id: 132,
         repository_deleted_max_id: 23,
         repository_renamed_max_id: 11,
-        repositories_changed_max_id: 109
+        repositories_changed_max_id: 109,
+        status: {
+          projects_count: 10,
+          repositories_synced_count: 1,
+          repositories_failed_count: 2,
+          wikis_synced_count: 2,
+          wikis_failed_count: 3,
+          lfs_objects_count: 100,
+          lfs_objects_synced_count: 50,
+          lfs_objects_failed_count: 12,
+          lfs_objects_synced_missing_on_primary_count: 4,
+          job_artifacts_count: 100,
+          job_artifacts_synced_count: 50,
+          job_artifacts_failed_count: 12,
+          job_artifacts_synced_missing_on_primary_count: 5,
+          container_repositories_count: 100,
+          container_repositories_synced_count: 50,
+          container_repositories_failed_count: 12,
+          design_repositories_count: 100,
+          design_repositories_synced_count: 50,
+          design_repositories_failed_count: 12,
+          attachments_count: 30,
+          attachments_synced_count: 30,
+          attachments_failed_count: 25,
+          attachments_synced_missing_on_primary_count: 6,
+          attachments_replication_enabled: false,
+          container_repositories_replication_enabled: true,
+          design_repositories_replication_enabled: false,
+          job_artifacts_replication_enabled: true,
+          lfs_objects_replication_enabled: true,
+          repositories_replication_enabled: true,
+          repository_verification_enabled: true
+        }
       }
     end
 

ee/spec/services/geo/node_status_request_service_spec.rb

@@ -53,5 +53,19 @@ RSpec.describe Geo::NodeStatusRequestService, :geo do
         projects_count: 10
       }))
     end
+
+    it 'sends all of the data in the status JSONB field in the request' do
+      expect(Gitlab::HTTP).to receive(:perform_request)
+                                .with(
+                                  Net::HTTP::Post,
+                                  primary.status_url,
+                                  hash_including(
+                                    body: hash_including(
+                                      'status' => hash_including(
+                                        *GeoNodeStatus::RESOURCE_STATUS_FIELDS))))
+                                .and_return(double(success?: true))
+
+      subject.execute(create(:geo_node_status, :healthy))
+    end
   end
 end