Skip to content

feat(deploy): scope default sandbox memory budgets to Helm - #154

Merged
rcyw merged 12 commits into
inclusionAI:developfrom
iamWHTWD:default-memory-reserve-16g
Nov 9, 2026
Merged

rcyw merged 12 commits into
inclusionAI:developfrom
iamWHTWD:default-memory-reserve-16g

Conversation

@iamWHTWD

@iamWHTWD iamWHTWD commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

Enable the existing aggregate sandbox parent memory policy with a 16 GiB reserve in the ordinary direct Helm profile. Keep standalone, both cloud Terraform profiles and PVM disabled by default, with explicit operator configuration for qualified deployments.

Changes

  • Direct Helm enables plugin.resource.memory_limit with total_reserve="16GiB". Its existing sandboxd capacity source is unchanged.
  • Standalone keeps enabled=false and the bundled ADX auto capacity source. The default source conversion, enabled-memory CI fixture and SDK budget qualification added in earlier revisions have been removed.
  • Both cloud profiles default to sandbox_memory_limit_enabled=false, expose the reserve and opt-in through Terraform/guided configuration, and retain their OS defaults.
  • PVM defaults to disabled through AKERNEL_PVM_MEMORY_LIMIT_ENABLED=false / Helm node.config.sandboxd.pvmMemoryLimitEnabled=false. Explicit opt-in preserves the selected reserve. Older valid custom profiles with an omitted memory table/field remain supported. Its qualification fixture keeps enforcement disabled and retains strict no-skip checks.
  • Merge develop 724238c, preserving its maintained PVM guide and resolving the documentation conflict.
  • ADX, sandboxd, runtime archives and component gitlinks have no net changes against develop. This scope does not require a new upstream ADX release.

A packaged-CLI regression verifies that standard/PVM × plain/telemetry/shared-storage preparation preserves the complete original auto resource-source mapping.

Validation

Final source: bc3aabdd01a8be7176cbcd6e89f56827b707cef7.

Final-head CI passed: ten successful jobs, including the complete production image build and Standalone E2E; image publication was skipped as expected.

ECS focused checks: 186 passed, zero skipped, covering standalone configuration/platform, PVM profile contracts, Helm storage/memory rendering, evaluated Terraform/cloud contracts, build helpers, real released ADX CLI configurations and the SDK runner.

The isolated runtime comparison used the released e611 ADX payload and unchanged matching runtime rootfs, freshly compiled fbe69 sandboxd/sbox/logger binaries and the final production configuration. This was a qualification assembly, not a complete node Dockerfile build.

Outer CPU quota Develop 724238c public capacity This head public capacity This head SDK allocation / release
500m 500m 500m 500 → 0 → 500m
1500m 1000m 1000m 1000 → 0 → 1000m

Both SDK lifecycle checks created a sandbox with a 1024 MiB request, executed a command, deleted it and verified full allocation recovery. Memory followed 8192 → 7168 → 8192 MiB. TLS/hostname verification was enabled, the default loop-backed ext4 filestore was retained, the sandbox parent remained unlimited (memory.max=max), and OOM counters remained zero. Owned runtime resources were cleaned up and all seven existing host services, original loop mapping and listeners were preserved.

The initial exact-1500m precision assertion failed. The exact develop control reproduced the same 1000m result. This is an existing released ADX auto limitation in this environment; the comparison establishes unchanged behavior, not a fractional-CPU fix. Existing sandboxd-source CPU precision limitations are also outside this PR's reduced scope.

Rollout boundaries

Enabling the policy requires a qualified cgroup v2 node/image and effective memory above the selected reserve. Standalone operators must also select and qualify a compatible scheduling resource source before manually enabling a parent cap. PVM finite-cap runtime qualification remains separate; this PR defers that default enablement.

Adding or lowering a cap requires draining the sandbox subtree; an existing stricter cap is retained. Preserve canary rollout and rollback planning. Distill-fs keeps its independent limit. The reserve is deployment policy, not a measured universal host-OOM safety guarantee; production peaks and OOM-pressure qualification remain operator/profile-specific.

wanghongtao.wangho added 2 commits November 5, 2026 18:00
Enable the aggregate sandbox memory policy with a 16 GiB total reserve in
the standalone, Helm, Aliyun and Huawei templates. Document the cgroup v2
and effective-memory requirements, explicit overrides and migration of
existing sandbox subtrees.

Keep the policy enabled in small CI and PVM fixtures with an explicit 2 GiB
reserve, and verify that the parent hard limit matches resource reporting.
Add configuration-generation and override regressions across deployment
paths. The default is deployment policy, not a host OOM safety guarantee.

Signed-off-by: wanghongtao.wangho <wanghongtao.wangho@antgroup.com>
Document the observed standalone ADX capacity-source mismatch and the cloud operating-system requirements before treating the new memory policy as a deployment-wide default.

Signed-off-by: wanghongtao.wangho <wanghongtao.wangho@antgroup.com>
@iamWHTWD
iamWHTWD marked this pull request as ready for review November 5, 2026 10:52

rcyw commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator

Reviewed 49fdde41ad8654d89653f8674e994ea2a18c3787 against b42a6912daa061c7b2c8438c368236e76d5c2a53.

The aggregate sandbox memory limit and a single deployment reserve are a reasonable design direction. The current default-on rollout should wait for the two P1 fixes below. These are also acknowledged in the PR description; documenting them as prerequisites does not resolve the production configuration.

  1. R1 / P1 — connect standalone scheduling capacity to the actual sandbox budget. At deploy/standalone/config/sandboxd_config.toml:32–34, the policy is enabled, but the bundled ADX profile still inherits resource_source.kind: auto. I independently resolved the actual production template using the checksum-qualified packaged adxctl pinned by the Dockerfile (source d2337849eff8ffcd39b3a3d34d91840ae573b508). It resolves to:

    resource_source:
      kind: auto
      disk_path: /home/akernel
      valid_for_seconds: 30
    

    This source observes adxlet's own cgroup/ancestors rather than the sandboxd parent/socket, so the scheduler can advertise more memory than the new sandbox parent can sustain. The independent configuration-contract check passes on the base with the policy disabled and fails on this head with it enabled. Replacing the complete tagged source mapping with kind: sandboxd, socket: /var/run/resource.sock and valid_for_seconds: 30 in an explicitly resolved temporary configuration passes the packaged CLI's validation.

    Please implement this in the production generation path, covering the maintained standalone/PVM and derived profiles. Avoid recursive merging that leaves auto fields behind. Add a regression against the fully resolved configuration and verify that /akernel/memory.max, /resource.mem, and public ADX capacity/allocatable agree under finite ancestors, including allocation and release after create/delete. The author's ECS control/treatment results support this direction, but their replacement remains a temporary fixture rather than the production fix.

  2. R2 / P1 — make the Aliyun default OS compatible with the enabled policy, or narrow the default rollout. deploy/terraform/aliyun/values-akernel.yaml.tmpl:168–170 enables a v2-only policy, while variables.tf:184–188 still selects standard AliyunLinux3; the node-pool paths consume that default and bootstrap does not change cgroup mode. Standard Alinux 3 defaults to cgroup v1 (official OS comparison, ACK migration guidance). Pinned sandboxd rejects the enabled policy with memory_limit requires cgroup v2, and the server propagates that initialization error. I verified the disabled/enabled v1 behavior through its existing pure fixture tests; I did not provision a cloud node.

    Please select and qualify a compatible v2 image/default, or keep unsupported/unqualified profiles explicitly opt-in. A documentation warning alone leaves the default combination unable to start. Huawei's selected image also needs actual cgroup-mode qualification; I have not established a separate Huawei startup defect.

Before a broad default rollout, also requalify the new enabled, finite-cap PVM fixture on the maintained matching host profile. The 16 GiB reserve still needs profile-specific sizing/qualification evidence; it is deployment policy rather than a measured universal host-OOM safety budget. Keeping unqualified profiles opt-in is a reasonable way to reduce the initial scope. The existing documentation already explains smaller-node overrides, cap preservation, draining before adding/lowering a cap, and the independent distill-fs budget; preserve those contracts and the canary/drain upgrade procedure.

Independent validation on this exact head:

Check Result
Standalone helper suite PASS: 86 tests
Helm configuration suite PASS: 18 tests
Deployment script syntax and umbrella Helm lint PASS
Terraform contracts 4 PASS, 5 SKIP because Terraform is unavailable locally
PVM host-profile checks PASS: 2 static tests; not PVM runtime qualification
Pinned sandboxd memory-policy fixture tests PASS: 10 top-level tests and 34 subcases
Actual standard-CI and PVM configuration rewrites PASS: both retain enabled policy and select 2 GiB
core/umbrella × standard/Kruise render comparisons PASS: other sandboxd settings unchanged; DaemonSet differs only in the expected node-config checksum; Helm capacity source remains sandboxd
Both cloud TOML comparisons PASS: other sandboxd settings unchanged
Standalone capacity-source contract Base PASS / exit 0; head FAIL / exit 1 (R1)

CI run 42009405813 has ten successful checks and an expected skipped publication job. Its merge commit and this head have identical source trees. The successful Standalone E2E uses the 2 GiB reserve, and the new memory assertion checks parent/socket equality, not public ADX scheduling capacity. Green CI therefore does not close R1 or qualify the 16 GiB default/PVM path.

My independent checks used isolated source worktrees, the verified release CLI, rendered configurations and temporary-file fixtures. They did not start a live standalone/cloud deployment, modify host cgroups, run real PVM/default-storage workloads, or perform OOM-pressure testing. The PR author's ECS runtime measurements are separate evidence.

Please land R1/R2, add the production-path regressions and remaining qualification evidence (or explicitly narrow the rollout), then rerun the affected checks and CI on the resulting head before merging.

wanghongtao.wangho added 4 commits November 6, 2026 10:44
Resolve the bundled standalone ADX profile at image build time before replacing its complete capacity source with sandboxd. Preserve explicit custom sources and verify resolved standard, PVM, trace and shared-storage configurations.

Keep unqualified cloud OS profiles opt-in, exposing enabled and reserve inputs through Terraform and guided configuration. Add public scheduling capacity and allocation-release E2E coverage alongside the parent/socket assertion.

Signed-off-by: wanghongtao.wangho <wanghongtao.wangho@antgroup.com>
…ew-followup

Signed-off-by: wanghongtao.wangho <wanghongtao.wangho@antgroup.com>
Match public whole-MiB memory reporting and exercise fractional CPU
allocation in isolated qualification. Pass the measured parent cap to PVM
tests and retain strict skipped-test checks.

Keep the shared ACK fixture importable on Python 3.10, and include its
current policy file and boolean input in the cloud fixtures.

Signed-off-by: wanghongtao.wangho <wanghongtao.wangho@antgroup.com>
Document whole-MiB public memory reporting and the optional CPU budget
used by isolated qualification. Retain byte-level parent and socket
checks alongside public allocation and release assertions.

Signed-off-by: wanghongtao.wangho <wanghongtao.wangho@antgroup.com>

iamWHTWD commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator Author

@rcyw The fixes have been pushed to d676b67a1e27dab2a321b1971b64866a03c6cddf. I have also updated the PR description to reflect the implementation, validation evidence and remaining work. Following up on your review:

  1. R1: fixed in the production configuration-generation path. Image builds now resolve the complete standalone configuration with the packaged adxctl, then replace the entire tagged resource_source mapping with the sandboxd socket source. This avoids retaining auto.disk_path through recursive merging. Regression coverage includes standard/PVM × plain/telemetry/AFS and compares the complete resolved configuration to check for unrelated changes. Both the standard CI and PVM scripts pass the measured parent cgroup limit to SDK E2E, which checks public capacity, allocation after creation and recovery after deletion.

  2. R2: narrowed the default rollout. Both Aliyun and Huawei Terraform profiles now default to sandbox_memory_limit_enabled=false, with explicit opt-in, configurable reserve and matching guided inputs. Existing OS defaults are unchanged, and unqualified cloud images are not enabled by default. Standalone/direct Helm retain the 16 GiB reserve; bounded CI/PVM fixtures explicitly select 2 GiB.

  3. Additional regression fixes. Public memory assertions now round down to whole MiB and cover non-MiB-aligned limits. The shared fixture remains importable on Python 3.10, and the current ACK policy file and boolean input are included in the fixtures. PVM reads a finite positive budget from the actual parent cgroup while retaining strict zero-skip checks.

Validation completed:

  • Python 3.10 SDK: 442 unit tests, 35 benchmark-driver tests, Ruff and Mypy passed.
  • Actual Python 3.10 ACK/Terraform fixtures: 20 passed; cloud Terraform contracts: 13 passed; build helpers: 77 passed.
  • Earlier isolated ECS tests using released component payloads and the production configuration-generation fix verified 24 GiB / 16 GiB reserve → 8 GiB and 8 GiB / 2 GiB reserve → 6 GiB. Public capacity, allocation for a 1 GiB runsc sandbox and recovery after deletion agreed. These tests used the default loop-backed ext4 filestore.
  • Additional independent CPU-candidate qualification covered 24 GiB + 4 KiB / 16 GiB reserve / 500m and 8 GiB + 4 KiB / 2 GiB reserve / 1500m. Parent cgroup/socket limits were 8589938688 and 6442455040 bytes; public capacities were 500m / 8192 MiB and 1500m / 6144 MiB. Creation, command execution, deletion and full resource recovery passed. Parent and ancestor OOM counters remained zero, and owned test resources were cleaned up.

Remaining limitations: The fractional-CPU results used independent local sandboxd/ADX candidates and a qualification overlay. The component versions pinned by this PR do not yet include that protocol fix; the released pair still loses fractional CPU precision through the legacy integer-core field. Integrating a matching formal ADX release and sandboxd gitlink remains necessary, so this is not claimed as a fix delivered by the current PR. The full sandboxd Go suite also had one unmount-permission failure without CAP_SYS_ADMIN, reproduced on the unchanged baseline.

The enabled finite-cap PVM fixture still requires runtime qualification on the maintained matching host profile. Production component peaks and OOM-pressure behavior have not been qualified; ordinary Linux runsc/script tests and zero OOM events in bounded tests do not establish those guarantees.

Please re-review the R1/R2 implementation and regression coverage. The remaining integration and qualification work is explicitly retained as a limitation before merge or broader rollout.

rcyw commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator

Follow-up review of d676b67a1e27dab2a321b1971b64866a03c6cddf, following the author’s update. The aggregate memory ceiling and single-reserve design remain reasonable. R1 and R2 are verified fixed, but approval should remain on hold until the following merge requirements are met.

  1. R3 / P1 — Integrate the fractional-CPU fix into the components actually delivered by this PR. The default standalone source conversion now selects sandboxd instead of ADX auto. Pinned sandboxd fbe69e566859718414a207fa780b8dab363b9381 truncates provider millicores to integer cores before serving /resource; pinned ADX e611b9e4a83dcd6da5346bd17c2f6af28e244952 reads that legacy integer field and multiplies it by 1000. The former auto quota detection preserves fractional CPU, so the default switch exposes a standalone compatibility regression.

My independent regression exercised the pinned producer’s real refresh, memory ceiling and Unix HTTP socket with a fake capacity provider. The downstream values below follow the checked released ADX parser; this was not a full ADX/SDK deployment:

Provider budget Actual socket cpu (cores) Released consumer interpretation Result
500m 0 0m FAIL: all CPU capacity lost
1500m 1 1000m FAIL: 500m lost
2000m 2 2000m PASS: integer control

Memory was correct in all cases: 6442450944 bytes (6 GiB).

Before merge, integrate the backward-compatible millicore producer/consumer fix, update the sandboxd gitlink and matching formal ADX/component archive pins, and verify public capacity plus allocation/release at 500m and 1500m using those actual delivered artifacts. The independent locally compiled candidates described in the PR establish useful evidence, but the current pins still lack that fix. The acceptance regression should exercise the new consumer’s actual precision-field selection and legacy fallback, not reinterpret the old integer-core field with different units.

  1. Qualify the enabled finite-cap PVM profile, or narrow its enablement scope. The maintained PVM fixture now uses enabled + 2 GiB reserve and exports the measured finite parent limit to SDK E2E. Its five static/profile/script tests pass. The matching maintained PVM host-profile runtime qualification remains outstanding, as the author notes. Before merge, run the maintained bounded PVM qualification with the final component/profile inputs and record the finite parent/socket/public capacity and operation/recovery/cleanup results, or defer the unqualified profile enablement in this PR. Ordinary Linux runsc, static script checks and the separate CPU-candidate qualification do not establish that PVM result.

  2. Resolve the conflict with latest develop and rerun CI on the resulting head. Develop is now 724238ccf2d1b582824100f312f2031250daa3dc, including fix(deploy): prepare runc host filesystems before node startup #153. git merge-tree --write-tree --messages independently reports a content conflict in deploy/pvm-runtime.md, consistent with GitHub’s mergeable=false. Preserve fix(deploy): prepare runc host filesystems before node startup #153’s maintained guide and this PR’s memory-budget contract when resolving it. CI 42159796105 succeeds for the current head, with ten successful jobs and the expected skipped image-publish job, but its checkout is merge 4bfcfc02872b7e77809a79c81913e917bbf9631a against old develop b43d548. New component pins, conflict resolution or scope changes require successful CI on the resulting head.

Verified progress: R1 now resolves the complete bundled deployment with the packaged CLI at image-build time and replaces the whole tagged resource-source mapping. Standard/PVM × plain/telemetry/AFS configuration comparisons pass without unrelated resolved-configuration changes; explicit custom sources remain preserved. R2 now leaves both cloud Terraform profiles disabled by default, with explicit enabled/reserve inputs wired through guided configuration and templates; OS defaults are unchanged.

Independent checks on this head:

Check Result
Full AFS/config suite with exact pinned ADX sources and real packaged CLI 19 passed; includes six production configuration combinations
Terraform/cloud/guided contracts 13 passed, no skips; provider-free temporary fixtures only
Standalone helpers 86 passed
Build helpers 77 passed
Helm memory/storage contracts 18 passed
SDK memory/CPU unit fixture 1 test / 5 scenarios passed; mocked resource views
PVM profile/finite-budget script checks 5 passed; static qualification only
Deployment script syntax Passed
Fractional-CPU socket regression 500m/1500m failed; 2000m control passed
Merge against current develop Failed with the content conflict above

The configuration suite initially lacked fixed ADX example files; after supplying the exact e611b9e source, the complete unchanged suite passed. That preparation failure is retained locally and is not treated as a product defect. Local review did not change host cgroups/modules or run a complete backend, PVM, cloud-node or OOM-pressure campaign. The author’s released-payload and separate candidate-runtime results retain their stated provenance.

Reproduction against the currently pinned sandboxd and legacy released consumer contract

In a disposable checkout of sandboxd fbe69e566859718414a207fa780b8dab363b9381, save this as pkg/resourcemanager/pr154_review_resource_socket_test.go and run:

go test ./pkg/resourcemanager -run '^TestPR154ResourceSocketKeepsFractionalCPUBudget$' -count=1 -v

The two fractional cases fail and the integer control passes on the shipped pins. This reproduces the frozen legacy pair; after a protocol upgrade, verify the actual new consumer as described above. Source SHA-256: 735cb26b660b7cdce3c71a17e87db76ebd4f9320d841059ff6e0a52315ca08c1.

package resourcemanager

import (
	"context"
	"encoding/json"
	"fmt"
	"net"
	"net/http"
	"os"
	"path/filepath"
	"testing"
	"time"

	"github.com/stretchr/testify/require"
)

// Exercise the pinned producer's real Unix HTTP socket with a fake provider.
// The released e611 ADX consumer converts the legacy cpu core field to millis.
// No runtime, cgroup or full ADX deployment is started by this regression.
func TestPR154ResourceSocketKeepsFractionalCPUBudget(t *testing.T) {
	for _, budget := range []int64{500, 1500, 2000} {
		t.Run(fmt.Sprintf("%dm", budget), func(t *testing.T) {
			root, err := os.MkdirTemp("", "pr154-cpu-")
			require.NoError(t, err)
			t.Cleanup(func() { _ = os.RemoveAll(root) })
			m := &Module{
				nodeResource: &stubNodeResourceManager{cpu: budget, mem: 8 << 30},
				sockPath:     filepath.Join(root, "resource.sock"),
				stopCh:       make(chan struct{}),
			}
			require.NoError(t, m.SetMemoryLimit("/akernel", 6<<30))
			require.NoError(t, m.Start())
			t.Cleanup(m.Stop)
			require.Eventually(t, m.Healthy, time.Second, time.Millisecond)
			transport := &http.Transport{
				DialContext: func(ctx context.Context, _, _ string) (net.Conn, error) {
					return (&net.Dialer{}).DialContext(ctx, "unix", m.sockPath)
				},
			}
			t.Cleanup(transport.CloseIdleConnections)
			response, err := (&http.Client{Transport: transport, Timeout: time.Second}).Get("http://sandboxd/resource")
			require.NoError(t, err)
			defer response.Body.Close()
			require.Equal(t, http.StatusOK, response.StatusCode)
			var sample resourceInfo
			require.NoError(t, json.NewDecoder(response.Body).Decode(&sample))
			cachedCPU, cachedMemory := m.Capacity()
			observed := sample.Cpu * 1000
			t.Logf("provider=%dm; socket legacy cpu=%d; released consumer=%dm; cached=%dm; memory=%d", budget, sample.Cpu, observed, cachedCPU, sample.Mem)
			require.Equal(t, int64(6<<30), sample.Mem)
			require.Equal(t, sample.Mem, cachedMemory)
			require.Equal(t, observed, cachedCPU)
			if observed != budget {
				t.Errorf("resource socket loses fractional CPU: provider=%dm, released consumer=%dm", budget, observed)
			}
		})
	}
}

Please keep this PR unmerged until these integration/qualification requirements are satisfied and the resulting head’s CI is green. No approval or merge performed in this follow-up.

wanghongtao.wangho added 4 commits November 6, 2026 16:54
Preserve the maintained PVM guide and its qualification boundaries.
Retain the sandbox capacity source and document the deferred PVM memory
policy qualification.

Signed-off-by: wanghongtao.wangho <wanghongtao.wangho@antgroup.com>
Keep the aggregate parent policy disabled for PVM unless an operator
explicitly enables it and qualifies the selected reserve. Retain the
ordinary KVM defaults and keep PVM smoke tests free of unsupported budget
qualification claims.

Advance sandboxd to preserve millicores in its resource observations. The
matching ADX consumer candidate is qualified through release overrides;
its formal archive pins must be published before integration.

Signed-off-by: wanghongtao.wangho <wanghongtao.wangho@antgroup.com>
Keep older custom PVM profiles valid when the disabled memory policy is
omitted. Normalize supported quoted boolean keys while retaining strict
validation for duplicate fields, invalid types and explicit opt-in.

Signed-off-by: wanghongtao.wangho <wanghongtao.wangho@antgroup.com>
Leave standalone memory enforcement disabled and retain its existing
auto capacity source. Defer the source switch and remove the independent
CPU candidate from the parent dependency pins.

Keep cloud and PVM profiles explicitly opt-in. Restore the ordinary
standalone CI and SDK contracts, and verify with the packaged CLI that
all maintained standalone variants preserve their original auto source.

Signed-off-by: wanghongtao.wangho <wanghongtao.wangho@antgroup.com>
@iamWHTWD iamWHTWD changed the title feat(deploy): enable parent memory limits with a 16 GiB reserve feat(deploy): scope default sandbox memory budgets to Helm Nov 6, 2026

iamWHTWD commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator Author

@rcyw Updated to bc3aabdd, with develop 724238c merged and the PVM-guide conflict resolved. I have narrowed the rollout following your follow-up review:

  • Default enablement is limited to the ordinary direct Helm profile, with a 16 GiB reserve. Its existing sandboxd capacity source is unchanged.
  • Standalone remains disabled and retains its original ADX auto source. The default source conversion, enabled-memory CI fixture, SDK budget qualification and independent CPU-candidate changes have been withdrawn. Component gitlinks and archive pins have no net changes against develop; this scope requires no new ADX release.
  • Cloud and PVM parent-memory enforcement remain disabled by default, with explicit opt-in. The unqualified enabled finite-cap PVM fixture is deferred, while strict no-skip checks are preserved.

This removes the new standalone triggering paths behind R1/R3. It does not claim to fix upstream fractional-CPU handling; existing Helm sandboxd-source CPU behavior is unchanged.

Verification: 186 focused ECS checks passed with zero skips. The released e611 CLI confirms that all six standard/PVM × plain/telemetry/shared-storage variants preserve the complete original auto mapping.

Outer CPU quota Develop 724238c public capacity This head public capacity
500m 500m 500m
1500m 1000m 1000m

The initial exact-1500m assertion failed and was retained; the exact develop control reproduced the same result. This is unchanged released-ADX behavior in this environment, not a 1500m precision pass. At the observed schedulable budgets, both SDK create/exec/delete tests passed, with CPU and the 1024 MiB allocation fully restored after deletion. TLS verification remained enabled, the default ext4 loop filestore was retained, OOM counters stayed zero, and original host services were preserved.

The ECS image was a qualification assembly using the unmodified released e611 payload/rootfs, freshly compiled fbe69 binaries and final configuration. Separately, CI on this head passed all ten checks, including the full production Dockerfile build and Standalone E2E; publication was skipped as expected.

Please re-review the reduced scope.

rcyw commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator

Follow-up review of bc3aabdd01a8be7176cbcd6e89f56827b707cef7, including an independent integration check with current develop f81d033765335260fa74d4c583a27ccd29bce452 after #148.

The reduced rollout is reasonable: ordinary direct Helm retains its existing sandboxd resource source; standalone retains its original auto source; cloud/PVM standard defaults remain disabled. The earlier new standalone fractional-CPU triggering path has been withdrawn, so this reduced scope does not require a new ADX release.

P2 — fix the PVM false/opt-out contract in both Helm and standalone before merging.

The Helm guard and standalone guard/rewrite recognize only the literal [plugin.resource.memory_limit] table spelling. A valid custom TOML configuration can express the same policy using a quoted table name:

[plugin."resource".memory_limit]
enabled = true
total_reserve = "16GiB"

Or, inside the existing [plugin.resource] table, replace the separate memory-limit table with:

memory_limit = { enabled = true, total_reserve = "16GiB" }

Both inputs parse to exactly the same complete configuration as the canonical table. However, the guard sees zero matching memory fields, treats the policy as omitted and skips its rewrite. With PVM selected and the switch explicitly false, the effective output remains enabled=true, contrary to the documented override contract.

I reproduced this through real Helm rendering and the actual standalone configure_network() generator using the existing temporary-file fixture. The results are identical on this head and on the local integration with develop f81d033:

Entry point, PVM switch explicitly false Canonical table Quoted table Inline table
Helm: pvmMemoryLimitEnabled=false enabled=false enabled=true enabled=true
Standalone: AKERNEL_PVM_MEMORY_LIMIT_ENABLED=false enabled=false enabled=true enabled=true

The canonical/default controls pass. This defect is limited to custom PVM configurations; it can preserve finite parent enforcement even when the operator explicitly disables it. These are configuration results, not a live PVM/cgroup runtime reproduction.

Before merge:

  1. Fix both generators so the switch controls the effective memory_limit.enabled by TOML semantics, or explicitly reject unsupported representations before emitting a configuration. Preserve compatibility for genuinely absent disabled policies and leave KVM behavior unchanged.
  2. Add regressions for canonical, quoted-table and inline-table forms in both entry points. Assert the final parsed TOML with false/true switches, retaining the existing missing-policy and invalid/duplicate-field protections. The current quoted-enabled-key tests do not cover quoted table names or inline tables.
  3. Integrate the latest develop and rerun affected checks plus the full CI on the resulting head/merge combination. CI 42197801592 is green for this head, but its checked merge still uses develop 724238c. The newer f81d033 integration needs its own resulting-head CI after the fix.

Independent validation retained:

  • 229 checks passed, zero skips on bc3aabd: standalone, packaged-CLI/AFS, Terraform/guided configuration, PVM static contracts, Helm memory/storage and build helpers; deployment syntax also passed.
  • 166 checks passed, zero skips on the isolated f81d033 integration, including the PR148 rootfs-pin/shared-memory configuration combination. Object-level merge was conflict-free and retained develop's SDK/component pins.
  • Each semantic probe has two passing canonical controls and four failing equivalent-form cases, on both inputs. No host cgroups, mounts, containers, cloud deployments or real PVM runtime were modified or started.
Runnable configuration-only reproducer for the reviewed head

Requires Python 3.12, PyYAML and Helm on PATH. Save as /tmp/pr154-pvm-policy-probe.py, then run from the reviewed checkout:

python3 /tmp/pr154-pvm-policy-probe.py "$PWD" /tmp/pr154-pvm-results.json

It asserts complete input TOML equivalence, uses the repository's real rendering/generator fixtures, prints the six effective values and exits 1 when any false switch leaves enforcement enabled.

"""Reproduce PVM opt-out behavior for semantically equivalent TOML inputs."""

import importlib.util
import json
from pathlib import Path
import re
import sys
import tempfile
import tomllib

import yaml

checkout, output = map(Path, sys.argv[1:3])


def load(name, relative):
    spec = importlib.util.spec_from_file_location(name, checkout / relative)
    module = importlib.util.module_from_spec(spec)
    spec.loader.exec_module(module)
    return module


helm = load("review_helm", "deploy/akernel/charts/core/tests/test_shared_checkpoint_storage.py")
standalone = load("review_standalone", "deploy/standalone/test_start_config.py")
pattern = r"(?ms)^\[plugin\.resource\.memory_limit\]\n.*?(?=^\[|\Z)"


def variants(source):
    section = re.search(pattern, source).group()
    enabled = re.sub(r"(?m)^enabled\s*=\s*false", "enabled = true", section)
    source = source.replace(section, enabled)
    quoted = source.replace("[plugin.resource.memory_limit]", '[plugin."resource".memory_limit]')
    inline = re.sub(pattern, 'memory_limit = { enabled = true, total_reserve = "16GiB" }\n\n', source)
    assert tomllib.loads(source) == tomllib.loads(quoted) == tomllib.loads(inline)
    return {"canonical": source, "quoted_table": quoted, "inline_table": inline}


results = []
values = yaml.safe_load((checkout / "deploy/akernel/charts/core/values.yaml").read_text())
source = values["node"]["config"]["sandboxd"]["config"]
for name, candidate in variants(source).items():
    with tempfile.TemporaryDirectory() as directory:
        config = Path(directory) / "sandboxd.toml"
        config.write_text(candidate)
        rendered = helm.render("adx.tls.existingSecret=test-tls", "node.config.sandboxd.firecrackerBackend=pvm", "node.config.sandboxd.pvmMemoryLimitEnabled=false", set_files={"node.config.sandboxd.config": config})
        actual = tomllib.loads(rendered[("ConfigMap", "akernel-node-config")]["data"]["sandboxd_config.toml"])
        results.append({"path": "helm", "input": name, "explicit_pvm_opt_in": False, "effective_memory_enabled": actual["plugin"]["resource"]["memory_limit"]["enabled"]})

source = (checkout / "deploy/standalone/config/sandboxd_config.toml").read_text()
for name, candidate in variants(source).items():
    test = standalone.StartConfigTest("test_generated_config_preserves_explicit_memory_policy_overrides")
    test.setUp()
    try:
        test.template.write_text(candidate)
        run = test.generate(AKERNEL_FIRECRACKER_BACKEND="pvm", AKERNEL_PVM_MEMORY_LIMIT_ENABLED="false")
        assert run.returncode == 0, run.stderr
        actual = tomllib.loads(test.config_output.read_text())
        results.append({"path": "standalone", "input": name, "explicit_pvm_opt_in": False, "effective_memory_enabled": actual["plugin"]["resource"]["memory_limit"]["enabled"]})
    finally:
        test.doCleanups()
output.write_text(json.dumps(results, indent=2) + "\n")
print(json.dumps(results, indent=2))
raise SystemExit(any(row["effective_memory_enabled"] for row in results))

Please keep approval/merge on hold until the P2 and the resulting-head/latest-develop CI are resolved.

wanghongtao.wangho added 2 commits November 9, 2026 10:23
Bring the native build and runtime rootfs contracts into the review base.
Retain the component revisions selected by develop.

Signed-off-by: wanghongtao.wangho <wanghongtao.wangho@antgroup.com>
Resolve Helm policies by their parsed keys so equivalent table forms
cannot bypass an explicit opt-out. Verify standalone rewrites against
the full parsed input and reject unsupported overrides before writing.

Exercise both switch values through the deployment entry points and
retain compatibility with omitted disabled policies and KVM.

Signed-off-by: wanghongtao.wangho <wanghongtao.wangho@antgroup.com>

iamWHTWD commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator Author

@rcyw I addressed P2 in 47d8563 and integrated develop f81d033.

  • Helm now parses TOML before applying the PVM memory switch. Canonical tables, quoted table names, inline tables, and dotted keys all resolve to the same policy.
  • Standalone checks that the rewritten TOML applies the requested boolean and preserves the other fields. A rewrite uses the standard [plugin.resource.memory_limit] table. An unsupported override fails before replacing the existing configuration.
  • The regressions cover both switch values, omitted disabled policies, invalid and duplicate fields, and the KVM path.

ECS validation passed all 93 standalone tests on Python 3.10 with tomli, 24 configuration tests on Helm 3.17, and 77 Linux build-helper checks. The final focused standalone regression suite also passed all 18 tests on Python 3.10. ECS confirmed that Helm 3.16 can render the KVM profile and gives a clear version error for PVM. The PVM path uses Helm 3.17 or newer and Python 3.11 or newer, with tomli supported on Python 3.10.

Full CI passed all 11 jobs on this head, including the production image build and Standalone E2E, after retries on the same commit. CI run

Could you re-review P2 on this head?

@rcyw rcyw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at 47d8563687ee738e39875b3aeb922049eae330f2. The previous P2 is resolved in both entry points. No remaining blocking finding within the reduced rollout scope.

The design remains reasonable: ordinary direct Helm enables the existing aggregate cap with a 16 GiB reserve and retains its sandboxd capacity source. Standalone retains its original auto source, and standalone/cloud/PVM standard defaults keep enforcement disabled. This scope requires no new ADX release.

P2 verification

Helm now parses the effective TOML and applies the requested boolean. Standalone parses before rewriting, verifies the complete parsed result and preserves unrelated fields; unsupported rewrites are explicitly rejected before replacing the existing configuration. This satisfies the previously requested safe-rejection alternative.

Independent rerun of the original six cases, with the PVM switch explicitly false:

Entry point Canonical table Quoted table Inline table
Helm effective enabled=false effective enabled=false effective enabled=false
Standalone effective enabled=false explicit rejection; previous file unchanged explicit rejection; previous file unchanged

The product regressions also cover true/false switches, escaped/dotted forms, omitted disabled policies, invalid/duplicate fields and KVM behavior. The new version prerequisites are documented.

Independent validation

  • 198 local checks passed with zero skips: 93 standalone, 24 Helm memory/storage, 4 PVM static-profile and 77 build-helper tests; deployment syntax passed.
  • The full 24-test Helm memory/storage suite also passed with official Helm 3.17.0.
  • Eight native version/profile checks passed across core/umbrella × KVM/PVM: Helm 3.16.0 renders ordinary KVM and clearly rejects PVM with the required minimum version; Helm 3.17.0 renders both. Official Helm archives were checksum-verified.
  • Local checks used Python 3.12. Separately, CI deployment logs confirm Python 3.10.22 + tomli 2.5.0 and all 93 standalone tests passing.

Final-head/latest-develop CI

CI 42554549954 is completed/success: 11 successful jobs, including Python 3.10–3.14, SDK quality/distributions, managed helper checks, deployment contracts, the production image build and Standalone/runsc E2E; image publication was skipped as expected.

I verified the checked-out merge 84bca6e4674cc8f01e113bfbbeba31e0ef858874 has parents latest develop f81d033765335260fa74d4c583a27ccd29bce452 and this head. Its full tree and the head tree are identical: f6fc2861ccb88631861b58d93ab6c7f1e9f73c31. This closes the prior requirement for CI on the latest-develop combination. SDK, component gitlinks and platform/Execd Dockerfile inputs have no net changes against that base.

Non-blocking follow-ups:

  • Update the PR description's old final-source/CI references from bc3aabd/42197801592 to this head/run, while preserving the actual source attribution of older ECS runtime evidence.
  • Retain the CI retry history: attempt 1 observed 147 rather than 0 in the SDK service-process thaw test; attempt 2 hit an Ubuntu archive connection failure while downloading tini. Final same-commit retries passed. The relevant SDK code is unchanged from develop; the SDK failure's root cause was not diagnosed in this review and deserves separate tracking.

Local checks establish configuration behavior, not real finite-cap PVM/OOM qualification. PVM enforcement remains disabled by default; qualification on the matching maintained host remains required before explicit enablement. The documented reserve-sizing, drain/canary and existing-cap preservation contracts continue to apply.

@rcyw
rcyw merged commit f99615a into inclusionAI:develop Nov 9, 2026
34 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants