Change in vdsm[master]: vm: pre-validate disks for snapshot

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: vm: pre-validate disks for snapshot
......................................................................

vm: pre-validate disks for snapshot

This patch divides the snapshot disks validation from the path
preparation in order to limit the rollback scenarios.

Change-Id: I4fb507c2f7268bb688af5fd187dcd033d0a068a2
Signed-off-by: Federico Simoncelli 
---
M vdsm/vm.py
1 file changed, 15 insertions(+), 13 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/29/20029/1

diff --git a/vdsm/vm.py b/vdsm/vm.py
index c85dc10..7945d5d 100644
--- a/vdsm/vm.py
+++ b/vdsm/vm.py
@@ -3618,11 +3618,11 @@
                 vmDrive = self._findDriveByUUIDs(baseDrv)
             except LookupError:
                 # The volume we want to snapshot doesn't exist
-                _rollbackDrives(newDrives)
                 self.log.error("The base volume doesn't exist: %s", baseDrv)
                 return errCode['snapshotErr']

if vmDrive.hasVolumeLeases:
+                self.log.error("Disk %s has volume leases", vmDrive.name)
                 return errCode['noimpl']

vmDevName = vmDrive.name
@@ -3632,23 +3632,25 @@
             newDrives[vmDevName]["name"] = vmDevName
             newDrives[vmDevName]["format"] = "cow"

-            try:
-                newDrives[vmDevName]["path"] = \
-                    self.cif.prepareVolumePath(newDrives[vmDevName])
-            except Exception:
-                _rollbackDrives(newDrives)
-                self.log.error("Unable to prepare the volume path "
-                               "for the disk: %s", vmDevName, exc_info=True)
-                return errCode['snapshotErr']
-
-            snapelem = _diskSnapshot(vmDevName, newDrives[vmDevName]["path"])
-            disks.appendChild(snapelem)
-
         # If all the drives are the current ones, return success
         if len(newDrives) == 0:
             self.log.debug("All the drives are already in use, success")
             return {'status': doneCode}

+        for vmDevName in newDrives.keys():
+            try:
+                newDrives[vmDevName]["path"] = \
+                    self.cif.prepareVolumePath(newDrives[vmDevName])
+            except Exception:
+                self.log.error("Unable to prepare the volume path "
+                               "for the disk: %s", vmDevName, exc_info=True)
+                # Trying to rollback (teardown) all drives
+                _rollbackDrives(newDrives)
+                return errCode['snapshotErr']
+
+            snapelem = _diskSnapshot(vmDevName, newDrives[vmDevName]["path"])
+            disks.appendChild(snapelem)
+
         snap.appendChild(disks)

snapFlags = (libvirt.VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT |

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: pre-validate disks for snapshot
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4790/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4866/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3981/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: pre-validate disks for snapshot
......................................................................

Patch Set 2: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4884/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3999/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4809/ : FAILURE

Ayal Baron has posted comments on this change.

Change subject: vm: pre-validate disks for snapshot
......................................................................

Patch Set 2: Code-Review+2

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: pre-validate disks for snapshot
......................................................................

Patch Set 3: Verified+1

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: pre-validate disks for snapshot
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4938/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4052/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4862/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: pre-validate disks for snapshot
......................................................................

Patch Set 4: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4461/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5262/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5340/ : SUCCESS

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: pre-validate disks for snapshot
......................................................................

Patch Set 4: Verified+1

In the past month I worked very much on live snapshots issues and I haven't had a problem with this one.

Liron Ar has posted comments on this change.

Change subject: vm: pre-validate disks for snapshot
......................................................................

Patch Set 4:

(2 comments)

....................................................
File vdsm/vm.py
Line 3704:                 return errCode['snapshotErr']
Line 3705: 
Line 3706:             if vmDrive.hasVolumeLeases:
Line 3707:                 self.log.error("Disk %s has volume leases", vmDrive.name)
Line 3708:                 return errCode['noimpl']
consider changing also the error code
Line 3709: 
Line 3710:             if vmDrive.transientDisk:
Line 3711:                 self.log.error("Disk %s is a transient disk", vmDrive.name)
Line 3712:                 return errCode['transientErr']

Line 3730:             except Exception:
Line 3731:                 self.log.error("Unable to prepare the volume path "
Line 3732:                                "for the disk: %s", vmDevName, exc_info=True)
Line 3733:                 # Trying to rollback (teardown) all drives
Line 3734:                 _rollbackDrives(newDrives)
should be done only for the drives that we already did the operation for instead of all of the drives.
Line 3735:                 return errCode['snapshotErr']
Line 3736: 
Line 3737:             snapelem = _diskSnapshot(vmDevName, newDrives[vmDevName]["path"])
Line 3738:             disks.appendChild(snapelem)

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: pre-validate disks for snapshot
......................................................................

Patch Set 4:

(1 comment)

....................................................
File vdsm/vm.py
Line 3730:             except Exception:
Line 3731:                 self.log.error("Unable to prepare the volume path "
Line 3732:                                "for the disk: %s", vmDevName, exc_info=True)
Line 3733:                 # Trying to rollback (teardown) all drives
Line 3734:                 _rollbackDrives(newDrives)
it doesn't make much of a difference, but ok, let's not be cheap with variables
Line 3735:                 return errCode['snapshotErr']
Line 3736: 
Line 3737:             snapelem = _diskSnapshot(vmDevName, newDrives[vmDevName]["path"])
Line 3738:             disks.appendChild(snapelem)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: pre-validate disks for snapshot
......................................................................

Patch Set 5:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4598/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5398/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5477/ : SUCCESS

Liron Ar has posted comments on this change.

Change subject: vm: pre-validate disks for snapshot
......................................................................

Patch Set 5:

(1 comment)

....................................................
File vdsm/vm.py
Line 3712:         for vmDevName, vmDevice in newDrives.iteritems():
Line 3713:             # Adding the device before requesting to prepare it as we want
Line 3714:             # to be sure to teardown it down even when prepareVolumePath
Line 3715:             # failed for some unknown issue that left the volume active.
Line 3716:             preparedDrives.append(vmDevice)
that's not what _rollbackDrives expects, it uses both the vmDevice and the name.
Line 3717:             try:
Line 3718:                 newDrives[vmDevName]["path"] = \
Line 3719:                     self.cif.prepareVolumePath(newDrives[vmDevName])
Line 3720:             except Exception:

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: pre-validate disks for snapshot
......................................................................

Patch Set 6: Verified+1

(1 comment)

Verified (with http://gerrit.ovirt.org/24867 ): successful live snapshot and failed prepareVolumePath (rollback).

http://gerrit.ovirt.org/#/c/20029/6/vdsm/vm.py
File vdsm/vm.py:

Line 3966:                 return errCode['snapshotErr']
Line 3967: 
Line 3968:             if vmDrive.hasVolumeLeases:
Line 3969:                 return errCode['noimpl']
Line 3970: 
Transient disk check has been split to:

http://gerrit.ovirt.org/24867
Line 3971:             vmDevName = vmDrive.name
Line 3972: 
Line 3973:             newDrives[vmDevName] = tgetDrv.copy()
Line 3974:             newDrives[vmDevName]["poolID"] = vmDrive.poolID

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: pre-validate disks for snapshot
......................................................................

Patch Set 6:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6451/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7235/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7353/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: pre-validate disks for snapshot
......................................................................

Patch Set 7:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6455/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7239/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7357/ : SUCCESS

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: pre-validate disks for snapshot
......................................................................

Patch Set 7: Code-Review+2

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: vm: pre-validate disks for snapshot
......................................................................

vm: pre-validate disks for snapshot

This patch divides the snapshot disks validation from the path
preparation in order to limit the rollback scenarios.

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1065886
Change-Id: I4fb507c2f7268bb688af5fd187dcd033d0a068a2
Signed-off-by: Federico Simoncelli 
Reviewed-on: http://gerrit.ovirt.org/20029
Reviewed-by: Dan Kenigsberg 
---
M vdsm/vm.py
1 file changed, 15 insertions(+), 9 deletions(-)

Approvals:
  Federico Simoncelli: Verified
  Dan Kenigsberg: Looks good to me, approved