Skip to content

Bug/false positive success message vm start #5148

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.

Sign up for GitHub

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

Merged
yadvr merged 2 commits into from
Jun 27, 2021
Merged

Bug/false positive success message vm start #5148

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
432f6e6
add throws statement during the retry process
Jun 23, 2021
a785d7c
Update engine/orchestration/src/main/java/org/apache/cloudstack/engin…
DK101010 Jun 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ...tion/src/main/java/org/apache/cloudstack/engine/cloud/entity/api/VMEntityManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,8 @@ public void deployVirtualMachine(String reservationId, VMEntityVO vmEntityVO, St
}

_itMgr.start(vm.getUuid(), params, plan, null);
} else {
throw ex;
Copy link
Copy Markdown
Member

@GabrielBrascher GabrielBrascher Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend presenting more details to the user/admin. A message that gives some context (where/how) regarding this exception could help the caller to understand what is going on.

For instance:
throw new ResourceUnavailableException(String.format("message containing details of issue for VM %s", vm));

OR

throw new ResourceUnavailableException(String.format("message containing details of issue for VM %s due to exception: %s", vm, ex));

Copy link
Copy Markdown
Contributor Author

@DK101010 DK101010 Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GabrielBrascher Maybe I'am wrong but I think is a little bit to much info, the vm is not the problem in that case. Also throws the _itMgr.start method a couple exception, so that we can not say here when the vm the problem is.

Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GabrielBrascher, I tend to agree with @DK101010 but would it make sense to cature the exception in a CloudStackException saying that we are "not retrying as no host is given" ? and would that satisfy your requirements?

Copy link
Copy Markdown
Member

@GabrielBrascher GabrielBrascher Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland @DK101010, I agree that my suggestion might be a bit too much info for this context.
As long as we don't throw raw exceptions at the user's face I am good with it 🙂.

}
}
} else {
Expand Down