refactor: get platform calls (#1251)

* refactor: get platform calls - move check on platform into top of dsc and dsci reconcile to reduce duplicated calls - add logger when operator start to show which platform is running on - test: add testcase for GetPlatform() --------- Signed-off-by: Wen Zhou <[email protected]>
opendatahub-io · Sep 20, 2024 · d73b3da · d73b3da
1 parent 5d51806
commit d73b3da
Show file tree

Hide file tree

Showing 7 changed files with 127 additions and 35 deletions.
diff --git a/controllers/datasciencecluster/datasciencecluster_controller.go b/controllers/datasciencecluster/datasciencecluster_controller.go
@@ -85,6 +85,13 @@ const (
 func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { //nolint:maintidx,gocyclo
 	r.Log.Info("Reconciling DataScienceCluster resources", "Request.Name", req.Name)
 
+	// Get platform
+	platform, err := cluster.GetPlatform(ctx, r.Client)
+	if err != nil {
+		r.Log.Error(err, "Failed to determine platform")
+		return ctrl.Result{}, err
+	}
+
 	// Get information on version
 	currentOperatorReleaseVersion, err := cluster.GetRelease(ctx, r.Client)
 	if err != nil {
@@ -103,7 +110,7 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R
 		// For additional cleanup logic use operatorUninstall function.
 		// Return and don't requeue
 		if upgrade.HasDeleteConfigMap(ctx, r.Client) {
-			if uninstallErr := upgrade.OperatorUninstall(ctx, r.Client); uninstallErr != nil {
+			if uninstallErr := upgrade.OperatorUninstall(ctx, r.Client, platform); uninstallErr != nil {
 				return ctrl.Result{}, fmt.Errorf("error while operator uninstall: %w", uninstallErr)
 			}
 		}
@@ -241,7 +248,7 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R
 	var componentErrors *multierror.Error
 
 	for _, component := range allComponents {
-		if instance, err = r.reconcileSubComponent(ctx, instance, component); err != nil {
+		if instance, err = r.reconcileSubComponent(ctx, instance, platform, component); err != nil {
 			componentErrors = multierror.Append(componentErrors, err)
 		}
 	}
@@ -285,7 +292,7 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R
 }
 
 func (r *DataScienceClusterReconciler) reconcileSubComponent(ctx context.Context, instance *dscv1.DataScienceCluster,
-	component components.ComponentInterface,
+	platform cluster.Platform, component components.ComponentInterface,
 ) (*dscv1.DataScienceCluster, error) {
 	componentName := component.GetComponentName()
 
@@ -308,13 +315,7 @@ func (r *DataScienceClusterReconciler) reconcileSubComponent(ctx context.Context
 		}
 	}
 	// Reconcile component
-	// Get platform
-	platform, err := cluster.GetPlatform(ctx, r.Client)
-	if err != nil {
-		r.Log.Error(err, "Failed to determine platform")
-		return instance, err
-	}
-	err = component.ReconcileComponent(ctx, r.Client, r.Log, instance, r.DataScienceCluster.DSCISpec, platform, installedComponentValue)
+	err := component.ReconcileComponent(ctx, r.Client, r.Log, instance, r.DataScienceCluster.DSCISpec, platform, installedComponentValue)
 
 	// TODO: replace this hack with a full refactor of component status in the future
 

diff --git a/controllers/dscinitialization/dscinitialization_controller.go b/controllers/dscinitialization/dscinitialization_controller.go
@@ -153,9 +153,17 @@ func (r *DSCInitializationReconciler) Reconcile(ctx context.Context, req ctrl.Re
 		}
 	}
 
+	// Get platform
+	platform, err := cluster.GetPlatform(ctx, r.Client)
+	if err != nil {
+		r.Log.Error(err, "Failed to determine platform")
+
+		return reconcile.Result{}, err
+	}
+
 	// Check namespace is not exist, then create
 	namespace := instance.Spec.ApplicationsNamespace
-	err = r.createOdhNamespace(ctx, instance, namespace)
+	err = r.createOdhNamespace(ctx, instance, namespace, platform)
 	if err != nil {
 		// no need to log error as it was already logged in createOdhNamespace
 		return reconcile.Result{}, err
@@ -167,14 +175,6 @@ func (r *DSCInitializationReconciler) Reconcile(ctx context.Context, req ctrl.Re
 	}
 	managementStateChangeTrustedCA = false
 
-	// Get platform
-	platform, err := cluster.GetPlatform(ctx, r.Client)
-	if err != nil {
-		r.Log.Error(err, "Failed to determine platform (managed vs self-managed)")
-
-		return reconcile.Result{}, err
-	}
-
 	switch req.Name {
 	case "prometheus": // prometheus configmap
 		if instance.Spec.Monitoring.ManagementState == operatorv1.Managed && platform == cluster.ManagedRhods {

diff --git a/controllers/dscinitialization/utils.go b/controllers/dscinitialization/utils.go
@@ -36,7 +36,7 @@ var (
 // - ConfigMap  'odh-common-config'
 // - Network Policies 'opendatahub' that allow traffic between the ODH namespaces
 // - RoleBinding 'opendatahub'.
-func (r *DSCInitializationReconciler) createOdhNamespace(ctx context.Context, dscInit *dsciv1.DSCInitialization, name string) error {
+func (r *DSCInitializationReconciler) createOdhNamespace(ctx context.Context, dscInit *dsciv1.DSCInitialization, name string, platform cluster.Platform) error {
 	// Expected application namespace for the given name
 	desiredNamespace := &corev1.Namespace{
 		ObjectMeta: metav1.ObjectMeta{
@@ -118,7 +118,7 @@ func (r *DSCInitializationReconciler) createOdhNamespace(ctx context.Context, ds
 	}
 
 	// Create default NetworkPolicy for the namespace
-	err = r.reconcileDefaultNetworkPolicy(ctx, name, dscInit)
+	err = r.reconcileDefaultNetworkPolicy(ctx, name, dscInit, platform)
 	if err != nil {
 		r.Log.Error(err, "error reconciling network policy ", "name", name)
 		return err
@@ -190,14 +190,10 @@ func (r *DSCInitializationReconciler) createDefaultRoleBinding(ctx context.Conte
 	return nil
 }
 
-func (r *DSCInitializationReconciler) reconcileDefaultNetworkPolicy(ctx context.Context, name string, dscInit *dsciv1.DSCInitialization) error {
-	platform, err := cluster.GetPlatform(ctx, r.Client)
-	if err != nil {
-		return err
-	}
+func (r *DSCInitializationReconciler) reconcileDefaultNetworkPolicy(ctx context.Context, name string, dscInit *dsciv1.DSCInitialization, platform cluster.Platform) error {
 	if platform == cluster.ManagedRhods || platform == cluster.SelfManagedRhods {
 		// Deploy networkpolicy for operator namespace
-		err = deploy.DeployManifestsFromPath(ctx, r.Client, dscInit, networkpolicyPath+"/operator", "redhat-ods-operator", "networkpolicy", true)
+		err := deploy.DeployManifestsFromPath(ctx, r.Client, dscInit, networkpolicyPath+"/operator", "redhat-ods-operator", "networkpolicy", true)
 		if err != nil {
 			r.Log.Error(err, "error to set networkpolicy in operator namespace", "path", networkpolicyPath)
 			return err
@@ -287,7 +283,7 @@ func (r *DSCInitializationReconciler) reconcileDefaultNetworkPolicy(ctx context.
 		// Create NetworkPolicy if it doesn't exist
 		foundNetworkPolicy := &networkingv1.NetworkPolicy{}
 		justCreated := false
-		err = r.Client.Get(ctx, client.ObjectKey{
+		err := r.Client.Get(ctx, client.ObjectKey{
 			Name:      name,
 			Namespace: name,
 		}, foundNetworkPolicy)

diff --git a/main.go b/main.go
@@ -149,6 +149,7 @@ func main() { //nolint:funlen,maintidx
 		setupLog.Error(err, "error getting platform")
 		os.Exit(1)
 	}
+	setupLog.Info("running on", "platform", platform)
 
 	secretCache := createSecretCacheConfig(platform)
 	deploymentCache := createDeploymentCacheConfig(platform)

diff --git a/pkg/cluster/cluster_operations_int_test.go b/pkg/cluster/cluster_operations_int_test.go
@@ -4,6 +4,8 @@ import (
 	"context"
 	"time"
 
+	ofapiv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
+	ofapiv2 "github.com/operator-framework/api/pkg/operators/v2"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	ctrlruntime "sigs.k8s.io/controller-runtime/pkg/client"
@@ -92,6 +94,89 @@ var _ = Describe("Creating cluster resources", func() {
 
 	})
 
+	Context("Platform detection", func() {
+		var objectCleaner *envtestutil.Cleaner
+		operatorNS := "redhat-ods-operator"
+
+		BeforeEach(func(ctx context.Context) {
+			objectCleaner = envtestutil.CreateCleaner(envTestClient, envTest.Config, timeout, interval)
+		})
+
+		It("should run as unknown", func(ctx context.Context) {
+			// given nothing
+			// when
+			platform, err := cluster.GetPlatform(ctx, envTestClient)
+			Expect(err).ToNot(HaveOccurred())
+
+			// then
+			Expect(platform).To(Equal(cluster.Unknown))
+
+		})
+
+		It("should run as managed platform", func(ctx context.Context) {
+			// give catalogsource exists in operator namespace
+			_, errNs := cluster.CreateNamespace(ctx, envTestClient, operatorNS)
+			Expect(errNs).ToNot(HaveOccurred())
+
+			catalogSource := &ofapiv1alpha1.CatalogSource{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "addon-managed-odh-catalog",
+					Namespace: operatorNS,
+				},
+			}
+			Expect(envTestClient.Create(ctx, catalogSource)).To(Succeed())
+
+			// when
+			platform, err := cluster.GetPlatform(ctx, envTestClient)
+			Expect(err).ToNot(HaveOccurred())
+			defer objectCleaner.DeleteAll(ctx, catalogSource)
+
+			// then
+			Expect(platform).To(Equal(cluster.ManagedRhods))
+
+		})
+
+		It("should run as self-managed platform", func(ctx context.Context) {
+			// given rhoai operatorcondition exist
+			operatorCondition := &ofapiv2.OperatorCondition{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "rhods-operator-something",
+					Namespace: operatorNS,
+				},
+			}
+			Expect(envTestClient.Create(ctx, operatorCondition)).To(Succeed())
+
+			// when
+			platform, err := cluster.GetPlatform(ctx, envTestClient)
+			Expect(err).ToNot(HaveOccurred())
+			defer objectCleaner.DeleteAll(ctx, operatorCondition)
+
+			// then
+			Expect(platform).To(Equal(cluster.SelfManagedRhods))
+
+		})
+
+		It("should run as ODH platform", func(ctx context.Context) {
+			// given odh operatorcondition exist
+			operatorCondition := &ofapiv2.OperatorCondition{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "opendatahub-operator-something",
+					Namespace: operatorNS,
+				},
+			}
+			Expect(envTestClient.Create(ctx, operatorCondition)).To(Succeed())
+
+			// when
+			platform, err := cluster.GetPlatform(ctx, envTestClient)
+			Expect(err).ToNot(HaveOccurred())
+			defer objectCleaner.DeleteAll(ctx, operatorCondition)
+
+			// then
+			Expect(platform).To(Equal(cluster.OpenDataHub))
+
+		})
+	})
+
 	Context("config map manipulation", func() {
 
 		var (

diff --git a/pkg/cluster/cluster_operations_suite_int_test.go b/pkg/cluster/cluster_operations_suite_int_test.go
@@ -1,8 +1,11 @@
 package cluster_test
 
 import (
+	"path/filepath"
 	"testing"
 
+	ofapiv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
+	ofapiv2 "github.com/operator-framework/api/pkg/operators/v2"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
@@ -34,8 +37,19 @@ var _ = BeforeSuite(func() {
 	By("Bootstrapping k8s test environment")
 
 	utilruntime.Must(corev1.AddToScheme(testScheme))
+	utilruntime.Must(ofapiv1alpha1.AddToScheme(testScheme))
+	utilruntime.Must(ofapiv2.AddToScheme(testScheme))
 
-	envTest = &envtest.Environment{}
+	envTest = &envtest.Environment{
+		CRDInstallOptions: envtest.CRDInstallOptions{
+			Scheme: testScheme,
+			Paths: []string{
+				filepath.Join("..", "..", "config", "crd", "external"),
+			},
+			ErrorIfPathMissing: true,
+			CleanUpAfterUse:    false,
+		},
+	}
 
 	config, err := envTest.Start()
 	Expect(err).NotTo(HaveOccurred())

diff --git a/pkg/upgrade/uninstallation.go b/pkg/upgrade/uninstallation.go
@@ -24,12 +24,7 @@ const (
 
 // OperatorUninstall deletes all the externally generated resources.
 // This includes DSCI, namespace created by operator (but not workbench or MR's), subscription and CSV.
-func OperatorUninstall(ctx context.Context, cli client.Client) error {
-	platform, err := cluster.GetPlatform(ctx, cli)
-	if err != nil {
-		return err
-	}
-
+func OperatorUninstall(ctx context.Context, cli client.Client, platform cluster.Platform) error {
 	if err := removeDSCInitialization(ctx, cli); err != nil {
 		return err
 	}