-
Notifications
You must be signed in to change notification settings - Fork 22
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
Use emptydir for db pg_dump #2661
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use emptyDir on all the environments or some don't have enough local storage ?
If we can, I would move the volume def in base/.
Besides that, I think we should add a resources.requests.ephemeral-storage
matching the expected size of the db dumb. (If it's available. It's a beta k8s feature, not sure if exposed on the openshift versions we have)
2202315
to
33f8f57
Compare
path: "/spec/jobTemplate/spec/template/spec/containers/0/resources" | ||
value: {"requests": {"memory":"1Gi", "cpu": "1", "ephemeral-storage": "20Gi"}, "limits": {"memory":"1Gi", "cpu": "1", "ephemeral-storage": "20Gi"}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path: "/spec/jobTemplate/spec/template/spec/containers/0/resources" | |
value: {"requests": {"memory":"1Gi", "cpu": "1", "ephemeral-storage": "20Gi"}, "limits": {"memory":"1Gi", "cpu": "1", "ephemeral-storage": "20Gi"}} | |
path: "/spec/jobTemplate/spec/template/spec/containers/0/resources/requests/ephemeral-storage" | |
value: "20Gi" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path: "/spec/jobTemplate/spec/template/spec/containers/0/resources" | |
value: {"requests": {"memory":"1Gi", "cpu": "1", "ephemeral-storage": "20Gi"}, "limits": {"memory":"1Gi", "cpu": "1", "ephemeral-storage": "20Gi"}} | |
- op: add | |
path: "/spec/jobTemplate/spec/template/spec/containers/0/resources/limits/ephemeral-storage" | |
value: "20Gi" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would only add the ephemeral in the patch (same for other environment).
Or maybe we can add 20Gi as the base and only override test at 1Gi ?
Side note: Do we want to add the limit on ephemeral-storage ? The idea is to be alerted immediately if db dump starts to go over 20Gi ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not sure if my second sugg applies cleanly on top on the other, but the idea : two add ops specifying the value only at the correct path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, i have made the changes as you suggested.
The limits is something requested by cluster team so fit with the namespace limits.
about we being alerted , the exceed of those limit would cause failure in system, so we should be notified.
33f8f57
to
419c76d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone read the json patch docs and I think we need replace
rather than add
since we have a base value now ?
For the rest it's
/lgtm
Signed-off-by: Harshad Reddy Nalla <[email protected]>
419c76d
to
f61292c
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: codificat, VannTen The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/close |
Use emptydir for db pg_dump
Signed-off-by: Harshad Reddy Nalla [email protected]
Related Issues and Dependencies
Related-to: thoth-station/graph-backup-job#242