Draft: Use a Pipeline sha attribute instead of repository commit sha for redundant cancellation
What does this MR do and why?
This MR updates the CancelRedundantPipelineService to use a new pipeline's sha attribute as the exclusion filter, instead of pulling the commit out of the repository. This should offer some amount of performance improvement by removing a Gitaly call and using an attribute of a record that we already have instantiated.
Why didn't it work this way before?
I honestly can't tell. I've gone back in time as far as c81ef304, which is when this feature was introduced. In that commit, we use a method Repository#sha_from_ref(ref)
, which does the same thing.
In 04b8e00f, we updated the implementation to use Project#commit(pipeline.ref).try(:id)
which is a different method that also references the actual underlying commit in the repository. I remain unclear on why. As far back as the original feature, c81ef304, sha
has existed as an attribute of Ci::Pipeline
. Maybe it wasn't reliable at the time?
How did you come across this?
I had a spec fail in my merge request !144273 (diffs) because I moved the sha exclusion out of the query and into ruby:
The test, until now, has been passing because the Pipeline factories don't fully set up all the sha
values in a coordinated way:
[45, 54] in /Users/drew/gitlab-development-kit/gitlab/spec/workers/ci/cancel_redundant_pipelines_worker_spec.rb
45:
46: it 'cancels the previous pending pipeline' do
47: perform
48:
49: byebug
=> 50: expect(prev_pipeline.builds.pluck(:status)).to contain_exactly('canceled')
51: end
52: end
53: end
54: end
(byebug) prev_pipeline.sha
"b83d6e391c22777fca1ed3012fce84f633d7fed0"
(byebug) pipeline.sha
"b83d6e391c22777fca1ed3012fce84f633d7fed0"
(byebug) project.commit(pipeline.ref).try(:id)
nil
(byebug) project.commit(prev_pipeline.ref).try(:id)
nil
So the service pulls nil
out of Project#commit
because we don't set up an underlying repository here, compares it to b83d6e391c22777fca1ed3012fce84f633d7fed0
on the old Pipeline, and decides to go ahead and cancel it. Which happens to be the behavior we want in this particular spec, but ultimately goes against the logic that we assert in the CancelRedundantPipelineService spec.
What is the risk in changing this?
There might be something that I don't know about how ci_pipelines.sha
gets populated, that this attribute is somehow less reliable than the sha
off the repository commit.
MR acceptance checklist
Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
Before | After |
---|---|
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.