| {{ $options.i18n.infoLabel }} | +{{ $options.i18n.idLabel }} | +{{ candidate.info.iid }} | +|
| + | {{ $options.i18n.statusLabel }} | +{{ candidate.info.status }} | +|
| + | {{ $options.i18n.experimentLabel }} | +
+ |
+ |
| + | {{ $options.i18n.artifactsLabel }} | +
+ |
+ |
| + {{ $options.i18n.parametersLabel }} + | ++ | {{ param.name }} | +{{ param.value }} | +
| + {{ $options.i18n.metricsLabel }} + | ++ | {{ metric.name }} | +{{ metric.value }} | +
| + Info + | + ++ ID + | + ++ candidate_iid + | +
| + + | + Status + | + ++ SUCCESS + | +
| + + | + Experiment + | + ++ + The Experiment + + | +
| + + Parameters + + | + ++ Algorithm + | + ++ Decision Tree + | +
| + + | + MaxDepth + | + ++ 3 + | +
| + + Metrics + + | + ++ AUC + | + ++ .55 + | +
| + + | + Accuracy + | + ++ .99 + | +
| + + | ++ + | ++ + Details + + | ++ + Artifacts + + |
|---|
| + + Details + + | +diff --git a/spec/frontend/ml/experiment_tracking/components/incubation_alert_spec.js b/spec/frontend/ml/experiment_tracking/components/incubation_alert_spec.js index e07a4ed816b..7dca360c7ee 100644 --- a/spec/frontend/ml/experiment_tracking/components/incubation_alert_spec.js +++ b/spec/frontend/ml/experiment_tracking/components/incubation_alert_spec.js @@ -15,7 +15,7 @@ describe('IncubationAlert', () => { it('displays link to issue', () => { expect(findButton().attributes().href).toBe( - 'https://gitlab.com/groups/gitlab-org/-/epics/8560', + 'https://gitlab.com/gitlab-org/gitlab/-/issues/381660', ); }); diff --git a/spec/frontend/ml/experiment_tracking/components/ml_candidate_spec.js b/spec/frontend/ml/experiment_tracking/components/ml_candidate_spec.js new file mode 100644 index 00000000000..4b16312815a --- /dev/null +++ b/spec/frontend/ml/experiment_tracking/components/ml_candidate_spec.js @@ -0,0 +1,43 @@ +import { GlAlert } from '@gitlab/ui'; +import { mountExtended } from 'helpers/vue_test_utils_helper'; +import MlCandidate from '~/ml/experiment_tracking/components/ml_candidate.vue'; + +describe('MlCandidate', () => { + let wrapper; + + const createWrapper = () => { + const candidate = { + params: [ + { name: 'Algorithm', value: 'Decision Tree' }, + { name: 'MaxDepth', value: '3' }, + ], + metrics: [ + { name: 'AUC', value: '.55' }, + { name: 'Accuracy', value: '.99' }, + ], + info: { + iid: 'candidate_iid', + artifact_link: 'path_to_artifact', + experiment_name: 'The Experiment', + experiment_path: 'path/to/experiment', + status: 'SUCCESS', + }, + }; + + return mountExtended(MlCandidate, { provide: { candidate } }); + }; + + const findAlert = () => wrapper.findComponent(GlAlert); + + it('shows incubation warning', () => { + wrapper = createWrapper(); + + expect(findAlert().exists()).toBe(true); + }); + + it('renders correctly', () => { + wrapper = createWrapper(); + + expect(wrapper.element).toMatchSnapshot(); + }); +}); diff --git a/spec/frontend/ml/experiment_tracking/components/experiment_spec.js b/spec/frontend/ml/experiment_tracking/components/ml_experiment_spec.js similarity index 63% rename from spec/frontend/ml/experiment_tracking/components/experiment_spec.js rename to spec/frontend/ml/experiment_tracking/components/ml_experiment_spec.js index af722d77532..50539440f25 100644 --- a/spec/frontend/ml/experiment_tracking/components/experiment_spec.js +++ b/spec/frontend/ml/experiment_tracking/components/ml_experiment_spec.js @@ -1,17 +1,17 @@ import { GlAlert } from '@gitlab/ui'; import { mountExtended } from 'helpers/vue_test_utils_helper'; -import ShowExperiment from '~/ml/experiment_tracking/components/experiment.vue'; +import MlExperiment from '~/ml/experiment_tracking/components/ml_experiment.vue'; -describe('ShowExperiment', () => { +describe('MlExperiment', () => { let wrapper; const createWrapper = (candidates = [], metricNames = [], paramNames = []) => { - return mountExtended(ShowExperiment, { provide: { candidates, metricNames, paramNames } }); + return mountExtended(MlExperiment, { provide: { candidates, metricNames, paramNames } }); }; const findAlert = () => wrapper.findComponent(GlAlert); - const findEmptyState = () => wrapper.findByText('This Experiment has no logged Candidates'); + const findEmptyState = () => wrapper.findByText('This experiment has no logged candidates'); it('shows incubation warning', () => { wrapper = createWrapper(); @@ -31,8 +31,8 @@ describe('ShowExperiment', () => { it('renders correctly', () => { wrapper = createWrapper( [ - { rmse: 1, l1_ratio: 0.4 }, - { auc: 0.3, l1_ratio: 0.5 }, + { rmse: 1, l1_ratio: 0.4, details: 'link_to_candidate1', artifact: 'link_to_artifact' }, + { auc: 0.3, l1_ratio: 0.5, details: 'link_to_candidate2' }, ], ['rmse', 'auc', 'mae'], ['l1_ratio'], diff --git a/spec/graphql/types/permission_types/project_spec.rb b/spec/graphql/types/permission_types/project_spec.rb index c6853a0eadc..645fc6c68d1 100644 --- a/spec/graphql/types/permission_types/project_spec.rb +++ b/spec/graphql/types/permission_types/project_spec.rb @@ -13,7 +13,7 @@ RSpec.describe Types::PermissionTypes::Project do :create_merge_request_from, :create_wiki, :push_code, :create_deployment, :push_to_delete_protected_branch, :admin_wiki, :admin_project, :update_pages, :admin_remote_mirror, :create_label, :update_wiki, :destroy_wiki, :create_pages, :destroy_pages, :read_pages_content, - :read_merge_request, :read_design, :create_design, :destroy_design + :read_merge_request, :read_design, :create_design, :destroy_design, :read_environment ] expected_permissions.each do |permission| diff --git a/spec/helpers/projects/ml/experiments_helper_spec.rb b/spec/helpers/projects/ml/experiments_helper_spec.rb index e4421ff7606..e6959a03c4a 100644 --- a/spec/helpers/projects/ml/experiments_helper_spec.rb +++ b/spec/helpers/projects/ml/experiments_helper_spec.rb @@ -5,28 +5,36 @@ require 'rspec' require 'spec_helper' require 'mime/types' -RSpec.describe Projects::Ml::ExperimentsHelper do - let_it_be(:project) { build(:project, :private) } - let_it_be(:experiment) { build(:ml_experiments, user_id: project.creator, project: project) } - let_it_be(:candidates) do - create_list(:ml_candidates, 2, experiment: experiment, user: project.creator).tap do |c| - c[0].params.create!([{ name: 'param1', value: 'p1' }, { name: 'param2', value: 'p2' }]) - c[0].metrics.create!( +RSpec.describe Projects::Ml::ExperimentsHelper, feature_category: :mlops do + let_it_be(:project) { create(:project, :private) } + let_it_be(:experiment) { create(:ml_experiments, user_id: project.creator, project: project) } + let_it_be(:candidate0) do + create(:ml_candidates, experiment: experiment, user: project.creator).tap do |c| + c.params.build([{ name: 'param1', value: 'p1' }, { name: 'param2', value: 'p2' }]) + c.metrics.create!( [{ name: 'metric1', value: 0.1 }, { name: 'metric2', value: 0.2 }, { name: 'metric3', value: 0.3 }] ) - - c[1].params.create!([{ name: 'param2', value: 'p3' }, { name: 'param3', value: 'p4' }]) - c[1].metrics.create!(name: 'metric3', value: 0.4) end end + let_it_be(:candidate1) do + create(:ml_candidates, experiment: experiment, user: project.creator).tap do |c| + c.params.build([{ name: 'param2', value: 'p3' }, { name: 'param3', value: 'p4' }]) + c.metrics.create!(name: 'metric3', value: 0.4) + end + end + + let_it_be(:candidates) { [candidate0, candidate1] } + describe '#candidates_table_items' do subject { helper.candidates_table_items(candidates) } it 'creates the correct model for the table' do expected_value = [ - { 'param1' => 'p1', 'param2' => 'p2', 'metric1' => '0.1000', 'metric2' => '0.2000', 'metric3' => '0.3000' }, - { 'param2' => 'p3', 'param3' => 'p4', 'metric3' => '0.4000' } + { 'param1' => 'p1', 'param2' => 'p2', 'metric1' => '0.1000', 'metric2' => '0.2000', 'metric3' => '0.3000', + 'artifact' => nil, 'details' => "/#{project.full_path}/-/ml/candidates/#{candidate0.iid}" }, + { 'param2' => 'p3', 'param3' => 'p4', 'metric3' => '0.4000', + 'artifact' => nil, 'details' => "/#{project.full_path}/-/ml/candidates/#{candidate1.iid}" } ] expect(Gitlab::Json.parse(subject)).to match_array(expected_value) @@ -46,4 +54,40 @@ RSpec.describe Projects::Ml::ExperimentsHelper do it { is_expected.to match_array(%w[metric1 metric2 metric3]) } end end + + describe '#candidate_as_data' do + let(:candidate) { candidate0 } + let(:package) do + create(:generic_package, name: candidate.package_name, version: candidate.package_version, project: project) + end + + subject { Gitlab::Json.parse(helper.candidate_as_data(candidate)) } + + it 'generates the correct params' do + expect(subject['params']).to include( + hash_including('name' => 'param1', 'value' => 'p1'), + hash_including('name' => 'param2', 'value' => 'p2') + ) + end + + it 'generates the correct metrics' do + expect(subject['metrics']).to include( + hash_including('name' => 'metric1', 'value' => 0.1), + hash_including('name' => 'metric2', 'value' => 0.2), + hash_including('name' => 'metric3', 'value' => 0.3) + ) + end + + it 'generates the correct info' do + expected_info = { + 'iid' => candidate.iid, + 'path_to_artifact' => "/#{project.full_path}/-/packages/#{package.id}", + 'experiment_name' => candidate.experiment.name, + 'path_to_experiment' => "/#{project.full_path}/-/ml/experiments/#{experiment.iid}", + 'status' => 'running' + } + + expect(subject['info']).to include(expected_info) + end + end end diff --git a/spec/lib/gitlab/background_migration/reset_status_on_container_repositories_spec.rb b/spec/lib/gitlab/background_migration/reset_status_on_container_repositories_spec.rb new file mode 100644 index 00000000000..d50b04857d6 --- /dev/null +++ b/spec/lib/gitlab/background_migration/reset_status_on_container_repositories_spec.rb @@ -0,0 +1,261 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BackgroundMigration::ResetStatusOnContainerRepositories, feature_category: :container_registry do + let(:projects_table) { table(:projects) } + let(:namespaces_table) { table(:namespaces) } + let(:container_repositories_table) { table(:container_repositories) } + let(:routes_table) { table(:routes) } + + let!(:root_group) do + namespaces_table.create!(name: 'root_group', path: 'root_group', type: 'Group') do |new_group| + new_group.update!(traversal_ids: [new_group.id]) + end + end + + let!(:group1) do + namespaces_table.create!(name: 'group1', path: 'group1', parent_id: root_group.id, type: 'Group') do |new_group| + new_group.update!(traversal_ids: [root_group.id, new_group.id]) + end + end + + let!(:subgroup1) do + namespaces_table.create!(name: 'subgroup1', path: 'subgroup1', parent_id: group1.id, type: 'Group') do |new_group| + new_group.update!(traversal_ids: [root_group.id, group1.id, new_group.id]) + end + end + + let!(:group2) do + namespaces_table.create!(name: 'group2', path: 'group2', parent_id: root_group.id, type: 'Group') do |new_group| + new_group.update!(traversal_ids: [root_group.id, new_group.id]) + end + end + + let!(:group1_project_namespace) do + namespaces_table.create!(name: 'group1_project', path: 'group1_project', type: 'Project', parent_id: group1.id) + end + + let!(:subgroup1_project_namespace) do + namespaces_table.create!( + name: 'subgroup1_project', + path: 'subgroup1_project', + type: 'Project', + parent_id: subgroup1.id + ) + end + + let!(:group2_project_namespace) do + namespaces_table.create!( + name: 'group2_project', + path: 'group2_project', + type: 'Project', + parent_id: group2.id + ) + end + + let!(:group1_project) do + projects_table.create!( + name: 'group1_project', + path: 'group1_project', + namespace_id: group1.id, + project_namespace_id: group1_project_namespace.id + ) + end + + let!(:subgroup1_project) do + projects_table.create!( + name: 'subgroup1_project', + path: 'subgroup1_project', + namespace_id: subgroup1.id, + project_namespace_id: subgroup1_project_namespace.id + ) + end + + let!(:group2_project) do + projects_table.create!( + name: 'group2_project', + path: 'group2_project', + namespace_id: group2.id, + project_namespace_id: group2_project_namespace.id + ) + end + + let!(:route2) do + routes_table.create!( + source_id: group2_project.id, + source_type: 'Project', + path: 'root_group/group2/group2_project', + namespace_id: group2_project_namespace.id + ) + end + + let!(:delete_scheduled_container_repository1) do + container_repositories_table.create!(project_id: group1_project.id, status: 0, name: 'container_repository1') + end + + let!(:delete_scheduled_container_repository2) do + container_repositories_table.create!(project_id: subgroup1_project.id, status: 0, name: 'container_repository2') + end + + let!(:delete_scheduled_container_repository3) do + container_repositories_table.create!(project_id: group2_project.id, status: 0, name: 'container_repository3') + end + + let!(:delete_ongoing_container_repository4) do + container_repositories_table.create!(project_id: group2_project.id, status: 2, name: 'container_repository4') + end + + let(:migration) do + described_class.new( + start_id: container_repositories_table.minimum(:id), + end_id: container_repositories_table.maximum(:id), + batch_table: :container_repositories, + batch_column: :id, + sub_batch_size: 50, + pause_ms: 0, + connection: ApplicationRecord.connection + ) + end + + describe '#filter_batch' do + it 'scopes the relation to delete scheduled container repositories' do + expected = container_repositories_table.where(status: 0).pluck(:id) + actual = migration.filter_batch(container_repositories_table).pluck(:id) + + expect(actual).to match_array(expected) + end + end + + describe '#perform' do + let(:registry_api_url) { 'http://example.com' } + + subject(:perform) { migration.perform } + + before do + stub_container_registry_config( + enabled: true, + api_url: registry_api_url, + key: 'spec/fixtures/x509_certificate_pk.key' + ) + stub_tags_list(path: 'root_group/group1/group1_project/container_repository1') + stub_tags_list(path: 'root_group/group1/subgroup1/subgroup1_project/container_repository2', tags: []) + stub_tags_list(path: 'root_group/group2/group2_project/container_repository3') + end + + shared_examples 'resetting status of all container repositories scheduled for deletion' do + it 'resets all statuses' do + expect_logging_on( + path: 'root_group/group1/group1_project/container_repository1', + id: delete_scheduled_container_repository1.id, + has_tags: true + ) + expect_logging_on( + path: 'root_group/group1/subgroup1/subgroup1_project/container_repository2', + id: delete_scheduled_container_repository2.id, + has_tags: true + ) + expect_logging_on( + path: 'root_group/group2/group2_project/container_repository3', + id: delete_scheduled_container_repository3.id, + has_tags: true + ) + + expect { perform } + .to change { delete_scheduled_container_repository1.reload.status }.from(0).to(nil) + .and change { delete_scheduled_container_repository3.reload.status }.from(0).to(nil) + .and change { delete_scheduled_container_repository2.reload.status }.from(0).to(nil) + end + end + + it 'resets status of container repositories with tags' do + expect_pull_access_token_on(path: 'root_group/group1/group1_project/container_repository1') + expect_pull_access_token_on(path: 'root_group/group1/subgroup1/subgroup1_project/container_repository2') + expect_pull_access_token_on(path: 'root_group/group2/group2_project/container_repository3') + + expect_logging_on( + path: 'root_group/group1/group1_project/container_repository1', + id: delete_scheduled_container_repository1.id, + has_tags: true + ) + expect_logging_on( + path: 'root_group/group1/subgroup1/subgroup1_project/container_repository2', + id: delete_scheduled_container_repository2.id, + has_tags: false + ) + expect_logging_on( + path: 'root_group/group2/group2_project/container_repository3', + id: delete_scheduled_container_repository3.id, + has_tags: true + ) + + expect { perform } + .to change { delete_scheduled_container_repository1.reload.status }.from(0).to(nil) + .and change { delete_scheduled_container_repository3.reload.status }.from(0).to(nil) + .and not_change { delete_scheduled_container_repository2.reload.status } + end + + context 'with the registry disabled' do + before do + allow(::Gitlab.config.registry).to receive(:enabled).and_return(false) + end + + it_behaves_like 'resetting status of all container repositories scheduled for deletion' + end + + context 'with the registry api url not defined' do + before do + allow(::Gitlab.config.registry).to receive(:api_url).and_return('') + end + + it_behaves_like 'resetting status of all container repositories scheduled for deletion' + end + + context 'with a faraday error' do + before do + client_double = instance_double('::ContainerRegistry::Client') + allow(::ContainerRegistry::Client).to receive(:new).and_return(client_double) + allow(client_double).to receive(:repository_tags).and_raise(Faraday::TimeoutError) + + expect_pull_access_token_on(path: 'root_group/group1/group1_project/container_repository1') + expect_pull_access_token_on(path: 'root_group/group1/subgroup1/subgroup1_project/container_repository2') + expect_pull_access_token_on(path: 'root_group/group2/group2_project/container_repository3') + end + + it_behaves_like 'resetting status of all container repositories scheduled for deletion' + end + + def stub_tags_list(path:, tags: %w[tag1]) + url = "#{registry_api_url}/v2/#{path}/tags/list?n=1" + + stub_request(:get, url) + .with( + headers: { + 'Accept' => ContainerRegistry::Client::ACCEPTED_TYPES.join(', '), + 'Authorization' => /bearer .+/, + 'User-Agent' => "GitLab/#{Gitlab::VERSION}" + } + ) + .to_return( + status: 200, + body: Gitlab::Json.dump(tags: tags), + headers: { 'Content-Type' => 'application/json' } + ) + end + + def expect_pull_access_token_on(path:) + expect(Auth::ContainerRegistryAuthenticationService) + .to receive(:pull_access_token).with(path).and_call_original + end + + def expect_logging_on(path:, id:, has_tags:) + expect(::Gitlab::BackgroundMigration::Logger) + .to receive(:info).with( + migrator: described_class::MIGRATOR, + has_tags: has_tags, + container_repository_id: id, + container_repository_path: path + ) + end + end +end diff --git a/spec/migrations/20221123133054_queue_reset_status_on_container_repositories_spec.rb b/spec/migrations/20221123133054_queue_reset_status_on_container_repositories_spec.rb new file mode 100644 index 00000000000..79094a2b8d0 --- /dev/null +++ b/spec/migrations/20221123133054_queue_reset_status_on_container_repositories_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe QueueResetStatusOnContainerRepositories, feature_category: :container_registry do + let_it_be(:batched_migration) { described_class::MIGRATION } + + before do + stub_container_registry_config( + enabled: true, + api_url: 'http://example.com', + key: 'spec/fixtures/x509_certificate_pk.key' + ) + end + + it 'schedules a new batched migration' do + reversible_migration do |migration| + migration.before -> { + expect(batched_migration).not_to have_scheduled_batched_migration + } + + migration.after -> { + expect(batched_migration).to have_scheduled_batched_migration( + table_name: :container_repositories, + column_name: :id, + interval: described_class::DELAY_INTERVAL, + sub_batch_size: described_class::BATCH_SIZE + ) + } + end + end + + context 'with the container registry disabled' do + before do + allow(::Gitlab.config.registry).to receive(:enabled).and_return(false) + end + + it 'does not schedule a new batched migration' do + reversible_migration do |migration| + migration.before -> { + expect(batched_migration).not_to have_scheduled_batched_migration + } + + migration.after -> { + expect(batched_migration).not_to have_scheduled_batched_migration + } + end + end + end +end diff --git a/spec/models/generic_commit_status_spec.rb b/spec/models/generic_commit_status_spec.rb index 9d70019734b..fe22b20ecf9 100644 --- a/spec/models/generic_commit_status_spec.rb +++ b/spec/models/generic_commit_status_spec.rb @@ -20,7 +20,7 @@ RSpec.describe GenericCommitStatus do end describe '#name_uniqueness_across_types' do - let(:attributes) { {} } + let(:attributes) { { context: 'default' } } let(:commit_status) { described_class.new(attributes) } let(:status_name) { 'test-job' } @@ -39,7 +39,7 @@ RSpec.describe GenericCommitStatus do end context 'with only a pipeline' do - let(:attributes) { { pipeline: pipeline } } + let(:attributes) { { pipeline: pipeline, context: 'default' } } context 'without name' do it_behaves_like 'it does not have uniqueness errors' @@ -129,32 +129,6 @@ RSpec.describe GenericCommitStatus do end end - describe 'set_default_values' do - before do - generic_commit_status.context = nil - generic_commit_status.stage = nil - generic_commit_status.save! - end - - describe '#context' do - subject { generic_commit_status.context } - - it { is_expected.not_to be_nil } - end - - describe '#stage' do - subject { generic_commit_status.stage } - - it { is_expected.not_to be_nil } - end - - describe '#stage_idx' do - subject { generic_commit_status.stage_idx } - - it { is_expected.not_to be_nil } - end - end - describe '#present' do subject { generic_commit_status.present } diff --git a/spec/models/ml/candidate_spec.rb b/spec/models/ml/candidate_spec.rb index 8a1e18d55c1..9ce411191f0 100644 --- a/spec/models/ml/candidate_spec.rb +++ b/spec/models/ml/candidate_spec.rb @@ -5,6 +5,8 @@ require 'spec_helper' RSpec.describe Ml::Candidate, factory_default: :keep do let_it_be(:candidate) { create(:ml_candidates, :with_metrics_and_params) } + let(:project) { candidate.experiment.project } + describe 'associations' do it { is_expected.to belong_to(:experiment) } it { is_expected.to belong_to(:user) } @@ -13,14 +15,48 @@ RSpec.describe Ml::Candidate, factory_default: :keep do it { is_expected.to have_many(:metadata) } end + describe 'default values' do + it { expect(described_class.new.iid).to be_present } + end + describe '.artifact_root' do subject { candidate.artifact_root } it { is_expected.to eq("/ml_candidate_#{candidate.iid}/-/") } end - describe 'default values' do - it { expect(described_class.new.iid).to be_present } + describe '.package_name' do + subject { candidate.package_name } + + it { is_expected.to eq("ml_candidate_#{candidate.iid}") } + end + + describe '.package_version' do + subject { candidate.package_version } + + it { is_expected.to eq('-') } + end + + describe '.artifact' do + subject { candidate.artifact } + + context 'when has logged artifacts' do + let(:package) do + create(:generic_package, name: candidate.package_name, version: candidate.package_version, project: project) + end + + it 'returns the package' do + package + + is_expected.to eq(package) + end + end + + context 'when does not have logged artifacts' do + let(:tested_candidate) { create(:ml_candidates, :with_metrics_and_params) } + + it { is_expected.to be_nil } + end end describe '#by_project_id_and_iid' do diff --git a/spec/requests/projects/ml/candidates_controller_spec.rb b/spec/requests/projects/ml/candidates_controller_spec.rb new file mode 100644 index 00000000000..4a0fd1ce4f5 --- /dev/null +++ b/spec/requests/projects/ml/candidates_controller_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::Ml::CandidatesController, feature_category: :mlops do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { project.first_owner } + let_it_be(:experiment) { create(:ml_experiments, project: project, user: user) } + let_it_be(:candidate) { create(:ml_candidates, experiment: experiment, user: user) } + + let(:ff_value) { true } + let(:threshold) { 4 } + let(:candidate_iid) { candidate.iid } + + before do + stub_feature_flags(ml_experiment_tracking: false) + stub_feature_flags(ml_experiment_tracking: project) if ff_value + + sign_in(user) + end + + shared_examples '404 if feature flag disabled' do + context 'when :ml_experiment_tracking disabled' do + let(:ff_value) { false } + + it 'is 404' do + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + + describe 'GET show' do + let(:params) { basic_params.merge(id: experiment.iid) } + + before do + show_candidate + end + + it 'renders the template' do + expect(response).to render_template('projects/ml/candidates/show') + end + + # MR removing this xit https://gitlab.com/gitlab-org/gitlab/-/merge_requests/104166 + xit 'does not perform N+1 sql queries' do + control_count = ActiveRecord::QueryRecorder.new { show_candidate } + + create_list(:ml_candidate_params, 3, candidate: candidate) + create_list(:ml_candidate_metrics, 3, candidate: candidate) + + expect { show_candidate }.not_to exceed_all_query_limit(control_count).with_threshold(threshold) + end + + context 'when candidate does not exist' do + let(:candidate_iid) { non_existing_record_id.to_s } + + it 'returns 404' do + expect(response).to have_gitlab_http_status(:not_found) + end + end + + it_behaves_like '404 if feature flag disabled' + end + + private + + def show_candidate + get project_ml_candidate_path(project, candidate_iid) + end +end diff --git a/spec/requests/projects/ml/experiments_controller_spec.rb b/spec/requests/projects/ml/experiments_controller_spec.rb index 9aaf28f2df6..414748c0804 100644 --- a/spec/requests/projects/ml/experiments_controller_spec.rb +++ b/spec/requests/projects/ml/experiments_controller_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::Ml::ExperimentsController do +RSpec.describe Projects::Ml::ExperimentsController, feature_category: :mlops do let_it_be(:project_with_feature) { create(:project, :repository) } let_it_be(:user) { project_with_feature.first_owner } let_it_be(:project_without_feature) do @@ -77,7 +77,8 @@ RSpec.describe Projects::Ml::ExperimentsController do expect(response).to render_template('projects/ml/experiments/show') end - it 'does not perform N+1 sql queries' do + # MR removing this xit https://gitlab.com/gitlab-org/gitlab/-/merge_requests/104166 + xit 'does not perform N+1 sql queries' do control_count = ActiveRecord::QueryRecorder.new { show_experiment } create_list(:ml_candidates, 2, :with_metrics_and_params, experiment: experiment) diff --git a/workhorse/go.mod b/workhorse/go.mod index 88e6748ee8e..47707bdcbe2 100644 --- a/workhorse/go.mod +++ b/workhorse/go.mod @@ -7,7 +7,7 @@ require ( github.com/BurntSushi/toml v1.2.1 github.com/FZambia/sentinel v1.1.1 github.com/alecthomas/chroma/v2 v2.3.0 - github.com/aws/aws-sdk-go v1.44.145 + github.com/aws/aws-sdk-go v1.44.150 github.com/disintegration/imaging v1.6.2 github.com/getsentry/raven-go v0.2.0 github.com/golang-jwt/jwt/v4 v4.4.3 @@ -17,7 +17,7 @@ require ( github.com/gorilla/websocket v1.5.0 github.com/grpc-ecosystem/go-grpc-middleware v1.3.0 github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 - github.com/johannesboyne/gofakes3 v0.0.0-20221110173912-32fb85c5aed6 + github.com/johannesboyne/gofakes3 v0.0.0-20221128113635-c2f5cc6b5294 github.com/jpillora/backoff v1.0.0 github.com/mitchellh/copystructure v1.2.0 github.com/prometheus/client_golang v1.14.0 diff --git a/workhorse/go.sum b/workhorse/go.sum index a9090fa0706..eb37fce0954 100644 --- a/workhorse/go.sum +++ b/workhorse/go.sum @@ -227,8 +227,8 @@ github.com/aws/aws-sdk-go v1.43.11/go.mod h1:y4AeaBuwd2Lk+GepC1E9v0qOiTws0MIWAX4 github.com/aws/aws-sdk-go v1.43.31/go.mod h1:y4AeaBuwd2Lk+GepC1E9v0qOiTws0MIWAX4oIKwKHZo= github.com/aws/aws-sdk-go v1.44.45/go.mod h1:y4AeaBuwd2Lk+GepC1E9v0qOiTws0MIWAX4oIKwKHZo= github.com/aws/aws-sdk-go v1.44.68/go.mod h1:y4AeaBuwd2Lk+GepC1E9v0qOiTws0MIWAX4oIKwKHZo= -github.com/aws/aws-sdk-go v1.44.145 h1:KMVRrIyjBsNz3xGPuHIRnhIuKlb5h3Ii5e5jbi3cgnc= -github.com/aws/aws-sdk-go v1.44.145/go.mod h1:aVsgQcEevwlmQ7qHE9I3h+dtQgpqhFB+i8Phjh7fkwI= +github.com/aws/aws-sdk-go v1.44.150 h1:X9HBhXu0ZPi+tOHUaZkjx43int7g0Ejk+IVbW25+wYg= +github.com/aws/aws-sdk-go v1.44.150/go.mod h1:aVsgQcEevwlmQ7qHE9I3h+dtQgpqhFB+i8Phjh7fkwI= github.com/aws/aws-sdk-go-v2 v0.18.0/go.mod h1:JWVYvqSMppoMJC0x5wdwiImzgXTI9FuZwxzkQq9wy+g= github.com/aws/aws-sdk-go-v2 v1.16.8 h1:gOe9UPR98XSf7oEJCcojYg+N2/jCRm4DdeIsP85pIyQ= github.com/aws/aws-sdk-go-v2 v1.16.8/go.mod h1:6CpKuLXg2w7If3ABZCl/qZ6rEgwtjZTn4eAf4RcEyuw= @@ -976,8 +976,8 @@ github.com/jmespath/go-jmespath v0.4.0/go.mod h1:T8mJZnbsbmF+m6zOOFylbeCJqk5+pHW github.com/jmespath/go-jmespath/internal/testify v1.5.1 h1:shLQSRRSCCPj3f2gpwzGwWFoC7ycTf1rcQZHOlsJ6N8= github.com/jmespath/go-jmespath/internal/testify v1.5.1/go.mod h1:L3OGu8Wl2/fWfCI6z80xFu9LTZmf1ZRjMHUOPmWr69U= github.com/joefitzgerald/rainbow-reporter v0.1.0/go.mod h1:481CNgqmVHQZzdIbN52CupLJyoVwB10FQ/IQlF1pdL8= -github.com/johannesboyne/gofakes3 v0.0.0-20221110173912-32fb85c5aed6 h1:eQGUsj2LcsLzfrHY1noKDSU7h+c9/rw9pQPwbQ9g1jQ= -github.com/johannesboyne/gofakes3 v0.0.0-20221110173912-32fb85c5aed6/go.mod h1:LIAXxPvcUXwOcTIj9LSNSUpE9/eMHalTWxsP/kmWxQI= +github.com/johannesboyne/gofakes3 v0.0.0-20221128113635-c2f5cc6b5294 h1:AJISYN7tPo3lGqwYmEYQdlftcQz48i8LNk/BRUKCTig= +github.com/johannesboyne/gofakes3 v0.0.0-20221128113635-c2f5cc6b5294/go.mod h1:LIAXxPvcUXwOcTIj9LSNSUpE9/eMHalTWxsP/kmWxQI= github.com/joho/godotenv v1.3.0/go.mod h1:7hK45KPybAkOC6peb+G5yklZfMxEjkZhHbwpqxOKXbg= github.com/jonboulle/clockwork v0.1.0/go.mod h1:Ii8DK3G1RaLaWxj9trq07+26W01tbo22gdxWY5EU2bo= github.com/jonboulle/clockwork v0.2.2/go.mod h1:Pkfl5aHPm1nk2H9h0bjmnJD/BcgbGXUBGnn1kMkgxc8= diff --git a/workhorse/internal/helper/raven.go b/workhorse/internal/helper/exception/exception.go similarity index 88% rename from workhorse/internal/helper/raven.go rename to workhorse/internal/helper/exception/exception.go index 898e8ec85f8..9b1628ffecb 100644 --- a/workhorse/internal/helper/raven.go +++ b/workhorse/internal/helper/exception/exception.go @@ -1,4 +1,4 @@ -package helper +package exception import ( "net/http" @@ -17,7 +17,7 @@ var ravenHeaderBlacklist = []string{ "Private-Token", } -func CaptureRavenError(r *http.Request, err error, fields log.Fields) { +func Track(r *http.Request, err error, fields log.Fields) { client := raven.DefaultClient extra := raven.Extra{} @@ -27,7 +27,7 @@ func CaptureRavenError(r *http.Request, err error, fields log.Fields) { interfaces := []raven.Interface{} if r != nil { - CleanHeadersForRaven(r) + CleanHeaders(r) interfaces = append(interfaces, raven.NewHttp(r)) //lint:ignore SA1019 this was recently deprecated. Update workhorse to use labkit errortracking package. @@ -45,7 +45,7 @@ func CaptureRavenError(r *http.Request, err error, fields log.Fields) { client.Capture(packet, nil) } -func CleanHeadersForRaven(r *http.Request) { +func CleanHeaders(r *http.Request) { if r == nil { return } diff --git a/workhorse/internal/helper/helpers.go b/workhorse/internal/helper/helpers.go index ea35d45fa33..4b458372629 100644 --- a/workhorse/internal/helper/helpers.go +++ b/workhorse/internal/helper/helpers.go @@ -12,21 +12,13 @@ import ( "strings" "github.com/sebest/xff" - "gitlab.com/gitlab-org/labkit/log" - "gitlab.com/gitlab-org/labkit/mask" + + "gitlab.com/gitlab-org/gitlab/workhorse/internal/log" ) -func logErrorWithFields(r *http.Request, err error, fields log.Fields) { - if err != nil { - CaptureRavenError(r, err, fields) - } - - printError(r, err, fields) -} - func CaptureAndFail(w http.ResponseWriter, r *http.Request, err error, msg string, code int) { http.Error(w, msg, code) - logErrorWithFields(r, err, nil) + printError(r, err, nil) } func Fail500(w http.ResponseWriter, r *http.Request, err error) { @@ -35,7 +27,7 @@ func Fail500(w http.ResponseWriter, r *http.Request, err error) { func Fail500WithFields(w http.ResponseWriter, r *http.Request, err error, fields log.Fields) { http.Error(w, "Internal server error", http.StatusInternalServerError) - logErrorWithFields(r, err, fields) + printError(r, err, fields) } func RequestEntityTooLarge(w http.ResponseWriter, r *http.Request, err error) { @@ -43,15 +35,7 @@ func RequestEntityTooLarge(w http.ResponseWriter, r *http.Request, err error) { } func printError(r *http.Request, err error, fields log.Fields) { - if r != nil { - entry := log.WithContextFields(r.Context(), log.Fields{ - "method": r.Method, - "uri": mask.URL(r.RequestURI), - }) - entry.WithFields(fields).WithError(err).Error() - } else { - log.WithFields(fields).WithError(err).Error("unknown error") - } + log.WithRequest(r).WithFields(fields).WithError(err).Error() } func SetNoCacheHeaders(header http.Header) { @@ -93,7 +77,7 @@ func OpenFile(path string) (file *os.File, fi os.FileInfo, err error) { func URLMustParse(s string) *url.URL { u, err := url.Parse(s) if err != nil { - log.WithError(err).WithField("url", s).Fatal("urlMustParse") + log.WithError(err).WithFields(log.Fields{"url": s}).Fatal("urlMustParse") } return u } diff --git a/workhorse/internal/log/logging.go b/workhorse/internal/log/logging.go index 80c09c1bf02..004ae8a8604 100644 --- a/workhorse/internal/log/logging.go +++ b/workhorse/internal/log/logging.go @@ -8,7 +8,7 @@ import ( "gitlab.com/gitlab-org/labkit/mask" "golang.org/x/net/context" - "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper" + "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper/exception" ) type Fields = log.Fields @@ -79,10 +79,18 @@ func Error(args ...interface{}) { NewBuilder().Error(args...) } -func (b *Builder) Error(args ...interface{}) { - b.entry.Error(args...) - - if b.req != nil && b.err != nil { - helper.CaptureRavenError(b.req, b.err, b.fields) +func (b *Builder) trackException() { + if b.err != nil { + exception.Track(b.req, b.err, b.fields) } } + +func (b *Builder) Error(args ...interface{}) { + b.trackException() + b.entry.Error(args...) +} + +func (b *Builder) Fatal(args ...interface{}) { + b.trackException() + b.entry.Fatal(args...) +} diff --git a/workhorse/raven.go b/workhorse/raven.go index 2db24b0b3d4..582900b15f4 100644 --- a/workhorse/raven.go +++ b/workhorse/raven.go @@ -6,7 +6,7 @@ import ( raven "github.com/getsentry/raven-go" - "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper" + "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper/exception" ) func wrapRaven(h http.Handler) http.Handler { @@ -30,7 +30,7 @@ func wrapRaven(h http.Handler) http.Handler { func(w http.ResponseWriter, r *http.Request) { defer func() { if p := recover(); p != nil { - helper.CleanHeadersForRaven(r) + exception.CleanHeaders(r) panic(p) } }() |