fix: start ask node controller within boothook instead of service for non flatcar/acl#8993
fix: start ask node controller within boothook instead of service for non flatcar/acl#8993awesomenix wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Linux bootstrapping flow to reduce node boot time by launching aks-node-controller earlier from the cloud-boothook (instead of waiting on systemd’s normal target transactions), and adjusts VHD/e2e validation to match the new launch semantics.
Changes:
- Launch
aks-node-controller-wrapper.shdirectly from the boothook (with output redirected to/var/log/azure/aks-node-controller.output) and ensure required log directories exist. - Keep
aks-node-controller.servicedisabled in the VHD image and adjust VHD content tests accordingly; update the unit toDefaultDependencies=noto reduce start delays when it is used. - Update e2e tooling/tests to inject files based on the wrapper invocation, validate wrapper output appropriately, and collect additional cloud-init logs.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| vhdbuilder/packer/test/linux-vhd-content-test.sh | Updates VHD validation to expect aks-node-controller.service to be disabled. |
| vhdbuilder/packer/pre-install-dependencies.sh | Disables ANC systemd unit in the baked image; adds DHCP/cache-warmup related boot optimizations. |
| vhdbuilder/packer/imagecustomizer/azlosguard/azlosguard.yml | Moves ANC into the “disable” service list for OSGuard customization. |
| pkg/agent/baker.go | Changes boothook to create /var/log/azure and launch the ANC wrapper via nohup with file redirection; updates Flatcar/ACL ignition template to enable the unit. |
| pkg/agent/baker_test.go | Extends ignition JSON assertions for systemd unit enablement and link creation. |
| parts/linux/cloud-init/artifacts/aks-node-controller.service | Adds DefaultDependencies=no to reduce implicit systemd ordering delays. |
| e2e/vmss.go | Adds cloud-init logs to extraction; updates boothook injection insertion point to match wrapper invocation. |
| e2e/validators.go | Adds a helper to validate ANC wrapper output across different launch mechanisms. |
| e2e/scenario_test.go | Uses the new validator helper for ANC hotfix binary selection scenario. |
|
Wanted to bring this up about old VHD launching ANC twice (but it probably won't happen)
It probably won't happen because:
Just wanted to make sure this is in your radar |
8899814 to
ba12a1e
Compare
ba12a1e to
28aee12
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 14 comments.
Comments suppressed due to low confidence (7)
parts/linux/cloud-init/artifacts/aks-node-controller-wrapper.sh:26
- LOGGER_TAG was updated to "aks-node-controller-launcher" but the script file is still named aks-node-controller-wrapper.sh, while other changes reference aks-node-controller-launcher.sh. This mismatch makes it easy to ship a VHD where the referenced launcher path does not exist.
spec/parts/linux/cloud-init/artifacts/aks_node_controller_wrapper_spec.sh:5 - This ShellSpec now targets ./parts/linux/cloud-init/artifacts/aks-node-controller-launcher.sh, but that file does not exist in the repo (the script is still named aks-node-controller-wrapper.sh). The test suite will fail until the script rename/addition is done.
vhdbuilder/packer/vhd-image-builder-cvm.json:119 - This packer file provisioner uploads parts/linux/cloud-init/artifacts/aks-node-controller-launcher.sh, but that artifact does not exist in the repo (only aks-node-controller-wrapper.sh exists). This will cause the VHD build to fail.
{
"type": "file",
"source": "parts/linux/cloud-init/artifacts/aks-node-controller-launcher.sh",
"destination": "/home/packer/aks-node-controller-launcher.sh"
},
{
"type": "file",
"source": "parts/linux/cloud-init/artifacts/cloud-init-status-check.sh",
vhdbuilder/packer/packer_source.sh:280
- copyPackerFiles() copies $AKS_NODE_CONTROLLER_LAUNCHER_SRC to /opt/azure/containers/aks-node-controller-launcher.sh, but that source will not exist during packer builds unless the artifacts upload step provides it (the repo currently has only aks-node-controller-wrapper.sh).
AKS_NODE_CONTROLLER_SRC=/home/packer/aks-node-controller
AKS_NODE_CONTROLLER_DEST=/opt/azure/containers/aks-node-controller
cpAndMode $AKS_NODE_CONTROLLER_SRC $AKS_NODE_CONTROLLER_DEST 755
cpAndMode $AKS_NODE_CONTROLLER_LAUNCHER_SRC $AKS_NODE_CONTROLLER_LAUNCHER_DEST 0755
vhdbuilder/packer/pre-install-dependencies.sh:89
- These comments say the boothook's explicit "systemctl start --no-block aks-node-controller.service" call is the sole trigger for ANC, but the boothook template now prefers directly nohup'ing aks-node-controller-launcher.sh when present. The comment should reflect the current launch mechanism to avoid confusion.
# the boothook's own explicit "systemctl start" would be a no-op and ANC would
# never actually run with the real config. The boothook's explicit
# "systemctl start --no-block aks-node-controller.service" call (issued only
# after those files exist) remains the sole trigger for this unit.
vhdbuilder/packer/pre-install-dependencies.sh:142
- PR description says the boot-time improvement comes from removing an artificial delay in a DHCP dhcpd call during cloud-init, but the implementation here appends "nodelay" to /etc/dhcpcd.conf (dhcpcd). This either targets a different DHCP client than described, or the description needs updating.
if { isUbuntu "$OS" || isAzureLinux "$OS"; }; then
echo "nodelay" | tee -a /etc/dhcpcd.conf
e2e/vmss.go:1494
- The insertion point now searches for the launcher script invocation, but the error message still says "missing aks-node-controller service start", which is misleading when debugging boothook template changes.
insertPos := strings.Index(boothookStr, "/bin/bash /opt/azure/containers/aks-node-controller-launcher.sh")
if insertPos == -1 {
insertPos = strings.Index(boothookStr, "systemctl start --no-block aks-node-controller-hack.service")
}
| [Service] | ||
| Type=oneshot | ||
| ExecStart=/opt/azure/containers/aks-node-controller-wrapper.sh | ||
| ExecStart=/opt/azure/containers/aks-node-controller-launcher.sh | ||
| RemainAfterExit=yes |
28aee12 to
e8e6f51
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
parts/linux/cloud-init/artifacts/aks-node-controller.service:16
- ExecStart points to /opt/azure/containers/aks-node-controller-launcher.sh, but that script is not present anywhere under parts/linux/cloud-init/artifacts/ in this PR. As-is, VHD build/provisioning will fail (or ANC will never start) because the unit references a non-existent executable.
[Service]
Type=oneshot
ExecStart=/opt/azure/containers/aks-node-controller-launcher.sh
RemainAfterExit=yes
vhdbuilder/packer/vhd-image-builder-base.json:112
- This Packer file provisioner copies parts/linux/cloud-init/artifacts/aks-node-controller-launcher.sh, but that file does not exist in the repo with the current PR contents. This will cause the VHD build to fail when Packer tries to upload the file.
{
"type": "file",
"source": "parts/linux/cloud-init/artifacts/aks-node-controller-launcher.sh",
"destination": "/home/packer/aks-node-controller-launcher.sh"
},
vhdbuilder/packer/packer_source.sh:280
- copyPackerFiles copies /home/packer/aks-node-controller-launcher.sh into /opt/azure/containers, but the upstream Packer templates now reference a launcher script that isn't present under parts/linux/cloud-init/artifacts/. Unless the launcher file is added, this cpAndMode will fail (and/or the resulting VHD will be missing the intended launcher).
AKS_NODE_CONTROLLER_SRC=/home/packer/aks-node-controller
AKS_NODE_CONTROLLER_DEST=/opt/azure/containers/aks-node-controller
cpAndMode $AKS_NODE_CONTROLLER_SRC $AKS_NODE_CONTROLLER_DEST 755
cpAndMode $AKS_NODE_CONTROLLER_LAUNCHER_SRC $AKS_NODE_CONTROLLER_LAUNCHER_DEST 0755
vhdbuilder/packer/imagecustomizer/azlosguard/azlosguard.yml:75
- azlosguard imagecustomizer references /AgentBaker/parts/linux/cloud-init/artifacts/aks-node-controller-launcher.sh, but that script is not present in parts/linux/cloud-init/artifacts/ in this PR. This will break the image customization step (missing source file).
- source: /AgentBaker/parts/linux/cloud-init/artifacts/aks-node-controller.service
destination: /etc/systemd/system/aks-node-controller.service
permissions: 600
- source: /AgentBaker/parts/linux/cloud-init/artifacts/aks-node-controller-launcher.sh
destination: /opt/azure/containers/aks-node-controller-launcher.sh
permissions: 755
vhdbuilder/packer/pre-install-dependencies.sh:91
- pre-install-dependencies.sh does not appear to run with
set -e, and most systemctl calls here are guarded with|| exit 1. This newsystemctl disableis unguarded, so a failure (e.g., unit not present) would silently continue and potentially produce inconsistent VHDs.
# after those files exist) remains the sole trigger for this unit.
# Sometimes its also started diretly in boothook
systemctl disable aks-node-controller.service
vhdbuilder/packer/pre-install-dependencies.sh:143
- This block introduces several new commands (tee writes, systemctl daemon-reload/enable) without any failure handling, even though this script generally expects to exit on failure (
|| exit 1). Silent failures here can leave the VHD in a partially configured state (missing cache-warmup unit and/or dhcpcd config change).
if { isUbuntu "$OS" || isAzureLinux "$OS"; }; then
echo "nodelay" | tee -a /etc/dhcpcd.conf
tee /etc/systemd/system/cache-warmup.service > /dev/null << 'EOF'
| Type=oneshot | ||
| ExecStart=/opt/azure/containers/aks-node-controller-wrapper.sh | ||
| ExecStart=/opt/azure/containers/aks-node-controller-launcher.sh | ||
| RemainAfterExit=yes |
e8e6f51 to
4d424da
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
vhdbuilder/packer/pre-install-dependencies.sh:91
systemctl disable aks-node-controller.serviceisn’t guarded with|| exit 1(and this script doesn’t useset -e), so a failure here would silently leave the unit enabled despite the comment above stating it must remain disabled in the VHD image.
systemctl disable aks-node-controller.service
vhdbuilder/packer/pre-install-dependencies.sh:142
- This writes/creates
/etc/dhcpcd.conf, but there are no other references todhcpcdin the provisioning/VHD scripts, and the PR description mentions adhcpddelay. Guarding this avoids creating an unused config file on images that don’t run dhcpcd and makes it clearer whether this change can have any effect.
echo "nodelay" | tee -a /etc/dhcpcd.conf
vhdbuilder/packer/pre-install-dependencies.sh:157
- The cache-warmup unit setup is intended to affect boot behavior, but
systemctl daemon-reload/systemctl enablearen’t checked for failure (and this script doesn’t run withset -e). If either command fails, the image can be left in an unexpected state without failing the build.
systemctl daemon-reload
systemctl enable cache-warmup.service
vhdbuilder/packer/pre-install-dependencies.sh:86
- The explanatory comment still refers to the "wrapper" even though the unit now executes
aks-node-controller-launcher.sh. Updating terminology here avoids confusion when correlating the service behavior with the launcher script.
# boothook has written the provision config/nbc-cmd files, causing the wrapper's
# graceful no-op exit to mark the oneshot unit "active (exited)" - after which
# the boothook's own explicit "systemctl start" would be a no-op and ANC would
vhdbuilder/packer/pre-install-dependencies.sh:90
- Spelling/grammar in this comment ("its"/"diretly") makes the intent harder to read in a boot-critical script.
# Sometimes its also started diretly in boothook
vhdbuilder/packer/pre-install-dependencies.sh:89
- This comment says the boothook’s explicit
systemctl startremains the “sole trigger” for the unit, but the updated boothook template now prefers launchingaks-node-controller-launcher.shdirectly (falling back tosystemctl startonly when the launcher is absent). Updating the wording would keep the rationale accurate.
# never actually run with the real config. The boothook's explicit
# "systemctl start --no-block aks-node-controller.service" call (issued only
# after those files exist) remains the sole trigger for this unit.
| logger -t aks-boothook "launching aks-node-controller $(date -Ins)" | ||
| if [ -f /opt/azure/containers/aks-node-controller-launcher.sh ]; then | ||
| nohup /bin/bash /opt/azure/containers/aks-node-controller-launcher.sh > /var/log/azure/aks-node-controller.output 2>&1 & | ||
| else | ||
| systemctl start --no-block aks-node-controller.service |
4d424da to
7f29301
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
vhdbuilder/packer/pre-install-dependencies.sh:91
- The comment has typos/grammar issues ("its"/"diretly") which makes the intent harder to read.
# "systemctl start --no-block aks-node-controller.service" call (issued only
# after those files exist) remains the sole trigger for this unit.
# Sometimes its also started diretly in boothook
systemctl disable aks-node-controller.service
vhdbuilder/packer/pre-install-dependencies.sh:143
- This change appends
nodelayto/etc/dhcpcd.conf, but the PR description/review thread discussion refers to removing delay from adhcpd-based call during cloud-init. There are nodhcpdreferences anywhere in the repo, so it’s unclear whether this knob affects the described boot-time path or if the description/implementation is out of sync.
if { isUbuntu "$OS" || isAzureLinux "$OS"; }; then
echo "nodelay" | tee -a /etc/dhcpcd.conf
tee /etc/systemd/system/cache-warmup.service > /dev/null << 'EOF'
e2e/vmss.go:1497
- The error message is now misleading: the code looks for the launcher start marker (or the hack service), but the error still says the boothook is missing the "service start".
}
if insertPos == -1 {
return "", fmt.Errorf("cloud-boothook customData missing aks-node-controller service start")
}
spec/parts/linux/cloud-init/artifacts/aks_node_controller_launcher_spec.sh:8
- The spec was renamed to target the launcher, but the temp directory is still named
aks-node-controller-wrapper, which is confusing when debugging failures.
AgentBaker Linux gate detectiveRun: 173592672 Detective summary: Multiple scenarios timed out around 1020s while creating/preparing VMSS before meaningful node validation. Likely cause / signature: Known WestUS3 VMSS provisioning/context-deadline flake; this run matches the existing wiki signature. Confidence: High Recommended owner/action: AgentBaker E2E watcher to rerun and keep tracking under the existing signature. Strongest alternative: PR #8993 boothook/ANC startup change delayed provisioning, but less likely because failures hit multiple unrelated scenarios at the VMSS creation/prep timeout boundary. Evidence: timeline failed task exit 1; failed test run 544313106; task log 562 around line 849; build metadata/PR metadata. |
After all these changes we gain about 5-6s improvement in boot times.
Network online happens during cloud init local phase so when boothook runs which is part of cloud init phase network is definitely online so no check is required.
Before:
After: