-
Notifications
You must be signed in to change notification settings - Fork 132
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
ETCD-695: Add job parallelism to recurrent backups #1381
base: master
Are you sure you want to change the base?
Conversation
@Elbehery: This pull request references ETCD-695 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target either version "4.19." or "openshift-4.19.", but it targets "4.18" instead. 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 openshift-eng/jira-lifecycle-plugin repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Elbehery 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 |
8c463b6
to
efbaa09
Compare
fd87df7
to
e684567
Compare
e684567
to
a7e1fb0
Compare
@Elbehery: This pull request references ETCD-695 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target either version "4.19." or "openshift-4.19.", but it targets "4.18" instead. 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 openshift-eng/jira-lifecycle-plugin repository. |
/label tide/merge-method-squash |
Tested with this PR atop of Backup CR used during testing
notes that Upon applying the CR above,
The preceding
and
Below is the
The previous job contains two main containers
Note: this is the only master node where To summarize, this approach works but as you can see, the Looking for your consensus over an approach to merge Happy New Year 🥳 🥳 🥳 🥳 cc @JoelSpeed @jmhbnz @deads2k @wking @rhuss @vrutkovs @tjungblu @sjenning @csrwng @cuppett |
@JoelSpeed FYI ^^ |
@Elbehery: 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-sigs/prow repository. I understand the commands that are listed here. |
I guess I didn't fully understand the flow here, did not expect to see a cronjob to create EtcdBackup which then triggers the actual backups, I thought that would be a controller of itself and that we only had one Job involved here. Anyway, is there any particular reason why the pruning has to be done prior to the
How would you make the argument that it is more flexible? Which parts are more flexible? Is this flexibility required, and, is it worth having to implement and maintain a completely new backup system, vs just re-using/extending the existing system and having 1 path?
The intention in the future would be to extend this API and create a specific area to specify what kind of storage is being used, and assume not to just assume based on a magic keyname. I believe we discussed making this something like
|
So this is the current implementation, as I explained in #1381 (comment)
I am not the one who can answer this, I believe @tjungblu and @hasbro17 are the best to answer However, iirc, the I can also assume that since the we have a The templates used for creating the CronJob and the Job resources are below
As the whole code of the For instance, if we were now moving the
Indeed, if we were to move forward with this approach, then we would make the API changes 👍🏽 |
resolves https://issues.redhat.com/browse/ETCD-695
/hold