-
Notifications
You must be signed in to change notification settings - Fork 200
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
feat(local-snapshot-restore, velero) : support to restore local snapshot to different namespace using velero #1575
Conversation
Signed-off-by: mayank <[email protected]>
Signed-off-by: mayank <[email protected]>
Signed-off-by: mayank <[email protected]>
…estore Signed-off-by: mayank <[email protected]>
Signed-off-by: mayank <[email protected]>
Signed-off-by: mayank <[email protected]>
Signed-off-by: mayank <[email protected]>
Signed-off-by: mayank <[email protected]>
@prateekpandey14 Can you verify https://github.com/openebs/maya/pull/1575/files#diff-861ad0cc410461be20924c447e61bc86R281. I am creating volume through |
Signed-off-by: mayank <[email protected]>
@@ -62,7 +64,7 @@ func (rOps *restoreAPIOps) create() (interface{}, error) { | |||
} | |||
|
|||
// namespace is expected | |||
if len(strings.TrimSpace(restore.Namespace)) == 0 { | |||
if !restore.Spec.Local && len(strings.TrimSpace(restore.Namespace)) == 0 { |
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 we throw error if Local and NameSpace is NOT 0?
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.
In case of local We are not using namespace. We are ignoring it so skipped the check.
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.
Few things like adding comments, testcases are required. Overall flow looks to be good, however one more round of review need to be done.
APIs are changed however backup compatibility is maintained.
This involves good amount of changes with respect to approach as well. Now, storage classes with 'WatiForFirstConsumer' may not work. So, its better to get into the early cycles of testing.
Approving this in order to get into early cycles of testing.
@mynktl - can you verify that Though there is not a lot of information obtained by looking at mayactl volume list for cstor volumes, it should not result in - inability to fetch the jiva volumes list, as it is the only way to know the status of jiva volumes. If there are issue listing the cStor volumes - we can raise another PR to deprecate including cStor volumes in the |
In addition to @vishnuitta comments, this PR also will need to document the following:
|
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.
Changes are good.
A few follow-up items are mentioned as review comments, once they are verified and backlogs are created, the merge can proceed.
return nil, errors.Wrapf(err, "Failed to get restore volume details") | ||
} | ||
} else { | ||
return cvol, nil |
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 possible in case of cloud backup?
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, In case of cloud backups we are creating volume through PVC from velero-plugin. So this will return the existing volume. Added comment.
return cvol, nil | ||
} | ||
|
||
return vOps.Create() |
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 supposed to be for local backup?
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.
It serves for local and cloud backup. As of now we are creating volume through PVC from velero-plugin in case of remote restore So this will not execute for remote backup. #1575 (comment)
|
Signed-off-by: mayank <[email protected]>
For documentation I've covered it in openebs/velero-plugin#53. |
@kmova @vishnuitta PTAL. I've addressed the review comments. |
Ref: openebs/openebs#2844
Changes:
cstor-clone-
prefix.Sample output:
In above output, first two PVs are created from
pvc-109d7f73-699c-11ea-9326-42010a9a0064
PV.PVC output:
CStorVolume output:
For documentation:
error taking snapshot of volume: rpc error: code = Unknown desc = Failed to send backup request: Error calling REST api: Status error{Bad Request}