Rework to library-go #44

jsafrane · 2020-07-27T13:19:52Z

Rework to NewCSIControllerSet provided by library-go. In addition, following controllers are running:

ManilaController
- Checks if the cloud has Manila and starts CSIControllerSets for Manila and NFS.
- Creates StorageClasses based on Manila shares
SecretSync controller that translates cloud credentials to format understood by the driver.

The operator assumes that all connection details that the operator needs when talking to OpenStack is prepared by CSO, namely:

CredentialRequest and its result in "manila-cloud-credentials" Secret.
(Optional) self-signed CA certificate in "cloud-provider-config" ConfigMap.

@openshift/storage

bertinatto · 2020-07-29T10:41:51Z

Makefile

+# $3 - Dockerfile path
+# $4 - context directory for image build
+# It will generate target "image-$(1)" for building the image and binding it as a prerequisite to target "images".
+$(call build-image,csi-driver-manila-operator,$(IMAGE_REGISTRY)/ocp/4.5:csi-driver-manila-operator,./build/Dockerfile.openshift,.)


bertinatto · 2020-07-29T10:42:39Z

Makefile

@@ -0,0 +1,54 @@
+SHELL :=/bin/bash


not used (I need to remove from AWS EBS too)

bertinatto · 2020-07-29T10:43:24Z

assets/controller.yaml

+        - key: CriticalAddonsOnly
+          operator: Exists
+      containers:
+        # Warning: the operator expects the first container to be the CSI driver


now we can remove this

bertinatto · 2020-07-29T10:43:44Z

assets/controller.yaml

+            allowPrivilegeEscalation: true
+          image: ${DRIVER_IMAGE}
+          args:
+            - --v=5


${LOG_LEVEL} in all containers

bertinatto · 2020-07-29T10:46:48Z

assets/controller.yaml

+          securityContext:
+            privileged: true
+            capabilities:
+              add: ["SYS_ADMIN"]


Just making sure: provisioner and snapshotter need to be privileged?

No! Removed privileged from controller containers

bertinatto · 2020-07-29T12:19:20Z

test/e2e/storageclass.yaml

+metadata:
+  name: ebs.csi.aws.com
+provisioner: ebs.csi.aws.com
+volumeBindingMode: WaitForFirstConsumer


you might want to remove these files, or the whole test dir

Removed whole test directory

bertinatto · 2020-07-29T12:21:11Z

pkg/operator/starter.go

+
+	klog.Info("Starting controllers")
+	go manilaController.Run(ctx, 1)
+	go secretSyncController.Run(ctx, 1)


This controller will run regardless of manila; can we move it to the manila.Runnable slice above and have it run only when manila is present?

bertinatto · 2020-07-29T12:23:07Z

pkg/controllers/manila/manila.go

+			"csi.storage.k8s.io/node-publish-secret-namespace": util.OperatorNamespace,
+		},
+	}
+	return sc


storageclass-base.yaml isn't being used

bertinatto · 2020-07-29T12:34:04Z

pkg/controllers/secret/secretsync.go

+	return factory.New().WithSync(c.sync).WithSyncDegradedOnError(operatorClient).WithInformers(
+		operatorClient.Informer(),
+		secretInformer.Core().V1().Secrets().Informer(),
+	).ToController("SecretSync", eventRecorder)


There's no resync interval set with ResyncEvery() so this controller won't resync periodically.

Added 20 minutes resync

bertinatto · 2020-07-29T12:48:48Z

pkg/controllers/secret/secretsync.go

+		Data: data,
+	}
+
+	return &secret, nil


Side note: why does the CSI driver and the cluster-credentials-operator produce distinct secrets? Are both formats "officially" supported/valid in OpenStack? If so, I'd like to see either cloud-credentials-operator or the CSI driver supporting an additional format. But that's not a discussion for this PR...

I talked to @Fedosin and there are at least three different confing/secret formats in OpenStack, each component using its own. I agree it should be in cloud-credentials-operator

bertinatto · 2020-07-30T08:07:02Z

go.mod

+	k8s.io/client-go v0.19.0-rc.2
+	k8s.io/component-base v0.19.0-rc.2
+	k8s.io/klog/v2 v2.3.0
+	sigs.k8s.io/yaml v1.2.0


Out of curiosity: why are you pinning these deps?

go mod tidy does that.

bertinatto · 2020-07-30T08:20:00Z

pkg/controllers/manila/manila.go

+// under (short?) maintenance / reconfiguration.
+// Similarly, StorageClasses are not deleted when a share type disappears
+// from Manila.
+type Controller struct {


nit: in library-go (for debugging reasons) it's recommended to name these structs with specific names, like ManilaCSIDriverController. Same for the files. We're not in library-go, but just it might be good to follow th same convention

bertinatto · 2020-07-30T08:33:13Z

pkg/controllers/manila/manila.go

+					err = nil
+				}
+				return err
+			}


I think we don't want to return if it's not found here, we want to fall through to ApplyStorageClass instead, no?

Fixed + added comment about fallthrough

bertinatto · 2020-07-30T08:39:01Z

pkg/controllers/manila/manila.go

+	return err
+}
+
+func (c *Controller) setDisabled(msg string) error {


setDisableCondition?

bertinatto · 2020-07-30T08:52:05Z

pkg/operator/starter.go

+		kubeInformersForNamespaces,
+		openstackClient,
+		[]manila.Runnable{
+			manilaControllerSet,


nit: manila controller running the manila controller set sounds odd; perhaps rename:

manilaControllerSet -> csiDriverControllerSet
nfsController -> nfsCSIDriverController

?

bertinatto · 2020-07-30T08:59:31Z

pkg/operator/starter.go

+// Manila needs to replace two of them: Manila driver and NFS driver image.
+// Let the Manila image be replaced by CSIDriverController and NFS in this
+// custom asset loading func.
+func assetWithNFSDriver(file string) []byte {


bertinatto · 2020-07-30T09:04:31Z

pkg/operator/starter.go

+
+const (
+	operandName  = "manila-csi-driver"
+	operatorName = "manila-csi-driver-operator"


note: the operator namespace is in util, not sure if want to have them in the same place

It's name of the operator, not name of the namespace. It's just coincidence they're the same :-)

I meant to say: have the operator name and namespace consts in the same place. But that's not really important

bertinatto · 2020-07-30T09:14:51Z

pkg/operator/starter.go

+		operatorClient,
+		assetWithNFSDriver,
+		kubeClient,
+		controllerConfig.EventRecorder,


The order of these args is incorrect:

func NewCSIDriverController( name string, csiDriverName string, csiDriverNamespace string, instanceName string, operatorClient v1helpers.OperatorClient, assetFunc func(string) []byte, kubeClient kubernetes.Interface, eventRecorder events.Recorder,

Nice catch, fixed

bertinatto · 2020-07-30T09:29:06Z

pkg/controllers/manila/manila.go

+	// Minimal interval between controller resyncs. The controller will detect
+	// new share types in Manila and create StorageClasses for them at least
+	// once per this interval.
+	resyncInterval = 1 * time.Minute


Do we need to resync every minute?

When someone creates a new share type in OpenStack, this operator should create StorageClass for it. Question is what's the right frequency.

IIUC the optimal resync interval for this type of storage backend (manila, EFS) will vary from cluster to cluster. For instance, in some clusters the number of share types will seldom change, while in others new share types will be added and removed quite often.

Having said that, whatever we hardcode here might not be the ideal for some use cases.

IMO the best way to approach this is to define a standard value, say 20 minutes, and allow the cluster admin to customize it through the CR. If we choose this approach, we could tackle the customization part in another PR.

Thoughts?

I'm updating it to 20 min, however, I am not sure how important it is to have it customizable.

bertinatto · 2020-07-30T11:38:08Z

README.md


-Operator to create, configure and manage CSI driver for OpenStack Manila in OpenShift.
+An operator to deploy the [Manil CSI driver](https://github.com/openshift/cloud-provider-openstack/tree/master/pkg/csi/manila) in OpenShift.


bertinatto · 2020-07-30T11:39:06Z

README.md


-### Installing the operator
+The operator is based on [openshift/library-go](https://github.com/openshift/library-go). It manages `ManilaDriver` instance named `cluster` and runs several controllers in parallel:


Kind and CR changed

bertinatto · 2020-07-30T11:55:48Z

pkg/operator/starter.go

+	).WithCSIDriverController(
+		"ManilaDriverController",
+		operandName,
+		string(operatorapi.ManilaCSIDriver),


Aren't the instance/config name and driver/operand name swapped here?

Fixed, I think

bertinatto · 2020-07-30T11:57:08Z

pkg/operator/starter.go

+		"NFSDriverController",
+		string(operatorapi.ManilaCSIDriver),
+		util.OperatorNamespace,
+		"nfs-csi-driver",


Swap line 83 with line 85?

Fixed, I think

bertinatto · 2020-07-30T11:59:43Z

README.md

+    * It starts `nfsController`: Runs `csidriverset.Controller` that installs NFS CSI driver itself.
+    * It creates `StorageClass` for each share type reported by Manila and periodically syncs them at least once per minute, in case a new share type appears in Manila.
+    * It never removes StorageClass if Manila service or share type disappears. It may be temporary OpenStack ir Manila re-configuration hiccup.
+  * If there is no Manila service, it marks the `ManilaDriver` instance with `ManilaControllerDisabled: True` condition. It does not stop any CSI drivers started when Manila service was present! This allows pod to at least unmount their volumes. 


Replace ManilaDriver with ClusterCSIDriver

bertinatto · 2020-07-30T12:19:13Z

pkg/controllers/manila/manila.go

+type ManilaController struct {
+	operatorClient     v1helpers.OperatorClient
+	kubeClient         kubernetes.Interface
+	secretLister       corelisters.SecretLister


I think you're not using this anymore

bertinatto · 2020-07-30T12:22:03Z

pkg/controllers/manila/openstack.go

+
+	cert, err := getCloudProviderCert()
+	if err != nil && !os.IsNotExist(err) {
+		return nil, fmt.Errorf("Failed to get cloud provider CA certificate: %v", err)


nit: my editor says errors should not start with capital letters

bertinatto · 2020-07-30T12:22:20Z

pkg/controllers/manila/openstack.go

+	if len(cert) > 0 {
+		certPool, err := x509.SystemCertPool()
+		if err != nil {
+			return nil, fmt.Errorf("Create system cert pool failed: %v", err)


bertinatto · 2020-07-30T13:33:08Z

/lgtm

Adding hold in case @Fedosin wants to take a look
/hold

Fedosin · 2020-07-30T18:04:46Z

README.md

-```sh
-oc delete -f deploy/crds/csi.openshift.io_maniladrivers_crd.yaml -f deploy/role.yaml -f deploy/role_binding.yaml -f deploy/service_account.yaml -f deploy/namespace.yaml
-```
+* OpenStack cloud credentials are in Secret named "cloud-credentials" in the same namespace where the operator runs. The operator uses the credentials to check if Manila is present in the cluster and, since OpenStack does not allow any fine-grained access controll, it lets the CSI driver to use the same credentials.


access control

Fedosin · 2020-07-30T18:05:20Z

README.md

-oc delete -f deploy/crds/csi.openshift.io_maniladrivers_crd.yaml -f deploy/role.yaml -f deploy/role_binding.yaml -f deploy/service_account.yaml -f deploy/namespace.yaml
-```
+* OpenStack cloud credentials are in Secret named "cloud-credentials" in the same namespace where the operator runs. The operator uses the credentials to check if Manila is present in the cluster and, since OpenStack does not allow any fine-grained access controll, it lets the CSI driver to use the same credentials.
+* If underlying OpenStack uses self-signed certificate, the operator expects the ceritificate is present in a ConfigMap named "cloud-provider-config" with key "ca-bundle.pem" in the namespace where it runs. Generally, it should be a copy of "openshift-config/cloud-provider-config" ConfigMap. It then uses the certificate to talk to OpenStack API.


certificate

Fedosin · 2020-07-30T18:10:16Z

pkg/version/version.go

+func init() {
+	buildInfo := prometheus.NewGaugeVec(
+		prometheus.GaugeOpts{
+			Name: "openshift_aws_ebs_csi_driver_operator",


openshift_manila_csi_driver_operator

good catch, fixed

Fedosin · 2020-07-30T18:13:45Z

pkg/version/version.go

+	buildInfo := prometheus.NewGaugeVec(
+		prometheus.GaugeOpts{
+			Name: "openshift_aws_ebs_csi_driver_operator",
+			Help: "A metric with a constant '1' value labeled by major, minor, git commit & git version from which OpenShift AWS EBS CSI Driver Operator was built.",


Openshift Manila CSI Driver

Use OpenStack's ceritificates when OpenStack API uses self-signed ones. Assume that CSO creates a ConfigMap with it when it starts the operator. Since CSO is going to create the ConfigMap and operator Deployment in parallel, make sure the operator recovers when the ConfigMap is created later. In addition, the certificate may be added / updated later, when the operator already runs for some time.

The operator must already have valid cloud credentials when it starts to check if underlying OpenStack has Manila. Since OpenStack does not provide fine-grained permissions, the credentials used by the operator and the driver can be the same. Therefore let the driver reuse the operator credentials.

Delete and re-create a StorageClass when there is a change in StorageClass.Paramertes. Kubernetes API does not allow Parameters change after creation. This is useful when moving a StorageClass between namespaces - secrets namespace changes.

As required by OCP. 50 MB / 1% of a CPU core for most containers, 20MB / 0.5% for registrar.

It takes ~20 seconds to take some final error

1 minute is too often. We do not expect that new share types are added regularly.

library-go does full copare and it does not work well with defaulted fields

jsafrane · 2020-07-30T18:26:00Z

/hold cancel

Fedosin · 2020-07-30T18:58:56Z

/lgtm

openshift-ci-robot · 2020-07-30T18:58:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Fedosin, jsafrane

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Fedosin,jsafrane]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Rework to library-go

29687e2

openshift-ci-robot requested review from gnufied and iamemilio July 27, 2020 13:21

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2020

jsafrane force-pushed the library-go branch 3 times, most recently from 0c9b7b1 to fea78fd Compare July 29, 2020 08:45

jsafrane mentioned this pull request Jul 29, 2020

Add oVirt and manila to CSO openshift/cluster-storage-operator#65

Merged

bertinatto reviewed Jul 29, 2020

View reviewed changes

jsafrane force-pushed the library-go branch 3 times, most recently from 3f025c9 to 57addc4 Compare July 29, 2020 15:35

bertinatto reviewed Jul 30, 2020

View reviewed changes

jsafrane force-pushed the library-go branch from a2b1f30 to 9dede34 Compare July 30, 2020 09:55

bertinatto reviewed Jul 30, 2020

View reviewed changes

jsafrane force-pushed the library-go branch from c93348a to ac65fe1 Compare July 30, 2020 12:50

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2020

openshift-ci-robot assigned bertinatto Jul 30, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2020

Fedosin suggested changes Jul 30, 2020

View reviewed changes

openshift-ci-robot assigned Fedosin Jul 30, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2020

jsafrane added 4 commits July 30, 2020 20:24

Add manifests for local testing

c39434a

Replace existing storage classes

8821ad0

Delete and re-create a StorageClass when there is a change in StorageClass.Paramertes. Kubernetes API does not allow Parameters change after creation. This is useful when moving a StorageClass between namespaces - secrets namespace changes.

jsafrane added 22 commits July 30, 2020 20:24

Set Disabled condition when Manila is not available

e1ac610

Add resources to all containers

1531af0

As required by OCP. 50 MB / 1% of a CPU core for most containers, 20MB / 0.5% for registrar.

Use OpenStack's self-signed certificate if provided

2d0d211

Update Makefile

fb298aa

Update manifests

3d202d9

Use klog/v2

2594e0c

Remove unused dependency

4ef22c1

Use operator name as user-agent

23ab5b6

Add component to event recorders

0e1bb8c

Remove test directory

5120a99

Fix typos

326baed

Simplify return code

3edc43e

Start secret syncer only when manila is present

5e9d2a6

Add resync to secret syncer

66c65dd

Use longer timeout for provisioning

b19bc9a

It takes ~20 seconds to take some final error

Rename Controllers to have unique name

f122a05

Fixed processing of IsNotFound error on storage class deletion

5531d15

Rename functions and variables to be more descriptive

8973dfe

Fix NFS CSi driver controller parameters

4ad8bcc

Resync every 20 minutes

5ea292c

1 minute is too often. We do not expect that new share types are added regularly.

Fill all StorageClass fields

0fa3436

library-go does full copare and it does not work well with defaulted fields

Fixed README typos

4d48a87

jsafrane force-pushed the library-go branch from ac65fe1 to 4d48a87 Compare July 30, 2020 18:25

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2020

Fedosin approved these changes Jul 30, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2020

openshift-merge-robot merged commit 4661d85 into openshift:master Jul 30, 2020


		Operator to create, configure and manage CSI driver for OpenStack Manila in OpenShift.
		An operator to deploy the [Manil CSI driver](https://github.com/openshift/cloud-provider-openstack/tree/master/pkg/csi/manila) in OpenShift.


		### Installing the operator
		The operator is based on [openshift/library-go](https://github.com/openshift/library-go). It manages `ManilaDriver` instance named `cluster` and runs several controllers in parallel:

Rework to library-go #44

Rework to library-go #44

Conversation

jsafrane commented Jul 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertinatto commented Jul 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented Jul 30, 2020

Fedosin commented Jul 30, 2020

openshift-ci-robot commented Jul 30, 2020

jsafrane commented Jul 27, 2020 •

edited