Skip to content
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

fix(localpv): ensure kubeletDir trailing slash #297

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

mdonoughe
Copy link

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:

  • Fixes #
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Has the change log section been updated?
  • Commit has unit tests
  • Commit has integration tests
  • (Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
  • (Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

@tiagolobocastro
Copy link

Signed-off-by: Matthew Donoughe <mdonoughe@pm.me>
@Abhinandan-Purkait
Copy link
Member

Abhinandan-Purkait commented May 22, 2024

@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
Copy link
Contributor

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

@Abhinandan-Purkait
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants