From 3bd7723b017f1961d50b7c22d57ee39608887c53 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Wed, 29 Jun 2022 16:11:15 -0500 Subject: [PATCH 1/9] Adding Geo support of Project-level Secure Files Project-level Secure Files now support Geo replication Changelog: changed MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91430 EE: true --- app/models/ci/secure_file.rb | 2 + .../000_inflections.rb | 1 + db/docs/ci_secure_file_states.yml | 2 +- ee/app/models/ee/ci/secure_file.rb | 88 +++++++++++++++++++ ee/app/models/geo/ci_secure_file_registry.rb | 13 +++ ee/app/models/geo/secure_file_state.rb | 14 +++ .../geo/ci_secure_file_replicator.rb | 25 ++++++ .../secondary/registry_consistency_worker.rb | 3 +- .../geo_ci_secure_file_replication.yml | 8 ++ .../geo_ci_secure_file_verification.yml | 8 ++ ee/lib/gitlab/geo.rb | 3 +- .../factories/geo/ci_secure_file_registry.rb | 32 +++++++ .../factories/geo/ci_secure_file_states.rb | 15 ++++ ee/spec/models/ee/ci/secure_file_spec.rb | 20 +++++ .../geo/ci_secure_file_registry_spec.rb | 14 +++ .../geo/ci_secure_file_replicator_spec.rb | 12 +++ .../geo/registry_consistency_service_spec.rb | 3 +- .../registry_consistency_worker_spec.rb | 3 + spec/factories/ci/secure_files.rb | 12 +++ 19 files changed, 274 insertions(+), 4 deletions(-) create mode 100644 ee/app/models/ee/ci/secure_file.rb create mode 100644 ee/app/models/geo/ci_secure_file_registry.rb create mode 100644 ee/app/models/geo/secure_file_state.rb create mode 100644 ee/app/replicators/geo/ci_secure_file_replicator.rb create mode 100644 ee/config/feature_flags/development/geo_ci_secure_file_replication.yml create mode 100644 ee/config/feature_flags/development/geo_ci_secure_file_verification.yml create mode 100644 ee/spec/factories/geo/ci_secure_file_registry.rb create mode 100644 ee/spec/factories/geo/ci_secure_file_states.rb create mode 100644 ee/spec/models/ee/ci/secure_file_spec.rb create mode 100644 ee/spec/models/geo/ci_secure_file_registry_spec.rb create mode 100644 ee/spec/replicators/geo/ci_secure_file_replicator_spec.rb diff --git a/app/models/ci/secure_file.rb b/app/models/ci/secure_file.rb index 078b05ff779185..c90e1470f3634b 100644 --- a/app/models/ci/secure_file.rb +++ b/app/models/ci/secure_file.rb @@ -46,3 +46,5 @@ def generate_key_data end end end + +Ci::SecureFile.prepend_mod_with('Ci::SecureFile') diff --git a/config/initializers_before_autoloader/000_inflections.rb b/config/initializers_before_autoloader/000_inflections.rb index 64686bdd962005..70c9ec0a0bacf5 100644 --- a/config/initializers_before_autoloader/000_inflections.rb +++ b/config/initializers_before_autoloader/000_inflections.rb @@ -15,6 +15,7 @@ inflect.uncountable %w( custom_emoji award_emoji + ci_secure_file_registry container_repository_registry design_registry event_log diff --git a/db/docs/ci_secure_file_states.yml b/db/docs/ci_secure_file_states.yml index 5e8a748e52ac40..c39c4bd844b4fe 100644 --- a/db/docs/ci_secure_file_states.yml +++ b/db/docs/ci_secure_file_states.yml @@ -1,7 +1,7 @@ --- table_name: ci_secure_file_states classes: -- Geo::CiSecureFileState +- Geo::SecureFileState feature_categories: - pipeline_authoring description: Stores verification state for Geo replicated Project-level Secure Files. diff --git a/ee/app/models/ee/ci/secure_file.rb b/ee/app/models/ee/ci/secure_file.rb new file mode 100644 index 00000000000000..3290293588ee51 --- /dev/null +++ b/ee/app/models/ee/ci/secure_file.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +module EE + # CI::SecureFile EE mixin + # + # This module is intended to encapsulate EE-specific model logic + # and be prepended in the `Ci::SecureFile` model + module Ci + module SecureFile + extend ActiveSupport::Concern + + prepended do + include ::Geo::ReplicableModel + include ::Geo::VerifiableModel + + delegate(*::Geo::VerificationState::VERIFICATION_METHODS, to: :ci_secure_file_state) + + with_replicator Geo::CiSecureFileReplicator + + has_one :ci_secure_file_state, autosave: false, inverse_of: :ci_secure_file, + class_name: 'Geo::SecureFileState', foreign_key: :ci_secure_file_id + + after_save :save_verification_details + + scope :with_verification_state, ->(state) { + joins(:ci_secure_file_state).where( + ci_secure_file_states: { + verification_state: verification_state_value(state) + } + ) + } + scope :checksummed, -> { + joins(:ci_secure_file_state).where.not( + ci_secure_file_states: { verification_checksum: nil } + ) + } + scope :not_checksummed, -> { + joins(:ci_secure_file_state).where( + ci_secure_file_states: { verification_checksum: nil } + ) + } + + scope :available_verifiables, -> { joins(:ci_secure_file_state) } + + def verification_state_object + ci_secure_file_state + end + end + + class_methods do + extend ::Gitlab::Utils::Override + + # @param primary_key_in [Range, CiSecureFile] arg to pass to primary_key_in scope + # @return [ActiveRecord::Relation] everything that should be synced + # to this node, restricted by primary key + def replicables_for_current_secondary(primary_key_in) + # This issue template does not help you write this method. + # + # This method is called only on Geo secondary sites. It is called when + # we want to know which records to replicate. This is not easy to automate + # because for example: + # + # * The "selective sync" feature allows admins to choose which namespaces + # to replicate, per secondary site. Most Models are scoped to a + # namespace, but the nature of the relationship to a namespace varies + # between Models. + # * The "selective sync" feature allows admins to choose which shards to + # replicate, per secondary site. Repositories are associated with + # shards. Most blob types are not, but Project Uploads are. + # * Remote stored replicables are not replicated, by default. But the + # setting `sync_object_storage` enables replication of remote stored + # replicables. + # + # Search the codebase for examples, and consult a Geo expert if needed. + end + + override :verification_state_table_class + def verification_state_table_class + ::Geo::SecureFileState + end + end + + def ci_secure_file_state + super || build_ci_secure_file_state + end + end + end +end diff --git a/ee/app/models/geo/ci_secure_file_registry.rb b/ee/app/models/geo/ci_secure_file_registry.rb new file mode 100644 index 00000000000000..6d4e4255ed9768 --- /dev/null +++ b/ee/app/models/geo/ci_secure_file_registry.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Geo + class CiSecureFileRegistry < Geo::BaseRegistry + include ::Geo::ReplicableRegistry + include ::Geo::VerifiableRegistry + + MODEL_CLASS = ::Ci::SecureFile + MODEL_FOREIGN_KEY = :ci_secure_file_id + + belongs_to :ci_secure_file, class_name: 'Ci::SecureFile' + end +end diff --git a/ee/app/models/geo/secure_file_state.rb b/ee/app/models/geo/secure_file_state.rb new file mode 100644 index 00000000000000..0707a6fdddc9f4 --- /dev/null +++ b/ee/app/models/geo/secure_file_state.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true +module Geo + class SecureFileState < Ci::ApplicationRecord + include EachBatch + include ::Geo::VerificationStateDefinition + + self.primary_key = :ci_secure_file_id + + belongs_to :ci_secure_file, inverse_of: :ci_secure_file_state, class_name: 'Ci::SecureFile' + + validates :verification_failure, length: { maximum: 255 } + validates :verification_state, :ci_secure_file, presence: true + end +end diff --git a/ee/app/replicators/geo/ci_secure_file_replicator.rb b/ee/app/replicators/geo/ci_secure_file_replicator.rb new file mode 100644 index 00000000000000..b13e22f0e8aae6 --- /dev/null +++ b/ee/app/replicators/geo/ci_secure_file_replicator.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Geo + class CiSecureFileReplicator < Gitlab::Geo::Replicator + include ::Geo::BlobReplicatorStrategy + extend ::Gitlab::Utils::Override + + def self.model + ::Ci::SecureFile + end + + def carrierwave_uploader + model_record.file + end + + override :verification_feature_flag_enabled? + def self.verification_feature_flag_enabled? + # We are adding verification at the same time as replication, so we + # don't need to toggle verification separately from replication. When + # the replication feature flag is off, then verification is also off + # (see `VerifiableReplicator.verification_enabled?`) + true + end + end +end diff --git a/ee/app/workers/geo/secondary/registry_consistency_worker.rb b/ee/app/workers/geo/secondary/registry_consistency_worker.rb index 10c908caa2c762..3eebaf2f74c252 100644 --- a/ee/app/workers/geo/secondary/registry_consistency_worker.rb +++ b/ee/app/workers/geo/secondary/registry_consistency_worker.rb @@ -31,7 +31,8 @@ class RegistryConsistencyWorker Geo::UploadRegistry, Geo::SnippetRepositoryRegistry, Geo::GroupWikiRepositoryRegistry, - Geo::PagesDeploymentRegistry + Geo::PagesDeploymentRegistry, + Geo::CiSecureFileRegistry ].freeze BATCH_SIZE = 10000 diff --git a/ee/config/feature_flags/development/geo_ci_secure_file_replication.yml b/ee/config/feature_flags/development/geo_ci_secure_file_replication.yml new file mode 100644 index 00000000000000..ae2d2013c3cec8 --- /dev/null +++ b/ee/config/feature_flags/development/geo_ci_secure_file_replication.yml @@ -0,0 +1,8 @@ +--- +name: geo_ci_secure_file_replication +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91430 +rollout_issue_url: +milestone: '15.2' +type: development +group: group::geo +default_enabled: false diff --git a/ee/config/feature_flags/development/geo_ci_secure_file_verification.yml b/ee/config/feature_flags/development/geo_ci_secure_file_verification.yml new file mode 100644 index 00000000000000..bd870319ce9fd6 --- /dev/null +++ b/ee/config/feature_flags/development/geo_ci_secure_file_verification.yml @@ -0,0 +1,8 @@ +--- +name: geo_ci_secure_file_verification +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91430 +rollout_issue_url: +milestone: '15.2' +type: development +group: group::geo +default_enabled: false diff --git a/ee/lib/gitlab/geo.rb b/ee/lib/gitlab/geo.rb index eb0485d17fa41e..7de3591fbbf1ca 100644 --- a/ee/lib/gitlab/geo.rb +++ b/ee/lib/gitlab/geo.rb @@ -32,7 +32,8 @@ module Geo ::Geo::PipelineArtifactReplicator, ::Geo::PagesDeploymentReplicator, ::Geo::UploadReplicator, - ::Geo::JobArtifactReplicator + ::Geo::JobArtifactReplicator, + ::Geo::CiSecureFileReplicator ].freeze # We "regenerate" an 1hour valid JWT every 30 minutes, resulting in diff --git a/ee/spec/factories/geo/ci_secure_file_registry.rb b/ee/spec/factories/geo/ci_secure_file_registry.rb new file mode 100644 index 00000000000000..a17445b7f06cb2 --- /dev/null +++ b/ee/spec/factories/geo/ci_secure_file_registry.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :geo_ci_secure_file_registry, class: 'Geo::CiSecureFileRegistry' do + ci_secure_file # This association should have data, like a file or repository + state { Geo::CiSecureFileRegistry.state_value(:pending) } + + trait :synced do + state { Geo::CiSecureFileRegistry.state_value(:synced) } + last_synced_at { 5.days.ago } + end + + trait :failed do + state { Geo::CiSecureFileRegistry.state_value(:failed) } + last_synced_at { 1.day.ago } + retry_count { 2 } + last_sync_failure { 'Random error' } + end + + trait :started do + state { Geo::CiSecureFileRegistry.state_value(:started) } + last_synced_at { 1.day.ago } + retry_count { 0 } + end + + trait :verification_succeeded do + verification_checksum { 'e079a831cab27bcda7d81cd9b48296d0c3dd92ef' } + verification_state { Geo::CiSecureFileRegistry.verification_state_value(:verification_succeeded) } + verified_at { 5.days.ago } + end + end +end diff --git a/ee/spec/factories/geo/ci_secure_file_states.rb b/ee/spec/factories/geo/ci_secure_file_states.rb new file mode 100644 index 00000000000000..6d8359c1ea7776 --- /dev/null +++ b/ee/spec/factories/geo/ci_secure_file_states.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :geo_ci_secure_file_state, class: 'Geo::SecureFileState' do + ci_secure_file + + trait(:checksummed) do + verification_checksum { 'abc' } + end + + trait(:checksum_failure) do + verification_failure { 'Could not calculate the checksum' } + end + end +end diff --git a/ee/spec/models/ee/ci/secure_file_spec.rb b/ee/spec/models/ee/ci/secure_file_spec.rb new file mode 100644 index 00000000000000..ecc5f51fb22bc2 --- /dev/null +++ b/ee/spec/models/ee/ci/secure_file_spec.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::SecureFile do + using RSpec::Parameterized::TableSyntax + include EE::GeoHelpers + + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + + include_examples 'a replicable model with a separate table for verification state' do + before do + stub_ci_secure_file_object_storage + end + + let(:verifiable_model_record) { build(:ci_secure_file, project: project) } + let(:unverifiable_model_record) { build(:ci_secure_file, project: project) } + end +end diff --git a/ee/spec/models/geo/ci_secure_file_registry_spec.rb b/ee/spec/models/geo/ci_secure_file_registry_spec.rb new file mode 100644 index 00000000000000..1c31948f25add9 --- /dev/null +++ b/ee/spec/models/geo/ci_secure_file_registry_spec.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Geo::CiSecureFileRegistry, :geo, type: :model do + let_it_be(:registry) { create(:geo_ci_secure_file_registry) } + + specify 'factory is valid' do + expect(registry).to be_valid + end + + include_examples 'a Geo framework registry' + include_examples 'a Geo verifiable registry' +end diff --git a/ee/spec/replicators/geo/ci_secure_file_replicator_spec.rb b/ee/spec/replicators/geo/ci_secure_file_replicator_spec.rb new file mode 100644 index 00000000000000..1a354d7b447380 --- /dev/null +++ b/ee/spec/replicators/geo/ci_secure_file_replicator_spec.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Geo::CiSecureFileReplicator do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:model_record) { build(:ci_secure_file, project: project) } + + include_examples 'a blob replicator' + include_examples 'a verifiable replicator' +end diff --git a/ee/spec/services/geo/registry_consistency_service_spec.rb b/ee/spec/services/geo/registry_consistency_service_spec.rb index d182ee2bb1d625..187ed26946c572 100644 --- a/ee/spec/services/geo/registry_consistency_service_spec.rb +++ b/ee/spec/services/geo/registry_consistency_service_spec.rb @@ -21,7 +21,8 @@ def model_class_factory_name(registry_class) Geo::MergeRequestDiffRegistry => :external_merge_request_diff, Geo::PackageFileRegistry => :package_file, Geo::UploadRegistry => :upload, - Geo::JobArtifactRegistry => :ci_job_artifact + Geo::JobArtifactRegistry => :ci_job_artifact, + Geo::CiSecureFileRegistry => :ci_secure_file }.fetch(registry_class, default_factory_name) end diff --git a/ee/spec/workers/geo/secondary/registry_consistency_worker_spec.rb b/ee/spec/workers/geo/secondary/registry_consistency_worker_spec.rb index 94db718fda30c3..22ca629676c085 100644 --- a/ee/spec/workers/geo/secondary/registry_consistency_worker_spec.rb +++ b/ee/spec/workers/geo/secondary/registry_consistency_worker_spec.rb @@ -88,6 +88,7 @@ pipeline_artifact = create(:ci_pipeline_artifact) upload = create(:upload) pages_deployment = create(:pages_deployment) + ci_secure_file = create(:ci_secure_file) expect(Geo::ContainerRepositoryRegistry.where(container_repository_id: container_repository.id).count).to eq(0) expect(Geo::DesignRegistry.where(project_id: project.id).count).to eq(0) @@ -101,6 +102,7 @@ expect(Geo::UploadRegistry.where(file_id: upload.id).count).to eq(0) expect(Geo::PagesDeploymentRegistry.where(pages_deployment: pages_deployment.id).count).to eq(0) expect(Geo::JobArtifactRegistry.where(job_artifact: job_artifact.id).count).to eq(0) + expect(Geo::CiSecureFileRegistry.where(ci_secure_file: ci_secure_file.id).count).to eq(0) subject.perform @@ -116,6 +118,7 @@ expect(Geo::UploadRegistry.where(file_id: upload.id).count).to eq(1) expect(Geo::PagesDeploymentRegistry.where(pages_deployment: pages_deployment.id).count).to eq(1) expect(Geo::JobArtifactRegistry.where(job_artifact: job_artifact.id).count).to eq(1) + expect(Geo::CiSecureFileRegistry.where(ci_secure_file: ci_secure_file.id).count).to eq(1) end context 'when the current Geo node is disabled or primary' do diff --git a/spec/factories/ci/secure_files.rb b/spec/factories/ci/secure_files.rb index 9afec5db85879a..76875cfc163c32 100644 --- a/spec/factories/ci/secure_files.rb +++ b/spec/factories/ci/secure_files.rb @@ -6,5 +6,17 @@ file { fixture_file_upload('spec/fixtures/ci_secure_files/upload-keystore.jks', 'application/octet-stream') } checksum { 'foo1234' } project + + trait(:verification_succeeded) do + with_file + verification_checksum { 'abc' } + verification_state { Ci::SecureFile.verification_state_value(:verification_succeeded) } + end + + trait(:verification_failed) do + with_file + verification_failure { 'Could not calculate the checksum' } + verification_state { Ci::SecureFile.verification_state_value(:verification_failed) } + end end end -- GitLab From 63608bc3d696e06b4386f35c2d7dbe60cf210e08 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Thu, 14 Jul 2022 17:31:33 -0500 Subject: [PATCH 2/9] Adding code and tests for replicables_for_current_secondary --- app/models/ci/secure_file.rb | 1 + ee/app/models/ee/ci/secure_file.rb | 27 +------ .../geo_ci_secure_file_verification.yml | 8 -- ee/spec/factories/ci/secure_files.rb | 17 ++++ ee/spec/models/ee/ci/secure_file_spec.rb | 79 ++++++++++++++++++- .../geo/ci_secure_file_replicator_spec.rb | 5 +- spec/factories/ci/secure_files.rb | 13 +-- 7 files changed, 104 insertions(+), 46 deletions(-) delete mode 100644 ee/config/feature_flags/development/geo_ci_secure_file_verification.yml create mode 100644 ee/spec/factories/ci/secure_files.rb diff --git a/app/models/ci/secure_file.rb b/app/models/ci/secure_file.rb index c90e1470f3634b..30d2aca2f4fa1e 100644 --- a/app/models/ci/secure_file.rb +++ b/app/models/ci/secure_file.rb @@ -24,6 +24,7 @@ class SecureFile < Ci::ApplicationRecord before_validation :assign_checksum scope :order_by_created_at, -> { order(created_at: :desc) } + scope :project_id_in, ->(ids) { where(project_id: ids) } default_value_for(:file_store) { Ci::SecureFileUploader.default_store } diff --git a/ee/app/models/ee/ci/secure_file.rb b/ee/app/models/ee/ci/secure_file.rb index 3290293588ee51..74351bca90ed84 100644 --- a/ee/app/models/ee/ci/secure_file.rb +++ b/ee/app/models/ee/ci/secure_file.rb @@ -12,6 +12,7 @@ module SecureFile prepended do include ::Geo::ReplicableModel include ::Geo::VerifiableModel + include Artifactable delegate(*::Geo::VerificationState::VERIFICATION_METHODS, to: :ci_secure_file_state) @@ -41,6 +42,8 @@ module SecureFile } scope :available_verifiables, -> { joins(:ci_secure_file_state) } + scope :with_files_stored_locally, -> { where(file_store: ::ObjectStorage::Store::LOCAL) } + scope :with_files_stored_remotely, -> { where(file_store: ::ObjectStorage::Store::REMOTE) } def verification_state_object ci_secure_file_state @@ -50,30 +53,6 @@ def verification_state_object class_methods do extend ::Gitlab::Utils::Override - # @param primary_key_in [Range, CiSecureFile] arg to pass to primary_key_in scope - # @return [ActiveRecord::Relation] everything that should be synced - # to this node, restricted by primary key - def replicables_for_current_secondary(primary_key_in) - # This issue template does not help you write this method. - # - # This method is called only on Geo secondary sites. It is called when - # we want to know which records to replicate. This is not easy to automate - # because for example: - # - # * The "selective sync" feature allows admins to choose which namespaces - # to replicate, per secondary site. Most Models are scoped to a - # namespace, but the nature of the relationship to a namespace varies - # between Models. - # * The "selective sync" feature allows admins to choose which shards to - # replicate, per secondary site. Repositories are associated with - # shards. Most blob types are not, but Project Uploads are. - # * Remote stored replicables are not replicated, by default. But the - # setting `sync_object_storage` enables replication of remote stored - # replicables. - # - # Search the codebase for examples, and consult a Geo expert if needed. - end - override :verification_state_table_class def verification_state_table_class ::Geo::SecureFileState diff --git a/ee/config/feature_flags/development/geo_ci_secure_file_verification.yml b/ee/config/feature_flags/development/geo_ci_secure_file_verification.yml deleted file mode 100644 index bd870319ce9fd6..00000000000000 --- a/ee/config/feature_flags/development/geo_ci_secure_file_verification.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: geo_ci_secure_file_verification -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91430 -rollout_issue_url: -milestone: '15.2' -type: development -group: group::geo -default_enabled: false diff --git a/ee/spec/factories/ci/secure_files.rb b/ee/spec/factories/ci/secure_files.rb new file mode 100644 index 00000000000000..629350e3547056 --- /dev/null +++ b/ee/spec/factories/ci/secure_files.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :ee_ci_secure_file, class: '::Ci::SecureFile', parent: :ci_secure_file do + trait(:verification_succeeded) do + with_file + verification_checksum { 'abc' } + verification_state { Ci::SecureFile.verification_state_value(:verification_succeeded) } + end + + trait(:verification_failed) do + with_file + verification_failure { 'Could not calculate the checksum' } + verification_state { Ci::SecureFile.verification_state_value(:verification_failed) } + end + end +end diff --git a/ee/spec/models/ee/ci/secure_file_spec.rb b/ee/spec/models/ee/ci/secure_file_spec.rb index ecc5f51fb22bc2..eacfc5f51bd110 100644 --- a/ee/spec/models/ee/ci/secure_file_spec.rb +++ b/ee/spec/models/ee/ci/secure_file_spec.rb @@ -11,10 +11,87 @@ include_examples 'a replicable model with a separate table for verification state' do before do - stub_ci_secure_file_object_storage + stub_ci_secure_file_object_storage(enabled: false) end let(:verifiable_model_record) { build(:ci_secure_file, project: project) } let(:unverifiable_model_record) { build(:ci_secure_file, project: project) } end + + describe '#replicables_for_current_secondary' do + # Selective sync is configured relative to the secure file's project. + # + # Permutations of sync_object_storage combined with object-stored-artifacts + # are tested in code, because the logic is simple, and to do it in the table + # would quadruple its size and have too much duplication. + where(:selective_sync_namespaces, :selective_sync_shards, :factory, :project_factory, :include_expectation) do + nil | nil | [:ci_secure_file] | [:project] | true + # selective sync by shard + nil | :model | [:ci_secure_file] | [:project] | true + nil | :other | [:ci_secure_file] | [:project] | false + # selective sync by namespace + :model_parent | nil | [:ci_secure_file] | [:project] | true + :model_parent_parent | nil | [:ci_secure_file] | [:project, :in_subgroup] | true + :other | nil | [:ci_secure_file] | [:project] | false + :other | nil | [:ci_secure_file] | [:project, :in_subgroup] | false + end + + with_them do + subject(:ci_secure_file_included) { described_class.replicables_for_current_secondary(ci_secure_file).exists? } + + let(:project) { create(*project_factory) } # rubocop:disable Rails/SaveBang + let(:node) do + create(:geo_node_with_selective_sync_for, + model: project, + namespaces: selective_sync_namespaces, + shards: selective_sync_shards, + sync_object_storage: sync_object_storage) + end + + before do + stub_ci_secure_file_object_storage + stub_current_geo_node(node) + end + + context 'when sync object storage is enabled' do + let(:sync_object_storage) { true } + + context 'when the ci secure file is locally stored' do + before do + stub_ci_secure_file_object_storage(enabled: false) + end + + let(:ci_secure_file) { create(*factory, project: project) } + + it { is_expected.to eq(include_expectation) } + end + + context 'when the ci secure file is object stored' do + let(:ci_secure_file) { create(*factory, :remote_store, project: project) } + + it { is_expected.to eq(include_expectation) } + end + end + + context 'when sync object storage is disabled' do + let(:sync_object_storage) { false } + + context 'when the ci secure file is locally stored' do + before do + stub_ci_secure_file_object_storage(enabled: false) + end + + let(:ci_secure_file) { create(*factory, project: project) } + + it { is_expected.to eq(include_expectation) } + end + + context 'when the ci secure file is object stored' do + let(:ci_secure_file) { create(*factory, :remote_store, project: project) } + + it { is_expected.to be_falsey } + end + end + end + end end diff --git a/ee/spec/replicators/geo/ci_secure_file_replicator_spec.rb b/ee/spec/replicators/geo/ci_secure_file_replicator_spec.rb index 1a354d7b447380..15fcefdfe2ff8c 100644 --- a/ee/spec/replicators/geo/ci_secure_file_replicator_spec.rb +++ b/ee/spec/replicators/geo/ci_secure_file_replicator_spec.rb @@ -3,9 +3,8 @@ require 'spec_helper' RSpec.describe Geo::CiSecureFileReplicator do - let_it_be(:group) { create(:group) } - let_it_be(:project) { create(:project, group: group) } - let_it_be(:model_record) { build(:ci_secure_file, project: project) } + let(:project) { create(:project) } + let(:model_record) { create(:ci_secure_file, project: project) } include_examples 'a blob replicator' include_examples 'a verifiable replicator' diff --git a/spec/factories/ci/secure_files.rb b/spec/factories/ci/secure_files.rb index 76875cfc163c32..1dd33bb4260a04 100644 --- a/spec/factories/ci/secure_files.rb +++ b/spec/factories/ci/secure_files.rb @@ -4,19 +4,12 @@ factory :ci_secure_file, class: 'Ci::SecureFile' do sequence(:name) { |n| "file#{n}" } file { fixture_file_upload('spec/fixtures/ci_secure_files/upload-keystore.jks', 'application/octet-stream') } + file_store { ::ObjectStorage::Store::LOCAL } checksum { 'foo1234' } project - trait(:verification_succeeded) do - with_file - verification_checksum { 'abc' } - verification_state { Ci::SecureFile.verification_state_value(:verification_succeeded) } - end - - trait(:verification_failed) do - with_file - verification_failure { 'Could not calculate the checksum' } - verification_state { Ci::SecureFile.verification_state_value(:verification_failed) } + trait :remote_store do + file_store { ::ObjectStorage::Store::REMOTE } end end end -- GitLab From 4c70e6d34e70008efb7956a268b518cd685bcc07 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Fri, 15 Jul 2022 12:01:09 -0500 Subject: [PATCH 3/9] Renaming SecureFileState back to CiSecureFileState --- db/docs/ci_secure_file_states.yml | 2 +- ee/app/models/ee/ci/secure_file.rb | 4 ++-- .../{secure_file_state.rb => ci_secure_file_state.rb} | 3 ++- ee/spec/factories/geo/ci_secure_file_states.rb | 2 +- ee/spec/models/ee/ci/secure_file_spec.rb | 9 ++++----- 5 files changed, 10 insertions(+), 10 deletions(-) rename ee/app/models/geo/{secure_file_state.rb => ci_secure_file_state.rb} (80%) diff --git a/db/docs/ci_secure_file_states.yml b/db/docs/ci_secure_file_states.yml index c39c4bd844b4fe..5e8a748e52ac40 100644 --- a/db/docs/ci_secure_file_states.yml +++ b/db/docs/ci_secure_file_states.yml @@ -1,7 +1,7 @@ --- table_name: ci_secure_file_states classes: -- Geo::SecureFileState +- Geo::CiSecureFileState feature_categories: - pipeline_authoring description: Stores verification state for Geo replicated Project-level Secure Files. diff --git a/ee/app/models/ee/ci/secure_file.rb b/ee/app/models/ee/ci/secure_file.rb index 74351bca90ed84..5ba06e3f96617c 100644 --- a/ee/app/models/ee/ci/secure_file.rb +++ b/ee/app/models/ee/ci/secure_file.rb @@ -19,7 +19,7 @@ module SecureFile with_replicator Geo::CiSecureFileReplicator has_one :ci_secure_file_state, autosave: false, inverse_of: :ci_secure_file, - class_name: 'Geo::SecureFileState', foreign_key: :ci_secure_file_id + class_name: 'Geo::CiSecureFileState', foreign_key: :ci_secure_file_id after_save :save_verification_details @@ -55,7 +55,7 @@ def verification_state_object override :verification_state_table_class def verification_state_table_class - ::Geo::SecureFileState + ::Geo::CiSecureFileState end end diff --git a/ee/app/models/geo/secure_file_state.rb b/ee/app/models/geo/ci_secure_file_state.rb similarity index 80% rename from ee/app/models/geo/secure_file_state.rb rename to ee/app/models/geo/ci_secure_file_state.rb index 0707a6fdddc9f4..247fa5d084374c 100644 --- a/ee/app/models/geo/secure_file_state.rb +++ b/ee/app/models/geo/ci_secure_file_state.rb @@ -1,10 +1,11 @@ # frozen_string_literal: true module Geo - class SecureFileState < Ci::ApplicationRecord + class CiSecureFileState < Ci::ApplicationRecord include EachBatch include ::Geo::VerificationStateDefinition self.primary_key = :ci_secure_file_id + self.table_name = :ci_secure_file_states belongs_to :ci_secure_file, inverse_of: :ci_secure_file_state, class_name: 'Ci::SecureFile' diff --git a/ee/spec/factories/geo/ci_secure_file_states.rb b/ee/spec/factories/geo/ci_secure_file_states.rb index 6d8359c1ea7776..a390bd837ec9b9 100644 --- a/ee/spec/factories/geo/ci_secure_file_states.rb +++ b/ee/spec/factories/geo/ci_secure_file_states.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true FactoryBot.define do - factory :geo_ci_secure_file_state, class: 'Geo::SecureFileState' do + factory :geo_ci_secure_file_state, class: 'Geo::CiSecureFileState' do ci_secure_file trait(:checksummed) do diff --git a/ee/spec/models/ee/ci/secure_file_spec.rb b/ee/spec/models/ee/ci/secure_file_spec.rb index eacfc5f51bd110..2fded8775f3c33 100644 --- a/ee/spec/models/ee/ci/secure_file_spec.rb +++ b/ee/spec/models/ee/ci/secure_file_spec.rb @@ -6,16 +6,15 @@ using RSpec::Parameterized::TableSyntax include EE::GeoHelpers - let_it_be(:group) { create(:group) } - let_it_be(:project) { create(:project, group: group) } + let(:project) { create(:project) } include_examples 'a replicable model with a separate table for verification state' do before do - stub_ci_secure_file_object_storage(enabled: false) + stub_ci_secure_file_object_storage end - let(:verifiable_model_record) { build(:ci_secure_file, project: project) } - let(:unverifiable_model_record) { build(:ci_secure_file, project: project) } + let(:verifiable_model_record) { create(:ci_secure_file, project: project) } + let(:unverifiable_model_record) { create(:ci_secure_file, :remote_store, project: project) } end describe '#replicables_for_current_secondary' do -- GitLab From aeee7a41fe994709040a228b986ef662d4b2721e Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Mon, 18 Jul 2022 15:14:23 -0500 Subject: [PATCH 4/9] Fixing test failures --- .../geo_ci_secure_file_replication.yml | 2 +- .../public_api/v4/geo_node_status.json | 23 +++++++++++++++++ ee/spec/models/ee/ci/secure_file_spec.rb | 25 +++++++++---------- ...del_with_separate_table_shared_examples.rb | 1 + spec/factories/ci/secure_files.rb | 5 ++-- 5 files changed, 40 insertions(+), 16 deletions(-) diff --git a/ee/config/feature_flags/development/geo_ci_secure_file_replication.yml b/ee/config/feature_flags/development/geo_ci_secure_file_replication.yml index ae2d2013c3cec8..db3b2ba9dd722b 100644 --- a/ee/config/feature_flags/development/geo_ci_secure_file_replication.yml +++ b/ee/config/feature_flags/development/geo_ci_secure_file_replication.yml @@ -4,5 +4,5 @@ introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91430 rollout_issue_url: milestone: '15.2' type: development -group: group::geo +group: group::incubation default_enabled: false diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/geo_node_status.json b/ee/spec/fixtures/api/schemas/public_api/v4/geo_node_status.json index 50284970513a1e..9e9bdf54e50569 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/geo_node_status.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/geo_node_status.json @@ -29,6 +29,16 @@ "job_artifacts_verified_count", "job_artifacts_verification_failed_count", "job_artifacts_verified_in_percentage", + "ci_secure_files_count", + "ci_secure_files_checksum_total_count", + "ci_secure_files_checksummed_count", + "ci_secure_files_checksum_failed_count", + "ci_secure_files_synced_count", + "ci_secure_files_failed_count", + "ci_secure_files_registry_count", + "ci_secure_files_verification_total_count", + "ci_secure_files_verified_count", + "ci_secure_files_verification_failed_count", "db_replication_lag_seconds", "container_repositories_replication_enabled", "container_repositories_count", @@ -219,6 +229,19 @@ "job_artifacts_verified_count": { "type": ["integer", "null"] }, "job_artifacts_verification_failed_count": { "type": ["integer", "null"] }, "job_artifacts_verified_in_percentage": { "type": "string" }, + "ci_secure_files_count": { "type": "integer" }, + "ci_secure_files_failed_count": { "type": ["integer", "null"] }, + "ci_secure_files_synced_count": { "type": ["integer", "null"] }, + "ci_secure_files_synced_missing_on_primary_count": { "type": ["integer", "null"] }, + "ci_secure_files_synced_in_percentage": { "type": "string" }, + "ci_secure_files_checksum_total_count": { "type": ["integer", "null"] }, + "ci_secure_files_checksummed_count": { "type": ["integer", "null"] }, + "ci_secure_files_checksum_failed_count": { "type": ["integer", "null"] }, + "ci_secure_files_registry_count": { "type": ["integer", "null"] }, + "ci_secure_files_verification_total_count": { "type": ["integer", "null"] }, + "ci_secure_files_verified_count": { "type": ["integer", "null"] }, + "ci_secure_files_verification_failed_count": { "type": ["integer", "null"] }, + "ci_secure_files_verified_in_percentage": { "type": "string" }, "container_repositories_replication_enabled": { "type": ["boolean", "null"] }, "container_repositories_count": { "type": "integer" }, "container_repositories_failed_count": { "type": ["integer", "null"] }, diff --git a/ee/spec/models/ee/ci/secure_file_spec.rb b/ee/spec/models/ee/ci/secure_file_spec.rb index 2fded8775f3c33..9faf838cecacdf 100644 --- a/ee/spec/models/ee/ci/secure_file_spec.rb +++ b/ee/spec/models/ee/ci/secure_file_spec.rb @@ -6,15 +6,22 @@ using RSpec::Parameterized::TableSyntax include EE::GeoHelpers + before do + stub_ci_secure_file_object_storage(enabled: false) + end + let(:project) { create(:project) } include_examples 'a replicable model with a separate table for verification state' do - before do + let(:verifiable_model_record) { build(:ci_secure_file, project: project) } + + let(:unverifiable_model_record) do stub_ci_secure_file_object_storage - end + file = create(:ci_secure_file, :remote_store, project: project) + stub_ci_secure_file_object_storage(enabled: false) - let(:verifiable_model_record) { create(:ci_secure_file, project: project) } - let(:unverifiable_model_record) { create(:ci_secure_file, :remote_store, project: project) } + file + end end describe '#replicables_for_current_secondary' do @@ -48,7 +55,7 @@ end before do - stub_ci_secure_file_object_storage + stub_ci_secure_file_object_storage(enabled: false) stub_current_geo_node(node) end @@ -56,10 +63,6 @@ let(:sync_object_storage) { true } context 'when the ci secure file is locally stored' do - before do - stub_ci_secure_file_object_storage(enabled: false) - end - let(:ci_secure_file) { create(*factory, project: project) } it { is_expected.to eq(include_expectation) } @@ -76,10 +79,6 @@ let(:sync_object_storage) { false } context 'when the ci secure file is locally stored' do - before do - stub_ci_secure_file_object_storage(enabled: false) - end - let(:ci_secure_file) { create(*factory, project: project) } it { is_expected.to eq(include_expectation) } diff --git a/ee/spec/support/shared_examples/models/concerns/replicable_model_with_separate_table_shared_examples.rb b/ee/spec/support/shared_examples/models/concerns/replicable_model_with_separate_table_shared_examples.rb index 2bc4900517a2f1..09ff21814dad66 100644 --- a/ee/spec/support/shared_examples/models/concerns/replicable_model_with_separate_table_shared_examples.rb +++ b/ee/spec/support/shared_examples/models/concerns/replicable_model_with_separate_table_shared_examples.rb @@ -58,6 +58,7 @@ context 'when model record is not part of available_verifiables scope' do it 'does not create verification details' do + # binding.pry expect { unverifiable_model_record.save! }.not_to change { verification_state_table_class.count } end end diff --git a/spec/factories/ci/secure_files.rb b/spec/factories/ci/secure_files.rb index 1dd33bb4260a04..74988202c71477 100644 --- a/spec/factories/ci/secure_files.rb +++ b/spec/factories/ci/secure_files.rb @@ -4,12 +4,13 @@ factory :ci_secure_file, class: 'Ci::SecureFile' do sequence(:name) { |n| "file#{n}" } file { fixture_file_upload('spec/fixtures/ci_secure_files/upload-keystore.jks', 'application/octet-stream') } - file_store { ::ObjectStorage::Store::LOCAL } checksum { 'foo1234' } project trait :remote_store do - file_store { ::ObjectStorage::Store::REMOTE } + after(:create) do |ci_secure_file| + ci_secure_file.update!(file_store: ObjectStorage::Store::REMOTE) + end end end end -- GitLab From 778c3f3e309add0a4ef1e2510c88aee91d3bfee1 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Wed, 20 Jul 2022 10:36:44 -0500 Subject: [PATCH 5/9] Several clean up items --- app/models/ci/secure_file.rb | 2 +- ee/app/models/concerns/ee/ci/artifactable.rb | 8 ++++---- ee/app/models/ee/ci/secure_file.rb | 1 - ...eplicable_model_with_separate_table_shared_examples.rb | 1 - 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/models/ci/secure_file.rb b/app/models/ci/secure_file.rb index 30d2aca2f4fa1e..1d7c935aa95cfc 100644 --- a/app/models/ci/secure_file.rb +++ b/app/models/ci/secure_file.rb @@ -48,4 +48,4 @@ def generate_key_data end end -Ci::SecureFile.prepend_mod_with('Ci::SecureFile') +Ci::SecureFile.prepend_mod diff --git a/ee/app/models/concerns/ee/ci/artifactable.rb b/ee/app/models/concerns/ee/ci/artifactable.rb index 193649891033e8..e87d66ed713cae 100644 --- a/ee/app/models/concerns/ee/ci/artifactable.rb +++ b/ee/app/models/concerns/ee/ci/artifactable.rb @@ -6,8 +6,8 @@ module Artifactable extend ActiveSupport::Concern class_methods do - # @param primary_key_in [Range, Ci::{Pipeline|Job}Artifact] arg to pass to primary_key_in scope - # @return [ActiveRecord::Relation] everything that should be synced to this node, restricted by primary key + # @param primary_key_in [Range, Ci::{PipelineArtifact|JobArtifact|SecureFile}] arg to pass to primary_key_in scope + # @return [ActiveRecord::Relation] everything that should be synced to this node, restricted by primary key def replicables_for_current_secondary(primary_key_in) node = ::Gitlab::Geo.current_node @@ -18,7 +18,7 @@ def replicables_for_current_secondary(primary_key_in) selective_sync_scope(node, replicables) end - # @return [ActiveRecord::Relation] observing object storage settings of the given node + # @return [ActiveRecord::Relation] observing object storage settings of the given node def object_storage_scope(node) return all if node.sync_object_storage? @@ -28,7 +28,7 @@ def object_storage_scope(node) # The primary_key_in in replicables_for_current_secondary method is at most a range of IDs with a maximum of 10_000 records # between them. We can additionally reduce the batch size to 1_000 just for pipeline artifacts and job artifacts if needed. # - # @return [ActiveRecord::Relation] observing selective sync settings of the given node + # @return [ActiveRecord::Relation] observing selective sync settings of the given node def selective_sync_scope(node, replicables) return replicables unless node.selective_sync? diff --git a/ee/app/models/ee/ci/secure_file.rb b/ee/app/models/ee/ci/secure_file.rb index 5ba06e3f96617c..d2bb21e94600f6 100644 --- a/ee/app/models/ee/ci/secure_file.rb +++ b/ee/app/models/ee/ci/secure_file.rb @@ -43,7 +43,6 @@ module SecureFile scope :available_verifiables, -> { joins(:ci_secure_file_state) } scope :with_files_stored_locally, -> { where(file_store: ::ObjectStorage::Store::LOCAL) } - scope :with_files_stored_remotely, -> { where(file_store: ::ObjectStorage::Store::REMOTE) } def verification_state_object ci_secure_file_state diff --git a/ee/spec/support/shared_examples/models/concerns/replicable_model_with_separate_table_shared_examples.rb b/ee/spec/support/shared_examples/models/concerns/replicable_model_with_separate_table_shared_examples.rb index 09ff21814dad66..2bc4900517a2f1 100644 --- a/ee/spec/support/shared_examples/models/concerns/replicable_model_with_separate_table_shared_examples.rb +++ b/ee/spec/support/shared_examples/models/concerns/replicable_model_with_separate_table_shared_examples.rb @@ -58,7 +58,6 @@ context 'when model record is not part of available_verifiables scope' do it 'does not create verification details' do - # binding.pry expect { unverifiable_model_record.save! }.not_to change { verification_state_table_class.count } end end -- GitLab From bbe82fc5357b14cd5e9fdd55d0efa77982c7859d Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Wed, 20 Jul 2022 13:22:23 -0500 Subject: [PATCH 6/9] Adding metrics gathering --- .../monitoring/prometheus/gitlab_metrics.md | 12 +++++++++ doc/api/geo_nodes.md | 26 +++++++++++++++++++ .../public_api/v4/geo_node_status.json | 3 ++- ee/spec/models/geo_node_status_spec.rb | 1 + 4 files changed, 41 insertions(+), 1 deletion(-) diff --git a/doc/administration/monitoring/prometheus/gitlab_metrics.md b/doc/administration/monitoring/prometheus/gitlab_metrics.md index a74f6c39b728f6..53f802d67ccbf7 100644 --- a/doc/administration/monitoring/prometheus/gitlab_metrics.md +++ b/doc/administration/monitoring/prometheus/gitlab_metrics.md @@ -301,6 +301,18 @@ configuration option in `gitlab.yml`. These metrics are served from the | `geo_uploads_verification_failed` | Gauge | 14.6 | Number of uploads verifications failed on secondary | `url` | | `gitlab_sli:rails_request_apdex:total` | Counter | 14.4 | The number of request-apdex measurements, [more information the development documentation](../../../development/application_slis/rails_request_apdex.md) | `endpoint_id`, `feature_category`, `request_urgency` | | `gitlab_sli:rails_request_apdex:success_total` | Counter | 14.4 | The number of successful requests that met the target duration for their urgency. Divide by `gitlab_sli:rails_requests_apdex:total` to get a success ratio | `endpoint_id`, `feature_category`, `request_urgency` | +| `geo_ci_secure_files` | Gauge | 15.3 | Number of secure files on primary | `url` | +| `geo_ci_secure_files_checksum_total` | Gauge | 15.3 | Number of secure files tried to checksum on primary | `url` | +| `geo_ci_secure_files_checksummed` | Gauge | 15.3 | Number of secure files successfully checksummed on primary | `url` | +| `geo_ci_secure_files_checksum_failed` | Gauge | 15.3 | Number of secure files failed to calculate the checksum on primary | `url` | +| `geo_ci_secure_files_synced` | Gauge | 15.3 | Number of syncable secure files synced on secondary | `url` | +| `geo_ci_secure_files_failed` | Gauge | 15.3 | Number of syncable secure files failed to sync on secondary | `url` | +| `geo_ci_secure_files_registry` | Gauge | 15.3 | Number of secure files in the registry | `url` | +| `geo_ci_secure_files_verification_total` | Gauge | 15.3 | Number of secure files verifications tried on secondary | `url` | +| `geo_ci_secure_files_verified` | Gauge | 15.3 | Number of secure files verified on secondary | `url` | +| `geo_ci_secure_files_verification_failed` | Gauge | 15.3 | Number of secure files verifications failed on secondary | `url` | + + ## Database load balancing metrics **(PREMIUM SELF)** diff --git a/doc/api/geo_nodes.md b/doc/api/geo_nodes.md index fbb583f5a56433..b5b920ec0cd7b0 100644 --- a/doc/api/geo_nodes.md +++ b/doc/api/geo_nodes.md @@ -495,6 +495,19 @@ Example response: "job_artifacts_synced_in_percentage": "100.00%", "job_artifacts_verified_in_percentage": "100.00%", "job_artifacts_synced_missing_on_primary_count": 0, + "ci_secure_files_count": 5, + "ci_secure_files_checksum_total_count": 5, + "ci_secure_files_checksummed_count": 5, + "ci_secure_files_checksum_failed_count": 0, + "ci_secure_files_synced_count": 5, + "ci_secure_files_failed_count": 0, + "ci_secure_files_registry_count": 5, + "ci_secure_files_verification_total_count": 5, + "ci_secure_files_verified_count": 5, + "ci_secure_files_verification_failed_count": 0, + "ci_secure_files_synced_in_percentage": "100.00%", + "ci_secure_files_verified_in_percentage": "100.00%", + "ci_secure_files_synced_missing_on_primary_count": 0, }, { "geo_node_id": 2, @@ -830,6 +843,19 @@ Example response: "job_artifacts_synced_in_percentage": "100.00%", "job_artifacts_verified_in_percentage": "100.00%", "job_artifacts_synced_missing_on_primary_count": 0, + "ci_secure_files_count": 5, + "ci_secure_files_checksum_total_count": 5, + "ci_secure_files_checksummed_count": 5, + "ci_secure_files_checksum_failed_count": 0, + "ci_secure_files_synced_count": 5, + "ci_secure_files_failed_count": 0, + "ci_secure_files_registry_count": 5, + "ci_secure_files_verification_total_count": 5, + "ci_secure_files_verified_count": 5, + "ci_secure_files_verification_failed_count": 0, + "ci_secure_files_synced_in_percentage": "100.00%", + "ci_secure_files_verified_in_percentage": "100.00%", + "ci_secure_files_synced_missing_on_primary_count": 0, } ``` diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/geo_node_status.json b/ee/spec/fixtures/api/schemas/public_api/v4/geo_node_status.json index 9e9bdf54e50569..213c4311da0743 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/geo_node_status.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/geo_node_status.json @@ -39,6 +39,8 @@ "ci_secure_files_verification_total_count", "ci_secure_files_verified_count", "ci_secure_files_verification_failed_count", + "ci_secure_files_synced_in_percentage", + "ci_secure_files_verified_in_percentage", "db_replication_lag_seconds", "container_repositories_replication_enabled", "container_repositories_count", @@ -232,7 +234,6 @@ "ci_secure_files_count": { "type": "integer" }, "ci_secure_files_failed_count": { "type": ["integer", "null"] }, "ci_secure_files_synced_count": { "type": ["integer", "null"] }, - "ci_secure_files_synced_missing_on_primary_count": { "type": ["integer", "null"] }, "ci_secure_files_synced_in_percentage": { "type": "string" }, "ci_secure_files_checksum_total_count": { "type": ["integer", "null"] }, "ci_secure_files_checksummed_count": { "type": ["integer", "null"] }, diff --git a/ee/spec/models/geo_node_status_spec.rb b/ee/spec/models/geo_node_status_spec.rb index ae7b641c0c8b42..22cab9fb08a70b 100644 --- a/ee/spec/models/geo_node_status_spec.rb +++ b/ee/spec/models/geo_node_status_spec.rb @@ -1031,6 +1031,7 @@ Geo::PagesDeploymentReplicator | :pages_deployment | :geo_pages_deployment_registry Geo::UploadReplicator | :upload | :geo_upload_registry Geo::JobArtifactReplicator | :ci_job_artifact | :geo_job_artifact_registry + Geo::CiSecureFileReplicator | :ci_secure_file | :geo_ci_secure_file_registry end with_them do -- GitLab From 63701295ca39dfa911444cf288824703b613266e Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Wed, 20 Jul 2022 15:09:27 -0500 Subject: [PATCH 7/9] Implementing GraphQL API --- .../monitoring/prometheus/gitlab_metrics.md | 2 - doc/api/graphql/reference/index.md | 56 +++++++++++++++++++ .../geo/ci_secure_file_registry_finder.rb | 7 +++ .../geo/ci_secure_file_registries_resolver.rb | 11 ++++ .../types/geo/ci_secure_file_registry_type.rb | 15 +++++ ee/app/graphql/types/geo/geo_node_type.rb | 5 ++ .../ci_secure_file_registry_finder_spec.rb | 7 +++ ...ci_secure_file_registries_resolver_spec.rb | 7 +++ .../geo/ci_secure_file_registry_type_spec.rb | 13 +++++ .../graphql/types/geo/geo_node_type_spec.rb | 2 +- .../api/graphql/geo/registries_spec.rb | 7 +++ 11 files changed, 129 insertions(+), 3 deletions(-) create mode 100644 ee/app/finders/geo/ci_secure_file_registry_finder.rb create mode 100644 ee/app/graphql/resolvers/geo/ci_secure_file_registries_resolver.rb create mode 100644 ee/app/graphql/types/geo/ci_secure_file_registry_type.rb create mode 100644 ee/spec/finders/geo/ci_secure_file_registry_finder_spec.rb create mode 100644 ee/spec/graphql/resolvers/geo/ci_secure_file_registries_resolver_spec.rb create mode 100644 ee/spec/graphql/types/geo/ci_secure_file_registry_type_spec.rb diff --git a/doc/administration/monitoring/prometheus/gitlab_metrics.md b/doc/administration/monitoring/prometheus/gitlab_metrics.md index 53f802d67ccbf7..54dc877ead70a6 100644 --- a/doc/administration/monitoring/prometheus/gitlab_metrics.md +++ b/doc/administration/monitoring/prometheus/gitlab_metrics.md @@ -312,8 +312,6 @@ configuration option in `gitlab.yml`. These metrics are served from the | `geo_ci_secure_files_verified` | Gauge | 15.3 | Number of secure files verified on secondary | `url` | | `geo_ci_secure_files_verification_failed` | Gauge | 15.3 | Number of secure files verifications failed on secondary | `url` | - - ## Database load balancing metrics **(PREMIUM SELF)** The following metrics are available: diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index fa317490c56877..eb0dd08b78ee76 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -6302,6 +6302,29 @@ The edge type for [`CiRunner`](#cirunner). | `node` | [`CiRunner`](#cirunner) | The item at the end of the edge. | | `webUrl` | [`String`](#string) | Web URL of the runner. The value depends on where you put this field in the query. You can use it for projects or groups. | +#### `CiSecureFileRegistryConnection` + +The connection type for [`CiSecureFileRegistry`](#cisecurefileregistry). + +##### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `edges` | [`[CiSecureFileRegistryEdge]`](#cisecurefileregistryedge) | A list of edges. | +| `nodes` | [`[CiSecureFileRegistry]`](#cisecurefileregistry) | A list of nodes. | +| `pageInfo` | [`PageInfo!`](#pageinfo) | Information to aid in pagination. | + +#### `CiSecureFileRegistryEdge` + +The edge type for [`CiSecureFileRegistry`](#cisecurefileregistry). + +##### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `cursor` | [`String!`](#string) | A cursor for use in pagination. | +| `node` | [`CiSecureFileRegistry`](#cisecurefileregistry) | The item at the end of the edge. | + #### `CiStageConnection` The connection type for [`CiStage`](#cistage). @@ -10070,6 +10093,23 @@ Returns [`CiRunnerStatus!`](#cirunnerstatus). | ---- | ---- | ----------- | | `legacyMode` **{warning-solid}** | [`String`](#string) | **Deprecated** in 15.0. Will be removed in 17.0. In GitLab 16.0 and later, the field will act as if `legacyMode` is null. | +### `CiSecureFileRegistry` + +Represents the Geo replication and verification state of a ci_secure_file. + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `ciSecureFileId` | [`ID!`](#id) | ID of the Ci Secure File. | +| `createdAt` | [`Time`](#time) | Timestamp when the CiSecureFileRegistry was created. | +| `id` | [`ID!`](#id) | ID of the CiSecureFileRegistry. | +| `lastSyncFailure` | [`String`](#string) | Error message during sync of the CiSecureFileRegistry. | +| `lastSyncedAt` | [`Time`](#time) | Timestamp of the most recent successful sync of the CiSecureFileRegistry. | +| `retryAt` | [`Time`](#time) | Timestamp after which the CiSecureFileRegistry should be resynced. | +| `retryCount` | [`Int`](#int) | Number of consecutive failed sync attempts of the CiSecureFileRegistry. | +| `state` | [`RegistryState`](#registrystate) | Sync state of the CiSecureFileRegistry. | + ### `CiStage` #### Fields @@ -11681,6 +11721,22 @@ Represents an external issue. #### Fields with arguments +##### `GeoNode.ciSecureFileRegistries` + +Find Ci Secure File registries on this Geo node Available only when feature flag `geo_ci_secure_file_replication` is enabled. This flag is disabled by default, because the feature is experimental and is subject to change without notice. + +Returns [`CiSecureFileRegistryConnection`](#cisecurefileregistryconnection). + +This field returns a [connection](#connections). It accepts the +four standard [pagination arguments](#connection-pagination-arguments): +`before: String`, `after: String`, `first: Int`, `last: Int`. + +###### Arguments + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `ids` | [`[ID!]`](#id) | Filters registries by their ID. | + ##### `GeoNode.groupWikiRepositoryRegistries` Find group wiki repository registries on this Geo node. diff --git a/ee/app/finders/geo/ci_secure_file_registry_finder.rb b/ee/app/finders/geo/ci_secure_file_registry_finder.rb new file mode 100644 index 00000000000000..ef4f69a9425946 --- /dev/null +++ b/ee/app/finders/geo/ci_secure_file_registry_finder.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module Geo + class CiSecureFileRegistryFinder + include FrameworkRegistryFinder + end +end diff --git a/ee/app/graphql/resolvers/geo/ci_secure_file_registries_resolver.rb b/ee/app/graphql/resolvers/geo/ci_secure_file_registries_resolver.rb new file mode 100644 index 00000000000000..2818b4be0efaaf --- /dev/null +++ b/ee/app/graphql/resolvers/geo/ci_secure_file_registries_resolver.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Resolvers + module Geo + class CiSecureFileRegistriesResolver < BaseResolver + type ::Types::Geo::GeoNodeType.connection_type, null: true + + include RegistriesResolver + end + end +end diff --git a/ee/app/graphql/types/geo/ci_secure_file_registry_type.rb b/ee/app/graphql/types/geo/ci_secure_file_registry_type.rb new file mode 100644 index 00000000000000..4aae738baa6f43 --- /dev/null +++ b/ee/app/graphql/types/geo/ci_secure_file_registry_type.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Types + module Geo + # rubocop:disable Graphql/AuthorizeTypes because it is included + class CiSecureFileRegistryType < BaseObject + graphql_name 'CiSecureFileRegistry' + description 'Represents the Geo replication and verification state of a ci_secure_file.' + + include ::Types::Geo::RegistryType + + field :ci_secure_file_id, GraphQL::Types::ID, null: false, description: 'ID of the Ci Secure File.' + end + end +end diff --git a/ee/app/graphql/types/geo/geo_node_type.rb b/ee/app/graphql/types/geo/geo_node_type.rb index 314ed1b2b5e003..a0be29b5bfe504 100644 --- a/ee/app/graphql/types/geo/geo_node_type.rb +++ b/ee/app/graphql/types/geo/geo_node_type.rb @@ -7,6 +7,11 @@ class GeoNodeType < BaseObject authorize :read_geo_node + field :ci_secure_file_registries, ::Types::Geo::CiSecureFileRegistryType.connection_type, + null: true, + resolver: ::Resolvers::Geo::CiSecureFileRegistriesResolver, + description: 'Find Ci Secure File registries on this Geo node', + feature_flag: :geo_ci_secure_file_replication field :container_repositories_max_capacity, GraphQL::Types::Int, null: true, description: 'Maximum concurrency of container repository sync for this secondary node.' field :enabled, GraphQL::Types::Boolean, null: true, description: 'Indicates whether this Geo node is enabled.' field :files_max_capacity, GraphQL::Types::Int, null: true, description: 'Maximum concurrency of LFS/attachment backfill for this secondary node.' diff --git a/ee/spec/finders/geo/ci_secure_file_registry_finder_spec.rb b/ee/spec/finders/geo/ci_secure_file_registry_finder_spec.rb new file mode 100644 index 00000000000000..764c29c5d59133 --- /dev/null +++ b/ee/spec/finders/geo/ci_secure_file_registry_finder_spec.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Geo::CiSecureFileRegistryFinder do + it_behaves_like 'a framework registry finder', :geo_ci_secure_file_registry +end diff --git a/ee/spec/graphql/resolvers/geo/ci_secure_file_registries_resolver_spec.rb b/ee/spec/graphql/resolvers/geo/ci_secure_file_registries_resolver_spec.rb new file mode 100644 index 00000000000000..2a7c26a68d052e --- /dev/null +++ b/ee/spec/graphql/resolvers/geo/ci_secure_file_registries_resolver_spec.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Resolvers::Geo::CiSecureFileRegistriesResolver do + it_behaves_like 'a Geo registries resolver', :geo_ci_secure_file_registry +end diff --git a/ee/spec/graphql/types/geo/ci_secure_file_registry_type_spec.rb b/ee/spec/graphql/types/geo/ci_secure_file_registry_type_spec.rb new file mode 100644 index 00000000000000..2c80c7463fd29b --- /dev/null +++ b/ee/spec/graphql/types/geo/ci_secure_file_registry_type_spec.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GitlabSchema.types['CiSecureFileRegistry'] do + it_behaves_like 'a Geo registry type' + + it 'has the expected fields (other than those included in RegistryType)' do + expected_fields = %i[ci_secure_file_id] + + expect(described_class).to have_graphql_fields(*expected_fields).at_least + end +end diff --git a/ee/spec/graphql/types/geo/geo_node_type_spec.rb b/ee/spec/graphql/types/geo/geo_node_type_spec.rb index 6d9becaa901c08..676251819e0cb5 100644 --- a/ee/spec/graphql/types/geo/geo_node_type_spec.rb +++ b/ee/spec/graphql/types/geo/geo_node_type_spec.rb @@ -15,7 +15,7 @@ package_file_registries snippet_repository_registries terraform_state_version_registries group_wiki_repository_registries pages_deployment_registries lfs_object_registries pipeline_artifact_registries - upload_registries job_artifact_registries + upload_registries job_artifact_registries ci_secure_file_registries ] expect(described_class).to have_graphql_fields(*expected_fields) diff --git a/ee/spec/requests/api/graphql/geo/registries_spec.rb b/ee/spec/requests/api/graphql/geo/registries_spec.rb index 7c4cdd0680e6d5..7c00649dedf0da 100644 --- a/ee/spec/requests/api/graphql/geo/registries_spec.rb +++ b/ee/spec/requests/api/graphql/geo/registries_spec.rb @@ -65,4 +65,11 @@ registry_factory: :geo_job_artifact_registry, registry_foreign_key_field_name: 'artifactId' } + + it_behaves_like 'gets registries for', { + field_name: 'ciSecureFileRegistries', + registry_class_name: 'CiSecureFileRegistry', + registry_factory: :geo_ci_secure_file_registry, + registry_foreign_key_field_name: 'ciSecureFileId' + } end -- GitLab From ea546a28a160eebb69b58689e0df3debe6237f12 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Thu, 21 Jul 2022 18:16:25 -0500 Subject: [PATCH 8/9] Spec improvements --- ee/spec/models/ee/ci/secure_file_spec.rb | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/ee/spec/models/ee/ci/secure_file_spec.rb b/ee/spec/models/ee/ci/secure_file_spec.rb index 9faf838cecacdf..5f11c315311136 100644 --- a/ee/spec/models/ee/ci/secure_file_spec.rb +++ b/ee/spec/models/ee/ci/secure_file_spec.rb @@ -6,18 +6,13 @@ using RSpec::Parameterized::TableSyntax include EE::GeoHelpers - before do - stub_ci_secure_file_object_storage(enabled: false) - end - - let(:project) { create(:project) } - include_examples 'a replicable model with a separate table for verification state' do + let(:project) { create(:project) } let(:verifiable_model_record) { build(:ci_secure_file, project: project) } let(:unverifiable_model_record) do stub_ci_secure_file_object_storage - file = create(:ci_secure_file, :remote_store, project: project) + file = build(:ci_secure_file, :remote_store, project: project) stub_ci_secure_file_object_storage(enabled: false) file @@ -55,7 +50,6 @@ end before do - stub_ci_secure_file_object_storage(enabled: false) stub_current_geo_node(node) end -- GitLab From bf0fbfe6dab17a7899d62343bc925aa2cbafe461 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Fri, 22 Jul 2022 06:31:16 -0500 Subject: [PATCH 9/9] Updated GraphQL docs --- doc/api/graphql/reference/index.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index eb0dd08b78ee76..e0d97cbc6fbfbb 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -10106,9 +10106,11 @@ Represents the Geo replication and verification state of a ci_secure_file. | `id` | [`ID!`](#id) | ID of the CiSecureFileRegistry. | | `lastSyncFailure` | [`String`](#string) | Error message during sync of the CiSecureFileRegistry. | | `lastSyncedAt` | [`Time`](#time) | Timestamp of the most recent successful sync of the CiSecureFileRegistry. | -| `retryAt` | [`Time`](#time) | Timestamp after which the CiSecureFileRegistry should be resynced. | +| `retryAt` | [`Time`](#time) | Timestamp after which the CiSecureFileRegistry is resynced. | | `retryCount` | [`Int`](#int) | Number of consecutive failed sync attempts of the CiSecureFileRegistry. | | `state` | [`RegistryState`](#registrystate) | Sync state of the CiSecureFileRegistry. | +| `verificationRetryAt` | [`Time`](#time) | Timestamp after which the CiSecureFileRegistry is reverified. | +| `verifiedAt` | [`Time`](#time) | Timestamp of the most recent successful verification of the CiSecureFileRegistry. | ### `CiStage` -- GitLab