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

Added resources limits definition for wait-for-db container #2501

Merged
merged 10 commits into from
Jul 12, 2023

Conversation

Shelestov7
Copy link
Contributor

@Shelestov7 Shelestov7 commented Jul 11, 2023

What this PR does

Added 'resources limits' definition for wait-for-db container

Which issue(s) this PR fixes

I face a problem: when i install OnCall by Helm, my pods with oncall-engine and oncall-celery stuck on Init state, because they don't have enough resources to run.

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)
  • Documentation added (or pr:no public docs PR label added if not required)

@Shelestov7 Shelestov7 requested a review from a team July 11, 2023 15:13
@CLAassistant
Copy link

CLAassistant commented Jul 11, 2023

CLA assistant check
All committers have signed the CLA.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@joeyorlando joeyorlando left a comment

Choose a reason for hiding this comment

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

thanks for your contribution 🙂 left some minor comments. Once those are addressed, I can ✅

helm/oncall/templates/_helpers.tpl Outdated Show resolved Hide resolved
helm/oncall/templates/_helpers.tpl Outdated Show resolved Hide resolved
helm/oncall/tests/__snapshot__/wait_for_db_test.yaml.snap Outdated Show resolved Hide resolved
Copy link
Contributor

@joeyorlando joeyorlando left a comment

Choose a reason for hiding this comment

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

thanks for adding tests! one last minor suggestion w/ the indentation

Comment on lines 12 to 18
resources:
limits:
cpu: 100m
memory: 128Mi
requests:
cpu: 100m
memory: 128Mi
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resources:
limits:
cpu: 100m
memory: 128Mi
requests:
cpu: 100m
memory: 128Mi
resources:
limits:
cpu: 100m
memory: 128Mi
requests:
cpu: 100m
memory: 128Mi

Comment on lines 37 to 43
resources:
limits:
cpu: 100m
memory: 128Mi
requests:
cpu: 100m
memory: 128Mi
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resources:
limits:
cpu: 100m
memory: 128Mi
requests:
cpu: 100m
memory: 128Mi
resources:
limits:
cpu: 100m
memory: 128Mi
requests:
cpu: 100m
memory: 128Mi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIXED

Copy link
Contributor

@joeyorlando joeyorlando left a comment

Choose a reason for hiding this comment

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

you'll need to rerun the tests locally and update the wait_for_db_test.yaml.snap file to reflect the new changes. That should be the last thing 🙂

@@ -26,6 +34,13 @@ tests:
set:
database.type: postgresql
externalPostgresql.host: some-postgresql-host
resources:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resources:
init.resources:

actually this probably needs to be set to this. The tests should be failing atm because the wait_for_db_test.yaml.snap still shows:

resources: {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were right. Fixed it.

helm/oncall/tests/wait_for_db_test.yaml Outdated Show resolved Hide resolved
@joeyorlando joeyorlando added this pull request to the merge queue Jul 12, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 12, 2023
@Shelestov7
Copy link
Contributor Author

2 checks was Failed i have no idea why.

@joeyorlando joeyorlando added this pull request to the merge queue Jul 12, 2023
Merged via the queue into grafana:dev with commit 939590f Jul 12, 2023
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