fix(nodemeta): sync template locality to localcache on periodic reload

[dushulin]

Summary

In a multi-replica CubeMaster deployment, template locality is not synced across replicas on the periodic DB reload, so sandbox creation intermittently fails with template has no ready replica (error 130400) depending on which replica serves the request.

Component

CubeMaster (pkg/nodemeta + pkg/localcache)

Environment

• CubeSandbox version / commit: master
• Deployment mode: cluster, multi-replica CubeMaster (>= 2 replicas)
• Relevant component: CubeMaster
• KVM / kernel: not relevant — this is a control-plane scheduler-cache consistency bug, independent of host/hardware.

Steps to Reproduce

1. Deploy CubeMaster with >= 2 replicas behind a LoadBalancer/VIP (each cubelet reports to a single replica).
2. Register/import a template so its ready replicas are learned by whichever CubeMaster replica the cubelet reports to.
3. Repeatedly create sandboxes from that template through the API (which round-robins across CubeMaster replicas).

Expected Behavior

Every CubeMaster replica knows which templates are ready on which node, so sandbox creation succeeds regardless of which replica serves the request.

Actual Behavior

Only the replica that received the node's direct register/heartbeat has the template locality in its imageCache. Any other replica returns GetImageStateByNode == nil in EnsureTemplateLocalityReady, and creation fails with template has no ready replica (error 130400). In a 2-replica setup, creation succeeded roughly 50/50 depending on which replica the request hit.

Root Cause

The periodic node-metadata reload (loopReload / applyReloadResult, added in #542 for cross-replica node/heartbeat sync) rebuilds only the nodemeta in-memory node map. It does not rebuild localcache (scheduler node cache + template locality / imageCache), which is populated solely from the register/heartbeat paths via syncLocalcache. So a replica that learns a node only via the DB reload never registers that node's ready template replicas into its imageCache.

This is the template-locality analogue of the heartbeat bug fixed in #542: node registration/heartbeat is now synced across replicas, but template locality was still per-process.

Fix

applyReloadResult now re-syncs every reloaded node into localcache (UpsertNode + SyncNodeTemplates) after releasing s.mu, mirroring the register/heartbeat paths. A replica that only learned the node via the DB reload now also registers its ready template replicas into the imageCache, so template locality converges across replicas within one SyncMetaDataInterval.

The sync is gated on a new localcache.Ready() check: nodemeta.Init() runs an initial reload before localcache.Init() in coreInit, so the first reload must skip the sync (localcache.Init loads all nodes/templates from the DB itself right after); otherwise it would panic on a nil node cache. The periodic loopReload runs long after both packages are initialised.

Snapshots are cloned while holding s.mu and syncLocalcache is called after unlocking, to avoid holding the nodemeta lock across localcache calls.

Testing

• New regression test TestApplyReloadResultSyncsTemplateLocalityToLocalcache: models a replica with no in-memory node that learns the node (and its local templates) only via DB reload, and asserts GetImageStateByNode is non-nil after applyReloadResult.
• Adds localcache.InitInMemoryForTest (in-memory-only init) so cross-package nodemeta tests can exercise the sync path without a full Init (DB / Redis / goroutines).
• go test ./CubeMaster/pkg/nodemeta/... passes.
• Verified on a 2-replica staging deployment: a 20x sandbox-create loop that previously failed ~50% (130400) now succeeds 20/20; both replicas report healthy=true.

Related

• Follow-up to fix(nodemeta): start periodic reload goroutine for cross-replica node sync #542 (fix(nodemeta): start periodic reload goroutine for cross-replica node sync), which fixed the same cross-replica gap for node heartbeat/health but not for template locality.

[cubesandboxbot]

Panic safety: Replacing defer s.mu.Unlock() with a bare s.mu.Unlock() here means any panic in the merge loop (lines 865–903) leaves s.mu locked forever. While loopReload catches panics via recov.WithRecover, a leaked mutex causes the next heartbeat handler or label update to deadlock silently — harder to diagnose than a crash.

Suggestion: Extract the critical section (loop body) into a helper with defer s.mu.Unlock(), or restore defer before the function body:

func (s *service) applyReloadResult(next map[string]*NodeSnapshot) {
    syncSnaps := s.mergeReloadResult(next)  // contains s.mu with defer
    if localcache.Ready() {
        for _, snap := range syncSnaps {
            syncLocalcache(snap)
        }
    }
}

[cubesandboxbot]

Doc inaccuracy: localcache.Init() calls loadAllFromDB() which only populates the scheduler node cache (l.cache). Template locality (imageCache) is NOT loaded by localcache.Init — it is populated later by syncLocalcache calls from nodemeta's register/heartbeat/reload paths. The safety of skipping the sync here is still correct (template locality converges on the next periodic reload or incoming heartbeat), but the stated reason is slightly wrong. Suggest changing "nodes/templates" to "nodes".

[cubesandboxbot]

cloneSnapshot is now called in this new code path. It uses append([]ContainerImage(nil), in.Images...) which creates a new outer slice, but each element's Names []string field still aliases the source backing array. If any future code path mutates Images[i].Names outside s.mu, it would race with the original inside s.nodes. Currently harmless (neither toSchedulerNode nor syncLocalcache accesses Images), but worth noting for future refactors.

[cubesandboxbot]

Review of PR #730 — fix(nodemeta): sync template locality to localcache on periodic reload

Overall

This is a well-structured fix for a cross-replica template-locality consistency bug. The approach is correct: applyReloadResult now mirrors the register/heartbeat paths by re-syncing each reloaded node into localcache outside the s.mu lock. The split into mergeReloadResult (under lock) + localcache sync (outside lock) is clean.

Notable findings

1. Slice-header aliasing (service.go:918-920) — Conditions, Images, and LocalTemplates are assigned as shallow slice-header copies, sharing the backing array with the DB-sourced newSnap. Not exploitable today but inconsistent with the deep copy on Versions (line 913). Future refactors that reuse the DB snapshot could silently corrupt these fields.

2. newTestService carries global-state side-effect (service_reload_test.go:17) — Calls localcache.InitInMemoryForTest(), mutating package globals. Tests sharing newTestService cannot safely use t.Parallel() — a future addition could introduce data races.

3. No dirty-check on re-sync (minor) — Every reloaded node is re-synced to localcache even if unchanged. Fine for default 30s cycle / typical cluster sizes; worth noting at scale.

4. Test coverage is solid — Both regression tests cover the happy path and init-ordering edge case.

[cubesandboxbot]

Panic-Safety Concern: Removing defer s.mu.Unlock() in favor of a manual s.mu.Unlock() at line 904 means a panic in the locked section (lines 865-903) will permanently deadlock the mutex, blocking all writers on s.nodes (including the full registration/heartbeat path).

Consider wrapping the locked section in an IIFE to preserve defer's panic safety while keeping the unlock-before-localcache semantics:

syncSnaps := func() []*NodeSnapshot {
    s.mu.Lock()
    defer s.mu.Unlock()
    // ... existing loop ...
    return syncSnaps
}()

[cubesandboxbot]

Production-Safety Concern: This function is exported with no compile-time guard. A future refactor importing localcache could accidentally call it in production, wiping all node and template-locality state on a live replica.

Consider either:

1. Unexporting to initInMemoryForTest() + use //go:build test build constraint, or
2. Moving to a *_test.go file or a testutil internal sub-package excluded from production builds.

[cubesandboxbot]

Data Race Concern: Ready() reads l.cache without any memory barrier (no mutex, no atomic load), while l.cache is written in Init() and InitInMemoryForTest(). Under the Go memory model, concurrent unsynchronized reads/writes to the same pointer constitute a data race.

Consider using atomic.Bool instead:

var cacheInitialized atomic.Bool

func Ready() bool { return cacheInitialized.Load() }

(Set cacheInitialized.Store(true) at the end of Init and InitInMemoryForTest.)

[fslongjin]

Thanks for your PR! I'll review and test this next Monday~