rbd: added RBD features support for krbd #2514

k0ste · 2021-09-16T10:40:47Z

Added support for object-map, fast-diff

Describe what this PR does

This PR added support for all currently supported krbd image features:

#define RBD_FEATURES_ALL	(RBD_FEATURE_LAYERING |		\
				 RBD_FEATURE_STRIPINGV2 |	\
				 RBD_FEATURE_EXCLUSIVE_LOCK |	\
				 RBD_FEATURE_OBJECT_MAP |	\
				 RBD_FEATURE_FAST_DIFF |	\
				 RBD_FEATURE_DEEP_FLATTEN |	\
				 RBD_FEATURE_DATA_POOL |	\
				 RBD_FEATURE_OPERATIONS)

/* Features supported by this (client software) implementation. */

#define RBD_FEATURES_SUPPORTED	(RBD_FEATURES_ALL)

The features is described in man rbd

Specifies which RBD format 2 feature should be enabled when creating an image. Multiple features can be enabled by repeating this option multiple times. The following features are supported:

· layering: layering support
· striping: striping v2 support
· exclusive-lock: exclusive locking support
· object-map: object map support (requires exclusive-lock)
· fast-diff: fast diff calculations (requires object-map)
· deep-flatten: snapshot flatten support
· journaling: journaled IO support (requires exclusive-lock)
· data-pool: erasure coded pool support

Related issues

Mention any github issues relevant to this PR. Adding below line
will help to auto close the issue once the PR is merged.

Fixes: #2513

pkalever

It will be great if we can add e2e tests for each of these features with krbd and rbd-nbd mounters?

examples/rbd/storageclass.yaml

Madhu-1

@k0ste Thanks for the contribution please take care of the below items

Update the existing documentations
Update storageclass in helm charts
Add E2E for CreateVolume, CreateSnapshot, PVC-PVC Clone, PVC restore, etc for new image features added

internal/rbd/rbd_util_test.go

internal/rbd/rbd_util.go

k0ste · 2021-09-16T11:35:40Z

Update the existing documentations

Updated

Update storageclass in helm charts

Updated

Add E2E for CreateVolume, CreateSnapshot, PVC-PVC Clone, PVC restore, etc for new image features added

Test added

Madhu-1

@k0ste CI is failing can you please check

e2e/rbd.go

tomiles · 2021-09-30T16:39:27Z

@k0ste Thanks for this PR, looking forward to this merging. We've recently hit this issue too of using the features already supported in the kernel but not being allowed by ceph-csi.

pkalever · 2021-10-01T10:26:54Z

Honestly! I'm not quite convinced with the approach that we are taking in using various features of krbd. The feature set will be dependent on the kernel version (rbd driver loaded).

We should check for krbd supported features first and if the feature is available use krbd else we should fall back to rbd-nbd.

tomiles · 2021-10-01T11:26:17Z

I agree that detecting it would be even better. But without this PR we are just blocking features supported in most of the current LTS distros kernel versions which also isn’t a good approach.

k0ste · 2021-10-01T14:21:43Z

We should check for krbd supported features first and if the feature is available use krbd else we should fall back to rbd-nbd

I think better is fail, because krbd & rbd-nbd performance is dramatically different. Admin should get error and make a self decision - use nbd or update kernel

pkalever · 2021-10-03T14:25:40Z

I think better is fail, because krbd & rbd-nbd performance is dramatically different. Admin should get error and make a self decision - use nbd or update kernel

Do you have any benchmarking results comparing both?
Right, architecture wise it makes sense to agree that the nbd performance would be a bit low, but I don't think they should be too bad or incomparable with krbd.

pkalever · 2021-10-05T14:06:52Z

FYI: for detecting the runtime krbd features supported, here is the PR: #2556

humblec · 2021-10-13T07:13:05Z

We should check for krbd supported features first and if the feature is available use krbd else we should fall back to rbd-nbd

I think better is fail, because krbd & rbd-nbd performance is dramatically different. Admin should get error and make a self decision - use nbd or update kernel

@k0ste agreed, however if admin is flexible to fallback, may be the SC option ( fallbackNbd) as in #2556 helps? just checking what is the normal practice/desire from admins in different setups in these kind of scenarios .

mergify · 2021-11-01T08:18:11Z

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

Madhu-1 · 2021-12-06T04:32:22Z

@Mergifyio rebase

mergify · 2021-12-06T04:33:04Z

rebase

✅ Branch has been successfully rebased

Madhu-1 · 2021-12-06T04:34:04Z

@k0ste do you need any help in moving this forward? if you are busy I can cherry-pick your commit and take this to completion by this week. please let me know what do you think?

Madhu-1

need to adjust the go-tests and fix go linter errors

internal/rbd/rbd_util.go

Madhu-1 · 2021-12-06T05:08:11Z

internal/rbd/rbd_util.go

+			needRbdNbd: false,
+			dependsOn:  []string{librbd.FeatureNameObjectMap},
+		},
+		librbd.FeatureNameDeepFlatten: {


@k0ste any specific reason for adding deep-flatten image feature? currently deep flatten is handled internally by cephcsi to make clones and snapshots independent of each other

Madhu-1 · 2021-12-06T05:09:02Z

I have tested this on my local minikube machine, as expected the mapping fails as minikube uses a lower kernel version (4.9.x).

rbd info replicapool/csi-vol-c3f1eb8a-5650-11ec-af4c-ca30c5bfe2cb
rbd image 'csi-vol-c3f1eb8a-5650-11ec-af4c-ca30c5bfe2cb':
	size 1 GiB in 256 objects
	order 22 (4 MiB objects)
	snapshot_count: 0
	id: 11cbfe59d66d
	block_name_prefix: rbd_data.11cbfe59d66d
	format: 2
	features: layering, exclusive-lock, object-map, fast-diff
	op_features: 
	flags: 
	create_timestamp: Mon Dec  6 04:55:42 2021
	access_timestamp: Mon Dec  6 04:55:42 2021
	modify_timestamp: Mon Dec  6 04:55:42 2021

Pod describe ouput

 Warning  FailedMount             0s (x4 over 4s)  kubelet                  MountVolume.MountDevice failed for volume "pvc-47d81c8c-76de-420e-8c63-a130641dbec4" : rpc error: code = Internal desc = rbd: map failed with error an error (exit status 6) occurred while running rbd args: [--id csi-rbd-node -m 10.104.3.155:6789 --keyfile=***stripped*** map replicapool/csi-vol-c3f1eb8a-5650-11ec-af4c-ca30c5bfe2cb --device-type krbd --options noudev], rbd error output: rbd: sysfs write failed
rbd: map failed: (6) No such device or address

dmesg logs from the minikube node

$ uname -r
4.19.202

$ dmesg
...
[  892.675164] libceph: mon0 10.104.3.155:6789 session established
[  892.675246] libceph: mon0 10.104.3.155:6789 socket closed (con state OPEN)
[  892.675262] libceph: mon0 10.104.3.155:6789 session lost, hunting for new mon
[  892.675594] libceph: mon0 10.104.3.155:6789 session established
[  892.675693] libceph: client4642 fsid e65ffd0f-8425-443f-97e8-276410d951cb
[  892.683899] rbd: image csi-vol-c3f1eb8a-5650-11ec-af4c-ca30c5bfe2cb: image uses unsupported features: 0x18
[  924.899178] libceph: mon0 10.104.3.155:6789 session established
[  924.899249] libceph: mon0 10.104.3.155:6789 socket closed (con state OPEN)
[  924.899265] libceph: mon0 10.104.3.155:6789 session lost, hunting for new mon
[  924.899601] libceph: mon0 10.104.3.155:6789 session established
[  924.899705] libceph: client4657 fsid e65ffd0f-8425-443f-97e8-276410d951cb
[  924.905023] rbd: image csi-vol-c3f1eb8a-5650-11ec-af4c-ca30c5bfe2cb: image uses unsupported features: 0x18
$

@k0ste if you can remove the e2e changes we can continue on reviews other changes

k0ste · 2021-12-06T08:17:25Z

@k0ste if you can remove the e2e changes we can continue on reviews other changes

What exactly e2e changes? I was included everything tips that your team left🤔

Madhu-1 · 2021-12-06T08:26:38Z

@k0ste if you can remove the e2e changes we can continue on reviews other changes

What exactly e2e changes? I was included everything tips that your team leftthinking

the newly added e2e test will fail in our e2e environment as we are using a minikube to create a kubernetes cluster. please check #2514 (comment)

k0ste · 2021-12-06T08:43:57Z

deep-flatten was removed as requested

Madhu-1 · 2021-12-06T08:45:16Z

Please run CONTAINER_CMD=docker make containerized-test TARGET=go-test and run: CONTAINER_CMD=docker make containerized-test TARGET=go-lint locally to see what is the issue if you see any go-test or go-lint test failure.

k0ste · 2021-12-06T11:09:31Z

Some CI test failed due:

kernel 4.19.202 does not support required features

Madhu-1 · 2021-12-06T13:01:42Z

Some CI test failed due:
kernel 4.19.202 does not support required features

CI is not going to pass as minikube comes with a kernel version of 4.19.202 which does not support fast-diff,obj-map... image feature. i tested this one and mentioned the same at #2514 (comment) and also you need to remove the e2e changes you are adding as I mentioned here #2514 (comment).

Added support for `object-map, fast-diff` Signed-off-by: Konstantin Shalygin <k0ste@k0ste.ru>

k0ste · 2021-12-06T13:14:41Z

Some CI test failed due:
kernel 4.19.202 does not support required features
CI is not going to pass as minikube comes with a kernel version of 4.19.202 which does not support fast-diff,obj-map... image feature. i tested this one and mentioned the same at #2514 (comment) and also you need to remove the e2e changes you are adding as I mentioned here #2514 (comment).

e2e changes was removed

pkalever

IMO, we shouldn't have removed e2e tests rather defended the test with util.CheckKernelSupport for the minimum required version, that way the CI will not bailout.

@k0ste can this be reverted with added defence check?

cc: @Madhu-1

pkalever · 2021-12-07T06:59:08Z

internal/rbd/rbd_util_test.go

 		{
 			"layering,journaling",
 			&rbdVolume{
-				Mounter: rbdNbdMounter,
+				Mounter: rbdDefaultMounter,


Why do we need this change?

no harm in this one.

Madhu-1 · 2021-12-07T07:29:07Z

IMO, we shouldn't have removed e2e tests rather defended the test with util.CheckKernelSupport for the minimum required version, that way the CI will not bailout.

Yes, but I don't think it's useful as this will never get executed in near time as we are still on 4.9 and what we need is 5.3 and moreover, we need to rework that one to check the kernel version on all nodes and schedule pod for that node.

Madhu-1

LGTM

Madhu-1 · 2021-12-07T07:30:06Z

internal/rbd/rbd_util_test.go

 		{
 			"layering,journaling",
 			&rbdVolume{
-				Mounter: rbdNbdMounter,
+				Mounter: rbdDefaultMounter,


no harm in this one.

Madhu-1 · 2021-12-07T07:32:31Z

@Mergifyio rebase

mergify · 2021-12-07T07:33:05Z

rebase

☑️ Nothing to do

-closed [:pushpin: rebase requirement]
#commits-behind>0 [:pushpin: rebase requirement]

pkalever · 2021-12-07T07:35:46Z

Yes, but I don't think it's useful as this will never get executed in near time as we are still on 4.9 and what we need is 5.3 and moreover, we need to rework that one to check the kernel version on all nodes and schedule pod for that node.

Soon minikube will have 5.x, minkube community is working on it. I just wanted to avoid rework on it later.

@k0ste I don't insist on having the e2e, but please create an issue for this so that we don't miss the e2e coverage later.

pkalever

Well, the changes LGTM. My only concern is on the e2e tests. Assuming @k0ste will open an issue for e2e tests later. Approving this now.

mergify bot added the component/rbd Issues related to RBD label Sep 16, 2021

k0ste force-pushed the help branch 3 times, most recently from 07fdc0e to 54a5e6e Compare September 16, 2021 10:45

k0ste changed the title ~~rbd: added RBD features for support for krbd~~ rbd: added RBD features support for krbd Sep 16, 2021

pkalever requested changes Sep 16, 2021

View reviewed changes

examples/rbd/storageclass.yaml Show resolved Hide resolved

Madhu-1 requested changes Sep 16, 2021

View reviewed changes

internal/rbd/rbd_util_test.go Show resolved Hide resolved

internal/rbd/rbd_util_test.go Show resolved Hide resolved

internal/rbd/rbd_util.go Outdated Show resolved Hide resolved

k0ste force-pushed the help branch 2 times, most recently from fe67565 to 0a4a8dd Compare September 16, 2021 11:32

k0ste force-pushed the help branch 4 times, most recently from 2576ff0 to e7390ee Compare September 16, 2021 12:56

Madhu-1 reviewed Sep 20, 2021

View reviewed changes

e2e/rbd.go Outdated Show resolved Hide resolved

k0ste force-pushed the help branch 3 times, most recently from ce754bc to 9dfb83a Compare October 1, 2021 14:18

k0ste force-pushed the help branch from 9dfb83a to 38e87b4 Compare October 1, 2021 19:25

k0ste force-pushed the help branch from 38e87b4 to c5ecdb8 Compare October 18, 2021 15:47

k0ste force-pushed the help branch from c5ecdb8 to c5470e7 Compare November 1, 2021 08:54

mergify bot added the ci/skip/e2e skip running e2e CI jobs label Nov 23, 2021

k0ste force-pushed the help branch from b6f8c29 to 5e598c9 Compare November 26, 2021 08:30

rootfs force-pushed the help branch from 5e598c9 to 5fabde2 Compare December 6, 2021 04:33

Madhu-1 requested changes Dec 6, 2021

View reviewed changes

k0ste force-pushed the help branch from 5fabde2 to 6f53361 Compare December 6, 2021 08:16

k0ste force-pushed the help branch 2 times, most recently from 6209d06 to 6d462d8 Compare December 6, 2021 08:43

k0ste force-pushed the help branch from 6d462d8 to aee6f09 Compare December 6, 2021 09:26

rbd: added RBD features support for krbd

e221012

Added support for `object-map, fast-diff` Signed-off-by: Konstantin Shalygin <k0ste@k0ste.ru>

k0ste force-pushed the help branch from aee6f09 to e221012 Compare December 6, 2021 13:13

pkalever requested changes Dec 7, 2021

View reviewed changes

Madhu-1 approved these changes Dec 7, 2021

View reviewed changes

pkalever approved these changes Dec 7, 2021

View reviewed changes

mergify bot merged commit 7411773 into ceph:devel Dec 7, 2021

Madhu-1 mentioned this pull request Dec 7, 2021

add e2e for new image features #2695

Closed

Madhu-1 mentioned this pull request Dec 14, 2021

BUG 2030745: rbd: added RBD features support for krbd red-hat-storage/ceph-csi#57

Merged

rbd: added RBD features support for krbd #2514

rbd: added RBD features support for krbd #2514

Conversation

k0ste commented Sep 16, 2021 • edited Loading

Describe what this PR does

Related issues

pkalever left a comment

Choose a reason for hiding this comment

Madhu-1 left a comment

Choose a reason for hiding this comment

k0ste commented Sep 16, 2021

Madhu-1 left a comment

Choose a reason for hiding this comment

tomiles commented Sep 30, 2021 • edited Loading

pkalever commented Oct 1, 2021

tomiles commented Oct 1, 2021

k0ste commented Oct 1, 2021

pkalever commented Oct 3, 2021

pkalever commented Oct 5, 2021

humblec commented Oct 13, 2021 • edited Loading

mergify bot commented Nov 1, 2021

Madhu-1 commented Dec 6, 2021

mergify bot commented Dec 6, 2021

✅ Branch has been successfully rebased

Madhu-1 commented Dec 6, 2021

Madhu-1 left a comment

Choose a reason for hiding this comment

Madhu-1 Dec 6, 2021

Choose a reason for hiding this comment

Madhu-1 commented Dec 6, 2021

k0ste commented Dec 6, 2021

Madhu-1 commented Dec 6, 2021

k0ste commented Dec 6, 2021

Madhu-1 commented Dec 6, 2021

k0ste commented Dec 6, 2021

Madhu-1 commented Dec 6, 2021

k0ste commented Dec 6, 2021

pkalever left a comment

Choose a reason for hiding this comment

pkalever Dec 7, 2021

Choose a reason for hiding this comment

Madhu-1 Dec 7, 2021

Choose a reason for hiding this comment

Madhu-1 commented Dec 7, 2021

Madhu-1 left a comment

Choose a reason for hiding this comment

Madhu-1 Dec 7, 2021

Choose a reason for hiding this comment

Madhu-1 commented Dec 7, 2021

mergify bot commented Dec 7, 2021

☑️ Nothing to do

pkalever commented Dec 7, 2021

pkalever left a comment

Choose a reason for hiding this comment

k0ste commented Sep 16, 2021 •

edited

Loading

tomiles commented Sep 30, 2021 •

edited

Loading

humblec commented Oct 13, 2021 •

edited

Loading