make kubelet path configurable from helm values #110

boddumanohar · 2020-09-07T09:27:11Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
Typically when setting kubernetes, the Kubelet path varies for different kind of setups. For example in Racher2 boxes, the Kubelet path is /opt/rke/var/lib/kubelet/ not /var/lib/kubelet/

The changes in this PR help make the Kubelet path configurable from helm template

Which issue(s) this PR fixes:
Fixes #69

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Special notes for your reviewer:

Release note:

Ability to make Kubelet path configurable from helm template

k8s-ci-robot · 2020-09-07T09:27:19Z

Hi @boddumanohar. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

coveralls · 2020-09-07T09:28:09Z

Pull Request Test Coverage Report for Build 244151853

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 87.819%

Totals
Change from base Build 242866399:	0.0%
Covered Lines:	620
Relevant Lines:	706

💛 - Coveralls

andyzhangx

thanks for the contribution.
Could you only make change on master branch? and also add a new parameter in charts/README.md description, thanks.

boddumanohar · 2020-09-07T09:41:16Z

Could you only make change on master branch?

@andyzhangx I am sorry, Did you mean to say to keep the changes in only charts/latest/ folder and discard the changes in other folders?

andyzhangx · 2020-09-07T10:23:17Z

Could you only make change on master branch?

@andyzhangx I am sorry, Did you mean to say to keep the changes in only charts/latest/ folder and discard the changes in other folders?

yes, only make change in latest version.

boddumanohar · 2020-09-07T10:28:01Z

Update:

Made changes only to charts/latest folder.
added the new parameter(Kubelet.path) to README.md.

andyzhangx · 2020-09-07T12:48:15Z

charts/README.md

@@ -70,6 +70,7 @@ The following table lists the configurable parameters of the latest SMB CSI Driv
 | `windows.image.nodeDriverRegistrar.repository`    | windows csi-node-driver-registrar docker image             | mcr.microsoft.com/oss/kubernetes-csi/csi-node-driver-registrar    |
 | `windows.image.nodeDriverRegistrar.tag`           | windows csi-node-driver-registrar docker image tag         | v1.2.1-alpha.1-windows-1809-amd64                                 |
 | `windows.image.nodeDriverRegistrar.pullPolicy`    | windows csi-node-driver-registrar image pull policy        | IfNotPresent                                                      |
+| `kubelet.path`                                    | configure the kubelet path                                 | `/var/lib/kubelet/`                                                      |


I would suggest set /var/lib/kubelet

use /var/lib/kubelet/ may cause error in volumeMounts string concatenate

andyzhangx · 2020-09-07T12:48:36Z

charts/latest/csi-driver-smb/templates/csi-smb-node.yaml

@@ -115,15 +115,15 @@ spec:
              memory: 20Mi
      volumes:
        - hostPath:
-            path: /var/lib/kubelet/plugins/smb.csi.k8s.io
+            path: {{ .Values.kubelet.path }}plugins/smb.csi.k8s.io


{{ .Values.kubelet.path }}/plugins/smb.csi.k8s.io

andyzhangx · 2020-09-07T12:48:59Z

charts/latest/csi-driver-smb/templates/csi-smb-node.yaml

            type: DirectoryOrCreate
          name: mountpoint-dir
        - hostPath:
-            path: /var/lib/kubelet/plugins_registry/
+            path: {{ .Values.kubelet.path }}plugins_registry/


{{ .Values.kubelet.path }}/plugins_registry/

andyzhangx · 2020-09-07T12:49:33Z

charts/latest/csi-driver-smb/templates/csi-smb-node.yaml

@@ -103,7 +103,7 @@ spec:
          volumeMounts:
            - mountPath: /csi
              name: socket-dir
-            - mountPath: /var/lib/kubelet/
+            - mountPath: {{ .Values.kubelet.path }}


don't need to change value here, it's inside container, let's keep /var/lib/kubelet/

andyzhangx · 2020-09-07T12:49:41Z

charts/latest/csi-driver-smb/templates/csi-smb-node.yaml

@@ -53,7 +53,7 @@ spec:
            - name: ADDRESS
              value: /csi/csi.sock
            - name: DRIVER_REG_SOCK_PATH
-              value: /var/lib/kubelet/plugins/smb.csi.k8s.io/csi.sock
+              value: {{ .Values.kubelet.path }}plugins/smb.csi.k8s.io/csi.sock


don't need to change value here, it's inside container, let's keep /var/lib/kubelet/

andyzhangx · 2020-09-07T13:18:38Z

/ok-to-test

boddumanohar · 2020-09-07T13:19:05Z

updated as per comments.

andyzhangx · 2020-09-08T04:01:09Z

charts/latest/csi-driver-smb/values.yaml

@@ -43,3 +43,6 @@ windows:
      repository: mcr.microsoft.com/oss/kubernetes-csi/csi-node-driver-registrar
      tag: v1.2.1-alpha.1-windows-1809-amd64
      pullPolicy: IfNotPresent
+
+kubelet:
+  path: /var/lib/kubelet


actually there is windows kubelet too, by default it's C:\var\lib\kubelet, so it could better be:

kubelet: linuxPath: /var/lib/kubelet windowsPath: 'C:\var\lib\kubelet'

Ok. What value should I specify for volumes.hostPath? Earlier we have only one(Kubelet.path) but now we will have to values: kubelet.linuxPath and Kubelet.windowsPath. Is there's a way to conditionally take values in helm based on Node Types(windows or linux)?

Or Else can have one more parameter called: node.os (supported values: windows, linux). If the node.os is windows, then we use windows paths or else we use linux paths.

you only need to change hostPath field here:

csi-driver-smb/charts/latest/csi-driver-smb/templates/csi-smb-node-windows.yaml

Lines 129 to 140 in 99b7bbe

- name: registration-dir

hostPath:

path: C:\var\lib\kubelet\plugins_registry\

type: Directory

- name: kubelet-dir

hostPath:

path: C:\var\lib\kubelet\

type: Directory

- name: plugin-dir

hostPath:

path: C:\var\lib\kubelet\plugins\smb.csi.k8s.io\

type: DirectoryOrCreate

boddumanohar · 2020-09-08T06:46:58Z

Thanks, updated as per comments.

andyzhangx · 2020-09-08T07:08:08Z

charts/README.md

@@ -70,6 +70,8 @@ The following table lists the configurable parameters of the latest SMB CSI Driv
 | `windows.image.nodeDriverRegistrar.repository`    | windows csi-node-driver-registrar docker image             | mcr.microsoft.com/oss/kubernetes-csi/csi-node-driver-registrar    |
 | `windows.image.nodeDriverRegistrar.tag`           | windows csi-node-driver-registrar docker image tag         | v1.2.1-alpha.1-windows-1809-amd64                                 |
 | `windows.image.nodeDriverRegistrar.pullPolicy`    | windows csi-node-driver-registrar image pull policy        | IfNotPresent                                                      |
+| `kubelet.linuxPath`                               | configure the kubelet path for linux nodes                 | `/var/lib/kubelet`                                                |


for Linux node

andyzhangx · 2020-09-08T07:08:19Z

charts/README.md

@@ -70,6 +70,8 @@ The following table lists the configurable parameters of the latest SMB CSI Driv
 | `windows.image.nodeDriverRegistrar.repository`    | windows csi-node-driver-registrar docker image             | mcr.microsoft.com/oss/kubernetes-csi/csi-node-driver-registrar    |
 | `windows.image.nodeDriverRegistrar.tag`           | windows csi-node-driver-registrar docker image tag         | v1.2.1-alpha.1-windows-1809-amd64                                 |
 | `windows.image.nodeDriverRegistrar.pullPolicy`    | windows csi-node-driver-registrar image pull policy        | IfNotPresent                                                      |
+| `kubelet.linuxPath`                               | configure the kubelet path for linux nodes                 | `/var/lib/kubelet`                                                |
+| `kubelet.windowsPath`                             | configure the kubelet path windows nodes                   | `'C:\var\lib\kubelet'`                                            |


for Windows node

boddumanohar · 2020-09-08T07:31:19Z

updated as per comments

andyzhangx

/lgtm
/approve

k8s-ci-robot · 2020-09-09T01:59:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, boddumanohar

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:

~~OWNERS~~ [andyzhangx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…-workaround prow.sh: work around "kind build node-image" failure

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Sep 7, 2020

k8s-ci-robot requested review from andyzhangx and ZeroMagic September 7, 2020 09:27

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 7, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 7, 2020

andyzhangx reviewed Sep 7, 2020

View reviewed changes

boddumanohar force-pushed the helm-configure-kubelet-path branch from 1cfd600 to 48c1188 Compare September 7, 2020 10:26

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 7, 2020

andyzhangx reviewed Sep 7, 2020

View reviewed changes

boddumanohar force-pushed the helm-configure-kubelet-path branch 2 times, most recently from eee19ee to 37b04dc Compare September 7, 2020 13:13

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 7, 2020

andyzhangx reviewed Sep 8, 2020

View reviewed changes

boddumanohar force-pushed the helm-configure-kubelet-path branch from 37b04dc to e2fb804 Compare September 8, 2020 06:46

andyzhangx reviewed Sep 8, 2020

View reviewed changes

make kubelet path configurable from helm values

68ac597

boddumanohar force-pushed the helm-configure-kubelet-path branch from e2fb804 to 68ac597 Compare September 8, 2020 07:22

andyzhangx approved these changes Sep 9, 2020

View reviewed changes

k8s-ci-robot assigned andyzhangx Sep 9, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2020

k8s-ci-robot merged commit 97f41af into kubernetes-csi:master Sep 9, 2020

boddumanohar mentioned this pull request Jun 3, 2021

REQUEST: New membership for boddumanohar kubernetes/org#2754

Closed

andyzhangx pushed a commit to andyzhangx/csi-driver-smb that referenced this pull request May 1, 2022

Merge pull request kubernetes-csi#110 from pohly/kind-0.9-bazel-build…

0427289

…-workaround prow.sh: work around "kind build node-image" failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make kubelet path configurable from helm values #110

make kubelet path configurable from helm values #110

boddumanohar commented Sep 7, 2020

k8s-ci-robot commented Sep 7, 2020

coveralls commented Sep 7, 2020 •

edited

Loading

andyzhangx left a comment

boddumanohar commented Sep 7, 2020

andyzhangx commented Sep 7, 2020

boddumanohar commented Sep 7, 2020

andyzhangx Sep 7, 2020

andyzhangx Sep 7, 2020

andyzhangx Sep 7, 2020

andyzhangx Sep 7, 2020

andyzhangx Sep 7, 2020

andyzhangx Sep 7, 2020

andyzhangx commented Sep 7, 2020

boddumanohar commented Sep 7, 2020

andyzhangx Sep 8, 2020 •

edited

Loading

boddumanohar Sep 8, 2020

andyzhangx Sep 8, 2020

boddumanohar commented Sep 8, 2020

andyzhangx Sep 8, 2020

andyzhangx Sep 8, 2020

boddumanohar commented Sep 8, 2020

andyzhangx left a comment

k8s-ci-robot commented Sep 9, 2020

	- name: registration-dir
	hostPath:
	path: C:\var\lib\kubelet\plugins_registry\
	type: Directory
	- name: kubelet-dir
	hostPath:
	path: C:\var\lib\kubelet\
	type: Directory
	- name: plugin-dir
	hostPath:
	path: C:\var\lib\kubelet\plugins\smb.csi.k8s.io\
	type: DirectoryOrCreate

make kubelet path configurable from helm values #110

make kubelet path configurable from helm values #110

Conversation

boddumanohar commented Sep 7, 2020

k8s-ci-robot commented Sep 7, 2020

coveralls commented Sep 7, 2020 • edited Loading

Pull Request Test Coverage Report for Build 244151853

💛 - Coveralls

andyzhangx left a comment

Choose a reason for hiding this comment

boddumanohar commented Sep 7, 2020

andyzhangx commented Sep 7, 2020

boddumanohar commented Sep 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx commented Sep 7, 2020

boddumanohar commented Sep 7, 2020

andyzhangx Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boddumanohar commented Sep 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boddumanohar commented Sep 8, 2020

andyzhangx left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 9, 2020

coveralls commented Sep 7, 2020 •

edited

Loading

andyzhangx Sep 8, 2020 •

edited

Loading