Add timeout support in Projects::ContainerRepository::DeleteTagsService#execute

What does this MR do?

MR 1 of #208193 (comment 362910703) MR plan:
Put a timeout around DeleteTagsService#execute but only under those conditions:
- The GitLab Registry is in use. (This is what gitlab.com uses)
- The feature flag
container_registry_expiration_policies_throttling
is enabled - Application Setting
container_registry_delete_tags_service_timeout
set to anything else thannil
or0

Design choices

- For a better understanding how this service is used, see the schema in #208193 (comment 362910703).
- This service function is really simple: it receives a list of tags, it will loop on them and sending the appropriate request to the Container Registry to delete them.
- The main issue is that these requests to the Container Registry can be really slow.
- We can easily imagine what happens when the service receives let's say 100K tags to delete. Well, let's summarize it with
- One upcoming feature is having Container Registry Cleanup Policies open to all projects. The policies are executed (with a background worker) under a given schedule and each execution will use this service to delete a given list of tags.
- To avoid resources starvation, this MR puts an execution time limit to this service.
- When the limit is hit, the loop is stopped.
- As such, we can have a partial execution which is an accepted outcome.
- The default value of 250 seconds has been selected so that we are way below from the 300 seconds threshold that is applied for background workers (see gitlab-com/gl-infra/scalability#461 (comment 370427582))
- To have a better control of this execution time limit value, it is implemented as an application setting (
container_registry_delete_tags_service_timeout
)- The corresponding UI has been added to the container settings in the admin UI (located at
/admin/application_settings/ci_cd
) - This way, we can update this value directly from the UI
- The corresponding UI has been added to the container settings in the admin UI (located at
- As stated above, this MR is part of a larger effort to limit resources usage during Container Registry Cleanup Policies execution.
- This being a deep change from the current behavior, all the limits we are implementing in the larger effort are gated behind a feature flag:
container_registry_expiration_policies_throttling
. This provides us a safety switch between non throttled executions (current implementation) <-> throttled executions. This switch can be used in the case of a production issue. - This MR being the first of the larger effort, it is the one introducing the feature flag.
- This being a deep change from the current behavior, all the limits we are implementing in the larger effort are gated behind a feature flag:
- To be complete in this MR, the corresponding documentation has been updated with a
Caution
block to indicate that Cleanup Policies can be partially executed.

Database Migrations

Up
$ rails db:migrate:up VERSION=20200710113437
== 20200710113437 AddContainerRegistryDeleteTagsServiceTimeoutToApplicationSettings: migrating
-- add_column(:application_settings, :container_registry_delete_tags_service_timeout, :integer, {:default=>250, :null=>false})
-> 0.0054s
== 20200710113437 AddContainerRegistryDeleteTagsServiceTimeoutToApplicationSettings: migrated (0.0054s)
Down
$ rails db:migrate:down VERSION=20200710113437
== 20200710113437 AddContainerRegistryDeleteTagsServiceTimeoutToApplicationSettings: reverting
-- remove_column(:application_settings, :container_registry_delete_tags_service_timeout)
-> 0.0165s
== 20200710113437 AddContainerRegistryDeleteTagsServiceTimeoutToApplicationSettings: reverted (0.0166s)

Screenshots

The admin UI:

Does this MR meet the acceptance criteria?

Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides - [-] Separation of EE specific content
Availability and Testing
- [-] Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
- [-] Tested in all supported browsers
- [-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
- [-] Label as security and @ mention
@gitlab-com/gl-security/appsec
- [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
- [-] Security reports checked/validated by a reviewer from the AppSec team
Merge request reports
Activity
changed milestone to %13.3
added 1 commit
- ca2deb94 - Use a feature flag for `container_registry_expiration_policies_throttling`
1 Warning The title of this merge request is longer than 72 characters and would violate our commit message rules when using the Squash on Merge feature. Please consider adjusting the title, or rebase the commits manually and don’t use Squash on Merge. 1 Message This merge request adds or changes files that require a review from the Technical Writing team. Documentation review
The following files require a review from a technical writer:
doc/user/packages/container_registry/index.md
The review does not need to block merging this merge request. See the:
- Technical Writers assignments for the appropriate technical writer for this review.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.
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 the chosen person is unavailable.
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, mention them as you normally would! Danger does not automatically notify them for you.
Category Reviewer Maintainer backend Kirstie Cook ( @k_cook
) (UTC-5, 7 hours behind@10io
)Vitali Tatarintev ( @ck3g
) (UTC+2, same timezone as@10io
)frontend Zack Cuddy ( @zcuddy
) (UTC-5, 7 hours behind@10io
)Kushal Pandya ( @kushalpandya
) (UTC+5.5, 3.5 hours ahead of@10io
)database Alexandru Croitor ( @acroitor
) (UTC+3, 1 hour ahead of@10io
)Adam Hegyi ( @ahegyi
) (UTC+2, same timezone as@10io
)test Quality for spec/features/*
Kirstie Cook ( @k_cook
) (UTC-5, 7 hours behind@10io
)Maintainer review is optional for test Quality for spec/features/*
If needed, you can retry the
danger-review
job that generated this comment.Generated by
DangerEdited by 🤖 GitLab Bot 🤖added 937 commits
-
ca2deb94...0f68786a - 935 commits from branch
master
- 31860314 - Add timeout support in DeleteTagsService#execute
- dd1049c5 - Use a feature flag for `container_registry_expiration_policies_throttling`
-
ca2deb94...0f68786a - 935 commits from branch
mentioned in issue #208193 (closed)
added database databasereview pending labels
Bundle size analysis [beta]
This compares changes in bundle size for entry points between the commits 7184f293 and f3ccd73c
Special assetsEntrypoint / Name Size before Size after Diff Diff in percent average 3.53 MB 3.53 MB - 0.0 % mainChunk 2.68 MB 2.68 MB - 0.0 %
Note: We do not have exact data for 7184f293. So we have used data from: 921a6ad5.
The target commit was too new, so we used the latest commit from master we have info on.
It might help to rerun thebundle-size-review
job
This might mean that you have a few false positives in this report. If something unrelated to your code changes is reported, you can check this comparison in order to see if they caused this change.Please look at the full report for more details
Read more about how this report works.
Generated by
DangerEdited by 🤖 GitLab Bot 🤖added 6719 commits
-
e37b5f24...12a33075 - 6717 commits from branch
master
- 6c14303b - Add timeout support in DeleteTagsService#execute
- d86118c0 - Add application setting and UI
-
e37b5f24...12a33075 - 6717 commits from branch
changed milestone to %13.4
added feature flag label
added 580 commits
-
99b7eedd...03b2a710 - 579 commits from branch
master
- 20a83ed5 - Add timeout support in Projects::ContainerRepository::Gitlab::DeleteTagsService#execute
-
99b7eedd...03b2a710 - 579 commits from branch
added documentation label
removed documentation label
added 1 commit
- 1e7a519c - Add timeout support in the delete tags service
added documentation label
marked the checklist item Database guides as completed
marked the checklist item Style guides as completed
marked the checklist item Merge request performance guidelines as completed
marked the checklist item Code review guidelines as completed
marked the checklist item Documentation (if required) as completed
marked the checklist item Changelog entry as completed
- Resolved by David Fernandez
added 1 commit
- 3eb048f6 - Provide a key to the container registry config
- Resolved by Jan Provaznik
- Resolved by David Fernandez
Hello!
- @sabrams Can you do a backend review?
- @a_akgun Can you do a frontend review?
- @acroitor Can you do a database review?
- @sselhorn Can you do a documentation review?
- @icamacho Can you review the admin UI changes?
Many thanks everyone!
Please note the following maintainers:
-
@nmezzopera
for the frontend maintainer review as this is his area of work. -
@reprazent
for the backend maintainer review as he helped us around the analysis to implement those limits (see gitlab-com/gl-infra/scalability#461 (comment 375560582))
Side note: The pipeline failure seems to be unrelated (https://gitlab.com/gitlab-org/gitlab/-/jobs/693851515). I will rebase off master in the coming days and this failure should go away.
added workflowin review label and removed workflowin dev label
unassigned @sselhorn
added Technical Writing docsfeature twdoing labels
added UI text label
- Resolved by David Fernandez
added twfinished label and removed twdoing label
- Resolved by David Fernandez
- Resolved by David Fernandez