fix: model ready state check

[kerthcet]

What this PR does / why we need it

Which issue(s) this PR fixes

Fixes #

Special notes for your reviewer

Does this PR introduce a user-facing change?

[Copilot]

Pull request overview

This PR refines how external provider lifecycle signals (especially Modal) map into Kubernetes Pod readiness and Nebula’s NodeClaim ledger, so workloads aren’t marked “Ready” before the underlying instance is actually reachable.

Changes:

• Centralizes Pod status Reason strings into api/v1alpha1 and updates vnode/controller code to consume shared constants.
• Adds Modal readiness gating (probe-tagging + background WaitUntilReady latch) and improves Modal terminal-state reporting (failed vs terminated).
• Updates status documentation and samples to reflect the revised lifecycle mapping and readiness behavior.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
README.md	Adds Discord badge; links to new status documentation.
pkg/vnode/status.go	Switches vnode pod-reason literals to shared API constants; clarifies readiness semantics.
pkg/provider/modal/modal.go	Adds Modal statusInitializing/failed mapping; tags sandboxes that actually received a readiness probe.
pkg/provider/modal/modal_test.go	Expands tests for Modal state mapping, exit-code classification, and probe tagging behavior.
pkg/provider/modal/client.go	Implements readiness latch + background waiter and uses it to gate “running” status.
internal/controller/nodepool_controller.go	Clarifies that “Placed” counts existing (including booting) instances.
internal/controller/nodeclaim_controller.go	Treats Pending/Initializing pods as evidence of instance existence (Bound); uses shared pod-reason constants.
internal/controller/nodeclaim_controller_test.go	Updates/extends tests to validate the new Bound-on-boot behavior and teardown-without-grace.
docs/status.md	New comprehensive lifecycle/status mapping document.
docs/architecture.md	Refactors status-flow section to reference docs/status.md and updated phase semantics.
config/samples/nodepool.yaml	Adjusts sample provider list (Modal enabled; AWS commented).
config/samples/deployment.yaml	Updates sample workload sizing and adds a readinessProbe illustrating Modal readiness gating.
config/manager/kustomization.yaml	Changes default controller image name/tag.
config/crd/bases/nebula.inftyai.com_nodepools.yaml	Updates status.placed field description to include booting instances.
api/v1alpha1/nodepool_types.go	Updates NodePoolStatus.Placed doc to include booting instances.
api/v1alpha1/nodeclaim_types.go	Updates NodeClaimPhase semantics (Bound == existence, no Initializing phase).
api/v1alpha1/groupversion_info.go	Introduces shared PodReason* constants for vnode/controller contract.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (4)

pkg/provider/modal/client.go:418

• observeReady treats a missing ProbeTagKey as "ready". When GetTags fails, out.Tags stays nil, so this code will return true and advance a sandbox to statusRunning even though probe-ness is unknown (bypassing the readiness gate on a transient tag error). Default should be not-ready when tags are unavailable.

func (c *sdkClient) observeReady(ctx context.Context, id string, tags map[string]string) bool {
	if tags[ProbeTagKey] != probeTagValue {
		return true
	}

internal/controller/nodeclaim_controller.go:367

• wasBound only treats Bound/Terminated as "ever existed". After upgrading from older versions, existing NodeClaims may still have status.phase="Initializing"; treating that as not-bound reintroduces the grace window and can leak instances on mid-boot Pod disappearance (the exact scenario this PR is trying to fix). Consider treating legacy "Initializing" as bound for teardown-guard purposes.

func (r *NodeClaimReconciler) wasBound(nc *nebulav1alpha1.NodeClaim) bool {
	return nc.Status.Phase == nebulav1alpha1.NodeClaimBound ||
		nc.Status.Phase == nebulav1alpha1.NodeClaimTerminated
}

pkg/provider/modal/client.go:431

• awaitReady is documented as not inheriting the caller's cancellation, but observeReady currently passes the List/observe context directly. If that context has a short deadline or is canceled when List returns, the background waiter will be canceled early and readiness may never latch (causing repeated retries and/or perpetual Initializing). Strip cancellation/deadline when spawning the goroutine and let awaitReady manage its own timeout budget.

	if !ready && !waiting {
		go c.awaitReady(ctx, id)
	}

pkg/provider/modal/client.go:305

• The comment says observe is a "BOUNDED read" where every call carries a short deadline, but observe currently calls GetTags and Poll with the incoming ctx directly (no per-call timeout), and only bounds Tunnels. Either add timeouts around those calls or update the comment to match the actual behavior.

// observe is a BOUNDED read: every call it makes carries a short deadline, so it
// returns promptly even for a sandbox that is still coming up. It must be, since
// it runs once per sandbox inside the List iteration.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (3)

pkg/provider/modal/client.go:418

• observeReady treats a sandbox as ready when tags are missing. If GetTags fails, out.Tags stays nil and tags[ProbeTagKey] reads as "no probe", which can incorrectly advance a probe-gated sandbox to statusRunning and mark the Pod Ready early.

func (c *sdkClient) observeReady(ctx context.Context, id string, tags map[string]string) bool {
	if tags[ProbeTagKey] != probeTagValue {
		return true
	}

pkg/provider/modal/client.go:431

• awaitReady is started with the caller's ctx from List/observe. If that ctx is canceled when List returns (as the comment above says is expected), the background waiter will be canceled immediately and readiness may never latch, causing repeated retries/goroutines.

	if !ready && !waiting {
		go c.awaitReady(ctx, id)
	}

pkg/provider/modal/client.go:72

• The struct comment says ready/waiting entries are "dropped by forgetReady when the sandbox goes away" and therefore can't grow without bound, but forgetReady is only called on Poll exit or TerminateSandbox. If a sandbox disappears from List without either path (e.g., out-of-band deletion), entries can remain indefinitely, so the comment is misleading.

	// ready is latched on CONFIRMED readiness only — the default is not-ready, so an
	// ambiguous error can no longer promote a sandbox that is still coming up.
	// waiting dedupes waiters so repeated ticks don't pile up goroutines on one
	// sandbox. Both are keyed by sandbox id and dropped by forgetReady when the
	// sandbox goes away, so neither grows without bound.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (3)

pkg/provider/modal/client.go:414

• This comment says observeReady “takes no context on purpose”, but the method currently takes a context.Context argument (which is also unused). Either drop the ctx parameter entirely, or update the comment to reflect that ctx cancellation is intentionally ignored.

// It takes no context on purpose: there is nothing here to cancel, which is the
// property that makes it safe to call once per sandbox inside List.

pkg/provider/modal/client.go:313

• observe claims every call is bounded by a short deadline, but GetTags and Poll currently use the caller’s ctx directly (only Tunnels is wrapped). If ctx has no deadline, a slow/hung Modal call can stall ListSandboxes and therefore the whole poll loop. Wrap GetTags and Poll in context.WithTimeout (similar to Tunnels), or update the comment if the intent is not to bound these calls.

	// Tags carry Nebula identity (ClaimTagKey), recovered by toInstance, and
	// probe-ness (ProbeTagKey), read by isReady below — so this must precede the
	// status block.
	if tags, err := sb.GetTags(ctx, &modal.SandboxGetTagsParams{}); err == nil {
		out.Tags = tags

pkg/provider/modal/client.go:72

• The struct comment says the readiness latch maps “do not grow without bound” because entries are dropped by forgetReady when a sandbox goes away, but forgetReady is only called on explicit TerminateSandbox or when Poll observes an exit code. If a sandbox disappears from List between ticks (no Poll observation), ready entries can linger. Either add a pruning path (e.g., GC based on the current List IDs) or soften this comment so it matches the actual cleanup semantics.

This issue also appears on line 413 of the same file.

	// waiting dedupes waiters so repeated ticks don't pile up goroutines on one
	// sandbox. Both are keyed by sandbox id and dropped by forgetReady when the
	// sandbox goes away, so neither grows without bound.

[kerthcet]

/lgtm
/kind feature

[InftyAI-Agent]

Approved: PR has both lgtm and approved labels

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (5)

pkg/provider/modal/client.go:418

• observeReady treats a missing ProbeTagKey as “no probe => ready”, but tags can be nil here when GetTags fails in observe. That makes a probe-backed sandbox look ready and can incorrectly advance it to statusRunning (and thus Pod Ready) even though readiness was never confirmed.

func (c *sdkClient) observeReady(id string, tags map[string]string) bool {
	if tags[ProbeTagKey] != probeTagValue {
		return true
	}

config/samples/deployment.yaml:41

• The sample workload switches the accelerator type to a100-80gb, which is a much higher-cost default for a sample manifest and can surprise users applying config/samples verbatim. Consider reverting to the previous smaller example shape and letting users opt into larger GPUs.

        nebula.inftyai.com/enabled: "true"
        nebula.inftyai.com/nodepool: sample
        nebula.inftyai.com/accelerator-type: a100-80gb

config/samples/deployment.yaml:87

• The sample workload now requests nvidia.com/gpu: "8", which combined with replicas: 8 can allocate a very large amount of GPU capacity by default. If the goal is to demonstrate multi-GPU requests, consider a separate example file or a clear warning comment; otherwise revert to a smaller default.

            # GPU count. Standard extended resource, so the scheduler's fit check
            # and provisioning read the same number. 8 => 8x the accelerator-type
            # above.
            nvidia.com/gpu: "8"

pkg/provider/modal/client.go:476

• awaitReady can re-latch c.ready[id]=true after forgetReady(id) has already been called (e.g. the sandbox exits or is terminated while the waiter is in-flight). That violates the “maps track live sandboxes only” intent and can leave stale readiness behind.

	c.readyMu.Lock()
	defer c.readyMu.Unlock()
	delete(c.waiting, id)
	if confirmed {
		c.ready[id] = true

config/samples/deployment.yaml:31

• This sample Deployment now defaults to replicas: 8, which is likely to create a large number of GPU instances when users apply config/samples (and is unrelated to the PR’s stated “ready state check” fix). Consider keeping samples safe-by-default and scaling via a comment instead.

This issue also appears in the following locations of the same file:

• line 39
• line 84

  replicas: 8