feat: run aks node controller at boot time faster by 15s#8082
feat: run aks node controller at boot time faster by 15s#8082awesomenix merged 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts the e2e VMSS provisioning flow to run the aks-node-controller hack in the foreground (synchronously) and alters how provisioning status is validated during scenario setup.
Changes:
- Run
/opt/azure/bin/aks-node-controller-hack provision ...synchronously in cloud-init instead of backgrounding it. - Stop setting the VMSS CustomScript
commandToExecutewhen provisioning viaAKSNodeConfig(commented out). - Disable the post-create Custom Script Extension status check (commented out).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| e2e/vmss.go | Runs aks-node-controller-hack provision synchronously; comments out CSE command wiring when using AKSNodeConfig. |
| e2e/test_helpers.go | Comments out the VMSS Custom Script Extension status validation after VMSS creation. |
Comments suppressed due to low confidence (1)
e2e/vmss.go:158
cseis no longer set whens.Runtime.AKSNodeConfig != nil. In theDisableScriptLessCompilationpath this results in generating CustomData that only writes the aks-node-controller config file, but does not execute aks-node-controller (the VMSS CustomScript extension is skipped becausecseCmdis empty). This will prevent the node from being provisioned. Restore wiring so scriptless mode still executes/opt/azure/containers/aks-node-controller provision-wait(or alternatively ensure CustomDataWithHack runs the equivalent ofprovision-wait). Note: leavingcseempty can also lead to a nil dereference ingetBaseVMSSModelfor Windows, which assumes an ExtensionProfile exists.
if s.Runtime.AKSNodeConfig != nil {
//cse = nodeconfigutils.CSE
customData = func() string {
if config.Config.DisableScriptLessCompilation {
data, err := nodeconfigutils.CustomData(s.Runtime.AKSNodeConfig)
require.NoError(s.T, err, "failed to generate custom data from AKSNodeConfig")
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
You can also share your feedback on Copilot code review. Take the survey.
| Key components: | ||
|
|
||
| 1. `aks-node-controller.service`: systemd unit that is triggered once cloud-init is complete (guaranteeing that config is present on disk) and then kickstarts bootstrapping. | ||
| 1. `aks-node-controller.service`: systemd unit that can be started directly by cloud-boothook as soon as the config file is written, while remaining enabled on the VHD as a fallback boot hook. |
There was a problem hiding this comment.
The README states aks-node-controller.service remains enabled on the VHD as a fallback boot hook, but this PR’s unit file change removes the [Install] section (so systemctl enable aks-node-controller.service fails during VHD build). Either update this doc to match the new enable/start model, or restore an enable-able unit definition so the fallback claim is accurate.
| 1. `aks-node-controller.service`: systemd unit that can be started directly by cloud-boothook as soon as the config file is written, while remaining enabled on the VHD as a fallback boot hook. | |
| 1. `aks-node-controller.service`: systemd unit that is started directly by cloud-boothook as soon as the config file is written; it is started explicitly by the provisioning flow rather than being persistently enabled on the VHD as a fallback boot hook. |
c874fa5 to
0ac573b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 84 changed files in this pull request and generated 7 comments.
You can also share your feedback on Copilot code review. Take the survey.
| return &aksnodeconfigv1.Configuration{ | ||
| Version: "v1", | ||
| BootstrappingConfig: bootstrappingConfig, | ||
| DisableCustomData: nbc.AgentPoolProfile.IsFlatcar() || nbc.AgentPoolProfile.IsACL(), | ||
| DisableCustomData: true, | ||
| LinuxAdminUsername: "azureuser", | ||
| VmSize: config.Config.DefaultVMSKU, | ||
| ClusterConfig: &aksnodeconfigv1.ClusterConfig{ | ||
| Location: nbc.ContainerService.Location, | ||
| ResourceGroup: nbc.ResourceGroupName, | ||
| VmType: aksnodeconfigv1.VmType_VM_TYPE_VMSS, | ||
| ClusterNetworkConfig: &aksnodeconfigv1.ClusterNetworkConfig{ | ||
| SecurityGroupName: cs.Properties.GetNSGName(), | ||
| VnetName: cs.Properties.GetVirtualNetworkName(), | ||
| VnetResourceGroup: cs.Properties.GetVNetResourceGroupName(), | ||
| Subnet: cs.Properties.GetSubnetName(), | ||
| RouteTable: cs.Properties.GetRouteTableName(), | ||
| }, | ||
| CloudProviderConfig: &aksnodeconfigv1.CloudProviderConfig{ | ||
| Backoff: cs.Properties.OrchestratorProfile.KubernetesConfig.CloudProviderBackoff, | ||
| BackoffMode: cs.Properties.OrchestratorProfile.KubernetesConfig.CloudProviderBackoffMode, | ||
| BackoffRetries: to.Ptr(int32(cs.Properties.OrchestratorProfile.KubernetesConfig.CloudProviderBackoffRetries)), | ||
| BackoffExponent: to.Ptr(cs.Properties.OrchestratorProfile.KubernetesConfig.CloudProviderBackoffExponent), | ||
| BackoffDuration: to.Ptr(int32(cs.Properties.OrchestratorProfile.KubernetesConfig.CloudProviderBackoffDuration)), | ||
| BackoffJitter: to.Ptr(cs.Properties.OrchestratorProfile.KubernetesConfig.CloudProviderBackoffJitter), | ||
| RateLimit: cs.Properties.OrchestratorProfile.KubernetesConfig.CloudProviderRateLimit, | ||
| RateLimitQps: to.Ptr(cs.Properties.OrchestratorProfile.KubernetesConfig.CloudProviderRateLimitQPS), | ||
| RateLimitQpsWrite: to.Ptr(cs.Properties.OrchestratorProfile.KubernetesConfig.CloudProviderRateLimitQPSWrite), | ||
| RateLimitBucket: to.Ptr(int32(cs.Properties.OrchestratorProfile.KubernetesConfig.CloudProviderRateLimitBucket)), | ||
| RateLimitBucketWrite: to.Ptr(int32(cs.Properties.OrchestratorProfile.KubernetesConfig.CloudProviderRateLimitBucketWrite)), | ||
| }, | ||
| PrimaryScaleSet: nbc.PrimaryScaleSetName, | ||
| }, | ||
| ApiServerConfig: &aksnodeconfigv1.ApiServerConfig{ | ||
| ApiServerName: cs.Properties.HostedMasterProfile.FQDN, | ||
| }, | ||
| AuthConfig: &aksnodeconfigv1.AuthConfig{ | ||
| ServicePrincipalId: cs.Properties.ServicePrincipalProfile.ClientID, | ||
| ServicePrincipalSecret: cs.Properties.ServicePrincipalProfile.Secret, | ||
| TenantId: nbc.TenantID, | ||
| SubscriptionId: nbc.SubscriptionID, | ||
| AssignedIdentityId: nbc.UserAssignedIdentityClientID, | ||
| }, | ||
| NetworkConfig: &aksnodeconfigv1.NetworkConfig{ | ||
| NetworkPlugin: aksnodeconfigv1.NetworkPlugin_NETWORK_PLUGIN_KUBENET, | ||
| CniPluginsUrl: nbc.CloudSpecConfig.KubernetesSpecConfig.CNIPluginsDownloadURL, | ||
| VnetCniPluginsUrl: cs.Properties.OrchestratorProfile.KubernetesConfig.AzureCNIURLLinux, | ||
| }, | ||
| GpuConfig: &aksnodeconfigv1.GpuConfig{ | ||
| ConfigGpuDriver: true, | ||
| GpuDevicePlugin: false, | ||
| }, | ||
| EnableUnattendedUpgrade: true, | ||
| KubernetesVersion: cs.Properties.OrchestratorProfile.OrchestratorVersion, | ||
| ContainerdConfig: &aksnodeconfigv1.ContainerdConfig{ | ||
| ContainerdDownloadUrlBase: nbc.CloudSpecConfig.KubernetesSpecConfig.ContainerdDownloadURLBase, | ||
| }, | ||
| OutboundCommand: helpers.GetDefaultOutboundCommand(), | ||
| KubernetesCaCert: base64.StdEncoding.EncodeToString([]byte(cs.Properties.CertificateProfile.CaCertificate)), | ||
| KubeBinaryConfig: &aksnodeconfigv1.KubeBinaryConfig{ | ||
| KubeBinaryUrl: cs.Properties.OrchestratorProfile.KubernetesConfig.CustomKubeBinaryURL, | ||
| PodInfraContainerImageUrl: nbc.K8sComponents.PodInfraContainerImageURL, | ||
| }, | ||
| KubeProxyUrl: cs.Properties.OrchestratorProfile.KubernetesConfig.CustomKubeProxyImage, | ||
| HttpProxyConfig: &aksnodeconfigv1.HttpProxyConfig{ | ||
| NoProxyEntries: *nbc.HTTPProxyConfig.NoProxy, | ||
| }, | ||
| LocalDnsProfile: &aksnodeconfigv1.LocalDnsProfile{ | ||
| EnableLocalDns: true, | ||
| EnableLocalDns: false, | ||
| CpuLimitInMilliCores: to.Ptr(int32(2008)), |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 85 changed files in this pull request and generated 13 comments.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 89 changed files in this pull request and generated 7 comments.
You can also share your feedback on Copilot code review. Take the survey.
aks-node-controller/parser/parser.go
Outdated
| "SERVICE_ACCOUNT_IMAGE_PULL_DEFAULT_TENANT_ID": config.GetServiceAccountImagePullProfile().GetDefaultTenantId(), | ||
| "IDENTITY_BINDINGS_LOCAL_AUTHORITY_SNI": config.GetServiceAccountImagePullProfile().GetLocalAuthoritySni(), | ||
| "CSE_TIMEOUT": getCSETimeout(config), | ||
| "SKIP_WALA_HOLD": "true", |
There was a problem hiding this comment.
wala lol
nit: SKIP_WAAGENT_HOLD? more clear what ur referring to
| return base64.StdEncoding.EncodeToString([]byte(customDataYAML)), nil | ||
| } | ||
|
|
||
| func writeMIMEPart(writer *multipart.Writer, contentType, content string) error { |
There was a problem hiding this comment.
what does MIME stand for? can we had a comment explaining
| data, err := nodeconfigutils.CustomData(s.Runtime.AKSNodeConfig) | ||
| var data string | ||
| var err error | ||
| if s.VHD.Flatcar { |
There was a problem hiding this comment.
nit: add a comment explaining why different for flatcar?
| @@ -1,21 +1,4 @@ | |||
| echo $(date),$(hostname) > ${PROVISION_OUTPUT}; | |||
| {{if not .GetDisableCustomData}} | |||
| CLOUD_INIT_STATUS_SCRIPT="/opt/azure/containers/cloud-init-status-check.sh"; | |||
There was a problem hiding this comment.
I understand the effort in this PR is somewhat remove the hard dependency with cloud-init status ready. However, the cloud-init-status-check.sh was added by a repair item for some intermittent sev2. Not meaning we can't remove it, just need to be aware that it could cause intermittent sev2.
There was a problem hiding this comment.
we run the service before even cloud init is finished, so waiting for it doesnt make sense.
There was a problem hiding this comment.
may be add this part of provision-wait?
There was a problem hiding this comment.
aks-node-controller doesn't depend on it. The status check is more for the provisioning scripts cse_*.sh, which was what I saw from that sev2.
There was a problem hiding this comment.
Sync'd offline. Nishchay checked with the original owner of cloud-init-status-check.sh and confirmed this is no longer needed.
aks-node-controller/parser/parser.go
Outdated
| "SERVICE_ACCOUNT_IMAGE_PULL_DEFAULT_TENANT_ID": config.GetServiceAccountImagePullProfile().GetDefaultTenantId(), | ||
| "IDENTITY_BINDINGS_LOCAL_AUTHORITY_SNI": config.GetServiceAccountImagePullProfile().GetLocalAuthoritySni(), | ||
| "CSE_TIMEOUT": getCSETimeout(config), | ||
| "SKIP_WALA_HOLD": "true", |
There was a problem hiding this comment.
Would this not cause the sev2 that @SriHarsha001 encountered with lower end VMs in early Feb?
There was a problem hiding this comment.
we arent dependent on WAAGENT anymore since we run way early
There was a problem hiding this comment.
aks-node-controller doesn't depends on waagent, but the scripts may still depend on it? Anyway, if we can use the lower end vm to do multiple runs, then we may have a better understanding of the impact.
There was a problem hiding this comment.
Will test on A series VM. Also we are no dependent on WAagent like phase1 since cse scripts are not executed as part of waagent and there was a worry there might be a restart of waagent in middle of execution of cse, right now we only wait.
| writer := multipart.NewWriter(&customData) | ||
|
|
||
| fmt.Fprintf(&customData, "MIME-Version: 1.0\r\n") | ||
| fmt.Fprintf(&customData, "Content-Type: multipart/mixed; boundary=%q\r\n\r\n", writer.Boundary()) |
There was a problem hiding this comment.
Question: what is the reason we need it to be multipart MIME?
IIUC, the first part is the cloud-boothook which tries to write the aks-node-config to the node asap. Is the second part only to print a message currently? Are we going to use this second part for other purpose in the future?
There was a problem hiding this comment.
idea is to bundle our current nodecustomdata.yaml probably for hotfixing doesnt work since hotfixing needs to use boothook as well.
| ConditionPathExists=/opt/azure/containers/aks-node-controller-config.json | ||
| After=cloud-init.target | ||
| After=oem-cloudinit.service enable-oem-cloudinit.service | ||
| Wants=cloud-init.target | ||
| After=network-online.target | ||
| Wants=network-online.target | ||
|
|
||
| [Service] | ||
| Type=oneshot | ||
| ExecStart=/opt/azure/containers/aks-node-controller-wrapper.sh | ||
| RemainAfterExit=No | ||
| RemainAfterExit=yes | ||
|
|
||
| [Install] | ||
| WantedBy=cloud-init.target | ||
| WantedBy=oem-cloudinit.service | ||
| WantedBy=basic.target |
There was a problem hiding this comment.
Switching the unit to WantedBy=basic.target while keeping ConditionPathExists=/opt/azure/containers/aks-node-controller-config.json can prevent the service from ever running on flows where the config is only written later by cloud-init (e.g., older/custom data that uses write_files). systemd will skip the unit when the condition fails at basic.target time and won’t automatically retry when the file appears. Consider keeping the enable/fallback path tied to cloud-init.target (as before) and rely on the boothook’s explicit systemctl start for early-start, or add a path/trigger that starts the service when the config file is created.
| systemctl daemon-reload | ||
| systemctl enable aks-node-controller-hack.service | ||
| ` |
There was a problem hiding this comment.
The boothook creates and enables aks-node-controller-hack.service but never starts it. systemctl enable does not start the unit, and depending on when the boothook runs relative to basic.target, the unit may not run on the first boot at all. To mirror the main boothook flow and make this deterministic, start the unit explicitly (e.g., systemctl start --no-block ...) or use systemctl enable --now ....
| systemctl daemon-reload | |
| systemctl enable aks-node-controller-hack.service | |
| ` | |
| systemctl daemon-reload | |
| systemctl enable --now aks-node-controller-hack.service | |
| ` |
Summary
cloud-boothookaks-node-controller.serviceordering to match the earlier-start model while keeping the VHD enable path intactDetails
aks-node-controller/pkg/nodeconfigutils/utils.gonow writes the config from boothook and startsaks-node-controller.serviceimmediatelyparts/linux/cloud-init/artifacts/aks-node-controller.servicenow waits onnetwork-online.targetand staysactive (exited)after the one-shot rune2e/vmss.goswitches the hack flow fromruncmdto a boothook-dropped service and wrappergenerate-testdatawas run to refresh generated snapshot data impacted by thepkgchangeTimings