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

Adjust resource requests and autoscaling of calico-node side car containers. #489

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ScheererJ
Copy link
Member

How to categorize this PR?

/area networking
/kind enhancement

What this PR does / why we need it:

Adjust resource requests and autoscaling of calico-node side car containers.

The side car containers use negligible amount of resources compared to calico-node. Therefore, requests likely just waste resources. Similarly, they fall below the minimum threshold of VPA. Hence, autoscaling them does not make sense.
minAllowed and maxAllowed of calico-node also are not that useful. Therefore, they are removed as well.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

In VPA autoscaling mode, `calico-node` should be disrupted less often as side car containers are no longer considered by VPA. Additionally, the minimum/maximum restriction are removed, which can lead to less memory consumption.

containers.

The side car containers use negligible amount of resources compared to
`calico-node`. Therefore, requests likely just waste resources.
Similarly, they fall below the minimum threshold of VPA. Hence,
autoscaling them does not make sense.
`minAllowed` and `maxAllowed` of `calico-node` also are not that useful.
Therefore, they are removed as well.
@ScheererJ ScheererJ requested review from a team as code owners September 19, 2024 13:21
@gardener-robot gardener-robot added the needs/review Needs review label Sep 19, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 19, 2024
@gardener-robot gardener-robot added area/networking Networking related kind/enhancement Enhancement, improvement, extension size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Sep 19, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 19, 2024
@axel7born
Copy link
Contributor

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Sep 19, 2024
axel7born
axel7born previously approved these changes Sep 19, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 19, 2024
@vlerenc
Copy link
Member

vlerenc commented Sep 20, 2024

Hmm... isn't removing a container from the VPA spec resulting in applying the default configuration, i.e. whatever is defined in spec.updatePolicy? That would keep all "removed" containers in updateMode Auto (instead of off) and even set controlledValues to RequestsAndLimits (even worse than RequestsOnly)?

cc @voelzmo

@ScheererJ
Copy link
Member Author

Hmm... isn't removing a container from the VPA spec resulting in applying the default configuration, i.e. whatever is defined in spec.updatePolicy? That would keep all "removed" containers in updateMode Auto (instead of off) and even set controlledValues to RequestsAndLimits (even worse than RequestsOnly)?

cc @voelzmo

Good point. Adjusted accordingly with 0f5afa2.

@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 20, 2024
@vlerenc
Copy link
Member

vlerenc commented Sep 21, 2024

Thanks @ScheererJ. I also checked in the meantime VPA and it requires the container indeed to be put explicitly into off mode if the general policy is auto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking Networking related kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants