fix: create new field kubelet_cmd_flags which can replace kubelet_flags #7771
Conversation
awesomenix
requested review from
AbelHu,
Devinwong,
cameronmeissner,
djsly,
ganeshkumarashok,
juan-lee,
junjiezhang1997,
lilypan26,
mxj220,
pdamianov-dev,
phealy,
r2k1,
timmy-wright and
zachary-bailey
as code owners
There was a problem hiding this comment.
Pull request overview
This PR introduces a new kubelet_cmd_flags field to provide a raw string representation of kubelet command-line flags, addressing limitations with the existing map-based kubelet_flags approach that loses ordering and filtering control.
Changes:
- Added
kubelet_cmd_flagsstring field toKubeletConfigproto definition - Updated parser to prioritize
kubelet_cmd_flagsoverkubelet_flagswhen present - Modified tests to validate both flag formats and use
agent.GetOrderedKubeletConfigFlagString()
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| aks-node-controller/proto/aksnodeconfig/v1/kubelet_config.proto | Added new optional kubelet_cmd_flags string field to support raw flag format |
| aks-node-controller/pkg/gen/aksnodeconfig/v1/kubelet_config.pb.go | Generated Go code for new proto field including getter method |
| aks-node-controller/parser/parser_test.go | Updated test to use kubelet_cmd_flags via agent.GetOrderedKubeletConfigFlagString() |
| aks-node-controller/parser/helper_test.go | Added test case for kubelet_cmd_flags and improved comparison with trimmed whitespace |
| aks-node-controller/parser/helper.go | Modified getKubeletFlags() to return kubelet_cmd_flags if present, falling back to sorted map |
| aks-node-controller/Makefile | Updated docker platform specification and branch reference from master to main |
| if kubeletConfig.GetKubeletCmdFlags() != "" { | ||
| return kubeletConfig.GetKubeletCmdFlags() | ||
| } |
There was a problem hiding this comment.
The function now has two different return paths with distinct behaviors (raw string vs. sorted map), but this is not documented. Add a comment explaining that kubelet_cmd_flags takes precedence when present, and that the fallback behavior constructs flags from the map.
There was a problem hiding this comment.
@awesomenix could you please document this new field here? KubeletConfig in aks-node-controller/proto/README.md. Thanks for making this change
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
Devinwong
left a comment
Devinwong
left a comment
There was a problem hiding this comment.
approved with a minor suggestion
|
@awesomenix I've opened a new pull request, #7772, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: awesomenix <[email protected]>
What this PR does / why we need it:
Current kubelet flags expects a map which is odd since the caller already know the ordering and filtering, provide a raw way to use cmd flags as is.
Which issue(s) this PR fixes:
Fixes #