Skip to content
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

CA-220505: Destroy stale VM record on destination pool on failed … #2783

Merged
merged 1 commit into from Oct 7, 2016

Conversation

sharady
Copy link
Contributor

@sharady sharady commented Sep 26, 2016

…SXM.

Cancelling cross-pool SXM ends up in destroying VDIs and stale VM record
on destination Pool.
After cancellation, If VM exists on source Pool then destroy the stale VM
record on destination Pool.

Signed-off-by: Sharad Yadav sharad.yadav@citrix.com

Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

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

This looks solid; my comments are minor.

@@ -966,6 +966,22 @@ let migrate_send' ~__context ~vm ~dest ~live ~vdi_map ~vif_map ~options =
debug "Mirror failed for VDI: %s" failed_vdi;
raise (Api_errors.Server_error(Api_errors.mirror_failed,[Ref.string_of vconf.vdi]))
end;

if (TaskHelper.is_cancelling ~__context) && not is_intra_pool then
Copy link
Contributor

@lindig lindig Sep 26, 2016

Choose a reason for hiding this comment

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

You are using parentheses for the TaskHelper.is_canceling function application but not for not. You don't need parentheses in either case (it's fine to write if f x && not y then .. else ..) but I think this should be consistent.

if (TaskHelper.is_cancelling ~__context) && not is_intra_pool then
begin match e with
| Storage_interface.Does_not_exist(_) ->
if (Db.is_valid_ref __context vm) then begin
Copy link
Contributor

Choose a reason for hiding this comment

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

You are using parentheses again here. They are not strictly needed and the convention is not to use them for simple if-expressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lindig Thanks for the review, I have removed the parentheses.

@jonludlam
Copy link
Contributor

We're matching some very specific conditions here - it seems odd that we only destroy the remote VM if we catch 'Storage_interface.Does_not_exist' - can you justify that?

@sharady
Copy link
Contributor Author

sharady commented Sep 29, 2016

@jonludlam I have seen this specific error Storage_interface.Does_not_exist in case of CA-220505. Where cancellation happens after VM metadata import is complete and VDI mirror is done. I have posted the trace for it on the ticket. Might need to discuss it further if any other cases are missing which i am also not sure. Thanks.

Cancelling cross-pool SXM ends up in destroying VDIs and stale VM record
on destination Pool.
On SXM failure, If VM exists on source Pool then destroy the stale VM
record on destination Pool.

Signed-off-by: Sharad Yadav <sharad.yadav@citrix.com>
@sharady
Copy link
Contributor Author

sharady commented Sep 29, 2016

@jonludlam Thanks for the suggestion, I have updated the PR accordingly. Please have a look :)

@sharady sharady changed the title CA-220505: Destroy stale VM record on destination pool on cancelling … CA-220505: Destroy stale VM record on destination pool on failed … Sep 29, 2016
@lindig
Copy link
Contributor

lindig commented Oct 5, 2016

Paging @jonludlam ;)

@jonludlam
Copy link
Contributor

Looks good now, thanks!

@jonludlam jonludlam merged commit cad6ac6 into xapi-project:master Oct 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants