Helm chart limits setting balancing-ignore-label flag to a single label

[sbuzonas]

The snippet in the chart to set extra arguments is in the format of key=value pairs. For arguments that can be passed multiple times, this limits you to only setting one value.

          {{- range $key, $value := .Values.extraArgs }}
            - --{{ $key }}={{ $value }}
          {{- end }}

Switching it from a hash to an array would solve the problem, but would break backwards compatibility and complexity to the existing configuration. I think accepting a separate block of multi use arguments would keep the configuration simple.

extraMultiUseArgs:
  balancing-ignore-label:
    - some-label
    - and-another

[gjtempleton]

Hi @sbuzonas,

Thanks for raising this issue and proposing a solution for it.

What you're proposing sounds like a reasonable approach to me, keeping the complexity of the arrays only where they're needed. If you want to raise a PR to add this functionality I'd be more than happy to give it a review.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

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.

[stevehipwell]

Could this issue get re-opened as it's a pretty major defect in the CA chart considering how many labels need to be ignored by CA to make it actually work?

[gjtempleton]

/reopen

[k8s-ci-robot]

@gjtempleton: Reopened this issue.

In response to this:

/reopen

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.

[stevehipwell]

Thanks @gjtempleton, I'm just writhing a simple PR to solve this.

[gjtempleton]

No problem, give me a ping once it's ready, and I'll happily review it.

Depending on when you raise it, you may need to cherry-pick this commit in as Github actions not running by default for first-time contributors has bitten us. Looking to move to ensure they always run in the SIG meeting this afternoon.

[infa-ddeore]

Thanks @gjtempleton, I'm just writhing a simple PR to solve this.

any chance it will be available in next ~5-10 days?

[stevehipwell]

@infa-ddeore this has already been merged and released.

[gjtempleton]

The fix was implemented by this PR. It should be available as of the release of version 9.10.3 3 days ago, have you tried that version?

[infa-ddeore]

didnt see github merged message, will try the chart, thx

[infa-ddeore]

The fix was implemented by this PR. It should be available as of the release of version 9.10.3 3 days ago, have you tried that version?

it adds _1 and _2 to --balancing-ignore-label arguments, will it work with cluster-autoscaler?

$ helm template --repo https://charts.helm.sh/stable cluster-autoscaler --version 9.10.3 -f /tmp/values.yaml |grep balancing
            - --balancing-ignore-label_1=first-label-to-ignore
            - --balancing-ignore-label_2=second-label-to-ignore

binary looks like require --balancing-ignore-label and not --balancing-ignore-label_1 --balancing-ignore-label_2 and so on.

$ docker run --rm -it --entrypoint /cluster-autoscaler k8s.gcr.io/autoscaling/cluster-autoscaler:v1.21.0 --help |grep balanc
      --balance-similar-node-groups                                        Detect similar node groups and balance the number of nodes between them
      --balancing-ignore-label MultiStringFlag                             Specifies a label to ignore in addition to the basic and cloud-provider set of labels when comparing if two node groups are similar (default [])

[stevehipwell]

@infa-ddeore the chart values explains how this works and the deployment template has the implementation in it. Basically the extraArgs chart value is a map type meaning that a key can only appear once which would not work for an argument called multiple times; so the workaround is for you to suffix the argument with an underscore and anything else to make it unique which will be removed in the template.

I can confirm that this is working correctly for me, could you share the values that you're using?

[infa-ddeore]

my bad, i found the problem, my command was using https://charts.helm.sh/stable repo which doesnt have 9.10.3 chart, however helm didnt error out with chart not found message because my current working dir had cluster-autoscaler folder with old version of chart so helm was using that.

this now looks fine:

helm template --repo https://kubernetes.github.io/autoscaler cluster-autoscaler -f values.yaml --version 9.10.3 | grep balancing
            - --balancing-ignore-label=first-label-to-ignore
            - --balancing-ignore-label=second-label-to-ignore

which helm repo should we use moving forward, https://charts.helm.sh/stable or https://kubernetes.github.io/autoscaler ?

[stevehipwell]

https://charts.helm.sh/stable is deprecated.