OCPBUGS-11146: cluster-storage-operator must re-create in-tree StorageClass

assets/storageclasses/vsphere.yaml

@@ -0,0 +1,11 @@
+apiVersion: storage.k8s.io/v1
+kind: StorageClass
+metadata:
+  name: thin
+  annotations:
+    storageclass.kubernetes.io/is-default-class: "true"
+provisioner: kubernetes.io/vsphere-volume
+parameters:
+  diskformat: thin
+reclaimPolicy: Delete
+volumeBindingMode: Immediate

go.mod

@@ -6,7 +6,7 @@ require (
 	github.com/go-logr/logr v1.2.3 // indirect
 	github.com/google/go-cmp v0.5.9
 	github.com/google/gofuzz v1.2.0 // indirect
-	github.com/openshift/api v0.0.0-20230228142948-d170fcdc0fa6
+	github.com/openshift/api v0.0.0-20230330150608-05635858d40f
 	github.com/openshift/build-machinery-go v0.0.0-20220913142420-e25cf57ea46d
 	github.com/openshift/client-go v0.0.0-20230120202327-72f107311084
 	github.com/openshift/library-go v0.0.0-20230130121854-13dc1e57ef79

go.sum

@@ -937,8 +937,8 @@ github.com/op/go-logging v0.0.0-20160315200505-970db520ece7/go.mod h1:HzydrMdWEr
 github.com/opencontainers/go-digest v1.0.0-rc1/go.mod h1:cMLVZDEM3+U2I4VmLI6N8jQYUd2OVphdqWwCJHrFt2s=
 github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM=
 github.com/opencontainers/image-spec v1.0.1/go.mod h1:BtxoFyWECRxE4U/7sNtV5W15zMzWCbyJoFRP3s7yZA0=
-github.com/openshift/api v0.0.0-20230228142948-d170fcdc0fa6 h1:SKsW89hdqceWIg1BT+vhLia8CltOV/zpY63hO67ckbw=
-github.com/openshift/api v0.0.0-20230228142948-d170fcdc0fa6/go.mod h1:ctXNyWanKEjGj8sss1KjjHQ3ENKFm33FFnS5BKaIPh4=
+github.com/openshift/api v0.0.0-20230330150608-05635858d40f h1:mGpCtfoehMcvmg/sSYLiv6nCbTl04cmtkUfYzP7H1AQ=
+github.com/openshift/api v0.0.0-20230330150608-05635858d40f/go.mod h1:ctXNyWanKEjGj8sss1KjjHQ3ENKFm33FFnS5BKaIPh4=
 github.com/openshift/build-machinery-go v0.0.0-20220913142420-e25cf57ea46d h1:RR4ah7FfaPR1WePizm0jlrsbmPu91xQZnAsVVreQV1k=
 github.com/openshift/build-machinery-go v0.0.0-20220913142420-e25cf57ea46d/go.mod h1:b1BuldmJlbA/xYtdZvKi+7j5YGB44qJUJDZ9zwiNCfE=
 github.com/openshift/client-go v0.0.0-20230120202327-72f107311084 h1:66uaqNwA+qYyQDwsMWUfjjau8ezmg1dzCqub13KZOcE=

manifests/06_operator_cr-hypershift.yaml

@@ -7,3 +7,4 @@ spec:
   logLevel: Normal
   managementState: Managed
   operatorLogLevel: Normal
+  vsphereStorageDriver: CSIWithMigrationDriver

manifests/06_operator_cr.yaml

@@ -12,3 +12,4 @@ spec:
   managementState: Managed
   logLevel: Normal
   operatorLogLevel: Normal
+  vsphereStorageDriver: CSIWithMigrationDriver

pkg/operator/defaultstorageclass/controller.go

@@ -7,10 +7,13 @@ import (
 	configv1 "github.com/openshift/api/config/v1"
 	operatorapi "github.com/openshift/api/operator/v1"
 	openshiftv1 "github.com/openshift/client-go/config/listers/config/v1"
+	oplisters "github.com/openshift/client-go/operator/listers/operator/v1"
+	"github.com/openshift/cluster-storage-operator/assets"
 	"github.com/openshift/cluster-storage-operator/pkg/csoclients"
 	"github.com/openshift/library-go/pkg/controller/factory"
 	"github.com/openshift/library-go/pkg/operator/events"
 	"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
+	"github.com/openshift/library-go/pkg/operator/resource/resourceread"
 	"github.com/openshift/library-go/pkg/operator/v1helpers"
 	storagev1 "k8s.io/api/storage/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -23,6 +26,7 @@ import (
 const (
 	conditionsPrefix      = "DefaultStorageClassController"
 	infraConfigName       = "cluster"
+	storageOperatorName   = "cluster"
 	disabledConditionType = "Disabled"
 )
 
@@ -41,6 +45,7 @@ type Controller struct {
 	operatorClient     v1helpers.OperatorClient
 	kubeClient         kubernetes.Interface
 	infraLister        openshiftv1.InfrastructureLister
+	storageLister      oplisters.StorageLister
 	storageClassLister v1.StorageClassLister
 	eventRecorder      events.Recorder
 }
@@ -52,12 +57,14 @@ func NewController(
 		operatorClient:     clients.OperatorClient,
 		kubeClient:         clients.KubeClient,
 		infraLister:        clients.ConfigInformers.Config().V1().Infrastructures().Lister(),
+		storageLister:      clients.OperatorInformers.Operator().V1().Storages().Lister(),
 		storageClassLister: clients.KubeInformers.InformersFor("").Storage().V1().StorageClasses().Lister(),
 		eventRecorder:      eventRecorder,
 	}
 	return factory.New().WithSync(c.sync).WithSyncDegradedOnError(clients.OperatorClient).WithInformers(
 		clients.OperatorClient.Informer(),
 		clients.ConfigInformers.Config().V1().Infrastructures().Informer(),
+		clients.OperatorInformers.Operator().V1().Storages().Informer(),
 		clients.KubeInformers.InformersFor("").Storage().V1().StorageClasses().Informer(),
 	).ToController("DefaultStorageClassController", eventRecorder)
 }
@@ -153,13 +160,17 @@ func (c *Controller) syncStorageClass(ctx context.Context) error {
 	if err != nil {
 		return err
 	}
+	storageOperator, err := c.storageLister.Get(storageOperatorName)
+	if err != nil {
+		return err
+	}
 	// Check to see if the PlatformStatus is nil. This has been seen on some
 	// UPI installs on baremetal platforms
 	if infrastructure.Status.PlatformStatus == nil {
 		return unsupportedPlatformError
 	}
 
-	expectedSC, err := newStorageClassForCluster(infrastructure)
+	expectedSC, err := newStorageClassForCluster(infrastructure, storageOperator)
 	if err != nil {
 		return err
 	}
@@ -183,15 +194,22 @@ func (c *Controller) syncStorageClass(ctx context.Context) error {
 	return err
 }
 
-// Returns an error indicating whether the StorageClass is provided by a CSI driver or an unsupported platform.
-func newStorageClassForCluster(infrastructure *configv1.Infrastructure) (*storagev1.StorageClass, error) {
+// Returns either the StorageClass, if the PlatformType is supported, or an error
+// indicating whether the StorageClass is provided by a CSI driver or an unsupported platform
+func newStorageClassForCluster(infrastructure *configv1.Infrastructure, storageOperator *operatorapi.Storage) (*storagev1.StorageClass, error) {
+	var storageClassFile string
 	switch infrastructure.Status.PlatformStatus.Type {
 	case configv1.AWSPlatformType:
 		return nil, supportedByCSIError
 	case configv1.GCPPlatformType:
 		return nil, supportedByCSIError
 	case configv1.VSpherePlatformType:
-		return nil, supportedByCSIError
+		// Only create the in-tree SC if CSI migration is disabled
+		if storageOperator.Spec.VSphereStorageDriver != operatorapi.CSIWithMigrationDriver {
+			storageClassFile = "storageclasses/vsphere.yaml"
+		} else {
+			return nil, supportedByCSIError
+		}
 	case configv1.AlibabaCloudPlatformType:
 		return nil, supportedByCSIError
 	case configv1.AzurePlatformType:
@@ -205,6 +223,12 @@ func newStorageClassForCluster(infrastructure *configv1.Infrastructure) (*storag
 	default:
 		return nil, unsupportedPlatformError
 	}
+
+	scBytes, err := assets.ReadFile(storageClassFile)
+	if err != nil {
+		return nil, err
+	}
+	return resourceread.ReadStorageClassV1OrDie(scBytes), nil
 }
 
 // UpdateConditionFunc returns a func to update a condition.

pkg/operator/defaultstorageclass/controller_test.go

@@ -158,8 +158,38 @@ func withFalseConditions(conditions ...string) crModifier {
 	}
 }
 
+func withVSphereMigrationEnabled() crModifier {
+	return func(i *opv1.Storage) *opv1.Storage {
+		i.Spec.VSphereStorageDriver = opv1.CSIWithMigrationDriver
+		return i
+	}
+}
+
+func withVSphereMigrationDisabled() crModifier {
+	return func(i *opv1.Storage) *opv1.Storage {
+		i.Spec.VSphereStorageDriver = opv1.LegacyDeprecatedInTreeDriver
+		return i
+	}
+}
+
 func TestSync(t *testing.T) {
 	tests := []operatorTest{
+		{
+			// Default storage class is deployed if it does not exist
+			name: "initial VSphere deployment",
+			initialObjects: testObjects{
+				storage:        getCR(),
+				infrastructure: getInfrastructure(cfgv1.VSpherePlatformType),
+			},
+			expectedObjects: testObjects{
+				storage: getCR(
+					withTrueConditions(conditionsPrefix+opv1.OperatorStatusTypeAvailable),
+					withFalseConditions(conditionsPrefix+opv1.OperatorStatusTypeProgressing),
+				),
+				storageClasses: []*storagev1.StorageClass{getPlatformStorageClass("storageclasses/vsphere.yaml")},
+			},
+			expectErr: false,
+		},
 		{
 			// The controller reports Disable on unsupported platforms
 			name: "initial unsupported platform deployment",
@@ -175,6 +205,43 @@ func TestSync(t *testing.T) {
 			},
 			expectErr: false,
 		},
+		{
+			// Everything is deployed and the controller does nothing
+			name: "everything deployed",
+			initialObjects: testObjects{
+				storage: getCR(
+					withTrueConditions(conditionsPrefix+opv1.OperatorStatusTypeAvailable),
+					withFalseConditions(conditionsPrefix+opv1.OperatorStatusTypeProgressing),
+				),
+				storageClasses: []*storagev1.StorageClass{getPlatformStorageClass("storageclasses/vsphere.yaml")},
+				infrastructure: getInfrastructure(cfgv1.VSpherePlatformType),
+			},
+			expectedObjects: testObjects{
+				storage: getCR(
+					withTrueConditions(conditionsPrefix+opv1.OperatorStatusTypeAvailable),
+					withFalseConditions(conditionsPrefix+opv1.OperatorStatusTypeProgressing),
+				),
+				storageClasses: []*storagev1.StorageClass{getPlatformStorageClass("storageclasses/vsphere.yaml")},
+			},
+			expectErr: false,
+		},
+		{
+			// The controller does not set default storage class when removed by user
+			name: "default storage class removed by user",
+			initialObjects: testObjects{
+				storage:        getCR(),
+				storageClasses: []*storagev1.StorageClass{getPlatformStorageClass("storageclasses/vsphere.yaml", withNoDefault)},
+				infrastructure: getInfrastructure(cfgv1.VSpherePlatformType),
+			},
+			expectedObjects: testObjects{
+				storage: getCR(
+					withTrueConditions(conditionsPrefix+opv1.OperatorStatusTypeAvailable),
+					withFalseConditions(conditionsPrefix+opv1.OperatorStatusTypeProgressing),
+				),
+				storageClasses: []*storagev1.StorageClass{getPlatformStorageClass("storageclasses/vsphere.yaml", withNoDefault)},
+			},
+			expectErr: false,
+		},
 		{
 			// The controller returns error - missing Available is added
 			name: "infrastructure not found",
@@ -189,6 +256,21 @@ func TestSync(t *testing.T) {
 			},
 			expectErr: true,
 		},
+		{
+			// The controller syncs fine when azurestackhub is used and does not create a storage class
+			name: "azurestackhub",
+			initialObjects: testObjects{
+				storage:        getCR(),
+				infrastructure: getAzureStackHubInfrastructure(),
+			},
+			expectedObjects: testObjects{
+				storage: getCR(
+					withFalseConditions(conditionsPrefix+opv1.OperatorStatusTypeProgressing),
+					withTrueConditions(conditionsPrefix+opv1.OperatorStatusTypeAvailable),
+				),
+			},
+			expectErr: false,
+		},
 		{
 			// The controller returns error + Available is True -> not flipped to False
 			name: "available not false after error",
@@ -206,6 +288,56 @@ func TestSync(t *testing.T) {
 			},
 			expectErr: true,
 		},
+		{
+			// The controller flips Avialable=False to True after a successful sync
+			name: "available=False set to True after OK",
+			initialObjects: testObjects{
+				infrastructure: getInfrastructure(cfgv1.VSpherePlatformType),
+				storage: getCR(
+					withFalseConditions(conditionsPrefix+opv1.OperatorStatusTypeAvailable),
+					withTrueConditions(conditionsPrefix+opv1.OperatorStatusTypeProgressing),
+				),
+			},
+			expectedObjects: testObjects{
+				storageClasses: []*storagev1.StorageClass{getPlatformStorageClass("storageclasses/vsphere.yaml")},
+				storage: getCR(
+					withTrueConditions(conditionsPrefix+opv1.OperatorStatusTypeAvailable),
+					withFalseConditions(conditionsPrefix+opv1.OperatorStatusTypeProgressing),
+				),
+			},
+			expectErr: false,
+		},
+		{
+			name: "VSphere StorageClass created if migration disabled",
+			initialObjects: testObjects{
+				storage:        getCR(withVSphereMigrationDisabled()),
+				infrastructure: getInfrastructure(cfgv1.VSpherePlatformType),
+			},
+			expectedObjects: testObjects{
+				storage: getCR(
+					withVSphereMigrationDisabled(),
+					withTrueConditions(conditionsPrefix+opv1.OperatorStatusTypeAvailable),
+					withFalseConditions(conditionsPrefix+opv1.OperatorStatusTypeProgressing),
+				),
+				storageClasses: []*storagev1.StorageClass{getPlatformStorageClass("storageclasses/vsphere.yaml")},
+			},
+			expectErr: false,
+		},
+		{
+			name: "VSphere StorageClass not created if migration enabled",
+			initialObjects: testObjects{
+				storage:        getCR(withVSphereMigrationEnabled()),
+				infrastructure: getInfrastructure(cfgv1.VSpherePlatformType),
+			},
+			expectedObjects: testObjects{
+				storage: getCR(
+					withVSphereMigrationEnabled(),
+					withTrueConditions(conditionsPrefix+opv1.OperatorStatusTypeAvailable),
+					withFalseConditions(conditionsPrefix+opv1.OperatorStatusTypeProgressing),
+				),
+			},
+			expectErr: false,
+		},
 	}
 
 	for _, test := range tests {