From 08489a6db8ddff0794f9beaf770930803dc7bdca Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Thu, 1 Dec 2022 15:07:35 +0000 Subject: Add latest changes from gitlab-org/gitlab@master --- .../ide/components/new_dropdown/index.vue | 2 +- .../ide/components/new_dropdown/upload.vue | 4 +- app/assets/javascripts/ide/lib/editor_options.js | 1 - .../experiment_tracking/components/experiment.vue | 36 --- .../components/incubation_alert.vue | 6 +- .../components/ml_candidate.vue | 94 +++++++ .../components/ml_experiment.vue | 59 +++++ .../pages/projects/ml/candidates/show/index.js | 27 ++ .../pages/projects/ml/experiments/show/index.js | 4 +- .../stylesheets/pages/ml_experiment_tracking.scss | 6 + .../projects/ml/candidates_controller.rb | 23 ++ .../projects/ml/experiments_controller.rb | 1 - app/graphql/types/permission_types/project.rb | 3 +- app/helpers/projects/ml/experiments_helper.rb | 40 ++- app/models/generic_commit_status.rb | 8 - app/models/ml/candidate.rb | 16 +- app/views/projects/ml/candidates/show.html.haml | 7 + config/routes/project.rb | 1 + ...queue_reset_status_on_container_repositories.rb | 25 ++ db/schema_migrations/20221123133054 | 1 + .../postgresql/replication_and_failover.md | 2 +- doc/api/graphql/reference/index.md | 1 + doc/api/invitations.md | 2 +- doc/development/documentation/topic_types/task.md | 6 + .../fe_guide/merge_request_widget_extensions.md | 7 +- .../offline_deployments/index.md | 2 +- .../ml/experiment_tracking/img/candidate_v15_7.png | Bin 0 -> 106770 bytes .../ml/experiment_tracking/img/candidates.png | Bin 62281 -> 0 bytes .../experiment_tracking/img/candidates_v15_7.png | Bin 0 -> 153995 bytes .../ml/experiment_tracking/img/experiments.png | Bin 45022 -> 0 bytes .../experiment_tracking/img/experiments_v15_7.png | Bin 0 -> 76730 bytes doc/user/project/ml/experiment_tracking/index.md | 16 +- lib/api/commit_statuses.rb | 3 +- .../reset_status_on_container_repositories.rb | 139 ++++++++++ .../Security/Secure-Binaries.gitlab-ci.yml | 4 +- locale/gitlab.pot | 23 +- spec/frontend/ide/lib/common/model_spec.js | 9 +- .../__snapshots__/experiment_spec.js.snap | 223 ---------------- .../__snapshots__/ml_candidate_spec.js.snap | 233 +++++++++++++++++ .../__snapshots__/ml_experiment_spec.js.snap | 284 +++++++++++++++++++++ .../components/experiment_spec.js | 44 ---- .../components/incubation_alert_spec.js | 2 +- .../components/ml_candidate_spec.js | 43 ++++ .../components/ml_experiment_spec.js | 44 ++++ .../graphql/types/permission_types/project_spec.rb | 2 +- .../helpers/projects/ml/experiments_helper_spec.rb | 66 ++++- .../reset_status_on_container_repositories_spec.rb | 261 +++++++++++++++++++ ..._reset_status_on_container_repositories_spec.rb | 51 ++++ spec/models/generic_commit_status_spec.rb | 30 +-- spec/models/ml/candidate_spec.rb | 40 ++- .../projects/ml/candidates_controller_spec.rb | 69 +++++ .../projects/ml/experiments_controller_spec.rb | 5 +- workhorse/go.mod | 4 +- workhorse/go.sum | 8 +- workhorse/internal/helper/exception/exception.go | 58 +++++ workhorse/internal/helper/helpers.go | 28 +- workhorse/internal/helper/raven.go | 58 ----- workhorse/internal/log/logging.go | 16 +- workhorse/raven.go | 4 +- 59 files changed, 1667 insertions(+), 484 deletions(-) delete mode 100644 app/assets/javascripts/ml/experiment_tracking/components/experiment.vue create mode 100644 app/assets/javascripts/ml/experiment_tracking/components/ml_candidate.vue create mode 100644 app/assets/javascripts/ml/experiment_tracking/components/ml_experiment.vue create mode 100644 app/assets/javascripts/pages/projects/ml/candidates/show/index.js create mode 100644 app/controllers/projects/ml/candidates_controller.rb create mode 100644 app/views/projects/ml/candidates/show.html.haml create mode 100644 db/post_migrate/20221123133054_queue_reset_status_on_container_repositories.rb create mode 100644 db/schema_migrations/20221123133054 create mode 100644 doc/user/project/ml/experiment_tracking/img/candidate_v15_7.png delete mode 100644 doc/user/project/ml/experiment_tracking/img/candidates.png create mode 100644 doc/user/project/ml/experiment_tracking/img/candidates_v15_7.png delete mode 100644 doc/user/project/ml/experiment_tracking/img/experiments.png create mode 100644 doc/user/project/ml/experiment_tracking/img/experiments_v15_7.png create mode 100644 lib/gitlab/background_migration/reset_status_on_container_repositories.rb delete mode 100644 spec/frontend/ml/experiment_tracking/components/__snapshots__/experiment_spec.js.snap create mode 100644 spec/frontend/ml/experiment_tracking/components/__snapshots__/ml_candidate_spec.js.snap create mode 100644 spec/frontend/ml/experiment_tracking/components/__snapshots__/ml_experiment_spec.js.snap delete mode 100644 spec/frontend/ml/experiment_tracking/components/experiment_spec.js create mode 100644 spec/frontend/ml/experiment_tracking/components/ml_candidate_spec.js create mode 100644 spec/frontend/ml/experiment_tracking/components/ml_experiment_spec.js create mode 100644 spec/lib/gitlab/background_migration/reset_status_on_container_repositories_spec.rb create mode 100644 spec/migrations/20221123133054_queue_reset_status_on_container_repositories_spec.rb create mode 100644 spec/requests/projects/ml/candidates_controller_spec.rb create mode 100644 workhorse/internal/helper/exception/exception.go delete mode 100644 workhorse/internal/helper/raven.go diff --git a/app/assets/javascripts/ide/components/new_dropdown/index.vue b/app/assets/javascripts/ide/components/new_dropdown/index.vue index 9a529bdcee1..ea1dbee4669 100644 --- a/app/assets/javascripts/ide/components/new_dropdown/index.vue +++ b/app/assets/javascripts/ide/components/new_dropdown/index.vue @@ -80,7 +80,7 @@ export default { @click="createNewItem('blob')" /> -
    • +
  • diff --git a/app/assets/javascripts/ide/lib/editor_options.js b/app/assets/javascripts/ide/lib/editor_options.js index 525afcb2083..02424ca7328 100644 --- a/app/assets/javascripts/ide/lib/editor_options.js +++ b/app/assets/javascripts/ide/lib/editor_options.js @@ -27,7 +27,6 @@ export const defaultDiffEditorOptions = { }; export const defaultModelOptions = { - endOfLine: 0, insertFinalNewline: true, trimTrailingWhitespace: false, }; diff --git a/app/assets/javascripts/ml/experiment_tracking/components/experiment.vue b/app/assets/javascripts/ml/experiment_tracking/components/experiment.vue deleted file mode 100644 index 73cdfbc44b0..00000000000 --- a/app/assets/javascripts/ml/experiment_tracking/components/experiment.vue +++ /dev/null @@ -1,36 +0,0 @@ - - - diff --git a/app/assets/javascripts/ml/experiment_tracking/components/incubation_alert.vue b/app/assets/javascripts/ml/experiment_tracking/components/incubation_alert.vue index 51c1e935677..42f6394ed68 100644 --- a/app/assets/javascripts/ml/experiment_tracking/components/incubation_alert.vue +++ b/app/assets/javascripts/ml/experiment_tracking/components/incubation_alert.vue @@ -8,8 +8,8 @@ export default { contentLabel: __( 'GitLab incubates features to explore new use cases. These features are updated regularly, and support is limited', ), - learnMoreLabel: __('Learn More'), - feedbackLabel: __('Feedback and Updates'), + learnMoreLabel: __('Learn more'), + feedbackLabel: __('Feedback'), }, name: 'MlopsIncubationAlert', components: { GlAlert, GlLink }, @@ -37,7 +37,7 @@ export default { :title="$options.i18n.titleLabel" variant="warning" :primary-button-text="$options.i18n.feedbackLabel" - primary-button-link="https://gitlab.com/groups/gitlab-org/-/epics/8560" + primary-button-link="https://gitlab.com/gitlab-org/gitlab/-/issues/381660" @dismiss="dismissAlert" > {{ $options.i18n.contentLabel }} diff --git a/app/assets/javascripts/ml/experiment_tracking/components/ml_candidate.vue b/app/assets/javascripts/ml/experiment_tracking/components/ml_candidate.vue new file mode 100644 index 00000000000..5f54f24e24c --- /dev/null +++ b/app/assets/javascripts/ml/experiment_tracking/components/ml_candidate.vue @@ -0,0 +1,94 @@ + + + diff --git a/app/assets/javascripts/ml/experiment_tracking/components/ml_experiment.vue b/app/assets/javascripts/ml/experiment_tracking/components/ml_experiment.vue new file mode 100644 index 00000000000..f8e269d3b57 --- /dev/null +++ b/app/assets/javascripts/ml/experiment_tracking/components/ml_experiment.vue @@ -0,0 +1,59 @@ + + + diff --git a/app/assets/javascripts/pages/projects/ml/candidates/show/index.js b/app/assets/javascripts/pages/projects/ml/candidates/show/index.js new file mode 100644 index 00000000000..c1acef5ac13 --- /dev/null +++ b/app/assets/javascripts/pages/projects/ml/candidates/show/index.js @@ -0,0 +1,27 @@ +import Vue from 'vue'; +import MlCandidate from '~/ml/experiment_tracking/components/ml_candidate.vue'; + +const initShowCandidate = () => { + const element = document.querySelector('#js-show-ml-candidate'); + if (!element) { + return; + } + + const container = document.createElement('div'); + element.appendChild(container); + + const candidate = JSON.parse(element.dataset.candidate); + + // eslint-disable-next-line no-new + new Vue({ + el: container, + provide: { + candidate, + }, + render(h) { + return h(MlCandidate); + }, + }); +}; + +initShowCandidate(); diff --git a/app/assets/javascripts/pages/projects/ml/experiments/show/index.js b/app/assets/javascripts/pages/projects/ml/experiments/show/index.js index 0a9d9f4c987..97e436920c7 100644 --- a/app/assets/javascripts/pages/projects/ml/experiments/show/index.js +++ b/app/assets/javascripts/pages/projects/ml/experiments/show/index.js @@ -1,5 +1,5 @@ import Vue from 'vue'; -import ShowExperiment from '~/ml/experiment_tracking/components/experiment.vue'; +import MlExperiment from '~/ml/experiment_tracking/components/ml_experiment.vue'; const initShowExperiment = () => { const element = document.querySelector('#js-show-ml-experiment'); @@ -23,7 +23,7 @@ const initShowExperiment = () => { paramNames, }, render(h) { - return h(ShowExperiment); + return h(MlExperiment); }, }); }; diff --git a/app/assets/stylesheets/pages/ml_experiment_tracking.scss b/app/assets/stylesheets/pages/ml_experiment_tracking.scss index 2dff51cff92..c1582f2090b 100644 --- a/app/assets/stylesheets/pages/ml_experiment_tracking.scss +++ b/app/assets/stylesheets/pages/ml_experiment_tracking.scss @@ -14,3 +14,9 @@ color: $gl-text-color; } } + +table.candidate-details { + td { + padding: $gl-spacing-scale-3; + } +} diff --git a/app/controllers/projects/ml/candidates_controller.rb b/app/controllers/projects/ml/candidates_controller.rb new file mode 100644 index 00000000000..b702edb858e --- /dev/null +++ b/app/controllers/projects/ml/candidates_controller.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Projects + module Ml + class CandidatesController < ApplicationController + before_action :check_feature_flag + + feature_category :mlops + + def show + @candidate = ::Ml::Candidate.with_project_id_and_iid(@project.id, params['iid']) + + render_404 unless @candidate.present? + end + + private + + def check_feature_flag + render_404 unless Feature.enabled?(:ml_experiment_tracking, @project) + end + end + end +end diff --git a/app/controllers/projects/ml/experiments_controller.rb b/app/controllers/projects/ml/experiments_controller.rb index 749586791ac..c82a959d612 100644 --- a/app/controllers/projects/ml/experiments_controller.rb +++ b/app/controllers/projects/ml/experiments_controller.rb @@ -3,7 +3,6 @@ module Projects module Ml class ExperimentsController < ::Projects::ApplicationController - include Projects::Ml::ExperimentsHelper before_action :check_feature_flag feature_category :mlops diff --git a/app/graphql/types/permission_types/project.rb b/app/graphql/types/permission_types/project.rb index f6a5563d367..c833b512222 100644 --- a/app/graphql/types/permission_types/project.rb +++ b/app/graphql/types/permission_types/project.rb @@ -17,7 +17,8 @@ module Types :admin_wiki, :admin_project, :update_pages, :admin_remote_mirror, :create_label, :update_wiki, :destroy_wiki, :create_pages, :destroy_pages, :read_pages_content, :admin_operations, - :read_merge_request, :read_design, :create_design, :destroy_design + :read_merge_request, :read_design, :create_design, :destroy_design, + :read_environment permission_field :create_snippet diff --git a/app/helpers/projects/ml/experiments_helper.rb b/app/helpers/projects/ml/experiments_helper.rb index 29bd879859e..a67484e3d2f 100644 --- a/app/helpers/projects/ml/experiments_helper.rb +++ b/app/helpers/projects/ml/experiments_helper.rb @@ -9,7 +9,9 @@ module Projects items = candidates.map do |candidate| { **candidate.params.to_h { |p| [p.name, p.value] }, - **candidate.latest_metrics.to_h { |m| [m.name, number_with_precision(m.value, precision: 4)] } + **candidate.latest_metrics.to_h { |m| [m.name, number_with_precision(m.value, precision: 4)] }, + artifact: link_to_artifact(candidate), + details: link_to_details(candidate) } end @@ -19,6 +21,42 @@ module Projects def unique_logged_names(candidates, &selector) Gitlab::Json.generate(candidates.flat_map(&selector).map(&:name).uniq) end + + def candidate_as_data(candidate) + data = { + params: candidate.params, + metrics: candidate.latest_metrics, + info: { + iid: candidate.iid, + path_to_artifact: link_to_artifact(candidate), + experiment_name: candidate.experiment.name, + path_to_experiment: link_to_experiment(candidate), + status: candidate.status + } + } + + Gitlab::Json.generate(data) + end + + private + + def link_to_artifact(candidate) + artifact = candidate.artifact + + return unless artifact.present? + + project_package_path(candidate.experiment.project, artifact) + end + + def link_to_details(candidate) + project_ml_candidate_path(candidate.experiment.project, candidate.iid) + end + + def link_to_experiment(candidate) + experiment = candidate.experiment + + project_ml_experiment_path(experiment.project, experiment.iid) + end end end end diff --git a/app/models/generic_commit_status.rb b/app/models/generic_commit_status.rb index 6c8bfc35334..b02074849a1 100644 --- a/app/models/generic_commit_status.rb +++ b/app/models/generic_commit_status.rb @@ -3,8 +3,6 @@ class GenericCommitStatus < CommitStatus EXTERNAL_STAGE_IDX = 1_000_000 - before_validation :set_default_values - validates :target_url, addressable_url: true, length: { maximum: 255 }, allow_nil: true @@ -13,12 +11,6 @@ class GenericCommitStatus < CommitStatus # GitHub compatible API alias_attribute :context, :name - def set_default_values - self.context ||= 'default' - self.stage ||= 'external' - self.stage_idx ||= EXTERNAL_STAGE_IDX - end - def tags [:external] end diff --git a/app/models/ml/candidate.rb b/app/models/ml/candidate.rb index 56b468f8286..f24161d598f 100644 --- a/app/models/ml/candidate.rb +++ b/app/models/ml/candidate.rb @@ -19,7 +19,21 @@ module Ml scope :including_metrics_and_params, -> { includes(:latest_metrics, :params) } def artifact_root - "/ml_candidate_#{iid}/-/" + "/#{package_name}/#{package_version}/" + end + + def artifact + ::Packages::Generic::PackageFinder.new(experiment.project).execute!(package_name, package_version) + rescue ActiveRecord::RecordNotFound + nil + end + + def package_name + "ml_candidate_#{iid}" + end + + def package_version + '-' end class << self diff --git a/app/views/projects/ml/candidates/show.html.haml b/app/views/projects/ml/candidates/show.html.haml new file mode 100644 index 00000000000..7fa98f69edf --- /dev/null +++ b/app/views/projects/ml/candidates/show.html.haml @@ -0,0 +1,7 @@ +- experiment = @candidate.experiment +- add_to_breadcrumbs _("Experiments"), project_ml_experiments_path(@project) +- add_to_breadcrumbs experiment.name, project_ml_experiment_path(@project, experiment.iid) +- breadcrumb_title "Candidate #{@candidate.iid}" +- data = candidate_as_data(@candidate) + +#js-show-ml-candidate{ data: { candidate: data } } diff --git a/config/routes/project.rb b/config/routes/project.rb index 5a85a029607..798829484da 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -475,6 +475,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do namespace :ml do resources :experiments, only: [:index, :show], controller: 'experiments' + resources :candidates, only: [:show], controller: 'candidates', param: :iid end end # End of the /-/ scope. diff --git a/db/post_migrate/20221123133054_queue_reset_status_on_container_repositories.rb b/db/post_migrate/20221123133054_queue_reset_status_on_container_repositories.rb new file mode 100644 index 00000000000..2d482e0b83c --- /dev/null +++ b/db/post_migrate/20221123133054_queue_reset_status_on_container_repositories.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class QueueResetStatusOnContainerRepositories < Gitlab::Database::Migration[2.0] + MIGRATION = 'ResetStatusOnContainerRepositories' + DELAY_INTERVAL = 2.minutes + BATCH_SIZE = 50 + + restrict_gitlab_migration gitlab_schema: :gitlab_main + + def up + return unless ::Gitlab.config.registry.enabled + + queue_batched_background_migration( + MIGRATION, + :container_repositories, + :id, + job_interval: DELAY_INTERVAL, + sub_batch_size: BATCH_SIZE + ) + end + + def down + delete_batched_background_migration(MIGRATION, :container_repositories, :id, []) + end +end diff --git a/db/schema_migrations/20221123133054 b/db/schema_migrations/20221123133054 new file mode 100644 index 00000000000..3a7a382ee74 --- /dev/null +++ b/db/schema_migrations/20221123133054 @@ -0,0 +1 @@ +1a0a090433dd422b1bd9efdb56f82c02af8bab45b1a651b51a6ed224d823964c \ No newline at end of file diff --git a/doc/administration/postgresql/replication_and_failover.md b/doc/administration/postgresql/replication_and_failover.md index 34974046620..a273c6da7aa 100644 --- a/doc/administration/postgresql/replication_and_failover.md +++ b/doc/administration/postgresql/replication_and_failover.md @@ -1255,7 +1255,7 @@ To do the switch on **all** PgBouncer nodes: ``` 1. Run `gitlab-ctl reconfigure`. -1. You must also run `rm /var/opt/gitlab/consul/watcher_postgresql.json`. +1. You must also run `rm /var/opt/gitlab/consul/config.d/watcher_postgresql.json`. This is a [known issue](https://gitlab.com/gitlab-org/omnibus-gitlab/-/issues/7293). #### Clean up diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 7e69d2d266e..1fc778c3f0e 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -18263,6 +18263,7 @@ Returns [`UserMergeRequestInteraction`](#usermergerequestinteraction). | `readCommitStatus` | [`Boolean!`](#boolean) | Indicates the user can perform `read_commit_status` on this resource. | | `readCycleAnalytics` | [`Boolean!`](#boolean) | Indicates the user can perform `read_cycle_analytics` on this resource. | | `readDesign` | [`Boolean!`](#boolean) | Indicates the user can perform `read_design` on this resource. | +| `readEnvironment` | [`Boolean!`](#boolean) | Indicates the user can perform `read_environment` on this resource. | | `readMergeRequest` | [`Boolean!`](#boolean) | Indicates the user can perform `read_merge_request` on this resource. | | `readPagesContent` | [`Boolean!`](#boolean) | Indicates the user can perform `read_pages_content` on this resource. | | `readProject` | [`Boolean!`](#boolean) | Indicates the user can perform `read_project` on this resource. | diff --git a/doc/api/invitations.md b/doc/api/invitations.md index 94362b097af..908fa0ce890 100644 --- a/doc/api/invitations.md +++ b/doc/api/invitations.md @@ -126,7 +126,7 @@ PUT /projects/:id/invitations/:email | Attribute | Type | Required | Description | | --------- | ---- | -------- | ----------- | | `id` | integer/string | yes | The ID or [URL-encoded path of the project or group](index.md#namespaced-path-encoding) owned by the authenticated user. | -| `email` | string | yes | The email address to which the invitation was previously sent. | +| `email` | string | yes | The email address the invitation was previously sent to. | | `access_level` | integer | no | A valid access level (defaults: `30`, the Developer role). | | `expires_at` | string | no | A date string in ISO 8601 format (`YYYY-MM-DDTHH:MM:SSZ`). | diff --git a/doc/development/documentation/topic_types/task.md b/doc/development/documentation/topic_types/task.md index 78d670a16d6..0dba3e079b6 100644 --- a/doc/development/documentation/topic_types/task.md +++ b/doc/development/documentation/topic_types/task.md @@ -69,6 +69,12 @@ For example, `Create an issue when you want to track bugs or future work`. To start the task steps, use a succinct action followed by a colon. For example, `To create an issue:` +## Task prerequisites + +As a best practice, if the task requires the user to have a role other than Guest, +put the minimum role in the prerequisites. See [the Word list](../styleguide/word_list.md) for +how to write the phrase for each role. + ## Related topics - [View the format for writing task steps](../styleguide/index.md#navigation). diff --git a/doc/development/fe_guide/merge_request_widget_extensions.md b/doc/development/fe_guide/merge_request_widget_extensions.md index 61d3e79a080..49c6664c6d6 100644 --- a/doc/development/fe_guide/merge_request_widget_extensions.md +++ b/doc/development/fe_guide/merge_request_widget_extensions.md @@ -355,7 +355,12 @@ To generate these known events for a single widget: 1. `redis_slot` = `code_review` 1. `category` = `code_review` 1. `aggregation` = `weekly` -1. Add each event to the appropriate aggregates in `config/metrics/aggregates/code_review.yml` +1. Add each event (those listed in the command in step 7, replacing `test_reports` + with the appropriate name slug) to the aggregate files: + 1. `config/metrics/counts_7d/{timestamp}_code_review_category_monthly_active_users.yml` + 1. `config/metrics/counts_7d/{timestamp}_code_review_group_monthly_active_users.yml` + 1. `config/metrics/counts_28d/{timestamp}_code_review_category_monthly_active_users.yml` + 1. `config/metrics/counts_28d/{timestamp}_code_review_group_monthly_active_users.yml` ### Add new events diff --git a/doc/user/application_security/offline_deployments/index.md b/doc/user/application_security/offline_deployments/index.md index 2db8e9522db..05e56560f95 100644 --- a/doc/user/application_security/offline_deployments/index.md +++ b/doc/user/application_security/offline_deployments/index.md @@ -117,7 +117,7 @@ This template should be used in a new, empty project, with a `.gitlab-ci.yml` fi ```yaml include: - - template: Secure-Binaries.gitlab-ci.yml + - template: Security/Secure-Binaries.gitlab-ci.yml ``` The pipeline downloads the Docker images needed for the Security Scanners and saves them as diff --git a/doc/user/project/ml/experiment_tracking/img/candidate_v15_7.png b/doc/user/project/ml/experiment_tracking/img/candidate_v15_7.png new file mode 100644 index 00000000000..c7a49ad7608 Binary files /dev/null and b/doc/user/project/ml/experiment_tracking/img/candidate_v15_7.png differ diff --git a/doc/user/project/ml/experiment_tracking/img/candidates.png b/doc/user/project/ml/experiment_tracking/img/candidates.png deleted file mode 100644 index df70a01a2bd..00000000000 Binary files a/doc/user/project/ml/experiment_tracking/img/candidates.png and /dev/null differ diff --git a/doc/user/project/ml/experiment_tracking/img/candidates_v15_7.png b/doc/user/project/ml/experiment_tracking/img/candidates_v15_7.png new file mode 100644 index 00000000000..fed749cbee3 Binary files /dev/null and b/doc/user/project/ml/experiment_tracking/img/candidates_v15_7.png differ diff --git a/doc/user/project/ml/experiment_tracking/img/experiments.png b/doc/user/project/ml/experiment_tracking/img/experiments.png deleted file mode 100644 index a6472406b90..00000000000 Binary files a/doc/user/project/ml/experiment_tracking/img/experiments.png and /dev/null differ diff --git a/doc/user/project/ml/experiment_tracking/img/experiments_v15_7.png b/doc/user/project/ml/experiment_tracking/img/experiments_v15_7.png new file mode 100644 index 00000000000..f2a9d79e47b Binary files /dev/null and b/doc/user/project/ml/experiment_tracking/img/experiments_v15_7.png differ diff --git a/doc/user/project/ml/experiment_tracking/index.md b/doc/user/project/ml/experiment_tracking/index.md index d7d737572e0..a7096d633a0 100644 --- a/doc/user/project/ml/experiment_tracking/index.md +++ b/doc/user/project/ml/experiment_tracking/index.md @@ -16,9 +16,11 @@ engineering, and so on, to improve the performance of the model. Keeping track o artifacts so that the data scientist can later replicate the experiment is not trivial. Machine learning experiment tracking enables them to log parameters, metrics, and artifacts directly into GitLab, giving easy access later on. -![List of Experiments](img/experiments.png) +![List of Experiments](img/experiments_v15_7.png) -![Experiment Candidates](img/candidates.png) +![Experiment Candidates](img/candidates_v15_7.png) + +![Candidate Detail](img/candidate_v15_7.png) ## What is an experiment? @@ -53,13 +55,15 @@ integration. More information on how to use GitLab as a backend for MLFlow Clien ### Exploring model candidates To list the current active experiments, navigate to `https/-/ml/experiments`. To display all trials -that have been logged, along with their metrics and parameters, selecting an experiment. +that have been logged, along with their metrics and parameters, select an experiment. To display details for a candidate, +select **Details**. ### Logging artifacts Trial artifacts are saved as [generic packages](../../../packages/generic_packages/index.md), and follow all their conventions. After an artifact is logged for a candidate, all artifacts logged for the candidate are listed in the -package registry. The package name for a candidate is `ml_candidate_`, with version `-`. +package registry. The package name for a candidate is `ml_candidate_`, with version `-`. The link to the +artifacts can also be accessed from the **Experiment Candidates** list or **Candidate detail**. ### Limitations and future @@ -72,4 +76,6 @@ On GitLab.com, this feature is currently on private testing. ## Feedback, roadmap and reports -For updates on the development, feedback and bug reports, refer to the [development epic](https://gitlab.com/groups/gitlab-org/-/epics/8560). +For updates on the development, refer to the [development epic](https://gitlab.com/groups/gitlab-org/-/epics/8560). + +For feedback, bug reports and feature requests, refer to the [feedback issue](https://gitlab.com/gitlab-org/gitlab/-/issues/381660). diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index 7a198f30e38..531235dc9b2 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -125,7 +125,8 @@ module API user: current_user, protected: user_project.protected_for?(ref), ci_stage: stage, - stage_idx: stage.position + stage_idx: stage.position, + stage: 'external' ) updatable_optional_attributes = %w[target_url description coverage] diff --git a/lib/gitlab/background_migration/reset_status_on_container_repositories.rb b/lib/gitlab/background_migration/reset_status_on_container_repositories.rb new file mode 100644 index 00000000000..09cd3b1895f --- /dev/null +++ b/lib/gitlab/background_migration/reset_status_on_container_repositories.rb @@ -0,0 +1,139 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + # A job that: + # * pickup container repositories with delete_scheduled status. + # * check if there are tags linked to it. + # * if there are tags, reset the status to nil. + class ResetStatusOnContainerRepositories < BatchedMigrationJob + DELETE_SCHEDULED_STATUS = 0 + DUMMY_TAGS = %w[tag].freeze + MIGRATOR = 'ResetStatusOnContainerRepositories' + + scope_to ->(relation) { relation.where(status: DELETE_SCHEDULED_STATUS) } + operation_name :reset_status_on_container_repositories + + def perform + each_sub_batch do |sub_batch| + reset_status_if_tags(sub_batch) + end + end + + private + + def reset_status_if_tags(container_repositories) + container_repositories_with_tags = container_repositories.select { |cr| cr.becomes(ContainerRepository).tags? } # rubocop:disable Cop/AvoidBecomes + + ContainerRepository.where(id: container_repositories_with_tags.map(&:id)) + .update_all(status: nil) + end + + # rubocop:disable Style/Documentation + module Routable + extend ActiveSupport::Concern + + included do + has_one :route, + as: :source, + class_name: '::Gitlab::BackgroundMigration::ResetStatusOnContainerRepositories::Route' + end + + def full_path + route&.path || build_full_path + end + + def build_full_path + if parent && path + "#{parent.full_path}/#{path}" + else + path + end + end + end + + class Route < ::ApplicationRecord + self.table_name = 'routes' + end + + class Namespace < ::ApplicationRecord + include ::Gitlab::BackgroundMigration::ResetStatusOnContainerRepositories::Routable + include ::Namespaces::Traversal::Recursive + include ::Namespaces::Traversal::Linear + include ::Gitlab::Utils::StrongMemoize + + self.table_name = 'namespaces' + self.inheritance_column = :_type_disabled + + belongs_to :parent, + class_name: '::Gitlab::BackgroundMigration::ResetStatusOnContainerRepositories::Namespace' + + def self.polymorphic_name + 'Namespace' + end + end + + class Project < ::ApplicationRecord + include ::Gitlab::BackgroundMigration::ResetStatusOnContainerRepositories::Routable + + self.table_name = 'projects' + + belongs_to :namespace, + class_name: '::Gitlab::BackgroundMigration::ResetStatusOnContainerRepositories::Namespace' + + alias_method :parent, :namespace + alias_attribute :parent_id, :namespace_id + + delegate :root_ancestor, to: :namespace, allow_nil: true + end + + class ContainerRepository < ::ApplicationRecord + self.table_name = 'container_repositories' + + belongs_to :project, + class_name: '::Gitlab::BackgroundMigration::ResetStatusOnContainerRepositories::Project' + + def tags? + result = ContainerRegistry.tags_for(path).any? + ::Gitlab::BackgroundMigration::Logger.info( + migrator: MIGRATOR, + has_tags: result, + container_repository_id: id, + container_repository_path: path + ) + result + end + + def path + @path ||= [project.full_path, name].select(&:present?).join('/').downcase + end + end + + class ContainerRegistry + class << self + def tags_for(path) + response = ContainerRegistryClient.repository_tags(path, page_size: 1) + return DUMMY_TAGS unless response + + response['tags'] || [] + rescue StandardError + DUMMY_TAGS + end + end + end + + class ContainerRegistryClient + def self.repository_tags(path, page_size:) + registry_config = ::Gitlab.config.registry + + return { 'tags' => DUMMY_TAGS } unless registry_config.enabled && registry_config.api_url.present? + + pull_token = ::Auth::ContainerRegistryAuthenticationService.pull_access_token(path) + client = ::ContainerRegistry::Client.new(registry_config.api_url, token: pull_token) + client.repository_tags(path, page_size: page_size) + end + end + # rubocop:enable Style/Documentation + end + end +end diff --git a/lib/gitlab/ci/templates/Security/Secure-Binaries.gitlab-ci.yml b/lib/gitlab/ci/templates/Security/Secure-Binaries.gitlab-ci.yml index 9ca2e66d508..631f6cecddf 100644 --- a/lib/gitlab/ci/templates/Security/Secure-Binaries.gitlab-ci.yml +++ b/lib/gitlab/ci/templates/Security/Secure-Binaries.gitlab-ci.yml @@ -9,9 +9,9 @@ # Usage: # # include: -# - template: Secure-Binaries.gitlab-ci.yml +# - template: Security/Secure-Binaries.gitlab-ci.yml # -# Docs: https://docs.gitlab.com/ee/topics/airgap/ +# Docs: https://docs.gitlab.com/ee/user/application_security/offline_deployments/ variables: # Setting this variable will affect all Security templates diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 0256549ddbe..4a71143c826 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -16320,7 +16320,10 @@ msgstr "" msgid "Expected documents: %{expected_documents}" msgstr "" -msgid "Experiment Candidates" +msgid "Experiment" +msgstr "" + +msgid "Experiment candidates" msgstr "" msgid "Experiments" @@ -17052,6 +17055,9 @@ msgstr "" msgid "February" msgstr "" +msgid "Feedback" +msgstr "" + msgid "Feedback and Updates" msgstr "" @@ -21799,6 +21805,9 @@ msgstr "" msgid "Indicates whether this runner can pick jobs without tags" msgstr "" +msgid "Info" +msgstr "" + msgid "Inform users without uploaded SSH keys that they can't push over SSH until one is added" msgstr "" @@ -26633,6 +26642,9 @@ msgstr "" msgid "Modal|Close" msgstr "" +msgid "Model candidate details" +msgstr "" + msgid "Modified" msgstr "" @@ -29506,6 +29518,9 @@ msgstr "" msgid "Parameter \"job_id\" cannot exceed length of %{job_id_max_size}" msgstr "" +msgid "Parameters" +msgstr "" + msgid "Parent" msgstr "" @@ -41764,9 +41779,6 @@ msgstr "" msgid "This Cron pattern is invalid" msgstr "" -msgid "This Experiment has no logged Candidates" -msgstr "" - msgid "This GitLab instance does not provide any shared runners yet. Instance administrators can register shared runners in the admin area." msgstr "" @@ -41956,6 +41968,9 @@ msgstr "" msgid "This epic would exceed maximum number of related epics." msgstr "" +msgid "This experiment has no logged candidates" +msgstr "" + msgid "This feature requires local storage to be enabled" msgstr "" diff --git a/spec/frontend/ide/lib/common/model_spec.js b/spec/frontend/ide/lib/common/model_spec.js index 5d1623429c0..39c50f628c2 100644 --- a/spec/frontend/ide/lib/common/model_spec.js +++ b/spec/frontend/ide/lib/common/model_spec.js @@ -149,7 +149,6 @@ describe('Multi-file editor library model', () => { model.updateOptions({ insertSpaces: true, someOption: 'some value' }); expect(model.options).toEqual({ - endOfLine: 0, insertFinalNewline: true, insertSpaces: true, someOption: 'some value', @@ -181,16 +180,12 @@ describe('Multi-file editor library model', () => { describe('applyCustomOptions', () => { it.each` option | value | contentBefore | contentAfter - ${'endOfLine'} | ${0} | ${'hello\nworld\n'} | ${'hello\nworld\n'} - ${'endOfLine'} | ${0} | ${'hello\r\nworld\r\n'} | ${'hello\nworld\n'} - ${'endOfLine'} | ${1} | ${'hello\nworld\n'} | ${'hello\r\nworld\r\n'} - ${'endOfLine'} | ${1} | ${'hello\r\nworld\r\n'} | ${'hello\r\nworld\r\n'} ${'insertFinalNewline'} | ${true} | ${'hello\nworld'} | ${'hello\nworld\n'} ${'insertFinalNewline'} | ${true} | ${'hello\nworld\n'} | ${'hello\nworld\n'} ${'insertFinalNewline'} | ${false} | ${'hello\nworld'} | ${'hello\nworld'} ${'trimTrailingWhitespace'} | ${true} | ${'hello \t\nworld \t\n'} | ${'hello\nworld\n'} - ${'trimTrailingWhitespace'} | ${true} | ${'hello \t\r\nworld \t\r\n'} | ${'hello\nworld\n'} - ${'trimTrailingWhitespace'} | ${false} | ${'hello \t\r\nworld \t\r\n'} | ${'hello \t\nworld \t\n'} + ${'trimTrailingWhitespace'} | ${true} | ${'hello \t\r\nworld \t\r\n'} | ${'hello\r\nworld\r\n'} + ${'trimTrailingWhitespace'} | ${false} | ${'hello \t\r\nworld \t\r\n'} | ${'hello \t\r\nworld \t\r\n'} `( 'correctly applies custom option $option=$value to content', ({ option, value, contentBefore, contentAfter }) => { diff --git a/spec/frontend/ml/experiment_tracking/components/__snapshots__/experiment_spec.js.snap b/spec/frontend/ml/experiment_tracking/components/__snapshots__/experiment_spec.js.snap deleted file mode 100644 index 2eba8869535..00000000000 --- a/spec/frontend/ml/experiment_tracking/components/__snapshots__/experiment_spec.js.snap +++ /dev/null @@ -1,223 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`ShowExperiment with candidates renders correctly 1`] = ` -
    -
    - - - - - -
    - -

    - - Experiment Candidates - -

    - - - - - - - - - - - - - - - - - - - - - - - - - - - -
    -
    - L1 Ratio -
    -     		-
    - Rmse -
    -     		-
    - Auc -
    -     		-
    - Mae -
    -
    - 0.4 - - 1 - - -
    - 0.5 - - - 0.3 - -
    -
    -`; diff --git a/spec/frontend/ml/experiment_tracking/components/__snapshots__/ml_candidate_spec.js.snap b/spec/frontend/ml/experiment_tracking/components/__snapshots__/ml_candidate_spec.js.snap new file mode 100644 index 00000000000..8af0753f929 --- /dev/null +++ b/spec/frontend/ml/experiment_tracking/components/__snapshots__/ml_candidate_spec.js.snap @@ -0,0 +1,233 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`MlCandidate renders correctly 1`] = ` +
    +
    + + + + + +
    + +

    + + Model candidate details + +

    + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
    + Info + + ID + + candidate_iid +
    + + + Status + + SUCCESS +
    + + + Experiment + + + The Experiment + +
    + + Parameters + + + Algorithm + + Decision Tree +
    + + + MaxDepth + + 3 +
    + + Metrics + + + AUC + + .55 +
    + + + Accuracy + + .99 +
    +
    +`; diff --git a/spec/frontend/ml/experiment_tracking/components/__snapshots__/ml_experiment_spec.js.snap b/spec/frontend/ml/experiment_tracking/components/__snapshots__/ml_experiment_spec.js.snap new file mode 100644 index 00000000000..e253a0afc6c --- /dev/null +++ b/spec/frontend/ml/experiment_tracking/components/__snapshots__/ml_experiment_spec.js.snap @@ -0,0 +1,284 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`MlExperiment with candidates renders correctly 1`] = ` +
    +
    + + + + + +
    + +

    + + Experiment candidates + +

    + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
    +
    + L1 Ratio +
    +     		+
    + Rmse +
    +     		+
    + Auc +
    +     		+
    + Mae +
    +     		+
    +
    		+
    +
    + 0.4 + + 1 + + + + + Details + + + + Artifacts + +
    + 0.5 + + + 0.3 + + + + Details + + +
    +
    +`; diff --git a/spec/frontend/ml/experiment_tracking/components/experiment_spec.js b/spec/frontend/ml/experiment_tracking/components/experiment_spec.js deleted file mode 100644 index af722d77532..00000000000 --- a/spec/frontend/ml/experiment_tracking/components/experiment_spec.js +++ /dev/null @@ -1,44 +0,0 @@ -import { GlAlert } from '@gitlab/ui'; -import { mountExtended } from 'helpers/vue_test_utils_helper'; -import ShowExperiment from '~/ml/experiment_tracking/components/experiment.vue'; - -describe('ShowExperiment', () => { - let wrapper; - - const createWrapper = (candidates = [], metricNames = [], paramNames = []) => { - return mountExtended(ShowExperiment, { provide: { candidates, metricNames, paramNames } }); - }; - - const findAlert = () => wrapper.findComponent(GlAlert); - - const findEmptyState = () => wrapper.findByText('This Experiment has no logged Candidates'); - - it('shows incubation warning', () => { - wrapper = createWrapper(); - - expect(findAlert().exists()).toBe(true); - }); - - describe('no candidates', () => { - it('shows empty state', () => { - wrapper = createWrapper(); - - expect(findEmptyState().exists()).toBe(true); - }); - }); - - describe('with candidates', () => { - it('renders correctly', () => { - wrapper = createWrapper( - [ - { rmse: 1, l1_ratio: 0.4 }, - { auc: 0.3, l1_ratio: 0.5 }, - ], - ['rmse', 'auc', 'mae'], - ['l1_ratio'], - ); - - expect(wrapper.element).toMatchSnapshot(); - }); - }); -}); 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/ml_experiment_spec.js b/spec/frontend/ml/experiment_tracking/components/ml_experiment_spec.js new file mode 100644 index 00000000000..50539440f25 --- /dev/null +++ b/spec/frontend/ml/experiment_tracking/components/ml_experiment_spec.js @@ -0,0 +1,44 @@ +import { GlAlert } from '@gitlab/ui'; +import { mountExtended } from 'helpers/vue_test_utils_helper'; +import MlExperiment from '~/ml/experiment_tracking/components/ml_experiment.vue'; + +describe('MlExperiment', () => { + let wrapper; + + const createWrapper = (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'); + + it('shows incubation warning', () => { + wrapper = createWrapper(); + + expect(findAlert().exists()).toBe(true); + }); + + describe('no candidates', () => { + it('shows empty state', () => { + wrapper = createWrapper(); + + expect(findEmptyState().exists()).toBe(true); + }); + }); + + describe('with candidates', () => { + it('renders correctly', () => { + wrapper = createWrapper( + [ + { 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'], + ); + + expect(wrapper.element).toMatchSnapshot(); + }); + }); +}); 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 }] ) + end + end - c[1].params.create!([{ name: 'param2', value: 'p3' }, { name: 'param3', value: 'p4' }]) - c[1].metrics.create!(name: 'metric3', value: 0.4) + 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/exception/exception.go b/workhorse/internal/helper/exception/exception.go new file mode 100644 index 00000000000..9b1628ffecb --- /dev/null +++ b/workhorse/internal/helper/exception/exception.go @@ -0,0 +1,58 @@ +package exception + +import ( + "net/http" + "reflect" + + raven "github.com/getsentry/raven-go" + + //lint:ignore SA1019 this was recently deprecated. Update workhorse to use labkit errortracking package. + correlation "gitlab.com/gitlab-org/labkit/correlation/raven" + + "gitlab.com/gitlab-org/labkit/log" +) + +var ravenHeaderBlacklist = []string{ + "Authorization", + "Private-Token", +} + +func Track(r *http.Request, err error, fields log.Fields) { + client := raven.DefaultClient + extra := raven.Extra{} + + for k, v := range fields { + extra[k] = v + } + + interfaces := []raven.Interface{} + if r != nil { + CleanHeaders(r) + interfaces = append(interfaces, raven.NewHttp(r)) + + //lint:ignore SA1019 this was recently deprecated. Update workhorse to use labkit errortracking package. + extra = correlation.SetExtra(r.Context(), extra) + } + + exception := &raven.Exception{ + Stacktrace: raven.NewStacktrace(2, 3, nil), + Value: err.Error(), + Type: reflect.TypeOf(err).String(), + } + interfaces = append(interfaces, exception) + + packet := raven.NewPacketWithExtra(err.Error(), extra, interfaces...) + client.Capture(packet, nil) +} + +func CleanHeaders(r *http.Request) { + if r == nil { + return + } + + for _, key := range ravenHeaderBlacklist { + if r.Header.Get(key) != "" { + r.Header.Set(key, "[redacted]") + } + } +} 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" -) -func logErrorWithFields(r *http.Request, err error, fields log.Fields) { - if err != nil { - CaptureRavenError(r, err, fields) - } - - printError(r, err, fields) -} + "gitlab.com/gitlab-org/gitlab/workhorse/internal/log" +) 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/helper/raven.go b/workhorse/internal/helper/raven.go deleted file mode 100644 index 898e8ec85f8..00000000000 --- a/workhorse/internal/helper/raven.go +++ /dev/null @@ -1,58 +0,0 @@ -package helper - -import ( - "net/http" - "reflect" - - raven "github.com/getsentry/raven-go" - - //lint:ignore SA1019 this was recently deprecated. Update workhorse to use labkit errortracking package. - correlation "gitlab.com/gitlab-org/labkit/correlation/raven" - - "gitlab.com/gitlab-org/labkit/log" -) - -var ravenHeaderBlacklist = []string{ - "Authorization", - "Private-Token", -} - -func CaptureRavenError(r *http.Request, err error, fields log.Fields) { - client := raven.DefaultClient - extra := raven.Extra{} - - for k, v := range fields { - extra[k] = v - } - - interfaces := []raven.Interface{} - if r != nil { - CleanHeadersForRaven(r) - interfaces = append(interfaces, raven.NewHttp(r)) - - //lint:ignore SA1019 this was recently deprecated. Update workhorse to use labkit errortracking package. - extra = correlation.SetExtra(r.Context(), extra) - } - - exception := &raven.Exception{ - Stacktrace: raven.NewStacktrace(2, 3, nil), - Value: err.Error(), - Type: reflect.TypeOf(err).String(), - } - interfaces = append(interfaces, exception) - - packet := raven.NewPacketWithExtra(err.Error(), extra, interfaces...) - client.Capture(packet, nil) -} - -func CleanHeadersForRaven(r *http.Request) { - if r == nil { - return - } - - for _, key := range ravenHeaderBlacklist { - if r.Header.Get(key) != "" { - r.Header.Set(key, "[redacted]") - } - } -} 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) 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...) +} - if b.req != nil && b.err != nil { - helper.CaptureRavenError(b.req, b.err, b.fields) - } +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) } }() -- cgit v1.2.3