-
Notifications
You must be signed in to change notification settings - Fork 78
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
Support for CSI Manila driver deployment #572
Conversation
f8633c6
to
7111081
Compare
7111081
to
b4cad6b
Compare
rebased |
b4cad6b
to
8ac8a53
Compare
8ac8a53
to
948c4b2
Compare
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
docs/usage-as-end-user.md
Outdated
@@ -106,6 +112,9 @@ loadBalancerClasses: | |||
cloudControllerManager: | |||
featureGates: | |||
CustomResourceValidation: true | |||
#csi |
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.
this needs to be updated right ? And can we also update the example/{infrastructure,controlplane} files ?
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.
Thanks, missed that.
This reverts commit 4566a6b.
add: ["SYS_ADMIN"] | ||
allowPrivilegeEscalation: true | ||
args: |
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.
The controller does not need the priviledge escalation or sys_admin
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.
addressed
automountServiceAccountToken: false | ||
priorityClassName: gardener-system-300 | ||
containers: | ||
{{- range .Values.shareProtocols }} |
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 a bit conflicted. For now we only care about NFS but even if we did have more protocols that we want to support, I am not sure if I would prefer to run them in a "mega"-pod rather than its own separate deployments.
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.
dropped the range over share protocols
capabilities: | ||
add: ["SYS_ADMIN"] | ||
allowPrivilegeEscalation: true |
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.
No hostpath == no need for the extra permissions
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.
addressed, thanks
- name: {{ .protocolSelector | lower }}-plugin-dir | ||
mountPath: /csi | ||
|
||
- name: {{ .protocolSelector | lower }}-nfs-liveness-probe |
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.
Technically not necessary because manila forwards the requests to NFS ref
charts/internal/shoot-system-components/charts/csi-driver-manila/templates/daemonset-node.yaml
Outdated
Show resolved
Hide resolved
charts/internal/shoot-system-components/charts/csi-driver-manila/templates/daemonset-node.yaml
Outdated
Show resolved
Hide resolved
charts/internal/shoot-system-components/charts/csi-driver-manila/templates/daemonset-node.yaml
Outdated
Show resolved
Hide resolved
...l/seed-controlplane/charts/csi-driver-manila-controller/templates/deployment-controller.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Angelopoulos, Konstantinos <konstantinos.angelopoulos@sap.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.
/lgtm
How to categorize this PR?
/area control-plane
/kind enhancement
/platform openstack
What this PR does / why we need it:
Out-of-the-box support for NFS persistent volumes has been added. If enabled, a share network is created and the CSI Manila driver is deployed to the kube-system namespace on the shoot. The following two additional configuration fields are needed to set. In
InfrastructureConfig
the fieldnetworks.shareNetwork.enabled
must be set totrue
to create the share network used later. InControlPlaneConfig
the fieldcsi.manila.enabled
must be set totrue
to deploy the CSI Manila driver together with the generic CSI NFS driver.Which issue(s) this PR fixes:
Fixes #153
Special notes for your reviewer:
Release note: