From cc5c5560c10614400ce3880b764c8ba908957384 Mon Sep 17 00:00:00 2001 From: Jason Hall Date: Thu, 1 Oct 2020 13:33:27 -0400 Subject: [PATCH] Derive cancel patch bytes once at controller startup This avoids JSON marshaling the same object every time a PipelineRun is cancelled, and marshals the object once at startup. If for some reason marshaling that object fails (which it should never do), the controller will exit and restart. Failure to marshal this object should only be caused by a bug in our code, since the object isn't derived from anything related to user requests. If we have a bug in our cancel patch generation, we'd rather find out by having the controller crashloop than find out by having PipelineRuns be uncancellable, which might be harder to detect. --- pkg/reconciler/pipelinerun/cancel.go | 42 ++++++++++++---------------- 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/pkg/reconciler/pipelinerun/cancel.go b/pkg/reconciler/pipelinerun/cancel.go index 86bbb7f9fcb..81dcea11819 100644 --- a/pkg/reconciler/pipelinerun/cancel.go +++ b/pkg/reconciler/pipelinerun/cancel.go @@ -19,37 +19,44 @@ package pipelinerun import ( "encoding/json" "fmt" + "log" "strings" "time" - "go.uber.org/zap" - jsonpatch "gomodules.xyz/jsonpatch/v2" - "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned" + "go.uber.org/zap" + jsonpatch "gomodules.xyz/jsonpatch/v2" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "knative.dev/pkg/apis" ) -// cancelPipelineRun marks the PipelineRun as cancelled and any resolved TaskRun(s) too. -func cancelPipelineRun(logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, clientSet clientset.Interface) error { - errs := []string{} +var cancelPatchBytes []byte - // Use Patch to update the TaskRuns since the TaskRun controller may be operating on the - // TaskRuns at the same time and trying to update the entire object may cause a race - b, err := getCancelPatch() +func init() { + var err error + cancelPatchBytes, err = json.Marshal([]jsonpatch.JsonPatchOperation{{ + Operation: "add", + Path: "/spec/status", + Value: v1beta1.TaskRunSpecStatusCancelled, + }}) if err != nil { - return fmt.Errorf("couldn't make patch to update TaskRun cancellation: %v", err) + log.Fatalf("failed to marshal cancel patch bytes: %v", err) } +} + +// cancelPipelineRun marks the PipelineRun as cancelled and any resolved TaskRun(s) too. +func cancelPipelineRun(logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, clientSet clientset.Interface) error { + errs := []string{} // Loop over the TaskRuns in the PipelineRun status. // If a TaskRun is not in the status yet we should not cancel it anyways. for taskRunName := range pr.Status.TaskRuns { logger.Infof("cancelling TaskRun %s", taskRunName) - if _, err := clientSet.TektonV1beta1().TaskRuns(pr.Namespace).Patch(taskRunName, types.JSONPatchType, b, ""); err != nil { + if _, err := clientSet.TektonV1beta1().TaskRuns(pr.Namespace).Patch(taskRunName, types.JSONPatchType, cancelPatchBytes, ""); err != nil { errs = append(errs, fmt.Errorf("Failed to patch TaskRun `%s` with cancellation: %s", taskRunName, err).Error()) continue } @@ -77,16 +84,3 @@ func cancelPipelineRun(logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, clien } return nil } - -func getCancelPatch() ([]byte, error) { - patches := []jsonpatch.JsonPatchOperation{{ - Operation: "add", - Path: "/spec/status", - Value: v1beta1.TaskRunSpecStatusCancelled, - }} - patchBytes, err := json.Marshal(patches) - if err != nil { - return nil, fmt.Errorf("failed to marshal patch bytes in order to cancel: %v", err) - } - return patchBytes, nil -}