-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Create Instance from backup on another Zone (DRaaS use case) #11560
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11560 +/- ##
============================================
+ Coverage 17.36% 17.39% +0.02%
- Complexity 15237 15277 +40
============================================
Files 5888 5889 +1
Lines 525741 525906 +165
Branches 64164 64186 +22
============================================
+ Hits 91274 91458 +184
+ Misses 424167 424140 -27
- Partials 10300 10308 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ed UpdateBackupRepositoryCmd api.
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
} else { | ||
clusterId = getClusterIdFromRootVolume(vm); |
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.
fyi - hostId won't be set for create Instance from Backup. But we can get the clusterId from the root volume which is already created.
mount += " -o " + mountOptions; | ||
} | ||
|
||
int exitValue = Script.runSimpleBashScriptForExitValue(mount, mountTimeout, false); |
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.
fiy - runSimpleBashScript doesn't return any exception. just a return value. So no need to keep it inside try-catch. So, checking the return value instead.
} catch (Exception e) { | ||
throw new CloudRuntimeException(String.format("Failed to unmount backup directory: %s", backupDirectory), e); | ||
String umountCmd = String.format(UMOUNT_COMMAND, backupDirectory); | ||
int exitValue = Script.runSimpleBashScriptForExitValue(umountCmd); |
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.
fyi - same here. runSimpleBashScript doesn't throw any exception.
String error_msg = String.format("Failed to create Instance [%s] from backup [%s] due to: %s.", vm.getInstanceName(), backupDetailsInMessage, result.second()); | ||
logger.error(error_msg); | ||
processRestoreBackupToVMFailure(vm, backup, eventId); | ||
throw new CloudRuntimeException(error_msg); |
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.
fyi - this is done to pass on the error passed by the provider.
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.
overall looking good, can you please clarify me with few questions I've raised @abh1sar thank you
...java/org/apache/cloudstack/api/command/user/backup/repository/UpdateBackupRepositoryCmd.java
Outdated
Show resolved
Hide resolved
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
Outdated
Show resolved
Hide resolved
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java
Outdated
Show resolved
Hide resolved
@abh1sar also can you please check the failures in simulator CI run |
Thanks @harikrishna-patnala. I have fixed the issue and addressed other comments. |
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.
code LGTM
@blueorangutan package |
1 similar comment
@blueorangutan package |
@abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 14912 |
@blueorangutan test |
@abh1sar a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
[SF] Trillian test result (tid-14275)
|
Description
In continuation of #10140 , this PR adds the functionality that a new Instance can be created on a different Zone from which the backup was taken.
Doc PR: apache/cloudstack-documentation#556
DRaaS Enabled
should be checked while creating the backup repository if it is to be used for creating instance in other zones.(The operator should ensure that the backup repository is accessible in other the Zones as well.)
Other changes done:
Throw relevant errors to the end user. i.e, if backup repository is not accessible or the backup files are not present or corrupted.
Added a timeout to mount operation so that restore fails early if backup repository is not accessible. Current behaviour is that mount will block indefinitely until libvirt times out the command after 1 hour.
Changes to decrease the RTO: Earlier, the new Instance had to be started (to create volumes), stopped, restored and started again. The start-stop cycle takes a long time. The whole process took around 200s on my local env with 15 GB of volume size.
I have added a new param VirtualMachineProfile.Param.ReturnAfterVolumePrepare which creates the volume but doesn't start the VM in orchestrateStart. So one Stop and one Start is avoided during the Instance creation. The whole process finished within 60s with this change.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?