fix: keep the shadow of a pod recreated under the same name

pkg/virtualkubelet/kept_containers.go

@@ -159,7 +159,7 @@ func (p *Provider) currentShadowPod(ctx context.Context, identity shadowResource
 	var newest *v1.Pod
 	for i := range pods.Items {
 		pod := &pods.Items[i]
-		if pod.DeletionTimestamp != nil {
+		if pod.DeletionTimestamp != nil || !isShadowPodOf(pod, identity.OwnerUID) {
 			continue
 		}
 		if newest == nil || newest.CreationTimestamp.Before(&pod.CreationTimestamp) {

pkg/virtualkubelet/kept_containers_test.go

@@ -270,6 +270,28 @@
 		assert.Equal(t, "/delete", path.Load())
 	})
 
+	// The recreated pod took the shadow over in place: the previous pod's shadow pod,
+	// whose kept init container already completed, must not count for this pod.
+	t.Run("a shadow pod of the previous pod with the same name does not count", func(t *testing.T) {
+		pod := podKeepingInit(v1.RestartPolicyNever)
+		previous := shadowOf(t, pod, exited(0))
+		previous.Labels[shadowOfLabel] = "uid-previous-generation"
+		client := fake.NewSimpleClientset(previous)
+		p := keptProvider(client, pod)
+
+		ctx := p.trackKeptInitWait(t.Context(), pod)
+		done := make(chan error, 1)
+		go func() { done <- p.waitForKeptInitContainers(ctx, pod) }()
+
+		select {
+		case err := <-done:
+			t.Fatalf("returned on the previous pod's shadow: %v", err)
+		case <-time.After(100 * time.Millisecond):
+		}
+		p.stopKeptInitWait(string(pod.UID))
+		<-done
+	})
+
 	t.Run("no kept init container: nothing to wait for", func(t *testing.T) {
 		pod := podKeepingInit(v1.RestartPolicyNever)
 		delete(pod.Annotations, annDisableOffloadInitContainers)

pkg/virtualkubelet/shadow.go

@@ -159,6 +159,21 @@ func uniqueTruncate(s string, maxLen int, full string) string {
 type shadowResourceIdentity struct {
 	Name      string
 	Namespace string
+	// OwnerUID is the UID of the pod the shadow serves. Shadow resources are named
+	// after the pod, so a pod recreated under the same name reuses those names; the
+	// UID, stamped as a label, tells which pod a resource belongs to.
+	OwnerUID string
+	// PodName and PodNamespace name that pod.
+	PodName      string
+	PodNamespace string
+}
+
+// ownerPod is the namespace/name stamped as shadowOwnerPodAnnotation.
+func (identity shadowResourceIdentity) ownerPod() string {
+	if identity.PodName == "" {
+		return ""
+	}
+	return identity.PodNamespace + "/" + identity.PodName
 }
 
 func isShadowSameNamespace(pod *v1.Pod) bool {
@@ -183,7 +198,10 @@ func computeShadowResourceIdentity(pod *v1.Pod) (shadowResourceIdentity, error)
 		name, namespace = computeShadowResourceNames(pod.Name, pod.Namespace)
 	}
 
-	identity := shadowResourceIdentity{Name: name, Namespace: namespace}
+	identity := shadowResourceIdentity{
+		Name: name, Namespace: namespace,
+		OwnerUID: string(pod.UID), PodName: pod.Name, PodNamespace: pod.Namespace,
+	}
 	if len(fmt.Sprintf("%s-%s", identity.Name, identity.Namespace)) > 63 {
 		return shadowResourceIdentity{}, fmt.Errorf("shadow ingress hostname label %q exceeds 63 characters; shorten pod/namespace or disable interlink.eu/shadow-same-ns", fmt.Sprintf("%s-%s", identity.Name, identity.Namespace))
 	}
@@ -301,7 +319,9 @@ func shadowNodeConfigMapName(shadowName string) string {
 // resetShadowNodeConfigMap creates, or blanks, the per-shadow node ConfigMap.
 // The shadow is rendered before the remote batch system has allocated anything,
 // so the ConfigMap has to exist - and be mountable - while still empty. Blanking
-// an existing one stops a previous run's node from being tunnelled to.
+// an existing one stops a previous run's node from being tunnelled to. The cleanup
+// of a deleted pod with the same name can remove an existing one in between, in
+// which case it is created again.
 func (p *Provider) resetShadowNodeConfigMap(ctx context.Context, identity shadowResourceIdentity) error {
 	cm := &v1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
@@ -310,18 +330,25 @@ func (p *Provider) resetShadowNodeConfigMap(ctx context.Context, identity shadow
 		},
 		Data: map[string]string{shadowNodeNameKey: "", shadowSocketDirKey: ""},
 	}
+	stampShadowOwner(cm, identity.OwnerUID, identity.ownerPod())
 
-	_, err := p.clientSet.CoreV1().ConfigMaps(identity.Namespace).Create(ctx, cm, metav1.CreateOptions{})
-	if err == nil {
-		return nil
-	}
-	if !apierrors.IsAlreadyExists(err) {
-		return fmt.Errorf("failed to create shadow node configmap %s/%s: %w", identity.Namespace, cm.Name, err)
-	}
-	if _, err := p.clientSet.CoreV1().ConfigMaps(identity.Namespace).Update(ctx, cm, metav1.UpdateOptions{}); err != nil {
-		return fmt.Errorf("failed to reset shadow node configmap %s/%s: %w", identity.Namespace, cm.Name, err)
+	configMaps := p.clientSet.CoreV1().ConfigMaps(identity.Namespace)
+	for attempt := 0; ; attempt++ {
+		_, err := configMaps.Create(ctx, cm, metav1.CreateOptions{})
+		if err == nil {
+			return nil
+		}
+		if !apierrors.IsAlreadyExists(err) {
+			return fmt.Errorf("failed to create shadow node configmap %s/%s: %w", identity.Namespace, cm.Name, err)
+		}
+		_, err = configMaps.Update(ctx, cm, metav1.UpdateOptions{})
+		if err == nil {
+			return nil
+		}
+		if !apierrors.IsNotFound(err) || attempt == 2 {
+			return fmt.Errorf("failed to reset shadow node configmap %s/%s: %w", identity.Namespace, cm.Name, err)
+		}
 	}
-	return nil
 }
 
 // publishShadowNode records the remote compute node a pod was allocated on, and
@@ -333,7 +360,9 @@ func (p *Provider) resetShadowNodeConfigMap(ctx context.Context, identity shadow
 // patch, so the shadow never sees a new node with the previous job's directory.
 //
 // Repeated calls with unchanged values are dropped, so the status loop does not
-// write on every poll.
+// write on every poll. A ConfigMap stamped with another pod's UID belongs to a pod
+// recreated under the same name and is left alone; the patch carries the
+// resourceVersion read, so it fails rather than overwrite a change made meanwhile.
 func (p *Provider) publishShadowNode(ctx context.Context, pod *v1.Pod, nodeName, socketDir string) {
 	if nodeName == "" || !p.hasShadow(pod) {
 		return
@@ -361,19 +390,34 @@ func (p *Provider) publishShadowNode(ctx context.Context, pod *v1.Pod, nodeName,
 		return
 	}
 
-	patch, err := json.Marshal(map[string]map[string]string{"data": {
-		shadowNodeNameKey:  nodeName,
-		shadowSocketDirKey: socketDir,
-	}})
+	name := shadowNodeConfigMapName(identity.Name)
+	configMaps := p.clientSet.CoreV1().ConfigMaps(identity.Namespace)
+	current, err := configMaps.Get(ctx, name, metav1.GetOptions{})
+	if err != nil {
+		log.G(ctx).Warningf("Failed to read shadow configmap %s/%s: %v", identity.Namespace, name, err)
+		return
+	}
+	if owner := current.Labels[shadowOwnerUIDLabel]; owner != "" && identity.OwnerUID != "" && owner != identity.OwnerUID {
+		log.G(ctx).Infof("Not publishing compute node %q for %s/%s: shadow configmap %s belongs to pod UID %s",
+			nodeName, pod.Namespace, pod.Name, name, owner)
+		return
+	}
+
+	var nodePatch struct {
+		Metadata struct {
+			ResourceVersion string `json:"resourceVersion"`
+		} `json:"metadata"`
+		Data map[string]string `json:"data"`
+	}
+	nodePatch.Metadata.ResourceVersion = current.ResourceVersion
+	nodePatch.Data = map[string]string{shadowNodeNameKey: nodeName, shadowSocketDirKey: socketDir}
+	patch, err := json.Marshal(nodePatch)
 	if err != nil {
 		log.G(ctx).Warningf("Failed to marshal shadow node patch for %s/%s: %v", pod.Namespace, pod.Name, err)
 		return
 	}
 
-	name := shadowNodeConfigMapName(identity.Name)
-	_, err = p.clientSet.CoreV1().ConfigMaps(identity.Namespace).Patch(
-		ctx, name, k8stypes.StrategicMergePatchType, patch, metav1.PatchOptions{},
-	)
+	_, err = configMaps.Patch(ctx, name, k8stypes.StrategicMergePatchType, patch, metav1.PatchOptions{})
 	if err != nil {
 		log.G(ctx).Warningf("Failed to publish compute node %q to shadow configmap %s/%s: %v", nodeName, identity.Namespace, name, err)
 		return
@@ -404,10 +448,43 @@ func (p *Provider) releaseShadowOfEndedPod(ctx context.Context, pod *v1.Pod) {
 		return
 	}
 	log.G(ctx).Infof("Pod %s/%s is %s: removing its shadow", pod.Namespace, pod.Name, pod.Status.Phase)
-	p.cleanupShadowResources(ctx, identity.Name, identity.Namespace)
+	p.cleanupShadowResources(ctx, identity)
 	p.forgetShadowNodeName(pod)
 }
 
+// isShadowPodOf reports whether a shadow pod may serve the pod with UID podUID: it is
+// labelled for that pod, or it predates shadowOfLabel, or the UID is unknown.
+func isShadowPodOf(shadowPod *v1.Pod, podUID string) bool {
+	of := shadowPod.Labels[shadowOfLabel]
+	return of == "" || podUID == "" || of == podUID
+}
+
+// ownerIsCurrentPod reports whether the pod a shadow resource is stamped for still
+// exists: the pod named by its shadowOwnerPodAnnotation (the pod of identity when it
+// has none) has the UID of its shadowOwnerUIDLabel. The name comes from the resource
+// because two pods can share a shadow name: sanitising maps web.0 and web-0 to one.
+// Otherwise the owner is gone and its cleanup never ran (it was deleted while the
+// virtual kubelet was down, say), so its leftovers are deleted. When the pod cannot
+// be read, the resource is kept.
+func (p *Provider) ownerIsCurrentPod(ctx context.Context, identity shadowResourceIdentity, obj metav1.Object) bool {
+	namespace, name, found := strings.Cut(obj.GetAnnotations()[shadowOwnerPodAnnotation], "/")
+	if !found {
+		namespace, name = identity.PodNamespace, identity.PodName
+	}
+	if name == "" {
+		return true
+	}
+	pod, err := p.clientSet.CoreV1().Pods(namespace).Get(ctx, name, metav1.GetOptions{})
+	if apierrors.IsNotFound(err) {
+		return false
+	}
+	if err != nil {
+		log.G(ctx).Warningf("Failed to read pod %s/%s: %v", namespace, name, err)
+		return true
+	}
+	return string(pod.UID) == obj.GetLabels()[shadowOwnerUIDLabel]
+}
+
 // indexShadowPods groups the shadow pods by the UID of the pod they stand for, read
 // from shadowOfLabel. The pods of a shadow rendered before that label existed are
 // left out, so their virtual pod keeps the IP it was created with.

pkg/virtualkubelet/shadow_pod_test.go

@@ -41,7 +41,7 @@ func TestWaitForDeploymentPodIgnoresPodsOfAPreviousDeployment(t *testing.T) {
 
 	p := &Provider{clientSet: fake.NewSimpleClientset(deployment, previous, current)}
 
-	got, err := p.waitForDeploymentPod(context.Background(), name, testNamespace)
+	got, err := p.waitForDeploymentPod(context.Background(), name, testNamespace, "")
 	require.NoError(t, err)
 	assert.Equal(t, current.Name, got.Name)
 }
@@ -61,7 +61,29 @@ func TestWaitForDeploymentPodIgnoresTerminatingPods(t *testing.T) {
 
 	p := &Provider{clientSet: fake.NewSimpleClientset(deployment, terminating, current)}
 
-	got, err := p.waitForDeploymentPod(context.Background(), name, testNamespace)
+	got, err := p.waitForDeploymentPod(context.Background(), name, testNamespace, "")
+	require.NoError(t, err)
+	assert.Equal(t, current.Name, got.Name)
+}
+
+// When the recreated pod takes the Deployment over in place, the Deployment keeps its
+// creation time, so the previous pod's shadow pod is not older than it. The label
+// naming the pod each shadow pod stands for tells them apart.
+func TestWaitForDeploymentPodIgnoresPodsOfAnotherPod(t *testing.T) {
+	const name, previousUID, currentUID = "shadow-nb-0-ns", "uid-p1", "uid-p2"
+	now := time.Now()
+
+	deployment := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{
+		Name: name, Namespace: testNamespace, CreationTimestamp: metav1.NewTime(now.Add(-time.Hour)),
+	}}
+	previous := shadowTestPod(name+"-6d5c666bc8-zzzzz", name, "10.0.0.146", now)
+	previous.Labels[shadowOfLabel] = previousUID
+	current := shadowTestPod(name+"-7f8d9a0b1c-aaaaa", name, "10.0.0.141", now.Add(-time.Minute))
+	current.Labels[shadowOfLabel] = currentUID
+
+	p := &Provider{clientSet: fake.NewSimpleClientset(deployment, previous, current)}
+
+	got, err := p.waitForDeploymentPod(context.Background(), name, testNamespace, currentUID)
 	require.NoError(t, err)
 	assert.Equal(t, current.Name, got.Name)
 }