diff --git a/pkg/vnode/handler.go b/pkg/vnode/handler.go index 196bf45..7ebf1dc 100644 --- a/pkg/vnode/handler.go +++ b/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() diff --git a/pkg/vnode/handler_test.go b/pkg/vnode/handler_test.go index 89581a5..5de0050 100644 --- a/pkg/vnode/handler_test.go +++ b/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()