From 0b803104349c16b6a2eceb6ae9cebf73b4e3134a Mon Sep 17 00:00:00 2001 From: yifeijin Date: Wed, 2 Mar 2022 10:50:47 -0500 Subject: [PATCH 1/3] handle error improvement --- controllers/csm_controller.go | 11 ++++++----- controllers/csm_controller_test.go | 8 +++----- pkg/drivers/commonconfig.go | 6 +++--- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/controllers/csm_controller.go b/controllers/csm_controller.go index d3d872370..4e32c52fe 100644 --- a/controllers/csm_controller.go +++ b/controllers/csm_controller.go @@ -195,7 +195,6 @@ func (r *ContainerStorageModuleReconciler) Reconcile(ctx context.Context, req ct return ctrl.Result{}, fmt.Errorf("error when adding finalizer: %v", err) } r.EventRecorder.Event(csm, corev1.EventTypeNormal, v1alpha1.EventUpdated, "Object finalizer is added") - return ctrl.Result{}, nil } oldStatus := csm.GetCSMStatus() @@ -604,8 +603,8 @@ func (r *ContainerStorageModuleReconciler) removeDriver(ctx context.Context, ins return err } else { log.Infow("Deleting object", "Name:", name, "Kind:", kind) - err = r.GetClient().Delete(ctx, obj) - if err != nil { + err = r.Delete(ctx, obj) + if err != nil && !k8serror.IsNotFound(err) { return err } } @@ -667,8 +666,9 @@ func (r *ContainerStorageModuleReconciler) removeDriver(ctx context.Context, ins daemonsetObj := &appsv1.DaemonSet{} err = r.Get(ctx, daemonsetKey, daemonsetObj) if err == nil { - if err = r.Delete(ctx, daemonsetObj); err != nil { + if err = r.Delete(ctx, daemonsetObj); err != nil && !k8serror.IsNotFound(err) { log.Errorw("error delete daemonset", "Error", err.Error()) + return err } } else { log.Infow("error getting daemonset", "daemonsetKey", daemonsetKey) @@ -681,8 +681,9 @@ func (r *ContainerStorageModuleReconciler) removeDriver(ctx context.Context, ins deploymentObj := &appsv1.Deployment{} if err = r.Get(ctx, deploymentKey, deploymentObj); err == nil { - if err = r.Delete(ctx, deploymentObj); err != nil { + if err = r.Delete(ctx, deploymentObj); err != nil && !k8serror.IsNotFound(err) { log.Errorw("error delete deployment", "Error", err.Error()) + return err } } else { log.Infow("error getting deployment", "deploymentKey", deploymentKey) diff --git a/controllers/csm_controller_test.go b/controllers/csm_controller_test.go index 903034010..529449152 100644 --- a/controllers/csm_controller_test.go +++ b/controllers/csm_controller_test.go @@ -160,8 +160,6 @@ func (suite *CSMControllerTestSuite) TestErrorInjection() { suite.runFakeCSMManager("", true) // make a csm without finalizer suite.makeFakeCSM(csmName, suite.namespace, false) - // reconcile adds finalizer - suite.runFakeCSMManager("", true) suite.reconcileWithErrorInjection(csmName, "") } @@ -219,9 +217,8 @@ func (suite *CSMControllerTestSuite) TestRemoveDriver() { {"get CM error", csm, &getCMError, getCMErrorStr}, {"get Driver error", csm, &getCSIError, getCSIErrorStr}, {"delete SA error", csm, &deleteSAError, deleteSAErrorStr}, - // code only logs the error when delete fails. No error returned - {"delete Daemonset error", csm, &deleteDSError, ""}, - {"delete Deployment error", csm, &deleteDeploymentError, ""}, + {"delete Daemonset error", csm, &deleteDSError, deleteDSErrorStr}, + {"delete Deployment error", csm, &deleteDeploymentError, deleteDeploymentErrorStr}, } for _, tt := range removeDriverTests { @@ -238,6 +235,7 @@ func (suite *CSMControllerTestSuite) TestRemoveDriver() { if tt.expectedErr == "" { assert.Nil(t, err) } else { + assert.Error(t, err) assert.Containsf(t, err.Error(), tt.expectedErr, "expected error containing %q, got %s", tt.expectedErr, err) } diff --git a/pkg/drivers/commonconfig.go b/pkg/drivers/commonconfig.go index da27d5811..fe1c2890c 100644 --- a/pkg/drivers/commonconfig.go +++ b/pkg/drivers/commonconfig.go @@ -77,13 +77,13 @@ func GetController(ctx context.Context, cr csmv1.ContainerStorageModule, operato for _, s := range cr.Spec.Driver.SideCars { if s.Name == *c.Name { if s.Enabled == nil { - log.Info("Container to be enabled : %s\n", *c.Name) + log.Infow("Container to be enabled : %s\n", *c.Name) break } else if !*s.Enabled { removeContainer = true - log.Info("Container to be removed : %s\n", *c.Name) + log.Infow("Container to be removed : %s\n", *c.Name) } else { - log.Info("Container to be enabled : %s\n", *c.Name) + log.Infow("Container to be enabled : %s\n", *c.Name) } break } From 92718005730627c5d77106d448dc5292e581b9c8 Mon Sep 17 00:00:00 2001 From: yifeijin Date: Wed, 2 Mar 2022 11:01:51 -0500 Subject: [PATCH 2/3] fix logging but --- pkg/drivers/commonconfig.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/drivers/commonconfig.go b/pkg/drivers/commonconfig.go index fe1c2890c..8714b2b8e 100644 --- a/pkg/drivers/commonconfig.go +++ b/pkg/drivers/commonconfig.go @@ -77,13 +77,13 @@ func GetController(ctx context.Context, cr csmv1.ContainerStorageModule, operato for _, s := range cr.Spec.Driver.SideCars { if s.Name == *c.Name { if s.Enabled == nil { - log.Infow("Container to be enabled : %s\n", *c.Name) + log.Infow("Container to be enabled", "name", *c.Name) break } else if !*s.Enabled { removeContainer = true - log.Infow("Container to be removed : %s\n", *c.Name) + log.Infow("Container to be removed", "name", *c.Name) } else { - log.Infow("Container to be enabled : %s\n", *c.Name) + log.Infow("Container to be enabled", "name", *c.Name) } break } From 4abbd6366eeb93a0cb9575092386458765c3b244 Mon Sep 17 00:00:00 2001 From: yifeijin Date: Wed, 2 Mar 2022 12:03:46 -0500 Subject: [PATCH 3/3] pass ctx to finalizeradd/move --- controllers/csm_controller.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/controllers/csm_controller.go b/controllers/csm_controller.go index 4e32c52fe..ecc860bfb 100644 --- a/controllers/csm_controller.go +++ b/controllers/csm_controller.go @@ -176,7 +176,7 @@ func (r *ContainerStorageModuleReconciler) Reconcile(ctx context.Context, req ct } } - if err := r.removeFinalizer(csm); err != nil { + if err := r.removeFinalizer(ctx, csm); err != nil { r.EventRecorder.Event(csm, corev1.EventTypeWarning, v1alpha1.EventDeleted, fmt.Sprintf("Failed to delete finalizer: %s", err)) log.Errorw("remove driver finalizer", "error", err.Error()) return ctrl.Result{}, fmt.Errorf("error when handling finalizer: %v", err) @@ -189,7 +189,7 @@ func (r *ContainerStorageModuleReconciler) Reconcile(ctx context.Context, req ct // Add finalizer if !csm.HasFinalizer(CSMFinalizerName) { log.Infow("HandleFinalizer", "name", CSMFinalizerName) - if err := r.addFinalizer(csm); err != nil { + if err := r.addFinalizer(ctx, csm); err != nil { r.EventRecorder.Event(csm, corev1.EventTypeWarning, v1alpha1.EventUpdated, fmt.Sprintf("Failed to add finalizer: %s", err)) log.Errorw("HandleFinalizer", "error", err.Error()) return ctrl.Result{}, fmt.Errorf("error when adding finalizer: %v", err) @@ -430,18 +430,18 @@ func (r *ContainerStorageModuleReconciler) SetupWithManager(mgr ctrl.Manager, li }).Complete(r) } -func (r *ContainerStorageModuleReconciler) removeFinalizer(instance *csmv1.ContainerStorageModule) error { +func (r *ContainerStorageModuleReconciler) removeFinalizer(ctx context.Context, instance *csmv1.ContainerStorageModule) error { if !instance.HasFinalizer(CSMFinalizerName) { return nil } instance.SetFinalizers(nil) - return r.Update(context.Background(), instance) + return r.Update(ctx, instance) } -func (r *ContainerStorageModuleReconciler) addFinalizer(instance *csmv1.ContainerStorageModule) error { +func (r *ContainerStorageModuleReconciler) addFinalizer(ctx context.Context, instance *csmv1.ContainerStorageModule) error { instance.SetFinalizers([]string{CSMFinalizerName}) instance.GetCSMStatus().State = constants.Creating - return r.Update(context.Background(), instance) + return r.Update(ctx, instance) } // SyncCSM - Sync the current installation - this can lead to a create or update