BG migration to fix incorrect job artifacts expire_at on self-managed
What does this MR do and why?
Fix remaining incorrect expire_at
on self-managed instances that was set by !47723 (merged). This gives instances that may not have fully executed the fix in DestroyBatchService
to still have a way to remove the incorrect values.
This MR adds a temporary index, which will be removed later on, tracked in #368979 (closed).
Screenshots or screen recordings
These are strongly recommended to assist reviewers and reduce the time to merge your change.
DB Queries
Create index:
CREATE INDEX tmp_index_ci_job_artifacts_on_id_expire_at_file_type_trace ON ci_job_artifacts USING btree (id, expire_at, file_type) WHERE (((date_part('day'::text, timezone('UTC'::text, expire_at)) = ANY (ARRAY[(21)::double precision, (22)::double precision, (23)::double precision])) AND (date_part('minute'::text, timezone('UTC'::text, expire_at)) = ANY (ARRAY[(0)::double precision, (30)::double precision, (45)::double precision])) AND (date_part('second'::text, timezone('UTC'::text, expire_at)) = (0)::double precision)) OR (file_type = 3));
Batching and update:
SELECT
"ci_job_artifacts"."id"
FROM
"ci_job_artifacts"
WHERE
"ci_job_artifacts"."id" BETWEEN 1 AND 100
ORDER BY
"ci_job_artifacts"."id" ASC
LIMIT 1
--
SELECT
"ci_job_artifacts"."id"
FROM
"ci_job_artifacts"
WHERE
"ci_job_artifacts"."id" BETWEEN 1 AND 100
AND "ci_job_artifacts"."id" >= 1
ORDER BY
"ci_job_artifacts"."id" ASC
LIMIT 1 OFFSET 10
--
UPDATE
"ci_job_artifacts"
SET
"expire_at" = NULL
WHERE
"ci_job_artifacts"."id" BETWEEN 1 AND 100
AND "ci_job_artifacts"."id" >= 1
AND "ci_job_artifacts"."id" < 11
AND (EXTRACT(day FROM timezone('UTC', expire_at)) IN (21, 22, 23)
AND EXTRACT(minute FROM timezone('UTC', expire_at)) IN (0, 30, 45)
AND EXTRACT(second FROM timezone('UTC', expire_at)) = 0
OR "ci_job_artifacts"."file_type" = 3)
--
SELECT
"ci_job_artifacts"."id"
FROM
"ci_job_artifacts"
WHERE
"ci_job_artifacts"."id" BETWEEN 1 AND 100
AND "ci_job_artifacts"."id" >= 11
ORDER BY
"ci_job_artifacts"."id" ASC
LIMIT 1 OFFSET 10
--
UPDATE
"ci_job_artifacts"
SET
"expire_at" = NULL
WHERE
"ci_job_artifacts"."id" BETWEEN 1 AND 100
AND "ci_job_artifacts"."id" >= 11
AND (EXTRACT(day FROM timezone('UTC', expire_at)) IN (21, 22, 23)
AND EXTRACT(minute FROM timezone('UTC', expire_at)) IN (0, 30, 45)
AND EXTRACT(second FROM timezone('UTC', expire_at)) = 0
OR "ci_job_artifacts"."file_type" = 3)
query plans:
- batching: https://console.postgres.ai/gitlab/gitlab-production-ci/sessions/11075/commands/40091
- update: https://postgres.ai/console/gitlab/gitlab-production-ci/sessions/11075/commands/40095
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #355833 (closed)
Merge request reports
Activity
changed milestone to %15.1
assigned to @alberts-gitlab
added typefeature label and removed typemaintenance label
Suggested Reviewers (beta)
The individuals below may be good candidates to participate in the review based on various factors.
You can use slash commands in comments to quickly assign
/assign_reviewer @user1
.Suggested Reviewers @mayra-cabrera
,@rspeicher
,@dbalexandre
,@psimyn
,@ahegyi
If you do not believe these suggestions are useful, please apply the label Bad Suggested Reviewer. You can also provide feedback for this feature on this issue:
https://gitlab.com/gitlab-org/gitlab/-/issues/357923
.Automatically generated by Suggested Reviewers Bot - an experimental ML-based recommendation engine created by ~"group::applied ml".
Edited by GitLab Reviewer-Recommender Botadded 1 commit
- b1c7b6c6 - Add background migration to fix incorrect job artifacts expire_at on self-managed
1 Warning featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.
For more information, see:
- The Handbook page on merge request types.
- The definition of done documentation.
1 Message This merge request adds or changes files that require a review from the Database team. This merge request requires a database review. To make sure these changes are reviewed, take the following steps:
-
Ensure the merge request has database and databasereview pending labels. If the merge request modifies database files, Danger will do this for you.
-
Prepare your MR for database review according to the docs.
-
Assign and mention the database reviewer suggested by Reviewer Roulette.
-
If this is not a Community contribution or from a Fork, kick off the
db:gitlabcom-database-testing
manual job.
The following files require a review from the Database team:
db/migrate/20220606080509_add_tmp_index_job_artifacts_id_and_expire_at.rb
db/post_migrate/20220606054503_fix_incorrect_job_artifacts_expire_at.rb
db/schema_migrations/20220606054503
db/schema_migrations/20220606080509
lib/gitlab/background_migration/batching_strategies/remove_backfilled_job_artifacts_expire_at_batching_strategy.rb
lib/gitlab/background_migration/remove_backfilled_job_artifacts_expire_at.rb
spec/migrations/20220606054503_fix_incorrect_job_artifacts_expire_at_spec.rb
db/structure.sql
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Maxime Orefice ( @morefice
) (UTC+2, 6 hours behind@alberts-gitlab
)Doug Stull ( @dstull
) (UTC-4, 12 hours behind@alberts-gitlab
)database Omar Qunsul ( @OmarQunsulGitlab
) (UTC+2, 6 hours behind@alberts-gitlab
)Dylan Griffith ( @DylanGriffith
) (UTC-4, 12 hours behind@alberts-gitlab
)~"migration" No reviewer available No maintainer available To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
DangerAllure report
allure-report-publisher
generated test report!review-qa-blocking:
test report for d97559c0expand test summary
+-----------------------------------------------------------------------------------------+ | suites summary | +------------------------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Plan | 47 | 0 | 1 | 45 | 48 | ❗ | | Create | 24 | 0 | 1 | 19 | 25 | ❗ | | Version sanity check | 0 | 0 | 1 | 0 | 1 | ➖ | | Feature flag handler sanity checks | 9 | 0 | 0 | 0 | 9 | ✅ | | Verify | 14 | 0 | 1 | 11 | 15 | ❗ | | Protect | 2 | 0 | 0 | 2 | 2 | ❗ | | Manage | 38 | 0 | 2 | 35 | 40 | ❗ | | Secure | 2 | 0 | 0 | 2 | 2 | ❗ | | Configure | 0 | 0 | 1 | 0 | 1 | ➖ | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Total | 136 | 0 | 7 | 114 | 143 | ❗ | +------------------------------------+--------+--------+---------+-------+-------+--------+
mentioned in merge request !88392 (merged)
cc @drew
requested review from @dfrazao-gitlab
- Resolved by Albert
Given that we have a fix in place for customers on a regular version-by-version upgrade path, how do we feel about writing this as a regular migration?
If I recall correctly, customers executing multi-version upgrades expect to do so with downtime. Since the write operation itself is idempotent, we could design a migration that clears the timestamps with a relatively conserved batched pacing strategy, and if it fails or the whole migration operation times out for some reason, the customer can simply retry the migration to pick up where the failed execution left off.
There are a few contingencies and a willing to accept failing migrations in this plan, so I'm not sure it's the best one. But I do think it would get us more certainty about the eventual outcome.
@drew to reply to your comments, I still think a batched background migration is preferred, because it gives option for users who have gone version-by-version up to 15.0 and want to continue with 15.1 and 15.2 upgrade without downtime.
Those that can accept downtime and would want to go directly from 15.0 to 15.2 can still do so with a batched background migration.
added 1 commit
- 3bdc36ca - Add background migration to fix incorrect job artifacts expire_at on self-managed
- Resolved by Albert
Database migrations
Migrations included in this change have been executed on gitlab.com data for testing purposes. For details, please see the migration testing pipeline (limited access).
Migration Type Total runtime Result DB size change 20220606080509 - AddTmpIndexJobArtifactsIdAndExpireAt Regular 1.9 s +0.00 B 20220606054503 - FixIncorrectJobArtifactsExpireAt Post deploy 1.6 s +0.00 B Migration: 20220606080509 - AddTmpIndexJobArtifactsIdAndExpireAt
- Type: Regular
- Duration: 1.9 s
- Database size change: +0.00 B
Migration: 20220606054503 - FixIncorrectJobArtifactsExpireAt
- Type: Post deploy
- Duration: 1.6 s
- Database size change: +0.00 B
Background migrations
Other migrations pending on GitLab.com
Migration Type Total runtime Result DB size change Clone Details
Clone ID Clone Created At Clone Data Timestamp Expected Removal Time database-testing-1365961-7781634-main
2022-08-16T08:51:18Z 2022-08-16T08:02:34Z 2022-08-16 20:53:46 +0000 Artifacts
Brought to you by gitlab-org/database-team/gitlab-com-database-testing. Epic
- A deleted user
added database-testing-automation label
- Resolved by Adam Hegyi
added 736 commits
-
3bdc36ca...77ad7b23 - 735 commits from branch
master
- d2d882a4 - Add background migration to fix incorrect job artifacts expire_at on self-managed
-
3bdc36ca...77ad7b23 - 735 commits from branch
added 1 commit
- b13738c5 - Fixme: Remove Gitlab.com? check for DB pipeline
- Resolved by Adam Hegyi
@dfrazao-gitlab could you review database please?
@drew could you review please?
requested review from @drew
- Resolved by Albert
@drew
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline has been started.
For more info, please refer to the following links:
removed review request for @drew
- Resolved by Albert
please don't forget to add a test to the
db/post_migrate/20220606054503_fix_incorrect_job_artifacts_expire_at.rb
- Resolved by Albert
added 1758 commits
-
3c491e69...60d1cf18 - 1756 commits from branch
master
- d4f7ba32 - Fix incorrect job artifacts expire_at
- a88efbf4 - Fixme: Remove Gitlab.com? check for DB pipeline
-
3c491e69...60d1cf18 - 1756 commits from branch
@dfrazao-gitlab back to you
mentioned in issue #355833 (closed)
changed milestone to %15.2
added missed-deliverable missed:15.1 labels
- Resolved by Adam Hegyi
The table
ci_job_artifacts
is huge. We are not able to add the temporary indexes synchronously.We need to add it asynchronously
https://docs.gitlab.com/ee/development/adding_database_indexes.html#create-indexes-asynchronously
- Resolved by Adam Hegyi
Also, I suggest adding a custom batching strategy where we apply a scope. Otherwise, we will need to iterate over the whole table.
You have an example here: !89865 (merged)
- Resolved by Adam Hegyi
- Resolved by Adam Hegyi
- Resolved by Adam Hegyi
One more thing:
I see you shared the query + query plan of the update query. Could you also share the query + query plan where we retrieve the batch?
https://postgres.ai/console/gitlab/gitlab-production-tunnel-pg12/sessions/10594/commands/38123
ModifyTable on public.ci_job_artifacts (cost=0.57..3.61 rows=1 width=189) (actual time=0.020..0.021 rows=0 loops=1) Buffers: shared hit=4 I/O Timings: read=0.000 write=0.000 -> Index Scan using tmp_index_ci_job_artifacts_on_id_expire_at on public.ci_job_artifacts (cost=0.57..3.61 rows=1 width=189) (actual time=0.018..0.018 rows=0 loops=1) Index Cond: ((ci_job_artifacts.id >= '2549151000'::bigint) AND (ci_job_artifacts.id <= '2549161000'::bigint)) Filter: ((date_part('second'::text, timezone('UTC'::text, ci_job_artifacts.expire_at)) = '0'::double precision) AND (date_part('day'::text, timezone('UTC'::text, ci_job_artifacts.expire_at)) = ANY ('{21,22,23}'::double precision[])) AND (date_part('minute'::text, timezone('UTC'::text, ci_job_artifacts.expire_at)) = ANY ('{0,30,45}'::double precision[]))) Rows Removed by Filter: 0 Buffers: shared hit=4 I/O Timings: read=0.000 write=0.000
From the query plan, I see that we are using the condition (
ci_job_artifacts.id >= '2549151000'::bigint) AND (ci_job_artifacts.id <= '2549161000'::bigint)
) from the index that we added, but we still filter the data from the data returned by the index.We need to double-check this.
Edited by Diogo Frazão
mentioned in issue gitlab-com/www-gitlab-com#12063 (closed)
removed review request for @dfrazao-gitlab
added 3938 commits
-
a88efbf4...d318ed5a - 3936 commits from branch
master
- 9f368311 - Fix incorrect job artifacts expire_at
- 1863cc25 - Fixme: Remove Gitlab.com? check for DB pipeline
-
a88efbf4...d318ed5a - 3936 commits from branch
- A deleted user
added backend database databasereview pending labels
- Resolved by Albert
mentioned in merge request !92931 (merged)
changed milestone to %15.3
added missed:15.2 label
requested review from @dfrazao-gitlab
removed review request for @dfrazao-gitlab
- Resolved by Adam Hegyi
added 746 commits
-
cd67385b...6810f4df - 739 commits from branch
master
- 4498f020 - Fix incorrect job artifacts expire_at
- 61be938c - Add trace job artifact to batching scope
- d5ebf5f6 - Add migration spec for post migration
- 1d6659bd - Add down migration to remove batch migration job
- 331b1d05 - Fixme: Remove Gitlab.com? check for DB pipeline
- ef75c7ac - Temp fix for migration spec helper
- 4eff1088 - Fix Rubocop warning
Toggle commit list-
cd67385b...6810f4df - 739 commits from branch
added 231 commits
-
4eff1088...8f58bc37 - 224 commits from branch
master
- 45ae274d - Fix incorrect job artifacts expire_at
- 89cb19d1 - Add trace job artifact to batching scope
- 60a92c8f - Add migration spec for post migration
- fff95ace - Add down migration to remove batch migration job
- a4a5f135 - Fixme: Remove Gitlab.com? check for DB pipeline
- 84d9ef02 - Temp fix for migration spec helper
- 9fb06dee - Fix Rubocop warning
Toggle commit list-
4eff1088...8f58bc37 - 224 commits from branch
requested review from @dfrazao-gitlab
- Resolved by Albert
- Resolved by Albert
mentioned in issue gitlab-org/database-team/gitlab-com-database-testing#70 (closed)
mentioned in merge request !87523 (closed)
@ahegyi could you help to review the index please?
requested review from @ahegyi
mentioned in issue #368979 (closed)
added 1 commit
- 17162c66 - Add custom batching strategy to skip out of scope records
1 # frozen_string_literal: true 2 3 class FixIncorrectJobArtifactsExpireAt < Gitlab::Database::Migration[2.0] 4 disable_ddl_transaction! 5 6 # FIXME: Revert to :gitlab_ci. 7 # :gitlab_main is required during review because db test pipeline is not configured to use ci 8 restrict_gitlab_migration gitlab_schema: :gitlab_main changed this line in version 19 of the diff
- Resolved by Albert
- Resolved by Albert
- Resolved by Albert
added 704 commits
-
b95115b2...4934301d - 700 commits from branch
master
- d88375a7 - Fix incorrect job artifacts expire_at
- 5391ef91 - Move where clause to sub batch
- 7ea559ab - Add custom batching strategy to skip out of scope records
- d97559c0 - Fixme: Remove Gitlab.com? check for DB pipeline
Toggle commit list-
b95115b2...4934301d - 700 commits from branch
added 1486 commits
-
d97559c0...a18c1d08 - 1482 commits from branch
master
- 84109556 - Fix incorrect job artifacts expire_at
- ec74d3d9 - Move where clause to sub batch
- afc86aa7 - Add custom batching strategy to skip out of scope records
- 1083e13d - Fixme: Remove Gitlab.com? check for DB pipeline
Toggle commit list-
d97559c0...a18c1d08 - 1482 commits from branch
mentioned in merge request !95038 (merged)
added 788 commits
-
1083e13d...236ecb26 - 784 commits from branch
master
- d140a096 - Fix incorrect job artifacts expire_at
- 26a95308 - Move where clause to sub batch
- befc5f2c - Add custom batching strategy to skip out of scope records
- ba7dfc39 - Fixme: Remove Gitlab.com? check for DB pipeline
Toggle commit list-
1083e13d...236ecb26 - 784 commits from branch
- Resolved by Adam Hegyi
- Resolved by Adam Hegyi
- Resolved by Adam Hegyi
added 825 commits
-
b2c71cbb...6f02c1d7 - 819 commits from branch
master
- 80db3465 - Fix incorrect job artifacts expire_at
- a5d10e14 - Move where clause to sub batch
- 29fe5724 - Add custom batching strategy to skip out of scope records
- 37ef6874 - Fixme: Remove Gitlab.com? check for DB pipeline
- c5370894 - Remove unneeded select distinct ids
- 834359dd - Check for existing index in migration
Toggle commit list-
b2c71cbb...6f02c1d7 - 819 commits from branch
added 2 commits
changed milestone to %15.4
added missed:15.3 label
added 180 commits
-
ab43ee03...4eabac4b - 174 commits from branch
master
- f9f29072 - Fix incorrect job artifacts expire_at
- 00d232ad - Move where clause to sub batch
- 3af16057 - Add custom batching strategy to skip out of scope records
- bd296a00 - Remove unneeded select distinct ids
- 0d579f02 - Check for existing index in migration
- b047d31c - Update DB schema for consistency
Toggle commit list-
ab43ee03...4eabac4b - 174 commits from branch
added documentation label
- Resolved by Adam Hegyi
@marcel.amirault could you review the documentation please?
@drew could you review backend please?
requested review from @drew and @marcel.amirault
added 200 commits
-
b047d31c...569e2476 - 193 commits from branch
master
- 571e3a0e - Fix incorrect job artifacts expire_at
- 9c8509b8 - Move where clause to sub batch
- 03630f32 - Add custom batching strategy to skip out of scope records
- b8ab267d - Remove unneeded select distinct ids
- 9837f453 - Check for existing index in migration
- 416abfb4 - Update DB schema for consistency
- efa45fc4 - Add version specific upgrade instruction
Toggle commit list-
b047d31c...569e2476 - 193 commits from branch
- Resolved by Albert
- Resolved by Albert
- Resolved by Albert
1 Message This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge. Documentation review
The following files require a review from a technical writer:
doc/update/index.md
The review does not need to block merging this merge request. See the:
-
Metadata for the
*.md
files that you've changed. The first few lines of each*.md
file identify the stage and group most closely associated with your docs change. - The Technical Writer assigned for that stage and group.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Sam Figueroa ( @sam.figueroa
) (UTC+0, 8 hours behind@alberts-gitlab
)Sincheol (David) Kim ( @dskim_gitlab
) (UTC+9.5, 1.5 hours ahead of@alberts-gitlab
)database Omar Qunsul ( @OmarQunsulGitlab
) (UTC+2, 6 hours behind@alberts-gitlab
)Krasimir Angelov ( @krasio
) (UTC+12, 4 hours ahead of@alberts-gitlab
)~"migration" No reviewer available No maintainer available To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger- Resolved by Albert
added Technical Writing docsfeature labels
removed review request for @marcel.amirault
- Resolved by Albert
- Resolved by Albert
@alberts-gitlab, this is super thorough, excellent work!
My only concern is about shipping the concurrent index build in
db/migrate
instead ofdb/post_migrate
. I'm guessing you have them set up that way to be able to ship them at the same time, instead of waiting another week the send the background migration in it's own MR. If database / groupdelivery is cool with this, go for it.I'm approving, since all the business logic and Ruby looks both good, and incredibly tested.
removed review request for @drew
added 743 commits
-
e2b5900a...343c151a - 735 commits from branch
master
- 34f96994 - Fix incorrect job artifacts expire_at
- 7f554c52 - Move where clause to sub batch
- 0698ad20 - Add custom batching strategy to skip out of scope records
- d831a408 - Remove unneeded select distinct ids
- 2a54ff3e - Check for existing index in migration
- efecdf50 - Update DB schema for consistency
- 807ec61e - Add version specific upgrade instruction
- ee16a84b - Move index creation to post_migrate
Toggle commit list-
e2b5900a...343c151a - 735 commits from branch
@ahegyi This is ready now. Could you do a final review please?
Thanks, @alberts-gitlab! DB LGTM!
@alberts-gitlab @ahegyi FYI
apply_additional_filters
is deprecated now, as we no longer need to create custom batching strategy to apply additional filters. We can use the recently introducesscope_to
:class RemoveBackfilledJobArtifactsExpireAt < BatchedMigrationJob scope_to ->(relation) { relation.where(EXPIRES_ON_21_22_23_AT_MIDNIGHT_IN_TIMEZONE) .or(relation.where(file_type: 3)) } end
I have a MR to remove
apply_additional_filters
- !96478 (merged), but it's merge result pipeline failed because this one was merged meanwhile. I'll update the migration and the batching strategy to use the new approach in !96478 (merged).
added databaseapproved label and removed databasereview pending label
enabled an automatic merge when the pipeline for 18df0c12 succeeds
mentioned in commit 3e04c65c
added Category:Build Artifacts workflowstaging-canary labels and removed Category:Build Artifacts workflowin review labels
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added workflowpost-deploy-db-staging label and removed workflowproduction label
4 disable_ddl_transaction! 5 6 INDEX_NAME = 'tmp_index_ci_job_artifacts_on_id_expire_at_file_type_trace' 7 8 EXPIRE_AT_ON_22_MIDNIGHT_IN_TIMEZONE_OR_TRACE = <<~SQL 9 (EXTRACT(day FROM timezone('UTC', expire_at)) IN (21, 22, 23) 10 AND EXTRACT(minute FROM timezone('UTC', expire_at)) IN (0, 30, 45) 11 AND EXTRACT(second FROM timezone('UTC', expire_at)) = 0) 12 OR file_type = 3 13 SQL 14 15 def up 16 return if Gitlab.com? 17 return if index_exists_by_name?(:ci_job_artifacts, INDEX_NAME) 18 19 add_concurrent_index :ci_job_artifacts, :id, @alberts-gitlab Why do we create the index after the migration is scheduled? Isn't there any chance that its execution start before the index is present?
@alberts-gitlab, I totally missed this. We can fix this by replacing the content of the two migration files. We can do it as part of !96478 (merged)
@alberts-gitlab, you can push a commit to Kras's MR: !96478 (merged)
21 # on the dates 21, 22, 23 of the month will not be deleted. 22 # https://en.wikipedia.org/wiki/List_of_UTC_time_offsets 23 EXPIRES_ON_21_22_23_AT_MIDNIGHT_IN_TIMEZONE = <<~SQL 24 EXTRACT(day FROM timezone('UTC', expire_at)) IN (21, 22, 23) 25 AND EXTRACT(minute FROM timezone('UTC', expire_at)) IN (0, 30, 45) 26 AND EXTRACT(second FROM timezone('UTC', expire_at)) = 0 27 SQL 28 29 def perform 30 each_sub_batch( 31 operation_name: :update_all 32 ) do |sub_batch| 33 sub_batch.where(EXPIRES_ON_21_22_23_AT_MIDNIGHT_IN_TIMEZONE) 34 .or(sub_batch.where(file_type: 3)) 35 .update_all(expire_at: nil) 36 end @alberts-gitlab @ahegyi When we use
apply_additional_filters
, the same filters should be applied here asbatching_scope
, otherwise the batch generated by the job class will be different (may include more rows) from the batch generated by the batching strategy.Yes, it's confusing and error-prone, this is one of the reasons for switching to
scope_to
- the filters defined this way will be used by both the batching strategy and the job class, and we do not have to define custom batching strategy class.We're still fine here, as we apply the filters on the yielded relation (
sub_batch
). I'll update this as part of !96478 (merged) anyway,Updated with !96478 (2097adef), can you please have a look and confirm all is good?
Thanks, @krasio! I'll take a look.
mentioned in merge request !96478 (merged)
mentioned in merge request !96644 (merged)
added Category:Build Artifacts releasedcandidate labels and removed Category:Build Artifacts label
mentioned in merge request kubitus-project/kubitus-installer!1453 (merged)
380 380 accordingly, while also consulting the 381 381 [version-specific upgrade instructions](#version-specific-upgrading-instructions): 382 382 383 `8.11.Z` -> `8.12.0` -> `8.17.7` -> `9.5.10` -> `10.8.7` -> [`11.11.8`](#1200) -> `12.0.12` -> [`12.1.17`](#1210) -> [`12.10.14`](#12100) -> `13.0.14` -> [`13.1.11`](#1310) -> [`13.8.8`](#1388) -> [`13.12.15`](#13120) -> [`14.0.12`](#1400) -> [`14.3.6`](#1430) -> [`14.9.5`](#1490) -> [`14.10.Z`](#14100) -> [`15.0.Z`](#1500) -> [latest `15.Y.Z`](https://gitlab.com/gitlab-org/gitlab/-/releases) 383 `8.11.Z` -> `8.12.0` -> `8.17.7` -> `9.5.10` -> `10.8.7` -> [`11.11.8`](#1200) -> `12.0.12` -> [`12.1.17`](#1210) -> [`12.10.14`](#12100) -> `13.0.14` -> [`13.1.11`](#1310) -> [`13.8.8`](#1388) -> [`13.12.15`](#13120) -> [`14.0.12`](#1400) -> [`14.3.6`](#1430) -> [`14.9.5`](#1490) -> [`14.10.Z`](#14100) -> [`15.0.Z`](#1500) -> [`15.4.0`](#1540) -> [latest `15.Y.Z`](https://gitlab.com/gitlab-org/gitlab/-/releases) @alberts-gitlab Can users jump to any
15.4.patch
/.Z
version here as well?@katrinleinweber The same guideline should apply.
@alberts-gitlab - I'm planning the 15.3.5 to 15.4.5 migration of a self hosted GitLab, and I'm a bit surprised/worried by this change (came here from the 15.4 Upgrade instructions). Could you clarify something please?
Here is my
ci_job_artifacts
table size, and what I understand would be the scope of this background migration:gitlab-db=> SELECT COUNT(*) FROM ci_job_artifacts; count ---------- 28377368 (1 row) gitlab-db=> SELECT COUNT(*) FROM ci_job_artifacts WHERE EXTRACT(day FROM timezone('UTC', expire_at)) IN (21, 22, 23) AND EXTRACT(minute FROM timezone('UTC', expire_at)) IN (0, 30, 45) AND EXTRACT(second FROM timezone('UTC', expire_at)) = 0 OR (file_type = 3); count ---------- 24495566 (1 row)
24.5M lines, batch size is 500 and interval is 2 minutes, this would be running for 68 days. Is that correct? (I won't rule out misunderstanding the whole
BatchedMigrationJob
logic)Now, it seems the purpose of this operation is to nullify
expire_at
of non-trace artifacts when it has been improperly set by some previous migration. But on most of my data,expire_at
is alreadyNULL
:gitlab-db=> SELECT file_type, CASE WHEN expire_at IS NULL THEN 'null' ELSE 'not_null' END AS expire_at_nullity, count(*) FROM ci_job_artifacts GROUP BY file_type, expire_at_nullity ORDER BY count(*) desc; file_type | expire_at_nullity | count -----------+-------------------+---------- 3 | null | 24193110 1 | not_null | 1525349 2 | not_null | 1525150 4 | not_null | 389634 16 | not_null | 313331 3 | not_null | 302398 9 | not_null | 92787 17 | not_null | 8798 ... (26 rows)
And indeed, reducing the migration scope with an
expire_at IS NOT NULL
(for thefile_type = 3
branch) narrows it down to a tiny portion (1.2%) of the planned 24.5M lines:gitlab-db=> SELECT COUNT(*) FROM ci_job_artifacts WHERE EXTRACT(day FROM timezone('UTC', expire_at)) IN (21, 22, 23) AND EXTRACT(minute FROM timezone('UTC', expire_at)) IN (0, 30, 45) AND EXTRACT(second FROM timezone('UTC', expire_at)) = 0 OR (file_type = 3 AND expire_at IS NOT NULL); count -------- 302456 (1 row)
The background migration would then complete in 20 hours. Is that correct?
Thanks in advance for taking time to confirm or correct this analysis :)
Edited by Thomas de Grenier de Latour@thomasgl-orange The batch will still include
file_type=3
with NULLexpire_at
. https://gitlab.com/gitlab-org/gitlab/-/blob/v15.4.5-ee/lib/gitlab/background_migration/remove_backfilled_job_artifacts_expire_at.rb#L23-32.@krasio Is there a way we can backport a patch to this migration so that we can add a condition on
expire_at IS NOT NULL
?My apologies for this oversight. I think we should correct it to improve the performance.
Edited by Albert@alberts-gitlab This is something we need to check with
@gitlab-org/release/managers
, but I don't think we backport fixes that far back, unless it's a security fix.
24.5M lines, batch size is 500 and interval is 2 minutes, this would be running for 68 days. Is that correct?
Answering to myself (sorry for the noise): no, it's not correct, the
BackgroundMigration
will progressively increase the batch size, until the migration reaches a smooth pace (each batch being processed in 90-95% of 2 minutes period). So now I'm relieved. In this specific case, with only 1% of the scoped lines actually requiring an updated, and a sub-batch size of 100 lines, it seems (intuitively) that there will much more update queries than actually required, but it's okay, it won't spread over weeks as I initially feared.@thomasgl-orange This is a GitLab.com only migration, it will be a no-op for self-managed instances - https://gitlab.com/gitlab-org/gitlab/-/blob/a05ba9819e1ba56d31f7cd13279473404bbc62dd/db/post_migrate/20220606080509_fix_incorrect_job_artifacts_expire_at.rb#L14.
BackgroundMigration
will progressively increase the batch size, until the migration reaches a smooth paceThis is true only if the
optimize_batched_migrations
feature flag is enabled. You can check this in a Rails console withFeature.enabled?(:optimize_batched_migrations, type: :ops)
and enable it with
Feature.enable(:optimize_batched_migrations)
and disable it with
Feature.disable(:optimize_batched_migrations)
This is a GitLab.com only migration, it will be a no-op for self-managed instances - https://gitlab.com/gitlab-org/gitlab/-/blob/a05ba9819e1ba56d31f7cd13279473404bbc62dd/db/post_migrate/20220606080509_fix_incorrect_job_artifacts_expire_at.rb#L14
@krasio This migration is for self-managed. The condition is
return if Gitlab.com?
. Nonetheless, thanks for the:optimize_batched_migrations
suggestion.
mentioned in issue #416689 (closed)
mentioned in merge request !147443 (merged)