Merge pull request #270 from nikhil-thomas/improve/csv-uncached-client

Simplify CSV reconciler
openshift · Nov 14, 2022 · aba8eda · aba8eda
2 parents 047e8be + d166261
commit aba8eda
Show file tree

Hide file tree

Showing 5 changed files with 26 additions and 76 deletions.
diff --git a/internal/controllers/addon/controller.go b/internal/controllers/addon/controller.go
@@ -105,6 +105,7 @@ func NewAddonReconciler(
 			// Step 4: Reconcile OLM objects
 			&olmReconciler{
 				client:          client,
+				uncachedClient:  uncachedClient,
 				scheme:          scheme,
 				csvEventHandler: csvEventHandler,
 			},

diff --git a/internal/controllers/addon/olm_reconciler.go b/internal/controllers/addon/olm_reconciler.go
@@ -19,6 +19,7 @@ const OLM_RECONCILER_NAME = "olmReconciler"
 type olmReconciler struct {
 	scheme          *runtime.Scheme
 	client          client.Client
+	uncachedClient  client.Client
 	csvEventHandler csvEventHandler
 }
 

diff --git a/internal/controllers/addon/phase_observe_csv.go b/internal/controllers/addon/phase_observe_csv.go
@@ -4,34 +4,24 @@ import (
 	"context"
 	"fmt"
 
-	"k8s.io/apimachinery/pkg/runtime/schema"
-
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-
 	operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	addonsv1alpha1 "github.com/openshift/addon-operator/apis/addons/v1alpha1"
 )
 
-const (
-	CSVGroup   string = "operators.coreos.com"
-	CSVVersion string = "v1alpha1"
-	CSVKind    string = "ClusterServiceVersion"
-)
-
 func (r *olmReconciler) observeCurrentCSV(
 	ctx context.Context,
 	addon *addonsv1alpha1.Addon,
 	csvKey client.ObjectKey,
 ) (requeueResult, error) {
-	phase, err := r.getCSVPhase(ctx, csvKey)
-	if err != nil {
-		return resultNil, fmt.Errorf("finding installed CSV phase: %w", err)
+	csv := &operatorsv1alpha1.ClusterServiceVersion{}
+	if err := r.uncachedClient.Get(ctx, csvKey, csv); err != nil {
+		return resultNil, fmt.Errorf("getting installed CSV: %w", err)
 	}
 
 	var message string
-	switch phase {
+	switch csv.Status.Phase {
 	case operatorsv1alpha1.CSVPhaseSucceeded:
 		// do nothing here
 	case operatorsv1alpha1.CSVPhaseFailed:
@@ -44,29 +34,6 @@ func (r *olmReconciler) observeCurrentCSV(
 		reportUnreadyCSV(addon, message)
 		return resultRetry, nil
 	}
-	return resultNil, nil
-}
 
-func (r *olmReconciler) getCSVPhase(
-	ctx context.Context,
-	csvKey client.ObjectKey,
-) (operatorsv1alpha1.ClusterServiceVersionPhase, error) {
-	csv := &unstructured.Unstructured{}
-	gvk := schema.GroupVersionKind{
-		Group:   CSVGroup,
-		Version: CSVVersion,
-		Kind:    CSVKind,
-	}
-	csv.SetGroupVersionKind(gvk)
-	if err := r.client.Get(ctx, csvKey, csv); err != nil {
-		return "", fmt.Errorf("getting CSV: %w", err)
-	}
-	phase, ok, err := unstructured.NestedString(csv.Object, "status", "phase")
-	if err != nil {
-		return "", fmt.Errorf("getting csv.status.phase: %w", err)
-	}
-	if !ok {
-		return "", nil
-	}
-	return operatorsv1alpha1.ClusterServiceVersionPhase(phase), nil
+	return resultNil, nil
 }
diff --git a/internal/controllers/addon/phase_observe_csv_test.go b/internal/controllers/addon/phase_observe_csv_test.go
@@ -6,13 +6,9 @@ import (
 	"testing"
 
 	operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
-
-	"github.com/stretchr/testify/require"
-
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -27,29 +23,20 @@ func TestObserveCurrentCSV(t *testing.T) {
 	}
 
 	for name, tc := range map[string]struct {
-		CSV      *unstructured.Unstructured
+		CSV      *operatorsv1alpha1.ClusterServiceVersion
 		expected Expected
 	}{
 		"No CSV present": {
-			CSV: &unstructured.Unstructured{
-				Object: map[string]interface{}{
-					"apiversion": fmt.Sprintf("%s/%s", CSVGroup, CSVVersion),
-					"kind":       CSVKind,
-				},
-			},
+			CSV: &operatorsv1alpha1.ClusterServiceVersion{},
 			expected: Expected{
 				Conditions: []metav1.Condition{unreadyCSVCondition("unkown/pending")},
 				Result:     resultRetry,
 			},
 		},
 		"Phase failed": {
-			CSV: &unstructured.Unstructured{
-				Object: map[string]interface{}{
-					"apiversion": fmt.Sprintf("%s/%s", CSVGroup, CSVVersion),
-					"kind":       CSVKind,
-					"status": map[string]interface{}{
-						"phase": string(operatorsv1alpha1.CSVPhaseFailed),
-					},
+			CSV: &operatorsv1alpha1.ClusterServiceVersion{
+				Status: operatorsv1alpha1.ClusterServiceVersionStatus{
+					Phase: operatorsv1alpha1.CSVPhaseFailed,
 				},
 			},
 			expected: Expected{
@@ -58,13 +45,9 @@ func TestObserveCurrentCSV(t *testing.T) {
 			},
 		},
 		"Phase succeded": {
-			CSV: &unstructured.Unstructured{
-				Object: map[string]interface{}{
-					"apiversion": fmt.Sprintf("%s/%s", CSVGroup, CSVVersion),
-					"kind":       CSVKind,
-					"status": map[string]interface{}{
-						"phase": string(operatorsv1alpha1.CSVPhaseSucceeded),
-					},
+			CSV: &operatorsv1alpha1.ClusterServiceVersion{
+				Status: operatorsv1alpha1.ClusterServiceVersionStatus{
+					Phase: operatorsv1alpha1.CSVPhaseSucceeded,
 				},
 			},
 			expected: Expected{
@@ -81,16 +64,16 @@ func TestObserveCurrentCSV(t *testing.T) {
 				On("Get",
 					mock.Anything,
 					mock.IsType(client.ObjectKey{}),
-					testutil.IsUnstructuredUnstructuredPtr,
+					testutil.IsOperatorsV1Alpha1ClusterServiceVersionPtr,
 				).
 				Run(func(args mock.Arguments) {
-					tc.CSV.DeepCopyInto(args.Get(2).(*unstructured.Unstructured))
+					tc.CSV.DeepCopyInto(args.Get(2).(*operatorsv1alpha1.ClusterServiceVersion))
 				}).
 				Return(nil)
 
 			r := &olmReconciler{
-				client: c,
-				scheme: testutil.NewTestSchemeWithAddonsv1alpha1(),
+				uncachedClient: c,
+				scheme:         testutil.NewTestSchemeWithAddonsv1alpha1(),
 			}
 			var addon addonsv1alpha1.Addon
 			res, err := r.observeCurrentCSV(context.Background(), &addon, client.ObjectKey{})

diff --git a/internal/testutil/matchers.go b/internal/testutil/matchers.go
@@ -3,8 +3,6 @@ package testutil
 import (
 	"context"
 
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-
 	operatorsv1 "github.com/operator-framework/api/pkg/operators/v1"
 	operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
 	monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
@@ -26,9 +24,10 @@ var (
 	IsNetworkingV1NetworkPolicyPtr = mock.IsType(&networkingv1.NetworkPolicy{})
 
 	// olm
-	IsOperatorsV1OperatorGroupPtr       = mock.IsType(&operatorsv1.OperatorGroup{})
-	IsOperatorsV1Alpha1CatalogSourcePtr = mock.IsType(&operatorsv1alpha1.CatalogSource{})
-	IsOperatorsV1Alpha1SubscriptionPtr  = mock.IsType(&operatorsv1alpha1.Subscription{})
+	IsOperatorsV1OperatorGroupPtr               = mock.IsType(&operatorsv1.OperatorGroup{})
+	IsOperatorsV1Alpha1CatalogSourcePtr         = mock.IsType(&operatorsv1alpha1.CatalogSource{})
+	IsOperatorsV1Alpha1ClusterServiceVersionPtr = mock.IsType(&operatorsv1alpha1.ClusterServiceVersion{})
+	IsOperatorsV1Alpha1SubscriptionPtr          = mock.IsType(&operatorsv1alpha1.Subscription{})
 
 	// prom
 	IsMonitoringV1ServiceMonitorPtr = mock.IsType(&monitoringv1.ServiceMonitor{})
@@ -40,7 +39,6 @@ var (
 	IsAddonsv1alpha1AddonOperatorListPtr = mock.IsType(&addonsv1alpha1.AddonOperatorList{})
 
 	// misc
-	IsContext                     = mock.IsType(context.TODO())
-	IsObjectKey                   = mock.IsType(client.ObjectKey{})
-	IsUnstructuredUnstructuredPtr = mock.IsType(&unstructured.Unstructured{})
+	IsContext   = mock.IsType(context.TODO())
+	IsObjectKey = mock.IsType(client.ObjectKey{})
 )