rbd: note that thick-provisioning is deprecated #2589

nixpanic · 2021-10-25T13:13:58Z

Thick-provisioning was introduced to make accounting of assigned space
for volumes easier. When thick-provisioned volumes are the only consumer
of the Ceph cluster, this works fine. However, it is unlikely that this
is the case. Instead, accounting of the requested (thin-provisioned)
size of volumes is much more practical as different types of volumes can
be tracked.

OpenShift already provides cluster-wide quotas, which can combine
accounting of requested volumes by grouping different StorageClasses.

In addition to the difficult practise of allowing only thick-provisioned
RBD backed volumes, the performance makes thick-provisioning
troublesome. As volumes need to be completely allocated, data needs to
be written to the volume. This can take a long time, depending on the
size of the volume. Provisioning, cloning and snapshotting becomes very
much noticeable, and because of the additional time consumption, more
prone to failures.

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

/retest ci/centos/<job-name>: retest the <job-name> after unrelated
failure (please report the failure too!)
/retest all: run this in case the CentOS CI failed to start/report any test
progress or results

Madhu-1 · 2021-10-25T13:18:07Z

internal/rbd/controllerserver.go

@@ -1517,5 +1521,11 @@ func isThickProvisionRequest(parameters map[string]string) bool {
 		return false
 	}

+	if logThickProvisioningDeprecation {


@nixpanic is it good to log when starting the csi driver(if we do so we don't need to cover create, expand etc?) ? as we are logging only once not sure it will help if the user if he misses this one (when having more PVC's operations going on).

I am not sure there is any value in logging more than once. We could also do logging every N-th time, or based on a timeout if you prefer something like that?

Not all users configure thick-provisioning (it is not enabled by default), so logging it when starting the CSI-driver is not suitable IMHO and might even cause confusion.

nothing coming to my mind as it's a tricky one. am fine with the current change. @ceph/contributors any suggestions?

It won't matter to me much if we log the deprecation msg about thick-provisioning even if one doesn't set it currently (actually it might help someone who has plans to use it in the future)

Hence I would rather just log it at driver initialization time.

I prefer to not log it by default, users might assume it is enabled globally somehow and they need to take action. This would be incorrect, as nobody is expected to use the feature.

I mean a log like:

"thick-provisioning is deprecated and will be removed in a future release, this is for your information in case if you are using it currently or planing to use it in the future."

.. will not hurt.

humblec · 2021-10-26T05:36:38Z

Thick-provisioning was introduced to make accounting of assigned space for volumes easier.

In general thick provisiioning helps to reduce storage latency..etc, and a useful thing to have in use cases like vm templating.etc. so may be good to add that as well to the description. also a small description about the issues/problems we encountered ( with pointers .).etc would be good ? so that we have some justification about why we are deprecating.

nixpanic · 2021-10-26T06:57:56Z

Thick-provisioning was introduced to make accounting of assigned space for volumes easier.

In general thick provisiioning helps to reduce storage latency..etc, and a useful thing to have in use cases like vm templating.etc. so may be good to add that as well to the description. also a small description about the issues/problems we encountered ( with pointers .).etc would be good ? so that we have some justification about why we are deprecating.

Any latency related to thick-provisioning depends on the storage system used. With Ceph, there is no difference, there is no performance benefit when thick-provisioning is used. This was known in advance, so the only benefit was related to the accounting of space consumption. We never mentioned any other benefit, so I wonder why we would need to remind people about that now.

humblec · 2021-10-26T08:47:54Z

Thick-provisioning was introduced to make accounting of assigned space for volumes easier.
In general thick provisiioning helps to reduce storage latency..etc, and a useful thing to have in use cases like vm templating.etc. so may be good to add that as well to the description. also a small description about the issues/problems we encountered ( with pointers .).etc would be good ? so that we have some justification about why we are deprecating.

Any latency related to thick-provisioning depends on the storage system used. With Ceph, there is no difference, there is no performance benefit when thick-provisioning is used. This was known in advance, so the only benefit was related to the accounting of space consumption. We never mentioned any other benefit, so I wonder why we would need to remind people about that now.

Agree, but just wondering what was the justifications we have provided while introducing it in first place :) , if it was only accounting of space consumption, I think thats good enough.

nixpanic · 2021-10-26T08:59:02Z

Agree, but just wondering what was the justifications we have provided while introducing it in first place :) , if it was only accounting of space consumption, I think thats good enough.

Yes, it was only introduced to make accounting of requested space easier.

Madhu-1

small nit.

Madhu-1 · 2021-10-26T11:04:20Z

internal/rbd/controllerserver.go

@@ -1517,5 +1521,11 @@ func isThickProvisionRequest(parameters map[string]string) bool {
 		return false
 	}

+	if logThickProvisioningDeprecation {
+		log.WarningLog(context.TODO(), "thick-provisioning is "+


use WarningLogMsg as no context need to be passed?

oh, yes, will do!

Pull request has been modified.

nixpanic · 2021-10-26T12:42:35Z

/retest ci/centos/mini-e2e-helm/k8s-1.22

nixpanic · 2021-10-26T12:43:18Z

/retest ci/centos/mini-e2e-helm/k8s-1.22

Failed to start minikube VM (logs), #2264

yati1998

LGTM

Thick-provisioning was introduced to make accounting of assigned space for volumes easier. When thick-provisioned volumes are the only consumer of the Ceph cluster, this works fine. However, it is unlikely that this is the case. Instead, accounting of the requested (thin-provisioned) size of volumes is much more practical as different types of volumes can be tracked. OpenShift already provides cluster-wide quotas, which can combine accounting of requested volumes by grouping different StorageClasses. In addition to the difficult practise of allowing only thick-provisioned RBD backed volumes, the performance makes thick-provisioning troublesome. As volumes need to be completely allocated, data needs to be written to the volume. This can take a long time, depending on the size of the volume. Provisioning, cloning and snapshotting becomes very much noticeable, and because of the additional time consumption, more prone to failures. Signed-off-by: Niels de Vos <ndevos@redhat.com>

mergify bot added the component/rbd Issues related to RBD label Oct 25, 2021

Madhu-1 reviewed Oct 25, 2021

View reviewed changes

This was referenced Oct 25, 2021

e2e: run thick-provisioning test with rbd-nbd #2172

Closed

rbd-nbd: thick provisioned volumes support #2237

Closed

nixpanic requested a review from Madhu-1 October 25, 2021 13:39

humblec previously approved these changes Oct 26, 2021

View reviewed changes

Madhu-1 reviewed Oct 26, 2021

View reviewed changes

nixpanic force-pushed the deprecation/thick-provisioning branch from 02c598d to eaf3813 Compare October 26, 2021 11:53

Madhu-1 approved these changes Oct 26, 2021

View reviewed changes

nixpanic added this to the release-3.5.0 milestone Oct 26, 2021

nixpanic requested a review from humblec October 26, 2021 11:57

yati1998 approved these changes Oct 27, 2021

View reviewed changes

nixpanic force-pushed the deprecation/thick-provisioning branch from eaf3813 to 2830438 Compare October 27, 2021 04:57

mergify bot merged commit b132696 into ceph:devel Oct 27, 2021

nixpanic mentioned this pull request Oct 28, 2021

Sync the devel branch with current upstream red-hat-storage/ceph-csi#41

Merged

Madhu-1 mentioned this pull request Jan 19, 2022

Remove thickprovision deprecated code from devel branch #2795

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rbd: note that thick-provisioning is deprecated #2589

rbd: note that thick-provisioning is deprecated #2589

nixpanic commented Oct 25, 2021

Madhu-1 Oct 25, 2021

nixpanic Oct 25, 2021

Madhu-1 Oct 25, 2021

pkalever Oct 25, 2021 •

edited

Loading

nixpanic Oct 25, 2021

pkalever Oct 25, 2021

humblec commented Oct 26, 2021

nixpanic commented Oct 26, 2021

humblec commented Oct 26, 2021

nixpanic commented Oct 26, 2021

Madhu-1 left a comment

Madhu-1 Oct 26, 2021

nixpanic Oct 26, 2021

nixpanic commented Oct 26, 2021

nixpanic commented Oct 26, 2021

yati1998 left a comment

rbd: note that thick-provisioning is deprecated #2589

rbd: note that thick-provisioning is deprecated #2589

Conversation

nixpanic commented Oct 25, 2021

Madhu-1 Oct 25, 2021

Choose a reason for hiding this comment

nixpanic Oct 25, 2021

Choose a reason for hiding this comment

Madhu-1 Oct 25, 2021

Choose a reason for hiding this comment

pkalever Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

nixpanic Oct 25, 2021

Choose a reason for hiding this comment

pkalever Oct 25, 2021

Choose a reason for hiding this comment

humblec commented Oct 26, 2021

nixpanic commented Oct 26, 2021

humblec commented Oct 26, 2021

nixpanic commented Oct 26, 2021

Madhu-1 left a comment

Choose a reason for hiding this comment

Madhu-1 Oct 26, 2021

Choose a reason for hiding this comment

nixpanic Oct 26, 2021

Choose a reason for hiding this comment

nixpanic commented Oct 26, 2021

nixpanic commented Oct 26, 2021

yati1998 left a comment

Choose a reason for hiding this comment

pkalever Oct 25, 2021 •

edited

Loading