Merge pull request #318 from shiftstack/fix-errror-handling

OCPBUGS-44340: Fix error handling for starter controller

Author: openshift-merge-bot[bot]

pkg/driver/openstack-manila/openstack_manila.go

@@ -169,12 +169,17 @@ func GetOpenStackManilaOperatorControllerConfig(ctx context.Context, flavour gen
 			return false, errors.New("manila does not provide any share types")
 		}
 
-		syncCSIDriver(ctx, c.KubeClient, c.EventRecorder)
+		err = syncCSIDriver(ctx, c.KubeClient, c.EventRecorder)
+		if err != nil {
+			return true, err
+		}
 
-		syncStorageClasses(ctx, shareTypes, c.KubeClient, c.EventRecorder, c.GuestNamespace)
+		err = syncStorageClasses(ctx, shareTypes, c.KubeClient, c.EventRecorder, c.GuestNamespace)
+		if err != nil {
+			return true, err
+		}
 
 		return true, nil
-
 	}
 
 	cfg.PreconditionInformers = []factory.Informer{c.GetCSIDriverInformer().Informer(), c.GetStorageClassInformer().Informer()}

pkg/operator/starter_controller.go

@@ -67,10 +67,11 @@ func (c *Controller) sync(ctx context.Context, syncCtx factory.SyncContext) erro
 		return nil
 	}
 
-	_, err = c.operatorControllerConfig.Precondition()
-	if err != nil {
+	valid, err := c.operatorControllerConfig.Precondition()
+	if !valid {
 		return c.setDisabledCondition(ctx, fmt.Sprintf("Pre-condition not satisfied: %v", err))
 	}
+
 	if !c.controllersRunning {
 		// Start controllers
 		for _, controller := range c.factoryControllers {
@@ -88,6 +89,11 @@ func (c *Controller) sync(ctx context.Context, syncCtx factory.SyncContext) erro
 		c.controllersRunning = true
 	}
 
+	// If the precondition was valid, let the controllers start and return any error after.
+	if err != nil {
+		return err
+	}
+
 	return c.setEnabledCondition(ctx)
 }
 

pkg/operator/starter_controller_test.go

@@ -0,0 +1,77 @@
+package operator
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	opv1 "github.com/openshift/api/operator/v1"
+	"github.com/openshift/csi-operator/pkg/operator/config"
+	"github.com/openshift/library-go/pkg/operator/v1helpers"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestValidatePreCondition(t *testing.T) {
+	testProvider := "test-driver.csi.k8s.io"
+
+	cases := []struct {
+		name                     string
+		cfg                      *config.OperatorControllerConfig
+		expectSyncError          bool
+		expectControllersRunning bool
+	}{
+		{
+			name: "Precondition valid without error",
+			cfg: &config.OperatorControllerConfig{
+				Precondition: func() (bool, error) { return true, nil },
+			},
+			expectSyncError:          false,
+			expectControllersRunning: true,
+		},
+		{
+			name: "Precondition valid with error from internal operation",
+			cfg: &config.OperatorControllerConfig{
+				Precondition: func() (bool, error) { return true, errors.New("") },
+			},
+			expectSyncError:          true,
+			expectControllersRunning: true,
+		},
+		{
+			name: "Precondition not valid without error",
+			cfg: &config.OperatorControllerConfig{
+				Precondition: func() (bool, error) { return false, nil },
+			},
+			expectSyncError:          false,
+			expectControllersRunning: false,
+		},
+		{
+			name: "Precondition not valid with error from internal operation",
+			cfg: &config.OperatorControllerConfig{
+				Precondition: func() (bool, error) { return false, errors.New("") },
+			},
+			expectSyncError:          false,
+			expectControllersRunning: false,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			operator := &FakeOperator{
+				ObjectMeta: metav1.ObjectMeta{Name: testProvider},
+				Spec:       opv1.OperatorSpec{ManagementState: opv1.Managed},
+			}
+			c := &Controller{
+				operatorClient:           v1helpers.NewFakeOperatorClientWithObjectMeta(&operator.ObjectMeta, &operator.Spec, &operator.Status, nil),
+				operatorControllerConfig: tc.cfg,
+			}
+			ctx := context.Background()
+			err := c.sync(ctx, nil)
+			if c.controllersRunning != tc.expectControllersRunning {
+				t.Fatalf("Unexpected CSI controller running state: %v", c.controllersRunning)
+			}
+			if err != nil && !tc.expectSyncError {
+				t.Fatalf("Did not expect an error when syncing")
+			}
+		})
+	}
+}