From b920d2a9831056cdf907cf71fd25d94f0aaf1e6c Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Thu, 30 Sep 2021 06:09:27 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- .gitlab/ci/global.gitlab-ci.yml | 2 + .gitlab/ci/rules.gitlab-ci.yml | 116 +++++++++--------- .../javascripts/notebook/cells/markdown.vue | 9 +- app/models/bulk_imports/tracker.rb | 2 + app/workers/bulk_imports/pipeline_worker.rb | 32 ++++- doc/development/snowplow/dictionary.md | 44 +------ doc/development/snowplow/index.md | 2 +- doc/development/snowplow/review_guidelines.md | 2 +- lib/bulk_imports/clients/graphql.rb | 2 + lib/bulk_imports/clients/http.rb | 4 +- lib/bulk_imports/network_error.rb | 61 +++++++++ lib/gitlab/cache/import/caching.rb | 4 +- lib/gitlab/tracking/docs/helper.rb | 67 ---------- lib/gitlab/tracking/docs/renderer.rb | 32 ----- .../tracking/docs/templates/default.md.haml | 35 ------ lib/tasks/gitlab/snowplow.rake | 11 -- .../formatters/test_stats_formatter.rb | 15 ++- .../formatters/test_stats_formatter_spec.rb | 11 +- spec/lib/bulk_imports/clients/http_spec.rb | 10 +- spec/lib/bulk_imports/network_error_spec.rb | 72 +++++++++++ spec/lib/gitlab/cache/import/caching_spec.rb | 10 ++ spec/lib/gitlab/tracking/docs/helper_spec.rb | 91 -------------- .../lib/gitlab/tracking/docs/renderer_spec.rb | 23 ---- .../bulk_imports/pipeline_worker_spec.rb | 102 +++++++++++---- 24 files changed, 354 insertions(+), 405 deletions(-) create mode 100644 lib/bulk_imports/network_error.rb delete mode 100644 lib/gitlab/tracking/docs/helper.rb delete mode 100644 lib/gitlab/tracking/docs/renderer.rb delete mode 100644 lib/gitlab/tracking/docs/templates/default.md.haml delete mode 100644 lib/tasks/gitlab/snowplow.rake create mode 100644 spec/lib/bulk_imports/network_error_spec.rb delete mode 100644 spec/lib/gitlab/tracking/docs/helper_spec.rb delete mode 100644 spec/lib/gitlab/tracking/docs/renderer_spec.rb diff --git a/.gitlab/ci/global.gitlab-ci.yml b/.gitlab/ci/global.gitlab-ci.yml index d9978a44ffb..a0faa6cc3a1 100644 --- a/.gitlab/ci/global.gitlab-ci.yml +++ b/.gitlab/ci/global.gitlab-ci.yml @@ -193,10 +193,12 @@ .storybook-yarn-cache: cache: + - *node-modules-cache - *storybook-node-modules-cache .storybook-yarn-cache-push: cache: + - *node-modules-cache # We don't push this cache as it's already rebuilt by `update-yarn-cache` - *storybook-node-modules-cache-push .use-pg11: diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml index d69988a83ed..3dfc26af2b5 100644 --- a/.gitlab/ci/rules.gitlab-ci.yml +++ b/.gitlab/ci/rules.gitlab-ci.yml @@ -37,19 +37,19 @@ .if-automated-merge-request: &if-automated-merge-request if: '$CI_MERGE_REQUEST_SOURCE_BRANCH_NAME == "release-tools/update-gitaly" || $CI_MERGE_REQUEST_TARGET_BRANCH_NAME =~ /stable-ee$/' -.if-merge-request-title-as-if-foss: &if-merge-request-title-as-if-foss +.if-merge-request-labels-as-if-foss: &if-merge-request-labels-as-if-foss if: '$CI_MERGE_REQUEST_LABELS =~ /pipeline:run-as-if-foss/' -.if-merge-request-title-update-caches: &if-merge-request-title-update-caches +.if-merge-request-labels-update-caches: &if-merge-request-labels-update-caches if: '$CI_MERGE_REQUEST_LABELS =~ /pipeline:update-cache/' -.if-merge-request-title-run-all-rspec: &if-merge-request-title-run-all-rspec +.if-merge-request-labels-run-all-rspec: &if-merge-request-labels-run-all-rspec if: '$CI_MERGE_REQUEST_LABELS =~ /pipeline:run-all-rspec/' -.if-merge-request-title-run-all-jest: &if-merge-request-title-run-all-jest +.if-merge-request-labels-run-all-jest: &if-merge-request-labels-run-all-jest if: '$CI_MERGE_REQUEST_LABELS =~ /pipeline:run-all-jest/' -.if-merge-request-run-decomposed: &if-merge-request-run-decomposed +.if-merge-request-labels-run-decomposed: &if-merge-request-labels-run-decomposed if: '$CI_MERGE_REQUEST_LABELS =~ /pipeline:run-decomposed/' .if-security-merge-request: &if-security-merge-request @@ -368,11 +368,11 @@ rules: - <<: *if-default-branch-schedule-2-hourly - <<: *if-security-schedule - - <<: *if-merge-request-title-update-caches + - <<: *if-merge-request-labels-update-caches .shared:rules:update-gitaly-binaries-cache: rules: - - <<: *if-merge-request-title-update-caches + - <<: *if-merge-request-labels-update-caches - changes: *gitaly-patterns ###################### @@ -481,14 +481,14 @@ .frontend:rules:compile-test-assets: rules: - changes: *code-backstage-qa-patterns - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec .frontend:rules:compile-test-assets-as-if-foss: rules: - <<: *if-not-ee when: never - changes: *code-backstage-qa-patterns - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec .frontend:rules:default-frontend-jobs: rules: @@ -501,8 +501,8 @@ when: never - <<: *if-security-merge-request changes: *code-backstage-patterns - - <<: *if-merge-request-title-as-if-foss - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-as-if-foss + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *startup-css-patterns - <<: *if-merge-request @@ -510,7 +510,7 @@ .frontend:rules:jest: rules: - - <<: *if-merge-request-title-run-all-jest + - <<: *if-merge-request-labels-run-all-jest - <<: *if-default-refs changes: *core-frontend-patterns - <<: *if-merge-request @@ -530,7 +530,7 @@ when: never - <<: *if-automated-merge-request when: never - - <<: *if-merge-request-title-run-all-jest + - <<: *if-merge-request-labels-run-all-jest when: never - <<: *if-default-refs changes: *core-frontend-patterns @@ -548,7 +548,7 @@ rules: - <<: *if-not-ee when: never - - <<: *if-merge-request-title-as-if-foss + - <<: *if-merge-request-labels-as-if-foss when: never - <<: *if-merge-request changes: *frontend-patterns @@ -618,8 +618,8 @@ when: never - <<: *if-security-merge-request changes: *code-qa-patterns - - <<: *if-merge-request-title-as-if-foss - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-as-if-foss + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *ci-patterns @@ -645,11 +645,11 @@ ############### .rails:rules:decomposed-databases: rules: - - <<: *if-merge-request-run-decomposed + - <<: *if-merge-request-labels-run-decomposed .rails:rules:ee-and-foss-migration: rules: - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *ci-patterns - <<: *if-merge-request @@ -666,7 +666,7 @@ when: never - <<: *if-automated-merge-request when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec when: never - <<: *if-merge-request changes: *ci-patterns @@ -679,7 +679,7 @@ rules: - <<: *if-merge-request changes: *db-patterns - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec .rails:rules:db:gitlabcom-database-testing: rules: @@ -691,7 +691,7 @@ .rails:rules:ee-and-foss-unit: rules: - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *ci-patterns - <<: *if-automated-merge-request @@ -706,7 +706,7 @@ when: never - <<: *if-automated-merge-request when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec when: never - <<: *if-merge-request changes: *ci-patterns @@ -716,7 +716,7 @@ .rails:rules:ee-and-foss-integration: rules: - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *ci-patterns - <<: *if-automated-merge-request @@ -731,7 +731,7 @@ when: never - <<: *if-automated-merge-request when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec when: never - <<: *if-merge-request changes: *ci-patterns @@ -741,7 +741,7 @@ .rails:rules:ee-and-foss-system: rules: - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *ci-patterns - <<: *if-automated-merge-request @@ -756,7 +756,7 @@ when: never - <<: *if-automated-merge-request when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec when: never - <<: *if-merge-request changes: *ci-patterns @@ -766,7 +766,7 @@ .rails:rules:ee-and-foss-fast_spec_helper: rules: - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *ci-patterns - <<: *if-automated-merge-request @@ -781,7 +781,7 @@ when: never - <<: *if-automated-merge-request when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec when: never - <<: *if-merge-request changes: *ci-patterns @@ -792,13 +792,13 @@ .rails:rules:code-backstage-qa: rules: - changes: *code-backstage-qa-patterns - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec .rails:rules:ee-only-migration: rules: - <<: *if-not-ee when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *ci-patterns - <<: *if-merge-request @@ -817,7 +817,7 @@ when: never - <<: *if-automated-merge-request when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec when: never - <<: *if-merge-request changes: *ci-patterns @@ -830,7 +830,7 @@ rules: - <<: *if-not-ee when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *ci-patterns - <<: *if-automated-merge-request @@ -847,7 +847,7 @@ when: never - <<: *if-automated-merge-request when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec when: never - <<: *if-merge-request changes: *ci-patterns @@ -859,7 +859,7 @@ rules: - <<: *if-not-ee when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *ci-patterns - <<: *if-automated-merge-request @@ -876,7 +876,7 @@ when: never - <<: *if-automated-merge-request when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec when: never - <<: *if-merge-request changes: *ci-patterns @@ -888,7 +888,7 @@ rules: - <<: *if-not-ee when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *ci-patterns - <<: *if-automated-merge-request @@ -905,7 +905,7 @@ when: never - <<: *if-automated-merge-request when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec when: never - <<: *if-merge-request changes: *ci-patterns @@ -917,12 +917,12 @@ rules: - <<: *if-not-ee when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *ci-patterns - <<: *if-security-merge-request changes: *db-patterns - - <<: *if-merge-request-title-as-if-foss + - <<: *if-merge-request-labels-as-if-foss changes: *db-patterns - <<: *if-automated-merge-request changes: *db-patterns @@ -943,7 +943,7 @@ - <<: *if-security-merge-request changes: *db-patterns when: never - - <<: *if-merge-request-title-as-if-foss + - <<: *if-merge-request-labels-as-if-foss changes: *db-patterns when: never @@ -951,7 +951,7 @@ rules: - <<: *if-not-ee when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *ci-patterns - <<: *if-automated-merge-request @@ -960,7 +960,7 @@ when: never - <<: *if-security-merge-request changes: *backend-patterns - - <<: *if-merge-request-title-as-if-foss + - <<: *if-merge-request-labels-as-if-foss changes: *backend-patterns .rails:rules:as-if-foss-unit:minimal: @@ -976,14 +976,14 @@ when: never - <<: *if-security-merge-request changes: *backend-patterns - - <<: *if-merge-request-title-as-if-foss + - <<: *if-merge-request-labels-as-if-foss changes: *backend-patterns .rails:rules:as-if-foss-integration: rules: - <<: *if-not-ee when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *ci-patterns - <<: *if-automated-merge-request @@ -992,7 +992,7 @@ when: never - <<: *if-security-merge-request changes: *backend-patterns - - <<: *if-merge-request-title-as-if-foss + - <<: *if-merge-request-labels-as-if-foss changes: *backend-patterns .rails:rules:as-if-foss-integration:minimal: @@ -1008,14 +1008,14 @@ when: never - <<: *if-security-merge-request changes: *backend-patterns - - <<: *if-merge-request-title-as-if-foss + - <<: *if-merge-request-labels-as-if-foss changes: *backend-patterns .rails:rules:as-if-foss-system: rules: - <<: *if-not-ee when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *ci-patterns - <<: *if-automated-merge-request @@ -1024,7 +1024,7 @@ when: never - <<: *if-security-merge-request changes: *code-backstage-patterns - - <<: *if-merge-request-title-as-if-foss + - <<: *if-merge-request-labels-as-if-foss changes: *code-backstage-patterns .rails:rules:as-if-foss-system:minimal: @@ -1040,19 +1040,19 @@ when: never - <<: *if-security-merge-request changes: *code-backstage-patterns - - <<: *if-merge-request-title-as-if-foss + - <<: *if-merge-request-labels-as-if-foss changes: *code-backstage-patterns .rails:rules:ee-and-foss-db-library-code: rules: - changes: *db-library-patterns - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec .rails:rules:ee-mr-and-default-branch-only: rules: - <<: *if-not-ee when: never - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *code-backstage-patterns - <<: *if-default-branch-refs @@ -1061,13 +1061,13 @@ .rails:rules:detect-tests: rules: - changes: *code-backstage-patterns - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec .rails:rules:rspec-foss-impact: rules: - <<: *if-not-ee when: never - - <<: *if-merge-request-title-as-if-foss + - <<: *if-merge-request-labels-as-if-foss when: never - <<: *if-security-merge-request changes: *code-backstage-patterns @@ -1099,7 +1099,7 @@ - <<: *if-not-ee when: never - <<: *if-default-branch-schedule-nightly - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec .rails:rules:rspec-coverage: rules: @@ -1109,7 +1109,7 @@ changes: *code-backstage-patterns when: always - <<: *if-default-branch-schedule-2-hourly - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec when: always .rails:rules:default-branch-schedule-nightly--code-backstage: @@ -1144,7 +1144,7 @@ rules: - <<: *if-not-ee when: never - - <<: *if-merge-request-title-as-if-foss + - <<: *if-merge-request-labels-as-if-foss changes: *code-backstage-qa-patterns - <<: *if-security-merge-request changes: *code-backstage-qa-patterns @@ -1159,7 +1159,7 @@ rules: - <<: *if-merge-request changes: ["vendor/gems/mail-smtp_pool/**/*"] - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec ################## # Releases rules # @@ -1504,7 +1504,7 @@ rules: - changes: *code-backstage-patterns when: on_success - - <<: *if-merge-request-title-run-all-rspec + - <<: *if-merge-request-labels-run-all-rspec .test-metadata:rules:update-tests-metadata: rules: diff --git a/app/assets/javascripts/notebook/cells/markdown.vue b/app/assets/javascripts/notebook/cells/markdown.vue index 1384c9c40b3..073b27605bb 100644 --- a/app/assets/javascripts/notebook/cells/markdown.vue +++ b/app/assets/javascripts/notebook/cells/markdown.vue @@ -1,6 +1,7 @@ diff --git a/app/models/bulk_imports/tracker.rb b/app/models/bulk_imports/tracker.rb index c185470b1c2..9de3239ee0f 100644 --- a/app/models/bulk_imports/tracker.rb +++ b/app/models/bulk_imports/tracker.rb @@ -50,6 +50,8 @@ class BulkImports::Tracker < ApplicationRecord event :start do transition created: :started + # To avoid errors when re-starting a pipeline in case of network errors + transition started: :started end event :finish do diff --git a/app/workers/bulk_imports/pipeline_worker.rb b/app/workers/bulk_imports/pipeline_worker.rb index 760a309a381..35633b55489 100644 --- a/app/workers/bulk_imports/pipeline_worker.rb +++ b/app/workers/bulk_imports/pipeline_worker.rb @@ -16,7 +16,7 @@ module BulkImports def perform(pipeline_tracker_id, stage, entity_id) pipeline_tracker = ::BulkImports::Tracker - .with_status(:created) + .with_status(:created, :started) .find_by_id(pipeline_tracker_id) if pipeline_tracker.present? @@ -59,18 +59,35 @@ module BulkImports pipeline_tracker.pipeline_class.new(context).run pipeline_tracker.finish! + rescue BulkImports::NetworkError => e + if e.retriable?(pipeline_tracker) + logger.error( + worker: self.class.name, + entity_id: pipeline_tracker.entity.id, + pipeline_name: pipeline_tracker.pipeline_name, + message: "Retrying error: #{e.message}" + ) + + reenqueue(pipeline_tracker, delay: e.retry_delay) + else + fail_tracker(pipeline_tracker, e) + end rescue StandardError => e + fail_tracker(pipeline_tracker, e) + end + + def fail_tracker(pipeline_tracker, exception) pipeline_tracker.update!(status_event: 'fail_op', jid: jid) logger.error( worker: self.class.name, entity_id: pipeline_tracker.entity.id, pipeline_name: pipeline_tracker.pipeline_name, - message: e.message + message: exception.message ) Gitlab::ErrorTracking.track_exception( - e, + exception, entity_id: pipeline_tracker.entity.id, pipeline_name: pipeline_tracker.pipeline_name ) @@ -88,8 +105,13 @@ module BulkImports (Time.zone.now - pipeline_tracker.entity.created_at) > Pipeline::NDJSON_EXPORT_TIMEOUT end - def reenqueue(pipeline_tracker) - self.class.perform_in(NDJSON_PIPELINE_PERFORM_DELAY, pipeline_tracker.id, pipeline_tracker.stage, pipeline_tracker.entity.id) + def reenqueue(pipeline_tracker, delay: NDJSON_PIPELINE_PERFORM_DELAY) + self.class.perform_in( + delay, + pipeline_tracker.id, + pipeline_tracker.stage, + pipeline_tracker.entity.id + ) end end end diff --git a/doc/development/snowplow/dictionary.md b/doc/development/snowplow/dictionary.md index 589d6f6fb9f..02e9ba5ce20 100644 --- a/doc/development/snowplow/dictionary.md +++ b/doc/development/snowplow/dictionary.md @@ -1,44 +1,4 @@ --- -stage: Growth -group: Product Intelligence -info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#designated-technical-writers +redirect_to: 'https://metrics.gitlab.com/snowplow.html' +remove_date: '2021-12-28' --- - - - - - -# Event Dictionary - -This file is autogenerated, please do not edit it directly. - -To generate these files from the GitLab repository, run: - -```shell -bundle exec rake gitlab:snowplow:generate_event_dictionary -``` - -The Event Dictionary is based on the following event definition YAML files: - -- [`config/events`](https://gitlab.com/gitlab-org/gitlab/-/tree/f9a404301ca22d038e7b9a9eb08d9c1bbd6c4d84/config/events) -- [`ee/config/events`](https://gitlab.com/gitlab-org/gitlab/-/tree/f9a404301ca22d038e7b9a9eb08d9c1bbd6c4d84/ee/config/events) - -## Event definitions - -### `epics promote` - -| category | action | label | property | value | -|---|---|---|---|---| -| `epics` | `promote` | `` | `The string "issue_id"` | `ID of the issue` | - -Issue promoted to epic - -YAML definition: `/ee/config/events/epics_promote.yml` - -Owner: `group::product planning` - -Tiers: `premium`, `ultimate` diff --git a/doc/development/snowplow/index.md b/doc/development/snowplow/index.md index 11525f186c1..7f3eae580c8 100644 --- a/doc/development/snowplow/index.md +++ b/doc/development/snowplow/index.md @@ -39,7 +39,7 @@ Snowplow is an enterprise-grade marketing and Product Intelligence platform whic - [Understanding the structure of Snowplow data](https://docs.snowplowanalytics.com/docs/understanding-your-pipeline/canonical-event/) - [Our Iglu schema registry](https://gitlab.com/gitlab-org/iglu) -- [List of events used in our codebase (Event Dictionary)](dictionary.md) +- [List of events used in our codebase (Event Dictionary)](https://metrics.gitlab.com/snowplow.html) ## Enable Snowplow tracking diff --git a/doc/development/snowplow/review_guidelines.md b/doc/development/snowplow/review_guidelines.md index 8edcbf06a0e..fa0985f6943 100644 --- a/doc/development/snowplow/review_guidelines.md +++ b/doc/development/snowplow/review_guidelines.md @@ -14,7 +14,7 @@ general best practices for code reviews, refer to our [code review guide](../cod ## Resources for reviewers - [Snowplow Guide](index.md) -- [Event Dictionary](dictionary.md) +- [Event Dictionary](https://metrics.gitlab.com/snowplow.html) ## Review process diff --git a/lib/bulk_imports/clients/graphql.rb b/lib/bulk_imports/clients/graphql.rb index 0adc2b1c57f..43ad9f0aa2d 100644 --- a/lib/bulk_imports/clients/graphql.rb +++ b/lib/bulk_imports/clients/graphql.rb @@ -17,6 +17,8 @@ module BulkImports ) ::Gitlab::Json.parse(response.body) + rescue *Gitlab::HTTP::HTTP_ERRORS => e + raise ::BulkImports::NetworkError, e end end private_constant :HTTP diff --git a/lib/bulk_imports/clients/http.rb b/lib/bulk_imports/clients/http.rb index 6c363a3552f..f98d4a5eb14 100644 --- a/lib/bulk_imports/clients/http.rb +++ b/lib/bulk_imports/clients/http.rb @@ -113,11 +113,11 @@ module BulkImports def with_error_handling response = yield - raise(::BulkImports::Error, "Error #{response.code}") unless response.success? + raise ::BulkImports::NetworkError.new(response: response) unless response.success? response rescue *Gitlab::HTTP::HTTP_ERRORS => e - raise(::BulkImports::Error, e) + raise ::BulkImports::NetworkError, e end def api_url diff --git a/lib/bulk_imports/network_error.rb b/lib/bulk_imports/network_error.rb new file mode 100644 index 00000000000..d69b0172f6c --- /dev/null +++ b/lib/bulk_imports/network_error.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +module BulkImports + class NetworkError < Error + COUNTER_KEY = 'bulk_imports/%{entity_id}/%{stage}/%{tracker_id}/network_error/%{error}' + + RETRIABLE_EXCEPTIONS = Gitlab::HTTP::HTTP_TIMEOUT_ERRORS + RETRIABLE_HTTP_CODES = [429].freeze + + DEFAULT_RETRY_DELAY_SECONDS = 60 + + MAX_RETRIABLE_COUNT = 3 + + def initialize(message = nil, response: nil) + raise ArgumentError, 'message or response required' if message.blank? && response.blank? + + super(message) + + @response = response + end + + def retriable?(tracker) + if retriable_exception? || retriable_http_code? + increment(tracker) <= MAX_RETRIABLE_COUNT + else + false + end + end + + def retry_delay + if response&.code == 429 + response.headers.fetch('Retry-After', DEFAULT_RETRY_DELAY_SECONDS).to_i + else + DEFAULT_RETRY_DELAY_SECONDS + end.seconds + end + + private + + attr_reader :response + + def retriable_exception? + RETRIABLE_EXCEPTIONS.include?(cause&.class) + end + + def retriable_http_code? + RETRIABLE_HTTP_CODES.include?(response&.code) + end + + def increment(tracker) + key = COUNTER_KEY % { + stage: tracker.stage, + tracker_id: tracker.id, + entity_id: tracker.entity.id, + error: cause.class.name + } + + Gitlab::Cache::Import::Caching.increment(key) + end + end +end diff --git a/lib/gitlab/cache/import/caching.rb b/lib/gitlab/cache/import/caching.rb index 947efee43a9..4dbce0b05e1 100644 --- a/lib/gitlab/cache/import/caching.rb +++ b/lib/gitlab/cache/import/caching.rb @@ -84,8 +84,10 @@ module Gitlab key = cache_key_for(raw_key) Redis::Cache.with do |redis| - redis.incr(key) + value = redis.incr(key) redis.expire(key, timeout) + + value end end diff --git a/lib/gitlab/tracking/docs/helper.rb b/lib/gitlab/tracking/docs/helper.rb deleted file mode 100644 index 4e03858b771..00000000000 --- a/lib/gitlab/tracking/docs/helper.rb +++ /dev/null @@ -1,67 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Tracking - module Docs - # Helper with functions to be used by HAML templates - module Helper - def auto_generated_comment - <<-MARKDOWN.strip_heredoc - --- - stage: Growth - group: Product Intelligence - info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#designated-technical-writers - --- - - - - - MARKDOWN - end - - def render_description(object) - return 'Missing description' unless object.description.present? - - object.description - end - - def render_event_taxonomy(object) - headers = %w[category action label property value] - values = %i[category action label property_description value_description] - values = values.map { |key| backtick(object.attributes[key]) } - values = values.join(" | ") - - [ - "| #{headers.join(" | ")} |", - "#{'|---' * headers.size}|", - "| #{values} |" - ].join("\n") - end - - def md_link_to(anchor_text, url) - "[#{anchor_text}](#{url})" - end - - def render_owner(object) - "Owner: #{backtick(object.product_group)}" - end - - def render_tiers(object) - "Tiers: #{object.tiers.map(&method(:backtick)).join(', ')}" - end - - def render_yaml_definition_path(object) - "YAML definition: #{backtick(object.yaml_path)}" - end - - def backtick(string) - "`#{string}`" - end - end - end - end -end diff --git a/lib/gitlab/tracking/docs/renderer.rb b/lib/gitlab/tracking/docs/renderer.rb deleted file mode 100644 index 184b935c2ba..00000000000 --- a/lib/gitlab/tracking/docs/renderer.rb +++ /dev/null @@ -1,32 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Tracking - module Docs - class Renderer - include Gitlab::Tracking::Docs::Helper - DICTIONARY_PATH = Rails.root.join('doc', 'development', 'snowplow') - TEMPLATE_PATH = Rails.root.join('lib', 'gitlab', 'tracking', 'docs', 'templates', 'default.md.haml') - - def initialize(event_definitions) - @layout = Haml::Engine.new(File.read(TEMPLATE_PATH)) - @event_definitions = event_definitions.sort - end - - def contents - # Render and remove an extra trailing new line - @contents ||= @layout.render(self, event_definitions: @event_definitions).sub!(/\n(?=\Z)/, '') - end - - def write - filename = DICTIONARY_PATH.join('dictionary.md').to_s - - FileUtils.mkdir_p(DICTIONARY_PATH) - File.write(filename, contents) - - filename - end - end - end - end -end diff --git a/lib/gitlab/tracking/docs/templates/default.md.haml b/lib/gitlab/tracking/docs/templates/default.md.haml deleted file mode 100644 index 568f56590fa..00000000000 --- a/lib/gitlab/tracking/docs/templates/default.md.haml +++ /dev/null @@ -1,35 +0,0 @@ -= auto_generated_comment - -:plain - # Event Dictionary - - This file is autogenerated, please do not edit it directly. - - To generate these files from the GitLab repository, run: - - ```shell - bundle exec rake gitlab:snowplow:generate_event_dictionary - ``` - - The Event Dictionary is based on the following event definition YAML files: - - - [`config/events`](https://gitlab.com/gitlab-org/gitlab/-/tree/f9a404301ca22d038e7b9a9eb08d9c1bbd6c4d84/config/events) - - [`ee/config/events`](https://gitlab.com/gitlab-org/gitlab/-/tree/f9a404301ca22d038e7b9a9eb08d9c1bbd6c4d84/ee/config/events) - - ## Event definitions - -\ -- event_definitions.each do |_path, object| - - = "### `#{object.category} #{object.action}`" - \ - = render_event_taxonomy(object) - \ - = render_description(object) - \ - = render_yaml_definition_path(object) - \ - = render_owner(object) - \ - = render_tiers(object) - \ diff --git a/lib/tasks/gitlab/snowplow.rake b/lib/tasks/gitlab/snowplow.rake deleted file mode 100644 index 278ba4a471c..00000000000 --- a/lib/tasks/gitlab/snowplow.rake +++ /dev/null @@ -1,11 +0,0 @@ -# frozen_string_literal: true - -namespace :gitlab do - namespace :snowplow do - desc 'GitLab | Snowplow | Generate event dictionary' - task generate_event_dictionary: :environment do - items = Gitlab::Tracking::EventDefinition.definitions - Gitlab::Tracking::Docs::Renderer.new(items).write - end - end -end diff --git a/qa/qa/support/formatters/test_stats_formatter.rb b/qa/qa/support/formatters/test_stats_formatter.rb index 0f76a924b10..5c0daca3b06 100644 --- a/qa/qa/support/formatters/test_stats_formatter.rb +++ b/qa/qa/support/formatters/test_stats_formatter.rb @@ -57,19 +57,22 @@ module QA # @param [RSpec::Core::Example] example # @return [Hash] def test_stats(example) + file_path = example.metadata[:file_path].gsub('./qa/specs/features', '') + { name: 'test-stats', time: time, tags: { name: example.full_description, - file_path: example.metadata[:file_path].gsub('./qa/specs/features', ''), + file_path: file_path, status: example.execution_result.status, reliable: example.metadata.key?(:reliable).to_s, quarantined: example.metadata.key?(:quarantine).to_s, retried: ((example.metadata[:retry_attempts] || 0) > 0).to_s, job_name: job_name, merge_request: merge_request, - run_type: env('QA_RUN_TYPE') || run_type + run_type: env('QA_RUN_TYPE') || run_type, + stage: devops_stage(file_path) }, fields: { id: example.id, @@ -150,6 +153,14 @@ module QA ENV[name] end + + # Get spec devops stage + # + # @param [String] location + # @return [String, nil] + def devops_stage(file_path) + file_path.match(%r{(\d{1,2}_\w+)/})&.captures&.first + end end end end diff --git a/qa/spec/support/formatters/test_stats_formatter_spec.rb b/qa/spec/support/formatters/test_stats_formatter_spec.rb index fec7ec1c7c0..a14452debaa 100644 --- a/qa/spec/support/formatters/test_stats_formatter_spec.rb +++ b/qa/spec/support/formatters/test_stats_formatter_spec.rb @@ -18,6 +18,8 @@ describe QA::Support::Formatters::TestStatsFormatter do let(:quarantined) { 'false' } let(:influx_client) { instance_double('InfluxDB2::Client', create_write_api: influx_write_api) } let(:influx_write_api) { instance_double('InfluxDB2::WriteApi', write: nil) } + let(:stage) { '1_manage' } + let(:file_path) { "./qa/specs/features/#{stage}/subfolder/some_spec.rb" } let(:influx_client_args) do { @@ -34,14 +36,15 @@ describe QA::Support::Formatters::TestStatsFormatter do time: DateTime.strptime(ci_timestamp).to_time, tags: { name: 'stats export spec', - file_path: './spec/support/formatters/test_stats_formatter_spec.rb', + file_path: file_path.gsub('./qa/specs/features', ''), status: :passed, reliable: reliable, quarantined: quarantined, retried: "false", job_name: "test-job", merge_request: "false", - run_type: run_type + run_type: run_type, + stage: stage }, fields: { id: './spec/support/formatters/test_stats_formatter_spec.rb[1:1]', @@ -57,7 +60,9 @@ describe QA::Support::Formatters::TestStatsFormatter do def run_spec(&spec) spec ||= -> { it('spec') {} } - describe_successfully('stats export', &spec) + describe_successfully('stats export', &spec).tap do |example_group| + example_group.examples.each { |ex| ex.metadata[:file_path] = file_path } + end send_stop_notification end diff --git a/spec/lib/bulk_imports/clients/http_spec.rb b/spec/lib/bulk_imports/clients/http_spec.rb index c36cb80851a..023562626a1 100644 --- a/spec/lib/bulk_imports/clients/http_spec.rb +++ b/spec/lib/bulk_imports/clients/http_spec.rb @@ -32,7 +32,7 @@ RSpec.describe BulkImports::Clients::HTTP do it 'raises BulkImports::Error' do allow(Gitlab::HTTP).to receive(method).and_raise(Errno::ECONNREFUSED) - expect { subject.public_send(method, resource) }.to raise_exception(BulkImports::Error) + expect { subject.public_send(method, resource) }.to raise_exception(BulkImports::NetworkError) end end @@ -42,7 +42,7 @@ RSpec.describe BulkImports::Clients::HTTP do allow(Gitlab::HTTP).to receive(method).and_return(response_double) - expect { subject.public_send(method, resource) }.to raise_exception(BulkImports::Error) + expect { subject.public_send(method, resource) }.to raise_exception(BulkImports::NetworkError) end end end @@ -180,7 +180,11 @@ RSpec.describe BulkImports::Clients::HTTP do let(:version) { '13.0.0' } it 'raises an error' do - expect { subject.get(resource) }.to raise_error(::BulkImports::Error, "Unsupported GitLab Version. Minimum Supported Gitlab Version #{BulkImport::MINIMUM_GITLAB_MAJOR_VERSION}.") + expect { subject.get(resource) } + .to raise_error( + ::BulkImports::Error, + "Unsupported GitLab Version. Minimum Supported Gitlab Version #{BulkImport::MINIMUM_GITLAB_MAJOR_VERSION}." + ) end end diff --git a/spec/lib/bulk_imports/network_error_spec.rb b/spec/lib/bulk_imports/network_error_spec.rb new file mode 100644 index 00000000000..11f555fee09 --- /dev/null +++ b/spec/lib/bulk_imports/network_error_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe BulkImports::NetworkError, :clean_gitlab_redis_cache do + let(:tracker) { double(id: 1, stage: 2, entity: double(id: 3)) } + + describe '.new' do + it 'requires either a message or a HTTP response' do + expect { described_class.new } + .to raise_error(ArgumentError, 'message or response required') + end + end + + describe '#retriable?' do + it 'returns true for MAX_RETRIABLE_COUNT times when cause if one of RETRIABLE_EXCEPTIONS' do + raise described_class::RETRIABLE_EXCEPTIONS.sample + rescue StandardError => cause + begin + raise described_class, cause + rescue StandardError => exception + described_class::MAX_RETRIABLE_COUNT.times do + expect(exception.retriable?(tracker)).to eq(true) + end + + expect(exception.retriable?(tracker)).to eq(false) + end + end + + it 'returns true for MAX_RETRIABLE_COUNT times when response is one of RETRIABLE_CODES' do + exception = described_class.new(response: double(code: 429)) + + described_class::MAX_RETRIABLE_COUNT.times do + expect(exception.retriable?(tracker)).to eq(true) + end + + expect(exception.retriable?(tracker)).to eq(false) + end + + it 'returns false for other exceptions' do + raise StandardError + rescue StandardError => cause + begin + raise described_class, cause + rescue StandardError => exception + expect(exception.retriable?(tracker)).to eq(false) + end + end + end + + describe '#retry_delay' do + it 'returns the default value when there is not a rate limit error' do + exception = described_class.new('foo') + + expect(exception.retry_delay).to eq(described_class::DEFAULT_RETRY_DELAY_SECONDS.seconds) + end + + context 'when the exception is a rate limit error' do + it 'returns the "Retry-After"' do + exception = described_class.new(response: double(code: 429, headers: { 'Retry-After' => 20 })) + + expect(exception.retry_delay).to eq(20.seconds) + end + + it 'returns the default value when there is no "Retry-After" header' do + exception = described_class.new(response: double(code: 429, headers: {})) + + expect(exception.retry_delay).to eq(described_class::DEFAULT_RETRY_DELAY_SECONDS.seconds) + end + end + end +end diff --git a/spec/lib/gitlab/cache/import/caching_spec.rb b/spec/lib/gitlab/cache/import/caching_spec.rb index f770960e27a..946a7c604a1 100644 --- a/spec/lib/gitlab/cache/import/caching_spec.rb +++ b/spec/lib/gitlab/cache/import/caching_spec.rb @@ -58,6 +58,16 @@ RSpec.describe Gitlab::Cache::Import::Caching, :clean_gitlab_redis_cache do end end + describe '.increment' do + it 'increment a key and returns the current value' do + expect(described_class.increment('foo')).to eq(1) + + value = Gitlab::Redis::Cache.with { |r| r.get(described_class.cache_key_for('foo')) } + + expect(value.to_i).to eq(1) + end + end + describe '.set_add' do it 'adds a value to a set' do described_class.set_add('foo', 10) diff --git a/spec/lib/gitlab/tracking/docs/helper_spec.rb b/spec/lib/gitlab/tracking/docs/helper_spec.rb deleted file mode 100644 index 5f7965502f1..00000000000 --- a/spec/lib/gitlab/tracking/docs/helper_spec.rb +++ /dev/null @@ -1,91 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::Tracking::Docs::Helper do - let_it_be(:klass) do - Class.new do - include Gitlab::Tracking::Docs::Helper - end - end - - describe '#auto_generated_comment' do - it 'renders information about missing description' do - expect(klass.new.auto_generated_comment).to match /This documentation is auto generated by a script/ - end - end - - describe '#render_description' do - context 'description is empty' do - it 'renders information about missing description' do - object = double(description: '') - - expect(klass.new.render_description(object)).to eq('Missing description') - end - end - - context 'description is present' do - it 'render description' do - object = double(description: 'some description') - - expect(klass.new.render_description(object)).to eq('some description') - end - end - end - - describe '#render_event_taxonomy' do - it 'render table with event taxonomy' do - attributes = { - category: 'epics', - action: 'promote', - label: nil, - property_description: 'String with issue id', - value_description: 'Integer issue id' - } - object = double(attributes: attributes) - event_taxonomy = <<~MD.chomp - | category | action | label | property | value | - |---|---|---|---|---| - | `epics` | `promote` | `` | `String with issue id` | `Integer issue id` | - MD - - expect(klass.new.render_event_taxonomy(object)).to eq(event_taxonomy) - end - end - - describe '#md_link_to' do - it 'render link in md format' do - expect(klass.new.md_link_to('zelda', 'link')).to eq('[zelda](link)') - end - end - - describe '#render_owner' do - it 'render information about group owning event' do - object = double(product_group: "group::product intelligence") - - expect(klass.new.render_owner(object)).to eq("Owner: `group::product intelligence`") - end - end - - describe '#render_tiers' do - it 'render information about tiers' do - object = double(tiers: %w[bronze silver gold]) - - expect(klass.new.render_tiers(object)).to eq("Tiers: `bronze`, `silver`, `gold`") - end - end - - describe '#render_yaml_definition_path' do - it 'render relative location of yaml definition' do - object = double(yaml_path: 'config/events/button_click.yaml') - - expect(klass.new.render_yaml_definition_path(object)).to eq("YAML definition: `config/events/button_click.yaml`") - end - end - - describe '#backtick' do - it 'wraps string in backticks chars' do - expect(klass.new.backtick('test')).to eql("`test`") - end - end -end diff --git a/spec/lib/gitlab/tracking/docs/renderer_spec.rb b/spec/lib/gitlab/tracking/docs/renderer_spec.rb deleted file mode 100644 index 386aea6c23a..00000000000 --- a/spec/lib/gitlab/tracking/docs/renderer_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::Tracking::Docs::Renderer do - describe 'contents' do - let(:dictionary_path) { described_class::DICTIONARY_PATH } - let(:items) { Gitlab::Tracking::EventDefinition.definitions.first(10).to_h } - - it 'generates dictionary for given items' do - generated_dictionary = described_class.new(items).contents - table_of_contents_items = items.values.map { |item| "#{item.category} #{item.action}"} - - generated_dictionary_keys = RDoc::Markdown - .parse(generated_dictionary) - .table_of_contents - .select { |metric_doc| metric_doc.level == 3 } - .map { |item| item.text.match(%r{(.*)})&.captures&.first } - - expect(generated_dictionary_keys).to match_array(table_of_contents_items) - end - end -end diff --git a/spec/workers/bulk_imports/pipeline_worker_spec.rb b/spec/workers/bulk_imports/pipeline_worker_spec.rb index 56f28654ac5..fc70a2582e4 100644 --- a/spec/workers/bulk_imports/pipeline_worker_spec.rb +++ b/spec/workers/bulk_imports/pipeline_worker_spec.rb @@ -27,42 +27,59 @@ RSpec.describe BulkImports::PipelineWorker do .and_return([[0, pipeline_class]]) end - it 'runs the given pipeline successfully' do - pipeline_tracker = create( - :bulk_import_tracker, - entity: entity, - pipeline_name: 'FakePipeline' - ) + shared_examples 'successfully runs the pipeline' do + it 'runs the given pipeline successfully' do + expect_next_instance_of(Gitlab::Import::Logger) do |logger| + expect(logger) + .to receive(:info) + .with( + worker: described_class.name, + pipeline_name: 'FakePipeline', + entity_id: entity.id + ) + end - expect_next_instance_of(Gitlab::Import::Logger) do |logger| - expect(logger) - .to receive(:info) - .with( - worker: described_class.name, - pipeline_name: 'FakePipeline', - entity_id: entity.id - ) + expect(BulkImports::EntityWorker) + .to receive(:perform_async) + .with(entity.id, pipeline_tracker.stage) + + expect(subject).to receive(:jid).and_return('jid') + + subject.perform(pipeline_tracker.id, pipeline_tracker.stage, entity.id) + + pipeline_tracker.reload + + expect(pipeline_tracker.status_name).to eq(:finished) + expect(pipeline_tracker.jid).to eq('jid') end + end - expect(BulkImports::EntityWorker) - .to receive(:perform_async) - .with(entity.id, pipeline_tracker.stage) + it_behaves_like 'successfully runs the pipeline' do + let(:pipeline_tracker) do + create( + :bulk_import_tracker, + entity: entity, + pipeline_name: 'FakePipeline' + ) + end + end - expect(subject).to receive(:jid).and_return('jid') - - subject.perform(pipeline_tracker.id, pipeline_tracker.stage, entity.id) - - pipeline_tracker.reload - - expect(pipeline_tracker.status_name).to eq(:finished) - expect(pipeline_tracker.jid).to eq('jid') + it_behaves_like 'successfully runs the pipeline' do + let(:pipeline_tracker) do + create( + :bulk_import_tracker, + :started, + entity: entity, + pipeline_name: 'FakePipeline' + ) + end end context 'when the pipeline cannot be found' do it 'logs the error' do pipeline_tracker = create( :bulk_import_tracker, - :started, + :finished, entity: entity, pipeline_name: 'FakePipeline' ) @@ -126,6 +143,39 @@ RSpec.describe BulkImports::PipelineWorker do expect(pipeline_tracker.status_name).to eq(:failed) expect(pipeline_tracker.jid).to eq('jid') end + + context 'when it is a network error' do + it 'reenqueue on retriable network errors' do + pipeline_tracker = create( + :bulk_import_tracker, + entity: entity, + pipeline_name: 'FakePipeline' + ) + + exception = BulkImports::NetworkError.new( + response: double(code: 429, headers: {}) + ) + + expect_next_instance_of(pipeline_class) do |pipeline| + expect(pipeline) + .to receive(:run) + .and_raise(exception) + end + + expect(subject).to receive(:jid).and_return('jid') + + expect(described_class) + .to receive(:perform_in) + .with( + 60.seconds, + pipeline_tracker.id, + pipeline_tracker.stage, + pipeline_tracker.entity.id + ) + + subject.perform(pipeline_tracker.id, pipeline_tracker.stage, entity.id) + end + end end context 'when ndjson pipeline' do