ceph: added support for multus for csi #5740

rohan47 · 2020-06-30T18:06:43Z

Description of your changes:
CSI pods now utilize multus networking and connect to public
network specified in the CephCluster CR.

Signed-off-by: rohan47 rohgupta@redhat.com
Which issue is resolved by this Pull Request:
Resolves #5356

Checklist:

[test ceph]

rohan47 · 2020-06-30T18:12:40Z

pkg/operator/ceph/csi/spec.go

@@ -378,6 +379,10 @@ func startDrivers(namespace string, clientset kubernetes.Interface, ver *version
 		// apply resource request and limit to rbdplugin containers
 		applyResourcesToContainers(clientset, rbdPluginResource, &rbdPlugin.Spec.Template.Spec)
 		k8sutil.SetOwnerRef(&rbdPlugin.ObjectMeta, ownerRef)
+		err = applyCephClusterNetworkConfig(&rbdPlugin.Spec.Template.ObjectMeta, rookclientset)


The annotations are applied to the csi daemonset pods but the k8s.v1.cni.cncf.io/networks-status annotation is missing so couldn't verify if the pods are using multus

Is the annotation getting overwritten?

No.
Just that there is no k8s.v1.cni.cncf.io/networks-status annotation in the pod and I am not able to get in the containers to check if the multus interfaces are added

The annotations are applied normally

Well how can the annotation be applied normally if not visible later on? Something is off.

The annotation is applied

annotations: k8s.v1.cni.cncf.io/networks: public openshift.io/scc: rook-ceph-csi

There should be a k8s.v1.cni.cncf.io/networks-status
something like the following:

Annotations: k8s.v1.cni.cncf.io/networks: public k8s.v1.cni.cncf.io/networks-status: [{ "name": "openshift-sdn", "interface": "eth0", "ips": [ "10.128.2.31" ], "dns": {}, "default-route": [ "10.128.2.1" ] },{ "name": "public", "interface": "net1", "ips": [ "192.168.1.209" ], "mac": "4e:5d:a8:b7:36:2b", "dns": {} }] openshift.io/scc: rook-ceph-csi

leseb · 2020-07-01T13:08:33Z

pkg/operator/ceph/csi/spec.go

@@ -378,6 +379,10 @@ func startDrivers(namespace string, clientset kubernetes.Interface, ver *version
 		// apply resource request and limit to rbdplugin containers
 		applyResourcesToContainers(clientset, rbdPluginResource, &rbdPlugin.Spec.Template.Spec)
 		k8sutil.SetOwnerRef(&rbdPlugin.ObjectMeta, ownerRef)
+		err = applyCephClusterNetworkConfig(&rbdPlugin.Spec.Template.ObjectMeta, rookclientset)


Is the annotation getting overwritten?

pkg/operator/ceph/csi/spec.go

pkg/operator/k8sutil/network.go

Madhu-1 · 2020-07-02T05:45:48Z

I have 2 questions related to multus support.

As the multus is per ceph cluster and a CSI driver is deployed once per rook operator, a rook operator can manage multiple ceph clusters. what happens if different ceph cluster uses different multus network?
Ceph CSI daemonset pods run with host network as we are applying the multus to daemonset pods, how it's going to behave, is daemonset pods going to use multus instead of host network?

cc @travisn

rohan47 · 2020-07-02T08:59:28Z

I have 2 questions related to multus support.

1. As the multus is per ceph cluster and a CSI driver is deployed once per rook operator, a rook operator can manage multiple ceph clusters. what happens if different ceph cluster uses different multus network?

After thinking about this it looks like the annotatins will get replaced.
Will fix this.

2. Ceph CSI daemonset pods run with host network as we are applying the multus to daemonset pods, how it's going to behave, is daemonset pods going to use multus instead of host network?

This is the part where I was stuck. Trying to use multus instead of host network and will update once I have the results.

cc @travisn

mergify · 2020-07-07T17:17:11Z

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

rohan47 · 2020-07-08T12:36:03Z

@Madhu-1 csi daemonset doesn't work on multus network. So here are my questions:

What was the reason for using host network for the daemonsets?
What will be the drawback if we use a host network for daemonset pods and multus for the rest of the csi pods?

leseb

With this PR we can remove the "experimental" flag from the doc https://rook.io/docs/rook/v1.3/ceph-cluster-crd.html and add a mention in the release note too.

travisn · 2020-07-10T18:13:59Z

With this PR we can remove the "experimental" flag from the doc https://rook.io/docs/rook/v1.3/ceph-cluster-crd.html and add a mention in the release note too.

Before we remove the experimental flag, how about we get 3rd party validation as suggested in the separate email thread today.

pkg/operator/k8sutil/network.go

rohan47 · 2020-07-21T19:58:22Z

This doc has the connection information of mons and osds and the logs of the failed OSD

mergify · 2020-07-24T05:20:40Z

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

rohan47 · 2020-08-05T13:23:22Z

With this PR we can remove the "experimental" flag from the doc https://rook.io/docs/rook/v1.3/ceph-cluster-crd.html and add a mention in the release note too.

Before we remove the experimental flag, how about we get 3rd party validation as suggested in the separate email thread today.

We are keeping the experimental flag right?

leseb · 2020-08-05T13:24:35Z

With this PR we can remove the "experimental" flag from the doc https://rook.io/docs/rook/v1.3/ceph-cluster-crd.html and add a mention in the release note too.

Before we remove the experimental flag, how about we get 3rd party validation as suggested in the separate email thread today.

We are keeping the experimental flag right?

Yes and let's link the ceph-csi issue too.

rohan47 · 2020-08-05T13:25:48Z

With this PR we can remove the "experimental" flag from the doc https://rook.io/docs/rook/v1.3/ceph-cluster-crd.html and add a mention in the release note too.

Before we remove the experimental flag, how about we get 3rd party validation as suggested in the separate email thread today.

We are keeping the experimental flag right?

Yes and let's link the ceph-csi issue too.

Okay

leseb · 2020-08-20T09:18:50Z

With this PR we can remove the "experimental" flag from the doc https://rook.io/docs/rook/v1.3/ceph-cluster-crd.html and add a mention in the release note too.

Before we remove the experimental flag, how about we get 3rd party validation as suggested in the separate email thread today.

We are keeping the experimental flag right?

Yes and let's link the ceph-csi issue too.

With this PR we can remove the "experimental" flag from the doc https://rook.io/docs/rook/v1.3/ceph-cluster-crd.html and add a mention in the release note too.

Before we remove the experimental flag, how about we get 3rd party validation as suggested in the separate email thread today.

We are keeping the experimental flag right?

Yes and let's link the ceph-csi issue too.

Okay

Any updates @rohan47? We should move forward with that.

pkg/operator/ceph/csi/spec.go

CSI pods now utilize multus networking and connect to public network specified in the CephCluster CR. Closes: rook#5356 Signed-off-by: rohan47 <rohgupta@redhat.com>

Madhu-1 · 2020-08-25T07:32:29Z

pkg/operator/ceph/csi/spec.go

+		}
+	}
+
+	return true, nil


even if the ceph cluster is not using multus it's returning true?
don't we need to return false if the ceph cluster network is not multus?

Good point, we should only return if multus got configured, I'm sending a PR to fix this.

Madhu-1 · 2020-08-25T07:35:07Z

pkg/operator/ceph/csi/spec.go

@@ -498,6 +522,23 @@ func deleteCSIDriverResources(
 	return succeeded
 }

+func applyCephClusterNetworkConfig(objectMeta *metav1.ObjectMeta, rookclientset rookclient.Interface) (bool, error) {
+	cephClusters, err := rookclientset.CephV1().CephClusters(objectMeta.Namespace).List(metav1.ListOptions{})


question: won't this fail if the ceph cluster and CSI is in a different namespace?

Yes.
The namespace we have here is actually the Rook Ceph operators namespace, right?

Yes its rook ceph operator namespace

ceph: added support for multus for csi (bp #5740)

rohan47 force-pushed the csionmultus branch from 44c12c5 to d2e1615 Compare June 30, 2020 18:08

rohan47 requested review from leseb and Madhu-1 June 30, 2020 18:09

rohan47 commented Jun 30, 2020

View reviewed changes

leseb requested changes Jul 1, 2020

View reviewed changes

rohan47 force-pushed the csionmultus branch from d2e1615 to 0a5250f Compare July 1, 2020 14:51

rohan47 force-pushed the csionmultus branch from 0a5250f to 1319378 Compare July 8, 2020 12:33

leseb requested changes Jul 10, 2020

View reviewed changes

rohan47 added the do-not-merge DO NOT MERGE :) label Jul 10, 2020

rohan47 requested a review from leseb July 20, 2020 09:48

rohan47 force-pushed the csionmultus branch 2 times, most recently from 4923bdf to af1d258 Compare July 20, 2020 14:40

leseb requested changes Jul 20, 2020

View reviewed changes

pkg/operator/k8sutil/network.go Outdated Show resolved Hide resolved

leseb requested changes Jul 21, 2020

View reviewed changes

pkg/operator/k8sutil/network.go Outdated Show resolved Hide resolved

rohan47 force-pushed the csionmultus branch from af1d258 to d1054d1 Compare July 23, 2020 10:34

rohan47 force-pushed the csionmultus branch 2 times, most recently from 9c395a0 to 226abe7 Compare August 5, 2020 11:31

mergify bot added the ceph main ceph tag label Aug 13, 2020

rohan47 force-pushed the csionmultus branch from 226abe7 to 59124d9 Compare August 20, 2020 09:49

rohan47 requested a review from leseb August 20, 2020 09:54

leseb requested changes Aug 20, 2020

View reviewed changes

pkg/operator/ceph/csi/spec.go Outdated Show resolved Hide resolved

pkg/operator/ceph/csi/spec.go Outdated Show resolved Hide resolved

rohan47 force-pushed the csionmultus branch from 59124d9 to 4e08130 Compare August 20, 2020 12:39

leseb requested changes Aug 20, 2020

View reviewed changes

pkg/operator/ceph/csi/spec.go Outdated Show resolved Hide resolved

ceph: added support for multus for csi

8c4ede1

CSI pods now utilize multus networking and connect to public network specified in the CephCluster CR. Closes: rook#5356 Signed-off-by: rohan47 <rohgupta@redhat.com>

rohan47 force-pushed the csionmultus branch from 4e08130 to 8c4ede1 Compare August 20, 2020 12:55

leseb approved these changes Aug 20, 2020

View reviewed changes

leseb removed the do-not-merge DO NOT MERGE :) label Aug 20, 2020

leseb merged commit 3f8d312 into rook:master Aug 20, 2020

leseb added the backport-release-1.4 label Aug 24, 2020

mergify bot mentioned this pull request Aug 24, 2020

ceph: added support for multus for csi (bp #5740) #6143

Merged

travisn mentioned this pull request Aug 24, 2020

SmokeSuite is failing in master with test pod failing to mount a volume #6151

Closed

Madhu-1 reviewed Aug 25, 2020

View reviewed changes

mergify bot added a commit that referenced this pull request Aug 27, 2020

Merge pull request #6143 from rook/mergify/bp/release-1.4/pr-5740

3947a7d

ceph: added support for multus for csi (bp #5740)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ceph: added support for multus for csi #5740

ceph: added support for multus for csi #5740

rohan47 commented Jun 30, 2020

rohan47 Jun 30, 2020

leseb Jul 1, 2020

rohan47 Jul 1, 2020

rohan47 Jul 1, 2020

leseb Jul 1, 2020

rohan47 Jul 1, 2020 •

edited

leseb Jul 1, 2020

Madhu-1 commented Jul 2, 2020

rohan47 commented Jul 2, 2020

mergify bot commented Jul 7, 2020

rohan47 commented Jul 8, 2020 •

edited

leseb left a comment

travisn commented Jul 10, 2020

rohan47 commented Jul 21, 2020 •

edited

mergify bot commented Jul 24, 2020

rohan47 commented Aug 5, 2020

leseb commented Aug 5, 2020

rohan47 commented Aug 5, 2020

leseb commented Aug 20, 2020

Madhu-1 Aug 25, 2020

leseb Aug 25, 2020

leseb Aug 25, 2020

Madhu-1 Aug 25, 2020

rohan47 Aug 25, 2020

Madhu-1 Aug 25, 2020

ceph: added support for multus for csi #5740

ceph: added support for multus for csi #5740

Conversation

rohan47 commented Jun 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

rohan47 Jul 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 commented Jul 2, 2020

rohan47 commented Jul 2, 2020

mergify bot commented Jul 7, 2020

rohan47 commented Jul 8, 2020 • edited

leseb left a comment

Choose a reason for hiding this comment

travisn commented Jul 10, 2020

rohan47 commented Jul 21, 2020 • edited

mergify bot commented Jul 24, 2020

rohan47 commented Aug 5, 2020

leseb commented Aug 5, 2020

rohan47 commented Aug 5, 2020

leseb commented Aug 20, 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

rohan47 Jul 1, 2020 •

edited

rohan47 commented Jul 8, 2020 •

edited

rohan47 commented Jul 21, 2020 •

edited