Throw an error from jobReady() if the job exceeds its BackoffLimit #9950

jeffrosenberg · 2021-07-16T21:28:08Z

Signed-off-by: Rosenberg, Jeff jeff.rosenberg@icfnext.com

What this PR does / why we need it:

As noted in #9285, if --wait-for-jobs is specified, and a job exceeds its backoff limit, Helm will always wait for the full timeout before failing, even though there is no chance of its succeeding. This PR modifies the jobReady() function to return an error. Based on the comments on k8s.io/apimachinery/pkg/util/wait#PollImmediateUntil, it appears that returning an error should stop that function from polling.

Special notes for your reviewer:

I've updated the tests in ready_test.go, but there is no test coverage in client_test.go to validate that this works as expected. Ideally this would have a test in client_test.go, but these comments indicate that there haven't been tests for waits since the feature was introduced.

If applicable:

this PR contains documentation
this PR contains unit tests
this PR has been tested for backwards compatibility

Closes helm#9285 Signed-off-by: Rosenberg, Jeff <jeff.rosenberg@icfnext.com>

jeffrosenberg · 2021-07-19T16:40:48Z

In local testing, this speeds up how quickly we fail, but not yet to the extent I'd like. This still waits for all deployments to be ready, and then once the failed job is the last thing we're waiting for, it fails immediately rather than spinning on the failed job until reaching the timeout. That's still a significant improvement, but I'd love to figure out if there's a way to fail the install immediately on job failure.

pkg/kube/ready.go

zonggen · 2021-09-10T16:21:19Z

This still waits for all deployments to be ready, and then once the failed job is the last thing we're waiting for

Yes because the ResourceList ordering: https://github.com/helm/helm/blob/main/pkg/kube/wait.go#L54

Test cases passed locally as expected. I think this PR is sane and able to fix the original issue.

jeffrosenberg · 2021-09-11T16:15:07Z

Thanks for the review @zonggen; FYI, I've resolved the merge conflict.

joejulian

lgtm, but I'm not an approver.

stuartdrennan · 2022-10-20T13:24:41Z

End user test

chart.zip

Job:

apiVersion: batch/v1
kind: Job
metadata:
  name: pi
spec:
  template:
    spec:
      containers:
      - name: pi
        image: perl:5.34.0
        command: ["perl",  "-Mbignum=bpi", "-wle", "sleep(30) && print bpi(2000)"]
      restartPolicy: Never
  backoffLimit: 0

Baseline Test

Released Helm version 3.9.1

helm install d . --wait-for-jobs --wait --debug --timeout 60s

client.go:128: [debug] creating 1 resource(s)
wait.go:48: [debug] beginning wait for 1 resources with timeout of 1m0s
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:231: [debug] Job is not completed: default/pi
ready.go:227: [debug] Job is failed: default/pi
ready.go:227: [debug] Job is failed: default/pi
ready.go:227: [debug] Job is failed: default/pi
ready.go:227: [debug] Job is failed: default/pi
ready.go:227: [debug] Job is failed: default/pi
ready.go:227: [debug] Job is failed: default/pi
ready.go:227: [debug] Job is failed: default/pi
ready.go:227: [debug] Job is failed: default/pi
ready.go:227: [debug] Job is failed: default/pi
ready.go:227: [debug] Job is failed: default/pi
ready.go:227: [debug] Job is failed: default/pi
Error: INSTALLATION FAILED: timed out waiting for the condition
helm.go:84: [debug] timed out waiting for the condition
INSTALLATION FAILED
main.newInstallCmd.func2
        helm.sh/helm/v3/cmd/helm/install.go:127
github.com/spf13/cobra.(*Command).execute
        github.com/spf13/cobra@v1.4.0/command.go:856
github.com/spf13/cobra.(*Command).ExecuteC
        github.com/spf13/cobra@v1.4.0/command.go:974
github.com/spf13/cobra.(*Command).Execute
        github.com/spf13/cobra@v1.4.0/command.go:902
main.main
        helm.sh/helm/v3/cmd/helm/helm.go:83
runtime.main
        runtime/proc.go:255
runtime.goexit
        runtime/asm_arm64.s:1133

Summary

Job failed but Helm waited until timeout before exiting. Not the desired response but expected.
Same result with backoffLimit: 0 and backoffLimit: 1

PR Test

install.go:192: [debug] Original chart version: ""
install.go:209: [debug] CHART PATH: /Users/sdrennan/code/tmp/gg/chart

client.go:128: [debug] creating 1 resource(s)
wait.go:66: [debug] beginning wait for 1 resources with timeout of 1m0s
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:235: [debug] Job is not completed: default/pi
ready.go:230: [debug] Job is failed: default/pi
Error: INSTALLATION FAILED: job is failed: default/pi
helm.go:84: [debug] job is failed: default/pi
INSTALLATION FAILED
main.newInstallCmd.func2
        helm.sh/helm/v3/cmd/helm/install.go:141
github.com/spf13/cobra.(*Command).execute
        github.com/spf13/cobra@v1.5.0/command.go:872
github.com/spf13/cobra.(*Command).ExecuteC
        github.com/spf13/cobra@v1.5.0/command.go:990
github.com/spf13/cobra.(*Command).Execute
        github.com/spf13/cobra@v1.5.0/command.go:918
main.main
        helm.sh/helm/v3/cmd/helm/helm.go:83
runtime.main
        runtime/proc.go:250
runtime.goexit
        runtime/asm_arm64.s:1165

Summary

Helm exited correctly within timeout period
Same result with backoffLimit: 0 and backoffLimit: 1

jouve · 2022-10-31T13:51:49Z

another blocker for this PR ?

I could help if needed :)

armstrongli · 2023-05-18T09:39:41Z

this PR works for me. 👍

armstrongli · 2023-05-24T10:12:35Z

@mattfarina , need your help on this PR?

armstrongli · 2023-06-06T06:44:13Z

@jouve , need your help on this PR

armstrongli · 2023-06-16T09:22:17Z

/assign mattfarina

Throw an error from jobReady() if the job exceeds its BackoffLimit

cd04e3f

Closes helm#9285 Signed-off-by: Rosenberg, Jeff <jeff.rosenberg@icfnext.com>

helm-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 16, 2021

jeffrosenberg mentioned this pull request Jul 16, 2021

--wait-for-jobs does not fails if job failed #9285

Closed

zonggen reviewed Sep 10, 2021

View reviewed changes

pkg/kube/ready.go Show resolved Hide resolved

Merge branch 'main' into error-on-failed-jobs-9285

b13f3a1

egorchepiga mentioned this pull request Dec 23, 2021

--wait-for-jobs does not fails if job failed #10500

Closed

joejulian approved these changes Mar 2, 2022

View reviewed changes

bclewis1 mentioned this pull request Sep 1, 2022

Use conditions rather than pod counters to wait for jobs #11289

Open

3 tasks

joejulian added this to the 3.10.2 milestone Oct 31, 2022

mattfarina modified the milestones: 3.10.2, 3.10.3 Nov 10, 2022

hickeyma modified the milestones: 3.10.3, 3.11.0 Dec 14, 2022

mattfarina modified the milestones: 3.11.0, 3.12.0 Jan 18, 2023

joejulian modified the milestones: 3.12.0, 3.13.0 May 5, 2023

joejulian merged commit 99e1dce into helm:main Jul 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw an error from jobReady() if the job exceeds its BackoffLimit #9950

Throw an error from jobReady() if the job exceeds its BackoffLimit #9950

jeffrosenberg commented Jul 16, 2021

jeffrosenberg commented Jul 19, 2021

zonggen commented Sep 10, 2021 •

edited

Loading

jeffrosenberg commented Sep 11, 2021

joejulian left a comment

stuartdrennan commented Oct 20, 2022

jouve commented Oct 31, 2022

armstrongli commented May 18, 2023

armstrongli commented May 24, 2023

armstrongli commented Jun 6, 2023

armstrongli commented Jun 16, 2023

Throw an error from jobReady() if the job exceeds its BackoffLimit #9950

Throw an error from jobReady() if the job exceeds its BackoffLimit #9950

Conversation

jeffrosenberg commented Jul 16, 2021

jeffrosenberg commented Jul 19, 2021

zonggen commented Sep 10, 2021 • edited Loading

jeffrosenberg commented Sep 11, 2021

joejulian left a comment

Choose a reason for hiding this comment

stuartdrennan commented Oct 20, 2022

Baseline Test

PR Test

jouve commented Oct 31, 2022

armstrongli commented May 18, 2023

armstrongli commented May 24, 2023

armstrongli commented Jun 6, 2023

armstrongli commented Jun 16, 2023

zonggen commented Sep 10, 2021 •

edited

Loading