/sig network

Hi @declangallagher few questions to get a better picture of the issue:

1. is client-pod accessing the server-pod (of which the IP has been given to a different pod) via a service or directly?
2. What was the health of the controller manager specifically endpoint and endpointSlice controllers?

Are you saying you have two running pods with the same IP? Can you show us something like kubectl get pods -o json | jq '.items[] | .metadata.name, .status.podIP, .status.phase'

Is this on containerd or dockershim?

I've seen something similar with a CNI whose IPAM was recycling IPs, per example, podA was assigned 10.1.1.11, it was deleted and podB was created at the same time, reusing the previously assigned IP 10.1.1.11 ... just sharing

@declangallagher ICYMI we are waiting on you for more info.

We're experiencing similar symptoms after upgrading from v1.21.5 to v1.22.8. Bisecting leads to 3eadd1a (#102344) as the first broken commit. The issue is still present on HEAD at the time (= 076168b). I'll attach the reproducer at the bottom.

We observe this when kubelet evicts a Pod, which then either goes into ContainerStatusUnknown or Error, depending on which exact version is checked out. While bisecting, I also observed some of the already fixed issues like #105462 . The issue does not appear when I manually evict through the Eviction API.

Prior to 3eadd1a, the Pod would not retain its PodIP after eviction and/or container termination/cleanup. After this PR it does, so that the IPs still show up in the Endpoint under notReadyAddresses. Since the IP eventually gets reused by unrelated pods, we run into issues similar to the ones described in this Prometheus ticket prometheus/prometheus#10257 (comment) .

The containers on the node are cleaned up properly.

I am not sure in which component the fault is. I.e. should the Pod lose its PodIP upon/after eviction; should it just not be present in the Endpoint/EndpointSlice at all; or should the IP not be reused until the Pod has been (manually) cleaned up?

My reproducer is similar to the ones in other tickets; essentially spawn a Pod that gets evicted due to ephemeral storage/disk pressure. I used resources.limits.ephemeral-storage here because that was easiest to change (and I did not want to exhaust my actual disk). On our non-kind cluster it works the same through normal node disk pressure eviction, though.

# setup
kubetest2 kind -v 2 --kube-root . --build --up
kubectl apply -f evictme.yaml
# observe through either; need to wait for the eviction to happen of course
kubectl get pods -owide
kubectl get endpoints -ojson evictme | jq '.subsets[0].notReadyAddresses'
# teardown
kubetest2 kind -v 2 --kube-root . --down

# evictme.yaml
kind: Pod
apiVersion: v1
metadata:
  name: evictme
  labels:
    pod: evictme
spec:
  terminationGracePeriodSeconds: 10
  containers:
  - name: busybox
    image: k8s.gcr.io/e2e-test-images/nginx:1.14-2
    # the first sleep is meant to curb races, as that's not the main issue.
    # I didn't check if it's actually necessary.
    command: ["sh", "-c", "sleep 10; fallocate -l 10M file; sleep 100000"]
    ports:
    - containerPort: 80
    resources:
      limits:
        ephemeral-storage: 5Mia
  tolerations:
  - key: "node-role.kubernetes.io/control-plane"
    operator: "Exists"
    effect: "NoSchedule"
---
apiVersion: v1
kind: Service
metadata:
  name: evictme
spec:
  ports:
  - name: http
    port: 80
  selector:
    pod: "evictme"
  type: ClusterIP

/triage accepted

This is legit, thanks for the reproducer, and there more issues related like this #109718

In 1.22 there was a change in the kubelet #106884 (comment) , most of the issues were solved but seems the surface was bigger than expected and is not deleting the pod.Status.PodIPs field cc: @smarterclayton

Are CNI plug-ins depending on pod status ips as the authoritative record of allocation? Also, do we formally define CNI destroy as happening before that release?

The answer to those two questions is required to correctly determine where in pod shutdown logic needs to be added (and this is another kubelet e2e test we need to add). Ie if the second is no, we can clear the status podIPs once the pod containers are confirmed shutdown. If the second is yes, we have to defer the final pod status update and the clear until after cni destroy is guaranteed to succeed (that has other safety implications).

/assign

Are CNI plug-ins depending on pod status ips as the authoritative record of allocation?

the problem is on the contrary, pod.status.PodIPs are authoritative for endpoints and endpointslices , with the reproducer I can see the CNI works fine, the pod and its network disappears

Also, do we formally define CNI destroy as happening before that release?

after StopPodSandbox there should not be any IP assigned to that Pod

The main difference in behavior is that evicted pods , since 1.22 , show the IPs in its status

Evict

1.23
$ kubectl get pods -o wide
NAME       READY   STATUS       RESTARTS   AGE     IP           NODE                 NOMINATED NODE   READINESS GATES
busybox    0/1     Completed    0          22m     10.244.0.5   kind-control-plane   <none>           <none>
busybox3   0/1     StartError   0          5m36s   10.244.0.6   kind-control-plane   <none>           <none>
evictme    0/1     Error        0          67s     10.244.0.7   kind-control-plane   <none>           <none>

---

1.21

$ kubectl get pods -o wide
NAME       READY   STATUS       RESTARTS   AGE     IP           NODE                NOMINATED NODE   READINESS GATES
busybox    0/1     Completed    0          15m     10.244.0.2   121-control-plane   <none>           <none>
busybox3   0/1     StartError   0          3m56s   10.244.0.6   121-control-plane   <none>           <none>
evictme    0/1     Evicted      0          63s     <none>       121-control-plane   <none>           <none>

the logic for endpoint and endpointslices is very simplistic and is not even considering this situation, it worked before because the evicted pods weren't reporting IPs, but current behavior of pods seems more correct and consistent with other states

(we should centralize this logic for both controllers too) cc:@robscott @thockin

On another note, I don't think that tolerateUnreadyEndpoints should mean "add any podIP that I find on the pods", is there any reason to have terminal pods IPs on endpoints?

I will send a PR soon