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

helm: Allow configuration of the install-cni container resources #27469

Merged
merged 1 commit into from
Nov 14, 2023
Merged

helm: Allow configuration of the install-cni container resources #27469

merged 1 commit into from
Nov 14, 2023
+20 −3

Conversation

RenaudWasTaken
Copy link
Contributor

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • [N/A] All code is covered by unit and/or runtime tests where feasible
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • [N/A] If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

The helm chart does not provide a way to specify how much resources this container uses.
This change adds a way to configure this field.

Helm: Allow configuration of the install-cni container resources field

Sorry, something went wrong.

@RenaudWasTaken RenaudWasTaken requested review from a team as code owners August 11, 2023 22:15
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 11, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Aug 11, 2023
@RenaudWasTaken
Copy link
Contributor Author

Hello @joamaki and @kaworu!

Gentle bump on this small PR, if you can take a look at it when you have some time and let me know if it looks good :)

Thanks a lot!

@ti-mo ti-mo added the release-note/misc This PR makes changes that have no direct user impact. label Aug 16, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 16, 2023
@ti-mo ti-mo requested a review from squeed August 16, 2023 10:10
@ti-mo
Copy link
Contributor

ti-mo commented Aug 16, 2023

Added @squeed as a reviewer to hopefully speed things along.
@RenaudWasTaken Could you please include the reason for this change in your commit message and PR description? Does the absence of this cause problems? Also, is 10MiB enough? The cilium-cni binary alone weighs 47MiB.

@squeed
Copy link
Contributor

squeed commented Aug 16, 2023

This is an initContainer; we don't have configurable resources for any other initContainers -- what's the use-case?

(In fact, no other initContainers have resources at all... I wonder which is the better practice?)

kaworu
kaworu approved these changes Aug 16, 2023
View reviewed changes
Copy link
Member

@kaworu kaworu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Helm changes LGTM, also interested about @squeed question.

joestringer
joestringer requested changes Aug 23, 2023
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. In principle I don't see why not to make this configurable if it's important for your deployment case, since this presumably would help to reserve additional required resources on the nodes, and perhaps in constrained environments that can be the difference between being able to reliably redeploy Cilium or not.

At the same time, I'm not sure I understand where the limits came from or how we could keep these accurately up to date over time. I'd suggest dropping the defaults and just making it configurable, then in your environment you can specify whatever limits you find to be viable.

I'm also curious like the others above if there's anything particular about your environments that motivate this change?

@joestringer
Copy link
Member

It looks like the smoke tests failed, see the error below.

please run 'make -C install/kubernetes' and submit your changes
Error: Process completed with exit code 1.

@RenaudWasTaken
Copy link
Contributor Author

Hello!

Sorry for the lag! I'll fix the PR early September (I'm out of office until then).

Our use case is that we run a pod at node initialization that runs checks on the node before releasing it for general scheduling.

To do that it's currently configured to take all the resources of the node.

Typically to support this use case we set out system daemonsets such that requests and limits are at 0.

When we deploy cilium with this init container, because of the kubelet accounting strategy, the node does not have it's full resources available.

Which then prevents our pod from scheduling 😅

@joestringer
Copy link
Member

Interesting use case! I'll mark the PR as draft for now while you address the feedback, then you can click "Ready for review" to notify us afresh when you've updated the PR.

@joestringer joestringer marked this pull request as draft August 23, 2023 17:00
@RenaudWasTaken RenaudWasTaken force-pushed the helm-cilium branch 5 times, most recently from c941781 to a450e46 Compare September 7, 2023 00:12
@RenaudWasTaken
Copy link
Contributor Author

Alright, I've update the PR with the specific changes, but I don't seem to have the ability to mark my PR out of draft
@joestringer could you take another stab and move the PR out of draft?

Thanks a lot!

@RenaudWasTaken
Copy link
Contributor Author

@kaworu, @joestringer and @squeed gentle bump in your inbox :)

@joestringer
Copy link
Member

Do you see this button near the bottom of the page? This is the one I was talking about.

image

@joestringer
Copy link
Member

I can run the testsuite to gather that feedback.

@joestringer
Copy link
Member

/test

joestringer
joestringer approved these changes Sep 18, 2023
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Looks to me like nothing is functionally changing for the defaults here, the PR just makes the resources for this particular container configurable 👍

@RenaudWasTaken RenaudWasTaken force-pushed the helm-cilium branch 2 times, most recently from a4a3b41 to 922aa1e Compare October 4, 2023 05:30
@RenaudWasTaken RenaudWasTaken marked this pull request as ready for review October 4, 2023 05:30
@RenaudWasTaken RenaudWasTaken force-pushed the helm-cilium branch 2 times, most recently from e7db7ed to bb9c747 Compare October 5, 2023 02:09
@aanm aanm requested a review from squeed October 31, 2023 08:57
@joestringer
Copy link
Member

/test

@aanm
Copy link
Member

aanm commented Nov 3, 2023

/test

@squeed
Copy link
Contributor

squeed commented Nov 8, 2023

@RenaudWasTaken this should go in, I'm just rebasing on main to pick up some test fixes.

@RenaudWasTaken @squeed
helm: Allow configuration of the install-cni container resources
a00820d 
Signed-off-by: Renaud Gaubert <renaud@openai.com>
@squeed
Copy link
Contributor

squeed commented Nov 8, 2023

/test

@julianwiedmann julianwiedmann added area/cni Impacts the Container Networking Interface between Cilium and the orchestrator. area/helm Impacts helm charts and user deployment experience labels Nov 14, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 14, 2023
@julianwiedmann julianwiedmann merged commit 80722af into cilium:main Nov 14, 2023
@RenaudWasTaken
Copy link
Contributor Author

Thanks for pushing this PR over the line!

@RenaudWasTaken RenaudWasTaken deleted the helm-cilium branch November 22, 2023 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer

@aanm aanm

@kaworu kaworu

@squeed squeed

@joamaki joamaki

Assignees
No one assigned
Labels
area/cni Impacts the Container Networking Interface between Cilium and the orchestrator. area/helm Impacts helm charts and user deployment experience kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@RenaudWasTaken @ti-mo @squeed @joestringer @aanm @kaworu @joamaki @julianwiedmann