set task as failed

[pritidesai]

Changes

Set task as failed when the pod encounters CreateContainerConfigError. TaskRun status is now set to false instead of unknown with the same reason CreateContainerConfigError.

Before this change, taskRun status had the same reason CreateContainerConfigError but status was set to unknown which means task is still running in turn pipeline is still running until it hits the timeout.

PR #1907 tried to fix this issue which was also reported in issue #1902.

Fixes #3412

/cc @vdemeester @vincent-pli @afrittoli @chmouel

/kind bug

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes tests (if functionality changed/added)
• Includes docs (if user facing)
• Commit messages follow commit message best practices
• Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

• If you are adding a new binary/image to the cmd dir, please update
  the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Declare task failure when it hits `CreateContainerConfigError` instead of setting it to unknown i.e. running.

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	92.4%	92.2%	-0.2

[ghost]

When a pod exceeds node resources I think Kubernetes will try to re-schedule it again when resources may have freed up? So the TaskRun might eventually succeed. If I'm understanding right, I don't know if we want to mark that as an immediate failure?

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/status.go	92.4%	92.2%	-0.2

[pritidesai]

When a pod exceeds node resources I think Kubernetes will try to re-schedule it again when resources may have freed up? So the TaskRun might eventually succeed. If I'm understanding right, I don't know if we want to mark that as an immediate failure?

I am not sure either, was going by the comment here. I have reverted the changes for ExceededNodeResources and it continue being marked as unknown.

pipeline/pkg/pod/status.go

Lines 64 to 66 in ba2d2e7

	// ReasonPending indicates that the pod is in corev1.Pending, and the reason is not
	// ReasonExceededNodeResources or IsPodHitConfigError
	ReasonPending = "Pending"

[vincent-pli]

@pritidesai Pod does not fit any host will be preempt or re-queue after a short time sleep. so @sbwsg is correct.

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg, vincent-pli

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [sbwsg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[skaegi]

@pritidesai would this handle the case when someone has a bad imagePullSecret -- we get that all the time and just see the Task Cancelled with no explanation. If so this PR would be a huge help for users trying to determine what went wrong.

[skaegi]

ah... ok according to our team this is needed because currently if we hit a config error that results in an ImagePullErr Tekton will just hang and cycle to ImagePullBackOff before ultimately waiting for the Pipeline to timeout. We're managing this currently by detecting and cancelling the TaskRun but this unfortunately hides the ConfigError from the user.

[pritidesai]

thanks @sbwsg and @vincent-pli for the review.
looking for one lgtm 🙏
Adding it to next milestone 0.21

[chmouel]

awesome, thanks you so much for fixing this @pritidesai 🙇🏽

/lgtm

[ghost]

/test check-pr-has-kind-label