Helm chart : expanderPriorities in values are object but string expected in template

[eddycharly]

In values.yaml it looks like expanderPriorities should be passed as an object

autoscaler/charts/cluster-autoscaler/values.yaml

Line 126 in 04b2ae8

expanderPriorities: {}

But the template expects it to be a string

autoscaler/charts/cluster-autoscaler/templates/priority-expander-configmap.yaml

Line 11 in 04b2ae8

{{ .Values.expanderPriorities | indent 4 }}

[brennerm]

For anyone encountering this. A simple workaround is to use a YAML literal:

expanderPriorities: |-
  10:
    - asg10
  0:
    - asg0

[eddycharly]

@brennerm why not change the template to:

 {{- toYaml .Values.expanderPriorities | nindent 4 }}

instead ?

[brennerm]

@eddycharly Cause the chart expects the value to be a string. Try passing the YAML object and it'll fail.

[eddycharly]

@brennerm yeah sure, i meant changing the template (not the values).

That is, changing

autoscaler/charts/cluster-autoscaler/templates/priority-expander-configmap.yaml

Line 11 in 04b2ae8

{{ .Values.expanderPriorities | indent 4 }}

with:

 {{- toYaml .Values.expanderPriorities | nindent 4 }}

And then use an object in the values instead of a string literal:

expanderPriorities:
  10:
    - asg10
  0:
    - asg0

I feel like an object would be more natural than a string literal in this case.

[brennerm]

Ahh, yeah that would fix it but would introduce a breaking change. 👍

[eddycharly]

Unfortunately yes.
Not sure if it is possible to check if the value is a string in the template and then adapt to support both string or map.
It would allow to change the template while preserving backward compatibility.
Will check that tomorrow.

[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

[stevehipwell]

Either the docs or the template needs updating so this is consistent.

[stevehipwell]

/remove-lifecycle rotten

[jbilliau-rcd]

much thanks @brennerm , i've spent 2 hours trying to figure out wtf is the syntax for this since it's not documented. Your suggestion of the YAML literal fixed it for me.

[bwhaley]

Just noticed this issue after opening the PR! Any chance of cutting a backward release to fix this? My use case is to use the chart with the Terraform Helm provider. The code looks like this:

variable "expander_priorities" {
  description = "If scaling_strategy is set to 'priority', you can use this variable to define cluster-autoscaler-priority-expander priorities. See: https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/expander/priority/readme.md"
  type        = map(list(string))
  default     = {}
  # Example:
  #
  # expander_priorities = {
  #   10 = [".*"]
  #   50 = ["foo.*"]
  # }
}

variable "priority_config_map_annotations" {
  description = "If scaling_strategy is set to 'priority', you can use this to specify annotations to add to the cluster-autoscaler-priority-expander ConfigMap."
  type        = map(string)
  default     = {}
}

resource "helm_release" "k8s_autoscaler" {
  repository = "https://kubernetes.github.io/autoscaler"
  name       = local.release_name
  chart      = local.chart_name
  version    = var.cluster_autoscaler_chart_version
  namespace  = local.chart_namespace

  values = [yamlencode(local.chart_values)]

  depends_on = [
    aws_eks_fargate_profile.cluster_autoscaler,
    null_resource.dependency_getter,
  ]
}

locals {
  chart_values = merge(
    {
      ... <snip> ...
      expanderPriorities           = var.expander_priorities
      priorityConfigMapAnnotations = var.priority_config_map_annotations

      extraArgs = merge(
        {
          expander                    = var.scaling_strategy
          balance-similar-node-groups = "true"
        },
        var.container_extra_args,
      )
    },
      ... <snip> ...  
  )
}

It's a nice clean interface with types for expander_priorities and priority_config_map_annotations. To make it work with this work around, I think I'll have to make expander_priorities a string and pass in the workaround as a heredoc, explain it in the docs, etc. e.g.

variable "expander_priorities" {
  description = "If scaling_strategy is set to 'priority', you can use this variable to define cluster-autoscaler-priority-expander priorities. See: https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/expander/priority/readme.md"
  type        = string
  default     = "
  # Example:
  #
  # expander_priorities = <<EOF
  #   10:
  #     -  ".*"
  #   50: 
  #     - "foo.*"
  # EOF
}

I think this will work, but not sure yet, and anyway it's not ideal as it will be easy to pass in the wrong value.

WDYT about toYaml?

[stevehipwell]

I do this in terraform at the moment and just capture it as a string.

locals {
  expander_config = <<-EOT
    100:
      - .*-spot-.*
    10:
      - .*
  EOT
}

I stand by my comment above that either the docs or the implementation need changing; it wouldn't be too complicated to change the implementation to support both formats (see example).

[bwhaley]

Good point, @stevehipwell! Updated my PR with your idea.

I hope this will be merged. I burned a couple of hours on this issue.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[bwhaley]

/remove-lifecycle stale

[gjtempleton]

I believe this can be closed given your PR has been merged and the new version of the chart released, any objection @bwhaley ?

[bwhaley]

Yup, agreed!

[gjtempleton]

/close

[k8s-ci-robot]

@gjtempleton: Closing this issue.

In response to this:

/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.