Bring back vm.suspend during deleting VM snapshot#4029
Conversation
|
@blueorangutan package |
|
@andrijapanicsb a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
@blueorangutan package |
|
@andrijapanicsb a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
@andrijapanicsb thanks for opening this PR up. From your testing were you able to reproduce the corruption using a "VM Snapshot" also (create followed by a delete)? Or just a volume-based snapshot? I ask because the delete of a VM snapshot still does not have the pause/resume. |
|
I'm testing with the latest master first, trying to corrupt the VM (delete VM snap during high IO load inside a VM) - but can't manage to do that (hm, my cent7 template is EXT4, not the gentle XFS - perhaps that's the case) - tired of testing... Anyway, it's safe to suspend the VM before the snap restore, so we shall proceed with it - so I will verify the VM is suspended (this PR) and move from there. |
|
Packaging result: ✖centos7 ✖debian. JID-1164 |
|
Packaging result: ✔centos7 ✔debian. JID-1165 |
|
@Slair1 I don't see VM being paused when snapshots (VM snapshot) is being deleted... Sorry to ask, did you ever observe/confirm code works on 4.11.3? @Slair1 If I don't see this solved in next 2-3h, I'm afraid I will have to move it to 4.15, as we aim to cut 4.14 RC1 on Friday and this issue you've raised way too late (in the #3969). |
|
@blueorangutan package |
|
@andrijapanicsb a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos7 ✔debian. JID-1170 |
|
@weizhouapache this PR is no good. I'm moving this PR to 4.15, unless you miraculously come (wouldn't be the first time :) with proper code and add that vm.suspend whereever the snap IS actually deleted...) |
|
@andrijapanicsb you're correct, we also need the suspend and resume when a VM Snapshot is deleted also - that is not currently done. This PR is partially correct though as is. Snapshots are also deleted after a Backup to Secondary Storage operation is finished. @ggoodrich-ipp perhaps you can push your PR and reference the new PR here? @ggoodrich-ipp has a more complete PR with the pause/resume in both applicable locations. |
|
Guys, we are touching here, as I understood – VM snapshots?
And keep in mind that we need to craft 4.14 RC1 on Friday – not sure we have any time for new stuff, unless a VERY quick fix.
From: Slair1 <notifications@github.com>
Sent: Wednesday, 15 April 2020 20:29
To: apache/cloudstack <cloudstack@noreply.github.com>
Cc: Andrija Panic <andrija.panic@shapeblue.com>; Mention <mention@noreply.github.com>
Subject: Re: [apache/cloudstack] Bring back vm.suspend during deleting VM snapshot (#4029)
@andrijapanicsb<https://github.com/andrijapanicsb> you're correct, we also need the suspend and resume when a VM Snapshot is deleted also - that is not currently done. This PR is partially correct though as is. Snapshots are also deleted after a Backup to Secondary Storage operation is finished. @ggoodrich-ipp<https://github.com/ggoodrich-ipp> perhaps you can push your PR and reference the new PR here?
@ggoodrich-ipp<https://github.com/ggoodrich-ipp> has a more complete PR with the pause/resume in both applicable locations.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#4029 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AK5EN3LZK5SBB6QOTACSKK3RMX4GNANCNFSM4MH7LMKQ>.
Andrija Panic
Cloud Architect
s: +44 20 3603 0540 | m: +381 69 272 3690
e: andrija.panic@shapeblue.com | w: www.shapeblue.com | t: @shapeblue
a: 3 London Bridge Street, 3rd floor, News Building, London SE1 9SGUK
Shape Blue Ltd is a company incorporated in England & Wales. ShapeBlue Services India LLP is a company incorporated in India and is operated under license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is a company incorporated in Brasil and is operated under license from Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered by The Republic of South Africa and is traded under license from Shape Blue Ltd. ShapeBlue is a registered trademark.This email and any attachments to it may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Shape Blue Ltd or related companies. If you are not the intended recipient of this email, you must neither take any action based upon its contents, nor copy or show it to anyone. Please contact the sender if you believe you have received this email in error.
Find out more about ShapeBlue and our range of CloudStack related services:
IaaS Cloud Design & Build | CSForge - rapid IaaS deployment framework
CloudStack Consulting | CloudStack Software Engineering
CloudStack Infrastructure Support | CloudStack Bootcamp Training Courses
|
| try { | ||
| s_logger.info(String.format("Suspending VM '%s' to delete snapshot,", vm.getName())); | ||
| vm.suspend(); | ||
| } catch (final LibvirtException e) { | ||
| s_logger.error("Failed to suspend the VM", e); | ||
| throw e; |
There was a problem hiding this comment.
same comment as in #4033: first we suspend unconditionaly, and than, we just assume it need resuming without checking if it was running in the first place. Are there situations we should guard against more diligently? (cc @weizhouapache @rhtyd )
There was a problem hiding this comment.
@DaanHoogland I see on line 1007, we are checking that domain is running? Did I miss anything?
There was a problem hiding this comment.
yes, we are, i am confusing PRs. this one actually doesn't do anything in the delete vmshapshot scenario.
yadvr
left a comment
yadvr
left a comment
There was a problem hiding this comment.
LGTM, see Daan's comment though.
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
code (and state check) is actually good on this one. Merge!
…n, th… (#4032) These changes are related to PR #3194, but include suspending/resuming the VM when doing a VM snapshot as well, when deleting a VM snapshot, as it is performing the same operations via Libvirt. Also, there was an issue with the UI/localization changes in the prior PR, as that PR was altering the Volume snapshot behavior, but was altering the VM snapshot wording. Both have been altered in this PR. Issuing this in response to the work happening in PR #4029.
7 participants
Original commit, merged to 4.11.3, got mysteriously missing when merging branch 4.11 to 4.12 on the very same day that original PR was merged - so bringing back the same code to 4.13.1 and 4.14(master atm)
I'm talking about the PR #3194 (the commit itself is there, but NOT the content/code - possibly a wrongly resolved merge conflicts etc)
/cc @weizhouapache (you approved the original PR) and @rhtyd (you also approved the original PR)
UPDATE: the VM snapshots deletion is NOT handled via this code (this code is used in some other scenario - was missing so there it is now again)
Pausing a VM BEFORE the VM snap is deleted is handled now by #4033. Possible improvements (for 4.15) can be taken from #4032 (additional checks and such)