feat: dynamically resolve time source for CVMs during CSE (PHC vs NTP) - #9591
Open
Zachary (zachary-bailey) wants to merge 21 commits into
Open
Zachary (zachary-bailey) wants to merge 21 commits into
Zachary (zachary-bailey) wants to merge 21 commits into
Conversation
Zachary (zachary-bailey)
requested review from
a team,
Devinwong,
Nishchay (awesomenix),
Cameron Meissner (cameronmeissner),
Sylvain Boily (djsly),
lilypan26,
Artur Khantimirov (r2k1) and
Tim Wright (timmy-wright)
as code owners
September 18, 2026 18:14
Copilot started reviewing on behalf of
Zachary (zachary-bailey)
September 18, 2026 18:15
View session
Contributor
Windows Unit Test Results 3 files 17 suites 57s ⏱️ Results for commit 80b1b3e. ♻️ This comment has been updated with latest results. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
An unhandled Chrony setup status can allow TDX provisioning to continue after configuration fails.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates Linux node provisioning so Ubuntu 26.04 confidential VMs select Chrony sources appropriate to AMD SEV-SNP or Intel TDX.
Changes:
- Detects the confidential VM platform during initialization.
- Preserves PHC for SEV-SNP and configures Ubuntu NTP pools for TDX.
- Adds synchronization checks, diagnostics, exit handling, and tests.
File summaries
| File | Description |
|---|---|
| spec/parts/linux/cloud-init/artifacts/init_aks_cloud_spec.sh | Tests platform detection and Chrony behavior. |
| pkg/agent/baker_test.go | Tests generated CSE command handling. |
| parts/linux/cloud-init/artifacts/init-aks-cloud.sh | Implements platform-specific time synchronization. |
| parts/linux/cloud-init/artifacts/cse_helpers.sh | Defines the platform-detection exit code. |
| parts/linux/cloud-init/artifacts/cse_cmd.sh | Propagates selected initialization failures. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot started reviewing on behalf of
Zachary (zachary-bailey)
September 18, 2026 18:49
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
NTP failures use the wrong exit-code contract, and Chrony setup failures can allow provisioning to continue.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
parts/linux/cloud-init/artifacts/cse_cmd.sh:27
- 🔴 High Risk — Script Logic: this wrapper only exits for 244/245.
configure_ubuntu_2604_cvm_time_synccan return status 1 whenconfigure_chronyfails (the TDX branch explicitly maps that failure toreturn 1, and the SEV-SNP branch returns the helper status), so this branch falls through and continues provisioning a CVM node without the required time source. Propagate any nonzero result or map configuration failures to a code handled here.
if [ "$initAKSCloudExitCode" -eq 245 ]; then
echo "NTP not reachable; init-aks-cloud failed with NTP synchronization error code ${initAKSCloudExitCode}" >> ${PROVISION_OUTPUT};
exit ${initAKSCloudExitCode};
elif [ "$initAKSCloudExitCode" -eq 244 ]; then
echo "Unable to determine confidential VM platform; init-aks-cloud failed with exit code ${initAKSCloudExitCode}" >> ${PROVISION_OUTPUT};
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
Copilot started reviewing on behalf of
Zachary (zachary-bailey)
September 18, 2026 19:47
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Exit-code classification and absent systemd-timesyncd handling can break provisioning.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
parts/linux/cloud-init/artifacts/init-aks-cloud.sh:696
- This returns 245, but the requested classification for an NTP connectivity failure is the existing outbound-connectivity code 50 (
cse_helpers.sh:37, used bycse_main.sh:460). Becausecse_cmd.shexits immediately on this status beforecse_mainruns, TDX NTP failures are reported as 245 and never use the established outbound-connectivity code. Return 50 for this path and update the CSE command's special-case handling and tests accordingly.
return "$ERR_NTP_UNREACHABLE"
parts/linux/cloud-init/artifacts/init-aks-cloud.sh:696
- [High Risk] The PR contract says an NTP synchronization failure must use the existing outbound-connectivity exit code 50 (
ERR_OUTBOUND_CONN_FAIL), but this path returns the new 245 (ERR_NTP_UNREACHABLE), andcse_cmd.shpropagates 245. As a result, consumers that classify exit 50 (including the e2e retry handling) will not recognize this failure as the outbound-connectivity case. Return 50 here and update the related command/tests, or revise the stated contract.
return "$ERR_NTP_UNREACHABLE"
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
Comment on lines
+26
to
+28
| elif [ "$initAKSCloudExitCode" -eq 245 ]; then | ||
| echo "NTP not reachable; init-aks-cloud failed with NTP synchronization error code ${initAKSCloudExitCode}" >> ${PROVISION_OUTPUT}; | ||
| exit ${initAKSCloudExitCode}; |
Contributor
There was a problem hiding this comment.
the issue is that any failure here is not exposed to customer or very hard to debug :( is there a way to log this at end of the script.
Also customers in AGC frown when things break in this script because of lack of observability.
Can you move the chrony setup out of this if possible.
Copilot started reviewing on behalf of
Zachary (zachary-bailey)
September 18, 2026 20:33
View session
Zachary (zachary-bailey)
changed the title
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
NTP failure handling still conflicts with the documented outbound-connectivity exit-code contract.
Review details
Suppressed comments (8)
Previously missed (1) — in code that hasn't changed since the last review.
parts/linux/cloud-init/artifacts/cse_cmd.sh:31
- The new tests only assert that fragments of this exit-code dispatch appear in the rendered command. They do not execute the flattened command, so a quoting or control-flow error could still allow the main CSE to start after
init-aks-cloud.shreturns 244, 245, or 246. Add an execution test inpkg/agent/baker_test.go, using temporary mock init/CSE scripts, that verifies each new failure code exits before the main CSE and that the zero exit path continues.
parts/linux/cloud-init/artifacts/init-aks-cloud.sh:85
- 🔴 High Risk — Script Logic: The PR contract says an NTP synchronization failure must use the outbound-connectivity exit code
50, but this defines and propagates245. The E2E harness only recognizesERR_OUTBOUND_CONN_FAIL/50as retryable (e2e/scenario/vmss.go:248-256), so this failure will be classified differently and will not receive the intended retry handling. Use50consistently across both constant definitions,cse_cmd.sh, and the tests, or update the stated contract if245is intentional.
ERR_NTP_UNREACHABLE=245 # Chrony could not synchronize with the configured NTP pools
parts/linux/cloud-init/artifacts/init-aks-cloud.sh:589
- [🔴 High Risk][bug] Ubuntu VHD creation already treats a missing/removed
systemd-timesyncdunit as normal (vhdbuilder/scripts/linux/ubuntu/tool_installs_ubuntu.sh:117-124). On those 26.04 FDE images, these unconditional stop/disable calls can return nonzero, so the new CVM path returns 246 before it writes the PHC/NTP configuration. Use the existing SubState/dead guard (or tolerate unit-not-found) while still failing on genuine systemctl errors.
if ! systemctl stop systemd-timesyncd; then
parts/linux/cloud-init/artifacts/init-aks-cloud.sh:696
- [🔴 High Risk][discrepancy_with_pr_description] The PR description requires NTP synchronization failures to use the existing outbound-connectivity exit code 50 (
ERR_OUTBOUND_CONN_FAIL), but this path returns 245 andcse_cmd.shpropagates 245 instead. That changes the stated provisioning contract; align the implementation and generated command with code 50, or update the requirement and all consumers/tests.
return "$ERR_NTP_UNREACHABLE"
parts/linux/cloud-init/artifacts/init-aks-cloud.sh:696
- 🔴 High Risk — Script Logic: The PR contract says an NTP failure must use the existing outbound-connectivity exit code 50 (
ERR_OUTBOUND_CONN_FAILincse_helpers.sh:37), but this returns 245 andcse_cmd.shpropagates 245. Consumers that classify provisioning failures by code 50 will miss an NTP outage. Use code 50 for this path and update the wrapper/tests to propagate that established code.
return "$ERR_NTP_UNREACHABLE"
parts/linux/cloud-init/artifacts/init-aks-cloud.sh:590
- 🟡 Medium Risk — Script Logic: The refactor now treats stopping/disabling
systemd-timesyncdas a hard failure. The Ubuntu VHD build path explicitly supports this unit being absent (vhdbuilder/scripts/linux/ubuntu/tool_installs_ubuntu.sh:116-124), so a CVM/minimal image can enter this branch with no unit;configure_chronythen returns before writing the NTP/PHC config, and TDX provisioning fails with 246. Preserve the prior behavior by checking the unit state first and only stopping/disabling it when present.
if ! systemctl stop systemd-timesyncd; then
echo "ERROR: failed to stop systemd-timesyncd" >&2
parts/linux/cloud-init/artifacts/init-aks-cloud.sh:85
- 🔴 High Risk — Script Logic. This assigns the NTP failure to 245, but the repository's established outbound-connectivity code is
ERR_OUTBOUND_CONN_FAIL=50(cse_helpers.sh:37), and the e2e retry path only recognizes exit 50 (e2e/scenario/vmss.go:193-197). As a result, a transient NTP reachability failure will not follow the outbound-connectivity retry/classification path and also contradicts the PR's stated exit code. Use the established 50 code and update the CSE dispatch/tests consistently.
ERR_NTP_UNREACHABLE=245 # Chrony could not synchronize with the configured NTP pools
parts/linux/cloud-init/artifacts/init-aks-cloud.sh:595
- 🔴 High Risk — Script Logic. Ubuntu VHD setup explicitly supports images where
systemd-timesyncdis absent by checking its state and skipping stop/disable (vhdbuilder/scripts/linux/ubuntu/tool_installs_ubuntu.sh:116-124), but this refactored path treats either command failure as fatal. On such an image, Ubuntu 26.04 CVM provisioning exits 246 before writing the selected Chrony configuration. Guard these calls with the same missing-unit check used by the VHD builder, while still failing on real service-operation errors.
return 1
fi
if ! systemctl disable systemd-timesyncd; then
echo "ERROR: failed to disable systemd-timesyncd" >&2
return 1
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Copilot started reviewing on behalf of
Zachary (zachary-bailey)
September 23, 2026 19:54
View session
Copilot started reviewing on behalf of
Zachary (zachary-bailey)
September 23, 2026 20:44
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The embedded-script hotfix E2E omits the newly required Chrony module and will fail against older VHDs.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 3
Open (3)
Resolved since last review (1)
| source "${CSE_CONFIG_KUBELET_FILEPATH:-${BASH_SOURCE[0]%.sh}_kubelet.sh}" | ||
| source "${CSE_CONFIG_NETWORK_FILEPATH:-${BASH_SOURCE[0]%.sh}_network.sh}" | ||
| source "${CSE_CONFIG_ADDONS_FILEPATH:-${BASH_SOURCE[0]%.sh}_addons.sh}" | ||
| source "${CSE_CONFIG_CHRONY_FILEPATH:-${BASH_SOURCE[0]%.sh}_chrony.sh}" |
Copilot started reviewing on behalf of
Zachary (zachary-bailey)
September 23, 2026 21:21
View session
Copilot started reviewing on behalf of
Zachary (zachary-bailey)
September 23, 2026 22:15
View session
Contributor
|
Contributor
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The multiline TDX NTP configuration is word-split by logs_to_events, producing an invalid Chrony configuration.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 4
| tdx) | ||
| echo "Intel TDX detected; configuring Chrony to use the Ubuntu NTP pools" | ||
| ntp_pools="$(ubuntu_ntp_pools)" | ||
| if ! logs_to_events "AKS.CSE.configureChronyTDX" apply_chrony_configuration "$ntp_pools"; then |
Contributor
|
Copilot started reviewing on behalf of
Zachary (zachary-bailey)
September 23, 2026 22:21
View session
| local chrony_conf="${CHRONY_CONF:-/etc/chrony.conf}" | ||
|
|
||
| cat > "$chrony_conf" <<'EOF' | ||
| # This directive specify the location of the file containing ID/key pairs for |
Contributor
There was a problem hiding this comment.
nit: specifies*
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Multiline NTP configuration is split by the production logging wrapper, causing TDX Chrony setup to fail.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 5
Open (5)
Comment on lines
+217
to
+218
| ntp_pools="$(ubuntu_ntp_pools)" | ||
| if ! logs_to_events "AKS.CSE.configureChronyTDX" apply_chrony_configuration "$ntp_pools"; then |
| # For VHD image creation workflows, only basePrep runs initially, and nodePrep runs later | ||
| # when nodes are created from that VHD image. | ||
| chronyExitCode=0 | ||
| logs_to_events "AKS.CSE.configureChrony" configure_node_time_sync || chronyExitCode=$? |
Contributor
There was a problem hiding this comment.
can we align the name of the func and the name of the event if possible?
| Expect(cseCmd).To(ContainSubstring("cloud-init status --wait")) | ||
| Expect(cseCmd).To(ContainSubstring("cloudInitExitCode=$?")) | ||
| Expect(cseCmd).To(ContainSubstring("REPO_DEPOT_ENDPOINT=")) | ||
| Expect(cseCmd).NotTo(ContainSubstring("initAKSCloudExitCode")) |
Contributor
There was a problem hiding this comment.
why this assertion?
| fi | ||
|
|
||
| if [ ! -e "$chrony_conf" ]; then | ||
| if ! apt-get update; then |
Contributor
There was a problem hiding this comment.
should we use our established apt helper with retries here instead?
| makestep 1.0 -1 | ||
| EOF | ||
|
|
||
| systemctl restart chronyd |
Contributor
There was a problem hiding this comment.
we're not capturing failures here - is that intended? I see we do in the ubuntu/flatcar paths
| echo "Skipping chrony configuration for ACL (PTP clock baked into chronyd, no external NTP sources)" | ||
| elif isMarinerOrAzureLinux "$OS"; then | ||
| logs_to_events "AKS.CSE.configureChronyMarinerAzureLinux" configure_mariner_azurelinux_chrony || true | ||
| elif should_configure_ubuntu_2604_cvm_time_sync; then |
Contributor
There was a problem hiding this comment.
should we generalize this to the ubuntu path? not specifically 2604, the new stuff we're doing is actually gated on the confidential_vm_platform right?
| return "$ERR_NTP_UNREACHABLE" | ||
| } | ||
|
|
||
| configure_ubuntu_2604_cvm_time_sync() { |
Contributor
There was a problem hiding this comment.
consequently, this should just be configure_ubuntu_cvm_time_sync right?
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Ubuntu 26.04 CVM images are built on AMD SEV-SNP and contain the existing Hyper-V PHC Chrony configuration. The same image can also run on Intel TDX nodes, where the inherited PHC source is not usable and the node must synchronize through network NTP.
This change detects the confidential VM platform on the actual node and selects the appropriate time source:
Chrony configuration is also moved out of
init-aks-cloud.shinto a dedicatedcse_config_chrony.shmodule. This places Chrony failures inside the normal CSE execution and reporting path, improving customer-visible diagnostics and hotfixability.Chrony CSE module
Add
cse_config_chrony.shas an independently delivered CSE configuration module.Source the module through
cse_config.sh.Remove all Chrony configuration and CVM platform detection from
init-aks-cloud.sh.Preserve the existing execution order from
main:basePrepnodePrepUbuntu 26.04 CVM behavior
Scope platform-specific behavior to Ubuntu 26.04 FDE images.
Detect the confidential VM platform with:
bash systemd-detect-virt --cvm• Preserve the existing SEV-SNP PHC configuration
• Configure Intel TDX with only the approved Ubuntu NTP pools:
• Validate TDX synchronization
Which issue(s) this PR fixes:
Intel TDX nodes cannot use the inherited Hyper-V PHC time source from the Ubuntu 26.04 SEV-SNP-built image and must instead synchronize through the approved Ubuntu NTP pools.