Bad interaction of needs and allow_failure keywords.

added ci-build devopsverify grouppipeline execution severity4 typebug labels

added pipeline label

removed ci-build label

marked this issue as related to #31673 (closed)

It seems one could argue that it's rather bad wording than bad interaction, i.e. that needs:, as interpreted by the runner, is actually a directly_after:.

I support your "expected correct behavior", but it seems there's interference with allow_failure implicitly being set to true when a job has the parameter when: manual set (see e.g. !25823 (closed) for apparently current expected behaviors on various combinations)

Maybe a solution would be adding a strict or, to keep it consistent, allow_failure keyword to needs:?

e.g.

stages:
  - test
  - build

testJob:
  script:
    - exit 1
  allow_failure: true

buildJob:
  script:
    - echo Building
  needs:
    - job: testJob
      allow_failure: false
      # (or)
      strict: true

changed milestone to %Backlog

added [deprecated] Accepting merge requests label

mentioned in merge request eyeo/adblockplus/adblockpluschrome!191 (closed)

added Category:Continuous Integration DEPRECATE_pipeline dag labels

added sectionops label

added grouppipeline authoring + 1 deleted label and removed grouppipeline execution label

added to epic &5206

mentioned in issue #25727

added customer label

Hello @dhershkovitch, a customer has opened a ticket (internal link) about this issue as well. Do you know if the milestone will be updated? Thank you!

We also would like to have this fixed. Use case: if e.g. a security scan job succeeds then another job with 'needs' annotated will update the binary attestation. But we do not want the whole pipeline to fail, so for the scan job we set 'allow_failures' to true. But now the binary attestation is done even if the scan fails.

@furkanayhan I dont think this issue is a bug but expected behavior, wdyt?

@dhershkovitch @marknuzzo This is indeed an expected behavior but I think there is a feature request here. Here's the thread: #32598 (comment 1323490522). The user explained their use-case with an example. I am not sure if we have similar issues like this because it seems that this can be a common use-case. I also wonder if our Engineering Productivity uses this kind of workflow for our pipelines. Maybe, there is an existing solution or we should come up with one.

I also wonder if our Engineering Productivity uses this kind of workflow for our pipelines. Maybe, there is an existing solution

Hi @jennli - would you be able to tell if Engineering Productivity uses this kind of workflow discussed here for our pipelines?

/cc @furkanayhan @dhershkovitch

When job B depends on job A with the needs keyword, and job A is marked as allow_failure, job B is run even if job A failed.

@marknuzzo I am not aware that we defined any CI config that matches this description. I did a quick search in the gitlab repo for all the jobs with allow_failure: true rule, did my best tracking down if any of them are needed by a different job, but the search came empty. However we have so many jobs that are allowed to fail so I may have missed some.

Opening this up to my team who may have more info: @gl-quality/eng-prod

Although, I wonder if you can try something like this:

Job A can upload an artifact with its exit status, or some indication of its success/fail status, have Job B pull this artifact (done automatically with the need keyword I believe), and read the value. Only proceed if the value is a passing status. Would that work?

Would that work?

@furkanayhan - WDYT?

If I may, this is what we tried down below at #32598 (comment 1330436265) (read from the part "> Have you considered using [...]").

In short: yes it works, but in term of usability it's not great, because if you « only proceed if the value is a passing status », in practice it means to exit 0 early, and so the job that you "skipped" is marked as successful. It's not equivalent to a proper "skip" that would appear in yellow in the pipeline.

@marknuzzo @jennli Thanks for following up on this issue. We decided to move this problem into #357819 because that's actually what the customer needed in the thread below.

Without trying to search around and see how we use allow_failure along with needs, I think I can still say that I hope we don't change the behaviour in any way, even though I do agree this might be a weird interaction, or at least ambiguous and unclear what should happen.

I hope we don't change the behaviour because we do use both extensively, along with when: on_failure and so on. A lot of things were done by trial and error. I would prefer we can add constructs that we can express the intention more clearly, rather than changing this. We can deprecate it, and we can follow up and update configurations where we're relying on this, with the new recommended constructs. Relying on exit code is a great idea.

Edited: And when: manual also plays a role here that allow_failure: true is added to maintain compatibility with the old manual jobs which are not blocking.

A happy medium would be to keep the same behaviour, however, add a new keyword that can skip the job B if job A fails.. So something like:

job_a:
  script:
    - exit 1
  allow_failure: true

job_b:
  script:
    - echo "Hello world!"
  needs:
    - job: "job_a"
      ignore_allow_failure: true

So something like how it was mentioned by @triluc 3 years ago...

If it is the expected behavior, then it would be helpful to provide an alternative way of achieving the behavior that many users were expecting instead. For instance, by following the suggestion at #32598 (comment 296101771).

mentioned in issue #389276 (closed)

marked this issue as related to #389276 (closed)

Adding strict: true/false would be nice. There needs to be a way to skip a job if the previous job fails (with allow_failure). @bgadberry

I am not sure if I understand the issue correctly, so let me provide an example and discuss with it;

build1:
  stage: build
  script: exit 1
  allow_failure: true

test1:
  stage: test
  script: echo

test2:
  stage: test
  script: echo
  needs: [build1]

with this config, when build1 fails, what would you expect?

test1 is skipped? run?
test2 is skipped? run?

Setting label(s) Category:Pipeline Composition based on grouppipeline authoring.

I'm not the OP but I will answer anyway.

test1 is skipped? run?

test1 must run IMO. build1 has allow_failure set, so when it fails, the pipeline keeps going. So test1 must run, it seems obvious, unless I'm missing something?

test2 is skipped? run?

test2 should NOT run. But right now it does, and now that it shipped this way for a while (3 years?), it can't be changed. But it should be possible to make it more flexible.

Let me try to elaborate. allow_failure is useful because we don't want build1 to fail the pipeline. But that doesn't mean that we don't care about the outcome of build1. We care, and we want to be able to make decisions based on this outcome. So there should be a way that jobX runs regardless of the outcome of build1 (and that's the current behavior with needs), and then jobY should run only if build1 succeeded, and why not have a jobZ that runs only if build1 failed.

Maybe all of this is already possible, and if so, I'd be happy to know how. Thanks!

@arnaudr Thank you for sharing your ideas.

Can you provide a simple YAML with your example of jobX, Y, Z?

@furkanayhan Sure! There it is (I used real names, instead of X, Y, Z for clarity):

image: debian:sid

stages:
  - check
  - build
  - test

# pre-build checks

check-i386:
  stage: check
  script: exit 0  # yes, we can build for i386 architecture
  allow_failure:
    exit_codes: 77

check-arm64:
  stage: check
  script: exit 77  # no, arm64 builds are not supported
  allow_failure:
    exit_codes: 77

# builds

build:
  stage: build
  script: echo main build

build-i386:
  stage: build
  script: echo build i386
  needs: [ check-i386 ]

build-arm64:
  stage: build
  script: echo build arm64
  needs: [ check-arm64 ]

# tests

test:
  stage: test
  script: echo test

Now let me explain that with plain words:

I want to build and test my program for a "main architecture", and those are the jobs build and test. If any of those fail, the pipeline fails.
Then I have extra architectures i386 and arm64. For any of those, I want to:
1. check if my program can build for this architecture (I don't have this information when I write the YAML, so I need to run a script to know that). These are the jobs check-*.
2. if it can build for this architecture, then I want to build it. Those are the jobs build-*.
Finally, I'm not going to run tests for extra archs. Running tests for the main architecture only is enough.

Picture:

And looking at the picture above: I wish that build-arm64 was skipped, based on the fact that check-arm64 previously failed.

You can find this pipeline at:

Note that there is a similar discussion at #357819 (comment 902816616). Similar goal, but different implementation.

@arnaudr Thanks for the example, now I see your use-case. However, I am not sure if using the failed status is a good idea for this because the check-* jobs are not failed actually, just finished with a result, right? Have you considered using artifacts for this? In the check-* jobs, you can create artifacts based on the result. Then in the build-* jobs, you can read the result and exit the build immediately.

Maybe, we should have a feature of allow_failure like (allow_failure:only_for_pipeline:true). Or maybe we should think about this kind of use case and come up with another better solution. We'll discuss this.

However, I am not sure if using the failed status is a good idea for this because the check-* jobs are not failed actually, just finished with a result, right?

Correct, the check-arm64 jobs finish with 77 in order to say "please skip the build for the arm64 architecture". To be exhaustive, the idea is that the various check-* scripts return the following:

0: indicate that I want to build the program
77: indicate that I want to skip the build
anything else: an error happened

But GitLab CI doesn't really allow me to take decisions based on the job's exit code. So the idea with allow_failure: 77 was to workaround that, but it doesn't really fly. Because, as is discussed here, build-arm64 will run anyway, even if check-arm64 failed before (due to allow_failure). And, from build-arm64, I don't have access to the exit code of check-arm64 anyway...

So, it's a bit clunky indeed.

Have you considered using artifacts for this? In the check-* jobs, you can create artifacts based on the result. Then in the build-* jobs, you can read the result and exit the build immediately.

I just tried, it is indeed a better approach. Here's the YAML I used:

image: debian:sid

stages:
  - check
  - build
  - test

# pre-build checks

check-i386:
  stage: check
  script: touch .please-build
  artifacts:
    paths: [ .please-* ]

check-arm64:
  stage: check
  script: touch .please-skip
  artifacts:
    paths: [ .please-* ]

# builds

build:
  stage: build
  script: echo main build

build-i386:
  stage: build
  dependencies: [ check-i386 ]
  script:
    - if [ -e .please-skip  ]; then echo skipping; exit 0; fi
    - if [ -e .please-build ]; then echo building; fi

build-arm64:
  stage: build
  dependencies: [ check-arm64 ]
  script:
    - if [ -e .please-skip  ]; then echo skipping; exit 0; fi
    - if [ -e .please-build ]; then echo building; fi

# tests

test:
  stage: test
  script: echo test

And the resulting pipeline (available at https://gitlab.com/arnaudr/gitlab-ci-allow-failure/-/pipelines/819739424/):

However, and as you can see above, there's one major downside: there is absolutely no indication of whether a job was actually skipped or not. I have to check the logs to see that for the green build-i386, there was a successful build, while for the green build-arm64, in fact nothing was built.

So, using artifacts this way is still, IMHO, a workaround.

I have two suggestions for improving that:

1. rules + artifacts

The rules keyword could check if an artifact exists (or doesn't exist) in order to skip a job. At the moment, there is a rules:exists but it's only for files in the Git repo. So maybe extending rules:exists for artifacts, or having a new rules:artifacts keyword. That would be useful for this situation.

This kind of construct would only check if files exist (not read files and check their content). But that can already be quite powerful.

Discussed here it seems: #215100

2. rules + exit code

Alternatively, there could be a rules:job keyword, to decide what to do based on the exit code of other jobs. This way, there would be no need to create artifacts.

Although, with this kind of feature, I would still need allow_failure, because AFAIK a job can only return 0, everything else is considered a failure by GitLab. Does it feel like abusing the allow_failure keyword? If so, maybe introduce a new keyword consider_success to list non-zero exit codes that should be considered as success. But maybe that's going a bit too far.

Thank you for your comment. We are currently discussing it internally. And as you said, there is no other workaround other than using artifacts. (or dotenv variables).

Discussed here it seems: #215100

Actually, #235812 would be a lot better.

I looked at #235812 and I have the impression that it would work indeed. It looks better than checking the existence of an artifact (more powerful).

With that, my YAML would become something like that:

image: debian:sid

stages:
  - check
  - build
  - test

# pre-build checks

check-i386:
  stage: check
  script: echo SKIP_BUILD_I386=0 >> build.env
  artifacts:
    reports:
      dotenv: build.env

check-arm64:
  stage: check
  script: echo SKIP_BUILD_ARM64=1 >> build.env
  artifacts:
    reports:
      dotenv: build.env

# builds

build:
  stage: build
  script: echo main build

build-i386:
  stage: build
  rules:
    - if: '$SKIP_BUILD_I386 == 1'
        when: never
  script:
    - echo build i386
  dependencies:
    - check-i386

build-arm64:
  stage: build
  rules:
    - if: '$SKIP_BUILD_ARM64 == 1'
        when: never
  script:
    - echo build arm64
  dependencies:
    - check-arm64

# tests

test:
  stage: test
  script: echo test

I think one important detail with this proposal is to decide what happens if there's already a variable SKIP_BUILD_ARM64 defined in the YAML. Does it take precedence over what might have been set in the dotenv file? I'd say yes, IMO anything user-defined must take precedence over the rest.

"Dotenv variables/dependency variables" overrides all YAML variables, so we'd be good to go if we had this feature.

@arnaudr I think we can move this discussion to its dedicated issue: #357819

"Dotenv variables/dependency variables" overrides all YAML variables, so we'd be good to go if we had this feature.

I just wanted to clarify this point.

I think I was thinking exactly the opposite: variables defined in the YAML, or defined in the pipeline settings, or defined by user for manual run, should always take precedence over what's in the dotenv. Because if user says FOO=bar, and then down the line some job write FOO=xyz in a file, who's right? IMO the user is right, it shouldn't be allowed that code overwrites a variable that was already clearly set by user.

But that's just my perspective, what seems logical to me, from where I come from. In any case, if the behavior "Dotenv variables/dependency variables overrides all YAML variables" is already what's out there, and clearly documented, then be it, there's nothing to discuss then.

@arnaudr I think we can move this discussion to its dedicated issue: #357819

Sure, thanks for following up so far with me on this discussion @furkanayhan! Much appreciated!

I think I was thinking exactly the opposite: variables defined in the YAML, or defined in the pipeline settings, or defined by user for manual run, should always take precedence over what's in the dotenv.

@arnaudr Since you asked about the YAML variables, I responded with the YAML variables. You can see on this page https://docs.gitlab.com/ee/ci/variables/#cicd-variable-precedence that user-defined variables have the highest priority.

removed DEPRECATE_pipeline dag label

removed [deprecated] Accepting merge requests label

changed the description

mentioned in issue #357819

mentioned in issue gitlab-org/quality/quality-engineering/team-tasks#1802 (closed)

I wanted to add an example for which a new job:needs:allow_failure keyword is needed:

job1:
  script: exit 1
  allow_failure: true

job2:
  needs:
    - job: job1
      allow_failure: false
  trigger:
    project: other-group/other-project

Not only are "workarounds" using artifacts:reports:dotenv inappropriate for most use cases (as the dependant job still runs, and needs to be modified to know when to exit early), but for uses like this example where another project is triggered, it would be entirely wrong to require a change to the pipeline definition of other-group/other-project to support this.

This issue was originally raised as a bug. Should a Feature Proposal be created for the new functionality?

added sectionci label and removed sectionops label

added SLOMissed label

Bad interaction of needs and allow_failure keywords.

Summary

Steps to reproduce

Example Project

What is the current bug behavior?

What is the expected correct behavior?

Output of checks

Designs

Child items ...

Activity

Bad interaction of needs and allow_failure keywords.

Summary

Steps to reproduce

Example Project

What is the current bug behavior?

What is the expected correct behavior?

Output of checks

Relates to

Activity