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 options for resource requests for vault-agent #275

Merged

Conversation

dhumphries-sainsburys
Copy link
Contributor

Currently the vault-agent takes a resource for requests and limits combined but in usage we see that startup usage is high and ongoing usage is low so we are faced with either large amounts of wasted resources across a cluster or a risk of OOM kills on startup. This PR will just allow requests and limits to be defined independently of each other

@dhumphries-sainsburys dhumphries-sainsburys requested a review from a team as a code owner December 14, 2023 13:24
@dhumphries-sainsburys dhumphries-sainsburys requested review from ramizpolic and removed request for a team December 14, 2023 13:24
@github-actions github-actions bot added the size/S Denotes a PR that changes 10-99 lines label Dec 14, 2023
Makefile Outdated Show resolved Hide resolved
pkg/webhook/config.go Outdated Show resolved Hide resolved
@ramizpolic
Copy link
Member

ramizpolic commented Dec 22, 2023

Hi @dhumphries-sainsburys, thanks for the contribution! I left a couple of notes to address and we can merge it afterwards. Please also include the docs for the new flags where appropriate.

Copy link
Member

@ramizpolic ramizpolic left a comment

Choose a reason for hiding this comment

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

Added a couple of notes, can merge after these are resolved. Thanks for the updates @dhumphries-sainsburys!

pkg/webhook/config.go Outdated Show resolved Hide resolved
pkg/webhook/config.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@dhumphries-sainsburys
Copy link
Contributor Author

@ramizpolic - Think all changes are made but github UI seems to be showing something different to the commits so not entirely sure

@ramizpolic
Copy link
Member

Linter seems to be complaining. If you could sign the commits and address the lint issue, we can merge this. Not sure why it's complaining though.

Signed-off-by: Daniel.Humphries <daniel.humphries@sainsburys.co.uk>
@dhumphries-sainsburys dhumphries-sainsburys force-pushed the add_agent_resource_requests branch from 187c582 to 1e8c11b Compare January 31, 2024 17:19
@dhumphries-sainsburys
Copy link
Contributor Author

Squashed the commits as well as signed. In regards to the linter i couldn't find a problem and the error previous looked more like a problem with the linter rather than what it was linting to me

Signed-off-by: Daniel.Humphries <daniel.humphries@sainsburys.co.uk>
Copy link
Member

@ramizpolic ramizpolic left a comment

Choose a reason for hiding this comment

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

great work @dhumphries-sainsburys, thanks for the contribution!

@ramizpolic ramizpolic merged commit 876f424 into bank-vaults:main Feb 2, 2024
21 checks passed
@dhumphries-sainsburys dhumphries-sainsburys deleted the add_agent_resource_requests branch February 5, 2024 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/artifacts size/S Denotes a PR that changes 10-99 lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants