pulumi-kubernetes icon indicating copy to clipboard operation
pulumi-kubernetes copied to clipboard

HelmRelease: Path is used as the chart name, triggering unneeded releases

Open elthariel opened this issue 3 years ago • 10 comments

What happened?

When using a local chart in an HelmRelease, the chart's path is used as the chart's name, and is then stored in the state.

This means that, even if the chart and/or value haven't changed, if you move the chart, it will trigger a new release for nothing.

I'm working on a tool that is using the automation api and that I'm packaging as a .pex file (a self-extracting and executing python archive). When it's run, it's decompressed in a temporary folder with a random name. The charts are packaged in that archive, which means everything I run the tool, a new release is created, eating a lot of space on the kube cluster (or replacing olders releases with identical ones, depending on the history configuration)

Steps to reproduce

  • Create a local chart, or fetch one from a registry and unpack it
  • Create a HelmRelease with this chart (chart="/path/to/chart")
  • Run pulumi up
  • Move the chart, or rename the chart's folder.
  • Update the HelmRelease resource creation
  • Run pulumi up

Expected Behavior

This should be a no-op as no meaningful change was made

Actual Behavior

  • The provider considers the resource has changed (update [diff: ~chart])
  • A new helm release is stored on the cluster (as a secret of type helm.sh/release.v1)

Output of pulumi about

As I'm using the automation API, I don't have a Pulumi.yaml file. I'd love for someone to point me to some doc about how to do that :)

CLI          
Version      3.43.1
Go Version   go1.19.1
Go Compiler  gc

Host     
OS       debian
Version  11.5
Arch     x86_64

Pulumi locates its logs in /tmp by default
warning: Failed to read project: no Pulumi.yaml project file found (searching upwards from /home/lta/code/projects/mainstage/main/ops). If you have not created a project yet, use `pulumi new` to do so: no project file found
warning: A new version of Pulumi is available. To upgrade from version '3.43.1' to '3.48.0', visit https://pulumi.com/docs/reference/install/ for manual instructions and release notes.

Additional context

I hope there's an obvious workaround I haven't found (outside of using relative paths which isn't an option ATM) If not, I'd love some pointers on how to fix this. This isn't really a blocking problem, but it's been annoying me for months, so my annoyance is reaching a sufficiently high level for me to dig into this

Contributing

Vote on this issue by adding a 👍 reaction. To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

elthariel avatar Nov 28 '22 20:11 elthariel

Hi @elthariel -- could you add an example of the helm.Release constructor you are running? If I understand correctly, you might be able to specify the name of the resource rather than using auto-naming to avoid this issue.

lblackstone avatar Nov 29 '22 23:11 lblackstone

The exact call is a bit hard to extract, as it's embedded into an internal framework, so it's a bit split overall. But I'll compute you a relevant call:

        return k8s.helm.v3.Release(
            self.name,
            args=k8s.helm.v3.ReleaseArgs(
                # A static sting
                name=self.name,
                # The source of the issue :)
                # /tmp/pex_02hfyhyl/pex-lsbfztnblfNIgJt4c41om7o2RDA/lib/infra/resource/kube/charts/ingress
                # The first part of the pex changes are every run in dev mode, and at every build otherwise
                chart=self._chart,            
                namespace=self.kube_namespace,
                value_yaml_files=self._values_file_as_file_assets(),
                values=self.values(),
            ),
            opts=self.resource_opts(),
        )

# The resource options are basic
pulumi.ResourceOptions(
            provider=provider,
            depends_on=self.depends_on(),
        )

elthariel avatar Dec 01 '22 13:12 elthariel

I might not be understanding your message correctly, but it seems to me that I don't use auto-naming.

My understanding of the problem is that the chart's name is it's path, as a such is stored in the state. And the change of the chart name triggers an update of the resource. This makes perfect sense when using a chart from a repository, but a little bit less when using a locally developped chart, where the name is the path. In that case, it'd be better to get the actual name from the Chart.yaml.

My go skills are quite bad and I haven't looked enough into the helm API, so I'm not sure if this is a limitation of their API, or something we could work around in this provider :-/

elthariel avatar Dec 01 '22 13:12 elthariel

Ah, ok. Thanks for the code and clarification. This seems like an issue we could work around in the provider.

lblackstone avatar Dec 01 '22 16:12 lblackstone

@lblackstone Any pointers on the approach to fix this ? I can give it a try, but I don't really known where to start

elthariel avatar Dec 16 '22 09:12 elthariel

Sorry for the delayed response. https://github.com/pulumi/pulumi-kubernetes/blob/v3.23.1/provider/pkg/provider/helm_release.go#L1124 is the relevant code that sets the name. It looks like we aren't doing anything special with it; the value is plumbed directly to the helm client.

I noticed that our helm.v3.Chart resource has separate arguments for path and chart for local charts, which seems like a better solution than using chart for both purposes.

Adding that option would involve adding it to the schema, and then updating the client code I linked above to use the path if provided.

lblackstone avatar Jan 05 '23 23:01 lblackstone

Also sorry for the delayed response :)

I've started to look into this issue, but so far I cannot build because of a java_sdk failure:

FAILURE: Build failed with an exception.

* What went wrong:
kotlin/jvm/internal/Intrinsics
[...]
** Exception is:
java.lang.NoClassDefFoundError: kotlin/jvm/internal/Intrinsics
[...]

I don't know much about Java, and honestly would prefer to keep it that way :p The CONTRIBUTING.md file doesn't mention the requirements to be able to build the java sdk. Do you know where I could find those ?

Edit: This was due to a bad version of gradle. I've sent a separate MR to update the CONTRIBUTING.md guide

elthariel avatar Mar 27 '23 22:03 elthariel

So, after trying to implement this as you suggested, it appears that it's not solving my issue. The change in the chart's path attribute also triggers a new release of the chart :sob:. Since the path of the chart contains random character, I still get a new release every run, polluting my namespace with many useless secrets :-/

elthariel avatar Mar 29 '23 14:03 elthariel

Since https://github.com/pulumi/pulumi-kubernetes/pull/2672 the provider looks to the version within Chart.yaml to determine whether the local chart has substantively changed (thus triggering an upgrade). Imagine that the provider simply ignored the chart input property when computing the diff, it would solve this issue of triggering an upgrade when the path is volatile. However, it would create a different problem, that switching to a truly different chart that happens to use the version number would fail to trigger an upgrade.

Now imagine that the provider were to normalize the chart property such that the checked value is drawn from the name field of Chart.yaml. In other words, the checked value of chart and of version would be the values from Chart.yaml, and the diff would be on that basis. Aside from complications related to migration of existing state, this seems like a practical fix. Thoughts on this?

EronWright avatar Jan 25 '24 18:01 EronWright