Fix creation and deletion of host port maps that would occasionally leave pods without them

[javanthropus]

Fixes: #32805

[joamaki]

Hey, could you add descriptions to the commits that describe what the problem was and how exactly this fixes the issue? It is not clear to me what including the pod UID does here to improve things.

I need to port this fix to the new control-plane (pkg/loadbalancer/experimental) which replaces this code in v1.18, so need to figure out what the root cause is. Do you also have an example scenario of where the problem occurs? Is it in context of stateful sets or is it when the HostPort is handed over from one pod to another?

[javanthropus]

Hey, could you add descriptions to the commits that describe what the problem was and how exactly this fixes the issue? It is not clear to me what including the pod UID does here to improve things.

Done.

Do you also have an example scenario of where the problem occurs? Is it in context of stateful sets or is it when the HostPort is handed over from one pod to another?

I included a manifest and instructions for reproducing the issue in #32805. In short, this can happen any time the HostPort is handed from one pod to another. In my example, it affects pods of a Deployment, but we also saw it affect DaemonSet pods. I chose to use the pod UID in this PR because it could just as easily affect StatefulSet pods where the pod names don't necessarily change. Hopefully the descriptions in the commits help clarify things.

[joamaki]

Hey, could you add descriptions to the commits that describe what the problem was and how exactly this fixes the issue? It is not clear to me what including the pod UID does here to improve things.

Done.

Do you also have an example scenario of where the problem occurs? Is it in context of stateful sets or is it when the HostPort is handed over from one pod to another?

I included a manifest and instructions for reproducing the issue in #32805. In short, this can happen any time the HostPort is handed from one pod to another. In my example, it affects pods of a Deployment, but we also saw it affect DaemonSet pods. I chose to use the pod UID in this PR because it could just as easily affect StatefulSet pods where the pod names don't necessarily change. Hopefully the descriptions in the commits help clarify things.

Thanks! This clarifies it. I don't think including the pod UID will do anything since pods are indexed by name everywhere, e.g. it is not possible to have two copies of a pod with the same name in the system.

I'll take a look at reproducing this issue and implement the fix&test for the upcoming control-plane. Will think about if there's a point in including the pod UID and will get back on that...

[javanthropus]

I don't think including the pod UID will do anything since pods are indexed by name everywhere, e.g. it is not possible to have two copies of a pod with the same name in the system.

That's true, but the events associated with pod state changes are asynchronous and could arrive or be processed out of order. Via debug logs, I saw this happen and cause the removal of the host port map after a new pod was started to replace an old pod. I added the pod identity check in the first commit to guard against this case. When doing so, it seemed prudent to guard against the case of the old and new pods having the same name as well, such as for StatefulSet managed pods. I'm not aware of any better solution than the pod UID to do that.

Maybe what I should have done though is replace the pod name in the Name field of the ServiceName struct with the pod UID rather than add it. The pod name is redundant the vast majority of the time since the UID is guaranteed to be unique.

[joamaki]

I don't think including the pod UID will do anything since pods are indexed by name everywhere, e.g. it is not possible to have two copies of a pod with the same name in the system.

That's true, but the events associated with pod state changes are asynchronous and could arrive or be processed out of order. Via debug logs, I saw this happen and cause the removal of the host port map after a new pod was started to replace an old pod. I added the pod identity check in the first commit to guard against this case. When doing so, it seemed prudent to guard against the case of the old and new pods having the same name as well, such as for StatefulSet managed pods. I'm not aware of any better solution than the pod UID to do that.

The name/namespace is how pods are indexed in Kubernetes. It is not possible to have two instances of the pod with the same name and different UID at the same time and since the events will arrive in order for a resource with a specific name, Cilium will not see things out of order, e.g. it will see the pod terminating, being deleted and then recreated. What can happen is that the deletion and recreation gets coalesced and cilium-agent will only see an "update" to the pod where the UID changes. And that is something that needs to be handled correctly (for example in the endpoint sub-system). However that is not going to affect HostPort handling. Adding the UID in the service name for HostPort doesn't really do anything except make the service name longer. However, I don't really have anything against this and I can see it being useful in debugging scenarios, so let's go for it.

The fix seems to be correct, however it's quite subtle and relies on the fact that a pod being updated to phase=Completed is considered a deletion.

Maybe what I should have done though is replace the pod name in the Name field of the ServiceName struct with the pod UID rather than add it. The pod name is redundant the vast majority of the time since the UID is guaranteed to be unique.

Let's keep the name as well since it's very informative.

[javanthropus]

@joamaki, I think I jammed this by requesting a re-review from you after pushing my last set of updates. Can you approve this again?

How much longer before this is merged?

[joamaki]

/test

[javanthropus]

I can't tell if the failing tests are a result of my changes, but I don't think they are.