-
Notifications
You must be signed in to change notification settings - Fork 69
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
Datamover CRD design #597
Datamover CRD design #597
Conversation
Skipping CI for Draft Pull Request. |
|
||
This design supports adding the data mover feature to the OADP operator and facilitates integrating various vendor implemented data movers. | ||
|
||
![DataMover CRD](../images/datamovercrd.png) |
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.
Please update this image 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.
Should I create a PR to just merge the image first? I think the path is correct. WDYT?
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.
Looks fine to me @ https://github.com/savitharaghunathan/oadp-operator/blob/data_mover_enhancement/docs/design/datamover.md
GitHub PR .md previews don't show (new??) images.
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.
@kaovilai I think so.
docs/design/datamover.md
Outdated
spec: | ||
DataMoverClass: <DataMoverClass name> | ||
- type: <VolumeSnapshot|PVC> | ||
sourceClaimRef: |
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.
sourceRef: can we model the ref based on that go type and remove the type field in line 99?
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.
Updated. Please take a look and let me know if it looks good.
docs/design/datamover.md
Outdated
|
||
### Data Mover Backup | ||
|
||
Assuming that the `DataMover Enable` flag is set to true in the DPA config, when a velero backup is created, it triggers DataMover plugin to create the `DataMoverBackup` CR in the app namespace. The plugin looks up for the PVCs in the user namespace mentioned in the velero backup and creates a `DataMoverBackup` CR for every PVC in that namespace. |
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.
Is this just a regular velero BackupItemAction plugin that creates the DataMoverBackup CR without modifying the PVC being acted on?
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.
There may be a possibility of a race condition here -- the thing we just created may not immediately be available in a Get call. I know we've had this issue in the past with ImageStreamTags, but since this hasn't been an issue for the CSI plugin, maybe it's fine here too.
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.
Thanks for the review, Scott. Clarified that the plugin is a BackupItemAction plugin. Should I capture the race condition information anywhere?
@savitharaghunathan: all tests passed! 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. |
* Adding initial datamover design * Typo in reviewers * Add summary & user stories * Fixing nits * Adding intial feedback * Adding feedback * nit: changing the selector field name * Adding review comments * Adding plugin clarification * updating image to reflect the latest design * Reflect recent velero csi plugin changes
…p (fix azure flakes) (#717) * revert to b64e96e * bump probe to 10s * mimic velero cli loglevel options (#692) use logrus ParseLevel directly * Fix OADP 526 (#704) * fix OADP 526 * add ignore err comments * fix docs link for build from src (#706) * Datamover CRD design (#597) * Adding initial datamover design * Typo in reviewers * Add summary & user stories * Fixing nits * Adding intial feedback * Adding feedback * nit: changing the selector field name * Adding review comments * Adding plugin clarification * updating image to reflect the latest design * Reflect recent velero csi plugin changes * use a full vm for arm builds in travis (#713) * OADP-535 allow for nullable resource allocations (#711) * OADP-535 allow for nullable resource allocations * Add missing nullables * Include nullable on additionalProperties Co-authored-by: Shubham Pampattiwar <shubhampampattiwar7@gmail.com> Co-authored-by: Savitha Raghunathan <sraghuna@redhat.com> Co-authored-by: Jason Montleon <jmontleo@redhat.com> Co-authored-by: Dylan Murray <dymurray@redhat.com>
Adding Data Mover CRD design