Skip to content
Snippets Groups Projects

ci: Avoid blanket changes to avoid unexpected run

Merged Lin Jen-Shin requested to merge ci-avoid-blanket-changes into master
All threads resolved!

What does this MR do and why?

Avoid blanket changes to avoid unexpected run

Documentation to this caveat: https://docs.gitlab.com/ee/ci/yaml/#ruleschanges

You should use rules: changes only with branch pipelines or merge request pipelines. You can use rules: changes with other pipeline types, but rules: changes always evaluates to true when there is no Git push event. Tag pipelines, scheduled pipelines, manual pipelines, and so on do not have a Git push event associated with them. A rules: changes job is always added to those pipelines if there is no if that limits the job to branch or merge request pipelines.

If we trigger a pipeline from another pipeline, regardless whatever the actual changes were, it'll always evaluate to true which can be unexpected.

If we also apply if-default-refs it would at least limit the scope to those default scenarios we care most about.

That being said, adding if-default-refs to those conditions will not stop the incident from happening, because we can see that if $FORCE_GITLAB_CI is being set, it's also considered if-default-refs, and the triggered pipeline did also contain $FORCE_GITLAB_CI as we can see from gitaly!4986 (merged)

Therefore, I don't expect this to change the actual behaviours, nor does it prevent from cache poisoning.

However I think it's likely still beneficial to have this:

  • More clear what to expect
  • If we tweak if-default-refs we can tweak the behaviour when needed

Background

See gitlab-org/quality/engineering-productivity/master-broken-incidents#45 (comment 1159064802)

  • An unmerged Gitaly merge request gitaly!4986 (merged) triggered a cross-project pipeline on GitLab toon-gitaly-version branch which also has an unmerged merge request: !102281 (merged)
  • !102281 (merged) showed a mixed of pipelines with its owned merge requests pipelines and "branch pipelines" triggered from Gitaly.
  • Merge request pipelines did not run update-gitaly-binaries-cache nor update-tests-metadata because relevant files did not change. However, the "branch pipelines" did run those because changes are always true to those pipelines, because those were not pushed but triggered. This was discovered from !102727 (comment 1158917472)

Merge request pipeline without update-gitaly-binaries-cache: https://gitlab.com/gitlab-org/gitlab/-/pipelines/678677901

Screen_Shot_2022-11-03_at_22.02.36

Branch pipeline having update-gitaly-binaries-cache: https://gitlab.com/gitlab-org/gitlab/-/pipelines/683127875

Screen_Shot_2022-11-03_at_22.02.54

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

See gitlab-org/quality/engineering-productivity/master-broken-incidents#45 (comment 1159064802)

Edited by Lin Jen-Shin

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Lin Jen-Shin changed the description

    changed the description

  • mentioned in issue #379128 (closed)

  • Toon Claes mentioned in merge request gitaly!4986 (merged)

    mentioned in merge request gitaly!4986 (merged)

  • David Dieulivol approved this merge request

    approved this merge request

  • David Dieulivol requested review from @rymai and removed review request for @ddieulivol

    requested review from @rymai and removed review request for @ddieulivol

    • Resolved by Rémy Coutable

      question (non-blocking): From the MR description:

      we can see that if $FORCE_GITLAB_CI is being set, it's also considered if-default-refs

      I don't think I understand this statement.

      I see in the code that if FORCE_GITLAB_CI is set, we'll always create a pipeline (probably a branch pipeline, which are the problematic ones in this discussion), BUT thanks to this MR, if the branch pipeline isn't from a stable ref (which is quite likely), then *if-default-refs would be false and the job wouldn't be executed? Is this reasoning wrong?

      Also,

      I don't expect this to change the actual behaviours, nor does it prevent from cache poisoning.

      Probably related to what's above, but it should definitely help for cache poisoning, no?

      Without this MR, if anybody triggers a branch pipeline from their branch, it would for instance update the gitaly binary cache if the gitaly version were to be different in the branch...At least that's my understanding of it :laughing:

  • :wave: @ddieulivol, thanks for approving this merge request.

    This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.

    For more info, please refer to the following links:

  • 🤖 GitLab Bot 🤖 added 1 deleted label

    added 1 deleted label

  • Lin Jen-Shin changed the description

    changed the description

    • Resolved by Lin Jen-Shin

      That being said, adding if-default-refs to those conditions will not stop the incident from happening, because we can see that if $FORCE_GITLAB_CI is being set, it's also considered if-default-refs, and the triggered pipeline did also contain $FORCE_GITLAB_CI as we can see from gitaly!4986 (merged)

      Isn't this the actual problem here? Setting FORCE_GITLAB_CI allows anyone with permission to trigger an arbitrary pipeline with variables. I think we should consider disallow that.

      For instance, in this case, the testing done in gitaly!4986 (merged) could have been done by modifying the GITALY_SERVER_VERSION file directly in a gitlab-org/gitlab MR instead of !102281 (merged) + gitaly!4986 (merged).


      That being said, the change looks reasonable and can only improve things (and we can always remove $FORCE_GITLAB_CI from if-default-refs and/or workflow:rules later).

  • Rémy Coutable resolved all threads

    resolved all threads

  • Rémy Coutable approved this merge request

    approved this merge request

  • @godfat-gitlab Looks good to me, thanks! :heart: :yellow_heart: :green_heart: :purple_heart:

  • Rémy Coutable added 520 commits

    added 520 commits

    Compare with previous version

  • Lin Jen-Shin resolved all threads

    resolved all threads

  • Lin Jen-Shin resolved all threads

    resolved all threads

  • Thank you for resolving the conflicts for me! The failure shouldn't be relevant, but here's the failure log: https://gitlab.com/gitlab-org/gitlab/-/jobs/3284944401

    2022-11-07 14:58:24 +0000 Rack app ("POST /namespace27/project63/-/issues" - (127.0.0.1)):
    #<Gitlab::QueryLimiting::Transaction::ThresholdExceededError:
    Too many SQL queries were executed in Projects::IssuesController#create: a maximum of 100 is allowed but 104 SQL queries were executed

    Might be flaky from the default branch?

  • Rémy Coutable approved this merge request

    approved this merge request

  • Rémy Coutable mentioned in commit 60b63f1e

    mentioned in commit 60b63f1e

  • added workflowstaging label and removed workflowcanary label

  • mentioned in issue #378981 (closed)

  • Lin Jen-Shin mentioned in merge request !103484 (merged)

    mentioned in merge request !103484 (merged)

  • mentioned in issue gitaly#4615 (closed)

  • Lin Jen-Shin mentioned in merge request !107682 (merged)

    mentioned in merge request !107682 (merged)

  • Lin Jen-Shin mentioned in merge request gitaly!5937 (merged)

    mentioned in merge request gitaly!5937 (merged)

  • mentioned in issue #438553 (closed)

  • Please register or sign in to reply
    Loading