Handle Pod UID updates in PodStore #6964
Conversation
| // From https://pkg.go.dev/k8s.io/client-go/tools/cache#SharedInformer: | ||
| // Because `ObjectMeta.UID` has no role in identifying objects, it is possible that when (1) | ||
| // object O1 with ID (e.g. namespace and name) X and `ObjectMeta.UID` U1 in the | ||
| // SharedInformer's local cache is deleted and later (2) another object O2 with ID X and | ||
| // ObjectMeta.UID U2 is created the informer's clients are not notified of (1) and (2) but | ||
| // rather are notified only of an update from O1 to O2. Clients that need to detect such | ||
| // cases might do so by comparing the `ObjectMeta.UID` field of the old and the new object | ||
| // in the code that handles update notifications (i.e. `OnUpdate` method of | ||
| // ResourceEventHandler). |
There was a problem hiding this comment.
@tnqn I have not been able to reproduce, but looking at the issue that triggered this documentation update, it seems that it would only happen if a watch is interrupted.
There was a similar change in kube-scheduler at some point: kubernetes/kubernetes#91126. The logic was later changed, but I believe the current code still accounts for the fact that a Delete and an Add event can be "merged" as an Update under certain rare conditions.
Now I am wondering: is it ever safe to assume that the update event handler will never observe a change to an immutable field? If oldObj and newObj can have different UIDs, it feels like other fields which are supposed to be immutable can also be changed through an "update" (of course these are 2 distinct objects, but with the same name, so from the point of view of the event handler they are the "same"). Am I over thinking this?
There was a problem hiding this comment.
I think you are not overthinking this. We should estabilish a principle that do not assume objects received from update handler refer to the same objects. I think we didn't encounter issues was because:
- It's a very corner case;
- In most cases we don't care about UID and these immutable fields. I can find one place that may have the problem: but it's unlikely to happen because two Pods having the same name will unlikely have same PodIP but different containerPorts (although we should still fix it.)
antrea/pkg/controller/grouping/group_entity_index.go
Lines 746 to 747 in f3394c5
There was a problem hiding this comment.
I think I identified another possible controller with this issue earlier: #6965
| // From https://pkg.go.dev/k8s.io/client-go/tools/cache#SharedInformer: | ||
| // Because `ObjectMeta.UID` has no role in identifying objects, it is possible that when (1) | ||
| // object O1 with ID (e.g. namespace and name) X and `ObjectMeta.UID` U1 in the | ||
| // SharedInformer's local cache is deleted and later (2) another object O2 with ID X and | ||
| // ObjectMeta.UID U2 is created the informer's clients are not notified of (1) and (2) but | ||
| // rather are notified only of an update from O1 to O2. Clients that need to detect such | ||
| // cases might do so by comparing the `ObjectMeta.UID` field of the old and the new object | ||
| // in the code that handles update notifications (i.e. `OnUpdate` method of | ||
| // ResourceEventHandler). |
There was a problem hiding this comment.
I think you are not overthinking this. We should estabilish a principle that do not assume objects received from update handler refer to the same objects. I think we didn't encounter issues was because:
- It's a very corner case;
- In most cases we don't care about UID and these immutable fields. I can find one place that may have the problem: but it's unlikely to happen because two Pods having the same name will unlikely have same PodIP but different containerPorts (although we should still fix it.)
antrea/pkg/controller/grouping/group_entity_index.go
Lines 746 to 747 in f3394c5
The PodStore uses the UID as the unique key in the indexer, and not the usual namespace/name. Because it is possible for the deletion of a Pod, followed by the creation of a different Pod wih the same name, to be observed as an Update by a client (see https://pkg.go.dev/k8s.io/client-go/tools/cache#SharedInformer), the Update event handler needs to handle this case gracefully by comparing UIDs. Note that this is probably quite an unlikely situation in the case of Pods, but we should still account for this in the PodStore implementation. Signed-off-by: Antonin Bas <[email protected]>
antoninbas
force-pushed
the
handle-uid-updates-in-pod-store
branch
from
0947890 to
5700fe7
Compare
|
@tnqn I rebased the PR, please take another look |
|
/test-all |
antoninbas
added
the
action/release-note
The PodStore uses the UID as the unique key in the indexer, and not the usual namespace/name. Because it is possible for the deletion of a Pod, followed by the creation of a different Pod wih the same name, to be observed as an Update by a client (see
https://pkg.go.dev/k8s.io/client-go/tools/cache#SharedInformer), the Update event handler needs to handle this case gracefully by comparing UIDs. Note that this is probably quite an unlikely situation in the case of Pods, but we should still account for this in the PodStore implementation.