Skip to content

Commit

Permalink
Cleanup: 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 22, 2023
1 parent eb01d5d commit cb89738
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 30 deletions.
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
// 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"
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
15 changes: 9 additions & 6 deletions pkg/reconciler/taskrun/taskrun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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.
Expand All @@ -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:
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

0 comments on commit cb89738

Please sign in to comment.