fix: just in case patch the Pod failed then the instance ID will be l…

pkg/vnode/handler.go

@@ -445,8 +445,8 @@ func (h *Handler) UpdatePod(_ context.Context, pod *corev1.Pod) error {
 	h.mu.Lock()
 	defer h.mu.Unlock()
 	if tp, ok := h.tracked[key(pod.Namespace, pod.Name)]; ok {
-		// Copy the status and endpoint just in case the update failed,
-		// so we do not lose them in the tracked copy.
+		// Preserve status and the endpoint if this API-server copy predates our writes.
+		// persistMetadata recovers the instance ID from tp.instance instead.
 		status := tp.pod.Status
 		endpoint := tp.pod.Annotations[nebulav1alpha1.EndpointAnnotation]
 		tp.pod = pod.DeepCopy()
@@ -758,9 +758,14 @@ func setEndpoint(pod *corev1.Pod, endpoint string) {
 	setAnnotation(pod, nebulav1alpha1.EndpointAnnotation, endpoint)
 }
 
-// setInstanceID stamps the provider's instance id on the Pod. One writer only —
-// CreatePod, the moment Provision returns it — because that is the only place it is
-// learned; the poll loop matches instances by CLAIM NAME and never re-derives the id.
+// setInstanceID stamps the provider's instance id on the Pod. One stamping site —
+// CreatePod, the moment Provision returns it — and, unlike the endpoint, no read-path
+// site that re-applies it every tick.
+//
+// So this is not the only way the id reaches etcd, and must not become it: a re-adopted
+// pod (see GetPod) learns its id from the provider's List without any Pod to stamp, which
+// is why persistMetadata falls back to trackedPod.instance. Anything that drops the
+// annotation from a tracked copy depends on that fallback to recover.
 //
 // It rides the Pod so the NodeClaim controller can record it from an object it already
 // has, instead of asking the provider for a full instance list on every reconcile (see
@@ -875,6 +880,19 @@ func (h *Handler) persistMetadata(ctx context.Context, pod *corev1.Pod) {
 	// An untracked pod has nothing to compare against, so everything it carries is
 	// patched: these annotations are the only place those values reach a reader.
 	if tp, tracked := h.tracked[key(pod.Namespace, pod.Name)]; tracked {
+		// The ANNOTATION wins: it is the value already offered to readers, so etcd stays the
+		// source of truth. trackedPod.instance is the fallback for the paths that hold an id
+		// without one ever being stamped on the Pod — a re-adopted pod (see GetPod), and one
+		// whose copy UpdatePod replaced from the API server before the patch landed. They are
+		// the same value wherever both exist.
+		//
+		// Load-bearing, not belt-and-braces: nothing re-derives this annotation later (see
+		// setInstanceID), so an id missing here is missing for the instance's whole life, and
+		// with it status.InstanceID — leaving teardown to search the provider by claim name
+		// and logship to skip the Pod entirely.
+		if want.instanceID == "" {
+			want.instanceID = tp.instance
+		}
 		want = want.minus(tp.patchedMeta)
 	}
 	h.mu.Unlock()

pkg/vnode/handler_test.go

@@ -950,6 +950,85 @@ func TestCreatePod_PersistsInstanceIDAlongsideEndpoint(t *testing.T) {
 	}
 }
 
+// A pod re-adopted after a VK restart is the case the create-path stamp cannot cover: GetPod
+// builds a fresh Pod from the provider's List, so the id exists only in trackedPod.instance and
+// there is no annotation to re-offer. Nothing re-derives it later either (see setInstanceID), so
+// without persistMetadata's fallback status.InstanceID stays empty for the instance's whole life —
+// leaving teardown to search the provider by claim name and logship to skip the Pod.
+func TestReadoptedPod_PersistsInstanceIDFromTracking(t *testing.T) {
+	pod := testPod("default", "p1")
+	client := fake.NewSimpleClientset(pod)
+
+	var patches int
+	client.PrependReactor("patch", "pods", func(k8stesting.Action) (bool, runtime.Object, error) {
+		patches++
+		return false, nil, nil // fall through to the tracker so the object updates
+	})
+
+	fp := &fakeProvider{list: []provider.Instance{
+		{ID: "inst-1", ClaimName: "default-p1", State: provider.InstanceRunning},
+	}}
+	h := NewHandler(fp, client, nil, openCluster())
+	h.NotifyPods(context.Background(), func(*corev1.Pod) {})
+
+	// Cold tracking map, live instance: VK's existence check re-adopts it. No emit here, so
+	// nothing is written yet — the id is in tracking alone.
+	if _, err := h.GetPod(context.Background(), "default", "p1"); err != nil {
+		t.Fatalf("GetPod: %v", err)
+	}
+	h.reconcileOnce(context.Background())
+
+	live, err := client.CoreV1().Pods("default").Get(context.Background(), "p1", metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("get patched pod: %v", err)
+	}
+	if got := live.Annotations[nebulav1alpha1.InstanceIDAnnotation]; got != "inst-1" {
+		t.Fatalf("instance id annotation = %q, want inst-1 — a re-adopted pod's id never reaches "+
+			"a reader otherwise", got)
+	}
+
+	// The fallback feeds the same dedup as a stamped value, or every tick re-patches the fleet.
+	h.reconcileOnce(context.Background())
+	if patches != 1 {
+		t.Fatalf("an unchanged instance id must not re-patch; got %d patches", patches)
+	}
+}
+
+// The dedup is only safe because the write is a MERGE patch: a body carrying one annotation
+// leaves every other one alone. So once the endpoint has been patched, minus drops it from every
+// later body and the id travels by itself without erasing the address. Sending "" for the omitted
+// field instead — the obvious-looking alternative to podMeta.annotations omitting it — would
+// overwrite a working address with an empty string.
+func TestPersistMetadata_OneFieldPatchLeavesTheOtherAlone(t *testing.T) {
+	const dns = "ec2-1-2-3-4.compute.amazonaws.com"
+	pod := testPod("default", "p1")
+	pod.Annotations = map[string]string{nebulav1alpha1.EndpointAnnotation: dns}
+	client := fake.NewSimpleClientset(pod)
+
+	h := NewHandler(&fakeProvider{}, client, nil, openCluster())
+	// The endpoint is already durable, the id is not — the state after any tick that patched
+	// the address before the id was known.
+	h.tracked[key("default", "p1")] = &trackedPod{
+		pod:         pod.DeepCopy(),
+		claimName:   "default-p1",
+		instance:    "inst-1",
+		patchedMeta: podMeta{endpoint: dns},
+	}
+
+	h.persistMetadata(context.Background(), pod.DeepCopy())
+
+	live, err := client.CoreV1().Pods("default").Get(context.Background(), "p1", metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("get patched pod: %v", err)
+	}
+	if got := live.Annotations[nebulav1alpha1.InstanceIDAnnotation]; got != "inst-1" {
+		t.Fatalf("instance id annotation = %q, want inst-1", got)
+	}
+	if got := live.Annotations[nebulav1alpha1.EndpointAnnotation]; got != dns {
+		t.Fatalf("endpoint annotation = %q, want %q — a patch that omits a field must not clear it", got, dns)
+	}
+}
+
 // connectSecret fetches the connect Secret for a pod, or nil when absent.
 func connectSecret(t *testing.T, client *fake.Clientset, ns, podName string) *corev1.Secret {
 	t.Helper()