kubelet: Set --config to avoid using legacy defaults #12309
Conversation
|
@brandond PTAL |
pkg/daemons/agent/agent.go
Outdated
| @@ -185,6 +185,7 @@ func defaultKubeletConfig(cfg *daemonconfig.Agent) (*kubeletconfig.KubeletConfig | |||
| NodeStatusReportFrequency: metav1.Duration{Duration: time.Minute * 5}, | |||
| NodeStatusUpdateFrequency: metav1.Duration{Duration: time.Second * 10}, | |||
| ProtectKernelDefaults: cfg.ProtectKernelDefaults, | |||
| ReadOnlyPort: 0, | |||
There was a problem hiding this comment.
You read the k/k issue. This field will be omitted from the output unless set to a nonzero value, why bother?
There was a problem hiding this comment.
https://github.com/kubernetes/kubernetes/blob/c6658c933e0ab7ed821167547c5e1ce4b891271b/cmd/kubelet/app/server.go#L218C26-L218C40 overwrites https://github.com/kubernetes/kubernetes/blob/c6658c933e0ab7ed821167547c5e1ce4b891271b/cmd/kubelet/app/server.go#L143
The latter applies the legacy non-zero default.
There was a problem hiding this comment.
There was a problem hiding this comment.
Check the output on disk, and tell me what you see as the value of this field when the config is marshalled to yaml. Can you get the zero value to appear?
There was a problem hiding this comment.
We do not need to have 0 in the yaml file. Not having it is equivalent to having it as 0.
There was a problem hiding this comment.
Right so why did you add it back to this struct if it doesn't end up in the yaml file?
You could probably resolve this by just making your other changes without reverting the commit that removed this.
pkg/daemons/agent/agent_windows.go
Outdated
| @@ -48,11 +49,9 @@ func kubeletArgsAndConfig(cfg *config.Agent) (map[string]string, *kubeletconfig. | |||
| return nil, nil, err | |||
| } | |||
| argsMap := map[string]string{ | |||
| "config": filepath.Join(cfg.KubeletConfigDir, "00-"+version.Program+"-defaults.conf"), | |||
There was a problem hiding this comment.
This will result in loading the config twice, first as the config, and then again as a drop-in. As discussed in the upstream issue, there are critical differences in behavior wrt defaults when the config file is loaded as config vs a config drop-in. I'm not confident that this will result in the behavior we want in terms of allowing users to provide their own config files or drop-ins that are loaded before or after our defaults.
There was a problem hiding this comment.
Yeah. We can move the default config to etc/kubelet.conf to avoid parsing it twice as I commented in the k/k issue. However, the current PR still works and is the simple change to fix the issue.
There was a problem hiding this comment.
The k/k logic for loading config is as follows
config := legacyDefault{}
config = apply flags to config
if --config is set: config = load --config // note that this throw away config above
if --config-dir is set: config = merge config with --config-dir
if --config or --config-dir is set: config = apply flags to config
when applying flags, it is doing config.Flag = read(--flag, default = config.Flag)
There was a problem hiding this comment.
it is currently "fixed" by using the CLI flag, as we previously did. If you're going to revert and rework an existing change it would be good to provide some justification as to why it is an improvement over the current behavior.
Ideally this would be done in the appropriate place in our PR template - instead of just deleting the template and saying "fixes X" with a link to an already-closed issue.
|
If you want to open a new issue (using the issue template) describing the current problem this addresses, and fill out the PR template, we can consider this. |
I have updated the description. Hope it is more clear. The title of the PR is also stating the issue it tries to fix. |
|
We still need a new issue for our QA team to test, describing what you're trying to fix or improve. The issue you linked has already been tested and closed. Also please fill out the PR template - since you deleted it when opening the PR, you can find it here: https://raw.githubusercontent.com/k3s-io/k3s/refs/heads/master/.github/PULL_REQUEST_TEMPLATE.md |
|
Added an issue and updated the description. |
|
I will overlay another commit to put default config in etc/kubelet.conf. If it can be a breaking change, feel free to remove the last commit. |
|
Drop the revert commit - there's no need to re-add 0 to the struct - and then make the rest of your changes on top of that. |
|
please take another look |
BoleynSu
force-pushed
the
kubelet-config
branch
2 times, most recently
from
19adc84 to
6cbc639
Compare
| if err := writeKubeletConfig(cfg.KubeletConfig, defaultConfig); err != nil { | ||
| return pkgerrors.WithMessage(err, "generate default kubelet configuration") |
There was a problem hiding this comment.
Should we remove the old drop-in at "00-"+version.Program+"-defaults.conf" if it exists? If we're going to generate our defaults in a new path, we should clean up the file generated by previous releases to avoid leaving unwanted drop-ins behind that will override what is currently expected. This could cause unexpected behavior when upgrading.
There was a problem hiding this comment.
I am not sure if the whole agent directory will be regenerated or not upon upgrade. If not, I agree we should remove "00-"+version.Program+"-defaults.conf"
There was a problem hiding this comment.
no, things do not get cleaned out of there when upgrading. We also intentionally leave the kubelet config dir alone so that users can put their own stuff in it.
There was a problem hiding this comment.
Updated the PR. I did not check the err returned by Remove. I guess it should be fine?
Fixes k3s-io#12310 It also help avoid similar issues as k3s-io#12164 in the future. I.e. we do not need to explicity override defaults by using cli flags. Signed-off-by: Boleyn Su <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12309 +/- ##
===========================================
- Coverage 40.55% 19.89% -20.67%
===========================================
Files 187 184 -3
Lines 19279 19220 -59
===========================================
- Hits 7819 3824 -3995
- Misses 10270 14965 +4695
+ Partials 1190 431 -759
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Proposed Changes
Set --config to avoid using legacy defaults
Types of Changes
New Feature
It should be a no-op for now but prevents future issues.
Verification
Testing
Linked Issues
User-Facing Change
Further Comments
It also help avoid similar issues as #12164 in the future. I.e. we do not need to explicity override defaults by using cli flags.