Repository navigation
fix: reduce legacy Linux custom data encoding overhead - #9880
Shun Lyu (sinmentis) wants to merge 2 commits into
Conversation
Avoid encoding each script separately when the complete traditional payload is already gzipped. Preserve file contents and alternate delivery modes. AB#40305782 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 162f68f8-110a-4d60-b4fe-b9fb42253542
Windows Unit Test Results 3 files 18 suites 1m 10s ⏱️ Results for commit d4da1bb. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
🔵 Needs a closer look
Production bootstrap serialization changes need human confirmation of cloud-init compatibility across supported Linux images beyond the offline regression coverage.
No open issues 🎉
What changed in this PR
Reduces legacy Linux CustomData size by removing per-file gzip/Base64 encoding while retaining whole-payload compression. This addresses the public-cloud payload overflow; the remaining Ubuntu custom-cloud overflow stays separate in #9790.
Changes:
- Renders legacy file contents as JSON-quoted YAML strings, preserving scriptless, Flatcar, and ACL encoding.
- Keeps existing file delivery, including LocalDNS, without requiring newer VHD contents.
- Adds encoding-equivalence, whitespace, delivery-mode, and payload-size regression coverage, plus a rendering benchmark.
| File | Description |
|---|---|
pkg/agent/variables.go |
Selects plain or compressed script content by delivery mode. |
pkg/agent/utils.go |
Separates script rendering from compression. |
pkg/agent/utils_test.go |
Updates script discovery for the encoder wrapper. |
pkg/agent/mariner_live_patching_delivery_test.go |
Checks size using the production Azure Linux payload. |
pkg/agent/live_patching_delivery_test.go |
Checks size using the production Ubuntu payload. |
pkg/agent/cloud_init_encoding_test.go |
Adds equivalence, mode, whitespace, size, and benchmark coverage. |
pkg/agent/baker.go |
Adds mode-aware encoding helpers and JSON serialization. |
pkg/agent/baker_test.go |
Checks delivered file paths rather than script references. |
parts/linux/cloud-init/nodecustomdata.yml |
Uses encoding helpers for embedded file entries. |
hotfix/render-nodecustomdata/main_test.go |
Updates hotfix fixtures and verifies plain encoding. |
hotfix/hotfix_generate_test.py |
Updates expectations for template encoding helpers. |
Review effort: Balanced
Keep template registration and the test matrix below the cognitive complexity limit without changing behavior. AB#40305782 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 162f68f8-110a-4d60-b4fe-b9fb42253542
|
|
|
Review: fix: reduce legacy Linux custom data encoding overheadWhat is goodThe change reduces encoding overhead without dropping delivered modules or depending on a newer VHD. LocalDNS remains an unconditional file entry, preserving old-image provisioning behavior (template, lines 140-144). One mode predicate selects both script representation and YAML formatting. Scriptless, Flatcar and ACL retain their existing encoding; JSON serialization handles quoting at both the YAML-scalar and outer JSON boundaries (baker.go, lines 348-364, lines 702-735). The regression tests inspect the final production payload and compare decoded file contents and metadata across distro, custom-cloud, LocalDNS and pre-provisioning combinations, rather than checking encoding labels alone (tests, lines 16-120). An additional review-time comparison of actual base/head output confirmed identical decoded cloud-config for both Ubuntu and Azure Linux custom-cloud fixtures, including all 18 files. Review outcomePR: #9880 Reviewed head: Intent: Remove inner gzip/base64 only for traditional Linux CustomData, retaining outer compression, decoded file bytes, paths, ownership, permissions and existing delivery behavior. Preserve alternate modes and six-month VHD compatibility. No increased payload limit, dependency or shell-body change. Risk and depth: Medium-high review risk because this serialization path feeds Linux node bootstrap. Planned units were encoder selection, YAML/JSON/compression boundaries, alternate modes/hotfix consumers, and behavioral validation. All seven distinct lenses completed against the current revision, including refreshes where needed; agents ran in waves of at most three. The eighth agent completed sequential previous-comment verification. All planned units received source review; deployed cloud-init/image-version validation remains outside the completed evidence. Conclusion: No supported introduced defect was found. Current-head focused tests, Go CI, CodeQL and all three lint jobs pass. This is not an unconditional merge-readiness assessment: infrastructure checks remain pending, and an existing request for supported-image cloud-init confirmation remains unverified. Ubuntu custom-cloud overflow remains outside this PR under #9790; it is not resolved by this change. The PR moved from Comments, ordered by importanceNo retained blocking defects, additional blocking validation gates, non-blocking recommendations or nitpicks. The readability lens suggested documenting the YAML-value helper's input/output contract. Current callers already obey the documented paired-helper convention; no present defect was demonstrated. This optional documentation preference was not promoted to a finding. H1. Existing question: supported-image cloud-init confirmationCanonical discussion: existing review summary. Classification: Question, report-only; do not create a duplicate inline comment. The earlier reviewer requested human confirmation using actual cloud-init on supported Linux images. Current source, Go tests and review-time PyYAML comparisons support the serialization contract, but none executes the guest The narrow next action is to supply representative supported-image cloud-init/write-files evidence tied to this revision, including image and cloud-init versions and resulting file bytes/metadata. Existing provisioning-gate evidence may suffice if it actually exercises that path. This review does not impose a new exhaustive image matrix or assert an unproven compatibility defect. Source: encoding helpers, lines 702-720. Confidence: high that the requested evidence is missing, not that a defect exists. Security findings
This is a bounded manual security review, not a completed automated security audit. Gitleaks, Semgrep, OSV-Scanner, Trivy and Checkov were unavailable; no tools were installed. Automation evidenceCurrent-head Go CI, CodeQL Analyze(go), shell tests/checks, Pester, and root/nested lint checks pass. The exact head was verified for Go CI, CodeQL and lint runs. Passing checks support their respective contracts, not operational safety or production performance. Historical lint run The Python hotfix suite has one failing custom-cloud subcase. The same failure was independently reproduced at the immutable base revision, so it is not attributed to this PR. Previous review commentsInventory cutoff: 2026-10-23 04:01:27 UTC. All pages retrieved for general comments, review bodies, inline comments and review threads. Five unique source comments, five concern-level todos; all five sequential adjudications are done, zero blocked or pending. Four outcomes are not actionable, one cannot verify; addressed, partial, unaddressed and duplicate counts are each zero. No inline threads, replies or provider-resolution discrepancies were present. A completed todo means adjudicated, not fixed; the history must not be described as "all comments resolved." Detailed concern ledger: session ledger. Every concern ID below has prefix
All five comments are bots. The summary's generated "No open issues" wording is not a provider resolution flag and does not close H1. New or edited comments after the cutoff are not covered by this frozen inventory. Validation and coverage
The in-tree legacy comparator uses the head template with legacy helpers, not immutable base output. The independent base/head comparison above covers that specific shared-template blind spot for its two fixtures. No provisioning-speed, peak-memory or complete historical-runtime guarantee is asserted. Historical cloud-init/deployed ANC certification would need image-domain evidence; no other domain-expert need emerged. Final remote-head recheck matched the reviewed revision. Final check inventory: 21 successes, one skipped, three queued, twelve in progress, two neutral; no failed conclusion. Evidence: The parent completed the report's comment-quality and completeness checks: obsolete automation findings removed; the optional documentation suggestion not promoted; existing concern linked rather than duplicated; source and runtime evidence distinguished; all eight assignments accounted for. Self-evaluation was performed inline without an additional reviewer. Evidence directory: Publication planGeneral PR comment: this report, only after explicit reviewer approval for version v1 and the reviewed revision. |
|
attempting to test with NAP check-in tests before merge |
|
|
||
| func getCloudInitFileContent(config *datamodel.NodeBootstrappingConfiguration, content string) (string, error) { | ||
| if usePlainCloudInit(config) { | ||
| encoded, err := json.Marshal(content) |
There was a problem hiding this comment.
I am worried about this line, marshalling plain text in json, i am worried that some introduction of text might break this in future.
What this PR does / why we need it:
Remove per-file gzip/Base64 encoding from traditional Linux CustomData, which is already gzipped as a whole. Render file contents as JSON-quoted YAML strings so script bytes, whitespace, permissions, ownership, and paths stay unchanged.
Keep scriptless, Flatcar, and ACL encoding unchanged. Deliver every existing module, including LocalDNS, without relying on newer VHD contents. No size-limit increase, dependency, or shell behavior change.
This fixes the independently reproduced
mainGo-test failure also seen in #9717. Measured final production payloads for the existing public-cloud fixtures:Ubuntu custom-cloud still exceeds the limit at 89,176 characters. That broader overflow remains separate in #9790. Unrelated E2E failures from #9717 are not addressed.
Validation:
custom cloud onlyexpectation intest_production_renderer_omits_empty_platform_payloadsalso fails on unchangedmainat732fedcbe0.Which issue(s) this PR fixes:
AB#40305782