Conversation
jknipper
requested review from
BugRoger,
databus23 and
onuryilmaz
as code owners
jknipper
changed the title
| if err != nil { | ||
| return fmt.Errorf("Failed to marshal annotation %v = %v: %s", key, val, err) | ||
| } | ||
| _, err = client.CoreV1().Nodes().Patch(nodeName, types.MergePatchType, data) |
There was a problem hiding this comment.
We need to be super careful regarding the patch type. I thinks this shreds every lable, but the one given. (https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/#use-a-json-merge-patch-to-update-a-deployment) I think we need a strategic merge patch. ObjectMeta.Labels is not tagged with patchStrategy: merge, so that does not cover it.
So, either we fetch all the labels beforehand or generate a proper (non-merge) JSON Patch
There was a problem hiding this comment.
I tested it and it worked that way, eg.
labels:
beta.kubernetes.io/arch: amd64
beta.kubernetes.io/instance-type: c_c2_m2
beta.kubernetes.io/os: linux
ccloud.sap.com/nodepool: payload
failure-domain.beta.kubernetes.io/region: eu-nl-1
failure-domain.beta.kubernetes.io/zone: eu-nl-1b
kubernetes.io/arch: amd64
kubernetes.io/hostname: kks-jan-payload-s7xbf
kubernetes.io/os: linux
kubernikus.cloud.sap/template-version: "6"
node.kubernetes.io/exclude-from-external-load-balancers: "true"
node.kubernetes.io/instance-type: c_c2_m2
topology.cinder.csi.openstack.org/zone: eu-nl-1b
topology.kubernetes.io/region: eu-nl-1
topology.kubernetes.io/zone: eu-nl-1b
It seems only the label itself gets replaced. Otherwise we should have the same problem here I guess: https://github.com/sapcc/kubernikus/blob/master/pkg/util/node.go#L121-L141
| if err != nil { | ||
| return fmt.Errorf("Failed to marshal annotation %v = %v: %s", key, nil, err) | ||
| } | ||
| _, err = client.CoreV1().Nodes().Patch(nodeName, types.MergePatchType, data) |
There was a problem hiding this comment.
Same note regarding patch strategy.
jknipper
changed the title
2 participants
No description provided.