Make kubelet work directory overridable via single chart parameter

[rkrzewski]

/var/lib/kubelet directory appears multiple times in node component
configuration. node.kubeletWorkDirectory parameter allows overriding
them all in concert, which is useful for deplyment on microk8s where
/var/snap/microk8s/common/var/lib/kubelet directory needs to be used
instead.

This is an alternative to #407. Either one can be used to solve the problem of deployment on microk8s cluster described in #406. This PR is larger but provides more streamlined user experience: only 1 string property to override instead of 4, 3 of which require structured values.

[toshipp]

@rkrzewski
We took a look at this PR and have a concern about there is necessary to write down all volumes section if the lvmd socket path is changed. This is a similar issue to your motivation for the PR, so I think #407 is better, as well as in terms of simplicity.

[rkrzewski]

#407 is indeed simpler in terms of implementation, but more complicated in usage. Helm offers path manipulation functions so generating correct mounts etc for a modified node.lvmdSocket can be implemented with little effort.

Before I delve into it I'd like to understand the requirements better

lvmd.socketName is /run/topolvm/lvmd.sock by default. The predefined volumes / volume mounts agree with the default value, but need to be overridden if the directory part of lvmd.socketName is changed. This could be improved.

node.lvmdSocket is /run/lvmd/lvmd.sock by default. It appears to disagree with default value of lvmd.socketName

lvmd-socket-dir volume for node component refers to /run/topolvm so it agrees with default socket location but disagrees with node.lvmdSocket location. It is then mounted at /run/lvmd location in the topolvm-node container. At the same time
--lvmd-socket={{ .Values.node.lvmdSocket }} is passed to the process. As long as directory part of node.lvmdSocket is not changed, it works albeit in a roundabout way.

I thought it would be more natural to set node.lvmdSocket a default value of /run/topolvm/lvmd.sock, use {{ dir .Values.node.lvmdSocket }} host path for lvmd-socket-dir volume and mount it at the same path in the container, however there is a problem: /run/topolvm path in the container is used for node-plugin-dir volume mounted from /var/lib/kubelet/plugins/topolvm.cybozu.com/node at the host. --csi-address=/run/topolvm/csi-topolvm.sock is then passed to CSI sidecar containers.

Okay, after following through the paths and mounts carefully and writing the above I have a plan for improvement. I'll add a couple more commits to PR for your consideration.

[toshipp]

@rkrzewski
Your insight is correct, and I can accept the PR, but more work is needed.
We use helm chart for e2e, so we have to modify values files for them.
https://github.com/topolvm/topolvm/tree/main/e2e/manifests/values
Would you update these files as well?

[rkrzewski]

I am glad to hear that! Yes I will look into updating the e2e tests.

I'll also look through the templates to check if the volumes / volumeMounts generation should be updated the same way as for the node component. lvmd component is the obvious candidate, but maybe there's something else.

[toshipp]

We set /tmp/topolvm/daemonset_lvmd/lvmd.sock to lvmd.socketName, so node.lvmdSocket is also needed to set as well, IMO.

[rkrzewski]

Thanks for the review @daichimukai. I have applied the requested changes.

[toshipp]

I'd passed tests locally with the following values. Would you change values accordingly?

diff --git a/e2e/manifests/values/daemonset-scheduler.yaml b/e2e/manifests/values/daemonset-scheduler.yaml
index 5fa3da84..2267486f 100644
--- a/e2e/manifests/values/daemonset-scheduler.yaml
+++ b/e2e/manifests/values/daemonset-scheduler.yaml
@@ -23,31 +23,7 @@ lvmd:
   managed: false

 node:
-  volumes:
-    - name: registration-dir
-      hostPath:
-        path: /var/lib/kubelet/plugins_registry/
-        type: Directory
-    - name: node-plugin-dir
-      hostPath:
-        path: /var/lib/kubelet/plugins/topolvm.cybozu.com/node
-        type: DirectoryOrCreate
-    - name: csi-plugin-dir
-      hostPath:
-        path: /var/lib/kubelet/plugins/kubernetes.io/csi
-        type: DirectoryOrCreate
-    - name: pod-volumes-dir
-      hostPath:
-        path: /var/lib/kubelet/pods/
-        type: DirectoryOrCreate
-    - name: lvmd-socket-dir
-      hostPath:
-        path: /tmp/topolvm
-        type: Directory
-    - name: device-dir
-      hostPath:
-        path: /dev
-        type: Directory
+  lvmdSocket: /tmp/topolvm/lvmd.sock

 storageClasses:
   - name: topolvm-provisioner

[toshipp]

@rkrzewski
Thank you for your update!
I confirmed that deployment-scheduler.yaml and storage-capacity.yaml can be also updated as well as daemonset-scheduler.yaml. Would you fix them?

[rkrzewski]

I've submitted the requested changes. Holding fingers crossed for the CI!

Would you like me to squash the changes before merging? There is some back-and-forth in those commits that won't provide much value from historical perspective.

There's one more thing that struck me while working with the CI manifests: there's a discrepancy between parameter names: lvmd.socketName vs node.lvmdSocket. Shouldn't the latter be node.lvmdSocketName? I realize this is a non-backwards compatible change and should be performed in a separate PR (if at all). Just wanted to put it out there.

[toshipp]

Would you like me to squash the changes before merging? There is some back-and-forth in those commits that won't provide much value from historical perspective.

Ok. I'll notify you to ready to squash.

There's one more thing that struck me while working with the CI manifests: there's a discrepancy between parameter names: lvmd.socketName vs node.lvmdSocket. Shouldn't the latter be node.lvmdSocketName? I realize this is a non-backwards compatible change and should be performed in a separate PR (if at all). Just wanted to put it out there.

Agreed. BTW, I think this PR already breaks backward compatibility because this changes lvmdSocket value, so if someone tweaks volumeMounts, topolvm-nodes may not work properly. Therefore, I intend to release this feature as major update, I think the renaming can be done in this PR too.

@daichimukai Do you have any concerns about it?

[toshipp]

I thought about it for a while, but I think it would be better to separate the PRs to make the intent clearer.

[toshipp]

@rkrzewski
I have no other comments except this, please fix this and squash them.

[daichimukai]

Now the changes (with deletion of trailing white space) look good to me. I'll approve PR when a squashed commit is pushed.

I have no objection on changing node.lvmdSocket to node.lvmdSocketName in the new PR :)

[rkrzewski]

I've squashed the changes to a single commit and amended the commit message. If you have any further suggestions, please let me know.

[toshipp]

LGTM. You did a great job!

[daichimukai]

LGTM. Thanks @rkrzewski

[rkrzewski]

Thank you, I really enjoyed working on this :)

I'll post the PR for renaming node.lvmdSocket to node.lvmdSocketName as discussed.