diff --git a/CubeAPI/src/services/sandboxes.rs b/CubeAPI/src/services/sandboxes.rs index e4eab3452..3d04e8c50 100644 --- a/CubeAPI/src/services/sandboxes.rs +++ b/CubeAPI/src/services/sandboxes.rs @@ -30,6 +30,15 @@ const RET_CODE_NOT_FOUND: i32 = 130404; const RET_CODE_CONFLICT: i32 = 130409; const RET_CODE_TASK_STATE_INVALID: i32 = 130490; const RET_CODE_TASK_RESUME_FAILED: i32 = 130589; +/// Stable machine marker CubeMaster appends to a pause ret_msg ONLY when it +/// reports an idempotent already-paused (TaskStateInvalid). Keying the redundant +/// pause → HTTP 200 decision on this exact token (not free-form message text) +/// keeps the contract robust: rewording / i18n of Master's human-readable prefix +/// cannot regress a redundant pause to 500, and a Cubelet-originated 130490 that +/// Master passes through verbatim (never carries this marker) is not swallowed as +/// success. CONTRACT: keep in sync with CubeMaster's alreadyPausedMarker +/// (CubeMaster/pkg/service/sandbox/sandbox_resume_pause.go). +const ALREADY_PAUSED_MARKER: &str = "[cube:already-paused]"; const HOSTDIR_MOUNT_KEY: &str = "host-mount"; const ENV_VAR_NAME_MAX_LEN: usize = 256; const ENV_VAR_VALUE_MAX_LEN: usize = 4096; @@ -312,11 +321,36 @@ impl SandboxService { } pub async fn pause_sandbox(&self, sandbox_id: &str) -> AppResult<()> { - let resp = self + let resp = match self .cubemaster .update_sandbox(&self.build_update_request(sandbox_id, "pause", None)) .await - .map_err(|e| map_update_cubemaster_err(e, sandbox_id))?; + { + Ok(resp) => resp, + // 130490 (TaskStateInvalid) on pause means the sandbox is already + // paused (a prior pause whose RPC timed out on the caller actually + // completed on Master). parse_response raises it as CubeMasterError, + // so it lands here. Treat it as idempotent success rather than HTTP + // 500, which would charge a client's redundant pause against the + // server-side success-rate SLI. + // + // Gate on the stable ALREADY_PAUSED_MARKER, not the bare code: + // pauseSandbox passes the Cubelet's ret_code through verbatim, so a + // bare 130490 check would silently swallow any future Cubelet- + // originated 130490 on the pause path as a 200. Master appends the + // marker only on its idempotent already-paused branch, so the + // "already-paused ⟺ existing pause snapshot" contract survives message + // rewording / i18n and does not leak to pass-through codes. Unlike the + // DELETE path (130490 → 503 retry), the pause path treats it as + // terminal success. + Err(CubeMasterError::Api { ret_code, ret_msg }) + if ret_code == RET_CODE_TASK_STATE_INVALID + && ret_msg.contains(ALREADY_PAUSED_MARKER) => + { + return Ok(()); + } + Err(e) => return Err(map_update_cubemaster_err(e, sandbox_id)), + }; ensure_update_result( resp.ret.ret_code, @@ -1123,8 +1157,8 @@ mod tests { use super::{ build_cube_network_config, filter_by_metadata, from_cubemaster_info, map_delete_cubemaster_err, map_volume_mounts, resolve_lifecycle_flags, - validate_mask_request_host, SandboxService, RET_CODE_CONFLICT, RET_CODE_NOT_FOUND, - RET_CODE_TASK_RESUME_FAILED, RET_CODE_TASK_STATE_INVALID, + validate_mask_request_host, SandboxService, ALREADY_PAUSED_MARKER, RET_CODE_CONFLICT, + RET_CODE_NOT_FOUND, RET_CODE_TASK_RESUME_FAILED, RET_CODE_TASK_STATE_INVALID, }; use crate::cubemaster::{ CreateSandboxRequest, CubeMasterClient, CubeMasterError, CubeVolumeMount, @@ -1256,6 +1290,78 @@ mod tests { assert_bad_request(err, reason); } + // Pausing an already-paused sandbox: CubeMaster returns 130490 + // (TaskStateInvalid) once a prior timed-out pause actually completed. That is + // an idempotent no-op for the client, so pause must resolve to Ok(()) rather + // than HTTP 500 (which would count a client's redundant pause against the + // server-side success-rate SLI). + #[tokio::test] + async fn pause_sandbox_treats_already_paused_as_idempotent_success() { + let service = spawn_fake_cubemaster(Router::new().route( + "/cube/sandbox/update", + post(move || async move { + ret_envelope( + RET_CODE_TASK_STATE_INVALID, + "[cube:already-paused] begin pause snapshot: sandbox sbx-1 already has pause snapshot snap-x", + ) + }), + )) + .await; + + service + .pause_sandbox("sbx-1") + .await + .expect("already-paused pause should be idempotent success"); + } + + // The already-paused marker is the load-bearing contract; assert the literal + // matches CubeMaster's alreadyPausedMarker so a rename on either side breaks + // this test rather than silently regressing redundant pauses to HTTP 500. + #[test] + fn already_paused_marker_is_stable() { + assert_eq!(ALREADY_PAUSED_MARKER, "[cube:already-paused]"); + } + + // Negative control: a genuine pause failure (not 130490) must still surface + // as an error so real faults are not masked as idempotent success. + #[tokio::test] + async fn pause_sandbox_keeps_backend_faults_as_error() { + let service = spawn_fake_cubemaster(Router::new().route( + "/cube/sandbox/update", + post(move || async move { ret_envelope(130593, "backend fault") }), + )) + .await; + + service + .pause_sandbox("sbx-1") + .await + .expect_err("backend fault pause should not be treated as success"); + } + + // Negative control: a 130490 WITHOUT the already-paused marker (e.g. a future + // Cubelet-originated TaskStateInvalid passed through verbatim by pauseSandbox, + // or a message that merely mentions "pause snapshot" in another context) must + // stay an error, not be swallowed as idempotent success. The contract is + // keyed on ALREADY_PAUSED_MARKER, not the bare code or free-form text. + #[tokio::test] + async fn pause_sandbox_keeps_unrelated_task_state_invalid_as_error() { + let service = spawn_fake_cubemaster(Router::new().route( + "/cube/sandbox/update", + post(move || async move { + ret_envelope( + RET_CODE_TASK_STATE_INVALID, + "sandbox is pausing (pause snapshot in progress)", + ) + }), + )) + .await; + + service + .pause_sandbox("sbx-1") + .await + .expect_err("unrelated 130490 must not be treated as idempotent success"); + } + // Negative control: genuine backend faults must keep counting as 5xx, and // 130408 CubeletUnHealthy must not be swept up by the 1304xx prefix. #[tokio::test] diff --git a/CubeMaster/pkg/pausesnap/store.go b/CubeMaster/pkg/pausesnap/store.go index 9475b8d91..a6356cb56 100644 --- a/CubeMaster/pkg/pausesnap/store.go +++ b/CubeMaster/pkg/pausesnap/store.go @@ -33,7 +33,11 @@ const ( // Normal Commit snaps use templatecenter.TemplateKindSnapshot ("snapshot"). KindPauseSnapshot = "pause_snapshot" statusReady = "READY" - statusCreating = "CREATING" + // StatusCreating is the initial (pause in flight) binding status. Exported so + // the pause path can recognise a binding stranded at CREATING by a Master + // crash between Cubelet PauseToSnapshot and Complete/MarkFailed. + StatusCreating = "CREATING" + statusCreating = StatusCreating // StatusFailed is a terminal Pause failure. Binding and sandbox proxy are // kept so the user can see the failure; Resume is rejected until Delete. StatusFailed = "FAILED" @@ -49,6 +53,15 @@ var ( ErrNotReady = errors.New("pausesnap store not initialized") ErrNotFound = errors.New("pause snapshot not found") + // ErrAlreadyExists means a READY pause snapshot binding already exists for + // the sandbox when Begin was called — i.e. the sandbox is already paused (or + // a prior pause whose RPC timed out on the caller side actually completed). + // Begin only wraps this for a READY binding; CREATING/FAILED bindings return + // a plain descriptive error instead. Callers should treat ErrAlreadyExists + // as an idempotent "already paused" success rather than a hard parameter + // error, so the dataplane state converges to paused instead of being rolled + // back to running. + ErrAlreadyExists = errors.New("pause snapshot already exists") ) // Init attaches to the shared Master DB. Safe to call multiple times. @@ -94,7 +107,18 @@ func Begin(ctx context.Context, sandboxID, nodeID, nodeIP, instanceType string) return "", errors.New("sandboxID is required") } if existing, err := GetBySandbox(ctx, sandboxID); err == nil && existing != nil { - return "", fmt.Errorf("sandbox %s already has pause snapshot %s", sandboxID, existing.SnapshotID) + if isReadyPauseSnapshot(existing.Status) { + // READY binding: the sandbox is genuinely paused, so callers may + // treat this as an idempotent already-paused success. + return "", fmt.Errorf("%w: sandbox %s already has pause snapshot %s", + ErrAlreadyExists, sandboxID, existing.SnapshotID) + } + // CREATING/FAILED: a pause is in flight or terminally failed — not a + // clean paused state. Keep the descriptive error so the caller does not + // mistake it for idempotent already-paused (which would mask the failure + // instead of routing to resume-heal / delete). + return "", fmt.Errorf("sandbox %s has pause snapshot %s in status %s", + sandboxID, existing.SnapshotID, existing.Status) } else if err != nil && !errors.Is(err, ErrNotFound) { return "", err } diff --git a/CubeMaster/pkg/pausesnap/store_test.go b/CubeMaster/pkg/pausesnap/store_test.go index 4b511eb7e..78900ad31 100644 --- a/CubeMaster/pkg/pausesnap/store_test.go +++ b/CubeMaster/pkg/pausesnap/store_test.go @@ -4,7 +4,9 @@ package pausesnap import ( + "context" "encoding/json" + "errors" "testing" "github.com/stretchr/testify/require" @@ -49,3 +51,33 @@ func TestIsReadyPauseSnapshot(t *testing.T) { require.False(t, isReadyPauseSnapshot(statusCreating)) require.False(t, isReadyPauseSnapshot("")) } + +// A READY binding means the sandbox is genuinely paused: Begin must wrap +// ErrAlreadyExists (via %w) so the caller can detect it with errors.Is and +// treat the pause as idempotent already-paused. +func TestBeginReadyBindingWrapsErrAlreadyExists(t *testing.T) { + db := setupPauseDeleteTest(t) + seedPauseBinding(t, db, "sb-ready", "snap-ready", statusReady, "10.0.0.1") + + _, err := Begin(context.Background(), "sb-ready", "node-1", "10.0.0.1", "cubebox") + require.Error(t, err) + require.True(t, errors.Is(err, ErrAlreadyExists)) + require.Contains(t, err.Error(), "snap-ready") +} + +// A CREATING/FAILED binding is not a clean paused state: Begin must NOT wrap +// ErrAlreadyExists, so the caller keeps the generic failure path instead of +// masking an in-flight or terminally-failed pause as already-paused. +func TestBeginNonReadyBindingDoesNotWrapErrAlreadyExists(t *testing.T) { + for _, status := range []string{statusCreating, statusFailed} { + t.Run(status, func(t *testing.T) { + db := setupPauseDeleteTest(t) + seedPauseBinding(t, db, "sb-x", "snap-x", status, "10.0.0.1") + + _, err := Begin(context.Background(), "sb-x", "node-1", "10.0.0.1", "cubebox") + require.Error(t, err) + require.False(t, errors.Is(err, ErrAlreadyExists)) + require.Contains(t, err.Error(), status) + }) + } +} diff --git a/CubeMaster/pkg/service/sandbox/sandbox_resume_pause.go b/CubeMaster/pkg/service/sandbox/sandbox_resume_pause.go index 9069667aa..5a77a9d3f 100644 --- a/CubeMaster/pkg/service/sandbox/sandbox_resume_pause.go +++ b/CubeMaster/pkg/service/sandbox/sandbox_resume_pause.go @@ -13,6 +13,7 @@ import ( "time" cubebox "github.com/tencentcloud/CubeSandbox/CubeMaster/api/services/cubebox/v1" + cubeleterrorcode "github.com/tencentcloud/CubeSandbox/CubeMaster/api/services/errorcode/v1" "github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/config" "github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/constants" "github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/log" @@ -36,6 +37,17 @@ const ( // Resume may heal only for timeout + Cubelet PAUSED. Explicit Cubelet // failure cannot Resume (delete only). pauseCubeletRPCTimeout = 120 * time.Second + + // alreadyPausedMarker is a stable machine token that Master appends to the + // ret_msg ONLY when a pause is reported as idempotent already-paused + // (TaskStateInvalid). CubeAPI keys its "redundant pause → HTTP 200" decision + // on this exact marker rather than free-form message text, so that (a) + // rewording / i18n of the human-readable prefix cannot silently regress a + // redundant pause back to 500, and (b) a Cubelet-originated 130490 that Master + // passes through verbatim (it never carries this marker) is never swallowed as + // success. CONTRACT: keep this literal in sync with CubeAPI's + // ALREADY_PAUSED_MARKER (CubeAPI/src/services/sandboxes.rs). + alreadyPausedMarker = "[cube:already-paused]" ) // pauseSandbox: @@ -57,11 +69,68 @@ func pauseSandbox(ctx context.Context, req *types.UpdateRequest, hostIP string) nodeID = n.ID() } - // Resume success + failed pausesnap.Delete must not brick the next Pause. - clearStalePauseBindingIfRunning(ctx, req.RequestID, req.SandboxID, hostIP) + // A leftover binding is what drives both the stale-RUNNING cleanup and the + // already-paused convergence below; a clean pause (no binding) needs neither. + // So only probe the live Cubelet state when a binding exists — a normal pause + // issues zero List calls (as before this fix), and a retried auto-pause of a + // stranded sandbox issues exactly one (both consumers share this single probe + // instead of taking a List each). + existing, getErr := pausesnap.GetBySandbox(ctx, req.SandboxID) + if getErr != nil && !errors.Is(getErr, pausesnap.ErrNotFound) { + log.G(ctx).Warnf("pause: get snapshot binding for sandbox %s: %v", req.SandboxID, getErr) + } + var ( + liveState cubebox.ContainerState + liveFound bool + probeIP string + ) + if existing != nil { + probeIP = strings.TrimSpace(hostIP) + if ip := strings.TrimSpace(existing.NodeIP); ip != "" { + probeIP = ip + } + liveState, liveFound = probePauseLiveState(ctx, probeIP, req.SandboxID) + + // Resume success + failed pausesnap.Delete must not brick the next Pause: + // clear the leftover binding only when the sandbox is confirmed RUNNING. + if liveFound && liveState == cubebox.ContainerState_CONTAINER_RUNNING { + clearStalePauseBinding(ctx, req.RequestID, probeIP, existing) + } + } snapID, err := pausesnap.Begin(ctx, req.SandboxID, nodeID, hostIP, req.InstanceType) if err != nil { + // A leftover binding means the sandbox may already be paused — most + // commonly when a prior pause RPC timed out on the caller side but + // actually completed on Master/Cubelet. Report this as an idempotent + // "already in state" (TaskStateInvalid) rather than a generic param error + // so the caller (CLM auto-pause) recognises it and converges the dataplane + // to paused instead of rolling proxy state back to running, which would + // leave auto-resume unable to fire (HTTP 504 on the next request). Only + // converge when the live probe confirms PAUSED: a stale binding on a + // genuinely RUNNING sandbox (or an inconclusive probe) must stay a hard + // error so CLM does not brick a running box. READY converges on a confirmed + // PAUSED probe; FAILED additionally requires the failure to be a timeout. + // Stale CREATING and timeout-FAILED bindings are healed to READY first. + if shouldTreatAsAlreadyPaused(err, existing, liveState, liveFound) { + status := strings.TrimSpace(existing.Status) + if strings.EqualFold(status, pausesnap.StatusCreating) || strings.EqualFold(status, pausesnap.StatusFailed) { + healedNodeID := strings.TrimSpace(existing.NodeID) + if healedNodeID == "" { + if n, ok := localcache.GetNodesByIp(probeIP); ok { + healedNodeID = n.ID() + } + } + if err := pausesnap.Complete(ctx, req.SandboxID, existing.SnapshotID, healedNodeID, probeIP, req.InstanceType, nil); err != nil { + rsp.Ret.RetCode = int(errorcode.ErrorCode_ReqCubeAPIFailed) + rsp.Ret.RetMsg = fmt.Sprintf("heal pause snapshot: %v", err) + return rsp + } + } + rsp.Ret.RetCode = int(errorcode.MasterCode(cubeleterrorcode.ErrorCode_TaskStateInvalid)) + rsp.Ret.RetMsg = fmt.Sprintf("%s begin pause snapshot: %v", alreadyPausedMarker, err) + return rsp + } rsp.Ret.RetCode = int(errorcode.ErrorCode_MasterParamsError) rsp.Ret.RetMsg = fmt.Sprintf("begin pause snapshot: %v", err) return rsp @@ -118,24 +187,78 @@ func pauseSandbox(ctx context.Context, req *types.UpdateRequest, hostIP string) return rsp } -func cubeletReportsPaused(ctx context.Context, hostIP, sandboxID string) bool { - listRsp, err := cubelet.List(ctx, cubelet.GetCubeletAddr(hostIP), &cubebox.ListCubeSandboxRequest{ - Id: &sandboxID, - }) - if err != nil || listRsp == nil { +// shouldTreatAsAlreadyPaused decides whether a Begin rejection for a leftover +// binding should be reported as idempotent already-paused (TaskStateInvalid) +// rather than a hard param error. It is a pure classifier over the Begin error, +// the leftover binding, and the single live Cubelet probe result, so its logic +// is unit-testable without patching: +// +// - READY (ErrAlreadyExists): converge only when the live probe CONFIRMS +// PAUSED. A stale READY binding on a genuinely RUNNING sandbox, or an +// inconclusive probe (probe failed / no container found), stays a hard error +// so CLM does not converge a running sandbox to paused. +// - FAILED: additionally require the failure to be an RPC timeout that the +// Cubelet confirms PAUSED — the same signal recoverTimedOutPauseForResume +// heals on. An explicit failure stays hard. +// - CREATING: a retry has acquired the sandbox lock after the prior holder +// exited, so a confirmed PAUSED probe identifies a stale crash-window binding. +func shouldTreatAsAlreadyPaused(beginErr error, rec *pausesnap.Record, liveState cubebox.ContainerState, liveFound bool) bool { + if rec == nil || !liveFound || liveState != cubebox.ContainerState_CONTAINER_PAUSED { return false } + if errors.Is(beginErr, pausesnap.ErrAlreadyExists) { + return true + } + status := strings.TrimSpace(rec.Status) + if strings.EqualFold(status, pausesnap.StatusCreating) { + return true + } + return strings.EqualFold(status, pausesnap.StatusFailed) && isPauseTimeoutFailure(rec) +} + +// probePauseLiveState returns the observed container state for a sandbox on the +// given node. liveFound is false when the probe could not be completed (List +// error / nil response) or the sandbox container was not found — callers must +// treat that as inconclusive, not as a confirmed absence of PAUSED. +func probePauseLiveState(ctx context.Context, hostIP, sandboxID string) (cubebox.ContainerState, bool) { + hostIP = strings.TrimSpace(hostIP) + if hostIP == "" { + return cubebox.ContainerState_CONTAINER_UNKNOWN, false + } + req := &cubebox.ListCubeSandboxRequest{Id: &sandboxID} + endpoint := cubelet.GetCubeletAddr(hostIP) + listRsp, err := cubelet.List(ctx, endpoint, req) + if err != nil || listRsp == nil { + log.G(ctx).Warnf("pause: probe sandbox %s live state failed, retrying: %v", sandboxID, err) + listRsp, err = cubelet.List(ctx, endpoint, req) + } + if err != nil || listRsp == nil { + log.G(ctx).Warnf("pause: probe sandbox %s live state after retry: %v", sandboxID, err) + return cubebox.ContainerState_CONTAINER_UNKNOWN, false + } + var observed cubebox.ContainerState + found := false for _, item := range listRsp.GetItems() { if item.GetId() != sandboxID { continue } for _, c := range item.GetContainers() { - if c.GetId() == sandboxID && c.GetState() == cubebox.ContainerState_CONTAINER_PAUSED { - return true + if c.GetId() != sandboxID { + continue + } + found = true + observed = c.GetState() + if observed == cubebox.ContainerState_CONTAINER_PAUSED { + return observed, true } } } - return false + return observed, found +} + +func cubeletReportsPaused(ctx context.Context, hostIP, sandboxID string) bool { + state, found := probePauseLiveState(ctx, hostIP, sandboxID) + return found && state == cubebox.ContainerState_CONTAINER_PAUSED } // recoverTimedOutPauseForResume heals a Master FAILED pause binding only when @@ -215,45 +338,16 @@ func isPauseTimeoutMessage(msg string) bool { (strings.Contains(e, "timeout") && strings.Contains(e, "rpc")) } -// clearStalePauseBindingIfRunning drops a leftover pause binding when the -// sandbox is already RUNNING (typical: Resume succeeded but pausesnap.Delete -// failed). Does not touch a real Paused sandbox's binding. -func clearStalePauseBindingIfRunning(ctx context.Context, requestID, sandboxID, hostIP string) { - rec, err := pausesnap.GetBySandbox(ctx, sandboxID) - if err != nil || rec == nil || strings.TrimSpace(rec.SnapshotID) == "" { - return - } - probeIP := hostIP - if ip := strings.TrimSpace(rec.NodeIP); ip != "" { - probeIP = ip - } - if probeIP == "" { - return - } - listRsp, err := cubelet.List(ctx, cubelet.GetCubeletAddr(probeIP), &cubebox.ListCubeSandboxRequest{ - Id: &sandboxID, - }) - if err != nil || listRsp == nil { - log.G(ctx).Warnf("pause: probe sandbox %s for stale binding: %v", sandboxID, err) - return - } - running := false - for _, item := range listRsp.GetItems() { - if item.GetId() != sandboxID { - continue - } - for _, c := range item.GetContainers() { - if c.GetId() == sandboxID && c.GetState() == cubebox.ContainerState_CONTAINER_RUNNING { - running = true - break - } - } - } - if !running { +// clearStalePauseBinding drops a leftover pause binding for a sandbox the caller +// has already confirmed is RUNNING (typical: Resume succeeded but pausesnap.Delete +// failed). The caller supplies the binding and the probe node so this does not +// re-probe the Cubelet. Never called for a real Paused sandbox's binding. +func clearStalePauseBinding(ctx context.Context, requestID, probeIP string, rec *pausesnap.Record) { + if rec == nil || strings.TrimSpace(rec.SnapshotID) == "" || strings.TrimSpace(probeIP) == "" { return } log.G(ctx).Warnf("pause: clearing stale pause binding sandbox=%s snap=%s (sandbox is RUNNING)", - sandboxID, rec.SnapshotID) + rec.SandboxID, rec.SnapshotID) cleanupPauseSnapshotLocal(ctx, requestID, probeIP, rec.SnapshotID) if delErr := pausesnap.Delete(ctx, rec.SnapshotID); delErr != nil { log.G(ctx).Warnf("pause: delete stale binding %s: %v", rec.SnapshotID, delErr) diff --git a/CubeMaster/pkg/service/sandbox/sandbox_resume_pause_test.go b/CubeMaster/pkg/service/sandbox/sandbox_resume_pause_test.go index 4a3dc71f9..7b09965f3 100644 --- a/CubeMaster/pkg/service/sandbox/sandbox_resume_pause_test.go +++ b/CubeMaster/pkg/service/sandbox/sandbox_resume_pause_test.go @@ -4,12 +4,20 @@ package sandbox import ( + "context" "fmt" "testing" "github.com/agiledragon/gomonkey/v2" "github.com/stretchr/testify/require" + cubebox "github.com/tencentcloud/CubeSandbox/CubeMaster/api/services/cubebox/v1" + cubeleterrorcode "github.com/tencentcloud/CubeSandbox/CubeMaster/api/services/errorcode/v1" dbmodels "github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/db/models" + "github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/node" + "github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/errorcode" + "github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/localcache" + "github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/pausesnap" + "github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/service/sandbox/types" ) func TestValidatePauseResumeVolumesEmptyOK(t *testing.T) { @@ -38,3 +46,314 @@ func TestValidatePauseResumeVolumesPresent(t *testing.T) { require.NoError(t, validatePauseResumeVolumes([]string{"vol-ok", " vol-ok2 "})) } + +// paused/running/notFound Cubelet probe stubs for pauseSandbox-level tests. +func patchProbeState(patches *gomonkey.Patches, state cubebox.ContainerState, found bool) { + patches.ApplyFunc(probePauseLiveState, + func(_ context.Context, _, _ string) (cubebox.ContainerState, bool) { + return state, found + }) +} + +// A pause whose earlier RPC timed out on the caller side but actually completed +// leaves a READY pause snapshot binding behind. The next auto-pause attempt must +// not surface a generic MasterParamsError — the caller (CLM) would treat that as +// a hard failure and roll the dataplane state back to "running", which strands +// the sandbox (paused backend, proxy thinks running → auto-resume never fires → +// HTTP 504). With the Cubelet confirmed PAUSED, pauseSandbox must instead report +// the idempotent "already in state" code so the caller converges to paused. +func TestPauseSandboxAlreadyPausedReturnsTaskStateInvalid(t *testing.T) { + patches := gomonkey.NewPatches() + defer patches.Reset() + + patches.ApplyFunc(localcache.GetNodesByIp, + func(_ string) (*node.Node, bool) { return nil, false }) + patches.ApplyFunc(pausesnap.GetBySandbox, + func(_ context.Context, sandboxID string) (*pausesnap.Record, error) { + return &pausesnap.Record{SandboxID: sandboxID, SnapshotID: "snap-x", Status: "READY"}, nil + }) + // Live Cubelet confirms PAUSED — safe to converge. + patchProbeState(patches, cubebox.ContainerState_CONTAINER_PAUSED, true) + // Begin reports the sandbox already has a READY pause snapshot binding. + patches.ApplyFunc(pausesnap.Begin, + func(_ context.Context, sandboxID, _, _, _ string) (string, error) { + return "", fmt.Errorf("%w: sandbox %s already has pause snapshot snap-x", + pausesnap.ErrAlreadyExists, sandboxID) + }) + + rsp := pauseSandbox(context.Background(), &types.UpdateRequest{ + SandboxID: "sb-already-paused", + InstanceType: "cubebox", + Action: "pause", + }, "10.0.0.1") + + require.Equal(t, + int(errorcode.MasterCode(cubeleterrorcode.ErrorCode_TaskStateInvalid)), + rsp.Ret.RetCode) + require.Contains(t, rsp.Ret.RetMsg, "already has pause snapshot") + // The already-paused marker is the load-bearing cross-service contract: + // CubeAPI keys its redundant-pause → HTTP 200 decision on this exact token. + require.Contains(t, rsp.Ret.RetMsg, alreadyPausedMarker) + require.Equal(t, "[cube:already-paused]", alreadyPausedMarker) + // Pin the wire codes: CLM only treats these exact numbers as already-paused + // (130490) vs. hard failure (130400). An enum/offset renumber would silently + // break the cross-service contract that this fix depends on. + require.Equal(t, 130490, int(errorcode.MasterCode(cubeleterrorcode.ErrorCode_TaskStateInvalid))) + require.Equal(t, 130400, int(errorcode.ErrorCode_MasterParamsError)) +} + +// A stale READY binding on a sandbox that is actually RUNNING (resume succeeded +// but pausesnap.Delete failed) must NOT be reported as already-paused when the +// probe does not confirm PAUSED — otherwise CLM would converge a live sandbox to +// paused and brick it. Here the probe confirms RUNNING, so Begin's +// ErrAlreadyExists stays a hard MasterParamsError. +func TestPauseSandboxStaleReadyRunningReturnsParamsError(t *testing.T) { + patches := gomonkey.NewPatches() + defer patches.Reset() + + patches.ApplyFunc(localcache.GetNodesByIp, + func(_ string) (*node.Node, bool) { return nil, false }) + patches.ApplyFunc(pausesnap.GetBySandbox, + func(_ context.Context, sandboxID string) (*pausesnap.Record, error) { + return &pausesnap.Record{SandboxID: sandboxID, SnapshotID: "snap-x", Status: "READY"}, nil + }) + // Sandbox is RUNNING — clearStalePauseBinding fires and Begin still races a + // leftover binding; convergence must NOT happen. + patchProbeState(patches, cubebox.ContainerState_CONTAINER_RUNNING, true) + patches.ApplyFunc(clearStalePauseBinding, + func(_ context.Context, _, _ string, _ *pausesnap.Record) {}) + patches.ApplyFunc(pausesnap.Begin, + func(_ context.Context, sandboxID, _, _, _ string) (string, error) { + return "", fmt.Errorf("%w: sandbox %s already has pause snapshot snap-x", + pausesnap.ErrAlreadyExists, sandboxID) + }) + + rsp := pauseSandbox(context.Background(), &types.UpdateRequest{ + SandboxID: "sb-stale-ready", + InstanceType: "cubebox", + Action: "pause", + }, "10.0.0.1") + + require.Equal(t, int(errorcode.ErrorCode_MasterParamsError), rsp.Ret.RetCode) +} + +// An inconclusive probe (List error / container not found) on a READY binding +// must also stay a hard error — convergence requires a CONFIRMED PAUSED probe. +func TestPauseSandboxReadyProbeInconclusiveReturnsParamsError(t *testing.T) { + patches := gomonkey.NewPatches() + defer patches.Reset() + + patches.ApplyFunc(localcache.GetNodesByIp, + func(_ string) (*node.Node, bool) { return nil, false }) + patches.ApplyFunc(pausesnap.GetBySandbox, + func(_ context.Context, sandboxID string) (*pausesnap.Record, error) { + return &pausesnap.Record{SandboxID: sandboxID, SnapshotID: "snap-x", Status: "READY"}, nil + }) + // Probe could not be completed (found=false) → inconclusive. + patchProbeState(patches, cubebox.ContainerState_CONTAINER_UNKNOWN, false) + patches.ApplyFunc(pausesnap.Begin, + func(_ context.Context, sandboxID, _, _, _ string) (string, error) { + return "", fmt.Errorf("%w: sandbox %s already has pause snapshot snap-x", + pausesnap.ErrAlreadyExists, sandboxID) + }) + + rsp := pauseSandbox(context.Background(), &types.UpdateRequest{ + SandboxID: "sb-probe-fail", + InstanceType: "cubebox", + Action: "pause", + }, "10.0.0.1") + + require.Equal(t, int(errorcode.ErrorCode_MasterParamsError), rsp.Ret.RetCode) +} + +// Any other Begin failure keeps the generic MasterParamsError contract. +func TestPauseSandboxBeginGenericErrorReturnsParamsError(t *testing.T) { + patches := gomonkey.NewPatches() + defer patches.Reset() + + patches.ApplyFunc(localcache.GetNodesByIp, + func(_ string) (*node.Node, bool) { return nil, false }) + patches.ApplyFunc(pausesnap.GetBySandbox, + func(_ context.Context, _ string) (*pausesnap.Record, error) { + return nil, pausesnap.ErrNotFound + }) + patchProbeState(patches, cubebox.ContainerState_CONTAINER_UNKNOWN, false) + patches.ApplyFunc(pausesnap.Begin, + func(_ context.Context, _, _, _, _ string) (string, error) { + return "", fmt.Errorf("db down") + }) + + rsp := pauseSandbox(context.Background(), &types.UpdateRequest{ + SandboxID: "sb-db-down", + InstanceType: "cubebox", + Action: "pause", + }, "10.0.0.1") + + require.Equal(t, int(errorcode.ErrorCode_MasterParamsError), rsp.Ret.RetCode) + require.Contains(t, rsp.Ret.RetMsg, "db down") +} + +// A FAILED pause binding left by a timed-out Master→Cubelet RPC that actually +// completed (Cubelet probes PAUSED) is the same stranded-504 symptom as the +// READY case. Begin returns a generic (non-ErrAlreadyExists) error for FAILED, +// but pauseSandbox must still converge it to the idempotent already-paused code +// so CLM pushes the dataplane to paused rather than rolling back to running. +func TestPauseSandboxFailedTimeoutCompletedReturnsTaskStateInvalid(t *testing.T) { + patches := gomonkey.NewPatches() + defer patches.Reset() + + patches.ApplyFunc(localcache.GetNodesByIp, + func(_ string) (*node.Node, bool) { return nil, false }) + patches.ApplyFunc(pausesnap.GetBySandbox, + func(_ context.Context, sandboxID string) (*pausesnap.Record, error) { + return &pausesnap.Record{ + SandboxID: sandboxID, + SnapshotID: "snap-x", + Status: pausesnap.StatusFailed, + LastError: "rpc error: context deadline exceeded", + }, nil + }) + // Timed-out pause that finished on the Cubelet: probe confirms PAUSED. + patchProbeState(patches, cubebox.ContainerState_CONTAINER_PAUSED, true) + patches.ApplyFunc(pausesnap.Begin, + func(_ context.Context, sandboxID, _, _, _ string) (string, error) { + return "", fmt.Errorf("sandbox %s has pause snapshot snap-x in status FAILED", sandboxID) + }) + completed := false + patches.ApplyFunc(pausesnap.Complete, + func(context.Context, string, string, string, string, string, []string) error { + completed = true + return nil + }) + + rsp := pauseSandbox(context.Background(), &types.UpdateRequest{ + SandboxID: "sb-failed-timeout", + InstanceType: "cubebox", + Action: "pause", + }, "10.0.0.1") + + require.True(t, completed) + require.Equal(t, + int(errorcode.MasterCode(cubeleterrorcode.ErrorCode_TaskStateInvalid)), + rsp.Ret.RetCode) + require.Equal(t, 130490, int(errorcode.MasterCode(cubeleterrorcode.ErrorCode_TaskStateInvalid))) + // FAILED-timeout convergence must also carry the marker so CubeAPI treats it + // as an idempotent 200, same as the READY path. + require.Contains(t, rsp.Ret.RetMsg, alreadyPausedMarker) +} + +// A FAILED binding that is NOT a healed timeout (explicit non-timeout failure) +// must stay a hard MasterParamsError so the caller does not mask a real pause +// failure as idempotent success — even if the Cubelet happens to probe PAUSED. +func TestPauseSandboxFailedNotCompletedReturnsParamsError(t *testing.T) { + patches := gomonkey.NewPatches() + defer patches.Reset() + + patches.ApplyFunc(localcache.GetNodesByIp, + func(_ string) (*node.Node, bool) { return nil, false }) + patches.ApplyFunc(pausesnap.GetBySandbox, + func(_ context.Context, sandboxID string) (*pausesnap.Record, error) { + return &pausesnap.Record{ + SandboxID: sandboxID, + SnapshotID: "snap-x", + Status: pausesnap.StatusFailed, + LastError: "cubelet pause: disk full", + }, nil + }) + patchProbeState(patches, cubebox.ContainerState_CONTAINER_PAUSED, true) + patches.ApplyFunc(pausesnap.Begin, + func(_ context.Context, sandboxID, _, _, _ string) (string, error) { + return "", fmt.Errorf("sandbox %s has pause snapshot snap-x in status FAILED", sandboxID) + }) + + rsp := pauseSandbox(context.Background(), &types.UpdateRequest{ + SandboxID: "sb-failed-hard", + InstanceType: "cubebox", + Action: "pause", + }, "10.0.0.1") + + require.Equal(t, int(errorcode.ErrorCode_MasterParamsError), rsp.Ret.RetCode) + require.Contains(t, rsp.Ret.RetMsg, "FAILED") +} + +func TestPauseSandboxCreatingPausedHealsBinding(t *testing.T) { + patches := gomonkey.NewPatches() + defer patches.Reset() + + patches.ApplyFunc(localcache.GetNodesByIp, + func(_ string) (*node.Node, bool) { return nil, false }) + patches.ApplyFunc(pausesnap.GetBySandbox, + func(_ context.Context, sandboxID string) (*pausesnap.Record, error) { + return &pausesnap.Record{ + SandboxID: sandboxID, + SnapshotID: "snap-x", + NodeID: "node-x", + NodeIP: "10.0.0.2", + Status: pausesnap.StatusCreating, + }, nil + }) + patchProbeState(patches, cubebox.ContainerState_CONTAINER_PAUSED, true) + patches.ApplyFunc(pausesnap.Begin, + func(_ context.Context, sandboxID, _, _, _ string) (string, error) { + return "", fmt.Errorf("sandbox %s has pause snapshot snap-x in status CREATING", sandboxID) + }) + completed := false + patches.ApplyFunc(pausesnap.Complete, + func(context.Context, string, string, string, string, string, []string) error { + completed = true + return nil + }) + + rsp := pauseSandbox(context.Background(), &types.UpdateRequest{ + SandboxID: "sb-creating", + InstanceType: "cubebox", + Action: "pause", + }, "10.0.0.1") + + require.True(t, completed) + require.Equal(t, + int(errorcode.MasterCode(cubeleterrorcode.ErrorCode_TaskStateInvalid)), + rsp.Ret.RetCode) + require.Contains(t, rsp.Ret.RetMsg, alreadyPausedMarker) +} + +// shouldTreatAsAlreadyPaused is the pure classifier the fix depends on. Exercise +// its real branches directly (no gomonkey) so the READY-needs-PAUSED gate and +// FAILED-needs-timeout+PAUSED gate are pinned independently of pauseSandbox. +func TestShouldTreatAsAlreadyPaused(t *testing.T) { + t.Parallel() + alreadyExists := fmt.Errorf("%w: snap-x", pausesnap.ErrAlreadyExists) + failedGeneric := fmt.Errorf("sandbox sb has pause snapshot snap-x in status FAILED") + timeoutRec := &pausesnap.Record{Status: pausesnap.StatusFailed, LastError: "rpc error: context deadline exceeded"} + explicitRec := &pausesnap.Record{Status: pausesnap.StatusFailed, LastError: "cubelet pause: disk full"} + creatingRec := &pausesnap.Record{Status: pausesnap.StatusCreating} + readyRec := &pausesnap.Record{Status: "READY"} + + cases := []struct { + name string + beginErr error + rec *pausesnap.Record + liveState cubebox.ContainerState + liveFound bool + want bool + }{ + {"ready+paused → converge", alreadyExists, readyRec, cubebox.ContainerState_CONTAINER_PAUSED, true, true}, + {"ready+running → hard error", alreadyExists, readyRec, cubebox.ContainerState_CONTAINER_RUNNING, true, false}, + {"ready+probe inconclusive → hard error", alreadyExists, readyRec, cubebox.ContainerState_CONTAINER_UNKNOWN, false, false}, + {"failed+timeout+paused → converge", failedGeneric, timeoutRec, cubebox.ContainerState_CONTAINER_PAUSED, true, true}, + {"failed+timeout+not paused → hard error", failedGeneric, timeoutRec, cubebox.ContainerState_CONTAINER_RUNNING, true, false}, + {"failed+explicit+paused → hard error", failedGeneric, explicitRec, cubebox.ContainerState_CONTAINER_PAUSED, true, false}, + {"creating+paused → converge", failedGeneric, creatingRec, cubebox.ContainerState_CONTAINER_PAUSED, true, true}, + {"creating+running → hard error", failedGeneric, creatingRec, cubebox.ContainerState_CONTAINER_RUNNING, true, false}, + {"nil rec+alreadyexists → hard error", alreadyExists, nil, cubebox.ContainerState_CONTAINER_PAUSED, true, false}, + {"nil rec, non-alreadyexists → hard error", failedGeneric, nil, cubebox.ContainerState_CONTAINER_PAUSED, true, false}, + } + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tc.want, + shouldTreatAsAlreadyPaused(tc.beginErr, tc.rec, tc.liveState, tc.liveFound)) + }) + } +}