Change in vdsm[master]: BZ#855924 fix VM config dictionary

Peter V. Saveliev has uploaded a new change for review.

Change subject: BZ#855924 fix VM config dictionary
......................................................................

BZ#855924 fix VM config dictionary

Fix vm.conf dictionary when migrating from 3.1 to 3.0.
The issue is that 3.0 defines cdrom and floppy as dedicated
dictionary keys, while 3.1 places them among other devices.
So we're to discover target VDSM version and change vm.conf
to comply the 3.0 format.

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=855924
Signed-off-by: Peter V. Saveliev <peet(a)redhat.com&gt;
Change-Id: I33e957f8daac4266d9e19cc008cf830bd3959521
---
M vdsm/vm.py
1 file changed, 17 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/78/8378/1

diff --git a/vdsm/vm.py b/vdsm/vm.py
index 742e9fd..350bc66 100644
--- a/vdsm/vm.py
+++ b/vdsm/vm.py
@@ -90,6 +90,7 @@
         self.log = vm.log
         self._vm = vm
         self._dst = dst
+        self._dst_version = ""
         self._mode = mode
         self._method = method
         self._dstparams = dstparams
@@ -126,6 +127,8 @@
         self.log.debug('Destination server is: ' + serverAddress)
         try:
             self.log.debug('Initiating connection with destination')
+            self._dst_version = str(max(
+               
self.destServer.getVdsCapabilities()['info']['supportedRHEVMs']))
             status = self.destServer.getVmStats(self._vm.id)
             if not status['status']['code']:
                 self.log.error("Machine already exists on the destination")
@@ -219,6 +222,20 @@
             MigrationSourceThread._ongoingMigrations.acquire()
             try:
                 self.log.debug("migration semaphore acquired")
+                # patch VM config for targets < 3.1
+                if self._dst_version < '3.1':
+                    for i in ('cdrom', 'floppy'):
+                        try:
+                            # add cdrom and floppy as paths
+                            self._vm.conf[i] = [x['path'] for x in \
+                                self._vm.conf['devices'] if x['device']
== i][0]
+                            # remove them from 'devices' list
+                            [self._vm.conf['devices'].pop(
+                                self._vm.conf['devices'].index(z)) \
+                                for z in [x for x in \
+                                self._vm.conf['devices'] if x['device']
== i]]
+                        except:
+                            pass
                 if not mstate:
                     self._vm.conf['_migrationParams'] = {'dst':
self._dst,
                                 'mode': self._mode, 'method':
self._method,


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I33e957f8daac4266d9e19cc008cf830bd3959521
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Peter V. Saveliev <peet(a)redhat.com&gt;

Change in vdsm[master]: [WIP] BZ#855924 fix VM config dictionary

Peter V. Saveliev has abandoned this change.

Change subject: [WIP] BZ#855924 fix VM config dictionary
......................................................................


Patch Set 10: Abandoned

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I33e957f8daac4266d9e19cc008cf830bd3959521
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;

Peter V. Saveliev has posted comments on this change.

Change subject: BZ#855924 fix VM config dictionary
......................................................................


Patch Set 8: (2 inline comments)

....................................................
File vdsm/vm.py
Line 132:                 self._dst_version = StrictVersion(str(max(
Line 133:                    
self.destServer.getVersionInfo()['info']['supportedRHEVMs']
Line 134:                     )))
Line 135:             except:
Line 136:                 pass
if the target has no such verb, the default version is StrictVersion('3.0')
Line 137:             status = self.destServer.getVmStats(self._vm.id)
Line 138:             if not status['status']['code']:
Line 139:                 self.log.error("Machine already exists on the
destination")
Line 140:                 self.status = errCode['exist']


Line 227:             MigrationSourceThread._ongoingMigrations.acquire()
Line 228:             try:
Line 229:                 self.log.debug("migration semaphore acquired")
Line 230:                 # patch VM config for targets < 3.1
Line 231:                 if self._dst_version < StrictVersion('3.1'):
here we can define compatibility levels. E.g.:


if self._dst_version < StrictVersion('3.1'):
   …
elif self._dst_version < StrictVersion('3.2'):
   …
Line 232:                     for i in ('cdrom', 'floppy'):
Line 233:                         try:
Line 234:                             # add cdrom and floppy as paths
Line 235:                             self._vm.conf[i] = [x['path'] for x in \


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I33e957f8daac4266d9e19cc008cf830bd3959521
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;

Change in vdsm[master]: [WIP] BZ#855924 fix VM config dictionary

Peter V. Saveliev has posted comments on this change.

Change subject: [WIP] BZ#855924 fix VM config dictionary
......................................................................


Patch Set 6:

Dan, we have a clear choice. Or to discover a version somehow, or *always* keep cdrom and
floppy as strings in the main dictionary, and then — why they were moved to drives at all?
were there some reason of extending the functionality? if so, we should keep it and
discover the version. If there is no new functionality, I propose to remove cdrom and
floppy from drives and always use the rhev-3.0 vm config — who knows, what else can be
broken next time.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I33e957f8daac4266d9e19cc008cf830bd3959521
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;

Change in vdsm[master]: [WIP] BZ#855924 fix VM config dictionary

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: [WIP] BZ#855924 fix VM config dictionary
......................................................................


Patch Set 6: I would prefer that you didn't submit this

(1 inline comment)

I think that a much cleaner approach would be to maintain backward compatibility in the
migrationCreate API itself.

Have you considered carrying "cdrom" and "floppy" everywhere? rhev-3.1
could know how to handle a case where a cdrom appears both as "cdrom" and as
devices[cdrom].

....................................................
File vdsm/vm.py
Line 127:         self.log.debug('Destination server is: ' + serverAddress)
Line 128:         try:
Line 129:             self.log.debug('Initiating connection with destination')
Line 130:             self._dst_version = str(max(
Line 131:                
self.destServer.getVersionInfo()['info']['supportedRHEVMs']))
has this ever been tested?
What would happen if getVersionInfo() is missing from the destination?

you assume that string comparison would always work for versions. I do not think this is
safe.

I would like to evade this, but maybe it is time to add capability flags to
getCapabilities in order to check if "devices" is supported.
Line 132:             status = self.destServer.getVmStats(self._vm.id)
Line 133:             if not status['status']['code']:
Line 134:                 self.log.error("Machine already exists on the
destination")
Line 135:                 self.status = errCode['exist']


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I33e957f8daac4266d9e19cc008cf830bd3959521
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;

Change in vdsm[master]: [WIP] BZ#855924 fix VM config dictionary

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: [WIP] BZ#855924 fix VM config dictionary
......................................................................


Patch Set 6: Verified; Looks good to me, but someone else must approve

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I33e957f8daac4266d9e19cc008cf830bd3959521
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;

Change in vdsm[master]: [WIP] BZ#855924 fix VM config dictionary

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: [WIP] BZ#855924 fix VM config dictionary
......................................................................


Patch Set 5: (1 inline comment)

....................................................
Commit Message
Line 10: The issue is that 3.0 defines cdrom and floppy as dedicated
Line 11: dictionary keys, while 3.1 places them among other devices.
Line 12: 
Line 13: Right now the only way to discover the VDSM version is
Line 14: getVdsCapabilities() call, but it takes too long time and
This should be no longer mentioned as you're now using getVersionInfo
Line 15: causes timeouts, so we can not use it. A hotfix is to assume
Line 16: the compatibility level always 3.0. Later it can be fixed
Line 17: with a dedicated getVersion() (or like that) call
Line 18: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I33e957f8daac4266d9e19cc008cf830bd3959521
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;

Change in vdsm[master]: [WIP] BZ#855924 fix VM config dictionary

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: [WIP] BZ#855924 fix VM config dictionary
......................................................................


Patch Set 5: Verified; I would prefer that you didn't submit this

Migration 3.1 -> 3.0 worked. CD-ROM was still attached and system still running.

The commit message has still to be updated.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I33e957f8daac4266d9e19cc008cf830bd3959521
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;

Change in vdsm[master]: [WIP] BZ#855924 fix VM config dictionary

Peter V. Saveliev has posted comments on this change.

Change subject: [WIP] BZ#855924 fix VM config dictionary
......................................................................


Patch Set 4: (1 inline comment)

....................................................
File vdsm/vm.py
Line 227:                         try:
Line 228:                             # add cdrom and floppy as paths
Line 229:                             self._vm.conf[i] = [x['path'] for x in \
Line 230:                                 self._vm.conf['drives'] if
x['device'] == i][0]
Line 231:                             # remove them from 'devices' list
1. a  typo
2. I do not completely understand you. What do you mean, that noone puts?
'cdrom/floppy' *are* in 'dirves' list. Someone puts them to it.
Line 232:                             [self._vm.conf['drives'].pop(
Line 233:                                 self._vm.conf['drives'].index(z)) \
Line 234:                                 for z in [x for x in \
Line 235:                                 self._vm.conf['drives'] if
x['device'] == i]]


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I33e957f8daac4266d9e19cc008cf830bd3959521
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;

Change in vdsm[master]: [WIP] BZ#855924 fix VM config dictionary

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: [WIP] BZ#855924 fix VM config dictionary
......................................................................


Patch Set 4: (1 inline comment)

....................................................
File vdsm/vm.py
Line 227:                         try:
Line 228:                             # add cdrom and floppy as paths
Line 229:                             self._vm.conf[i] = [x['path'] for x in \
Line 230:                                 self._vm.conf['drives'] if
x['device'] == i][0]
Line 231:                             # remove them from 'devices' list
1. you wrote 'devices' in comment
2. no one put cdrom/floppy in 'drives' list
Line 232:                             [self._vm.conf['drives'].pop(
Line 233:                                 self._vm.conf['drives'].index(z)) \
Line 234:                                 for z in [x for x in \
Line 235:                                 self._vm.conf['drives'] if
x['device'] == i]]


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I33e957f8daac4266d9e19cc008cf830bd3959521
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;

Change in vdsm[master]: [WIP] BZ#855924 fix VM config dictionary

Peter V. Saveliev has posted comments on this change.

Change subject: [WIP] BZ#855924 fix VM config dictionary
......................................................................


Patch Set 4: (2 inline comments)

....................................................
File vdsm/vm.py
Line 93:         self._method = method
Line 94:         self._dstparams = dstparams
Line 95:         # for now have this hardcoded to hot-fix 3.1 -> 3.0 issue
Line 96:         # later look for getVersion() call or like that
Line 97:         self._dst_version = "3.0"
That was exactly I was thinking about.
Line 98:         self._machineParams = {}
Line 99:         self._downtime = kwargs.get('downtime') or \
Line 100:                             config.get('vars',
'migration_downtime')
Line 101:         self.status = {'status': {'code': 0, 'message':
'Migration in process'}, 'progress': 0}


Line 227:                         try:
Line 228:                             # add cdrom and floppy as paths
Line 229:                             self._vm.conf[i] = [x['path'] for x in \
Line 230:                                 self._vm.conf['drives'] if
x['device'] == i][0]
Line 231:                             # remove them from 'devices' list
from 'dirves' list, not 'devices', surely.
Line 232:                             [self._vm.conf['drives'].pop(
Line 233:                                 self._vm.conf['drives'].index(z)) \
Line 234:                                 for z in [x for x in \
Line 235:                                 self._vm.conf['drives'] if
x['device'] == i]]


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I33e957f8daac4266d9e19cc008cf830bd3959521
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;

Change in vdsm[master]: [WIP] BZ#855924 fix VM config dictionary

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: [WIP] BZ#855924 fix VM config dictionary
......................................................................


Patch Set 4: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File vdsm/vm.py
Line 93:         self._method = method
Line 94:         self._dstparams = dstparams
Line 95:         # for now have this hardcoded to hot-fix 3.1 -> 3.0 issue
Line 96:         # later look for getVersion() call or like that
Line 97:         self._dst_version = "3.0"
Actually, I think to implement getVersiopn is too simple and you can do it in separate
patch (before this one).
I hate this hard coded stuff
Line 98:         self._machineParams = {}
Line 99:         self._downtime = kwargs.get('downtime') or \
Line 100:                             config.get('vars',
'migration_downtime')
Line 101:         self.status = {'status': {'code': 0, 'message':
'Migration in process'}, 'progress': 0}


Line 227:                         try:
Line 228:                             # add cdrom and floppy as paths
Line 229:                             self._vm.conf[i] = [x['path'] for x in \
Line 230:                                 self._vm.conf['drives'] if
x['device'] == i][0]
Line 231:                             # remove them from 'devices' list
why? what if you will migrate VM back?
Line 232:                             [self._vm.conf['drives'].pop(
Line 233:                                 self._vm.conf['drives'].index(z)) \
Line 234:                                 for z in [x for x in \
Line 235:                                 self._vm.conf['drives'] if
x['device'] == i]]


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I33e957f8daac4266d9e19cc008cf830bd3959521
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Peter V. Saveliev <peet(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;