-
Notifications
You must be signed in to change notification settings - Fork 70
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
Changed backup suite to be more generic & changed mssql app to mysql #565
Conversation
mperetzred
commented
Feb 6, 2022
- Changed backup suite to be more generic so it could be used later with other CSI drivers, different backup spec, etc.
- Changed mssql app to mysql because mssql fails with restic as it uses deploymentconfig (restore gets stuck on InProgress; known bug).
9aa8079
to
368542e
Compare
/retest |
/test |
/test 4.8-operator-e2e |
/test 4.7-operator-e2e |
@mperetzred: The
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test 4.7-operator-e2e |
/test 4.8-operator-e2e |
port: 3306 | ||
selector: | ||
name: mysql | ||
- apiVersion: v1 |
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.
the PVC was already defined above. It should be removed from here.
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.
fixed
spec: | ||
accessModes: | ||
- ReadWriteOnce | ||
storageClassName: gp2 |
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.
The gp2 storageClass is specific to AWS, and the deployment fails due to pending pvc for clusters deployed outside aws. Maybe this could be left blank, so that we use the clsuter's default storage class?
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.
yes, agree. The MSSQL also had the storageclasses hardcoded, so I made it in a similar way, cause I didn't want to change the logic. I plan later to refactor the apps, but not as part of this PR cause it's already too big.
selector: | ||
name: mysql | ||
- apiVersion: v1 | ||
kind: PersistentVolumeClaim |
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.
Same comment, the pvc is declared twice
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.
fixed
9317125
to
9d8fcee
Compare
@dymurray @kaovilai if we change the e2e mssql to mysql should we also change the example app to mysql in the same review or a follow up review? https://github.com/openshift/oadp-operator/blob/master/docs/examples/stateful.md |
Could do in a follow up.. or not touch it at all. The problem we've been seeing with mssql in e2e environment doesn't appear to be resolved by the switch to mysql as we are still reliant on provisioning pvcs. |
@tiger my vote would be to change the example app as the app does not consistently come up properly most times.. probably works 50% of the time at best outside of e2e. Let's get it down in a follow up patch would be my vote. @mperetzred |
/hold for #568 |
9b721a9
to
942b523
Compare
5d2a5ff
to
6054ad9
Compare
@mperetzred just in case you didn't see.. #565 merged. So you are unblocked :) Thank you for your patience. |
/unhold |
6054ad9
to
ba035fb
Compare
a59b9ce
to
d6265b6
Compare
d6265b6
to
a384532
Compare
@mperetzred: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@mperetzred: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
closing up this PR, will break it into few PRs as there is too much code changes here |