From 8bd06b114cd4faf7b0214cfd54e8850733599116 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Tue, 26 Mar 2024 13:31:26 +0100 Subject: [PATCH 1/2] controllers: reuse the context from reconcile We dont need to create a new context for each time for a reconcile we should use the ctx from the Reconcile for all the dependent operations. Signed-off-by: Madhu Rajanna --- controllers/clusterversion_controller.go | 19 +++++----- controllers/delete.go | 7 ++-- controllers/delete_test.go | 7 ++-- controllers/quickstarts.go | 10 +++--- controllers/quickstarts_test.go | 5 ++- controllers/storagecluster_controller.go | 10 +++--- controllers/storagesystem_controller.go | 12 ++++--- controllers/storagesystem_controller_fake.go | 2 ++ controllers/subscription_controller.go | 21 +++++------ controllers/subscriptions.go | 38 ++++++++++---------- controllers/subscriptions_test.go | 8 ++--- controllers/vendors.go | 5 ++- controllers/vendors_test.go | 4 +-- pkg/util/openshift.go | 4 +-- 14 files changed, 77 insertions(+), 75 deletions(-) diff --git a/controllers/clusterversion_controller.go b/controllers/clusterversion_controller.go index 3a4073847..99fc9c3df 100644 --- a/controllers/clusterversion_controller.go +++ b/controllers/clusterversion_controller.go @@ -37,6 +37,7 @@ import ( // ClusterVersionReconciler reconciles a ClusterVersion object type ClusterVersionReconciler struct { + ctx context.Context client.Client Scheme *runtime.Scheme ConsolePort int @@ -53,9 +54,10 @@ type ClusterVersionReconciler struct { // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.8.3/pkg/reconcile func (r *ClusterVersionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - logger := log.FromContext(ctx) + r.ctx = ctx + logger := log.FromContext(r.ctx) instance := configv1.ClusterVersion{} - if err := r.Client.Get(context.TODO(), req.NamespacedName, &instance); err != nil { + if err := r.Client.Get(r.ctx, req.NamespacedName, &instance); err != nil { return ctrl.Result{}, err } if err := r.ensureConsolePlugin(instance.Status.Desired.Version); err != nil { @@ -68,8 +70,9 @@ func (r *ClusterVersionReconciler) Reconcile(ctx context.Context, req ctrl.Reque // SetupWithManager sets up the controller with the Manager. func (r *ClusterVersionReconciler) SetupWithManager(mgr ctrl.Manager) error { + r.ctx = context.TODO() err := mgr.Add(manager.RunnableFunc(func(context.Context) error { - clusterVersion, err := util.DetermineOpenShiftVersion(r.Client) + clusterVersion, err := util.DetermineOpenShiftVersion(r.ctx, r.Client) if err != nil { return err } @@ -86,14 +89,14 @@ func (r *ClusterVersionReconciler) SetupWithManager(mgr ctrl.Manager) error { } func (r *ClusterVersionReconciler) ensureConsolePlugin(clusterVersion string) error { - logger := log.FromContext(context.TODO()) + logger := log.FromContext(r.ctx) // The base path to where the request are sent basePath := console.GetBasePath(clusterVersion) nginxConf := console.GetNginxConfiguration() // Get ODF console Deployment odfConsoleDeployment := console.GetDeployment(OperatorNamespace) - err := r.Client.Get(context.TODO(), types.NamespacedName{ + err := r.Client.Get(r.ctx, types.NamespacedName{ Name: odfConsoleDeployment.Name, Namespace: odfConsoleDeployment.Namespace, }, odfConsoleDeployment) @@ -103,7 +106,7 @@ func (r *ClusterVersionReconciler) ensureConsolePlugin(clusterVersion string) er // Create/Update ODF console ConfigMap (nginx configuration) odfConsoleConfigMap := console.GetNginxConfConfigMap(OperatorNamespace) - _, err = controllerutil.CreateOrUpdate(context.TODO(), r.Client, odfConsoleConfigMap, func() error { + _, err = controllerutil.CreateOrUpdate(r.ctx, r.Client, odfConsoleConfigMap, func() error { if odfConsoleConfigMapData := odfConsoleConfigMap.Data["nginx.conf"]; odfConsoleConfigMapData != nginxConf { logger.Info(fmt.Sprintf("Set the ConfigMap odf-console-nginx-conf data as '%s'", nginxConf)) odfConsoleConfigMap.Data["nginx.conf"] = nginxConf @@ -116,7 +119,7 @@ func (r *ClusterVersionReconciler) ensureConsolePlugin(clusterVersion string) er // Create/Update ODF console Service odfConsoleService := console.GetService(r.ConsolePort, OperatorNamespace) - _, err = controllerutil.CreateOrUpdate(context.TODO(), r.Client, odfConsoleService, func() error { + _, err = controllerutil.CreateOrUpdate(r.ctx, r.Client, odfConsoleService, func() error { return controllerutil.SetControllerReference(odfConsoleDeployment, odfConsoleService, r.Scheme) }) if err != nil && !errors.IsAlreadyExists(err) { @@ -125,7 +128,7 @@ func (r *ClusterVersionReconciler) ensureConsolePlugin(clusterVersion string) er // Create/Update ODF console ConsolePlugin odfConsolePlugin := console.GetConsolePluginCR(r.ConsolePort, OperatorNamespace) - _, err = controllerutil.CreateOrUpdate(context.TODO(), r.Client, odfConsolePlugin, func() error { + _, err = controllerutil.CreateOrUpdate(r.ctx, r.Client, odfConsolePlugin, func() error { if currentBasePath := odfConsolePlugin.Spec.Service.BasePath; currentBasePath != basePath { logger.Info(fmt.Sprintf("Set the BasePath for odf-console plugin as '%s'", basePath)) odfConsolePlugin.Spec.Service.BasePath = basePath diff --git a/controllers/delete.go b/controllers/delete.go index 81871bcc7..ac54422f5 100644 --- a/controllers/delete.go +++ b/controllers/delete.go @@ -17,7 +17,6 @@ limitations under the License. package controllers import ( - "context" "fmt" "github.com/go-logr/logr" @@ -41,7 +40,7 @@ func (r *StorageSystemReconciler) deleteResources(instance *odfv1alpha1.StorageS backendStorage = &ibmv1alpha1.FlashSystemCluster{} } - err := r.Client.Get(context.TODO(), types.NamespacedName{ + err := r.Client.Get(r.ctx, types.NamespacedName{ Name: instance.Spec.Name, Namespace: instance.Spec.Namespace}, backendStorage) @@ -53,7 +52,7 @@ func (r *StorageSystemReconciler) deleteResources(instance *odfv1alpha1.StorageS return err } - err = r.Client.Delete(context.TODO(), backendStorage) + err = r.Client.Delete(r.ctx, backendStorage) if err != nil { logger.Error(err, "Failed to delete", "Kind", instance.Spec.Kind, "Name", instance.Spec.Name) return err @@ -67,7 +66,7 @@ func (r *StorageSystemReconciler) deleteResources(instance *odfv1alpha1.StorageS func (r *StorageSystemReconciler) areAllStorageSystemsMarkedForDeletion(namespace string) (bool, error) { var storageSystems odfv1alpha1.StorageSystemList - err := r.Client.List(context.TODO(), &storageSystems, &client.ListOptions{Namespace: namespace}) + err := r.Client.List(r.ctx, &storageSystems, &client.ListOptions{Namespace: namespace}) if err != nil { return false, err } diff --git a/controllers/delete_test.go b/controllers/delete_test.go index d458699f6..d6be71b0d 100644 --- a/controllers/delete_test.go +++ b/controllers/delete_test.go @@ -67,6 +67,7 @@ func TestDeleteResources(t *testing.T) { } for i, tc := range testCases { + ctx := context.TODO() t.Logf("Case %d: %s\n", i+1, tc.label) fakeStorageSystem := GetFakeStorageSystem(tc.kind) @@ -83,7 +84,7 @@ func TestDeleteResources(t *testing.T) { fakeStorageSystem.Spec.Name = vendorSystem.GetName() fakeStorageSystem.Spec.Namespace = vendorSystem.GetNamespace() - err = fakeReconciler.Client.Create(context.TODO(), vendorSystem) + err = fakeReconciler.Client.Create(ctx, vendorSystem) assert.NoError(t, err) } @@ -93,14 +94,14 @@ func TestDeleteResources(t *testing.T) { // verify resource does not exist if tc.kind == VendorStorageCluster() { storageCluster := &ocsv1.StorageCluster{} - err = fakeReconciler.Client.Get(context.TODO(), types.NamespacedName{ + err = fakeReconciler.Client.Get(ctx, types.NamespacedName{ Name: fakeStorageSystem.Spec.Name, Namespace: fakeStorageSystem.Namespace}, storageCluster) assert.True(t, errors.IsNotFound(err)) } else if tc.kind == VendorFlashSystemCluster() { flashSystemCluster := &ibmv1alpha1.FlashSystemCluster{} - err = fakeReconciler.Client.Get(context.TODO(), types.NamespacedName{ + err = fakeReconciler.Client.Get(ctx, types.NamespacedName{ Name: fakeStorageSystem.Spec.Name, Namespace: fakeStorageSystem.Namespace}, flashSystemCluster) assert.True(t, errors.IsNotFound(err)) diff --git a/controllers/quickstarts.go b/controllers/quickstarts.go index 1f74ec6e4..c51aea03c 100644 --- a/controllers/quickstarts.go +++ b/controllers/quickstarts.go @@ -17,8 +17,6 @@ limitations under the License. package controllers import ( - "context" - "github.com/ghodss/yaml" "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/api/errors" @@ -41,10 +39,10 @@ func (r *StorageSystemReconciler) ensureQuickStarts(logger logr.Logger) error { continue } found := consolev1.ConsoleQuickStart{} - err = r.Client.Get(context.TODO(), types.NamespacedName{Name: cqs.Name, Namespace: cqs.Namespace}, &found) + err = r.Client.Get(r.ctx, types.NamespacedName{Name: cqs.Name, Namespace: cqs.Namespace}, &found) if err != nil { if errors.IsNotFound(err) { - err = r.Client.Create(context.TODO(), &cqs) + err = r.Client.Create(r.ctx, &cqs) if err != nil { logger.Error(err, "Failed to create quickstart", "Name", cqs.Name, "Namespace", cqs.Namespace) return nil @@ -56,7 +54,7 @@ func (r *StorageSystemReconciler) ensureQuickStarts(logger logr.Logger) error { return nil } found.Spec = cqs.Spec - err = r.Client.Update(context.TODO(), &found) + err = r.Client.Update(r.ctx, &found) if err != nil { logger.Error(err, "Failed to update quickstart", "Name", cqs.Name, "Namespace", cqs.Namespace) return nil @@ -90,7 +88,7 @@ func (r *StorageSystemReconciler) deleteQuickStarts(logger logr.Logger, instance continue } - err = r.Client.Delete(context.TODO(), &cqs) + err = r.Client.Delete(r.ctx, &cqs) if err != nil { if errors.IsNotFound(err) { continue diff --git a/controllers/quickstarts_test.go b/controllers/quickstarts_test.go index 76246794c..1dc315da2 100644 --- a/controllers/quickstarts_test.go +++ b/controllers/quickstarts_test.go @@ -140,7 +140,6 @@ func TestDeleteQuickStarts(t *testing.T) { for i, tc := range testCases { t.Logf("Case %d: %s\n", i+1, tc.label) - fakeReconciler := GetFakeStorageSystemReconciler(t) err := fakeReconciler.ensureQuickStarts(fakeReconciler.Log) @@ -150,11 +149,11 @@ func TestDeleteQuickStarts(t *testing.T) { assert.Equal(t, 2, len(quickstarts)) for i := range tc.createStorageSystems { - err = fakeReconciler.Client.Create(context.TODO(), &tc.createStorageSystems[i]) + err = fakeReconciler.Client.Create(fakeReconciler.ctx, &tc.createStorageSystems[i]) assert.NoError(t, err) } for i := range tc.deleteStorageSystems { - err := fakeReconciler.Client.Delete(context.TODO(), &tc.deleteStorageSystems[i]) + err := fakeReconciler.Client.Delete(fakeReconciler.ctx, &tc.deleteStorageSystems[i]) assert.NoError(t, err) err = fakeReconciler.deleteResources(&tc.deleteStorageSystems[i], fakeReconciler.Log) assert.NoError(t, err) diff --git a/controllers/storagecluster_controller.go b/controllers/storagecluster_controller.go index 38dce09f6..2034837e2 100644 --- a/controllers/storagecluster_controller.go +++ b/controllers/storagecluster_controller.go @@ -41,6 +41,7 @@ const ( // StorageClusterReconciler reconciles a StorageCluster object type StorageClusterReconciler struct { + ctx context.Context client.Client Scheme *runtime.Scheme Recorder *EventReporter @@ -54,10 +55,11 @@ type StorageClusterReconciler struct { // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.8.3/pkg/reconcile func (r *StorageClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - logger := log.FromContext(ctx) + r.ctx = ctx + logger := log.FromContext(r.ctx) instance := &ocsv1.StorageCluster{} - err := r.Client.Get(context.TODO(), req.NamespacedName, instance) + err := r.Client.Get(r.ctx, req.NamespacedName, instance) if err != nil { if errors.IsNotFound(err) { logger.Info("StorageCluster instance not found.") @@ -71,7 +73,7 @@ func (r *StorageClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reque // get list of StorageSystems storageSystemList := &odfv1alpha1.StorageSystemList{} - err = r.Client.List(context.TODO(), storageSystemList, &client.ListOptions{Namespace: instance.Namespace}) + err = r.Client.List(r.ctx, storageSystemList, &client.ListOptions{Namespace: instance.Namespace}) if err != nil { logger.Error(err, "Failed to list the StorageSystem.") return ctrl.Result{}, err @@ -94,7 +96,7 @@ func (r *StorageClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reque } // create StorageSystem for storageCluster - err = r.Client.Create(context.TODO(), storageSystem) + err = r.Client.Create(r.ctx, storageSystem) if err != nil { logger.Error(err, "Failed to create StorageSystem.", "StorageSystem", storageSystem.Name) return ctrl.Result{}, err diff --git a/controllers/storagesystem_controller.go b/controllers/storagesystem_controller.go index 3c3571a9c..7295301f3 100644 --- a/controllers/storagesystem_controller.go +++ b/controllers/storagesystem_controller.go @@ -46,6 +46,7 @@ const ( // StorageSystemReconciler reconciles a StorageSystem object type StorageSystemReconciler struct { client.Client + ctx context.Context Log logr.Logger Scheme *runtime.Scheme Recorder *EventReporter @@ -66,10 +67,11 @@ type StorageSystemReconciler struct { // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.8.3/pkg/reconcile func (r *StorageSystemReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + r.ctx = ctx logger := r.Log.WithValues("instance", req.NamespacedName) instance := &odfv1alpha1.StorageSystem{} - err := r.Client.Get(context.TODO(), req.NamespacedName, instance) + err := r.Client.Get(r.ctx, req.NamespacedName, instance) if err != nil { if errors.IsNotFound(err) { logger.Info("storagesystem instance not found") @@ -87,7 +89,7 @@ func (r *StorageSystemReconciler) Reconcile(ctx context.Context, req ctrl.Reques result, reconcileError := r.reconcile(instance, logger) // Apply status changes - statusError := r.Client.Status().Update(context.TODO(), instance) + statusError := r.Client.Status().Update(r.ctx, instance) if statusError != nil { logger.Error(statusError, "failed to update status") } @@ -178,7 +180,7 @@ func (r *StorageSystemReconciler) updateStorageSystem(instance *odfv1alpha1.Stor // save the status locally before the Update call, as update call does not update the status and we lost it instanceStatus := instance.Status.DeepCopy() - err := r.Client.Update(context.TODO(), instance) + err := r.Client.Update(r.ctx, instance) instance.Status = *(instanceStatus) return err } @@ -207,7 +209,7 @@ func (r *StorageSystemReconciler) ensureSubscriptions(instance *odfv1alpha1.Stor } for _, desiredSubscription := range subs { - err = EnsureDesiredSubscription(r.Client, desiredSubscription) + err = EnsureDesiredSubscription(r.ctx, r.Client, desiredSubscription) if err != nil && !errors.IsAlreadyExists(err) { logger.Error(err, "failed to ensure subscription", "Subscription", desiredSubscription.Name) return err @@ -224,7 +226,7 @@ func (r *StorageSystemReconciler) isVendorCsvReady(instance *odfv1alpha1.Storage var returnErr error for _, csvName := range csvNames { - csvObj, err := EnsureVendorCsv(r.Client, csvName) + csvObj, err := EnsureVendorCsv(r.ctx, r.Client, csvName) if err != nil { logger.Error(err, "failed to validate CSV", "ClusterServiceVersion", csvName) multierr.AppendInto(&returnErr, err) diff --git a/controllers/storagesystem_controller_fake.go b/controllers/storagesystem_controller_fake.go index 2ba896088..bea2c8fe7 100644 --- a/controllers/storagesystem_controller_fake.go +++ b/controllers/storagesystem_controller_fake.go @@ -17,6 +17,7 @@ limitations under the License. package controllers import ( + "context" "testing" "github.com/stretchr/testify/assert" @@ -52,6 +53,7 @@ func GetFakeStorageSystemReconciler(t *testing.T, objs ...runtime.Object) *Stora scheme := createFakeScheme(t) fakeStorageSystemReconciler := &StorageSystemReconciler{ + ctx: context.TODO(), Client: fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(objs...).Build(), Log: ctrl.Log.WithName("controllers").WithName("StorageSystem"), Scheme: scheme, diff --git a/controllers/subscription_controller.go b/controllers/subscription_controller.go index 694872aff..3cb8db2ac 100644 --- a/controllers/subscription_controller.go +++ b/controllers/subscription_controller.go @@ -45,6 +45,7 @@ import ( // SubscriptionReconciler reconciles a Subscription object type SubscriptionReconciler struct { client.Client + ctx context.Context Scheme *runtime.Scheme Recorder *EventReporter ConditionName string @@ -61,11 +62,11 @@ type SubscriptionReconciler struct { // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.8.3/pkg/reconcile func (r *SubscriptionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - - logger := log.FromContext(ctx) + r.ctx = ctx + logger := log.FromContext(r.ctx) instance := &operatorv1alpha1.Subscription{} - err := r.Client.Get(context.TODO(), req.NamespacedName, instance) + err := r.Client.Get(r.ctx, req.NamespacedName, instance) if err != nil { if errors.IsNotFound(err) { logger.Info("Subscription instance not found.") @@ -96,7 +97,7 @@ func (r *SubscriptionReconciler) ensureSubscriptions(logger logr.Logger, namespa subsList[StorageClusterKind] = GetSubscriptions(StorageClusterKind) ssList := &odfv1alpha1.StorageSystemList{} - err = r.Client.List(context.TODO(), ssList, &client.ListOptions{Namespace: namespace}) + err = r.Client.List(r.ctx, ssList, &client.ListOptions{Namespace: namespace}) if err != nil { return err } @@ -107,7 +108,7 @@ func (r *SubscriptionReconciler) ensureSubscriptions(logger logr.Logger, namespa for _, subs := range subsList { for _, sub := range subs { - errSub := EnsureDesiredSubscription(r.Client, sub) + errSub := EnsureDesiredSubscription(r.ctx, r.Client, sub) if errSub != nil { logger.Error(errSub, "failed to ensure Subscription", "Subscription", sub.Name) err = fmt.Errorf("failed to ensure Subscriptions") @@ -122,7 +123,7 @@ func (r *SubscriptionReconciler) ensureSubscriptions(logger logr.Logger, namespa for kind := range subsList { for _, csvName := range GetVendorCsvNames(kind) { - _, csvErr := EnsureVendorCsv(r.Client, csvName) + _, csvErr := EnsureVendorCsv(r.ctx, r.Client, csvName) if csvErr != nil { multierr.AppendInto(&err, csvErr) } @@ -134,7 +135,7 @@ func (r *SubscriptionReconciler) ensureSubscriptions(logger logr.Logger, namespa func (r *SubscriptionReconciler) setOperatorCondition(logger logr.Logger, namespace string) error { ocdList := &operatorv2.OperatorConditionList{} - err := r.Client.List(context.TODO(), ocdList, client.InNamespace(namespace)) + err := r.Client.List(r.ctx, ocdList, client.InNamespace(namespace)) if err != nil { logger.Error(err, "failed to list OperatorConditions") return err @@ -159,7 +160,7 @@ func (r *SubscriptionReconciler) setOperatorCondition(logger logr.Logger, namesp // operator is not upgradeable msg := fmt.Sprintf("%s:%s", ocd.GetName(), cond.Message) logger.Info("setting operator upgradeable status", "status", cond.Status) - return r.OperatorCondition.Set(context.TODO(), cond.Status, + return r.OperatorCondition.Set(r.ctx, cond.Status, conditions.WithReason(cond.Reason), conditions.WithMessage(msg)) } } @@ -167,7 +168,7 @@ func (r *SubscriptionReconciler) setOperatorCondition(logger logr.Logger, namesp // all operators are upgradeable status := metav1.ConditionTrue logger.Info("setting operator upgradeable status", "status", status) - return r.OperatorCondition.Set(context.TODO(), status, + return r.OperatorCondition.Set(r.ctx, status, conditions.WithReason("Dependents"), conditions.WithMessage("No dependent reports not upgradeable status")) } @@ -266,7 +267,7 @@ func (r *SubscriptionReconciler) SetupWithManager(mgr ctrl.Manager) error { return []reconcile.Request{} } logger := log.FromContext(ctx) - sub, err := GetOdfSubscription(r.Client) + sub, err := GetOdfSubscription(ctx, r.Client) if err != nil { logger.Error(err, "failed to get ODF Subscription") return []reconcile.Request{} diff --git a/controllers/subscriptions.go b/controllers/subscriptions.go index 1d998f666..97dee4308 100644 --- a/controllers/subscriptions.go +++ b/controllers/subscriptions.go @@ -40,9 +40,9 @@ import ( // // NOTE(jarrpa): We can't use client.MatchingFields to limit the list results // because fake.Client does not support them. -func CheckExistingSubscriptions(cli client.Client, desiredSubscription *operatorv1alpha1.Subscription) (*operatorv1alpha1.Subscription, error) { +func CheckExistingSubscriptions(ctx context.Context, cli client.Client, desiredSubscription *operatorv1alpha1.Subscription) (*operatorv1alpha1.Subscription, error) { - odfSub, err := GetOdfSubscription(cli) + odfSub, err := GetOdfSubscription(ctx, cli) if err != nil { return nil, err } @@ -52,7 +52,7 @@ func CheckExistingSubscriptions(cli client.Client, desiredSubscription *operator } subsList := &operatorv1alpha1.SubscriptionList{} - err = cli.List(context.TODO(), subsList, &client.ListOptions{Namespace: desiredSubscription.Namespace}) + err = cli.List(ctx, subsList, &client.ListOptions{Namespace: desiredSubscription.Namespace}) if err != nil { return nil, err } @@ -160,11 +160,11 @@ func getMergedEnvVars(envList1, envList2 []corev1.EnvVar) []corev1.EnvVar { return updatedEnvVars } -func EnsureDesiredSubscription(cli client.Client, desiredSubscription *operatorv1alpha1.Subscription) error { +func EnsureDesiredSubscription(ctx context.Context, cli client.Client, desiredSubscription *operatorv1alpha1.Subscription) error { var err error - desiredSubscription, err = CheckExistingSubscriptions(cli, desiredSubscription) + desiredSubscription, err = CheckExistingSubscriptions(ctx, cli, desiredSubscription) if err != nil { return err } @@ -172,9 +172,9 @@ func EnsureDesiredSubscription(cli client.Client, desiredSubscription *operatorv // create/update subscription sub := &operatorv1alpha1.Subscription{} sub.ObjectMeta = desiredSubscription.ObjectMeta - _, err = controllerutil.CreateOrUpdate(context.TODO(), cli, sub, func() error { + _, err = controllerutil.CreateOrUpdate(ctx, cli, sub, func() error { sub.Spec = desiredSubscription.Spec - return SetOdfSubControllerReference(cli, sub) + return SetOdfSubControllerReference(ctx, cli, sub) }) if err != nil && !errors.IsAlreadyExists(err) { return err @@ -183,9 +183,9 @@ func EnsureDesiredSubscription(cli client.Client, desiredSubscription *operatorv return nil } -func SetOdfSubControllerReference(cli client.Client, obj client.Object) error { +func SetOdfSubControllerReference(ctx context.Context, cli client.Client, obj client.Object) error { - odfSub, err := GetOdfSubscription(cli) + odfSub, err := GetOdfSubscription(ctx, cli) if err != nil { return err } @@ -199,10 +199,10 @@ func SetOdfSubControllerReference(cli client.Client, obj client.Object) error { } // GetOdfSubscription returns the subscription for odf-operator. -func GetOdfSubscription(cli client.Client) (*operatorv1alpha1.Subscription, error) { +func GetOdfSubscription(ctx context.Context, cli client.Client) (*operatorv1alpha1.Subscription, error) { subsList := &operatorv1alpha1.SubscriptionList{} - err := cli.List(context.TODO(), subsList, &client.ListOptions{Namespace: OperatorNamespace}) + err := cli.List(ctx, subsList, &client.ListOptions{Namespace: OperatorNamespace}) if err != nil { return nil, err } @@ -230,25 +230,25 @@ func GetVendorCsvNames(kind odfv1alpha1.StorageKind) []string { return csvNames } -func EnsureVendorCsv(cli client.Client, csvName string) (*operatorv1alpha1.ClusterServiceVersion, error) { +func EnsureVendorCsv(ctx context.Context, cli client.Client, csvName string) (*operatorv1alpha1.ClusterServiceVersion, error) { var err error csvObj := &operatorv1alpha1.ClusterServiceVersion{} - err = cli.Get(context.TODO(), types.NamespacedName{ + err = cli.Get(ctx, types.NamespacedName{ Name: csvName, Namespace: OperatorNamespace}, csvObj) if err != nil { if errors.IsNotFound(err) { - approvalErr := ApproveInstallPlanForCsv(cli, csvName) + approvalErr := ApproveInstallPlanForCsv(ctx, cli, csvName) if approvalErr != nil { return nil, approvalErr } } return nil, err } - _, err = controllerutil.CreateOrUpdate(context.TODO(), cli, csvObj, func() error { + _, err = controllerutil.CreateOrUpdate(ctx, cli, csvObj, func() error { csvObj.OwnerReferences = []metav1.OwnerReference{} - return SetOdfSubControllerReference(cli, csvObj) + return SetOdfSubControllerReference(ctx, cli, csvObj) }) if err != nil && !errors.IsAlreadyExists(err) { return nil, err @@ -267,13 +267,13 @@ func EnsureVendorCsv(cli client.Client, csvName string) (*operatorv1alpha1.Clust // ApproveInstallPlanForCsv approve the manual approval installPlan for the given CSV // and returns an error if none found -func ApproveInstallPlanForCsv(cli client.Client, csvName string) error { +func ApproveInstallPlanForCsv(ctx context.Context, cli client.Client, csvName string) error { var finalError error var foundInstallPlan bool installPlans := &operatorv1alpha1.InstallPlanList{} - err := cli.List(context.TODO(), installPlans, &client.ListOptions{Namespace: OperatorNamespace}) + err := cli.List(ctx, installPlans, &client.ListOptions{Namespace: OperatorNamespace}) if err != nil { return err @@ -286,7 +286,7 @@ func ApproveInstallPlanForCsv(cli client.Client, csvName string) error { !installPlan.Spec.Approved { installPlans.Items[i].Spec.Approved = true - err = cli.Update(context.TODO(), &installPlans.Items[i]) + err = cli.Update(ctx, &installPlans.Items[i]) if err != nil { multierr.AppendInto(&finalError, fmt.Errorf( "Failed to approve installplan %s", installPlan.Name)) diff --git a/controllers/subscriptions_test.go b/controllers/subscriptions_test.go index 9a0975323..b0273bfbb 100644 --- a/controllers/subscriptions_test.go +++ b/controllers/subscriptions_test.go @@ -17,7 +17,6 @@ limitations under the License. package controllers import ( - "context" "testing" "github.com/stretchr/testify/assert" @@ -51,7 +50,6 @@ func TestEnsureSubscription(t *testing.T) { for _, kind := range KnownKinds { var err error - fakeStorageSystem := GetFakeStorageSystem(kind) fakeReconciler := GetFakeStorageSystemReconciler(t, fakeStorageSystem) subs := GetSubscriptions(kind) @@ -60,7 +58,7 @@ func TestEnsureSubscription(t *testing.T) { for _, subscription := range subs { sub := subscription.DeepCopy() sub.Spec.Channel = "fake-channel" - err = fakeReconciler.Client.Create(context.TODO(), sub) + err = fakeReconciler.Client.Create(fakeReconciler.ctx, sub) assert.NoError(t, err) } } @@ -85,7 +83,7 @@ func TestEnsureSubscription(t *testing.T) { }, }, } - err = fakeReconciler.Client.Create(context.TODO(), odfSub) + err = fakeReconciler.Client.Create(fakeReconciler.ctx, odfSub) assert.NoError(t, err) err = fakeReconciler.ensureSubscriptions(fakeStorageSystem, fakeReconciler.Log) @@ -101,7 +99,7 @@ func TestEnsureSubscription(t *testing.T) { } actualSubscription := &operatorv1alpha1.Subscription{} - err = fakeReconciler.Client.Get(context.TODO(), types.NamespacedName{Name: expectedSubscription.Name, Namespace: expectedSubscription.Namespace}, actualSubscription) + err = fakeReconciler.Client.Get(fakeReconciler.ctx, types.NamespacedName{Name: expectedSubscription.Name, Namespace: expectedSubscription.Namespace}, actualSubscription) assert.NoError(t, err) assert.Equal(t, expectedSubscription.Spec, actualSubscription.Spec) diff --git a/controllers/vendors.go b/controllers/vendors.go index 65cc23358..74b57a87b 100644 --- a/controllers/vendors.go +++ b/controllers/vendors.go @@ -17,7 +17,6 @@ limitations under the License. package controllers import ( - "context" "reflect" "strings" @@ -69,11 +68,11 @@ func (r *StorageSystemReconciler) isVendorSystemPresent(instance *odfv1alpha1.St vendorSystem = &ibmv1alpha1.FlashSystemCluster{} } - err := r.Client.Get(context.TODO(), types.NamespacedName{Name: instance.Spec.Name, Namespace: instance.Spec.Namespace}, vendorSystem) + err := r.Client.Get(r.ctx, types.NamespacedName{Name: instance.Spec.Name, Namespace: instance.Spec.Namespace}, vendorSystem) if err == nil { logger.Info("Vendor system found", "Name", instance.Spec.Name) SetVendorSystemPresentCondition(&instance.Status.Conditions, corev1.ConditionTrue, "Found", "") - _, err = controllerutil.CreateOrUpdate(context.TODO(), r.Client, vendorSystem, func() error { + _, err = controllerutil.CreateOrUpdate(r.ctx, r.Client, vendorSystem, func() error { return controllerutil.SetOwnerReference(instance, vendorSystem, r.Scheme) }) if err != nil && !errors.IsAlreadyExists(err) { diff --git a/controllers/vendors_test.go b/controllers/vendors_test.go index 39c9d69b0..4adec6f86 100644 --- a/controllers/vendors_test.go +++ b/controllers/vendors_test.go @@ -17,7 +17,6 @@ limitations under the License. package controllers import ( - "context" "testing" "github.com/stretchr/testify/assert" @@ -51,7 +50,6 @@ func TestIsVendorSystemPresent(t *testing.T) { for i, tc := range testCases { t.Logf("Case %d: %s\n", i+1, tc.label) - fakeStorageSystem := GetFakeStorageSystem(StorageClusterKind) fakeReconciler := GetFakeStorageSystemReconciler(t, fakeStorageSystem) @@ -62,7 +60,7 @@ func TestIsVendorSystemPresent(t *testing.T) { Namespace: fakeStorageSystem.Spec.Namespace, }, } - err := fakeReconciler.Client.Create(context.TODO(), storageCluster) + err := fakeReconciler.Client.Create(fakeReconciler.ctx, storageCluster) assert.NoError(t, err) } diff --git a/pkg/util/openshift.go b/pkg/util/openshift.go index 2fb74bd83..5af14a6da 100644 --- a/pkg/util/openshift.go +++ b/pkg/util/openshift.go @@ -25,10 +25,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -func DetermineOpenShiftVersion(client client.Client) (string, error) { +func DetermineOpenShiftVersion(ctx context.Context, client client.Client) (string, error) { // Determine ocp version clusterVersionList := configv1.ClusterVersionList{} - if err := client.List(context.TODO(), &clusterVersionList); err != nil { + if err := client.List(ctx, &clusterVersionList); err != nil { return "", err } clusterVersion := "" From f7711698894760ea719c8382dea69aa4f2dfcfbf Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Tue, 26 Mar 2024 16:01:56 +0100 Subject: [PATCH 2/2] test: log complete status for CSV failure If the CSV is in Failed state, log/return the complete status field which helps in understanding what went wrong. Signed-off-by: Madhu Rajanna --- pkg/deploymanager/olm.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/deploymanager/olm.go b/pkg/deploymanager/olm.go index a6e5115d1..27f20a4f0 100644 --- a/pkg/deploymanager/olm.go +++ b/pkg/deploymanager/olm.go @@ -229,7 +229,9 @@ func (d *DeployManager) WaitForCsv(csv *operatorsv1alpha1.ClusterServiceVersion) return false, nil } if existingcsv.Status.Phase != operatorsv1alpha1.CSVPhaseSucceeded { - lastReason = fmt.Sprintf("waiting for CSV to succeed, but stuck in %s phase", existingcsv.Status.Phase) + // add complete status to lastReason which helps to understand what + // went wrong in testing. + lastReason = fmt.Sprintf("waiting for CSV to succeed, but stuck in %s phase with status:%+v", existingcsv.Status.Phase, existingcsv.Status) return false, nil } return true, nil