openebs / lvm-localpv

Dynamically provision Stateful Persistent Node-Local Volumes & Filesystems for Kubernetes that is integrated with a backend LVM2 data storage stack.
Apache License 2.0
235 stars 92 forks source link

fix(localpv): ensure kubeletDir trailing slash #297

Closed mdonoughe closed 3 days ago

mdonoughe commented 2 months ago

Pull Request template

Please, go through these steps before you submit a PR.

Why is this PR required? What issue does it fix?: This PR avoids subtle problems when installing on microk8s or other k8s distributions with a different kubelet path. If the user types the path into the values file without a trailing slash, all the openebs pods would be running and even generate PVs and LVs, but no pods using the PVs would start because the CSI plugin was not installed correctly on the node.

What this PR does?: The lvm-node template is updated to ensure that there is always a slash between the kubeletDir value and subdirectories that are appended. Additionally, the paths are now quoted, in case they contain some characters that would cause problems when interpreting the rendered template as YAML.

Does this PR require any upgrade changes?: No. The user sets the same value, and the current values with trailing slashes produce the same output as before.

If the changes in this PR are manually verified, list down the scenarios covered::

helm template . -s templates/lvm-node.yaml --set lvmNode.kubeletDir=/var/lib/kubelet/ and helm template . -s templates/lvm-node.yaml --set lvmNode.kubeletDir=/var/lib/kubelet now produce the same result.

Any additional information for your reviewer? : Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

tiagolobocastro commented 2 months ago

@mdonoughe could you please sign your commit? https://github.com/openebs/lvm-localpv/blob/develop/CONTRIBUTING.md#sign-your-work

Abhinandan-Purkait commented 1 month ago

@mdonoughe Thanks for the contribution. This PR would be reviewed and would eventually be cherry-picked into the next helm release once we achieve the approval quorum.

dsharma-dc commented 2 weeks ago

@Abhinandan-Purkait Can this be merged now? I see the label hold was put on this earlier.

Abhinandan-Purkait commented 2 weeks ago

@dsharma-dc no, merging this will create a helm release. We will cherry-pick this to a single PR before release.

niladrih commented 3 days ago

Closing this PR as this change has been included in #322. cc: @abhilashshetty04 @dsharma-dc