Skip to content

[Merged by Bors] - Added resource limits to all init containers and sidecar containers. #289

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
Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - Added resource limits to all init containers and sidecar containers. #289

wants to merge 4 commits into from

Conversation

soenkeliebau
Copy link
Member

@soenkeliebau soenkeliebau commented Jun 29, 2023 •
edited by lfrancke
Loading

Description

Renamed .resources in values.yaml to .node.resources to better reflect the fact that this applies to the daemonset pods.

Needed for stackabletech/issues#368

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes
# Author
- [ ] Changes are OpenShift compatible
- [ ] CRD changes approved
- [ ] Helm chart can be installed and deployed operator works
- [ ] Integration tests passed (for non trivial changes)

# Reviewer
- [ ] Code contains useful comments
- [ ] (Integration-)Test cases added
- [ ] Documentation added or updated
- [ ] Changelog updated
- [ ] Cargo.toml only contains references to git tags (not specific commits or branches)

# Acceptance
- [ ] Feature Tracker has been updated
- [x] Proper release label has been added

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

@soenkeliebau
Added resource limits to all init containers and sidecar containers.
8613ddd 
Renamed .resources in values.yaml to .node.resources to better reflect the fact that this applies to the daemonset pods.
@soenkeliebau soenkeliebau requested a review from nightkr July 4, 2023 07:04
@soenkeliebau
Split resource definitions for:
8fb7fce 
- driver on node
- driver-registrar on node
- external-provisioner (controller process, but currently runs per node)
@soenkeliebau soenkeliebau requested a review from sbernauer July 5, 2023 10:41
sbernauer
sbernauer reviewed Jul 5, 2023
View reviewed changes
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

What would you think of calling the properties resources, csiProvisioner.resources and csiNodeDriverRegistrar.resources to match the way we name the image settings?

@nightkr
Copy link
Member

nightkr commented Jul 5, 2023

Hm, I'd still put the driver under node since we'll want to separate that for controller Eventually:tm:, but for the CSI sidecars I'm inclined to agree.

Sorry about all the bikeshedding here, @soenkeliebau …

@sbernauer
Copy link
Member

Whatever works for me, just wanted to mention the idea

@soenkeliebau
Copy link
Member Author

Hm, I'd still put the driver under node since we'll want to separate that for controller Eventuallytm, but for the CSI sidecars I'm inclined to agree.

Sorry about all the bikeshedding here, @soenkeliebau …

But isn't the driver under node?
https://github.com/stackabletech/secret-operator/pull/289/files#diff-2d261c75b3f9442f39420de1b6b7da2f0af7cd193d30d082b027253f4637b59fR63

@nightkr

@sbernauer
Copy link
Member

I pushed a proposal that should address all comments (I hope at least)

sbernauer
sbernauer approved these changes Jul 11, 2023
View reviewed changes
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

@soenkeliebau feel free to merge when happy with the change

@soenkeliebau
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Jul 11, 2023
@soenkeliebau @sbernauer
Added resource limits to all init containers and sidecar containers. (#…
63d0a80 
…289)

# Description

Renamed .resources in values.yaml to .node.resources to better reflect the fact that this applies to the daemonset pods.

Needed for stackabletech/issues#368



Co-authored-by: Sebastian Bernauer <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jul 11, 2023

Pull request successfully merged into main.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title Added resource limits to all init containers and sidecar containers. [Merged by Bors] - Added resource limits to all init containers and sidecar containers. Jul 11, 2023
@bors bors bot closed this Jul 11, 2023
@bors bors bot deleted the iss/resource_limits branch July 11, 2023 09:00
This was referenced Jul 11, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbernauer sbernauer sbernauer approved these changes

@nightkr nightkr Awaiting requested review from nightkr

Assignees
No one assigned
Labels
release/23.7.0
Projects
Stackable Engineering
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@soenkeliebau @nightkr @sbernauer @lfrancke