Create deployments for ready-to-run jobs only
What does this MR do and why?
This change is part of the epic for Reduce main database load by optimizing `deploy... (&10169)
This ensures that deployment records are only created for deploy jobs that are ready to run. This means that deploy jobs that are still at the created
state will not have deployment records upfront, helping to reduce the load on the main database.
For further details, please check the Expected behavior section related issue, and the illustration of the expected behavior in the Screenshots below.
Screenshots or screen recordings
GitLab CI configuration example
Given the following GitLab CI configuration with jobs in different stages:
- [stage-0]
test
job - 4 deploy jobs
- [stage-1]
review
runs automatically aftertest
- [stage-2]
staging
is set aswhen: manual
andallow_failure: false
, meaning it must succeed before jobs in the next stages can run - [stage-3]
canary
is set aswhen: manual
and by defaultallow_failure: true
- [stage-4]
production-a
is set aswhen: manual
(and by defaultallow_failure: true
) - [stage-4]
production-b
is set aswhen: manual
andneeds: [canary]
(and by defaultallow_failure: true
)
- [stage-1]
=== EXPAND THIS SECTION FOR THE GITLAB CI CONFIG ===
stages: [stage-0, stage-1, stage-2, stage-3, stage-4]
test:
stage: stage-0
script: sleep 120
review:
stage: stage-1
environment:
name: review
action: start
script: sleep 60
staging:
stage: stage-2
environment:
name: staging
action: start
script: sleep 60
when: manual
allow_failure: false
canary:
stage: stage-3
environment:
name: canary
action: start
script: exit 0
when: manual
production-a:
stage: stage-4
environment:
name: production-a
action: start
script: exit 0
when: manual
production-b:
stage: stage-4
environment:
name: production-b
action: start
script: exit 0
when: manual
needs: [canary]
Current behavior
This is the behavior before the change in this MR is applied / the Feature Flag is disabled
=== EXPAND THIS SECTION FOR THE EXAMPLE TABLE ===
Expected behavior
This is the expected behavior after the change / the Feature Flag is enabled
=== EXPAND THIS SECTION FOR THE EXAMPLE TABLE ===
Note: since production-b
is set as needs: [canary]
, its corresponding deployment should ideally not be created until canary
succeeds. This will be addressed in a follow-up issue.
How to set up and validate locally
-
Create a project or use an existing project for testing
-
Enable the
create_deployment_only_for_processable_jobs
Feature Flag in the rails consoleFeature.enable(:create_deployment_only_for_processable_jobs)
-
Use the example above for the configuration in your
.gitlab-ci.yml
-
Run the pipeline / observe the running pipeline and verify that it follows the Expected behavior in the example above
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 #391291 (closed)
Merge request reports
Activity
changed milestone to %16.5
assigned to @partiaga
- A deleted user
added backend feature flag labels
- Resolved by Pam Artiaga
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 ahuntsman
(UTC-5, 16 hours behind
@partiaga
)sashi_kumar
(UTC+0, 11 hours behind
@partiaga
)~"Verify" Reviewer review is optional for ~"Verify" stanhu
(UTC-7, 18 hours behind
@partiaga
)Please check reviewer's status!
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
Dangermentioned in commit gitlab-org-sandbox/gitlab-jh-validation@10f48b97
- Resolved by Pam Artiaga
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@368ecc72
- Resolved by Pam Artiaga
Making the proposed change in the
AtomicProcessingService
does not result in the expected behavior detailed in the issue.In the
AtomicProcessingService
spec, we have this example CI config:CI config
stages: [stage-0, stage-1, stage-2, stage-3] test: stage: stage-0 script: exit 0 review: stage: stage-1 environment: name: review action: start script: exit 0 staging: stage: stage-2 environment: name: staging action: start script: exit 0 when: manual canary: stage: stage-3 environment: name: canary action: start script: exit 0 when: manual production: stage: stage-3 environment: name: production action: start script: exit 0 when: manual needs: [canary]
The expectation detailed in the issue seem to suggest that the manual deploy jobs (in this case,
staging
,canary
, andproduction
) change status fromcreated
tomanual
one after the other.But what actually happens is that all manual jobs seem to become
created
tomanual
at the same time, after the non-manual jobs in the previous stages are done. The only situation where a manual job does not becomecreated
tomanual
is if it needs another job, in which case it becomesskipped
.So, basing on what's described in the issue, the expectation for the pipeline above would be:
expectations based on issue description
This expectation does not match the actual state transitions happening.
# 1 - process test job, which becomes 'pending' process_pipeline succeed_pending # 2 - process review job, which becomes 'pending' # the review deployment record is created here process_pipeline succeed_pending # 3 - process staging job, which becomes 'manual' # the staging deployment record is created here process_pipeline play_manual_job('staging') # staging job becomes 'pending' succeed_pending # 4 - process canary and production jobs # with canary becoming 'manual' and production becoming 'skipped' # the canary deployment record is created here process_pipeline play_manual_job('canary') succeed_pending # 5 - process production job, which now becomes 'manual' with canary succeeding # the production deployment is only created here process_pipeline
But, basing on the actual state transitions of the jobs, the expectation should be as follows:
expectations based on actual state transition
This is also the logic in the specs for this MR
# 1 - process test job, which becomes 'pending' process_pipeline succeed_pending # 2 - process review job, which becomes 'pending' # the review deployment record is created here process_pipeline succeed_pending # 3 - process staging, canary, and production jobs # staging becomes 'manual', so we expect the staging deployment record to be created # - both canary and production are also processed even if they are at a later stage # canary becomes 'manual', so we expect the canary deployment record to be created # production becomes 'skipped' because it *needs* canary # we expect the production deployment record to NOT be created, but it is still created process_pipeline play_manual_job('staging') # staging job becomes 'pending' play_manual_job('canary') # canary job becomes 'pending' succeed_pending # staging and canary job becomes 'successful' # 4 - process production job, which now becomes 'manual' with canary being successful # we expect the production deploy to only be created here when it becomes 'manual' # but it was already created in step 3 even when it was 'skipped' process_pipeline
Now, the specs still fail because we are expecting the production job to only have a deployment when it becomes 'manual', but a deployment was actually already created when it become 'skipped'. (See details in !132835 (comment 1581552248))
In order for the expectations to succeed, we would need to call the
Deployments::CreateForJobService
in theCi::ProcessBuildService
, but we can't do this becauseCi::ProcessBuildService
is called withinGitlab::OptimisticLocking
. (See this commit for illustration)Edited by Pam Artiaga
Allure report
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for 51f9eeecexpand test summary
+-----------------------------------------------------------------------+ | suites summary | +------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------+--------+--------+---------+-------+-------+--------+ | Manage | 13 | 0 | 1 | 0 | 14 | ✅ | | Govern | 35 | 0 | 0 | 0 | 35 | ✅ | | Verify | 8 | 0 | 0 | 0 | 8 | ✅ | | Create | 45 | 0 | 1 | 0 | 46 | ✅ | | Data Stores | 18 | 0 | 4 | 0 | 22 | ✅ | | Plan | 55 | 0 | 0 | 0 | 55 | ✅ | | Monitor | 4 | 0 | 0 | 0 | 4 | ✅ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | | Framework sanity | 0 | 0 | 1 | 0 | 1 | ➖ | +------------------+--------+--------+---------+-------+-------+--------+ | Total | 178 | 0 | 8 | 0 | 186 | ✅ | +------------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
test report for 51f9eeecexpand test summary
+------------------------------------------------------------+ | suites summary | +-------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------+--------+--------+---------+-------+-------+--------+ +-------+--------+--------+---------+-------+-------+--------+ | Total | 0 | 0 | 0 | 0 | 0 | ➖ | +-------+--------+--------+---------+-------+-------+--------+
added 1234 commits
-
5c9bfd68...dfc3ddbf - 1229 commits from branch
master
- bfad9a1e - Create deployments for ready-to-run jobs only
- e9472f50 - Add tests for AtomicProcessingService
- 3bc1e064 - Update puts statements
- 727e0e26 - Update specs to consider allow_failure setting
- 8764dc81 - Remove puts, add back tests for disable FF
Toggle commit list-
5c9bfd68...dfc3ddbf - 1229 commits from branch
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@1ce028b7
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@6b645149
- Resolved by Shinya Maeda
- Resolved by Mehmet Emin INAC
Hi @shinya.maeda, would you mind giving this the initial backend review when you get the chance?
requested review from @shinya.maeda
- Resolved by Shinya Maeda
- Resolved by Shinya Maeda
- Resolved by Shinya Maeda
- Resolved by Shinya Maeda
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@56e536bd
@shinya.maeda
, thanks for approving this merge request.This is the first time the merge request has been approved. To ensure full test coverage, a new pipeline will be started shortly.
For more info, please refer to the following links:
added pipeline:mr-approved label
requested review from @minac and removed review request for @shinya.maeda
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@f71c69a5
- Resolved by Mehmet Emin INAC
removed review request for @minac
added 1 commit
- 65113f35 - Account for FF being switched midway through a process
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@dd419da4