feat(metrics): add lifecycle-aware sandbox resource metrics - #1122
feat(metrics): add lifecycle-aware sandbox resource metrics#1122zyl1121 wants to merge 3 commits into
Conversation
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/in/3653584?s=60&v=4){kind=small}
Review: feat(metrics) — lifecycle-aware sandbox resource metrics (#1122)AI-generated review. Reviewed against the base branch ( This is a large, carefully-engineered PR (~6.7k additions) that makes Cubelet the owner of lifecycle-aware CPU/memory accounting for sandboxes. The design is well thought out: guest metric epochs with persisted baselines, host cgroup assignment baselines that survive pool-slot reuse, bounded collection with per-scope concurrency limits, latest-sample caching so Prometheus scrapes never fan out to sandboxes, and a dedicated I did not find a correctness bug that breaks the primary path. The findings below are robustness/completeness issues, plus a couple of cross-component contracts worth verifying before merge. Findings1. OOM counters are collected, persisted, and normalized but never exported (medium) 2. One-shot baseline failures permanently disable a scope (medium)
Both would be more robust if a transient failure at lifecycle time could fall back to the same lazy/recovery path that already exists for pre-upgrade sandboxes (establish baseline on the first successful sample). 3. Guest CPU unit / metrics-type contract with CubeShim (verify before merge) Minor notes
Positive notes
Overall: a high-quality PR. The items I'd want resolved before merge are the OOM dead-collection path (#1) and the two permanent-fail-closed behaviors (#2); #3 is a verification step rather than a code change. |
3e2ffcc to
b179ec5
Compare
fslongjin
left a comment
fslongjin
left a comment
There was a problem hiding this comment.
Thanks — overall direction looks good to me.
This PR turns Task.Stats into Cubelet-owned sandbox CPU/memory metrics with lifecycle handling (create, rollback, and reused cgroup paths). The layering is clear: raw stats → baseline / accounting window → bounded background sampling + cache → read-only export on /v1/metrics/resource. I like that scrapes do not fan out per sandbox, guest/host stay separate, rollback uses prepare→restore→activate, and host_sandbox is the default.
Before merge, a few points I want to confirm first (details in the inline comments):
| ID | Question |
|---|---|
| P1 | Why remove the generic cgroups monitor in this PR? |
| P2-1 | Docs say “from cgroup assignment”, implementation is “from first sample” |
| P2-2 | After a failed restore, epoch stays Prepared — how do guest metrics recover? |
| P3-1 | After a failed activate persist, in-memory and on-disk state can diverge (follow-up OK) |
| P3-2 | guestWorkloadSnapshotFromTaskStats is unused on the production path (follow-up OK) |
I'm marking Request changes, mainly for these three:
- P1: The new endpoint and
/v1/metricscan coexist. I don't yet see why this PR must also remove the cgroups monitor — please explain the motivation. - P2-1: Default
host_sandboxdocs and implementation disagree; please fix the implementation or the wording. - P2-2: Preferring to export nothing on uncertain failure is fine, but please make recovery clear so metrics do not just disappear quietly.
Questions for the author:
- Why does this PR remove the generic cgroups monitor? How does that relate to the PR goal? Is anyone scraping
container_*from/v1/metrics? If we keep the removal, how will the upgrade notes call it out? - On the host side we can drop usage between sandbox start and the first sample (up to about one
collection_interval). Is that acceptable? If not, can the baseline be established at cgroup assignment time? - After a failed restore, guest metrics may stay unavailable for a long time. Is that the intended final behavior, or should we restore the previous accounting window when the runtime was not actually mutated?
|
|
||
| import ( | ||
| _ "github.com/containerd/containerd/v2/core/metrics/cgroups" | ||
| _ "github.com/containerd/containerd/v2/core/metrics/cgroups/v2" |
There was a problem hiding this comment.
[P1] Why remove the generic cgroups monitor here?
These two imports were deleted:
_ "github.com/containerd/containerd/v2/core/metrics/cgroups"
_ "github.com/containerd/containerd/v2/core/metrics/cgroups/v2"This feels odd to me.
/v1/metrics/resource is a new endpoint, and the docs say it is independent from /v1/metrics. Those two can coexist. I don't see why “add sandbox resource metrics” requires removing the old cgroups TaskMonitor. After this change, container_* series that used to show up on /v1/metrics may simply disappear.
The PR calls it unused, but before merge I'd like clarity on:
- Why remove it in this PR, and how that relates to the PR goal?
- Is anyone scraping
container_*? If we keep the removal, how do upgrade notes tell users?
My preference: if this is not required, restore these side-effect imports and keep this PR focused on sandbox resource metrics. If it must go, please explain the reason and dependency check in the review thread first, then update the changelog / upgrade docs.
There was a problem hiding this comment.
[P1] Why remove the generic cgroups monitor here?
The original removal was based on our observation that the generic monitor does not currently produce usable container_* series for Cube tasks and cannot provide the lifecycle semantics required by this PR.
I agree that this does not justify removing an existing containerd capability as part of the sandbox resource metrics change. I restored both side-effect imports. /v1/metrics/resource is now strictly additive and remains independent from /v1/metrics.
Any future removal of the generic monitor should be handled separately with a usage and compatibility audit.
| "github.com/containerd/plugin/registry" | ||
| ) | ||
|
|
||
| func TestGenericCgroupsTaskMonitorIsNotRegistered(t *testing.T) { |
There was a problem hiding this comment.
[P1 follow-up]
This test hard-locks “the cgroups monitor must never be registered again”.
That's fine only if we have really decided to remove it permanently. Please explain the motivation together with the builtins.go change: if the reason is weak, this test should be reverted with those imports; if we keep the removal, please document the reason and replacement in a comment or the upgrade guide — don't leave only an assertion.
There was a problem hiding this comment.
Agreed. Since the generic cgroups monitor is no longer being removed, I deleted this test as well.
| if baseline == nil || baseline.CGroupPath != cgroupPath { | ||
| previous := baseline | ||
| candidate := hostMetricsBaselineFromUsage(cgroupPath, usage) | ||
| cb.RestoreHostMetricsBaseline(&candidate) |
There was a problem hiding this comment.
[P2] This is a first-sample baseline, not a cgroup-assignment baseline
The implementation writes HostMetricsBaseline from the raw counters on the first successful UsageSnapshot.
But the help text / docs say since host cgroup assignment baseline — i.e. counting from cgroup assignment. With the current timing:
- After the cgroup is assigned and the process is attached, CPU / throttling / memory-failure counters are already increasing;
- The first sample can be up to one
collection_intervallater; - Using that moment as the baseline permanently drops the earlier delta.
Avoiding inherited history when a later sandbox reuses a cgroup path from the pool looks fine (empty baseline, or a changed cgroup path, triggers a rebuild).
What I'm worried about is the other case: default host_sandbox silently drops CPU / throttling / memory-failure counts between sandbox start and the first sample.
Please pick one:
- Establish the baseline in the cgroup assignment path (e.g. after
CleanForReuse, around process attach); the sampler should only read an existing baseline; - Or, if we keep this implementation for now, change the help text and docs to “since first successful sample” and stop saying assignment.
There was a problem hiding this comment.
Fixed by implementing an assignment-time baseline.
Cubelet now reads the raw host counters immediately after the cgroup pool slot is assigned and before the sandbox process is attached. The baseline is carried through the cgroup create result into CubeBox and persisted with the sandbox metadata. The sampler uses this persisted baseline instead of dropping startup usage before its first collection.
For compatibility, a sandbox that already existed before this field was introduced may still establish a baseline from its first successful sample after upgrade.
If assignment-time capture fails for a newly created sandbox, sandbox creation continues, but the failure is persisted and host_sandbox fails closed for that sandbox instead of exporting an incomplete accounting window.
| guestEpoch = prometheus.NewDesc("cubesandbox_guest_workload_metrics_epoch", "Current guest workload metrics epoch generation.", guestLabels, nil) | ||
| guestEpochStarted = prometheus.NewDesc("cubesandbox_guest_workload_metrics_epoch_start_time_seconds", "Start time of the current guest workload metrics epoch as Unix seconds.", guestLabels, nil) | ||
|
|
||
| hostCPUUsage = prometheus.NewDesc("cubesandbox_host_sandbox_cpu_usage_seconds_total", "CPU time consumed by the current sandbox since its host cgroup assignment baseline.", hostLabels, nil) |
There was a problem hiding this comment.
[P2 follow-up]
For example:
CPU time consumed by the current sandbox since its host cgroup assignment baseline.
Compared with the sampler, this is closer to “since first successful sample” than “since cgroup assignment completed”.
Please keep this consistent with the host-sampler comment above: either implement a real assignment-time baseline, or change this (and the EN/ZH docs) to a first-sample baseline. Otherwise users will read “from assignment” and under-count startup usage.
There was a problem hiding this comment.
The implementation now matches this wording: new sandboxes capture and persist the baseline at cgroup assignment time, before process attachment.
The first-successful-sample behavior is retained only as an upgrade compatibility path for existing sandboxes that do not have the new persisted assignment baseline. The English and Chinese documentation now describe both cases explicitly.
| if err := restore(); err != nil { | ||
| return fmt.Errorf("restore sandbox runtime: %w", err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
[P2] After a failed restore, epoch stays Prepared — how do guest metrics recover?
Today prepare first persists a new accounting window as Prepared, then restore runs; on failure it stays Prepared and the sampler skips it. Tests lock this in. I read the intent as: when restore outcome is uncertain, prefer exporting nothing over exporting wrong numbers.
Being conservative when restore may have already mutated the runtime is fine. There is another case though:
preparehas already overwritten the previous usable baseline;- restore fails before the runtime is actually mutated, so the sandbox may still be on the old state;
guest_workloadstays missing until a later successful create/rollback;- there is no clear signal that we are stuck in
Prepared.
Please decide the final behavior and capture it in code or docs:
- If we can tell “runtime not mutated yet” from “uncertain”: restore the previous accounting window in the former case;
- If we keep “on failure, never export”: document how to recover, and add searchable logs or a diagnostic metric so metrics do not disappear quietly.
There was a problem hiding this comment.
I split the failure handling at the point where Cubelet can still determine whether runtime mutation has begun.
Task lookup and other local preflight checks now complete before the new epoch is prepared. A preflight failure therefore preserves the previous usable epoch and does not affect guest metrics.
Once Task.Update has been dispatched, Cubelet cannot reliably determine whether the restore partially mutated the runtime. In that case the new epoch intentionally remains Prepared, and guest_workload stays fail closed rather than exporting counters against a potentially invalid baseline.
Recovery is a later successful rollback, which establishes a new epoch, or deleting and recreating the sandbox if it cannot be recovered. I added a searchable error log with the sandbox context and epoch generation, and documented the recovery behavior in both operator guides.
| setRuntimeRestoreBaseLabels(cb, snapshotID, activatedAt) | ||
| if err := syncer.SyncByID(ctx, cb.ID); err != nil { | ||
| return fmt.Errorf("persist activated guest metrics epoch for %s: %w", cb.ID, err) | ||
| } |
There was a problem hiding this comment.
[P3]
Here the in-memory state moves from Prepared to Pending, then SyncByID persists it. If persist fails:
- memory is already
Pending(the sampler can keep going) - disk may still be
Prepared
If Cubelet restarts, it reloads Prepared from disk and guest metrics get stuck again. In the same file, prepare/ready failure paths restore in-memory state; activate does not. The caller mostly gets a warning and may still think rollback succeeded.
Not a merge blocker for me. Prefer rolling memory back or retrying persist on failure; at least mark it abnormal and emit a searchable error.
There was a problem hiding this comment.
Fixed. Before activation, Cubelet now snapshots the previous Prepared epoch and the existing snapshot/restore binding labels.
If persistence fails after the in-memory transition to Pending, it conditionally restores the previous epoch and labels. The restoration checks the expected generation and state first, so it does not overwrite a concurrent successful transition.
| }, nil | ||
| } | ||
|
|
||
| func guestWorkloadSnapshotFromTaskStats( |
There was a problem hiding this comment.
[P3]
guestWorkloadSnapshotFromTaskStats is mainly used by transport-layer tests; production collection goes through Metrics() and normalization in the sampler. That makes the lower-level sandbox controller depend upward on resourcemetrics.
Not a merge blocker. Please move it to a test helper or a separate adapter instead of leaving it on the controller.
There was a problem hiding this comment.
Fixed. guestWorkloadSnapshotFromTaskStats has been removed from the production sandbox controller and moved into resource_metrics_transport_test.go as a test-only helper.
The production controller now only exposes the standard Metrics() transport boundary; decoding and normalization remain owned by the resource metrics layer.
Query the primary Cube task through standard Task.Stats, define typed guest workload metrics, and persist guest counter baselines across create, restore, and rollback lifecycle events. Read host sandbox cgroup usage with baselines captured when pool slots are assigned so reused cgroup paths do not inherit prior sandbox accounting. Fail host metrics closed without blocking sandbox creation when assignment-time capture is unavailable. Preflight rollback task lookup before preparing a new guest epoch, and keep uncertain runtime restore failures fail closed with explicit recovery guidance. Add focused store, lifecycle, cgroup reader, and normalization tests. Signed-off-by: zhengyilei <zheng_yilei@qq.com>
Add bounded guest workload and host sandbox samplers with independent concurrency limits, fixed per-cycle queues, and latest-sample caching. Expose selected scopes through a dedicated Prometheus endpoint while keeping the existing generic containerd cgroup monitor available under the generic metrics endpoint. Add focused scheduling, cache, scope, Prometheus, configuration, and race tests. Signed-off-by: zhengyilei <zheng_yilei@qq.com>
Document the host_sandbox and guest_workload scopes, lifecycle counter semantics, configuration, Prometheus endpoint, and operational guidance. Add aligned English and Chinese guides and register them in the documentation navigation. Signed-off-by: zhengyilei <zheng_yilei@qq.com>
b179ec5 to
c0aa5f1
Compare
|
@fslongjin Thanks for the detailed review. I have updated the PR to address all five points.
Validation completed on real deployment:
|
| } | ||
| } | ||
|
|
||
| sandBox, err := l.cubeboxManger.Get(ctx, opts.GetSandboxID()) |
There was a problem hiding this comment.
Best-effort epoch initialization silently swallows the store Get error. If l.cubeboxManger.Get fails here (e.g., a transient metadata-store read error right after createCubeboxContainer persisted the sandbox), the new sandbox is left with no GuestMetricsEpoch. The guest sampler's CollectOnce then hits the epoch == nil branch and permanently deletes the workload — guest_workload is never exported for this sandbox and nothing ever retries. The failure is only a Warnf in the Cubelet log. Consider surfacing this (e.g., a visible per-sandbox error) or letting the sampler lazily begin an epoch for started, epoch-less sandboxes so a transient failure doesn't permanently disable guest metrics.
| if err != nil { | ||
| return ret.Errorf(errorcode.ErrorCode_CreateCgroupFailed, "%s", err) | ||
| } | ||
| cgroupPath := MakeCgroupPathByID(*fullCgID) |
There was a problem hiding this comment.
A baseline-capture failure here is permanent: HostMetricsBaselineMissingAtAssignment is set once and never retried, so host_sandbox is disabled for the sandbox until it is deleted and recreated. If this read fails transiently at create time (cgroup momentarily unreadable, controller not yet enabled in the reused slot), a healthy sandbox loses host metrics permanently — the operator only sees a Warnf. Note the asymmetry: pre-upgrade sandboxes (nil baseline + flag false) get a compatibility baseline from the first successful sample in HostSandboxSampler.collect, but post-upgrade sandboxes with a one-shot read failure never do. Consider falling back to the same first-successful-sample baseline path when the assignment-time read fails, instead of a permanent fail-closed flag.
| } | ||
| } | ||
|
|
||
| func newHostSandboxSnapshot(sandboxID, cgroupPath string, timestamp time.Time, usage handle.UsageSnapshot) *HostSandboxSnapshot { |
There was a problem hiding this comment.
MemoryOOMTotal/MemoryOOMKillsTotal are collected here, persisted in the host baseline (hostMetricsBaselineFromUsage), and subtracted in normalizeHostSandboxUsage — but the Prometheus collector only emits MemoryFailuresTotal. These OOM counters are never exported, so the whole OOM accounting path is currently dead state. Also note the sources are inconsistent: the v1 usageSnapshotFromMetrics populates only MemoryOOMKillsTotal (never MemoryOOMTotal, which stays 0), while v2 populates both. Either export them (and fix the v1 gap) or drop the fields to avoid maintaining unused, cross-version-inconsistent state.
| type WorkloadRef struct { | ||
| SandboxID string | ||
| ContainerID string | ||
| } |
There was a problem hiding this comment.
DecodeGuestWorkloadMetric hard-requires the cgroup-v1 metrics type URL and treats Usage.Total/User/Kernel as nanoseconds. The guest cube-agent (agent/rustjail/src/cgroups/fs/mod.rs get_cpuacct_stats) reads cgroup-v2 cpu.stat usage_usec/user_usec/system_usec, which are microseconds, and puts them into CpuUsage.total_usage/usage_in_usermode/usage_in_kernelmode without scaling. So the CubeShim conversion (PR #1090) must multiply by 1000 when building this v1 payload — if it doesn't, exported cubesandbox_guest_workload_cpu_*_seconds_total counters are under-reported by 1000×. Worth explicitly verifying the shim's conversion is ns-scaled and that io.containerd.cgroups.v1.Metrics is exactly the type URL CubeShim emits (the docs say the guest requires a unified cgroup v2 hierarchy, which would otherwise suggest a v2 metrics type).
3 participants
Motivation
PR #1090 adds the standard guest
Task.Statstransport, but raw cumulative counters are not sufficient as a stable sandbox metrics contract. Template restore and rollback can inherit or rewind guest counters, while reusable host cgroup slots can retain counters from a previous sandbox. Query-time collection would also make every Prometheus scrape fan out to active sandboxes.This PR makes Cubelet the owner of lifecycle-aware CPU and memory semantics, bounded collection, latest-sample caching, and Prometheus export.
What Changed
Metricsby querying the primary Cube task through standard containerdTask.Statsand converting transport failures to native containerd errors.guest_workloadandhost_sandboxas independent accounting scopes;allreturns both without adding them together.GET /v1/metrics/resource.export_scopes; default tohost_sandbox, withguest_workloadandallavailable through configuration./v1/metrics; the Cube-native resource endpoint is additive and independent.The PR is organized as three commits so lifecycle accounting, collection/export, and operator documentation can be reviewed in that order.
Metrics Semantics
CPU total/user/system, throttling, periods, and memory failures are cumulative counters. Memory current and limit are point-in-time gauges. Cubelet exports base values; CPU cores and percentage are derived with Prometheus rates rather than precomputed in the runtime.
Fresh create, clone from inherited state, rollback, and workload recreation start a new
guest_workloadepoch and baseline. Snapshot/commit, pause/resume, and Cubelet restart preserve the current epoch. Paused, deleted, unavailable, or stale views disappear instead of exporting zero.host_sandboxuses a separate baseline for each sandbox/cgroup assignment. New sandboxes capture this baseline when the pool slot is assigned, before the sandbox process is attached. Existing sandboxes upgraded without a persisted baseline establish a compatibility baseline from their first successful sample. If assignment-time capture fails, sandbox creation continues but host metrics fail closed for that sandbox instead of exporting an incomplete counter window. Host accounting remains continuous across guest rollback because the host process and cgroup assignment are not rewound.Prometheus reads only the latest typed cache. A scrape never initiates
Task.Stats, guest RPCs, or host cgroup reads.Exact epoch-scoped memory peak remains intentionally unexported because inherited maxima cannot be normalized by subtraction and sampled peak tracking can miss values between samples.
Review Focus
Rollback counter semantics
Rollback keeps the same
sandbox_idbut restores an earlier guest kernel and cgroup state. Cubelet therefore persists a prepared guest epoch before entering the shim restore, does not sample the prepared epoch, and activates a new pending baseline only after successful restore.Task lookup and other local preflight checks complete before epoch preparation, so preflight failure preserves the current epoch. Preparation failure aborts before runtime mutation. A failed or uncertain restore leaves the new epoch prepared and
guest_workloadfail closed; a later successful rollback establishes a new epoch, while an unrecoverable sandbox must be deleted and recreated.host_sandboxcounters remain continuous across rollback.Relationship to #1022
This PR provides the Cubelet-owned collection/cache layer that can be reused by a later public sandbox metrics API. It does not add
GET /sandboxes/{id}/metrics, query-time envd collection, range history, or E2B-compatible response routing.Scope
This PR is limited to CPU and memory for the primary Cube workload container and the host sandbox cgroup. It does not add disk/network/filesystem metrics, OTLP, historical storage, Cube Guard, vsock metrics forwarding, eBPF collection, or a generic collector for non-Cube containerd runtimes.
Validation
c0aa5f1passes the full CPU/memory, snapshot/rollback, pause/resume, deletion, and cleanup lifecycle E2E through CubeAPI and CubeProxy.