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

chore: add requests and limits to initContainer #972

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

Conversation

pintohutch
Copy link
Collaborator

@pintohutch pintohutch commented May 14, 2024

Avoids issues with Gatekeeper policies rejecting workloads that do not specify resource requests.

Note - the scheduler should not be affected, as the maximum of initContainers and containers is considered the effective request amount.

Also fixes a minor nit where we were setting the wrong container requests from the values file. We rename "collector" values to "prometheus" and fix the incorrect mapping in our helm charts.

Avoids issues with Gatekeeper policies rejecting workloads that do not
specify resource requests.
Copy link
Collaborator

@bernot-dev bernot-dev left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice, thanks! Do you mind rebasing your second commit to one for conform? CI?

@@ -49,11 +49,11 @@ resources:
memory: 16M
bash:
limits:
memory: 32M
memory: 1M
Copy link
Collaborator

Choose a reason for hiding this comment

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

I hope touch CLI does not use more than 1M.. but it's likely we see occasional OOM here, no? Is it worth a risk?

@pintohutch
Copy link
Collaborator Author

Yea - I may hold off on merging. For some reason E2E tests are timing out with this change...

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.

None yet

3 participants