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

Feature: Resource Limits for backstage container #27

Merged
merged 10 commits into from
Dec 15, 2022
Merged

Feature: Resource Limits for backstage container #27

merged 10 commits into from
Dec 15, 2022

Conversation

JoshuaJackson-jobvite
Copy link
Contributor

@JoshuaJackson-jobvite JoshuaJackson-jobvite commented Dec 12, 2022

Description of the change

Adds support for setting requests and limits on the backstage deployment such that folks can execute to their desired best practices on resource consolidation.

Existing or Associated Issue(s)

#26

Additional Information

Looked over my local developer deployment that has a number of added plugins and a few charts etc and what the resource limits were. Set memory fairly high comparatively but cpu should be fully in line.

Checklist

  • Chart version bumped in Chart.yaml according to semver.
  • Variables are documented in the values.yaml and added to the README.md. The helm-docs utility can be used to generate the necessary content. Use helm-docs --dry-run to preview the content.
  • JSON Schema generated.
  • List tests pass for Chart using the Chart Testing tool and the ct lint command.

Why do we need this change?
=======================
As per best practices we should enable/allow users to set both request
and limits on their execution/running of the backstage app to ensure
that it won't take up all resources for a node due to any potential
memory leaks or run away cpu processes

What effects does this change have?
=======================
@JoshuaJackson-jobvite JoshuaJackson-jobvite requested a review from a team as a code owner December 12, 2022 20:06
@JoshuaJackson-jobvite
Copy link
Contributor Author

JoshuaJackson-jobvite commented Dec 12, 2022

So I'm assuming a bit that #18 is just waiting on a touch of things and should update the docs automatically for this, but unclear what that might be waiting on. If we want to proceed with this I can update this and potentially that pr depending on desires.

@tumido
Copy link
Member

tumido commented Dec 13, 2022

So I'm assuming a bit that #18 is just waiting on a touch of things and should update the docs automatically for this, but unclear what that might be waiting on. If we want to proceed with this I can update this and potentially that pr depending on desires.

hey @JoshuaJackson-jobvite yeah, yesterday it was still missing approval from another maintainer and then it was waiting for me to get back to work today (since the culture in Backstage is to self-merge once approved for maintainers). It is merged now. 🙂

Can you please rebase and run pre-commit run -a?

Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

Hey! 👍 I like it, I would change the default value to be present in values file, otherwise lgtm (others may have opinion on the toYaml but I don't feel educated enough on that bit, deferring to them).

charts/backstage/values.yaml Outdated Show resolved Hide resolved
charts/backstage/templates/backstage-deployment.yaml Outdated Show resolved Hide resolved
@JoshuaJackson-jobvite JoshuaJackson-jobvite requested review from tumido and ChrisJBurns and removed request for tumido and ChrisJBurns December 14, 2022 18:11
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

charts/backstage/Chart.yaml Outdated Show resolved Hide resolved
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@tumido tumido merged commit be5b46d into backstage:main Dec 15, 2022
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.

3 participants