Ignore Apiversion, use Kind and Name only to search for original resource on update/delete#2976
Ignore Apiversion, use Kind and Name only to search for original resource on update/delete#2976bacongobbler merged 1 commit intohelm:masterfrom HotelsDotCom:respect-apiversion-change
Conversation
…n, use Kind and Name only Kind can change it's APIVersion with time, such as "Deployment" which was "extensions/v1beta1" and then migrated to "apps/v1beta1" in Kube 1.6. Name, Kind and optinally Namespace are used to find relevant Info object for a original resource which is being upgraded, it safe enough, as it is uniquely identifies an object.
|
I think this should work. I do want to run it by @adamreese though to make sure that this wouldn't have unintended side effects I haven't taken into account. |
|
tentatively scheduling this for the v2.6.2 release. |
| // isMatchingInfo returns true if infos match on Name and GroupVersionKind. | ||
| func isMatchingInfo(a, b *resource.Info) bool { | ||
| return a.Name == b.Name && a.Mapping.GroupVersionKind == b.Mapping.GroupVersionKind | ||
| return a.Name == b.Name && a.Mapping.GroupVersionKind.Kind == b.Mapping.GroupVersionKind.Kind |
There was a problem hiding this comment.
Shouldn't we change this to a.Mapping.GroupVersionKind.GroupKind() == b.Mapping.GroupVersionKind.GroupKind() then? We still want to ensure that two resources that are the same "Kind" (e.g. "Deployment") come from the same "Group" (e.g. "k8s.io"). For example, aggregated APIs like OpenShift could come up with their own Deployment object that acts differently from Kubernetes' Deployment object.
There was a problem hiding this comment.
No, we can't. Otherwise Deployment in ‘extensions/v1beta1’ won't be matching Deployment in ‘apps/v1beta1’, which is the whole point of this PR
|
I'm going to say that I do not agree with this patch. There is a movement towards aggregated APIs in Kubernetes where resources are uniquely identified by group/kind, so only checking the resource by Kind will not be unique enough in the future. Network policies coming from For example, let's assume in a future Kubernetes release, Pods now has released a v2 resource, so old manifests look like this: And new manifests look like this: Should these two types match? No. They are not directly translatable and should not be able to upgrade from one to another without a Now that all being said, we should absolutely display a more informational error and explain to the user why the upgrade failed (and how to fix it moving forward using |
|
Given that this is not a regression that was introduced in v2.6, I'm going to remove this from the v2.6.2 milestone. If a future patch comes up then we can triage it into a future release. |
|
Ultimately, it is up to APIServer to say if resource is upgradable or not. Should there be no in-kube conversion supported between v2/pod and v1/Pod, helm would receive an authoritative APIServer error. All helm needs to do is not not stand in the way. kubectl apply copes with extensions/v1beta1 and app/v1beta1 update just fine. Kube 1.8 highlight is promotion of various workloads to an app/v1beta2 , people will be updating their charts, it would be sad if helm falls short. Please reconsider. |
|
I'm agreeing with you, but I don't believe this specific change (changing |
|
Helm on the other hand stores known state as a separate .Release entity (encoded in configmap), therefore there is a need to find a match between every resource in a new Release and a resource in previously stored Release (create or delete is attempted if no match found). This search have no option, but to tolerate apiVersion changes to have a chance to find a match. Unless Helm changes it's storage layout to uniquely track individual resources, lets say by their UIDs, I can see no way around it, but to relax matching condition. |
|
Now I understand... So ultimately this is a limitation of tiller being unable to track the deployed resources in kubernetes as described in #2070. Thank you for clarifying. I should've known better. I've already gone and cut v2.6.2 (release notes are coming soon) so this'll have to go into v2.7. Kubernetes 1.8 is out now so it shouldn't be much longer before we have it out. |
|
It just occurred to me, that even if Helm changes storage layout and starts to track individual resources it created in the past, it would still be needed to find a match between resources created in the past and resource definitions user provided in a new .Release. In other words this:
is not true, I don't know what I was thinking about :) It should say "Even if Helm changes it's storage layout..." |
Kind can change it's APIVersion with time, such as "Deployment" which was
"extensions/v1beta1" and then migrated to "apps/v1beta1" in Kube 1.6.
Name, Kind and optinally Namespace are used to find relevant Info
object for a original resource which is being upgraded, it safe enough,
as it is uniquely identifies an object.
Fixes: #2885