Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[wip] Switch to using kubelet config file for all supported flags #10433

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

[wip] Switch to using kubelet config file for all supported flags #10433

wants to merge 3 commits into from

Conversation

brandond
Copy link
Member

@brandond brandond commented Jun 29, 2024 •
edited
Loading

Proposed Changes

Switch to using kubelet config file for all supported flags

Types of Changes

enhancement / tech debt

Verification

See linked issue

Testing

Linked Issues

User-Facing Change

Further Comments

Currently using a hacky string replace when marshaling the config file to work around an upstream issue:

@brandond brandond requested a review from a team as a code owner June 29, 2024 07:52
@brandond brandond force-pushed the kubelet-config-dir branch 3 times, most recently from c6f443b to 5b85991 Compare June 30, 2024 08:00
@codecov Codecov
Copy link

codecov bot commented Jun 30, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 60.73620% with 64 lines in your changes missing coverage. Please review.

Project coverage is 43.68%. Comparing base (ed14f7f) to head (c9b86b7).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
pkg/daemons/agent/agent.go 67.46% 30 Missing and 11 partials ⚠️
pkg/daemons/agent/agent_linux.go 45.83% 11 Missing and 2 partials ⚠️
pkg/agent/util/file.go 0.00% 8 Missing ⚠️
pkg/agent/config/config.go 50.00% 1 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (ed14f7f) and HEAD (c9b86b7). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (ed14f7f) HEAD (c9b86b7)
e2etests 7 6
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #10433      +/-   ##
==========================================
- Coverage   49.64%   43.68%   -5.96%     
==========================================
  Files         178      178              
  Lines       14801    14903     +102     
==========================================
- Hits         7348     6511     -837     
- Misses       6105     7182    +1077     
+ Partials     1348     1210     -138
Flag Coverage Δ
e2etests 36.21% <60.73%> (-9.76%) ⬇️
inttests 19.59% <1.84%> (-0.13%) ⬇️
unittests 13.45% <0.00%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

manuelbuil
manuelbuil previously approved these changes Jul 1, 2024
View reviewed changes
dereknola
dereknola reviewed Jul 2, 2024
View reviewed changes
pkg/daemons/agent/agent_windows.go
// and a structured configuration file that upstream does not provide a convienent way to initailize with default values.
// The defaults and our desired config also vary by OS.
func kubeletArgsAndConfig(cfg *config.Agent) (map[string]string, *kubeletconfig.KubeletConfiguration, error) {
defaultConfig, err := defaultKubeletConfig(cfg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to work on Windows? Won't stuff like the cgroup driver fail/be unnecessary on this OS?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I'm honestly not sure what the windows kubelet does if you try to set the cgroup stuff. I'll try this out in RKE2 before merging.

dereknola
dereknola previously approved these changes Jul 8, 2024
View reviewed changes
Copy link
Member

@dereknola dereknola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending RKE2 cgroup checks

@brandond
Copy link
Member Author

I'm going to hold on this until the August release cycle, as I'd like more lead time on testing it.

@brandond brandond marked this pull request as draft July 10, 2024 22:03
@brandond brandond changed the title Switch to using kubelet config file for all supported flags [wip] Switch to using kubelet config file for all supported flags Jul 10, 2024
@brandond
Add "Error: " prefix to fatal errors from k3s app entry points
990660b 
Makes logged output more consistent when k3s fails during initialization

Signed-off-by: Brad Davidson <[email protected]>
@brandond
Improve error handling of CheckDeployments test helper
162b9be 
Expose actual error, so that we can tell if the deployment is not found or not ready/available

Signed-off-by: Brad Davidson <[email protected]>
@brandond brandond mentioned this pull request Oct 1, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dereknola dereknola dereknola left review comments

@manuelbuil manuelbuil manuelbuil left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brandond @dereknola @manuelbuil