Add tech preview `features` struct to DPA CRD, add `enableDataMover` flag to DPA `features` , data-mover controller deployment and data-mover csi plugin #727

shubham-pampattiwar · 2022-06-03T18:00:28Z

This PR does the following:

adds a new struct spec to DPA CRD called features for OADP tech preview features, this PR adds the first tech preview feature - a boolean flag called enableDataMover.
adds a new reconcile function ReconcileDataMoverController which enables the deployment of volume-snapshot-mover based on the enableDataMover flag.
updates the csi plugin image based on the existence/value of enableDataMover flag
adds unsupportedOverrides support for data-mover controller
adds unit tests
adds 2 new CRDs - VolumeSnapshotBackup and VolumeSnapshotRestore
fixes Enable DM through OADP migtools/volume-snapshot-mover#69

controllers/dpa_controller.go

kaovilai · 2022-06-03T19:13:10Z

/woof

openshift-ci · 2022-06-03T19:13:12Z

@kaovilai:

In response to this:

/woof

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.

shubham-pampattiwar · 2022-06-03T19:17:42Z

/retest

controllers/datamover.go

dymurray

This should be configurable from the DPA spec, a user needs to explicitly enable this feature for 1.1 since we are tech-preview.

kaovilai · 2022-06-03T20:53:18Z

MySQL application NoDefaultBackupStorageLocation was removed from master. Rebase to test without this entry.

…s for dm controller

…onality

shubham-pampattiwar · 2022-06-05T20:50:47Z

This should be configurable from the DPA spec, a user needs to explicitly enable this feature for 1.1 since we are tech-preview.

@dymurray Added a new spec enableDataMover which needs to be specifically set to true by the user in order to enable the data-mover functionality

shubham-pampattiwar · 2022-06-05T22:32:41Z

/retest

config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml

kaovilai

Please apply all bundle changes in config/ then use make bundle to generate updated bundle. Reach out if you have any questions.

dymurray

Overall changes look sane to me. Couple questions

dymurray · 2022-06-07T13:05:05Z

api/v1alpha1/oadp_types.go

-
+	// EnableDataMover is used to specify whether you want to deploy the volume snapshot mover controller and a modified csi datamover plugin
+	// +optional
+	EnableDataMover bool `json:"enableDataMover,omitempty"`


I was wondering, is it worth adding a new type TechPreviewConfiguration that hides this behind a pointer? This would allow us to change this API as we go to GA if upstream velero makes this totally native.

Then if we have other tech preview features in the future we can always use this field.

Using UnsupportedOverrides config would not require CRD changes. This maybe better approach as we discuss final api.

@dymurray I am not sure if TechPreviewConfigutarion is the right naming here, I am unsure. Can we not rely on the docs to let the users know that this is a tech-preview feature ? Or is it the right way to do from the API side as you suggested ?

yeah but this will be a supported field, just maybe something we don't keep around in future versions. So I don't think unsupportedOverrides is the correct place for it

I don't know what the right naming is... but my thought is as we add new features down the line we may not want to keep adding new bools to the top level of the spec.

What about Features and documentation covers what is tech-preview and what isn't? This way it goes:

features: enableDataMover: true

and if data mover becomes a first class citizen down the line we just rip it out of the feature set?

Cool, I like features more and its combination with the docs. Will be similar to what Velero does

@dymurray updated the PR, PTAL, Thanks !

controllers/datamover.go

pkg/credentials/credentials.go

pkg/common/common.go

kaovilai

please add a few comments regarding what each images does.

api/v1alpha1/oadp_types.go

dymurray

Tested PR and worked for me, agree with tiger on the unsupported override field for CSI plugin.

Please also rename the data mover deployment to be volume-snapshot-mover

Missing CRD

dymurray · 2022-06-07T20:16:38Z

Please add missing CRDs for volume snapshot mover as well

shubham-pampattiwar · 2022-06-07T20:54:37Z

@dymurray @kaovilai Added the CRDs - VolumeSnapshotBackup and VolumeSnapshotRestore.

savitharaghunathan · 2022-06-07T22:50:28Z

/test 4.10-operator-e2e-gcp
/test 4.10-operator-e2e-azure

kaovilai · 2022-06-08T00:11:50Z

Could be another PR.
Add example annotation to csv by adding example objects for following.

config/samples/volumesnapshotbackup.yaml
config/samples/volumesnapshotrestore.yaml

Add above to

config/samples/kustomization.yaml

Would resolve operator-sdk warnings as well as provide default template in openshift web console yaml view when creating these resources.

WARN[0000] Warning: Value datamover.oadp.openshift.io/v1alpha1, Kind=VolumeSnapshotBackup: provided API should have an example annotation 
WARN[0000] Warning: Value datamover.oadp.openshift.io/v1alpha1, Kind=VolumeSnapshotRestore: provided API should have an example annotation

openshift-ci · 2022-06-08T00:16:58Z

@shubham-pampattiwar: 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.

eemcmullan

Tested these changes and was able to complete a backup and restore successfully using data mover.

savitharaghunathan · 2022-06-08T11:46:04Z

backup/restore workflow worked with these changes!
/pony party

openshift-ci · 2022-06-08T11:46:08Z

@savitharaghunathan:

In response to this:

backup/restore workflow worked with these changes!
/pony party

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.

shubham-pampattiwar · 2022-06-08T12:01:18Z

Thank you everyone for the PR review and feedback. Going ahead with the merge.

shubham-pampattiwar requested review from savitharaghunathan, dymurray and eemcmullan June 3, 2022 18:00

shubham-pampattiwar self-assigned this Jun 3, 2022

openshift-ci bot requested review from alaypatel07 and JaydipGabani June 3, 2022 18:01

shubham-pampattiwar removed request for alaypatel07 and JaydipGabani June 3, 2022 18:02

savitharaghunathan reviewed Jun 3, 2022

View reviewed changes

controllers/dpa_controller.go Show resolved Hide resolved

dymurray reviewed Jun 3, 2022

View reviewed changes

controllers/datamover.go Outdated Show resolved Hide resolved

dymurray reviewed Jun 3, 2022

View reviewed changes

kaovilai added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2022

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2022

shubham-pampattiwar requested review from savitharaghunathan and dymurray June 5, 2022 20:07

shubham-pampattiwar changed the title ~~Add datamover controller deployment~~ Add enableDataMover flag to DPA, data-mover controller deployment and data-mover csi plugin Jun 5, 2022

shubham-pampattiwar added 6 commits June 5, 2022 16:48

add datamover controller deployment

0a3d874

revert to org images in csv

72bcb2a

add enableDataMover boolean flag on DPA CR

a284a86

update csi plugin image based on dm flag and add unsupported override…

32dbf1f

…s for dm controller

add labels to DM deployment and unit tests

33d431d

make users specifically specify enabledataMover as true for DM functi…

2e3753f

…onality

shubham-pampattiwar force-pushed the add-datamover-controller branch from ce33ca3 to 2e3753f Compare June 5, 2022 20:48

revert default dpa sample file

9a3272f

kaovilai reviewed Jun 6, 2022

View reviewed changes

config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml Outdated Show resolved Hide resolved

kaovilai requested changes Jun 6, 2022

View reviewed changes

dymurray reviewed Jun 7, 2022

View reviewed changes

update DPA API and run make bundle

d80d5a7

shubham-pampattiwar changed the title ~~Add enableDataMover flag to DPA, data-mover controller deployment and data-mover csi plugin~~ Add tech preview features struct to DPA CRD, add enableDataMover flag to DPA features , data-mover controller deployment and data-mover csi plugin Jun 7, 2022

kaovilai reviewed Jun 7, 2022

View reviewed changes

pkg/credentials/credentials.go Outdated Show resolved Hide resolved

kaovilai reviewed Jun 7, 2022

View reviewed changes

pkg/common/common.go Outdated Show resolved Hide resolved

kaovilai requested changes Jun 7, 2022

View reviewed changes

address feedback and add events perms

32a273c

shubham-pampattiwar requested review from kaovilai and dymurray June 7, 2022 19:21

kaovilai reviewed Jun 7, 2022

View reviewed changes

api/v1alpha1/oadp_types.go Show resolved Hide resolved

dymurray previously approved these changes Jun 7, 2022

View reviewed changes

add CRDs - VolumeSnapshotBackup and VolumeSnapshotRestore

80d2ba9

shubham-pampattiwar requested review from kaovilai and dymurray June 7, 2022 20:54

kaovilai approved these changes Jun 8, 2022

View reviewed changes

savitharaghunathan approved these changes Jun 8, 2022

View reviewed changes

eemcmullan approved these changes Jun 8, 2022

View reviewed changes

shubham-pampattiwar merged commit 3515262 into openshift:master Jun 8, 2022

kaovilai mentioned this pull request Aug 24, 2022

Add rebase command to prow kubernetes/test-infra#12595

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tech preview `features` struct to DPA CRD, add `enableDataMover` flag to DPA `features` , data-mover controller deployment and data-mover csi plugin #727

Add tech preview `features` struct to DPA CRD, add `enableDataMover` flag to DPA `features` , data-mover controller deployment and data-mover csi plugin #727

shubham-pampattiwar commented Jun 3, 2022 •

edited

Loading

kaovilai commented Jun 3, 2022

openshift-ci bot commented Jun 3, 2022

shubham-pampattiwar commented Jun 3, 2022

dymurray left a comment

kaovilai commented Jun 3, 2022

shubham-pampattiwar commented Jun 5, 2022

shubham-pampattiwar commented Jun 5, 2022

kaovilai left a comment

dymurray left a comment

dymurray Jun 7, 2022

kaovilai Jun 7, 2022

shubham-pampattiwar Jun 7, 2022

dymurray Jun 7, 2022

dymurray Jun 7, 2022

shubham-pampattiwar Jun 7, 2022

shubham-pampattiwar Jun 7, 2022

kaovilai left a comment

dymurray left a comment

dymurray commented Jun 7, 2022

shubham-pampattiwar commented Jun 7, 2022

savitharaghunathan commented Jun 7, 2022

kaovilai commented Jun 8, 2022 •

edited

Loading

openshift-ci bot commented Jun 8, 2022

eemcmullan left a comment

savitharaghunathan commented Jun 8, 2022

openshift-ci bot commented Jun 8, 2022

shubham-pampattiwar commented Jun 8, 2022

Add tech preview features struct to DPA CRD, add enableDataMover flag to DPA features , data-mover controller deployment and data-mover csi plugin #727

Add tech preview features struct to DPA CRD, add enableDataMover flag to DPA features , data-mover controller deployment and data-mover csi plugin #727

Conversation

shubham-pampattiwar commented Jun 3, 2022 • edited Loading

kaovilai commented Jun 3, 2022

openshift-ci bot commented Jun 3, 2022

shubham-pampattiwar commented Jun 3, 2022

dymurray left a comment

Choose a reason for hiding this comment

kaovilai commented Jun 3, 2022

shubham-pampattiwar commented Jun 5, 2022

shubham-pampattiwar commented Jun 5, 2022

kaovilai left a comment

Choose a reason for hiding this comment

dymurray left a comment

Choose a reason for hiding this comment

dymurray Jun 7, 2022

Choose a reason for hiding this comment

kaovilai Jun 7, 2022

Choose a reason for hiding this comment

shubham-pampattiwar Jun 7, 2022

Choose a reason for hiding this comment

dymurray Jun 7, 2022

Choose a reason for hiding this comment

dymurray Jun 7, 2022

Choose a reason for hiding this comment

shubham-pampattiwar Jun 7, 2022

Choose a reason for hiding this comment

shubham-pampattiwar Jun 7, 2022

Choose a reason for hiding this comment

kaovilai left a comment

Choose a reason for hiding this comment

dymurray left a comment

Choose a reason for hiding this comment

dymurray commented Jun 7, 2022

shubham-pampattiwar commented Jun 7, 2022

savitharaghunathan commented Jun 7, 2022

kaovilai commented Jun 8, 2022 • edited Loading

openshift-ci bot commented Jun 8, 2022

eemcmullan left a comment

Choose a reason for hiding this comment

savitharaghunathan commented Jun 8, 2022

openshift-ci bot commented Jun 8, 2022

shubham-pampattiwar commented Jun 8, 2022

Add tech preview `features` struct to DPA CRD, add `enableDataMover` flag to DPA `features` , data-mover controller deployment and data-mover csi plugin #727

Add tech preview `features` struct to DPA CRD, add `enableDataMover` flag to DPA `features` , data-mover controller deployment and data-mover csi plugin #727

shubham-pampattiwar commented Jun 3, 2022 •

edited

Loading

kaovilai commented Jun 8, 2022 •

edited

Loading