Consider full GroupVersionKind when matching resources#12581
Consider full GroupVersionKind when matching resources#12581scottrigby merged 2 commits intohelm:mainfrom
Conversation
There was a problem hiding this comment.
Funnily enough, back in 2017, it was determined that Helm should ignore the API Group/Version when comparing objects: #2976
From a through read of that PR (and related issues), I didn't find any reason that made sense to me to make that change (in fact #2976 (comment) IMHO was correct, then and today)
I think this PR makes the logic more correct. But given Helm has been like this for a (very) long time, it is probably worth proceeding with (a lot of) caution (my 2c).
|
Hi, the comment #2976 (comment makes sense, the issue is that when you upgrade a chart that have resources with API group and/or version changes the old resource is not removed but it is not managed by Helm anymore. The old resource is left orphaned in Kubernetes, we found that when we upgraded Traefik chart and they changed the API group from So, I get that if the API group/version is different Helm mus recognise them as different resources, but if a resource was being managed by Helm and in an upgrade operation a resource is not there anymore because it was replaced with a new resource with new API group/version, Helm should do something with the old resource it was managing, right now it just leave it orphaned as it was never part of the chart. |
|
I believe the solution of considering resources to be different if the apiVersion is different is consistent with the behaviour of 'kubectl apply', 'kubectl diff' |
|
For our use case that would be the expected behaviour, the old resource being deleted rather that left orphaned. You can always know if something like this is going to happen by running the commands with dry run and analyse the result. |
|
I labeled this as Helm v4, so at least it won't get forgotten. IMHO it probably should be merged into Helm v3 as a bug fix, but that needs maintainer attention/thought |
|
@MichaelMorrisEst -- could you please e.g. rebase to kick CI off (again). We should merge this for Helm 4 |
scottrigby
left a comment
There was a problem hiding this comment.
@MichaelMorrisEst -- could you please e.g. rebase to kick CI off (again). We should merge this for Helm 4
Overall I agree comparing GroupVersionKind is a good idea now. #13583 added group since this PR, so you can remove that too after the rebase.
Can you also update/write a test to check these conditions? This test function hasn't been updated in many years:
helm/pkg/kube/resource_test.go
Line 27 in b245620
This change shall take Group, Version and Kind from GroupVersionKind into consideration instead of the current behavior of only considering the Kind Closes: helm#12578 Signed-off-by: MichaelMorris <michael.morris@est.tech>
Signed-off-by: MichaelMorris <michael.morris@est.tech>
ab248d3 to
1460ebd
Compare
|
Hi @gjenkins8 , @scottrigby |
|
nice, thanks! |
Well, this has now grave consequences in Helm 4. The right choice was indeed not to take the API version in consideration all along. Because of this PR, Helm now deletes the "old" resource (old API version) and keeps waiting forever on the "new" resource (new API version), which never happens, because the object was the same in etcd all along. We need to revert this PR ASAP This PR is actually the root cause for this issue: #31768 |
|
@matheuscscp I was wrong earlier in my review of this PR. Glad we discussed in today's dev meeting and you got it sorted in #31772. Thanks! |
|
@matheuscscp I was wrong earlier in my review of this PR. Glad we discussed in today's dev meeting and you got it sorted in #31772. Thanks! |
What this PR does / why we need it:
This change shall take Group, Version and Kind from GroupVersionKind into consideration instead of the current behavior of only considering the Kind when matching resources.
This is needed as resources with different apiVersion are being considered the same.
Closes: #12578
Special notes for your reviewer:
If applicable: