From 767092fa741e84f9eff76629def60ce6282234d5 Mon Sep 17 00:00:00 2001 From: Sarthak Date: Sat, 26 Sep 2026 23:42:42 +0530 Subject: [PATCH] cleanup: move provisionStart into placement (#79) Signed-off-by: Sarthak --- pkg/vnode/handler.go | 95 +++++++++++++++++++-------------------- pkg/vnode/handler_test.go | 24 +++++++++- 2 files changed, 69 insertions(+), 50 deletions(-) diff --git a/pkg/vnode/handler.go b/pkg/vnode/handler.go index c8823b9..d8d8b01 100644 --- a/pkg/vnode/handler.go +++ b/pkg/vnode/handler.go @@ -148,25 +148,9 @@ type trackedPod struct { // persistCredential. patchedMeta podMeta - // provisioningAt is when THIS process began provisioning. It arms the one - // metrics.InstanceReadyDuration observation the poll loop makes on the first - // Running, and is consumed by it, so zero means "do not observe" for either reason: - // - // - never armed: a pod re-adopted after a restart lost its real start time with - // the process, and measuring from re-adoption would report minutes as - // milliseconds — a missing sample beats a wrong one; - // - already spent: the poll loop sees Running every tick for the pod's whole life, - // so a still-armed token would fire per tick with a growing duration, measuring - // longevity instead of readiness. - // - // Known bias: a provision still in flight across a restart never contributes, so the - // histogram under-samples the slowest boots. Fixing it means persisting the start - // time, a write on the provisioning path we have not taken. - provisioningAt time.Time - // initializingAt is when the instance was first observed Initializing, and arms the - // readiness deadline (see readyExpired). Deliberately NOT provisioningAt: it measures - // the boot alone, so a provision that took minutes does not eat the budget. + // readiness deadline (see readyExpired). Deliberately NOT placement.provisioningAt: it + // measures the boot alone, so a provision that took minutes does not eat the budget. // // Level-triggered, not one-shot like provisioningAt: it is cleared whenever the // instance is not Pending, so the clock tracks the CURRENT Initializing spell. A pod @@ -177,8 +161,8 @@ type trackedPod struct { // placement is what this pod was provisioned against. Two readers: the poll loop files // the ready duration under the same dimensions as the provision counters, and DeletePod - // takes the region the instance is reachable in. Set only where provisioningAt is armed. - placement + // takes the region the instance is reachable in. Nil unless THIS process provisioned it. + placement *placement } // placement is the decision one provision was issued against: the two facts the Pod itself @@ -187,12 +171,28 @@ type trackedPod struct { // because the claim can be deleted while its instance is still tracked. The NodeClaim // stays the durable record; this is the historical one. // -// The zero value means "unknown" and is what every path that never provisioned stores. Its -// readers degrade rather than guess: no ready sample is filed (provisioningAt is zero on -// those same paths), and teardown falls back to reading the claim. +// Nil means "unknown" and is what every path that never provisioned stores. Its readers +// degrade rather than guess: no ready sample is filed, and teardown falls back to reading +// the claim. Losing it on a restart is fine for the same reason. type placement struct { region string tier nebulav1alpha1.CapacityType + + // provisioningAt is when THIS process began provisioning. It arms the one + // metrics.InstanceReadyDuration observation the poll loop makes on the first + // Running, and is consumed by it, so zero means "do not observe" for either reason: + // + // - never armed: a pod re-adopted after a restart lost its real start time with + // the process, and measuring from re-adoption would report minutes as + // milliseconds — a missing sample beats a wrong one; + // - already spent: the poll loop sees Running every tick for the pod's whole life, + // so a still-armed token would fire per tick with a growing duration, measuring + // longevity instead of readiness. + // + // Known bias: a provision still in flight across a restart never contributes, so the + // histogram under-samples the slowest boots. Fixing it means persisting the start + // time, a write on the provisioning path we have not taken. + provisioningAt time.Time } // podMeta is the Pod metadata the virtual kubelet owns: the annotations it is the sole @@ -367,10 +367,9 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { // only the Provision call. Not interchangeable — the emit between them is a // synchronous notify that can issue an API write, which would otherwise be charged // to the provider's latency. - provisioningAt := time.Now() // Carried into store so the poll loop's ready observation is filed under the same region // and tier as the counters below, whatever the NodeClaim says by then. - place := placement{region: req.Region, tier: req.CapacityType} + place := &placement{region: req.Region, tier: req.CapacityType, provisioningAt: time.Now()} labels := h.metricLabels(pod, place.region, place.tier) // Report Provisioning BEFORE the call: it can run for minutes (AWS sweeps a region's @@ -395,7 +394,7 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { // update branch instead of provisioning again. // // Safe only because Pod with empty instanceID will be terminated on the next poll tick. - h.store(pod, claim, "", time.Time{}, placement{}) + h.store(pod, claim, "", nil) h.emit(pod) return err } @@ -419,7 +418,7 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { // tracked copy carries it, published by the emit below, re-offered every tick until a // write lands. Its reader is the NodeClaim controller (see InstanceIDAnnotation). setInstanceID(pod, res.InstanceID) - h.store(pod, claim, res.InstanceID, provisioningAt, place) + h.store(pod, claim, res.InstanceID, place) // The TOKEN cannot ride the Pod (readable with `get pod`, unencrypted in etcd), so it // gets its own write — the only place it exists, since the provider mints it once and @@ -483,7 +482,9 @@ func (h *Handler) DeletePod(ctx context.Context, pod *corev1.Pod) error { instance, region := "", "" if ok { instance = tp.instance - region = tp.region + if tp.placement != nil { + region = tp.placement.region + } } h.mu.Unlock() @@ -570,10 +571,9 @@ func (h *Handler) GetPod(ctx context.Context, namespace, name string) (*corev1.P } pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}} applyState(pod, inst.State, inst.Endpoint, h.nowFn()) - // Zero start: this process never provisioned it, so the real start time is gone and - // the ready-duration is not observable (see trackedPod.provisioningAt) — hence no - // placement either, since nothing here will be filed under it. - h.store(pod, claim, inst.ID, time.Time{}, placement{}) + // No placement: this process never provisioned it, so the real start time is gone and + // the ready-duration is not observable (see placement.provisioningAt). + h.store(pod, claim, inst.ID, nil) log.Info("re-adopted live instance after cold tracking map (VK restart)", "claim", claim, "instanceID", inst.ID, "state", inst.State) return pod.DeepCopy(), nil @@ -788,17 +788,18 @@ func (h *Handler) readyExpired(tp *trackedPod, state provider.InstanceState) (ti // pin to a fixed instant, and subtracting a real start time from a pinned now would give // a nonsense duration. // -// ONE-SHOT — it consumes provisioningAt, whose zero value covers both "never armed" and -// "already recorded" (see trackedPod.provisioningAt). That guard also keeps the poll loop -// cheap: labels are rendered behind it, so at most once per pod, never per tick. +// ONE-SHOT — it consumes placement.provisioningAt, whose zero value covers both "never +// armed" and "already recorded". That guard also keeps the poll loop cheap: labels are +// rendered behind it, so at most once per pod, never per tick. // // Callers must hold h.mu. func (h *Handler) observeReady(tp *trackedPod, state provider.InstanceState) { - if state != provider.InstanceRunning || tp.provisioningAt.IsZero() { + p := tp.placement + if state != provider.InstanceRunning || p == nil || p.provisioningAt.IsZero() { return } - metrics.ObserveReady(h.metricLabels(tp.pod, tp.region, tp.tier), time.Since(tp.provisioningAt)) - tp.provisioningAt = time.Time{} // spent; never observe this pod again + metrics.ObserveReady(h.metricLabels(tp.pod, p.region, p.tier), time.Since(p.provisioningAt)) + p.provisioningAt = time.Time{} // spent; never observe this pod again } // setEndpoint stamps a reachable address onto the Pod's annotation — the one assignment @@ -866,21 +867,17 @@ func statusSignature(pod *corev1.Pod) string { return string(pod.Status.Phase) + "|" + pod.Status.Reason + "|" + string(ready) + "|" + pod.Status.PodIP } -// store records/updates the tracked pod under lock. provisioningAt arms the -// ready-duration observation (see trackedPod.provisioningAt) and place is part of what that -// observation is filed under; pass the zero values from any path that cannot know them — +// store records/updates the tracked pod under lock. place arms the ready-duration +// observation (see placement.provisioningAt); pass nil from any path that cannot know it — // a re-adoption, or an already-terminal pod. -func (h *Handler) store( - pod *corev1.Pod, claim, instance string, provisioningAt time.Time, place placement, -) { +func (h *Handler) store(pod *corev1.Pod, claim, instance string, place *placement) { h.mu.Lock() defer h.mu.Unlock() h.tracked[key(pod.Namespace, pod.Name)] = &trackedPod{ - pod: pod.DeepCopy(), - claimName: claim, - instance: instance, - provisioningAt: provisioningAt, - placement: place, + pod: pod.DeepCopy(), + claimName: claim, + instance: instance, + placement: place, } } diff --git a/pkg/vnode/handler_test.go b/pkg/vnode/handler_test.go index 87b3fe5..9cc312e 100644 --- a/pkg/vnode/handler_test.go +++ b/pkg/vnode/handler_test.go @@ -1559,12 +1559,34 @@ func TestReconcileOnce_ReadinessDeadlineSparesRunningInstance(t *testing.T) { } } +// Spending the ready observation must not drop the placement: DeletePod still needs its +// region. A pod with no placement (re-adopted) must be skipped, not dereferenced. +func TestObserveReady_SpendsStartButKeepsPlacement(t *testing.T) { + h := NewHandler(&fakeProvider{}, nil, nil, openCluster()) + tp := &trackedPod{ + pod: testPod("default", "p1"), + placement: &placement{region: "us-east", provisioningAt: time.Now()}, + } + h.observeReady(tp, provider.InstanceRunning) + if !tp.placement.provisioningAt.IsZero() { + t.Error("provisioningAt still armed after the first Running") + } + if tp.placement.region != "us-east" { + t.Errorf("region = %q, want us-east kept for teardown", tp.placement.region) + } + + h.observeReady(&trackedPod{pod: testPod("default", "p2")}, provider.InstanceRunning) +} + func TestReadyExpired_ClockStartsAtInitializingNotAtProvision(t *testing.T) { // The provision call has its own timeout, so its duration must not eat the readiness // budget: a Provision that took an hour still leaves the box a full budget to boot in. fp := &fakeProvider{} h := NewHandler(fp, nil, nil, openCluster()) - tp := &trackedPod{pod: testPod("default", "p1"), provisioningAt: time.Now().Add(-time.Hour)} + tp := &trackedPod{ + pod: testPod("default", "p1"), + placement: &placement{provisioningAt: time.Now().Add(-time.Hour)}, + } if _, over := h.readyExpired(tp, provider.InstancePending); over { t.Fatal("a long provision must not expire the readiness deadline")