Update Helm chart - #280
Conversation
✅ Deploy Preview for dranet canceled.
|
|
Hi @OguzPastirmaci. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
/cc @fmuyassarov |
|
/ok-to-test |
fmuyassarov
left a comment
There was a problem hiding this comment.
Thanks for the fixes @OguzPastirmaci . It is true that directory mounting was missed as part of #115 implementation and I don't see any reason to not replicate it to Helm charts. +1 from me.
However regarding the template validation, in my experience bash script can become difficult to maintain over time and it also doesn't provide much in terms of structured assertions. I do think validation is something we should have but maybe it would be worth checking if something like https://github.com/helm-unittest/helm-unittest would be a better fit? It doesn't have to be done in this patch. I’m completely fine to see it as a follow up. And if you would rather not spend time on it that's completely fine, I can take a look into it.
Thanks for the review, @fmuyassarov. I addressed the comments and removed the shell verifier. I will start a separate discussion about Helm chart validation. |
|
/lgtm |
|
@aojea @gauravkghildiyal could you folks help with approval here if LGTY too? |
gauravkghildiyal
left a comment
There was a problem hiding this comment.
Thanks @OguzPastirmaci
One small question and this should be good to go.
|
Forgot to comment, if you are going to make another push, I would kindly ask to perhaps squash the last commit (9b906a1) into the first one |
7b37a06 to
dbc84b5
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fmuyassarov, gauravkghildiyal, OguzPastirmaci The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
install.yamlmounts/var/run/dranetfrom the host. The Helm chart did not include this mount.The Dranet binary stores its bbolt database at
/var/run/dranet/dranet.dbby default. Without the host mount, the database stays in the container file system. Kubernetes removes this database when it replaces the Dranet pod.Dranet stores prepared pod device configuration in this database. The NRI hooks need this configuration after a Dranet restart.
This PR mounts the host
/var/run/dranetdirectory in the Dranet container. The host path usesDirectoryOrCreate. The database now survives Dranet pod replacement.This PR also:
dbPath,profileProvider,webhookURL, andfeatureGates.webhookas a cloud provider hint.Which issue(s) this PR is related to:
N/A
Special notes for your reviewer:
All new argument values are optional. If users omit a value, the Dranet binary uses its built in default.
Set
args.dbPathto an empty string to disable database persistence. If users set a custom database path, they must place it under/var/run/dranet.An older chart stores the database in the container file system. Dranet cannot move this database to the new host mount during an upgrade.
Before the first upgrade, stop pods that use Dranet managed devices. Wait for the pods to terminate. Then upgrade the Helm release.
Does this PR introduce a user-facing change?