Repository navigation
fix: reduce legacy Linux custom data encoding overhead - #9880
Conversation
Windows Unit Test Results 3 files 18 suites 1m 16s ⏱️ Results for commit 61f82bf. ♻️ This comment has been updated with latest results. |
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 |
Avoid per-file gzip/Base64 when the complete payload is already compressed. Use YAML-native quoting and omit the plain-text encoding key. Preserve file bytes and alternate delivery modes. AB#40305782 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 162f68f8-110a-4d60-b4fe-b9fb42253542
Check delivered file contents and lengths before initialization. Keep expected checksums in the CSE command so CustomData remains unchanged. AB#40305782 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 162f68f8-110a-4d60-b4fe-b9fb42253542
Revert 89864be. The checksum gate can reject valid files when the CSE command and delivered scripts differ across provisioning attempts. Keep the CustomData compression improvement. AB#40305782 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 162f68f8-110a-4d60-b4fe-b9fb42253542
312200c to
f275048
Compare
Keep rendered file contents unencoded until template output and return Linux YAML directly without a JSON round trip. AB#40305782 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 162f68f8-110a-4d60-b4fe-b9fb42253542
|
|
|
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. Preserve the minifier from #9940 in the shared script-rendering path, so both delivery modes use the same minified bytes. Render plain file contents as YAML-native double-quoted strings, omit the optional encoding key, and reject invalid UTF-8 instead of silently replacing bytes.
Keep scriptless, Flatcar, and ACL encoding unchanged. Preserve main's file selection, contents, permissions, ownership, paths, and boot commands. No size-limit increase, additional dependency, or CSE command change.
The outer gzip checksum remains. There is no additional runtime per-file checksum gate.
This adds headroom on top of the minifier. Final encoded production payload lengths, compared with main at
48e4c052f2using identical fixtures:All four fixtures fit the 87,380-character limit. These are representative configurations, not worst-case bounds.
Validation:
U+0085. Eight edge cases pass the installed cloud-initwrite_filesmodule on Ubuntu 24.04 (26.1) and Azure Linux 3 (24.3.1), with matching file hashes and metadata. Paths were isolated and cleaned up; these are not full-bootstrap tests.cloud-init 26.1): a new node was Ready, all 10 test pods were ready, cloud-init exited 0 with no errors, and all 17 delivered files matched their contents, permissions, and ownership. This covered one scenario, not the full NAP check-in suite. The test clusters and isolated environment were cleaned up. The combined implementation has not had a new standalone run.custom cloud onlyexpectation intest_production_renderer_omits_empty_platform_payloadswas reproduced on unchanged main at48e4c052f2.Which issue(s) this PR fixes:
AB#40305782