search

ovn-org / ovn-kubernetes

A robust Kubernetes networking platform
https://ovn-kubernetes.io/
Apache License 2.0
819 stars 339 forks source link

Don't remove all annotations when patching pods #2278

Closed aojea closed 3 years ago

aojea commented 3 years ago

There is already an object in ovn-kube that allows to patch the Pod keeping current annotations and, updating or adding the new annotation if necessary.

https://github.com/ovn-org/ovn-kubernetes/blob/227539ee442e84f504c3d545cb26bbdce65bee6f/go-controller/pkg/kube/annotator.go#L96-L103

However, it is only used in one place

https://github.com/ovn-org/ovn-kubernetes/blob/227539ee442e84f504c3d545cb26bbdce65bee6f/go-controller/pkg/node/node_smartnic.go#L157

Other places are calling SetAnnotationsOnPod directly:

go-controller/pkg/kube/annotator.go:163:        return pa.kube.SetAnnotationsOnPod(pa.pod.Namespace, pa.pod.Name, annotations)
go-controller/pkg/ovn/pods.go:441:              if err = oc.kube.SetAnnotationsOnPod(pod.Namespace, pod.Name, marshalledAnnotation); err != nil {
go-controller/pkg/cni/cnismartnic.go:49:        err = kube.SetAnnotationsOnPod(pr.PodNamespace, pr.PodName, smartNicAnnotation)

that creates a merge patch with the annotations passed as paremeter https://github.com/ovn-org/ovn-kubernetes/blob/227539ee442e84f504c3d545cb26bbdce65bee6f/go-controller/pkg/kube/kube.go#L69

If my reading is correct, this will wipe out existing annotations on the pod and add only the new one

A strategic merge patch is different from a JSON merge patch. With a JSON merge patch, if you want to update a list, you have to specify the entire new list. And the new list completely replaces the existing list. JSON Merge Patch (RFC )

The goal is:

  1. check that my hypothesis is correct and this is actually removing existing annotations, we can have an unit test for this for SetAnnotationsOnPod
  2. if there is a bug and we are actually removing existing annotations, modify the other callers to make use of the PodAnnotator
astoycos commented 3 years ago

/assign @creydr

creydr commented 3 years ago

/assign

creydr commented 3 years ago

@aojea I added a unit test for the SetAnnotationsOnPod method. According to the testresults it keeps existing annotations and adds the new ones.

aojea commented 3 years ago

If my reading is correct, this will wipe out existing annotations on the pod and add only the new one

my reading was not correct :sweat_smile: Thanks @creydr for checking and verifying it