ceph: run CSI pods in multus-connected host network namespace #8686

renan-campos · 2021-09-09T19:21:48Z

When using multus networks, I/O operations on CephCluster volumes are blocked
when the node's CSI plugin pods are restarted. A similar bug was found
when using the default network configuration, and was solved by moving
the CSI plugin pods to the host network namespace. By being on the host
network namespace, the restarted CSI pods have the same network
configuration as its predecessor and reconnects to the volume. This
PR applies the same principle to solve the issue for multus
configurations by placing a multus-connected interface in the host
network namespace, and running the CSI plugin pods there.

To add a multus-connected interface to the host namespace, a new
daemonset is deployed when setting up the CSI drivers. This daemonset is
connected to the multus network, and starts a priviledged initContainer
that takes the multus-connected interface and moves it to the host
network namespace. This daemonset then runs indefinitely to keep the IP
address reserved. The program running in the daemonset pods sleep continuously,
to take as little resources as possible.

Signed-off-by: Renan Campos rcampos@redhat.com

Description of your changes:

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

mergify · 2021-09-09T19:54:16Z

This pull request has merge conflicts that must be resolved before it can be merged. @renan-campos please rebase it. https://rook.io/docs/rook/master/development-flow.html#updating-your-fork

pkg/operator/ceph/csi/template/multus-host-setup.yaml

cmd/if-mover/main.go

pkg/operator/ceph/csi/spec.go

travisn

A couple of docs to update:

The multus design doc
Are any documentation updates needed for user configuration, or this is all under the covers?

cmd/coma/main.go

cmd/if-mover/main.go

travisn · 2021-09-10T19:27:16Z

pkg/operator/ceph/csi/template/multus-host-setup.yaml

+          allowPrivilegeEscalation: true
+          privileged: true
+          runAsUser: 0
+          capabilities:


These capabilities are needed in addition to privileged?

travisn · 2021-09-10T19:30:22Z

pkg/operator/ceph/csi/spec.go

+	if err != nil {
+		return errors.Wrapf(err, "failed to get daemonset template to set up the csi network")
+	}
+	err = k8sutil.CreateDaemonSet(csiMultusSetup, namespace, clientset, multusHostSetup)


We will also need the ability to apply nodeAffinity, tolerations, and resource limits. See the other CSI settings for example.

pkg/operator/ceph/csi/template/multus-host-setup.yaml

mergify · 2021-09-17T17:40:33Z

This pull request has merge conflicts that must be resolved before it can be merged. @renan-campos please rebase it. https://rook.io/docs/rook/master/development-flow.html#updating-your-fork

cmd/rook/ceph/multus.go

pkg/daemon/ceph/multus/setup.go

pkg/operator/ceph/csi/spec.go

leseb · 2021-09-28T07:53:10Z

pkg/operator/ceph/csi/spec.go

+
+	// Only looking for ceph clusters running on the same namespace as the operator.
+	// This appears to be the only supported configuration. (See call to applyCephClusterNetworkConfig)
+	cephClusters, err := r.context.RookClientset.CephV1().CephClusters(r.opConfig.OperatorNamespace).List(ctx, metav1.ListOptions{})


Don't assume the op namespace. The cephcluster is listed earlier in reconcile(), see if you can carry it.

Instead of carrying it, I just made the same client call in the function.

pkg/operator/ceph/csi/spec.go

renan-campos · 2021-10-01T16:25:11Z

I have addressed the present comments, and manually tested my solution on a local CRC cluster. I put the changes in separate commits, but will squash as necessary when it is ready to merge. Please re-review at your convenience. Thank you.

BlaineEXE

I don't think I can speak a lot to the inner workings of this. I trust that your testing will reveal any issues. Mostly, I have flagged things that make the code follow more closely Rook's internal patterns. Notably, we really want evocative error messages that...

help the user fix any problems that may be a user configuration issue
help us developers find the exact place in the code where an error has come from based on returned error text
contain enough information about the current state of execution that helps us understand what might be going wrong

BlaineEXE · 2021-10-01T17:07:44Z

pkg/daemon/ceph/multus/setup.go

+	logger.Info("determining multus ip")
+	multusIpStr, found := os.LookupEnv(multusIpEnv)
+	if !found {
+		return fmt.Errorf("environment variable %s not set.", multusIpEnv)
+	}
+	logger.Infof("multus ip: %s", multusIpStr)
+
+	logger.Info("determining multus link")
+	multusLinkName, found := os.LookupEnv(multusLinkEnv)
+	if !found {
+		return fmt.Errorf("environment variable %s not set.", multusLinkEnv)
+	}
+	logger.Infof("multus link: %s", multusLinkName)


Logging is good, but I'm not sure the messages like "determining multus ip" and "determining multus link" are particularly helpful if we are also printing the results of the steps once they are complete.

I think maybe an initial message would be good "setting up multus" or something similar, and then outputting info about the setup results as it goes.

BlaineEXE · 2021-10-01T17:07:59Z

pkg/daemon/ceph/multus/setup.go

+	logger.Info("determining multus ip")
+	multusIpStr, found := os.LookupEnv(multusIpEnv)
+	if !found {
+		return fmt.Errorf("environment variable %s not set.", multusIpEnv)


We try to use errors.Errorf and not fmt.Errorf

BlaineEXE · 2021-10-01T17:08:55Z

pkg/daemon/ceph/multus/utils.go

+		logger.Info("finding the multus network connected link")
+		link, err := netlink.LinkByName(multusLinkName)
+		if err != nil {
+			return err


Please add context to all returned error messages with errors.Wrapf("message %s", stuff). Often, utilities Rook uses don't provide useful information for debugging, and we want to make sure from an error message that we can trace exactly where in the code the error originated.

BlaineEXE · 2021-10-01T17:10:14Z

pkg/daemon/ceph/multus/utils.go

+		logger.Info("determining the IP address of the multus link")
+		addrs, err := netlink.AddrList(link, 0)
+		if err != nil {
+			log.Fatal(err)


Why Fatal here? That's not how we have patterned our error handling in Rook. We want to be able to gracefully recover from failures and retry if it makes sense.

travisn

Just a few thoughts :)

travisn · 2021-10-04T18:38:51Z

cluster/examples/kubernetes/ceph/operator-openshift.yaml

@@ -88,6 +88,7 @@ users:
  - system:serviceaccount:rook-ceph:rook-csi-rbd-provisioner-sa # serviceaccount:namespace:operator
  - system:serviceaccount:rook-ceph:rook-csi-cephfs-plugin-sa # serviceaccount:namespace:operator
  - system:serviceaccount:rook-ceph:rook-csi-cephfs-provisioner-sa # serviceaccount:namespace:operator
+  - system:serviceaccount:rook-ceph:rook-ceph-multus


Let's add this to the rook scc (about line 45 above) instead of this csi scc

But I need SYS_ADMIN and NET_ADMIN capabilities, which the rook-ceph scc doesn't provide.

Since the service account is really related to the csi driver, how about we name it like this?

Suggested change

- system:serviceaccount:rook-ceph:rook-ceph-multus

- system:serviceaccount:rook-ceph:rook-csi-multus

travisn · 2021-10-05T19:57:48Z

pkg/operator/ceph/csi/spec.go

+		if cephCluster.Spec.Network.IsMultus() {
+			if publicNetwork, ok = cephCluster.Spec.Network.Selectors["public"]; !ok {
+				logger.Debug("public network not provided; not performing multus configuration.")
+				return nil


Shouldn't we continue in case another cluster is using multus?

Suggested change

return nil

continue

travisn · 2021-10-05T19:59:13Z

pkg/operator/ceph/csi/spec.go

+	}
+
+	// Populate the host network namespace with a multus-connected interface.
+	tp := templateParam{


nit: it's preferred to give vars readable names and reserve the single letter names for the func receivers, loops, or other similarly simple places.

Suggested change

tp := templateParam{

template := templateParam{

travisn · 2021-10-05T20:02:41Z

pkg/operator/ceph/csi/spec.go

+			cleanupError = true
+			continue
+		}
+		err = k8sutil.WaitForJobCompletion(r.context.Clientset, cleanupJob, 30*time.Second)


Shouldn't this wait for job be done in the goroutine below? Then the jobs would be deployed on all applicable nodes and wait for them as a group.

Yes, thank you for catching this.

travisn · 2021-10-05T20:57:05Z

pkg/operator/ceph/csi/spec.go

+	}
+
+	for _, cephCluster := range cephClusters.Items {
+		// Assuming one CephCluster in the r.opConfig.OperatorNamespace


The cephcluster is not necessarily in the operator namespace. How about if we just iterate through all cephclusters? There is a ROOK_CURRENT_NAMESPACE_ONLY setting in the operator that we should also honor. If that's true, then we can assume it's in the same namespace as the operator.

I am iterating through all of the cephclusters, I missed removing that comment. I gather all the cephClusters on line 704.

travisn · 2021-10-05T21:11:44Z

pkg/operator/ceph/csi/spec.go

+		return errors.Wrap(err, "failed to list ceph clusters")
+	}
+
+	for _, cephCluster := range cephClusters.Items {


Detecting if multus is needed looks like in two different places. How about factoring to a common method?

travisn · 2021-10-05T21:28:15Z

design/ceph/multus-network.md

+
+If an error occurs in any of the migration jobs, the operator runs a teardown job on all of the nodes where migration was to have occurred. The teardown job runs on the host network namespace and is passed the multus IP of that node. It searches for an interface in the host namespace with the multus IP and removes it. If there is no such interface present, the job is considered complete.
+
+Restarting the node will cause the multus interface in the host namespace to go away. The holder pod will once again have the interface, and will be remigrated once the job runs again on the node.


Is the operator watching for the restart of all nodes to trigger this job?

Good point. It is not. When I tested this out, the operator was on the node I restarted so it restarted as well and re-ran all the jobs.

In the latest commit, the csi controller watches the multus daemonset. Therefore the reconcile loop will be triggered when the node restarts.

pkg/daemon/ceph/multus/teardown.go

pkg/operator/ceph/csi/spec.go

travisn · 2021-10-05T21:46:28Z

design/ceph/multus-network.md

-We can add annotations to these pods and they can reach out the Ceph public network, then the driver will expose the block or the filesystem normally.
+The CSI pods will run in the node's host network namespace.
+
+When deploying a CephCluster resource configured to use multus networks, a multus-connected network interface will be added to the host network namespace of all nodes that will run CSI plugin pods.


After the cluster is started, what happens when a new node is added to the cluster? Will it be a problem that the daemonset for the csi volume plugins will start before the job is triggered to configure the multus interface?

The csi plugin won't work properly if the job that multus interface isn't run first. The code doesn't handle the addition of new nodes, I need to add this.

My latest push adds this functionality. When a new node is added, the multus holder pod will be deployed on it because of the daemonset definition; the csi reconcile loop will then deploy the interface migration job on that node.

The controller deploys a daemonset that holds the multus interface and controls the jobs responsible for migrating the interface to the host network namespace. When the daemonset pod is deleted, it deploys a teardown job to remove the migrated interface from the hostnet. This code has been tested on a single-node CRC cluster. Signed-off-by: Renan Campos <rcampos@redhat.com>

work its magic

renan-campos · 2022-01-06T17:19:06Z

build/rbac/rbac.yaml

+---
+# Service account for multus actions related to the csi
+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  name: rook-csi-multus
+  namespace: rook-ceph
+---
+kind: Role
+apiVersion: rbac.authorization.k8s.io/v1
+metadata:
+  name: rook-csi-multus
+  namespace: rook-ceph # namespace:operator
+rules:
+  - apiGroups: [""]
+    resources: ["pods"]
+    verbs: ["get", "update"]
+  - apiGroups: ["batch"]
+    resources: ["jobs"]
+    verbs: ["create", "get", "delete"]
+---
+kind: RoleBinding
+apiVersion: rbac.authorization.k8s.io/v1
+metadata:
+  name: rook-csi-multus
+  namespace: rook-ceph # namespace:operator
+roleRef:
+  apiGroup: rbac.authorization.k8s.io
+  kind: Role
+  name: rook-csi-multus
+subjects:
+  - kind: ServiceAccount
+    name: rook-csi-multus
+    namespace: rook-ceph # namespace:operator


The multus daemonset creates jobs for interface migration and cleanup. Information about the interface being migrated is gathered and placed in the pod metadata. This is the reason for this additional service account.

renan-campos · 2022-01-06T17:20:57Z

build/rbac/rbac.yaml

@@ -702,6 +702,7 @@ spec:
  allowedCapabilities:
    # required by CSI
    - SYS_ADMIN
+    - NET_ADMIN


Pod Security Policy is a deprecated resource, and I don't know if this is used. However, the pods running multus migration needs the NET_ADMIN capability to perform network related operations. This capability is added to the appropriate SecurityContextConstraint below, and added here so that they match.

renan-campos · 2022-01-06T17:23:18Z

cmd/rook/ceph/ceph.go

+		multusControllerCmd,
+		multusSetupCmd,
+		multusTeardownCmd,


multusControllerCmd runs a daemon that creates a job to migrate a multus interface into the host network namespace. If the daemon receives a SIGTEM, a cleanup job is run. multusSetupCmd is the migration job; multusTeadownCmd is the teardown job.

renan-campos · 2022-01-07T21:33:33Z

thank you for your patience, this pr is ready for rereview.

travisn

It's been a while since we discussed this feature, can you join the Rook huddle soon or setup another meeting to discuss? thanks!

travisn · 2022-01-10T22:42:07Z

pkg/operator/ceph/csi/network.go

+		return nil
+	}
+
+	if err := opcontroller.AddFinalizerWithNameIfNotPresent(r.opManagerContext, r.client, &cephCluster, multusFinalizer); err != nil {


Even if there are multiple resources to cleanup when a cluster CR is deleted, we only need a single finalizer so the operator can trigger all the cleanup code, so I don't believe we need this new finalizer.

travisn · 2022-01-10T22:46:22Z

pkg/operator/ceph/csi/controller.go

+		}
+	}
+
+	if !cluster.DeletionTimestamp.IsZero() {


Instead of triggering the teardown from the deletion timestamp, how about from within the stopDrivers() method when the entire csi driver is also stopped? There is just one daemonset to remove from the same namespace as the operator, right? If so, then no need to trigger this from the cluster CR.

travisn · 2022-01-10T22:54:38Z

cmd/rook/ceph/multus.go

+		logger.Errorf("failed to run teardown job: %v", err)
+		// Sleep so that the developer can view the log before the pod is destroyed.
+		// Pods being deleted have a grace period of 30 seconds after the SIGTERM.
+		time.Sleep(30 * time.Second)


Let's not include a sleep for production clusters

travisn · 2022-01-10T22:57:40Z

cmd/rook/ceph/multus.go

+	<-signalChan
+	logger.Info("Running teardown job")
+	logger.Infof("Removing multus interface %q", jobParams.MigratedInterface)
+	err = multus.RunTeardownJob(k8sClient, jobParams)


Jobs are very heavy-weight, so running a job during pod teardown just doesn't seem right. Let's discuss.

renan-campos · 2022-01-11T14:49:03Z

It's been a while since we discussed this feature, can you join the Rook huddle soon or setup another meeting to discuss? thanks!

Yes, I'll make diagrams and join tomorrow's to discuss if there is time; if not we can discuss the time for separate meeting then.

mergify · 2022-01-11T15:16:49Z

This pull request has merge conflicts that must be resolved before it can be merged. @renan-campos please rebase it. https://rook.io/docs/rook/latest/development-flow.html#updating-your-fork

renan-campos · 2022-01-13T14:35:56Z

I'm closing this PR. I don't the time, competence, or desire for this to succeed.

mergify bot added the ceph main ceph tag label Sep 9, 2021

renan-campos force-pushed the bz1979561 branch from d0af72b to ba2b882 Compare September 9, 2021 19:26

renan-campos force-pushed the bz1979561 branch from ba2b882 to c9c1456 Compare September 9, 2021 20:24

leseb requested changes Sep 10, 2021

View reviewed changes

travisn requested changes Sep 10, 2021

View reviewed changes

travisn reviewed Sep 10, 2021

View reviewed changes

pkg/operator/ceph/csi/template/multus-host-setup.yaml Outdated Show resolved Hide resolved

renan-campos force-pushed the bz1979561 branch from c9c1456 to 3b771c5 Compare September 27, 2021 15:10

This comment has been minimized.

Sign in to view

renan-campos changed the title ~~ceph: run CSI pods in multus-connected host network namespace~~ [WIP] ceph: run CSI pods in multus-connected host network namespace Sep 27, 2021

renan-campos force-pushed the bz1979561 branch 3 times, most recently from 3e5bdc6 to e158db4 Compare September 27, 2021 19:56

leseb requested changes Sep 28, 2021

View reviewed changes

leseb added this to In progress in v1.8 via automation Sep 28, 2021

renan-campos force-pushed the bz1979561 branch 3 times, most recently from 93a39a5 to de9974b Compare October 1, 2021 16:21

renan-campos changed the title ~~[WIP] ceph: run CSI pods in multus-connected host network namespace~~ ceph: run CSI pods in multus-connected host network namespace Oct 1, 2021

renan-campos requested review from leseb and travisn October 1, 2021 16:25

BlaineEXE requested changes Oct 1, 2021

View reviewed changes

v1.8 automation moved this from In progress to Review in progress Oct 1, 2021

renan-campos force-pushed the bz1979561 branch from de9974b to 14fd634 Compare October 4, 2021 18:01

renan-campos requested a review from BlaineEXE October 5, 2021 12:35

renan-campos force-pushed the bz1979561 branch from 14fd634 to 521c9cf Compare October 5, 2021 12:38

travisn requested changes Oct 5, 2021

View reviewed changes

renan-campos force-pushed the bz1979561 branch from 521c9cf to 45dac40 Compare October 6, 2021 15:29

renan-campos and others added 14 commits January 7, 2022 16:08

multus controller and job logic

fd1769e

adding rook-csi-multus service account to charts and examples

3449e7d

give me a green checkmark gen-rbac... plzzzzzzzz

850ef7c

addressing golangci-lint

82ab3b5

reverting resources files that changed at some point...

047112c

setup job cleans up after itself on failure

7f4ab27

contexts set to Background()

d1f089e

removing refactor comments

5a5e7f8

adding context to addFinalizerWithName function

e20bf03

fixing... copy paste error

68f43be

actually use the multusLabel

36972c2

Create multus-network.md

df60a60

the comment # namespace:operator is needed for replaceNamespaces() to

03a883a

work its magic

renan-campos commented Jan 7, 2022

View reviewed changes

renan-campos requested review from leseb and travisn January 7, 2022 21:33

renan-campos force-pushed the bz1979561 branch from f34d22d to 69539bb Compare January 7, 2022 21:34

s -> q

69539bb

travisn requested changes Jan 10, 2022

View reviewed changes

renan-campos closed this Jan 13, 2022

v1.8 automation moved this from Review in progress to Done Jan 13, 2022

BlaineEXE self-assigned this Jan 14, 2022

BlaineEXE moved this from Done to In progress in v1.8 Jan 14, 2022

BlaineEXE mentioned this pull request Jan 14, 2022

csi: run CSI pods in multus-connected host network namespace #9577

Closed

17 tasks

travisn removed this from In progress in v1.8 Feb 8, 2022

	- system:serviceaccount:rook-ceph:rook-ceph-multus
	- system:serviceaccount:rook-ceph:rook-csi-multus


		If an error occurs in any of the migration jobs, the operator runs a teardown job on all of the nodes where migration was to have occurred. The teardown job runs on the host network namespace and is passed the multus IP of that node. It searches for an interface in the host namespace with the multus IP and removes it. If there is no such interface present, the job is considered complete.

		Restarting the node will cause the multus interface in the host namespace to go away. The holder pod will once again have the interface, and will be remigrated once the job runs again on the node.

ceph: run CSI pods in multus-connected host network namespace #8686

ceph: run CSI pods in multus-connected host network namespace #8686

Conversation

renan-campos commented Sep 9, 2021

mergify bot commented Sep 9, 2021

travisn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Sep 17, 2021

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renan-campos commented Oct 1, 2021

BlaineEXE left a 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

travisn left a 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

renan-campos commented Jan 7, 2022

travisn left a 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

renan-campos commented Jan 11, 2022

mergify bot commented Jan 11, 2022

renan-campos commented Jan 13, 2022