-
Notifications
You must be signed in to change notification settings - Fork 559
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rbd: note that thick-provisioning is deprecated #2589
Conversation
@@ -1517,5 +1521,11 @@ func isThickProvisionRequest(parameters map[string]string) bool { | |||
return false | |||
} | |||
|
|||
if logThickProvisioningDeprecation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing coming to my mind as it's a tricky one. am fine with the current change. @ceph/contributors any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
Yes, it was only introduced to make accounting of requested space easier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit.
internal/rbd/controllerserver.go
Outdated
@@ -1517,5 +1521,11 @@ func isThickProvisionRequest(parameters map[string]string) bool { | |||
return false | |||
} | |||
|
|||
if logThickProvisioningDeprecation { | |||
log.WarningLog(context.TODO(), "thick-provisioning is "+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use WarningLogMsg
as no context need to be passed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, yes, will do!
02c598d
to
eaf3813
Compare
/retest ci/centos/mini-e2e-helm/k8s-1.22 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
eaf3813
to
2830438
Compare
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 unrelatedfailure (please report the failure too!)
/retest all
: run this in case the CentOS CI failed to start/report any testprogress or results