Improve Operator upgrade mechanism

Improve ReadyChecks for Pipelines and Triggers

Add mechanism to detect upgrade scenario in reconcilers and update
status with relevant messages

Minor fixes to ensure TektonAddon reconciler acts more predictably

Signed-off-by: Nikhil Thomas <[email protected]>
(cherry picked from commit e8f205b)

Makefile

@@ -107,7 +107,7 @@ clean-cr: | ; $(info $(M) clean CRs on $(TARGET)) @ ## Clean the CRs to the curr
 resolve: | $(KO) $(KUSTOMIZE) get-releases ; $(info $(M) ko resolve on $(TARGET)) @ ## Resolve config to the current cluster
 	@ ## --load-restrictor LoadRestrictionsNone is needed in kustomize build as files which not in child tree of kustomize base are pulled
 	@ ## https://github.com/kubernetes-sigs/kustomize/issues/766
-	$Q $(KUSTOMIZE) build --load-restrictor LoadRestrictionsNone config/$(TARGET) | $(KO) resolve --push=false --oci-layout-path=$(BIN)/oci -f -
+	$Q $(KUSTOMIZE) build --load-restrictor LoadRestrictionsNone config/$(TARGET)/overlays/default | $(KO) resolve --push=false --oci-layout-path=$(BIN)/oci -f -
 
 .PHONY: generated
 generated: | vendor ; $(info $(M) update generated files) ## Update generated files

pkg/apis/operator/v1alpha1/const.go

@@ -33,6 +33,15 @@ const (
 
 	ApiFieldAlpha  = "alpha"
 	ApiFieldStable = "stable"
+
+	LastAppliedHashKey     = "operator.tekton.dev/last-applied-hash"
+	CreatedByKey           = "operator.tekton.dev/created-by"
+	ReleaseVersionKey      = "operator.tekton.dev/release-version"
+	ReleaseMinorVersionKey = "operator.tekton.dev/release-minor-version"
+	TargetNamespaceKey     = "operator.tekton.dev/target-namespace"
+	InstallerSetType       = "operator.tekton.dev/type"
+
+	UpgradePending = "upgrade pending"
 )
 
 var (
@@ -41,6 +50,10 @@ var (
 	// that we proceed ahead with updated object
 	RECONCILE_AGAIN_ERR = fmt.Errorf("reconcile again and proceed")
 
+	// DEPENDENCY_UPGRADE_PENDING_ERR
+	// When a reconciler cannot proceed due to an upgrade in progress of a dependency
+	DEPENDENCY_UPGRADE_PENDING_ERR = fmt.Errorf("dependency upgrade pending")
+
 	// VERSION_ENV_NOT_SET_ERR Error when VERSION environment variable is not set
 	VERSION_ENV_NOT_SET_ERR = fmt.Errorf("version environment variable %s is not set or empty", VersionEnvKey)
 )

pkg/reconciler/common/common.go

@@ -17,6 +17,7 @@ limitations under the License.
 package common
 
 import (
+	"context"
 	"fmt"
 	"time"
 
@@ -57,6 +58,13 @@ func PipelineReady(informer informer.TektonPipelineInformer) (*v1alpha1.TektonPi
 		}
 		return nil, err
 	}
+	upgradePending, err := CheckUpgradePending(ppln)
+	if err != nil {
+		return nil, err
+	}
+	if upgradePending {
+		return nil, v1alpha1.DEPENDENCY_UPGRADE_PENDING_ERR
+	}
 	if !ppln.Status.IsReady() {
 		return nil, fmt.Errorf(PipelineNotReady)
 	}
@@ -76,6 +84,13 @@ func TriggerReady(informer informer.TektonTriggerInformer) (*v1alpha1.TektonTrig
 		}
 		return nil, err
 	}
+	upgradePending, err := CheckUpgradePending(trigger)
+	if err != nil {
+		return nil, err
+	}
+	if upgradePending {
+		return nil, v1alpha1.DEPENDENCY_UPGRADE_PENDING_ERR
+	}
 	if !trigger.Status.IsReady() {
 		return nil, fmt.Errorf(TriggerNotReady)
 	}
@@ -86,3 +101,19 @@ func getTriggerRes(informer informer.TektonTriggerInformer) (*v1alpha1.TektonTri
 	res, err := informer.Lister().Get(TriggerResourceName)
 	return res, err
 }
+
+func CheckUpgradePending(tc v1alpha1.TektonComponent) (bool, error) {
+	labels := tc.GetLabels()
+	ver, ok := labels[v1alpha1.ReleaseVersionKey]
+	if !ok {
+		return true, nil
+	}
+	operatorVersion, err := OperatorVersion(context.TODO())
+	if err != nil {
+		return false, err
+	}
+	if ver != operatorVersion {
+		return true, nil
+	}
+	return false, nil
+}

pkg/reconciler/kubernetes/initcontroller/initcontroller.go → pkg/reconciler/common/initcontroller.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package initcontroller
+package common
 
 import (
 	"context"
@@ -26,7 +26,6 @@ import (
 	mfc "github.com/manifestival/client-go-client"
 	mf "github.com/manifestival/manifestival"
 	"github.com/tektoncd/operator/pkg/apis/operator/v1alpha1"
-	"github.com/tektoncd/operator/pkg/reconciler/common"
 	"go.uber.org/zap"
 	"knative.dev/pkg/injection"
 	"knative.dev/pkg/logging"
@@ -72,9 +71,9 @@ func (ctrl Controller) InitController(ctx context.Context, opts PayloadOptions)
 
 	var releaseVersion string
 	// Read the release version of component
-	releaseVersion, err = common.FetchVersionFromConfigMap(manifest, ctrl.VersionConfigMap)
+	releaseVersion, err = FetchVersionFromConfigMap(manifest, ctrl.VersionConfigMap)
 	if err != nil {
-		if common.IsFetchVersionError(err) {
+		if IsFetchVersionError(err) {
 			ctrl.Logger.Warnf("failed to read version information from ConfigMap %s", ctrl.VersionConfigMap, err)
 			releaseVersion = "Unknown"
 		} else {
@@ -91,29 +90,29 @@ func (ctrl Controller) fetchSourceManifests(ctx context.Context, opts PayloadOpt
 	switch {
 	case strings.Contains(ctrl.VersionConfigMap, "pipeline"):
 		var pipeline *v1alpha1.TektonPipeline
-		if err := common.AppendTarget(ctx, ctrl.Manifest, pipeline); err != nil {
+		if err := AppendTarget(ctx, ctrl.Manifest, pipeline); err != nil {
 			return err
 		}
 		// add proxy configs to pipeline if any
 		return addProxy(ctrl.Manifest)
 	case strings.Contains(ctrl.VersionConfigMap, "triggers"):
 		var trigger *v1alpha1.TektonTrigger
-		return common.AppendTarget(ctx, ctrl.Manifest, trigger)
+		return AppendTarget(ctx, ctrl.Manifest, trigger)
 	case strings.Contains(ctrl.VersionConfigMap, "dashboard") && opts.ReadOnly:
 		var dashboard v1alpha1.TektonDashboard
 		dashboard.Spec.Readonly = true
-		return common.AppendTarget(ctx, ctrl.Manifest, &dashboard)
+		return AppendTarget(ctx, ctrl.Manifest, &dashboard)
 	case strings.Contains(ctrl.VersionConfigMap, "dashboard") && !opts.ReadOnly:
 		var dashboard v1alpha1.TektonDashboard
 		dashboard.Spec.Readonly = false
-		return common.AppendTarget(ctx, ctrl.Manifest, &dashboard)
+		return AppendTarget(ctx, ctrl.Manifest, &dashboard)
 	}
 
 	return nil
 }
 
 func addProxy(manifest *mf.Manifest) error {
-	koDataDir := os.Getenv(common.KoEnvKey)
+	koDataDir := os.Getenv(KoEnvKey)
 	proxyLocation := filepath.Join(koDataDir, "webhook")
-	return common.AppendManifest(manifest, proxyLocation)
+	return AppendManifest(manifest, proxyLocation)
 }

pkg/reconciler/kubernetes/tektondashboard/controller.go

@@ -19,8 +19,6 @@ package tektondashboard
 import (
 	"context"
 
-	"github.com/tektoncd/operator/pkg/reconciler/kubernetes/initcontroller"
-
 	"k8s.io/client-go/tools/cache"
 
 	"github.com/tektoncd/operator/pkg/apis/operator/v1alpha1"
@@ -53,16 +51,16 @@ func NewExtendedController(generator common.ExtensionGenerator) injection.Contro
 		kubeClient := kubeclient.Get(ctx)
 		logger := logging.FromContext(ctx)
 
-		ctrl := initcontroller.Controller{
+		ctrl := common.Controller{
 			Logger:           logger,
 			VersionConfigMap: versionConfigMap,
 		}
 
-		readonlyManifest, dashboardVer := ctrl.InitController(ctx, initcontroller.PayloadOptions{ReadOnly: true})
+		readonlyManifest, dashboardVer := ctrl.InitController(ctx, common.PayloadOptions{ReadOnly: true})
 
-		fullaccessManifest, _ := ctrl.InitController(ctx, initcontroller.PayloadOptions{ReadOnly: false})
+		fullaccessManifest, _ := ctrl.InitController(ctx, common.PayloadOptions{ReadOnly: false})
 
-		operatorVer, err := initcontroller.OperatorVersion(ctx)
+		operatorVer, err := common.OperatorVersion(ctx)
 		if err != nil {
 			logger.Fatal(err)
 		}

pkg/reconciler/kubernetes/tektondashboard/tektondashboard.go

@@ -21,19 +21,13 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/tektoncd/operator/pkg/reconciler/kubernetes/tektoninstallerset"
-	"github.com/tektoncd/operator/pkg/reconciler/shared/hash"
-
 	mf "github.com/manifestival/manifestival"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/kubernetes"
 
 	"github.com/tektoncd/operator/pkg/apis/operator/v1alpha1"
 	clientset "github.com/tektoncd/operator/pkg/client/clientset/versioned"
 	pipelineinformer "github.com/tektoncd/operator/pkg/client/informers/externalversions/operator/v1alpha1"
 	tektondashboardreconciler "github.com/tektoncd/operator/pkg/client/injection/reconciler/operator/v1alpha1/tektondashboard"
 	"github.com/tektoncd/operator/pkg/reconciler/common"
-	"github.com/tektoncd/operator/pkg/reconciler/kubernetes/tektoninstallerset"
 	"github.com/tektoncd/operator/pkg/reconciler/shared/hash"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -95,7 +89,7 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, original *v1alpha1.Tekton
 
 	ls := metav1.LabelSelector{
 		MatchLabels: map[string]string{
-			tektoninstallerset.CreatedByKey: createdByValue,
+			v1alpha1.CreatedByKey: createdByValue,
 		},
 	}
 	labelSelector, err := common.LabelSelector(ls)
@@ -141,7 +135,9 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, td *v1alpha1.TektonDashb
 		if err.Error() == common.PipelineNotReady {
 			td.Status.MarkDependencyInstalling("tekton-pipelines is still installing")
 			// wait for pipeline status to change
-			return fmt.Errorf(common.PipelineNotReady)
+			r.enqueueAfter(td, 10*time.Second)
+			return nil
+
 		}
 		// (tektonpipeline.opeator.tekton.dev instance not available yet)
 		td.Status.MarkDependencyMissing("tekton-pipelines does not exist")
@@ -185,8 +181,8 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, td *v1alpha1.TektonDashb
 		return err
 	}
 
-	installerSetTargetNamespace := installedTIS.Annotations[tektoninstallerset.TargetNamespaceKey]
-	installerSetReleaseVersion := installedTIS.Annotations[tektoninstallerset.ReleaseVersionKey]
+	installerSetTargetNamespace := installedTIS.Annotations[v1alpha1.TargetNamespaceKey]
+	installerSetReleaseVersion := installedTIS.Annotations[v1alpha1.ReleaseVersionKey]
 
 	// Check if TargetNamespace of existing TektonInstallerSet is same as expected
 	// Check if Release Version in TektonInstallerSet is same as expected
@@ -229,7 +225,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, td *v1alpha1.TektonDashb
 		}
 
 		// spec hash stored on installerSet
-		lastAppliedHash := installedTIS.GetAnnotations()[tektoninstallerset.LastAppliedHashKey]
+		lastAppliedHash := installedTIS.GetAnnotations()[v1alpha1.LastAppliedHashKey]
 
 		if lastAppliedHash != expectedSpecHash {
 
@@ -247,7 +243,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, td *v1alpha1.TektonDashb
 
 			// Update the spec hash
 			current := installedTIS.GetAnnotations()
-			current[tektoninstallerset.LastAppliedHashKey] = expectedSpecHash
+			current[v1alpha1.LastAppliedHashKey] = expectedSpecHash
 			installedTIS.SetAnnotations(current)
 
 			// Update the manifests
@@ -364,12 +360,12 @@ func makeInstallerSet(td *v1alpha1.TektonDashboard, manifest mf.Manifest, tdSpec
 		ObjectMeta: metav1.ObjectMeta{
 			GenerateName: fmt.Sprintf("%s-", v1alpha1.DashboardResourceName),
 			Labels: map[string]string{
-				tektoninstallerset.CreatedByKey: createdByValue,
+				v1alpha1.CreatedByKey: createdByValue,
 			},
 			Annotations: map[string]string{
-				tektoninstallerset.ReleaseVersionKey:  releaseVersion,
-				tektoninstallerset.TargetNamespaceKey: td.Spec.TargetNamespace,
-				tektoninstallerset.LastAppliedHashKey: tdSpecHash,
+				v1alpha1.ReleaseVersionKey:  releaseVersion,
+				v1alpha1.TargetNamespaceKey: td.Spec.TargetNamespace,
+				v1alpha1.LastAppliedHashKey: tdSpecHash,
 			},
 			OwnerReferences: []metav1.OwnerReference{ownerRef},
 		},

pkg/reconciler/kubernetes/tektoninstallerset/install.go

@@ -21,7 +21,8 @@ import (
 	"strings"
 
 	mf "github.com/manifestival/manifestival"
-	"github.com/tektoncd/operator/pkg/reconciler/common"
+	"github.com/tektoncd/operator/pkg/apis/operator/v1alpha1"
+	"github.com/tektoncd/operator/pkg/reconciler/shared/hash"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
@@ -31,8 +32,7 @@ import (
 )
 
 const (
-	replicasForHash    = 999
-	lastAppliedHashKey = "operator.tekton.dev/last-applied-hash"
+	replicasForHash = 999
 )
 
 var (
@@ -140,7 +140,7 @@ func computeDeploymentHash(d appsv1.Deployment) (string, error) {
 	// done to the deployment spec
 	d.Spec.Replicas = ptr.Int32(replicasForHash)
 
-	return common.ComputeHashOf(d.Spec)
+	return hash.Compute(d.Spec)
 }
 
 func (i *installer) createDeployment(expected *unstructured.Unstructured) error {
@@ -159,7 +159,7 @@ func (i *installer) createDeployment(expected *unstructured.Unstructured) error
 	if len(dep.Annotations) == 0 {
 		dep.Annotations = map[string]string{}
 	}
-	dep.Annotations[lastAppliedHashKey] = hash
+	dep.Annotations[v1alpha1.LastAppliedHashKey] = hash
 
 	unstrObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(dep)
 	if err != nil {
@@ -188,7 +188,7 @@ func (i *installer) updateDeployment(existing *unstructured.Unstructured, existi
 		existingDeployment.Annotations = map[string]string{}
 	}
 
-	existingDeployment.Annotations[lastAppliedHashKey] = newHash
+	existingDeployment.Annotations[v1alpha1.LastAppliedHashKey] = newHash
 
 	unstrObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(existingDeployment)
 	if err != nil {
@@ -231,7 +231,7 @@ func (i *installer) ensureDeployment(expected *unstructured.Unstructured) error
 		return fmt.Errorf("failed to compute hash of existing deployment: %v", err)
 	}
 
-	hashFromAnnotation, hashExist := existingDeployment.Annotations[lastAppliedHashKey]
+	hashFromAnnotation, hashExist := existingDeployment.Annotations[v1alpha1.LastAppliedHashKey]
 
 	// if hash doesn't exist then update the deployment with hash
 	if !hashExist {

pkg/reconciler/kubernetes/tektoninstallerset/install_test.go

@@ -126,7 +126,7 @@ func TestInstaller(t *testing.T) {
 	// reset created array
 	client.creates = []unstructured.Unstructured{}
 
-	want = []unstructured.Unstructured{namespace, clusterRole, podSecurityPolicy, validatingWebhook, mutatingWebhook}
+	want = []unstructured.Unstructured{namespace, podSecurityPolicy, clusterRole, validatingWebhook, mutatingWebhook}
 
 	err = i.EnsureClusterScopedResources()
 	if err != nil {

pkg/reconciler/kubernetes/tektoninstallerset/query.go

@@ -64,7 +64,7 @@ func CurrentInstallerSetName(ctx context.Context, client clientset.Interface, la
 func CleanUpObsoleteResources(ctx context.Context, client clientset.Interface, createdBy string) error {
 
 	labelSelector := labels.NewSelector()
-	createdReq, _ := labels.NewRequirement(CreatedByKey, selection.Equals, []string{createdBy})
+	createdReq, _ := labels.NewRequirement(v1alpha1.CreatedByKey, selection.Equals, []string{createdBy})
 	if createdReq != nil {
 		labelSelector = labelSelector.Add(*createdReq)
 	}
@@ -81,7 +81,7 @@ func CleanUpObsoleteResources(ctx context.Context, client clientset.Interface, c
 	for _, i := range list.Items {
 		// check if installerSet has InstallerSetType label
 		// if it doesn't exist then delete it
-		if _, ok := i.Labels[InstallerSetType]; !ok {
+		if _, ok := i.Labels[v1alpha1.InstallerSetType]; !ok {
 			err := client.OperatorV1alpha1().TektonInstallerSets().Delete(ctx, i.Name, v1.DeleteOptions{})
 			if err != nil {
 				return err