Replace polling based logic in reconcilers with requeue

Replace polling based logic in reconcilers with requeues

Improve Unit tests

For components:

- Addons
- Dashboard
- Pipeline
- Triggers

Signed-off-by: Nikhil Thomas <nikthoma@redhat.com>

pkg/reconciler/common/common.go

@@ -19,16 +19,13 @@ package common
 import (
 	"context"
 	"fmt"
-	"time"
 
 	"github.com/tektoncd/operator/pkg/apis/operator/v1alpha1"
 	informer "github.com/tektoncd/operator/pkg/client/informers/externalversions/operator/v1alpha1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 )
 
 var (
-	Interval = 10 * time.Second
-	Timeout  = 1 * time.Minute
 	// DefaultSA is the default service account
 	DefaultSA = "pipeline"
 )

pkg/reconciler/kubernetes/tektonconfig/extension.go

@@ -55,15 +55,15 @@ func (oe kubernetesExtension) PostReconcile(ctx context.Context, comp v1alpha1.T
 	}
 
 	if configInstance.Spec.Profile == v1alpha1.ProfileLite || configInstance.Spec.Profile == v1alpha1.ProfileBasic {
-		return extension.TektonDashboardCRDelete(ctx, oe.operatorClientSet.OperatorV1alpha1().TektonDashboards(), v1alpha1.DashboardResourceName)
+		return extension.EnsureTektonDashboardCRNotExists(ctx, oe.operatorClientSet.OperatorV1alpha1().TektonDashboards())
 	}
 
 	return nil
 }
 func (oe kubernetesExtension) Finalize(ctx context.Context, comp v1alpha1.TektonComponent) error {
 	configInstance := comp.(*v1alpha1.TektonConfig)
 	if configInstance.Spec.Profile == v1alpha1.ProfileAll {
-		return extension.TektonDashboardCRDelete(ctx, oe.operatorClientSet.OperatorV1alpha1().TektonDashboards(), v1alpha1.DashboardResourceName)
+		return extension.EnsureTektonDashboardCRNotExists(ctx, oe.operatorClientSet.OperatorV1alpha1().TektonDashboards())
 	}
 	return nil
 }

pkg/reconciler/kubernetes/tektonconfig/extension/dashboard.go

@@ -18,7 +18,6 @@ package extension
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"reflect"
 
@@ -27,7 +26,6 @@ import (
 	"github.com/tektoncd/operator/pkg/reconciler/common"
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/wait"
 )
 
 func EnsureTektonDashboardExists(ctx context.Context, clients op.TektonDashboardInterface, config *v1alpha1.TektonConfig) (*v1alpha1.TektonDashboard, error) {
@@ -108,7 +106,11 @@ func updateDashboard(ctx context.Context, tdCR *v1alpha1.TektonDashboard, config
 	}
 
 	if updated {
-		return clients.Update(ctx, tdCR, metav1.UpdateOptions{})
+		_, err := clients.Update(ctx, tdCR, metav1.UpdateOptions{})
+		if err != nil {
+			return nil, err
+		}
+		return nil, v1alpha1.RECONCILE_AGAIN_ERR
 	}
 
 	return tdCR, nil
@@ -126,37 +128,26 @@ func isTektonDashboardReady(s *v1alpha1.TektonDashboard, err error) (bool, error
 	return s.Status.IsReady(), err
 }
 
-// TektonDashboardCRDelete deletes tha TektonDashboard to see if all resources will be deleted
-func TektonDashboardCRDelete(ctx context.Context, clients op.TektonDashboardInterface, name string) error {
+// EnsureTektonDashboardCRNotExists deletes the singleton instance of TektonDashboard
+// and ensures the instance is removed checking whether in exists in a subsequent invocation
+func EnsureTektonDashboardCRNotExists(ctx context.Context, clients op.TektonDashboardInterface) error {
 	if _, err := GetDashboard(ctx, clients, v1alpha1.DashboardResourceName); err != nil {
 		if apierrs.IsNotFound(err) {
+			// TektonDashBoard CR is gone, hence return nil
 			return nil
 		}
 		return err
 	}
-	if err := clients.Delete(ctx, name, metav1.DeleteOptions{}); err != nil {
-		return fmt.Errorf("TektonDashboard %q failed to delete: %v", name, err)
-	}
-	err := wait.PollImmediate(common.Interval, common.Timeout, func() (bool, error) {
-		_, err := clients.Get(ctx, name, metav1.GetOptions{})
+	// if the Get was successful, try deleting the CR
+	if err := clients.Delete(ctx, v1alpha1.DashboardResourceName, metav1.DeleteOptions{}); err != nil {
 		if apierrs.IsNotFound(err) {
-			return true, nil
+			// TektonDashBoard CR is gone, hence return nil
+			return nil
 		}
-		return false, err
-	})
-	if err != nil {
-		return fmt.Errorf("Timed out waiting on TektonDashboard to delete %v", err)
-	}
-	return verifyNoTektonDashboardCR(ctx, clients)
-}
-
-func verifyNoTektonDashboardCR(ctx context.Context, clients op.TektonDashboardInterface) error {
-	dashboards, err := clients.List(ctx, metav1.ListOptions{})
-	if err != nil {
-		return err
-	}
-	if len(dashboards.Items) > 0 {
-		return errors.New("Unable to verify cluster-scoped resources are deleted if any TektonDashboard exists")
+		return fmt.Errorf("TektonDashboard %q failed to delete: %v", v1alpha1.DashboardResourceName, err)
 	}
-	return nil
+	// if the Delete API call was success,
+	// then return requeue_event
+	// so that in a subsequent reconcile call the absence of the CR is verified by one of the 2 checks above
+	return v1alpha1.RECONCILE_AGAIN_ERR
 }

pkg/reconciler/kubernetes/tektonconfig/extension/dashboard_test.go

@@ -17,8 +17,12 @@ limitations under the License.
 package extension
 
 import (
+	"context"
+	"os"
 	"testing"
 
+	op "github.com/tektoncd/operator/pkg/client/clientset/versioned/typed/operator/v1alpha1"
+
 	"github.com/tektoncd/operator/pkg/apis/operator/v1alpha1"
 	"github.com/tektoncd/operator/pkg/client/injection/client/fake"
 	util "github.com/tektoncd/operator/pkg/reconciler/common/testing"
@@ -27,53 +31,97 @@ import (
 	ts "knative.dev/pkg/reconciler/testing"
 )
 
-func TestTektonDashboardCreateAndDeleteCR(t *testing.T) {
+func TestEnsureTektonDashbordExists(t *testing.T) {
 	ctx, _, _ := ts.SetupFakeContextWithCancel(t)
 	c := fake.Get(ctx)
 	tConfig := pipeline.GetTektonConfig()
+
+	// first invocation should create instance as it is non-existent and return RECONCILE_AGAIN_ERR
 	_, err := EnsureTektonDashboardExists(ctx, c.OperatorV1alpha1().TektonDashboards(), tConfig)
-	util.AssertNotEqual(t, err, nil)
-	err = TektonDashboardCRDelete(ctx, c.OperatorV1alpha1().TektonDashboards(), v1alpha1.DashboardResourceName)
+	util.AssertEqual(t, err, v1alpha1.RECONCILE_AGAIN_ERR)
+
+	// during second invocation instance exists but waiting on dependencies (pipeline, triggers)
+	// hence returns DEPENDENCY_UPGRADE_PENDING_ERR
+	_, err = EnsureTektonDashboardExists(ctx, c.OperatorV1alpha1().TektonDashboards(), tConfig)
+	util.AssertEqual(t, err, v1alpha1.DEPENDENCY_UPGRADE_PENDING_ERR)
+
+	// make upgrade checks pass
+	makeUpgradeCheckPass(t, ctx, c.OperatorV1alpha1().TektonDashboards())
+
+	// next invocation should return RECONCILE_AGAIN_ERR as Dashboard is waiting for installation (prereconcile, postreconcile, installersets...)
+	_, err = EnsureTektonDashboardExists(ctx, c.OperatorV1alpha1().TektonDashboards(), tConfig)
+	util.AssertEqual(t, err, v1alpha1.RECONCILE_AGAIN_ERR)
+
+	// mark the instance ready
+	markDashboardsReady(t, ctx, c.OperatorV1alpha1().TektonDashboards())
+
+	// next invocation should return nil error as the instance is ready
+	_, err = EnsureTektonDashboardExists(ctx, c.OperatorV1alpha1().TektonDashboards(), tConfig)
+	util.AssertEqual(t, err, nil)
+
+	// test update propagation from tektonConfig
+	tConfig.Spec.TargetNamespace = "foobar"
+	_, err = EnsureTektonDashboardExists(ctx, c.OperatorV1alpha1().TektonDashboards(), tConfig)
+	util.AssertEqual(t, err, v1alpha1.RECONCILE_AGAIN_ERR)
+
+	_, err = EnsureTektonDashboardExists(ctx, c.OperatorV1alpha1().TektonDashboards(), tConfig)
 	util.AssertEqual(t, err, nil)
 }
 
-func TestTektonDashboardUpdate(t *testing.T) {
+func TestEnsureTektonDashboardCRNotExists(t *testing.T) {
 	ctx, _, _ := ts.SetupFakeContextWithCancel(t)
 	c := fake.Get(ctx)
-	tConfig := pipeline.GetTektonConfig()
-	_, err := createDashboard(ctx, c.OperatorV1alpha1().TektonDashboards(), tConfig)
+
+	// when no instance exists, nil error is returned immediately
+	err := EnsureTektonDashboardCRNotExists(ctx, c.OperatorV1alpha1().TektonDashboards())
 	util.AssertEqual(t, err, nil)
-	// to update dashboard instance
-	tConfig = &v1alpha1.TektonConfig{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: v1alpha1.ConfigResourceName,
-		},
-		Spec: v1alpha1.TektonConfigSpec{
-			Profile: "all",
-			CommonSpec: v1alpha1.CommonSpec{
-				TargetNamespace: "tekton-pipelines1",
-			},
-			Dashboard: v1alpha1.Dashboard{
-				DashboardProperties: v1alpha1.DashboardProperties{
-					Readonly: false,
-				},
-			},
-			Config: v1alpha1.Config{
-				NodeSelector: map[string]string{
-					"key": "value",
-				},
-			},
-		},
-	}
+
+	// create an instance for testing other cases
+	tConfig := pipeline.GetTektonConfig()
 	_, err = EnsureTektonDashboardExists(ctx, c.OperatorV1alpha1().TektonDashboards(), tConfig)
-	util.AssertNotEqual(t, err, nil)
-	err = TektonDashboardCRDelete(ctx, c.OperatorV1alpha1().TektonDashboards(), v1alpha1.DashboardResourceName)
+	util.AssertEqual(t, err, v1alpha1.RECONCILE_AGAIN_ERR)
+
+	// when an instance exists the first invoacation should make the delete API call and
+	// return RECONCILE_AGAI_ERROR. So that the deletion can be confirmed in a subsequent invocation
+	err = EnsureTektonDashboardCRNotExists(ctx, c.OperatorV1alpha1().TektonDashboards())
+	util.AssertEqual(t, err, v1alpha1.RECONCILE_AGAIN_ERR)
+
+	// when the instance is completely removed from a cluster, the function should return nil error
+	err = EnsureTektonDashboardCRNotExists(ctx, c.OperatorV1alpha1().TektonDashboards())
 	util.AssertEqual(t, err, nil)
 }
 
-func TestTektonDashboardCRDelete(t *testing.T) {
-	ctx, _, _ := ts.SetupFakeContextWithCancel(t)
-	c := fake.Get(ctx)
-	err := TektonDashboardCRDelete(ctx, c.OperatorV1alpha1().TektonDashboards(), v1alpha1.DashboardResourceName)
+func markDashboardsReady(t *testing.T, ctx context.Context, c op.TektonDashboardInterface) {
+	t.Helper()
+	td, err := c.Get(ctx, v1alpha1.DashboardResourceName, metav1.GetOptions{})
+	util.AssertEqual(t, err, nil)
+	td.Status.MarkDependenciesInstalled()
+	td.Status.MarkPreReconcilerComplete()
+	td.Status.MarkInstallerSetAvailable()
+	td.Status.MarkInstallerSetReady()
+	td.Status.MarkPostReconcilerComplete()
+	_, err = c.UpdateStatus(ctx, td, metav1.UpdateOptions{})
 	util.AssertEqual(t, err, nil)
 }
+
+func makeUpgradeCheckPass(t *testing.T, ctx context.Context, c op.TektonDashboardInterface) {
+	t.Helper()
+	// set necessary version labels to make upgrade check pass
+	dashboard, err := c.Get(ctx, v1alpha1.DashboardResourceName, metav1.GetOptions{})
+	util.AssertEqual(t, err, nil)
+	setDummyVersionLabel(dashboard)
+	_, err = c.Update(ctx, dashboard, metav1.UpdateOptions{})
+	util.AssertEqual(t, err, nil)
+}
+
+func setDummyVersionLabel(td *v1alpha1.TektonDashboard) {
+	oprVersion := "v1.2.3"
+	os.Setenv(v1alpha1.VersionEnvKey, oprVersion)
+
+	labels := td.GetLabels()
+	if labels == nil {
+		labels = map[string]string{}
+	}
+	labels[v1alpha1.ReleaseVersionKey] = oprVersion
+	td.SetLabels(labels)
+}

pkg/reconciler/openshift/tektonconfig/extension.go

@@ -108,15 +108,15 @@ func (oe openshiftExtension) PostReconcile(ctx context.Context, comp v1alpha1.Te
 	}
 
 	if configInstance.Spec.Profile == v1alpha1.ProfileLite || configInstance.Spec.Profile == v1alpha1.ProfileBasic {
-		return extension.TektonAddonCRDelete(ctx, oe.operatorClientSet.OperatorV1alpha1().TektonAddons(), v1alpha1.AddonResourceName)
+		return extension.EnsureTektonAddonCRNotExists(ctx, oe.operatorClientSet.OperatorV1alpha1().TektonAddons())
 	}
 
 	return nil
 }
 func (oe openshiftExtension) Finalize(ctx context.Context, comp v1alpha1.TektonComponent) error {
 	configInstance := comp.(*v1alpha1.TektonConfig)
 	if configInstance.Spec.Profile == v1alpha1.ProfileAll {
-		if err := extension.TektonAddonCRDelete(ctx, oe.operatorClientSet.OperatorV1alpha1().TektonAddons(), v1alpha1.AddonResourceName); err != nil {
+		if err := extension.EnsureTektonAddonCRNotExists(ctx, oe.operatorClientSet.OperatorV1alpha1().TektonAddons()); err != nil {
 			return err
 		}
 	}

pkg/reconciler/openshift/tektonconfig/extension/addon.go

@@ -18,7 +18,6 @@ package extension
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"reflect"
 
@@ -27,7 +26,6 @@ import (
 	"github.com/tektoncd/operator/pkg/reconciler/common"
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/wait"
 )
 
 func EnsureTektonAddonExists(ctx context.Context, clients op.TektonAddonInterface, config *v1alpha1.TektonConfig) (*v1alpha1.TektonAddon, error) {
@@ -114,7 +112,11 @@ func updateAddon(ctx context.Context, taCR *v1alpha1.TektonAddon, config *v1alph
 	}
 
 	if updated {
-		return clients.Update(ctx, taCR, metav1.UpdateOptions{})
+		_, err := clients.Update(ctx, taCR, metav1.UpdateOptions{})
+		if err != nil {
+			return nil, err
+		}
+		return nil, v1alpha1.RECONCILE_AGAIN_ERR
 	}
 
 	return taCR, nil
@@ -132,37 +134,24 @@ func isTektonAddonReady(s *v1alpha1.TektonAddon, err error) (bool, error) {
 	return s.Status.IsReady(), err
 }
 
-// TektonAddonCRDelete deletes tha TektonAddon to see if all resources will be deleted
-func TektonAddonCRDelete(ctx context.Context, clients op.TektonAddonInterface, name string) error {
+func EnsureTektonAddonCRNotExists(ctx context.Context, clients op.TektonAddonInterface) error {
 	if _, err := GetAddon(ctx, clients, v1alpha1.AddonResourceName); err != nil {
 		if apierrs.IsNotFound(err) {
+			// TektonAddon CR is gone, hence return nil
 			return nil
 		}
 		return err
 	}
-	if err := clients.Delete(ctx, name, metav1.DeleteOptions{}); err != nil {
-		return fmt.Errorf("TektonAddon %q failed to delete: %v", name, err)
-	}
-	err := wait.PollImmediate(common.Interval, common.Timeout, func() (bool, error) {
-		_, err := clients.Get(ctx, name, metav1.GetOptions{})
+	// if the Get was successful, try deleting the CR
+	if err := clients.Delete(ctx, v1alpha1.AddonResourceName, metav1.DeleteOptions{}); err != nil {
 		if apierrs.IsNotFound(err) {
-			return true, nil
+			// TektonAddon CR is gone, hence return nil
+			return nil
 		}
-		return false, err
-	})
-	if err != nil {
-		return fmt.Errorf("Timed out waiting on TektonAddon to delete %v", err)
-	}
-	return verifyNoTektonAddonCR(ctx, clients)
-}
-
-func verifyNoTektonAddonCR(ctx context.Context, clients op.TektonAddonInterface) error {
-	addons, err := clients.List(ctx, metav1.ListOptions{})
-	if err != nil {
-		return err
-	}
-	if len(addons.Items) > 0 {
-		return errors.New("Unable to verify cluster-scoped resources are deleted if any TektonAddon exists")
+		return fmt.Errorf("TektonAddon %q failed to delete: %v", v1alpha1.AddonResourceName, err)
 	}
-	return nil
+	// if the Delete API call was success,
+	// then return requeue_event
+	// so that in a subsequent reconcile call the absence of the CR is verified by one of the 2 checks above
+	return v1alpha1.RECONCILE_AGAIN_ERR
 }