-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CLOUDSTACK-8830 - [Vmware] VM snapshot fails for 12 min after instance creation #798
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1164,19 +1164,10 @@ public CreateVMSnapshotAnswer execute(VmwareHostService hostService, CreateVMSna | |
| try { | ||
| VmwareHypervisorHost hyperHost = hostService.getHyperHost(context, cmd); | ||
|
|
||
| // wait if there are already VM snapshot task running | ||
| ManagedObjectReference taskmgr = context.getServiceContent().getTaskManager(); | ||
| List<ManagedObjectReference> tasks = context.getVimClient().getDynamicProperty(taskmgr, "recentTask"); | ||
|
|
||
| for (ManagedObjectReference taskMor : tasks) { | ||
| TaskInfo info = (TaskInfo)(context.getVimClient().getDynamicProperty(taskMor, "info")); | ||
|
|
||
| if (info.getEntityName().equals(cmd.getVmName()) && info.getName().equalsIgnoreCase("CreateSnapshot_Task")) { | ||
| if (!(info.getState().equals(TaskInfoState.SUCCESS) || info.getState().equals(TaskInfoState.ERROR))) { | ||
| s_logger.debug("There is already a VM snapshot task running, wait for it"); | ||
| context.getVimClient().waitForTask(taskMor); | ||
| } | ||
| } | ||
| if(waitForRunningTaskOnVM(cmd.getVmName(), "CreateSnapshot_Task", context)== false){ | ||
| String msg = "vCenter task failed"; | ||
| s_logger.info(msg); | ||
| return new CreateVMSnapshotAnswer(cmd, false, msg); | ||
| } | ||
|
|
||
| vmMo = hyperHost.findVmOnHyperHost(vmName); | ||
|
|
@@ -1205,7 +1196,7 @@ public CreateVMSnapshotAnswer execute(VmwareHostService hostService, CreateVMSna | |
| } | ||
| } catch (Exception e) { | ||
| String msg = e.getMessage(); | ||
| s_logger.error("failed to create snapshot for vm:" + vmName + " due to " + msg); | ||
| s_logger.error("failed to create snapshot for vm:" + vmName + " due to " + msg, e); | ||
|
|
||
| try { | ||
| if (vmMo.getSnapshotMor(vmSnapshotName) != null) { | ||
|
|
@@ -1383,17 +1374,10 @@ public RevertToVMSnapshotAnswer execute(VmwareHostService hostService, RevertToV | |
| try { | ||
| VmwareHypervisorHost hyperHost = hostService.getHyperHost(context, cmd); | ||
|
|
||
| // wait if there are already VM revert task running | ||
| ManagedObjectReference taskmgr = context.getServiceContent().getTaskManager(); | ||
| List<ManagedObjectReference> tasks = context.getVimClient().getDynamicProperty(taskmgr, "recentTask"); | ||
|
|
||
| for (ManagedObjectReference taskMor : tasks) { | ||
| TaskInfo info = (TaskInfo)(context.getVimClient().getDynamicProperty(taskMor, "info")); | ||
|
|
||
| if (info.getEntityName().equals(cmd.getVmName()) && info.getName().equalsIgnoreCase("RevertToSnapshot_Task")) { | ||
| s_logger.debug("There is already a VM snapshot task running, wait for it"); | ||
| context.getVimClient().waitForTask(taskMor); | ||
| } | ||
| if(waitForRunningTaskOnVM(cmd.getVmName(), "RevertToSnapshot_Task", context)==false){ | ||
| String msg = "vCenter task failed"; | ||
| s_logger.info(msg); | ||
| return new RevertToVMSnapshotAnswer(cmd, false, msg); | ||
| } | ||
|
|
||
| HostMO hostMo = (HostMO)hyperHost; | ||
|
|
@@ -1439,7 +1423,7 @@ public RevertToVMSnapshotAnswer execute(VmwareHostService hostService, RevertToV | |
| } | ||
| } catch (Exception e) { | ||
| String msg = "revert vm " + vmName + " to snapshot " + snapshotName + " failed due to " + e.getMessage(); | ||
| s_logger.error(msg); | ||
| s_logger.error(msg, e); | ||
|
|
||
| return new RevertToVMSnapshotAnswer(cmd, false, msg); | ||
| } | ||
|
|
@@ -1464,4 +1448,55 @@ private String deleteDir(String dir) { | |
| private static String getVolumeRelativeDirInSecStroage(long volumeId) { | ||
| return "volumes/" + volumeId; | ||
| } | ||
|
|
||
| /** | ||
| * This method is to check if a given TaskInfo Object is valid( and has name and entity name assigned).It return true if TaskInfo Object is valid and false otherwise. | ||
| * | ||
| * @param TaskInfo | ||
| * info | ||
| * @return boolean(true or false) | ||
| **/ | ||
| private boolean isvalidTaskInfoObj(TaskInfo info){ | ||
| return !(info == null || info.getEntityName() == null || info.getName() == null); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please check |
||
| } | ||
|
|
||
| /** | ||
| * This method waits for tasks running on vm to complete | ||
| * | ||
| * @param vmName Name of the vm | ||
| * @param taskName Name of the task | ||
| * @param context Task context object | ||
| * | ||
| * @return boolean(true or false) True if it waited for tasks to finish and false when there are no tasks running on the Vm. | ||
| **/ | ||
|
|
||
| private boolean waitForRunningTaskOnVM(String vmName, String taskName, VmwareContext context) throws Exception { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @maneesha-p,
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rodrigo93 It returns the same value as method 'waitForTask' .
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Throwing |
||
| try { | ||
| ManagedObjectReference taskmgr = context.getServiceContent().getTaskManager(); | ||
| List<ManagedObjectReference> tasks = context.getVimClient().getDynamicProperty(taskmgr, "recentTask"); | ||
|
|
||
| for (ManagedObjectReference taskMor : tasks) { | ||
| TaskInfo info = (TaskInfo)(context.getVimClient().getDynamicProperty(taskMor, "info")); | ||
|
|
||
| if (!isvalidTaskInfoObj(info)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't this if's statements be merged into one? This would reduce the complexity of this part of the code.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean remove the “isvalidTaskInfoObj” and add that condition to the (if)?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, those if's statements could be merged into one if statement. This is just one suggestion to make a minor improvement in the complexity.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed you are only suggesting ;), no need to say that, when we review a PR we suggest changes to improve it, which is normal. I am going to ask again, Do you mean remove the “isvalidTaskInfoObj” and add that condition to the (if)? For instance, if you want to suggest something to reduce code complexity or the so called cyclomatic complexity, you should take a look at lines 1467 and 1468, there you have a lot of room for improvements. |
||
| continue; | ||
| } | ||
|
|
||
| if (info.getEntityName().equals(vmName) && info.getName().equalsIgnoreCase(taskName)) { | ||
| if (!(info.getState().equals(TaskInfoState.SUCCESS) || info.getState().equals(TaskInfoState.ERROR))) { | ||
| s_logger.debug("There is already a task: " + taskName + " running for VM: " + vmName + ", wait for it"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please wrap this statement in an |
||
| // wait if there is already similar VM task running | ||
| context.getVimClient().waitForTask(taskMor); | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } catch (Exception e) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are all checked and unchecked exceptions being caught here? |
||
| String msg = "Failed to check running task: " + taskName + " for vm: " + vmName + " due to " + e.getMessage(); | ||
| s_logger.error(msg, e); | ||
| throw new Exception(msg); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
| } | ||
| } | ||
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.
Why is this method necessary? It is a one line boolean result that is called from one place. It seems to obfuscate more than clarify.