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.

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

Update kubelet arguments #10404

Closed
wants to merge 1 commit into from
Closed

Update kubelet arguments #10404

wants to merge 1 commit into from

Conversation

fmunteanu
Copy link

Proposed Changes

See #10399 (comment) for a detailed explanation of proposed change.

Types of Changes

Bugfix

Verification

See #10399 (comment) for verification details.

Testing

This change is covered by testing.

Linked Issues

User-Facing Change

NONE

Signed-off-by: Floren Munteanu <floren@axivo.com>
@brandond
Copy link
Member

brandond commented Jun 26, 2024

These are long-standing defaults, I can't imagine everyone would want them removed. Most folks are not using kubelet config files, or any custom kubelet args, so they are not affected by the issue you diagnosed.

What we would probably accept is a PR to allow somehow clearing or removing the defaults, so that the value from the config file would be honored instead.

@fmunteanu
Copy link
Author

fmunteanu commented Jun 26, 2024

The problem is these long lasting settings you mention break the default garbage collection eviction behavior. As documented into issue. Even if people don’t use the settings, they are indirectly affected, without even realizing it.

This is the evictionHard output on a vanilla K8s cluster, which produces the expected results:

curl -X GET http://127.0.0.1:8001/api/v1/nodes/<node name>/proxy/configz -s | jq .kubeletconfig.evictionHard
{
  "imagefs.available": "15%",
  "memory.available": "100Mi",
  "nodefs.available": "10%",
  "nodefs.inodesFree": "5%"
}

We need to remove the two arguments and let kubelet set the default values.

@brandond
Copy link
Member

brandond commented Jun 26, 2024

The problem is these long lasting settings you mention break the default garbage collection eviction behavior.

I understand that the current default args make it difficult to pass configuration via the kubelet config file instead of using args, but they do not "break" the default eviction behavior. We have been shipping these same settings since the very first commit to the K3s project 6 years ago, and you are the first person to take issue with them.

As I said above, we would accept a PR that allowed clearing or removing the defaults args, but we are NOT willing to remove them by default.

@fmunteanu
Copy link
Author

fmunteanu commented Jun 26, 2024

I understand that the current default args make it difficult to pass configuration via the kubelet config file.

Actually the problem is related to the fact K3s inadvertently changed the default garbage collection behavior, is not about just the configuration. The new PR you linked into my original issue will fix the second problem, once K3s moves to a kubelet configuration based file. I just want to make sure the current kubelet settings I’m attempting to remove in this PR will not be kept, when the configuration will be migrated to a file. Therefore I prefer to have these settings removed, prior.

but they do not "break" the default eviction behavior.

I can assure you they do, please see the official documentation I linked into issue, where is clearly stated the breaking behavior.

Sometimes bugs like that can be present for years, until someone notices the issue. I’m sure the devs will agree with my detailed troubleshooting once they review it. From my perspective, what is important is to address the issue of defining custom kubelet settings in K3s, which prohibit default settings to be used.

@brandond
Copy link
Member

brandond commented Jun 27, 2024

We are not going to remove these defaults, as discussed above and at #10399 (comment). I will leave that issue open, to track improving the ability to clear or remove kubelet args so that you can override our CLI defaults via the kubelet config file.

Until then, I would recommend simply setting your preferred values via --kubelet-arg, instead of the kubelet config file.

@brandond brandond closed this Jun 27, 2024
@fmunteanu
Copy link
Author

fmunteanu commented Jun 27, 2024

Thank you @brandond for discussing it with the other devs internally. I’m honestly surprised at the outcome, these settings should not be there. At least with the new kubelet configuration file, end-user will be able to correct the wrong configuration.

Regardless, I appreciate you looked into this.

@fmunteanu fmunteanu deleted the fix/kubelet-args branch June 28, 2024 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants