KEP-2923: Ceph RBD CSI Migration #2963
Conversation
k8s-ci-robot
added
sig/storage
Jiawei0227
force-pushed
the
master
branch
2 times, most recently
from
84eca87
to
5789f18
Compare
k8s-ci-robot
added
size/L
|
/lgtm This is to track the Ceph-specific implementation of CSIMigration. The design, test plan and PRR sections from |
k8s-ci-robot
added
the
lgtm
| when this feature flag && the `CSIMigration` is enabled at the same time, the in-tree volume | ||
| plugin that the cloud-provider use will be redirect to use the corresponding CSI driver. From a | ||
| user perspective, nothing will be noticed. | ||
| - InTreePluginCephRBDUnregister |
There was a problem hiding this comment.
This feature gate isn't present or described in https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/625-csi-migration/README.md -- is there precedent for it? What happens if this feature flag is enabled but the others are not?
I think it's a bit confusing for end-users to have so many feature flags controlling an identical feature. Why is this controlled with a feature flag rather than a config?
I think you will need to ensure this is accounted for in the beta questionnaire.
There was a problem hiding this comment.
Each feature flag serves a different purpose:
- CSIMigration: The core feature that controls the vendor-neutral logic
- CSIMigrationVendor: Enables the feature for a specific vendor
- InTreePluginVendorUnregister: Disables the in-tree plugin from being registered with Kubernetes. This actually can be used independently from CSIMigration* flags. Is your suggestion that this particular flag may be better suited as a config? Given that this flag is still alpha we have the opportunity to change it, but I think I would actually not track this as part of the CSI migration feature. It's a more general problem of how do we disable in-tree volume plugins.
There was a problem hiding this comment.
Yes, as Michelle has already pointed out. The flags serves all different purpose and there is no way to replace one with another. And technically speaking, this InTreePluginCephRBDUnregister flag is not part of the CSI migration design. It is just happen to be related with CSI Migration feature. Itself is actually another big question which is in-tree volume plugins. I will rephrase this paragraph to reflect this. Thanks for the advice!
There was a problem hiding this comment.
I try to rephrase and make it more clear. Please help to take a look! Thanks and appreciate!
There was a problem hiding this comment.
I also make another PR to update the original KEP section to include the feature gates. Hopefully it makes more sense now.
#2966
There was a problem hiding this comment.
It's clear to me the flags all serve different purposes, but the combination of having to use three different feature flags doesn't make for a great user experience imho.
Wearing my PRR reviewer hat, I'm stuck because you're asking me to approve this changed based on the KEP-625 questionnaire, but you've added a new feature flag here that's not discussed there :) Hence, I can't do that because this third flag's behaviour isn't documented ... it needs to be added to the PRR questions.
There was a problem hiding this comment.
Let's continue discussion in #2966 which is adding documentation for the new feature gate.
k8s-ci-robot
removed
the
lgtm
k8s-ci-robot
added
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/hold cancel |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ehashman, Jiawei0227, msau42, xing-yang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
removed
the
do-not-merge/hold
/sig storage
/cc @msau42