From 532591101c08d7f6103c3dd7cd005ef1a5c3f8cb Mon Sep 17 00:00:00 2001 From: JeromeJu Date: Tue, 28 Nov 2023 23:20:38 +0000 Subject: [PATCH] Error sweep: 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 | 32 +++++++++++++------------- pkg/reconciler/taskrun/taskrun_test.go | 20 ++++++++-------- test/trusted_resources_test.go | 6 ++--- 6 files changed, 77 insertions(+), 43 deletions(-) diff --git a/docs/pipeline-api.md b/docs/pipeline-api.md index 88c0ab2f622..1aa5ffaaefe 100644 --- a/docs/pipeline-api.md +++ b/docs/pipeline-api.md @@ -5129,9 +5129,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

@@ -5141,9 +5156,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 ff78c122dfa..f3c44a6bf8a 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 4121bd417ec..f68ef71518a 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.String() // ReasonTaskFailedValidation indicated that the reason for failure status is // that task failed runtime validation - ReasonTaskFailedValidation = "TaskValidationFailed" + ReasonTaskFailedValidation = v1.TaskRunReasonTaskFailedValidation.String() + // 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.String() +) +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" @@ -508,7 +508,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 508e26ef840..fdbe78cb993 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -340,7 +340,7 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, tr.Status.MarkResourceOngoing(v1.TaskRunReasonResolvingTaskRef, message) return nil, nil, err case errors.Is(err, apiserver.ErrReferencedObjectValidationFailed), errors.Is(err, apiserver.ErrCouldntValidateObjectPermanent): - tr.Status.MarkResourceFailed(podconvert.ReasonTaskFailedValidation, err) + tr.Status.MarkResourceFailed(v1.TaskRunReasonTaskFailedValidation, err) return nil, nil, controller.NewPermanentError(err) case errors.Is(err, apiserver.ErrCouldntValidateObjectRetryable): return nil, nil, err @@ -349,7 +349,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 @@ -365,7 +365,7 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, tr.Status.MarkResourceOngoing(v1.TaskRunReasonResolvingStepActionRef, message) return nil, nil, err case errors.Is(err, apiserver.ErrReferencedObjectValidationFailed), errors.Is(err, apiserver.ErrCouldntValidateObjectPermanent): - tr.Status.MarkResourceFailed(podconvert.ReasonTaskFailedValidation, err) + tr.Status.MarkResourceFailed(v1.TaskRunReasonTaskFailedValidation, err) return nil, nil, controller.NewPermanentError(err) case errors.Is(err, apiserver.ErrCouldntValidateObjectRetryable): return nil, nil, err @@ -374,7 +374,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 +388,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, @@ -419,13 +419,13 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, if err := validateTaskSpecRequestResources(taskSpec); err != nil { logger.Errorf("TaskRun %s taskSpec request resources are invalid: %v", tr.Name, err) - tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) + tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedValidation, err) return nil, nil, controller.NewPermanentError(err) } if err := ValidateResolvedTask(ctx, tr.Spec.Params, &v1.Matrix{}, rtr); err != nil { logger.Errorf("TaskRun %q resources are invalid: %v", tr.Name, err) - tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) + tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedValidation, err) return nil, nil, controller.NewPermanentError(err) } @@ -439,13 +439,13 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, if err := resources.ValidateParamArrayIndex(rtr.TaskSpec, tr.Spec.Params); err != nil { logger.Errorf("TaskRun %q Param references are invalid: %v", tr.Name, err) - tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) + tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedValidation, err) return nil, nil, controller.NewPermanentError(err) } 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) } @@ -464,7 +464,7 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, } if err := workspace.ValidateBindings(ctx, workspaceDeclarations, tr.Spec.Workspaces); err != nil { logger.Errorf("TaskRun %q workspaces are invalid: %v", tr.Name, err) - tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) + tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedValidation, err) return nil, nil, controller.NewPermanentError(err) } @@ -475,14 +475,14 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, if aaBehavior == affinityassistant.AffinityAssistantPerWorkspace { if err := workspace.ValidateOnlyOnePVCIsUsed(tr.Spec.Workspaces); err != nil { logger.Errorf("TaskRun %q workspaces incompatible with Affinity Assistant: %v", tr.Name, err) - tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) + tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedValidation, err) return nil, nil, controller.NewPermanentError(err) } } if err := validateOverrides(taskSpec, &tr.Spec); err != nil { logger.Errorf("TaskRun %q step or sidecar overrides are invalid: %v", tr.Name, err) - tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) + tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedValidation, err) return nil, nil, controller.NewPermanentError(err) } @@ -596,7 +596,7 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1.TaskRun, rtr *resourc } if err := validateTaskRunResults(tr, rtr.TaskSpec); err != nil { - tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) + tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedValidation, err) return err } @@ -671,7 +671,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. @@ -679,9 +679,9 @@ func (c *Reconciler) handlePodCreationError(tr *v1.TaskRun, err error) error { tr.Status.MarkResourceOngoing(podconvert.ReasonExceededResourceQuota, fmt.Sprint("TaskRun Pod exceeded available resources: ", err)) return controller.NewRequeueAfter(time.Minute) case isTaskRunValidationFailed(err): - tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) + tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedValidation, 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 7bfd7266792..5148ac1b9d6 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() { @@ -3522,7 +3522,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")), @@ -3534,7 +3534,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"), @@ -3743,7 +3743,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 } } @@ -3809,7 +3809,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) } } @@ -4001,7 +4001,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{ @@ -5658,13 +5658,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{ { @@ -6016,7 +6016,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) @@ -6271,7 +6271,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) diff --git a/test/trusted_resources_test.go b/test/trusted_resources_test.go index f1b74c2c4b6..59aea1ca6dc 100644 --- a/test/trusted_resources_test.go +++ b/test/trusted_resources_test.go @@ -29,7 +29,7 @@ import ( "github.com/sigstore/sigstore/pkg/signature" "github.com/tektoncd/pipeline/pkg/apis/config" - "github.com/tektoncd/pipeline/pkg/pod" + pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "github.com/tektoncd/pipeline/test/parse" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -283,8 +283,8 @@ spec: if pr.Status.GetCondition(apis.ConditionSucceeded).IsTrue() { t.Errorf("Expected PipelineRun to fail but found condition: %s", pr.Status.GetCondition(apis.ConditionSucceeded)) } - if pr.Status.Conditions[0].Reason != pod.ReasonResourceVerificationFailed { - t.Errorf("Expected PipelineRun fail condition is: %s but got: %s", pod.ReasonResourceVerificationFailed, pr.Status.Conditions[0].Reason) + if pr.Status.Conditions[0].Reason != pipelinev1.TaskRunReasonResourceVerificationFailed.String() { + t.Errorf("Expected PipelineRun fail condition is: %s but got: %s", pipelinev1.TaskRunReasonResourceVerificationFailed, pr.Status.Conditions[0].Reason) } }