Autoscaler: cluster-autoscaler helm chart has '-chart' in its name

[llamahunter]

It seems bad form for a chart to include the word 'chart' in its name. It seems worse to then name every object the chart creates with 'chart' in the name. And it seems especially bad to RENAME the chart when moving it from one repo to another (i.e. the helm/stable chart was called 'cluster-autoscaler', not 'cluster-autoscaler-chart'), causing all existing named k8s objects to need to be destroyed and re-created, for no good reason.

Yes, I could set the 'nameOverride' and/or 'fullnameOverride' variables to work around this issue, but I really should not need to do so.

Please rename the chart to 'cluster-autoscaler'.

[gjtempleton]

Hi @llamahunter, thanks for opening the issue.

This is obviously not ideal, however as mentioned in the PR to change the chart name #3393, there are technical reasons for this change having been made as the releases conflicted with existing releases for the cluster autoscaler itself. The chart-releaser action hasn't found a solution for this yet https://github.com/helm/chart-releaser/issues/65. Despite us using a manual process for updating the index.yaml for now, we're still using the chart-releaser action to publish the binaries as releases.

I'm happy to discuss how we can work around the limitations to potentially get back to releasing the chart under the name cluster-autoscaler, however this isn't a change that was made for no reason.

[llamahunter]

I think changing the chart versioning is the source of the problem, no? Why did you go from 8.0.0 to 1.0.2? Why not just continue on with the same name and version scheme, since the chart is almost entirely unchanged? That is what happened with other charts that moved out of helm/stable. No duplicate tags that way.

[gjtempleton]

We also don't want people thinking that a new release of the Cluster Autoscaler itself has been cut when it's only a new cut of the chart.

I have an idea of how to solve this by naming the releases seperately to the helm chart name, but we won't be able to use the current releaser action to achieve that.

[llamahunter]

If that's the case, why in the former chart location did you go from 7.3.4 to 8.0.0 with NO change in the chart or autoscaler??
https://github.com/helm/charts/commit/6ed964a07209f4614c8cd8a952cec83305a79b7f

[llamahunter]

In any event, changing the name of _every_ k8s object created by this chart to now include the '-chart' suffix seems like a huge migration headache for users, especially since virtually nothing else has changed.

[gjtempleton]

I understand the pain point you're pointing out, I'm just trying to make clear the reasons for the change being made, and a potential path forward to solve it along with the constraints that has to work within.

If other people want to come up with a solution that works within those constraints, that would be great, I'd be more than happy to review it. Otherwise, as mentioned above, I have an idea of how to solve this but I can't make any promises on timescales.

I wasn't involved with the chart when it was part of helm/charts except as a user, however, a major version bump to signify a change from actively maintained to deprecated doesn't seem unreasonable to me.

[llamahunter]

I think the code in _helpers.tpl could be changed to strip off the '-chart' suffix from the .Chart.Name, or there could be a new variable introduced called correctedChartName with a default of 'cluster-autoscaler' used with higher priority than .Chart.Name if it is set.,

e.g.

in values.yaml

# Fix the string used to name all created k8s resources so that it matches legacy deployments and does
# not contain unnecessary information (e.g. the '-chart' suffix).  Set to empty string if you want to use the
# actual .Chart.Name in your helm deployments
correctedChartName = "cluster-autoscaler"

in _helpers.tpl

{{/*
Expand the name of the chart.
*/}}
{{- define "cluster-autoscaler.name" -}}
{{- $correctedChartName := default .Chart.Name .Values.correctedChartName -}}
{{- default (printf "%s-%s" .Values.cloudProvider $correctedChartName) .Values.nameOverride | trunc 63 | trimSuffix "-" -}}
{{- end -}}

{{/*
Create a default fully qualified app name.
We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
*/}}
{{- define "cluster-autoscaler.fullname" -}}
{{- if .Values.fullnameOverride -}}
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}}
{{- else -}}
{{- $correctedChartName := default .Chart.Name .Values.correctedChartName -}}
{{- $name := default (printf "%s-%s" .Values.cloudProvider $correctedChartName) .Values.nameOverride -}}
{{- if ne $name .Release.Name -}}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}}
{{- else -}}
{{- printf "%s" $name | trunc 63 | trimSuffix "-" -}}
{{- end -}}
{{- end -}}
{{- end -}}

[llamahunter]

Like I said earlier, I could set the nameOverride and fullnameOverride variables to accomplish nearly the same effect, but it would require manually managing the cloudProvider aspect of the name. Better to make this default behavior fully automatic for legacy chart users (which, at this point, I am expecting is 99+% of the userbase). Also, suffixing every k8s resource with '-chart' just seems ugly.

[stevehipwell]

The -chart post-fix should be able to be removed using the new --release-template flag on the _chart-releaser_ command (I'm 99% sure the action allows the pass through) while keeping the tag pattern.

[gjtempleton]

Brilliant, thanks very much for pointing it out @stevehipwell!

I'll get on trying that out this evening.

[stevehipwell]

Sorry, this action doesn't have a generic pass through for flags, I just checked and opened an issue to get the action updated.

https://github.com/helm/chart-releaser-action/issues/37

[llamahunter]

What's the status on renaming the chart not to have '-chart' in the name?

[stevehipwell]

@llamahunter I'd assume the blocker is still helm/chart-releaser-action#37 as the issues hasn't been closed and there hasn't been a release.

[llamahunter]

So is everyone just suffering through the chart rename of all the resources? Should I create a PR for https://github.com/kubernetes/autoscaler/issues/3475#issuecomment-683928136 in the meantime to work around the naming issue?

[stevehipwell]

@llamahunter I'd suggest following up on helm/chart-releaser-action#37 as this would allow the chart to be named correctly.

[stevehipwell]

@gjtempleton I've opened a PR at https://github.com/kubernetes-sigs/descheduler/pull/436 to rename the _descheduler_ chart using the latest helm/[email protected] release which should be of interest.

@llamahunter I think this is the way to go.

[gjtempleton]

Thanks for the heads up @stevehipwell, much appreciated.

@llamahunter does #3679 look like it'll achieve what you're looking for?

[llamahunter]

Yes... commented in PR. Thx!

[gjtempleton]

/reopen

Won't be fully resolved until #3716 is merged as this will publish it to this repo's index.yaml

[k8s-ci-robot]

@gjtempleton: Reopened this issue.

In response to this:

/reopen

Won't be fully resolved until #3716 is merged as this will publish it to this repo's index.yaml

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.

[gjtempleton]

3716 has been merged, I've validated locally, but will keep this issue open until you've confirmed this has unblocked you @llamahunter

[stevehipwell]

@gjtempleton the chart is showing up correctly for me.

It would be useful if you could add the autoscaler repo into https://artifacthub.io/.

[gjtempleton]

Yeah, going to grab some time this evening and resolve #3638 and #3571

[gjtempleton]

@stevehipwell with a helping hand this is now published on Artifact Hub, however, you may want to be aware of the reason I've had to raise https://github.com/kubernetes/autoscaler/pull/3719 as I notice the descheduler repo is preparing to switch over the naming as well, hopefully you can avoid the need to go back and mark the old release as deprecated as I've had to do here.

[mjtv]

3716 has been merged, I've validated locally, but will keep this issue open until you've confirmed this has unblocked you @llamahunter

I work with llamahunter. This unblocks us. Thanks so much!

[gjtempleton]

Thanks for confirming!

/close

[k8s-ci-robot]

@gjtempleton: Closing this issue.

In response to this:

Thanks for confirming!

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