From 251052853da114ade05306a2d3f0b40c068001b6 Mon Sep 17 00:00:00 2001 From: JeromeJu Date: Thu, 23 Nov 2023 03:13:39 +0000 Subject: [PATCH] Cleanup: Move TaskRun Reasons in pkg/pod to pkg/apis Prior to this commit, strings that were set as Reasons for the TaskRun status were split between pkg/apis and pkg/reconciler/taskrun. This commit moves all TaskRun related reasons to pkg/apis and adds aliases for backwards compatibility. This adds consistency, correctly signals to clients that all Reasons are part of the API, and helps avoid circular imports. It also renames ReasonPending to ReasonPodPending. /kind cleanup fixes: tektoncd#7397 --- docs/pipeline-api.md | 22 ++++++++++++++++++++++ pkg/apis/pipeline/v1/taskrun_types.go | 16 ++++++++++++++-- pkg/pod/status.go | 24 ++++++++++++------------ pkg/reconciler/taskrun/taskrun.go | 15 +++++++++------ pkg/reconciler/taskrun/taskrun_test.go | 20 ++++++++++---------- 5 files changed, 67 insertions(+), 30 deletions(-) diff --git a/docs/pipeline-api.md b/docs/pipeline-api.md index 5705f9ced47..e0414398cce 100644 --- a/docs/pipeline-api.md +++ b/docs/pipeline-api.md @@ -5026,9 +5026,24 @@ reasons that emerge from underlying resources are not included here

"Failed"

TaskRunReasonFailed is the reason set when the TaskRun completed with a failure

+

"TaskRunResolutionFailed"

+

TaskRunReasonFailedResolution indicated that the reason for failure status is +that references within the TaskRun could not be resolved

+ +

"TaskRunValidationFailed"

+

TaskRunReasonFailedValidation indicated that the reason for failure status is +that taskrun failed runtime validation

+

"TaskRunImagePullFailed"

TaskRunReasonImagePullFailed is the reason set when the step of a task fails due to image not being pulled

+

"InvalidParamValue"

+

TaskRunReasonInvalidParamValue indicates that the TaskRun Param input value is not allowed.

+ +

"ResourceVerificationFailed"

+

TaskRunReasonResourceVerificationFailed indicates that the task fails the trusted resource verification, +it could be the content has changed, signature is invalid or public key is invalid

+

"TaskRunResultLargerThanAllowedLimit"

TaskRunReasonResultLargerThanAllowedLimit is the reason set when one of the results exceeds its maximum allowed limit of 1 KB

@@ -5038,9 +5053,16 @@ reasons that emerge from underlying resources are not included here

"Started"

TaskRunReasonStarted is the reason set when the TaskRun has just started

+

"TaskRunStopSidecarFailed"

+

TaskRunReasonStopSidecarFailed indicates that the sidecar is not properly stopped.

+

"Succeeded"

TaskRunReasonSuccessful is the reason set when the TaskRun completed successfully

+

"TaskValidationFailed"

+

TaskRunReasonTaskFailedValidation indicated that the reason for failure status is +that task failed runtime validation

+

"TaskRunTimeout"

TaskRunReasonTimedOut is the reason set when one TaskRun execution has timed out

diff --git a/pkg/apis/pipeline/v1/taskrun_types.go b/pkg/apis/pipeline/v1/taskrun_types.go index 0b8e9451975..d3f2a263cad 100644 --- a/pkg/apis/pipeline/v1/taskrun_types.go +++ b/pkg/apis/pipeline/v1/taskrun_types.go @@ -187,9 +187,21 @@ const ( // TaskRunReasonResultLargerThanAllowedLimit is the reason set when one of the results exceeds its maximum allowed limit of 1 KB TaskRunReasonResultLargerThanAllowedLimit TaskRunReason = "TaskRunResultLargerThanAllowedLimit" // TaskRunReasonStopSidecarFailed indicates that the sidecar is not properly stopped. - TaskRunReasonStopSidecarFailed = "TaskRunStopSidecarFailed" + TaskRunReasonStopSidecarFailed TaskRunReason = "TaskRunStopSidecarFailed" // TaskRunReasonInvalidParamValue indicates that the TaskRun Param input value is not allowed. - TaskRunReasonInvalidParamValue = "InvalidParamValue" + TaskRunReasonInvalidParamValue TaskRunReason = "InvalidParamValue" + // TaskRunReasonFailedResolution indicated that the reason for failure status is + // that references within the TaskRun could not be resolved + TaskRunReasonFailedResolution TaskRunReason = "TaskRunResolutionFailed" + // TaskRunReasonFailedValidation indicated that the reason for failure status is + // that taskrun failed runtime validation + TaskRunReasonFailedValidation TaskRunReason = "TaskRunValidationFailed" + // TaskRunReasonTaskFailedValidation indicated that the reason for failure status is + // that task failed runtime validation + TaskRunReasonTaskFailedValidation TaskRunReason = "TaskValidationFailed" + // TaskRunReasonResourceVerificationFailed indicates that the task fails the trusted resource verification, + // it could be the content has changed, signature is invalid or public key is invalid + TaskRunReasonResourceVerificationFailed TaskRunReason = "ResourceVerificationFailed" ) func (t TaskRunReason) String() string { diff --git a/pkg/pod/status.go b/pkg/pod/status.go index e9a7595faf1..2a67d98162f 100644 --- a/pkg/pod/status.go +++ b/pkg/pod/status.go @@ -38,19 +38,23 @@ import ( "knative.dev/pkg/apis" ) -const ( +// Aliased for backwards compatibility; do not add additional TaskRun reasons here +var ( // ReasonFailedResolution indicated that the reason for failure status is // that references within the TaskRun could not be resolved - ReasonFailedResolution = "TaskRunResolutionFailed" - + ReasonFailedResolution = v1.TaskRunReasonFailedResolution.String() // ReasonFailedValidation indicated that the reason for failure status is // that taskrun failed runtime validation - ReasonFailedValidation = "TaskRunValidationFailed" - + ReasonFailedValidation = v1.TaskRunReasonFailedValidation // ReasonTaskFailedValidation indicated that the reason for failure status is // that task failed runtime validation - ReasonTaskFailedValidation = "TaskValidationFailed" + ReasonTaskFailedValidation = v1.TaskRunReasonTaskFailedValidation + // ReasonResourceVerificationFailed indicates that the task fails the trusted resource verification, + // it could be the content has changed, signature is invalid or public key is invalid + ReasonResourceVerificationFailed = v1.TaskRunReasonResourceVerificationFailed +) +const ( // ReasonExceededResourceQuota indicates that the TaskRun failed to create a pod due to // a ResourceQuota in the namespace ReasonExceededResourceQuota = "ExceededResourceQuota" @@ -75,11 +79,7 @@ const ( // ReasonPending indicates that the pod is in corev1.Pending, and the reason is not // ReasonExceededNodeResources or isPodHitConfigError - ReasonPending = "Pending" - - // ReasonResourceVerificationFailed indicates that the task fails the trusted resource verification, - // it could be the content has changed, signature is invalid or public key is invalid - ReasonResourceVerificationFailed = "ResourceVerificationFailed" + ReasonPodPending = "Pending" // timeFormat is RFC3339 with millisecond timeFormat = "2006-01-02T15:04:05.000Z07:00" @@ -380,7 +380,7 @@ func updateIncompleteTaskRunStatus(trs *v1.TaskRunStatus, pod *corev1.Pod) { case isPullImageError(pod): markStatusRunning(trs, ReasonPullImageFailed, getWaitingMessage(pod)) default: - markStatusRunning(trs, ReasonPending, getWaitingMessage(pod)) + markStatusRunning(trs, ReasonPodPending, getWaitingMessage(pod)) } case corev1.PodSucceeded, corev1.PodFailed, corev1.PodUnknown: // Do nothing; pod has completed or is in an unknown state. diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index a8463fe2b25..0105cdd8fbe 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -170,6 +170,9 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1.TaskRun) pkgrecon } // Check for Pod Failures + // TODO: + // tr.status.steps if imagepullbackoff error, then it will fail taskrun + // This is part of the reconciler process, if it already has step status if failed, reason, message := c.checkPodFailed(tr); failed { err := c.failTaskRun(ctx, tr, reason, message) return c.finishReconcileUpdateEmitEvents(ctx, tr, before, err) @@ -349,7 +352,7 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, if resources.IsErrTransient(err) { return nil, nil, err } - tr.Status.MarkResourceFailed(podconvert.ReasonFailedResolution, err) + tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedResolution, err) return nil, nil, controller.NewPermanentError(err) default: // Store the fetched TaskSpec on the TaskRun for auditing @@ -374,7 +377,7 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, if resources.IsErrTransient(err) { return nil, nil, err } - tr.Status.MarkResourceFailed(podconvert.ReasonFailedResolution, err) + tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedResolution, err) return nil, nil, controller.NewPermanentError(err) default: // Store the fetched StepActions to TaskSpec, and update the stored TaskSpec again @@ -388,7 +391,7 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, switch taskMeta.VerificationResult.VerificationResultType { case trustedresources.VerificationError: logger.Errorf("TaskRun %s/%s referred task failed signature verification", tr.Namespace, tr.Name) - tr.Status.MarkResourceFailed(podconvert.ReasonResourceVerificationFailed, taskMeta.VerificationResult.Err) + tr.Status.MarkResourceFailed(v1.TaskRunReasonResourceVerificationFailed, taskMeta.VerificationResult.Err) tr.Status.SetCondition(&apis.Condition{ Type: trustedresources.ConditionTrustedResourcesVerified, Status: corev1.ConditionFalse, @@ -445,7 +448,7 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, if err := c.updateTaskRunWithDefaultWorkspaces(ctx, tr, taskSpec); err != nil { logger.Errorf("Failed to update taskrun %s with default workspace: %v", tr.Name, err) - tr.Status.MarkResourceFailed(podconvert.ReasonFailedResolution, err) + tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedResolution, err) return nil, nil, controller.NewPermanentError(err) } @@ -671,7 +674,7 @@ func (c *Reconciler) handlePodCreationError(tr *v1.TaskRun, err error) error { case isResourceQuotaConflictError(err): // Requeue if it runs into ResourceQuotaConflictError Error i.e https://github.com/kubernetes/kubernetes/issues/67761 tr.Status.StartTime = nil - tr.Status.MarkResourceOngoing(podconvert.ReasonPending, "tried to create pod, but it failed with ResourceQuotaConflictError") + tr.Status.MarkResourceOngoing(podconvert.ReasonPodPending, "tried to create pod, but it failed with ResourceQuotaConflictError") return controller.NewRequeueAfter(time.Second) case isExceededResourceQuotaError(err): // If we are struggling to create the pod, then it hasn't started. @@ -681,7 +684,7 @@ func (c *Reconciler) handlePodCreationError(tr *v1.TaskRun, err error) error { case isTaskRunValidationFailed(err): tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) case k8serrors.IsAlreadyExists(err): - tr.Status.MarkResourceOngoing(podconvert.ReasonPending, "tried to create pod, but it already exists") + tr.Status.MarkResourceOngoing(podconvert.ReasonPodPending, "tried to create pod, but it already exists") case isPodAdmissionFailed(err): tr.Status.MarkResourceFailed(podconvert.ReasonPodAdmissionFailed, err) default: diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index 5c870577793..7c51e091810 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -2072,7 +2072,7 @@ spec: t.Fatalf("Somehow had error getting reconciled run out of fake client: %s", err) } - if tc.wantFailed && reconciledRun.Status.GetCondition(apis.ConditionSucceeded).Reason != podconvert.ReasonTaskFailedValidation { + if tc.wantFailed && reconciledRun.Status.GetCondition(apis.ConditionSucceeded).Reason != v1.TaskRunReasonTaskFailedValidation.String() { t.Errorf("Expected TaskRun to have reason FailedValidation, but condition reason is %s", reconciledRun.Status.GetCondition(apis.ConditionSucceeded)) } if !tc.wantFailed && reconciledRun.Status.GetCondition(apis.ConditionSucceeded).IsFalse() { @@ -3493,7 +3493,7 @@ status: err: k8sapierrors.NewConflict(k8sruntimeschema.GroupResource{Group: "v1", Resource: "resourcequotas"}, "sample", errors.New("operation cannot be fulfilled on resourcequotas sample the object has been modified please apply your changes to the latest version and try again")), expectedType: apis.ConditionSucceeded, expectedStatus: corev1.ConditionUnknown, - expectedReason: podconvert.ReasonPending, + expectedReason: podconvert.ReasonPodPending, }, { description: "exceeded quota errors are surfaced in taskrun condition but do not fail taskrun", err: k8sapierrors.NewForbidden(k8sruntimeschema.GroupResource{Group: "foo", Resource: "bar"}, "baz", errors.New("exceeded quota")), @@ -3505,7 +3505,7 @@ status: err: errors.New("TaskRun validation failed"), expectedType: apis.ConditionSucceeded, expectedStatus: corev1.ConditionFalse, - expectedReason: podconvert.ReasonFailedValidation, + expectedReason: v1.TaskRunReasonFailedValidation.String(), }, { description: "errors other than exceeded quota fail the taskrun", err: errors.New("this is a fatal error"), @@ -3714,7 +3714,7 @@ spec: failedCorrectly := false for _, c := range tr.Status.Conditions { - if c.Type == apis.ConditionSucceeded && c.Status == corev1.ConditionFalse && c.Reason == podconvert.ReasonFailedValidation { + if c.Type == apis.ConditionSucceeded && c.Status == corev1.ConditionFalse && c.Reason == v1.TaskRunReasonFailedValidation.String() { failedCorrectly = true } } @@ -3780,7 +3780,7 @@ spec: } for _, c := range tr.Status.Conditions { - if c.Type == apis.ConditionSucceeded && c.Status == corev1.ConditionFalse && c.Reason == podconvert.ReasonFailedValidation { + if c.Type == apis.ConditionSucceeded && c.Status == corev1.ConditionFalse && c.Reason == v1.TaskRunReasonFailedValidation.String() { t.Errorf("Expected TaskRun to pass Validation by using the default workspace but it did not. Final conditions were:\n%#v", tr.Status.Conditions) } } @@ -3972,7 +3972,7 @@ spec: "disable-affinity-assistant": "false", "coschedule": "workspaces", }, - expectFailureReason: podconvert.ReasonFailedValidation, + expectFailureReason: v1.TaskRunReasonFailedValidation.String(), }, { name: "multiple PVC based Workspaces in per pipelinerun coschedule mode - success", cfgMap: map[string]string{ @@ -5620,13 +5620,13 @@ status: }{{ name: "taskrun results type mismatched", taskRun: taskRunResultsTypeMismatched, - wantFailedReason: podconvert.ReasonFailedValidation, + wantFailedReason: v1.TaskRunReasonFailedValidation.String(), expectedError: fmt.Errorf("1 error occurred:\n\t* Provided results don't match declared results; may be invalid JSON or missing result declaration: \"aResult\": task result is expected to be \"array\" type but was initialized to a different type \"string\", \"objectResult\": task result is expected to be \"object\" type but was initialized to a different type \"string\""), expectedResults: nil, }, { name: "taskrun results object miss key", taskRun: taskRunResultsObjectMissKey, - wantFailedReason: podconvert.ReasonFailedValidation, + wantFailedReason: v1.TaskRunReasonFailedValidation.String(), expectedError: fmt.Errorf("1 error occurred:\n\t* missing keys for these results which are required in TaskResult's properties map[objectResult:[commit]]"), expectedResults: []v1.TaskRunResult{ { @@ -5978,7 +5978,7 @@ status: t.Fatalf("getting updated taskrun: %v", err) } condition := reconciledRun.Status.GetCondition(apis.ConditionSucceeded) - if condition.Type != apis.ConditionSucceeded || condition.Status != corev1.ConditionFalse || condition.Reason != podconvert.ReasonResourceVerificationFailed { + if condition.Type != apis.ConditionSucceeded || condition.Status != corev1.ConditionFalse || condition.Reason != v1.TaskRunReasonResourceVerificationFailed.String() { t.Errorf("Expected TaskRun to fail with reason \"%s\" but it did not. Final conditions were:\n%#v", podconvert.ReasonResourceVerificationFailed, tr.Status.Conditions) } gotVerificationCondition := reconciledRun.Status.GetCondition(trustedresources.ConditionTrustedResourcesVerified) @@ -6233,7 +6233,7 @@ status: t.Fatalf("getting updated taskrun: %v", err) } condition := reconciledRun.Status.GetCondition(apis.ConditionSucceeded) - if condition.Type != apis.ConditionSucceeded || condition.Status != corev1.ConditionFalse || condition.Reason != podconvert.ReasonResourceVerificationFailed { + if condition.Type != apis.ConditionSucceeded || condition.Status != corev1.ConditionFalse || condition.Reason != v1.TaskRunReasonResourceVerificationFailed.String() { t.Errorf("Expected TaskRun to fail with reason \"%s\" but it did not. Final conditions were:\n%#v", podconvert.ReasonResourceVerificationFailed, tr.Status.Conditions) } gotVerificationCondition := reconciledRun.Status.GetCondition(trustedresources.ConditionTrustedResourcesVerified)