Change in vdsm[master]: virt: Restore BC for block type live snapshots

Adam Litke has uploaded a new change for review.

Change subject: virt: Restore BC for block type live snapshots
......................................................................

virt: Restore BC for block type live snapshots

Commit 02741ce introduced a regression in older hosts running libvirt <
1.2.2 where libvirt rejects the type='block' and source/dev combination.
Eric Blake suggested that this could be worked around by supplying the
old format and new format in the domainshapshot XML document.  This
union would continue to work as before on old hosts but would pick up
the correct block snapshot behavior on newer versions.  Basically, we
still express the disk type but we repeat the newPath inside both a dev
and file attribute.

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1115126
Change-Id: I85290f17842ada6c04695bf45ffd1b1108c63d55
Signed-off-by: Adam Litke <alitke(a)redhat.com&gt;
---
M vdsm/virt/vm.py
1 file changed, 7 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/67/29567/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 6e937fc..7cca83e 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -4022,11 +4022,14 @@
 
             disk = XMLElement('disk', name=vmDev, snapshot='external',
                               type=sourceType)
+            # Libvirt versions before 1.2.2 do not understand 'type' and treat
+            # all snapshots as if they are type='file'.  In order to ensure
+            # proper handling of block snapshots in modern libvirt versions,
+            # we specify type='block' and dev=path for block volumes but we
+            # always speficy the file=path for backwards compatibility.
+            args = {'type': sourceType, 'file': newPath}
             if sourceType == 'block':
-                args = {'dev': newPath}
-            elif sourceType == 'file':
-                args = {'file': newPath}
-            args['type'] = sourceType
+                args['dev'] = newPath
             disk.appendChildWithArgs('source', **args)
             return disk
 


-- 
To view, visit http://gerrit.ovirt.org/29567
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I85290f17842ada6c04695bf45ffd1b1108c63d55
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: virt: Restore BC for block type live snapshots
......................................................................


Patch Set 2:

Build Successful 

http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged/1571/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5556/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3714/ : SUCCESS

-- 
To view, visit http://gerrit.ovirt.org/29567
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I85290f17842ada6c04695bf45ffd1b1108c63d55
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Elad Ben Aharon <eladba1990(a)gmail.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: virt: Restore BC for block type live snapshots
......................................................................


virt: Restore BC for block type live snapshots

Commit 02741ce introduced a regression in older hosts running libvirt <
1.2.2 where libvirt rejects the type='block' and source/dev combination.
Eric Blake suggested that this could be worked around by supplying the
old format and new format in the domainshapshot XML document.  This
union would continue to work as before on old hosts but would pick up
the correct block snapshot behavior on newer versions.  Basically, we
still express the disk type but we repeat the newPath inside both a dev
and file attribute.

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1115126
Change-Id: I85290f17842ada6c04695bf45ffd1b1108c63d55
Signed-off-by: Adam Litke <alitke(a)redhat.com&gt;
Reviewed-on: http://gerrit.ovirt.org/29567
Reviewed-by: Francesco Romani <fromani(a)redhat.com&gt;
Reviewed-by: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com&gt;
---
M vdsm/virt/vm.py
1 file changed, 7 insertions(+), 4 deletions(-)

Approvals:
  Adam Litke: Verified
  Federico Simoncelli: Looks good to me, but someone else must approve
  Dan Kenigsberg: Looks good to me, approved
  Francesco Romani: Looks good to me, but someone else must approve



-- 
To view, visit http://gerrit.ovirt.org/29567
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I85290f17842ada6c04695bf45ffd1b1108c63d55
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Elad Ben Aharon <eladba1990(a)gmail.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: virt: Restore BC for block type live snapshots
......................................................................


Patch Set 1: Code-Review+2

raising score; hopefully we can drop this kludge soon.

-- 
To view, visit http://gerrit.ovirt.org/29567
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I85290f17842ada6c04695bf45ffd1b1108c63d55
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Elad Ben Aharon <eladba1990(a)gmail.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: virt: Restore BC for block type live snapshots
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

It seems ok (only +1 because it's a Friday midnight review).

http://gerrit.ovirt.org/#/c/29567/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 4028:             # we specify type='block' and dev=path for block volumes
but we
Line 4029:             # always speficy the file=path for backwards compatibility.
Line 4030:             args = {'type': sourceType, 'file': newPath}
Line 4031:             if sourceType == 'block':
Line 4032:                 args['dev'] = newPath
>  Unrelated to this patch: sourceType looks redundant, it is already
> specifie 
What constructor? Anyway we can also use utils.isBlockDevice on newPath
(not sure I like to break the encapsulation though, I'd leave it as it is).
Line 4033:             disk.appendChildWithArgs('source', **args)
Line 4034:             return disk
Line 4035: 
Line 4036:         def _normSnapDriveParams(drive):


-- 
To view, visit http://gerrit.ovirt.org/29567
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I85290f17842ada6c04695bf45ffd1b1108c63d55
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Elad Ben Aharon <eladba1990(a)gmail.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Adam Litke has posted comments on this change.

Change subject: virt: Restore BC for block type live snapshots
......................................................................


Patch Set 1: Verified+1

Tested on f20 and RHEL6 and got the correct behavior in both systems.

-- 
To view, visit http://gerrit.ovirt.org/29567
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I85290f17842ada6c04695bf45ffd1b1108c63d55
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Elad Ben Aharon <eladba1990(a)gmail.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Francesco Romani has posted comments on this change.

Change subject: virt: Restore BC for block type live snapshots
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

Looks OK.

http://gerrit.ovirt.org/#/c/29567/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 4028:             # we specify type='block' and dev=path for block volumes
but we
Line 4029:             # always speficy the file=path for backwards compatibility.
Line 4030:             args = {'type': sourceType, 'file': newPath}
Line 4031:             if sourceType == 'block':
Line 4032:                 args['dev'] = newPath
Unrelated to this patch: sourceType looks redundant, it is already specified in the
constructor.
Line 4033:             disk.appendChildWithArgs('source', **args)
Line 4034:             return disk
Line 4035: 
Line 4036:         def _normSnapDriveParams(drive):


-- 
To view, visit http://gerrit.ovirt.org/29567
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I85290f17842ada6c04695bf45ffd1b1108c63d55
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Elad Ben Aharon <eladba1990(a)gmail.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: virt: Restore BC for block type live snapshots
......................................................................


Patch Set 1:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/9958/ : SUCCESS

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

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1158/ : There was an
infra issue, please contact infra(a)ovirt.org

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/10900/ : SUCCESS

-- 
To view, visit http://gerrit.ovirt.org/29567
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I85290f17842ada6c04695bf45ffd1b1108c63d55
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Elad Ben Aharon <eladba1990(a)gmail.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No