docs(kubelet): clarify config file generation precedence with tests and variable naming#8777
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates how pkg/agent generates kubelet config-file JSON by merging sources: start from customKubeletConfig content and only backfill missing fields from flag-derived values, while keeping the existing kubelet CLI flag filtering behavior unchanged.
Changes:
- Refactors
GetKubeletConfigFileContentto build a content-derived config first, then merge in missing fields from the flag-derived config. - Introduces helper functions to merge config content and flags via a map-based merge.
- Adds a unit test to verify that content values are not overwritten while missing fields are filled from flags.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/agent/utils.go | Refactors kubelet config-file generation and adds helper-based merge logic; keeps CLI filtering semantics while preparing for a future “config-file authoritative” shift. |
| pkg/agent/utils_test.go | Adds a unit test validating that content wins on conflicts and flags backfill missing fields. |
@microsoft-github-policy-service agree company="Microsoft" |
833db25 to
95b1c88
Compare
a2f1d91 to
8a3e6a5
Compare
337916c to
a1193cf
Compare
6a67ae2 to
c451bec
Compare
c451bec to
af3e717
Compare
af3e717 to
ab281ff
Compare
…ased merge Refactor the CSE path's kubelet config file generation so that CustomKubeletConfig (user-specified via AKS API) takes precedence over CLI flag-derived values, with flags backfilling any missing fields. Precedence model: 1. CustomKubeletConfig (~12 fields) — wins when both sources set same field 2. KubeletConfig flags (~40 fields) — backfills fields not set by CustomKC 3. kubelet v1beta1 defaults — used when neither source sets the field This is consistent with existing behavior (setCustomKubeletConfig has always overwritten flag-derived values), but makes the merge logic explicit and testable via mergeKubeletConfigContentFromFlags. Variable rename (kubeletConfig → flagConfig) clarifies the three-way merge between flagConfig, contentConfig, and mergedConfig.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
ab281ff to
26b5885
Compare
| return "" | ||
| } | ||
| // translate simple values. | ||
| kubeletConfig := getAKSKubeletConfiguration(kc) |
There was a problem hiding this comment.
what was wrong with previous name here? kubeletConfig seems accurate isnt it?
There was a problem hiding this comment.
changed the name to flagConfig to emphasize that this struct is derived from flags and will subsequently be overlaid by CustomKC—aligning with the "step 2a: flag-derived → step 2b: CustomKC overlay" flow described in the PR summary.
overall Im fine with both name, I can revert the rename if needed.
There was a problem hiding this comment.
i also prefer the old naming, i think kubeletConfig is more representative of what it is
There was a problem hiding this comment.
done
Reverts the kubeletConfig→flagConfig rename in pkg/agent/utils.go. The original name is accurate and less confusing.
Summary
Document and test the existing kubelet config file generation precedence model in the legacy CSE path. Adds variable rename (kubeletConfig → flagConfig) for clarity, error handling for json.MarshalIndent, and unit tests explicitly validating the three-layer precedence: CustomKubeletConfig wins > flags backfill > v1beta1 defaults. No behavioral changes. Scriptless path flag→config file sync to follow in a separate PR.
Why this matters
When kubelet config file mode is enabled, translated flags (e.g.
--max-pods,--cluster-dns,--event-qps) are removed from CLI and only exist in the config file. The config file needs ~40 fields populated, butCustomKubeletConfigonly covers ~12 user-configurable fields. The remaining ~28 fields must come from the RP-providedKubeletConfigflag map — otherwise kubelet falls back to v1beta1 defaults which are often not what AKS intends.Precedence model
Example:
imageGCHighThresholdPercent--image-gc-high-threshold=85maxPods--max-pods=110clusterDNS--cluster-dns=172.16.0.10evictionHard--eviction-hard=memory.available<750Mi,...eventRecordQPS--event-qps=0This precedence is consistent with existing behavior —
setCustomKubeletConfig()has always overwritten flag-derived values. The pre-existing testTestGetKubeletConfigFileCustomKCShouldOverrideValuesPassedInKcexplicitly validates this.Flag classification: config file vs CLI-only
When config file mode is enabled, flags are split into two categories:
Translated to config file (
TranslatedKubeletConfigFlags, 37 flags) — removed from CLI, written to/etc/default/kubeletconfig.json:--addressaddress--anonymous-authauthentication.anonymous.enabled--authentication-token-webhookauthentication.webhook.enabled--authorization-modeauthorization.mode--client-ca-fileauthentication.x509.clientCAFile--cgroups-per-qoscgroupsPerQOS--cluster-dnsclusterDNS--cluster-domainclusterDomain--container-log-max-filescontainerLogMaxFiles--container-log-max-sizecontainerLogMaxSize--cpu-cfs-quotacpuCFSQuota--cpu-cfs-quota-periodcpuCFSQuotaPeriod--cpu-manager-policycpuManagerPolicy--enforce-node-allocatableenforceNodeAllocatable--event-qpseventRecordQPS--eviction-hardevictionHard--eviction-max-pod-grace-periodevictionMaxPodGracePeriod--eviction-softevictionSoft--eviction-soft-grace-periodevictionSoftGracePeriod--fail-swap-onfailSwapOn--feature-gatesfeatureGates--image-gc-high-thresholdimageGCHighThresholdPercent--image-gc-low-thresholdimageGCLowThresholdPercent--kube-reservedkubeReserved--kube-reserved-cgroupkubeReservedCgroup--max-podsmaxPods--node-status-update-frequencynodeStatusUpdateFrequency--node-status-report-frequencynodeStatusReportFrequency(omitted from CLI)--pod-manifest-pathstaticPodPath--pod-max-pidspodPidsLimit--protect-kernel-defaultsprotectKernelDefaults--read-only-portreadOnlyPort--resolv-confresolvConf--rotate-certificatesrotateCertificates--rotate-server-certificatesserverTLSBootstrap--serialize-image-pullsserializeImagePulls--streaming-connection-idle-timeoutstreamingConnectionIdleTimeout--system-reservedsystemReserved--system-reserved-cgroupsystemReservedCgroup--tls-cert-filetlsCertFile--tls-cipher-suitestlsCipherSuites--tls-private-key-filetlsPrivateKeyFile--topology-manager-policytopologyManagerPolicy--allowed-unsafe-sysctlsallowedUnsafeSysctlsCLI-only — no corresponding field in
KubeletConfigurationschema, must stay on command line:--cloud-provider=external--kubeconfig--bootstrap-kubeconfig--node-ip--container-runtime-endpoint--runtime-cgroups--runtime-request-timeout--cgroup-driver--volume-plugin-dir--enable-server--node-labelsKUBELET_NODE_LABELS--v(verbosity)Changes
pkg/agent/utils.go: RenamekubeletConfig→flagConfigto clarify its role in the three-way flow (flagConfig → setCustomKubeletConfig → output). Add error handling forjson.MarshalIndent.pkg/agent/utils_test.go: AddTestGetKubeletConfigFileContent_PrecedenceRulesandTestGetKubeletConfigFileContent_MergesFlagsWithoutOverwritingContentdocumenting the precedence model.Future work (follow-up PR)
Test Plan
Unit Tests
All existing tests pass without modification, including:
TestGetKubeletConfigFileContent_PrecedenceRules— validates three-layer precedence (CustomKC wins > flags backfill > v1beta1 defaults)TestGetKubeletConfigFileContent_MergesFlagsWithoutOverwritingContent— CustomKC value preserved, flags fill missing fieldsTestGetKubeletConfigFileCustomKCShouldOverrideValuesPassedInKc— pre-existing test confirming CustomKC overrides flagsE2E Tests
go test -run '^Test_Ubuntu2204_KubeletCustomConfig/default$'"eventRecordQPS": 0(backfilled from flags) and"failSwapOn": true(CustomKC); CLI flags only have--cloud-provider,--kubeconfig,--node-ipgo test -run '^Test_Ubuntu2404Gen2/default$'E2E Manual Verification (SSH into VM)
Scope Note
No changes to
aks-node-controller/in this PR. Scriptless path flag→config file sync will be addressed in a follow-up PR.