Allow unsetting values in cmdOptions

[ankon]

By checking the value to not be an "invalid" kind it is possible to use the chart and unset
a value after installation:

helm install descheduler descheduler/descheduler-helm-chart ... --set cmdOptions.dry-run=
helm upgrade descheduler ... --reuse-values --set cmdOptions.dry-run=null

---

Basically I ended up doing exactly the install step from the example above, and when I wanted to unset that value found that I wasn't able to. Now I could just re-install descheduler I suppose, but it felt wrong to do that. Reading https://helm.sh/docs/chart_template_guide/values_files/#deleting-a-default-key it seemed that setting a value to null should do what I want though:

If you need to delete a key from the default values, you may override the value of the key to be null, in which case Helm will remove the key from the overridden values merge.

What I'm not sure is whether this is intended only for the default values ... but I also couldn't find a clear best practice pattern for unsetting values in general. :)

This change seems to work nicely when testing with helm template:

# Set the value to appear as `--dry-run`
helm template descheduler --set cmdOptions.dry-run=
# Unset the value and remove the `--dry-run`
helm template descheduler --set cmdOptions.dry-run=null
# Testing: Renders as `--dry-run` followed by `foo`
helm template descheduler --set cmdOptions.dry-run=foo
# Testing: Renders as `--dry-run`. Might be surprising for users.
helm template descheduler --set cmdOptions.dry-run=false
# Testing: Renders as `--dry-run` followed by `true`
helm template descheduler --set cmdOptions.dry-run=true

I think the last two cases could possibly be improved further in terms of user-that-didnt-read-much-documentation expectations as well, at the expense of making this part of the template even uglier.

Note: This is pretty much my first venture into helm and charts, so please take all this with a grain of salt!

[k8s-ci-robot]

Hi @ankon. 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 /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.

[damemi]

nit: could you fix the indentation for these?

Otherwise, looks good to me. I'll leave the lgtm for another reviewer (@ingvagabund or @seanmalloy)
/approve

[ankon]

Ha. I tried finding some form of guidelines how to actually do the indentation here, but I'm very much unsure. The problem for me is that it is a nested if, so in a way I want to indent for that, but at the same time this - {{ $value | quote }} seems like it must stay exactly where it is right now. Moving the ifs to the left gives this:

diff --git a/charts/descheduler/templates/cronjob.yaml b/charts/descheduler/templates/cronjob.yaml
index 3bdb9be5d..75ffb89e9 100644
--- a/charts/descheduler/templates/cronjob.yaml
+++ b/charts/descheduler/templates/cronjob.yaml
@@ -39,11 +39,13 @@ spec:
                 - "--policy-config-file"
                 - "/policy-dir/policy.yaml"
                 {{- range $key, $value := .Values.cmdOptions }}
+                {{- if ne (kindOf $value) "invalid" }}
                 - {{ printf "--%s" $key | quote }}
                 {{- if $value }}
                 - {{ $value | quote }}
                 {{- end }}
                 {{- end }}
+                {{- end }}
               volumeMounts:
                 - mountPath: /policy-dir
                   name: policy-volume

Looks off to me, WDYT?

[damemi]

Ah right, you can't move those list values true. In that case I think it looks fine

[ingvagabund]

Any way to unit test this?

[seanmalloy]

There is a helm test command. We don't currently run this as part of CI. Right now we only run helm lint I created #440 to track this.

[stevehipwell]

Could I suggest that IMHO it's not idiomatic to have a control statement such as {{- if }} nested deeper than the resulting output as it makes it harder to grep what's going on that in your example above. I'd definitely align the the {{- range }} and subsequent {{- if }} at the same level as they are part of a single logical block; but I'd possibly tab them out one level.

[ankon]

Part of the issue here is probably that the control statements should start their indentation independently from the data parts. But then one would have to read the file line by line:

              args:
                - "--policy-config-file"
                - "/policy-dir/policy.yaml"
{{- range $key, $value := .Values.cmdOptions }}
  {{- if ne (kindOf $value) "invalid" }}
                - {{ printf "--%s" $key | quote }}
    {{- if $value }}
                - {{ $value | quote }}
    {{- end }}
  {{- end }}
{{- end }}

This gives reasonable identation for the control flow, with the odd "yaml is unfortunately indentation-sensitive and templating it is annoying" scream. :)

[damemi]

/ok-to-test

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ankon, damemi

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 [damemi]

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

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ankon, damemi

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 [damemi]

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

[ingvagabund]

If $value is empty, what does kindOf $value return?

[ankon]

I couldn't find any clear documentation, so had to try it out:

Value as set in values.yaml	Result of kindOf $value inside that loop
number: 5	float64
string: "value"	string
empty: ""	string
emptyObject: {}	map
nullValue: null	invalid

[seanmalloy]

I'll try to review this soon.
/cc

@stevehipwell if you have time please take a look at this. Thanks!

[stevehipwell]

I'm trying to get my head around the "correctness" of this change. I can see that it works for the use case when the --reuse-values flag is set but I'm not sure if is a valid requirement to support this flow at a chart level when it looks like a Helm issue. The documentation linked in the PR is specifically for values files and doesn't have relevance here.

In summary I'm not in favour of merging this as it looks like a Helm limitation and not a chart issue.

[ankon]

I can buy into "helm limitation", but for me as a user the question remains of what to do now -- my chart is installed already, and I need to get rid of that dry-run now.

One other approach that sounded interesting but I didn't go for was to see whether the specific flag I'm having an issue with could take an explicit value and/or provide a "no" variant. In that case when upgrading my chart I might be able to specify the "don't dry-run anymore" explicitly.

[stevehipwell]

@ankon is there a reason why you can't re run with the original values without using the --reuse-values flag?

Have you tested what happens when you use this pattern on a value defined in values.yaml, for example --set cmdOptions.v=null which should be seen in the chart as cmdOptions.v: 3 with that set. I suspect that this wouldn't work and would result in no --v arg set.

In addition to the above concern we would also need to apply this too all chart values and test them all as I don't think cmdOptions should have inconsistent behaviour. Which would still leave this chart inconsistent with other charts.

[ankon]

@ankon is there a reason why you can't re run with the original values without using the --reuse-values flag?

Basically it's a question of trying to make our instructions as safe-to-use as possible, even with the assumption that someone updates only parts of them. What I want to avoid at all is a situation where someone executes the documentation, and then introduces a bigger issue.

In this specific case: The descheduler chart is added to the cluster when the cluster is just created, and at that point sizing/expected load isn't yet fully understood. So there's a desire to have the descheduler run in an "advisory" role only initially, by having the documentation specify to install it with --dry-run, and watch the output while scaling up the load based on performance tests etc.

At a later point the instructions for "bring it live!" should (ideally!) only specify "and now that everything is fine, and you checked descheduler logs for a couple of days, so remove --dry-run and let it do its job". The person executing these may or may not be the same as the one that set up the cluster initially.

If someone were to adjust the configuration when setting it up to add another flag, that other flag should not need to be mentioned in the "bring it live" documentation.

I suspect what I do need to look into is actually persisting the values.yml (or some form of overrides) somewhere separately, so that I can specify "remove setting from yml, and then run this command to update". I was hoping to avoid that by being able to use a --reuse-values --set cmdOptions.dry-run=not-anymore-please.

[stevehipwell]

@ankon the process we use for this exact scenario is to persist both the chart version and the input parameters as both are relevant when changing either. You could manage this via the helm get command to retrieve the current values and version.

I'm going to downvote this PR as I don't think it's safe in it's current state, the --set cmdOptions.v=null being my example of this. Moving forward I think the problem that you're trying to solve is outside the scope of this chart.

[ankon]

@stevehipwell Thanks for taking the time to look at this. I'll see whether an approach as you mentioned with helm get could work for us, and if not then persist indeed the whole configuration somewhere in the infra repository.

[stevehipwell]

@ankon if you're happy with that approach you might want to close the PR?

As an aside you might want to look into automation around your problem; specifically "config as code" where you are always getting a desired state. This could be achieved via standard CI/CD or git ops (we currently use a Flux + Flux Helm Operator config that does incremental change management).

[ankon]

"config as code"

We actually do that for the stuff after the cluster exists -- currently using a home-grown tool and Travis, eventually aiming for helm.

The problem is the chicken-eggness of getting the cluster up in the first place. Happens to rarely, has too many failure modes -- so xkcd says to not waste too much time to automate it :)