Skip to content

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:

image

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.

Edited by drew stachon

Merge request reports