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

kubelet: add setting for configuring containerLogMaxFiles and MaxSize #1589

Merged
merged 2 commits into from
May 27, 2021

Conversation

samjo-nyang
Copy link
Contributor

Description of changes:
Add containerLogMaxFiles and containerLogMaxSize to kubelet configuration.
FYI: https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet/

Testing done:
This patch is currently use for our infrastructure.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@jhaynes jhaynes added area/kubernetes K8s including EKS, EKS-A, and including VMW priority/p1 status/needs-triage Pending triage or re-evaluation type/enhancement New feature or request labels May 21, 2021
@jhaynes jhaynes requested review from tjkirch and bcressey May 21, 2021 16:32
@jpculp
Copy link
Member

jpculp commented May 21, 2021

@samjo-nyang, thank you for your contribution! It looks great! In order to facilitate proper upgrade/downgrade between Bottlerocket versions, changes to settings like this one require something known as a migration. Migrations remove new settings when a user downgrades to a previous version of Bottlerocket. I encourage you to take a look at this recent PR that adds a similar setting. The second commit in particular provides the migration. Of course, if you'd rather, we can take care of this for you. Just let us know 🚀

@jpculp jpculp self-requested a review May 21, 2021 20:19
@samjo-nyang
Copy link
Contributor Author

@jahkeup Thanks for your kind feedback! I added the migration files to v1.1.2; is this correct?

@jpculp
Copy link
Member

jpculp commented May 24, 2021

@samjo-nyang, code wise this all looks right to me 😃! I might recommend cleaning up the commits a little bit by either adding more detail to the first commit or removing the word config. It would also be great if you could combine the last 2 to a single migration commit, but we can do that on our end as well. If you wanted to keep the commit messages consistent with our previous kublet settings, you can use the previous PR I mentioned as a reference or search the git log for commits with the prefixes kubelet: or migrations:. Thank you again for your contribution!

@samjo-nyang samjo-nyang changed the title Add containerLogMaxFiles and containerLogMaxSize to kubelet config kubelet: add setting for configuring containerLogMaxFiles and MaxSize May 25, 2021
@samjo-nyang
Copy link
Contributor Author

@jpculp I just rebase & reword the commits

Copy link
Member

@jpculp jpculp left a comment

Choose a reason for hiding this comment

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

Thanks @samjo-nyang! 🚀

packages/kubernetes-1.16/kubelet-config Outdated Show resolved Hide resolved
packages/kubernetes-1.16/kubelet-config Outdated Show resolved Hide resolved
@webern
Copy link
Contributor

webern commented May 27, 2021

@samjo-nyang, thank you! Did you have a chance to run it with these changes? If not, one of us will check it today and get this merged.

@samjo-nyang
Copy link
Contributor Author

I had no time to run with these exact changes; I'm glad if the bottlerocket team test this pr. Thanks

@webern
Copy link
Contributor

webern commented May 27, 2021

Thanks again @samjo-nyang
I tested the latest push, described below, and it's good.
Merging 🚀 🚀 🚀

Testing of aa6d1ae

I ran an instance without these settings and observed that they were not in the kubelet config.

I logged in and ran:

apiclient set settings.kubernetes.container-log-max-size=100M

And saw that this was added to the kubelet config:

containerLogMaxSize: 100M

I ran this:

apiclient set settings.kubernetes.container-log-max-files=13

And saw that this was added to the kubelet config

containerLogMaxFiles: 13

I ran a pod, it worked.

Next, I ran an instance with this in its userdata

container-log-max-size = "101M"
container-log-max-files = 33

Ran a pod, it worked. Logged in and checked the config and saw:

containerLogMaxSize: 101M
containerLogMaxFiles: 33

@webern webern merged commit 94e0167 into bottlerocket-os:develop May 27, 2021
@bcressey bcressey removed the status/needs-triage Pending triage or re-evaluation label Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubernetes K8s including EKS, EKS-A, and including VMW type/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants