Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Update helm v2 and v3 to latest #604

Merged
merged 1 commit into from
May 21, 2021
Merged

Update helm v2 and v3 to latest #604

merged 1 commit into from
May 21, 2021

Conversation

fredr
Copy link
Contributor

@fredr fredr commented May 11, 2021

We encountered a bug that was introduced in helm 3.1.2, that when updating a hpa in autoscaling/v2beta2 with the new behavior field, we got this error message:

Upgrade "nginx-ingress" failed: failed to create patch: unable to find api field in struct HorizontalPodAutoscalerSpec for the json field "behavior"

I know helm-operator is in maintenance mode, but I hope this can be accepted since there is no way around this issue. There is probably lots of other bugs fixed in helm that could be good to get in to helm-operator.

Signed-off-by: Fredrik Enestad <fredrik@enestad.com>
@kingdonb
Copy link
Member

These look like appropriate changes for maintenance mode, I don't know if this change can be merged by itself or when we can plan on making another release of Helm Operator. I'm looking at #599 and I think that a release will be necessary soon.

I don't think there's any reason we wouldn't release this change, of course I'm speaking about it before having tested the change in any way. I'd like to avoid making several releases back to back if we can get the job done with a single one.

But if this is urgent for you, and #599 is waiting for 1.22 which will not be ready for maybe a few months yet, maybe it makes sense to do separate releases and get this one out sooner.

Please bear with us while I try to find the way forward, it will help if you can give us an idea of how much testing you've already done on this change. Thanks again for making this contribution!

@fredr
Copy link
Contributor Author

fredr commented May 11, 2021

Hey @kingdonb

If I know that you will eventually release it, I'm happy, and we can roll with a custom build version of this PR until then.

In terms of testing, I have deployed this PR to our lab-cluster, and can there see that we don't encounter that specific error anymore, and so far it have not caused any other havoc.

We use helm v3 only, so the helm v2 path has not been tested more than via the test command.

Let me know if there is anything specific that you want me to test. Otherwise I'll monitor it for a few days and then start rolling it out to our staging clusters.

@hiddeco
Copy link
Member

hiddeco commented May 12, 2021

This is an awesome pull request, many thanks!

While all tests pass and the code looks OK, I am wondering what changed (in client-go?), as this used to not be possible at all due to Helm 2 depending on K8s 1.16.x while Helm 3 moved on to K8s >=1.18.x. The latter introduced an (at the time) incompatible Context to the method signatures which we were unable to work around.

Do you by any chance have any knowledge of this, so that we can keep a reference?

@fredr
Copy link
Contributor Author

fredr commented May 13, 2021

This is an awesome pull request, many thanks!

🙏

Do you by any chance have any knowledge of this, so that we can keep a reference?

I did have to add a context to these calls in this PR, and I had to upgrade helm-2to3 to a later version as they also had calls without context in the version that were previously imported here. But other than that I didn't have to do anything. Maybe something was done upstream? If you have a branch or so where you encountered the problem, I can have a look to see if I can understand what is different.

@fredr
Copy link
Contributor Author

fredr commented May 21, 2021

We've been running this (helm v3) in production for a couple of days now without any problems.

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @fredr 🌻

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants