rbd: add support for network namespaces (Multus CNI) #1450

idryomov · 2020-09-01T19:16:18Z

Depends on ceph/ceph#36927, see discussion in #1323.

Only compile tested at this point.

idryomov · 2020-09-01T21:12:48Z

E2E tests are failing with

rbd: unknown map option 'noudev'
rbd: couldn't parse map options

as expected.

humblec · 2020-09-02T07:14:23Z

We could add this mount option to example SC of RBD in e2e directory if its going to a default mount options for a particular mounter ( krbd, rbd-nbd). Also we could have different SC examples in E2E for different mounters.. Just a suggestion.

Madhu-1 · 2020-09-02T07:21:15Z

We could add this mount option to example SC of RBD in e2e directory if its going to a default mount options for a particular mounter ( krbd, rbd-nbd). Also we could have different SC examples in E2E for different mounters.. Just a suggestion.

This is not a mount option, this is the rbd map specific options passed when we are mapping and unmapping the rbd devices

humblec · 2020-09-02T07:26:27Z

We could add this mount option to example SC of RBD in e2e directory if its going to a default mount options for a particular mounter ( krbd, rbd-nbd). Also we could have different SC examples in E2E for different mounters.. Just a suggestion.

This is not a mount option, this is the rbd map specific options passed when we are mapping and unmapping the rbd devices

Ah.. Discard.. I read noudev with nodev mount option :( ..

leseb · 2020-09-02T15:12:03Z

Once we get the backport in Octopus, we can temporarily switch the Ceph base image for ceph-csi to use: ceph/daemon-base:latest-octopus-devel to validate a few things.

idryomov · 2020-09-02T21:03:51Z

@leseb As mentioned elsewhere, I would like to get the whole thing smoke tested with Multus before merging ceph/ceph#36927 into master and release branches.

idryomov · 2020-09-02T21:09:02Z

@leseb Did you mean nautilus instead of octopus?

dillaman · 2020-09-02T21:20:00Z

@leseb Did you mean nautilus instead of octopus?

Upstream pulls from the ceph/ceph:v14.2 tag

humblec · 2020-09-03T04:45:28Z

Side note to this PR: Currently the mount options can be passed and its adjusted dynamically , but we dont have a mechanism to have map options for rbd mapping . We have to hardcode it for now. Ideally this should be configurable .

Madhu-1 · 2020-09-03T05:25:37Z

Side note to this PR: Currently the mount options can be passed and its adjusted dynamically , but we dont have a mechanism to have map options for rbd mapping . We have to hardcode it for now. Ideally this should be configurable .

the issue already exists to provide support for map options #291

Madhu-1 · 2020-09-03T05:26:58Z

@dillaman @idryomov currently cephcsi is using v15 as the base image

ceph-csi/build.env

Line 13 in f2edc92

BASE_IMAGE=ceph/ceph:v15

leseb · 2020-09-03T08:11:12Z

@dillaman @idryomov currently cephcsi is using v15 as the base image

ceph-csi/build.env

Line 13 in f2edc92

BASE_IMAGE=ceph/ceph:v15

@idryomov that's why I said Octopus devel :)

leseb · 2020-09-03T08:16:49Z

@leseb As mentioned elsewhere, I would like to get the whole thing smoke tested with Multus before merging ceph/ceph#36927 into master and release branches.

We would need this PR for that or a ceph-csi build of that branch. Also, ceph-csi needs to be able to detect that it's being configured with Multus, we can check the pod annotations for that. And if multus then we map with noudev. We still this logic in this PR.

I don't want to disrupt too much ceph-csi with this that's why it's probably best to map with noudev only when Multus is configured and not make it a default yet.

idryomov · 2020-09-03T08:23:40Z

@idryomov that's why I said Octopus devel :)

OK, I'll prepare the octopus backport.

(As mentioned elsewhere, the nautilus backport is at ceph/ceph-ci@6b5a851, but the container didn't get built because nautilus
container builds have been disabled by ceph/ceph-build#1607, pending the merge of ceph/ceph-container#1706 and ceph/ceph-build#1628.)

idryomov · 2020-09-03T08:53:35Z

I don't want to disrupt too much ceph-csi with this that's why it's probably best to map with noudev only when Multus is configured and not make it a default yet.

I hard-coded noudev without conditioning it Multus or anything else because I don't want to bifurcate. If it suits Multus, it should be enabled for all CSI mappings.

I don't see a reason to wait for Multus to become fully supported before rolling out noudev, but I want to make sure it actually works in a Multus environment because it's a little subtle (e.g. noudev requires the network namespace to be owned by the initial user namespace, etc).

humblec · 2020-09-03T08:59:37Z

I don't want to disrupt too much ceph-csi with this that's why it's probably best to map with noudev only when Multus is configured and not make it a default yet.

I hard-coded noudev without conditioning it Multus or anything else because I don't want to bifurcate. If it suits Multus, it should be enabled for all CSI mappings.

Sure, as long as noudev does not impact non multus setups, we can make it default. We will wrap a better mechanism later with multus configured or not or better configurable options for RBD map options based on input..etc.

I don't see a reason to wait for Multus to become fully supported before rolling out noudev, but I want to make sure it actually works in a Multus environment because it's a little subtle (e.g. noudev requires the network namespace to be owned by the initial user namespace, etc).

Sure. That would be good.

idryomov · 2020-09-03T09:18:24Z

Yeah, making it possible to supply custom rbd map and rbd unmap options as per #291 is good, but by default, we should be striving for as much uniformity as possible.

idryomov · 2020-09-03T12:13:28Z

The octopus backport is at ceph/ceph-ci@21998a2 and the container is at quay.ceph.io/ceph-ci/ceph:wip-krbd-noudev-octopus.

rohan47 · 2020-09-21T12:25:20Z

Tested out madhupr001/cephcsi:multus and It works perfectly @Madhu-1 @idryomov

Madhu-1 · 2020-09-21T12:30:28Z

Tested out madhupr001/cephcsi:multus and It works perfectly @Madhu-1 @idryomov

@rohan47 Thanks for testing it out.

leseb · 2020-09-21T12:39:33Z

Now I believe this should detect whether Multus annotations are present, if so, then we turn on the noudev flag on mapping.

idryomov · 2020-09-21T12:47:47Z

No, I think CSI should default to noudev (see discussion above). In case of breakage, users will be able to revert through mapOptions and unmapOptions storage class parameters. I'll rebase on top of #1460 once it merges to make that possible.

humblec · 2020-09-21T12:48:37Z

Thanks @rohan47 for testing this out! hopefully we can get this in soon! Thanks @idryomov for the noudev support to enable Multus 👍

mergify · 2020-09-21T13:28:14Z

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

idryomov · 2020-11-30T19:54:28Z

The bottom line is that this is going to need to wait for 15.2.8, unless someone produces a multi-arch container (either based on octopus branch or just 15.2.6 + rbd change) that ceph-csi CI can be temporarily switched to. We have an x86 container which was successfully used by multiple people for testing and benchmarking, but ceph-csi CI needs a multi-arch (x86 + arm64) one.

idryomov · 2020-12-17T09:43:30Z

So 15.2.8 is out, but I'm hitting the same issue: it's there on docker.io but not in registry-ceph-csi.apps.ocp.ci.centos.org. In fact, the internal registry is still on 15.2.6, meaning we missed 15.2.7 -- it looks like @nixpanic's bdf8fe7 ("build: use docker.io/ceph/ceph:v15 as BASE_IMAGE") change isn't working (or maybe I'm misinterpreting it)...

Is pushing images to the internal registry really supposed to be a manual process? Why not set it up to act as a cache?

idryomov · 2020-12-22T14:09:42Z

@nixpanic Ping, can we get ceph v15.2.8 pushed to registry-ceph-csi.apps.ocp.ci.centos.org?

nixpanic · 2021-01-06T16:11:48Z

@nixpanic Ping, can we get ceph v15.2.8 pushed to registry-ceph-csi.apps.ocp.ci.centos.org?

Done! ceph/ceph:v15.2.8 has been pushed, and ceph/ceph:v15 has been updated too.

nixpanic · 2021-01-06T16:14:24Z

Is pushing images to the internal registry really supposed to be a manual process? Why not set it up to act as a cache?

There were (and possibly still are) issues when using a registry mirror, as that connects to Docker Hub a lot too, and caused failures since Docker reduced the image-pull limits. Doing this manual is a pain, and I hope we can setup an automated job that checks for updated images (daily or so).

With the new support for passing --options, referring to ExecCommand() argument slices as mapOptions and options is confusing. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Fix "ndb" typo. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Factor out --device-type and --options formatting. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Make rbdplugin pod work in a non-initial network namespace (i.e. with "hostNetwork: false") by skipping waiting for udev events when mapping and unmapping images. CSI use case is very simple: all that is needed is a device node which is immediately fed to mkfs, so we should be able to tolerate udev not being finished with the device just fine. Fixes: ceph#1323 Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

idryomov · 2021-01-07T12:02:42Z

With ceph:v15.2.8 pushed, this is good to go!

leseb

LGTM, unit tests for appendDeviceTypeAndOptions() would have certainly been a good addition but I'm not familiar enough with the repo to request them.

joed96 · 2021-01-10T10:09:51Z

Hi all, I believe I've run into a race condition resulting in data loss following this patch whereby, when re-attaching a PVC with volumeMode: Filesystem, the csi-rbdplugin re-runs mkfs and deletes my data.

The basic idea of the race condition is that:

csi-rbdplugin maps the RBD image successfully, with the noudev option
csi-rbdplugin runs blkid to determine if the RBD image is formatted. However, udev has not yet created the /dev/rbd* device. blkid exits with an error code, and so the plugin believes the image is not formatted.
udev catches up and creates the /dev/rbd* device
csi-rbdplugin runs mkfs which successfully re-formats the RBD image, deleting the previously existing data.

Detailed diagnostics are in Issue #1820.

I am new to ceph-csi, so could be missing something obvious. Please feel free to challenge my test cases and diagnostics.

idryomov · 2021-01-11T10:43:51Z

Replied in #1820.

idryomov force-pushed the wip-krbd-noudev branch from 98f80c7 to 2c05d44 Compare September 1, 2020 19:53

Madhu-1 added component/rbd Issues related to RBD DNM DO NOT MERGE labels Sep 2, 2020

dillaman closed this Sep 2, 2020

dillaman reopened this Sep 2, 2020

idryomov mentioned this pull request Sep 8, 2020

Add support for rbd map options #1460

Merged

idryomov force-pushed the wip-krbd-noudev branch 3 times, most recently from 463f551 to 298593f Compare December 17, 2020 09:23

idryomov added 4 commits January 6, 2021 23:06

rbd: rename mapOptions and options argument slices

df3df54

With the new support for passing --options, referring to ExecCommand() argument slices as mapOptions and options is confusing. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

rbd: rename ndbType parameter

440beaa

Fix "ndb" typo. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

rbd: introduce appendDeviceTypeAndOptions()

6587ef0

Factor out --device-type and --options formatting. Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

idryomov force-pushed the wip-krbd-noudev branch from 298593f to 795d68b Compare January 6, 2021 22:09

idryomov changed the title ~~[DNM] rbd: add support for network namespaces (Multus CNI)~~ rbd: add support for network namespaces (Multus CNI) Jan 6, 2021

nixpanic mentioned this pull request Jan 7, 2021

Node RBDPlugin Needs /run/udev/control #1820

Closed

nixpanic approved these changes Jan 7, 2021

View reviewed changes

nixpanic added enhancement New feature or request and removed DNM DO NOT MERGE labels Jan 7, 2021

leseb approved these changes Jan 7, 2021

View reviewed changes

mergify bot merged commit 04644c1 into ceph:master Jan 7, 2021

idryomov deleted the wip-krbd-noudev branch January 8, 2021 10:30

nixpanic added the backport-to-release-v3.2 label Jan 13, 2021

mergify bot mentioned this pull request Jan 13, 2021

rbd: add support for network namespaces (Multus CNI) (bp #1450) #1832

Merged

nixpanic mentioned this pull request Jan 14, 2021

RHSTOR-1378: add support for Multus openshift/ceph-csi#15

Merged

This was referenced Feb 11, 2021

Disable hostnetworking for CSI drivers rook/rook#7203

Closed

Remove hostnetworking dependency #1865

Closed

nixpanic mentioned this pull request Mar 4, 2021

task can not migrate/move to different host #1907

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rbd: add support for network namespaces (Multus CNI) #1450

rbd: add support for network namespaces (Multus CNI) #1450

idryomov commented Sep 1, 2020

idryomov commented Sep 1, 2020

humblec commented Sep 2, 2020 •

edited

Loading

Madhu-1 commented Sep 2, 2020 •

edited

Loading

humblec commented Sep 2, 2020

leseb commented Sep 2, 2020

idryomov commented Sep 2, 2020

idryomov commented Sep 2, 2020

dillaman commented Sep 2, 2020

humblec commented Sep 3, 2020

Madhu-1 commented Sep 3, 2020

Madhu-1 commented Sep 3, 2020

leseb commented Sep 3, 2020

leseb commented Sep 3, 2020

idryomov commented Sep 3, 2020

idryomov commented Sep 3, 2020

humblec commented Sep 3, 2020

idryomov commented Sep 3, 2020

idryomov commented Sep 3, 2020

rohan47 commented Sep 21, 2020

Madhu-1 commented Sep 21, 2020

leseb commented Sep 21, 2020

idryomov commented Sep 21, 2020 •

edited

Loading

humblec commented Sep 21, 2020

mergify bot commented Sep 21, 2020

idryomov commented Nov 30, 2020

idryomov commented Dec 17, 2020

idryomov commented Dec 22, 2020

nixpanic commented Jan 6, 2021

nixpanic commented Jan 6, 2021

idryomov commented Jan 7, 2021

leseb left a comment •

edited

Loading

joed96 commented Jan 10, 2021 •

edited

Loading

idryomov commented Jan 11, 2021

rbd: add support for network namespaces (Multus CNI) #1450

rbd: add support for network namespaces (Multus CNI) #1450

Conversation

idryomov commented Sep 1, 2020

idryomov commented Sep 1, 2020

humblec commented Sep 2, 2020 • edited Loading

Madhu-1 commented Sep 2, 2020 • edited Loading

humblec commented Sep 2, 2020

leseb commented Sep 2, 2020

idryomov commented Sep 2, 2020

idryomov commented Sep 2, 2020

dillaman commented Sep 2, 2020

humblec commented Sep 3, 2020

Madhu-1 commented Sep 3, 2020

Madhu-1 commented Sep 3, 2020

leseb commented Sep 3, 2020

leseb commented Sep 3, 2020

idryomov commented Sep 3, 2020

idryomov commented Sep 3, 2020

humblec commented Sep 3, 2020

idryomov commented Sep 3, 2020

idryomov commented Sep 3, 2020

rohan47 commented Sep 21, 2020

Madhu-1 commented Sep 21, 2020

leseb commented Sep 21, 2020

idryomov commented Sep 21, 2020 • edited Loading

humblec commented Sep 21, 2020

mergify bot commented Sep 21, 2020

idryomov commented Nov 30, 2020

idryomov commented Dec 17, 2020

idryomov commented Dec 22, 2020

nixpanic commented Jan 6, 2021

nixpanic commented Jan 6, 2021

idryomov commented Jan 7, 2021

leseb left a comment • edited Loading

Choose a reason for hiding this comment

joed96 commented Jan 10, 2021 • edited Loading

idryomov commented Jan 11, 2021

humblec commented Sep 2, 2020 •

edited

Loading

Madhu-1 commented Sep 2, 2020 •

edited

Loading

idryomov commented Sep 21, 2020 •

edited

Loading

leseb left a comment •

edited

Loading

joed96 commented Jan 10, 2021 •

edited

Loading