From 1b2783a6206f0b48d154a493cc914d39afc49be7 Mon Sep 17 00:00:00 2001 From: Scott Date: Mon, 13 Sep 2021 10:07:42 -0400 Subject: [PATCH] Add controller flag to turn off built-in resolution We're currently trying to work out what the best way forward for TEP-0060 (remote resolution) is for Tekton Pipelines. As part of that we're creating a controller in the experimental repo that we can use to test out a bunch of the Alternatives from the TEP and figure out what works best in terms of implementation and DX. Right now the experimental controller would race with the Pipelines controllers since they're all going to try and "resolve" any taskRef in Taskruns and PipelineRuns. This commit adds a flag to the pipelines controller that explicitly switches off resolution performed by the taskrun and pipelinerun reconcilers. This gives us just enough room to perform resolution out-of-band and try out some of the alternatives. When the `-experimental-disable-in-tree-resolution` flag is set the taskrun and pipelinerun controllers will ignore taskruns and pipelineruns that don't have `status.taskSpec` or `status.pipelineSpec` populated. --- cmd/controller/main.go | 18 +++++- config/controller.yaml | 5 +- pkg/reconciler/pipelinerun/controller.go | 15 ++++- pkg/reconciler/pipelinerun/pipelinerun.go | 21 +++++++ .../pipelinerun/pipelinerun_test.go | 2 +- pkg/reconciler/taskrun/controller.go | 15 ++++- pkg/reconciler/taskrun/taskrun.go | 55 +++++++++++++------ pkg/reconciler/taskrun/taskrun_test.go | 2 +- 8 files changed, 107 insertions(+), 26 deletions(-) diff --git a/cmd/controller/main.go b/cmd/controller/main.go index 64bca8f2b5e..f844adb53a8 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -55,6 +55,10 @@ var ( disableHighAvailability = flag.Bool("disable-ha", false, "Whether to disable high-availability functionality for this component. This flag will be deprecated "+ "and removed when we have promoted this feature to stable, so do not pass it without filing an "+ "issue upstream!") + experimentalDisableInTreeResolution = flag.Bool(disableInTreeResolutionFlag, false, + "Disable resolution of taskrun and pipelinerun refs by the taskrun and pipelinerun reconcilers.") + + disableInTreeResolutionFlag = "experimental-disable-in-tree-resolution" ) func main() { @@ -108,10 +112,20 @@ func main() { log.Fatal(http.ListenAndServe(":"+port, mux)) }() + taskrunControllerConfig := taskrun.ControllerConfiguration{ + Images: images, + DisableTaskRefResolution: *experimentalDisableInTreeResolution, + } + + pipelinerunControllerConfig := pipelinerun.ControllerConfiguration{ + Images: images, + DisablePipelineRefResolution: *experimentalDisableInTreeResolution, + } + ctx = filteredinformerfactory.WithSelectors(ctx, v1beta1.ManagedByLabelKey) sharedmain.MainWithConfig(ctx, ControllerLogKey, cfg, - taskrun.NewController(*namespace, images), - pipelinerun.NewController(*namespace, images), + taskrun.NewController(*namespace, taskrunControllerConfig), + pipelinerun.NewController(*namespace, pipelinerunControllerConfig), ) } diff --git a/config/controller.yaml b/config/controller.yaml index 183da5b422a..422b0b29211 100644 --- a/config/controller.yaml +++ b/config/controller.yaml @@ -80,7 +80,10 @@ spec: "-shell-image", "gcr.io/distroless/base@sha256:aa4fd987555ea10e1a4ec8765da8158b5ffdfef1e72da512c7ede509bc9966c4", # for script mode to work with windows we need a powershell image # pinning to nanoserver tag as of July 15 2021 - "-shell-image-win", "mcr.microsoft.com/powershell:nanoserver@sha256:b6d5ff841b78bdf2dfed7550000fd4f3437385b8fa686ec0f010be24777654d6" + "-shell-image-win", "mcr.microsoft.com/powershell:nanoserver@sha256:b6d5ff841b78bdf2dfed7550000fd4f3437385b8fa686ec0f010be24777654d6", + # Experimental. Uncomment below to disable TaskRun and PipelineRun + # reconcilers' built-in taskRef and pipelineRef resolution procedures. + # "-experimental-disable-in-tree-resolution", ] volumeMounts: - name: config-logging diff --git a/pkg/reconciler/pipelinerun/controller.go b/pkg/reconciler/pipelinerun/controller.go index 46ecbaa00fd..04c2fb5c95b 100644 --- a/pkg/reconciler/pipelinerun/controller.go +++ b/pkg/reconciler/pipelinerun/controller.go @@ -42,8 +42,18 @@ import ( "knative.dev/pkg/logging" ) +// ControllerConfiguration holds fields used to configure the +// PipelineRun controller. +type ControllerConfiguration struct { + // Images are the image references used across Tekton Pipelines. + Images pipeline.Images + // DisablePipelineRefResolution tells the controller not to perform + // resolution of pipeline refs from the cluster or bundles. + DisablePipelineRefResolution bool +} + // NewController instantiates a new controller.Impl from knative.dev/pkg/controller -func NewController(namespace string, images pipeline.Images) func(context.Context, configmap.Watcher) *controller.Impl { +func NewController(namespace string, conf ControllerConfiguration) func(context.Context, configmap.Watcher) *controller.Impl { return func(ctx context.Context, cmw configmap.Watcher) *controller.Impl { logger := logging.FromContext(ctx) kubeclientset := kubeclient.Get(ctx) @@ -62,7 +72,7 @@ func NewController(namespace string, images pipeline.Images) func(context.Contex c := &Reconciler{ KubeClientSet: kubeclientset, PipelineClientSet: pipelineclientset, - Images: images, + Images: conf.Images, pipelineRunLister: pipelineRunInformer.Lister(), pipelineLister: pipelineInformer.Lister(), taskLister: taskInformer.Lister(), @@ -74,6 +84,7 @@ func NewController(namespace string, images pipeline.Images) func(context.Contex cloudEventClient: cloudeventclient.Get(ctx), metrics: pipelinerunmetrics.Get(ctx), pvcHandler: volumeclaim.NewPVCHandler(kubeclientset, logger), + disableResolution: conf.DisablePipelineRefResolution, } impl := pipelinerunreconciler.NewImpl(ctx, c, func(impl *controller.Impl) controller.Options { return controller.Options{ diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 092fb74314b..56c21ea4e58 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -132,11 +132,21 @@ type Reconciler struct { cloudEventClient cloudevent.CEClient metrics *pipelinerunmetrics.Recorder pvcHandler volumeclaim.PvcHandler + + // disableResolution is a flag to the reconciler that it should + // not be performing resolution of pipelineRefs. + // TODO(sbwsg): Once we've agreed on a way forward for TEP-0060 + // this can be removed in favor of whatever that chosen solution + // is. + disableResolution bool } var ( // Check that our Reconciler implements pipelinerunreconciler.Interface _ pipelinerunreconciler.Interface = (*Reconciler)(nil) + + // Indicates pipelinerun resolution hasn't occurred yet. + errResourceNotResolved = fmt.Errorf("pipeline ref has not been resolved") ) // ReconcileKind compares the actual state with the desired, and attempts to @@ -232,6 +242,13 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1beta1.PipelineRun) logger.Errorf("Reconcile error: %v", err.Error()) } + if c.disableResolution && err == errResourceNotResolved { + // This is not an error: an out-of-band process can + // still resolve the PipelineRun, at which point + // reconciliation can continue as normal. + err = nil + } + if err = c.finishReconcileUpdateEmitEvents(ctx, pr, before, err); err != nil { return err } @@ -336,6 +353,10 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get return nil } + if c.disableResolution && pr.Status.PipelineSpec == nil { + return errResourceNotResolved + } + pipelineMeta, pipelineSpec, err := resources.GetPipelineData(ctx, pr, getPipelineFunc) if err != nil { logger.Errorf("Failed to determine Pipeline spec to use for pipelinerun %s: %v", pr.Name, err) diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 9fcdf4bb16b..42152cdd74b 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -159,7 +159,7 @@ func getPipelineRunController(t *testing.T, d test.Data) (test.Assets, func()) { c, informers := test.SeedTestData(t, ctx, d) configMapWatcher := cminformer.NewInformedWatcher(c.Kube, system.Namespace()) - ctl := NewController(namespace, images)(ctx, configMapWatcher) + ctl := NewController(namespace, ControllerConfiguration{Images: images})(ctx, configMapWatcher) if la, ok := ctl.Reconciler.(reconciler.LeaderAware); ok { la.Promote(reconciler.UniversalBucket(), func(reconciler.Bucket, types.NamespacedName) {}) diff --git a/pkg/reconciler/taskrun/controller.go b/pkg/reconciler/taskrun/controller.go index 1bb197727f1..81d3bd91857 100644 --- a/pkg/reconciler/taskrun/controller.go +++ b/pkg/reconciler/taskrun/controller.go @@ -40,8 +40,18 @@ import ( "knative.dev/pkg/logging" ) +// ControllerConfiguration holds fields used to configure the +// TaskRun controller. +type ControllerConfiguration struct { + // Images are the image references used across Tekton Pipelines. + Images pipeline.Images + // DisableTaskRefResolution tells the controller not to perform + // resolution of task refs from the cluster or bundles. + DisableTaskRefResolution bool +} + // NewController instantiates a new controller.Impl from knative.dev/pkg/controller -func NewController(namespace string, images pipeline.Images) func(context.Context, configmap.Watcher) *controller.Impl { +func NewController(namespace string, conf ControllerConfiguration) func(context.Context, configmap.Watcher) *controller.Impl { return func(ctx context.Context, cmw configmap.Watcher) *controller.Impl { logger := logging.FromContext(ctx) kubeclientset := kubeclient.Get(ctx) @@ -62,7 +72,7 @@ func NewController(namespace string, images pipeline.Images) func(context.Contex c := &Reconciler{ KubeClientSet: kubeclientset, PipelineClientSet: pipelineclientset, - Images: images, + Images: conf.Images, taskRunLister: taskRunInformer.Lister(), taskLister: taskInformer.Lister(), clusterTaskLister: clusterTaskInformer.Lister(), @@ -71,6 +81,7 @@ func NewController(namespace string, images pipeline.Images) func(context.Contex metrics: taskrunmetrics.Get(ctx), entrypointCache: entrypointCache, pvcHandler: volumeclaim.NewPVCHandler(kubeclientset, logger), + disableResolution: conf.DisableTaskRefResolution, } impl := taskrunreconciler.NewImpl(ctx, c, func(impl *controller.Impl) controller.Options { return controller.Options{ diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 7261062cd26..26c96cf3af5 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -74,10 +74,22 @@ type Reconciler struct { entrypointCache podconvert.EntrypointCache metrics *taskrunmetrics.Recorder pvcHandler volumeclaim.PvcHandler + + // disableResolution is a flag to the reconciler that it should + // not be performing resolution of taskRefs. + // TODO(sbwsg): Once we've agreed on a way forward for TEP-0060 + // this can be removed in favor of whatever that chosen solution + // is. + disableResolution bool } // Check that our Reconciler implements taskrunreconciler.Interface -var _ taskrunreconciler.Interface = (*Reconciler)(nil) +var ( + _ taskrunreconciler.Interface = (*Reconciler)(nil) + + // Indicates taskrun resolution hasn't occurred yet. + errResourceNotResolved = fmt.Errorf("task ref has not been resolved") +) // ReconcileKind compares the actual state with the desired, and attempts to // converge the two. It then updates the Status block of the Task Run @@ -168,25 +180,30 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1beta1.TaskRun) pkg // taskrun, runs API convertions. Errors that come out of prepare are // permanent one, so in case of error we update, emit events and return _, rtr, err := c.prepare(ctx, tr) - if err != nil { - logger.Errorf("TaskRun prepare error: %v", err.Error()) - // We only return an error if update failed, otherwise we don't want to - // reconcile an invalid TaskRun anymore - return c.finishReconcileUpdateEmitEvents(ctx, tr, nil, err) - } + if c.disableResolution && err == errResourceNotResolved { + // This is not an error - the taskrun is still expected + // to be resolved out-of-band. + } else { + if err != nil { + logger.Errorf("TaskRun prepare error: %v", err.Error()) + // We only return an error if update failed, otherwise we don't want to + // reconcile an invalid TaskRun anymore + return c.finishReconcileUpdateEmitEvents(ctx, tr, nil, err) + } - // Store the condition before reconcile - before = tr.Status.GetCondition(apis.ConditionSucceeded) + // Store the condition before reconcile + before = tr.Status.GetCondition(apis.ConditionSucceeded) - // Reconcile this copy of the task run and then write back any status - // updates regardless of whether the reconciliation errored out. - if err = c.reconcile(ctx, tr, rtr); err != nil { - logger.Errorf("Reconcile: %v", err.Error()) - } + // Reconcile this copy of the task run and then write back any status + // updates regardless of whether the reconciliation errored out. + if err = c.reconcile(ctx, tr, rtr); err != nil { + logger.Errorf("Reconcile: %v", err.Error()) + } - // Emit events (only when ConditionSucceeded was changed) - if err = c.finishReconcileUpdateEmitEvents(ctx, tr, before, err); err != nil { - return err + // Emit events (only when ConditionSucceeded was changed) + if err = c.finishReconcileUpdateEmitEvents(ctx, tr, before, err); err != nil { + return err + } } if tr.Status.StartTime != nil { @@ -270,6 +287,10 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1 // and may not have had all of the assumed default specified. tr.SetDefaults(contexts.WithUpgradeViaDefaulting(ctx)) + if c.disableResolution && tr.Status.TaskSpec == nil { + return nil, nil, errResourceNotResolved + } + getTaskfunc, err := resources.GetTaskFuncFromTaskRun(ctx, c.KubeClientSet, c.PipelineClientSet, tr) if err != nil { logger.Errorf("Failed to fetch task reference %s: %v", tr.Spec.TaskRef.Name, err) diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index 10a22f8d1f8..66de42251d0 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -329,7 +329,7 @@ func getTaskRunController(t *testing.T, d test.Data) (test.Assets, func()) { c, informers := test.SeedTestData(t, ctx, d) configMapWatcher := cminformer.NewInformedWatcher(c.Kube, system.Namespace()) - ctl := NewController(namespace, images)(ctx, configMapWatcher) + ctl := NewController(namespace, ControllerConfiguration{Images: images})(ctx, configMapWatcher) if err := configMapWatcher.Start(ctx.Done()); err != nil { t.Fatalf("error starting configmap watcher: %v", err) }