-
Notifications
You must be signed in to change notification settings - Fork 16
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
add hostpath storageclass and csi drivers #27
Conversation
Signed-off-by: Huamin Chen <hchen@redhat.com>
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 afraid this PR has currently two many open questions / issues. Instead of commenting on individual lines, I would like to outline them here as basis for discussion.
- I am not convinced that we should add
hostpath
as a CSI option at all. I am quite sure we can make it work, so from experimentation point of view it won't bring any new information. Since it's not something we could use in production, I would only add it if there is a strong need to test storage workloads before the "real" kubevirt CSI driver is ready. So far, I am not aware of such a need. - If we decide to add
hostpath
CSI, I would strongly prefer to have this configurable per shoot, and introduce a general mechanism to add other CSI drivers (e.g. Ceph FS, etc.) configurable per shoot. E.g. on some shoots we want no CSI at all, on others onlyhostpath
, and on others bothhostpath
andceph
. - The charts in
seed-controlplane
andshoots-system-components
are quite different from charts for other cloud providers, see e.g. https://github.com/gardener/gardener-extension-provider-gcp/tree/master/charts/internal/seed-controlplane/charts/csi-driver-controller and https://github.com/gardener/gardener-extension-provider-gcp/tree/master/charts/internal/shoot-system-components/charts/csi-driver-node. There is no reason not to stick to familiar ways here - csi-hostpath is just yet another CSI driver and all the other aspects should still be valid. The current charts are also incomplete. In short, they would have to be reworked entirely before they could be accepted. - Code to properly configure CSI chart values in the
controlplane
controller is also missing, see e.g. https://github.com/gardener/gardener-extension-provider-gcp/blob/c940f39c4329a904a115c795d534d2312e562c0a/pkg/controller/controlplane/valuesprovider.go#L437-L473. - For CSI to work, changes to the controlplane webhook are needed as well, see all the CSI-related stuff e.g. in https://github.com/gardener/gardener-extension-provider-gcp/blob/master/pkg/webhook/controlplane/ensurer.go.
For all this reasons, I would not reject this change, but I would ask not to continue working on it until we have decided that we definitely needed, and if this is the case, also discussed the aspects above to ensure that it's added in a way that is both configurable and consistent with other provider extensions.
/area control-plane
/area storage
/priority normal
/platform kubevirt
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #26
Special notes for your reviewer:
Release note: