helm: Allow configuration of the install-cni container resources

[RenaudWasTaken]

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

[RenaudWasTaken]

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]

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]

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]

Helm changes LGTM, also interested about @squeed question.

[joestringer]

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]

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]

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]

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.

[RenaudWasTaken]

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]

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

[joestringer]

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

[joestringer]

I can run the testsuite to gather that feedback.

[joestringer]

/test

[joestringer]

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

[joestringer]

/test

[aanm]

/test

[squeed]

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

[squeed]

/test

[RenaudWasTaken]

Thanks for pushing this PR over the line!