Skip to content

Commit

Permalink
Error sweep: Move TaskRun Reasons in pkg/pod to pkg/apis
Browse files Browse the repository at this point in the history 
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: #7397
  • Loading branch information
@JeromeJu
JeromeJu committed Nov 28, 2023
1 parent ff59bae commit ff881a5
Show file tree
Hide file tree
Showing 6 changed files with 77 additions and 43 deletions.
22 changes: 22 additions & 0 deletions docs/pipeline-api.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5026,9 +5026,24 @@ reasons that emerge from underlying resources are not included here</p>
</tr><tr><td><p>&#34;Failed&#34;</p></td>
<td><p>TaskRunReasonFailed is the reason set when the TaskRun completed with a failure</p>
</td>
</tr><tr><td><p>&#34;TaskRunResolutionFailed&#34;</p></td>
<td><p>TaskRunReasonFailedResolution indicated that the reason for failure status is
that references within the TaskRun could not be resolved</p>
</td>
</tr><tr><td><p>&#34;TaskRunValidationFailed&#34;</p></td>
<td><p>TaskRunReasonFailedValidation indicated that the reason for failure status is
that taskrun failed runtime validation</p>
</td>
</tr><tr><td><p>&#34;TaskRunImagePullFailed&#34;</p></td>
<td><p>TaskRunReasonImagePullFailed is the reason set when the step of a task fails due to image not being pulled</p>
</td>
</tr><tr><td><p>&#34;InvalidParamValue&#34;</p></td>
<td><p>TaskRunReasonInvalidParamValue indicates that the TaskRun Param input value is not allowed.</p>
</td>
</tr><tr><td><p>&#34;ResourceVerificationFailed&#34;</p></td>
<td><p>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</p>
</td>
</tr><tr><td><p>&#34;TaskRunResultLargerThanAllowedLimit&#34;</p></td>
<td><p>TaskRunReasonResultLargerThanAllowedLimit is the reason set when one of the results exceeds its maximum allowed limit of 1 KB</p>
</td>
Expand All @@ -5038,9 +5053,16 @@ reasons that emerge from underlying resources are not included here</p>
</tr><tr><td><p>&#34;Started&#34;</p></td>
<td><p>TaskRunReasonStarted is the reason set when the TaskRun has just started</p>
</td>
</tr><tr><td><p>&#34;TaskRunStopSidecarFailed&#34;</p></td>
<td><p>TaskRunReasonStopSidecarFailed indicates that the sidecar is not properly stopped.</p>
</td>
</tr><tr><td><p>&#34;Succeeded&#34;</p></td>
<td><p>TaskRunReasonSuccessful is the reason set when the TaskRun completed successfully</p>
</td>
</tr><tr><td><p>&#34;TaskValidationFailed&#34;</p></td>
<td><p>TaskRunReasonTaskFailedValidation indicated that the reason for failure status is
that task failed runtime validation</p>
</td>
</tr><tr><td><p>&#34;TaskRunTimeout&#34;</p></td>
<td><p>TaskRunReasonTimedOut is the reason set when one TaskRun execution has timed out</p>
</td>
Expand Down
16 changes: 14 additions & 2 deletions pkg/apis/pipeline/v1/taskrun_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
24 changes: 12 additions & 12 deletions pkg/pod/status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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.
Expand Down
32 changes: 16 additions & 16 deletions pkg/reconciler/taskrun/taskrun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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)
}

Expand All @@ -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)
}

Expand All @@ -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)
}

Expand All @@ -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)
}

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -671,17 +671,17 @@ 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.
tr.Status.StartTime = nil
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:
Expand Down
20 changes: 10 additions & 10 deletions pkg/reconciler/taskrun/taskrun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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")),
Expand All @@ -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"),
Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
{
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
Loading

0 comments on commit ff881a5

Please sign in to comment.