Change in vdsm[master]: vm.py: State saving in hotunplugDisk.

Amit Aviram has uploaded a new change for review.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................

vm.py: State saving in hotunplugDisk.

VM's state was saved before detatching a disk in hotunplugDisk.

Saving vm's state should be performed only AFTER vdsm is actually detaching
the disk, otherwise the disk will remain in xmlDesc and be calculated in
the vm's hash.
This causes a bug in the engine's VM monitoring: the engine
gets the same hash as before the disk was unplugged, thus does not
update the disk's status.

Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Bug-Url:https://bugzilla.redhat.com/1206696
Bug-Url: https://bugzilla.redhat.com/1044466
Signed-off-by: Amit Aviram <aaviram(a)redhat.com&gt;
---
M vdsm/virt/vm.py
1 file changed, 1 insertion(+), 2 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/77/45077/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 0eff29a..cb354f0 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -2631,12 +2631,11 @@
                 diskDev = dev
                 break
 
-        self.saveState()
-
         hooks.before_disk_hotunplug(driveXml, self.conf,
                                     params=drive.custom)
         try:
             self._dom.detachDevice(driveXml)
+            self.saveState()
         except libvirt.libvirtError as e:
             self.log.exception("Hotunplug failed")
             if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;

automation(a)ovirt.org has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 1:

* Update tracker::#1044466::OK
* Check Bug-Url::OK
* Check Public Bug::#1044466::OK, public bug
* Check Product::#1044466::OK, Correct product oVirt
* Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Amit Aviram has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 1: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Francesco Romani has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 1: Code-Review-1

(4 comments)

questions inside, -1 for visibility.

Please also briefly state how you verified this patch.

https://gerrit.ovirt.org/#/c/45077/1//COMMIT_MSG
Commit Message:

Line 9: VM's state was saved before detatching a disk in hotunplugDisk.
Line 10: 
Line 11: Saving vm's state should be performed only AFTER vdsm is actually detaching
Line 12: the disk, otherwise the disk will remain in xmlDesc and be calculated in
Line 13: the vm's hash.
true. Another issue is that the xml refresh is well hidden inside saveState, and this
coude would be clearer if the XML update would be made manifest. But maybe this is out of
scope.
Line 14: This causes a bug in the engine's VM monitoring: the engine
Line 15: gets the same hash as before the disk was unplugged, thus does not
Line 16: update the disk's status.
Line 17: 


https://gerrit.ovirt.org/#/c/45077/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2644:             # Restore disk device in vm's conf and _devices
Line 2645:             if diskDev:
Line 2646:                 with self._confLock:
Line 2647:                     self.conf['devices'].append(diskDev)
Line 2648:             self.saveState()
if we move the saveState() out of the try block - as we should! this becomes unnecessary
and should go away as well.
Line 2649:             return response.error('hotunplugDisk', e.message)
Line 2650:         else:
Line 2651:             hooks.after_disk_hotunplug(driveXml, self.conf,
Line 2652:                                        params=drive.custom)


Line 2646:                 with self._confLock:
Line 2647:                     self.conf['devices'].append(diskDev)
Line 2648:             self.saveState()
Line 2649:             return response.error('hotunplugDisk', e.message)
Line 2650:         else:
I'd rather saveState here...
Line 2651:             hooks.after_disk_hotunplug(driveXml, self.conf,
Line 2652:                                        params=drive.custom)
Line 2653:             self._cleanupDrives(drive)
Line 2654: 


Line 2649:             return response.error('hotunplugDisk', e.message)
Line 2650:         else:
Line 2651:             hooks.after_disk_hotunplug(driveXml, self.conf,
Line 2652:                                        params=drive.custom)
Line 2653:             self._cleanupDrives(drive)
...or even here (disclaimer: need to check what cleanupDrives does).
Line 2654: 
Line 2655:         return {'status': doneCode, 'vmList': self.status()}
Line 2656: 
Line 2657:     def _readPauseCode(self):


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 2:

* Update tracker::#1044466::OK
* Check Bug-Url::OK
* Check Public Bug::#1044466::OK, public bug
* Check Product::#1044466::OK, Correct product oVirt
* Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Amit Aviram has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 2: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Francesco Romani has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 2:

please describe briefly how you verified this patch.

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 2: Code-Review-1

(2 comments)

https://gerrit.ovirt.org/#/c/45077/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2628
Line 2629
Line 2630
Line 2631
Line 2632
This should be done only if the disk was removed. There is no reason to remove a drive
before it was actually removed.


Line 2648:         else:
Line 2649:             hooks.after_disk_hotunplug(driveXml, self.conf,
Line 2650:                                        params=drive.custom)
Line 2651:             self._cleanupDrives(drive)
Line 2652:             self.saveState()
If vdsm crash after  invoking detachDevice, the unplugged disk will still be in the
recovery file, and the vm object after vdsm starts will contain the removed device. This
is the reason we save state *before* operations in many places.

Please check that we update the vm object drives based on the real vm state, so stale
drives are removed in this case.
Line 2653: 
Line 2654:         return {'status': doneCode, 'vmList': self.status()}
Line 2655: 
Line 2656:     def _readPauseCode(self):


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/45077/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2648:         else:
Line 2649:             hooks.after_disk_hotunplug(driveXml, self.conf,
Line 2650:                                        params=drive.custom)
Line 2651:             self._cleanupDrives(drive)
Line 2652:             self.saveState()
>  If vdsm crash after  invoking detachDevice, the unplugged disk will
> still b 
I think this should work like this:

- Ask libvirt to remove the device, fail on errors (no state changed)
- Remove the drive from vm devices disk and from conf (should never fail)
- Save the new state (may fail, we can do nothing about it)
Line 2653: 
Line 2654:         return {'status': doneCode, 'vmList': self.status()}
Line 2655: 
Line 2656:     def _readPauseCode(self):


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Francesco Romani has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/45077/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2648:         else:
Line 2649:             hooks.after_disk_hotunplug(driveXml, self.conf,
Line 2650:                                        params=drive.custom)
Line 2651:             self._cleanupDrives(drive)
Line 2652:             self.saveState()
>  I think this should work like this: 
Good catch, I overlooked
the crash after detach issue. My fault. It is partially because the real issue being
addressed here is that we want to update xml description and devices hash, which is done
by update domain descriptor, which in turn is buried deep saveState.

We should deentangle the two actions here, which involves a fair amount of refactoring if
we want to avoid useless duplicated work (explicit update _and_ implicit update inside
saveState)

BTW, the best solution with respect the saveState handling is what Nir described.
Line 2653: 
Line 2654:         return {'status': doneCode, 'vmList': self.status()}
Line 2655: 
Line 2656:     def _readPauseCode(self):


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 3:

* Update tracker::#1044466::OK
* Check Bug-Url::OK
* Check Public Bug::#1044466::OK, public bug
* Check Product::#1044466::OK, Correct product oVirt
* Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Amit Aviram has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 3: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Liron Aravot has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 3:

(2 comments)

https://gerrit.ovirt.org/#/c/45077/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2632:         else:
Line 2633:             hooks.after_disk_hotunplug(driveXml, self.conf,
Line 2634:                                        params=drive.custom)
Line 2635: 
Line 2636:             self._devices[hwclass.DISK].remove(drive)
we should keep the order as before, removal of the disk before the hook is executed (to
pass the conf  to the hook with the disk already removed)
Line 2637: 
Line 2638:             # Find and remove disk device from vm's conf
Line 2639:             for dev in self.conf['devices'][:]:
Line 2640:                 if (dev['type'] == hwclass.DISK and


Line 2643:                         self.conf['devices'].remove(dev)
Line 2644:                     break
Line 2645: 
Line 2646:             self._cleanupDrives(drive)
Line 2647:             self.saveState()
please keep _saveState() before _cleanupDrives().
Line 2648: 
Line 2649:         return {'status': doneCode, 'vmList': self.status()}
Line 2650: 
Line 2651:     def _readPauseCode(self):


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 3: Code-Review-1

(3 comments)

https://gerrit.ovirt.org/#/c/45077/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2633
Line 2634
Line 2635
Line 2636
Line 2637
This also fixes a bug in the old code, calling the before_disk_hotunplug hook with the
drive removed from conf, *before* it was removed. Now the hook will be called before the
drive was removed.

However, every bug you fix in vdsm can reveal another bug - please check that that we
don't document this buggy behavior, and that vdsm does not have bad hooks assuming
this buggy behavior.


Line 2632:         else:
Line 2633:             hooks.after_disk_hotunplug(driveXml, self.conf,
Line 2634:                                        params=drive.custom)
Line 2635: 
Line 2636:             self._devices[hwclass.DISK].remove(drive)
>  we should keep the order as before, removal of the disk before the
> hook is  
Right, the hook should the state after the removal.

But even more important, if the hook will raise, we end with incorrect state in memory
(self._devices) and on disk (self.conf).
Line 2637: 
Line 2638:             # Find and remove disk device from vm's conf
Line 2639:             for dev in self.conf['devices'][:]:
Line 2640:                 if (dev['type'] == hwclass.DISK and


Line 2643:                         self.conf['devices'].remove(dev)
Line 2644:                     break
Line 2645: 
Line 2646:             self._cleanupDrives(drive)
Line 2647:             self.saveState()
>  please keep _saveState() before _cleanupDrives(). 
This is
another function that may raise and leave the disk state out of sync.
Line 2648: 
Line 2649:         return {'status': doneCode, 'vmList': self.status()}
Line 2650: 
Line 2651:     def _readPauseCode(self):


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 4:

* Update tracker::#1044466::OK
* Check Bug-Url::OK
* Check Public Bug::#1044466::OK, public bug
* Check Product::#1044466::OK, Correct product oVirt
* Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 4:

(3 comments)

https://gerrit.ovirt.org/#/c/45077/4//COMMIT_MSG
Commit Message:

Line 9: VM's state was saved before detatching a disk in hotunplugDisk.
Line 10: 
Line 11: Saving vm's state should be performed only AFTER vdsm is actually detaching
Line 12: the disk, otherwise the disk will remain in xmlDesc and be calculated in
Line 13: the vm's hash.
Saving state does not effect xmlDesc.
Line 14: This causes a bug in the engine's VM monitoring: the engine
Line 15: gets the same hash as before the disk was unplugged, thus does not
Line 16: update the disk's status.
Line 17: 


Line 12: the disk, otherwise the disk will remain in xmlDesc and be calculated in
Line 13: the vm's hash.
Line 14: This causes a bug in the engine's VM monitoring: the engine
Line 15: gets the same hash as before the disk was unplugged, thus does not
Line 16: update the disk's status.
How saving state before removal can cause engine to get same hash as before the removal?
Line 17: 
Line 18: Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Line 19: Bug-Url:https://bugzilla.redhat.com/1206696
Line 20: Bug-Url: https://bugzilla.redhat.com/1044466


Line 16: update the disk's status.
Line 17: 
Line 18: Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Line 19: Bug-Url:https://bugzilla.redhat.com/1206696
Line 20: Bug-Url: https://bugzilla.redhat.com/1044466
This change looks good, but I don't think it is required for fixing this bug.

Unless you can explain how having incorrect state on disk effect these bugs.


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Francesco Romani has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 4:

(2 comments)

https://gerrit.ovirt.org/#/c/45077/4//COMMIT_MSG
Commit Message:

Line 9: VM's state was saved before detatching a disk in hotunplugDisk.
Line 10: 
Line 11: Saving vm's state should be performed only AFTER vdsm is actually detaching
Line 12: the disk, otherwise the disk will remain in xmlDesc and be calculated in
Line 13: the vm's hash.
>  Saving state does not effect xmlDesc. 
It does, as side effect.
Please check the implementation of Vm.saveState().
Line 14: This causes a bug in the engine's VM monitoring: the engine
Line 15: gets the same hash as before the disk was unplugged, thus does not
Line 16: update the disk's status.
Line 17: 


Line 12: the disk, otherwise the disk will remain in xmlDesc and be calculated in
Line 13: the vm's hash.
Line 14: This causes a bug in the engine's VM monitoring: the engine
Line 15: gets the same hash as before the disk was unplugged, thus does not
Line 16: update the disk's status.
>  How saving state before removal can cause engine to get same hash as
> before 
Most likely (Amit can correct me if I'm wrong) because the saveState
triggers the updateDomainDescriptor to refresh the XML from libvirt and the devices hash.

I commented this before in a past revision. This change can be good improvement to improve
the flow here, but for the issue described in the commit message the proper fix is to
update XML in a correct way, and to deentangle it from the saveState, at least in this
method.
Line 17: 
Line 18: Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Line 19: Bug-Url:https://bugzilla.redhat.com/1206696
Line 20: Bug-Url: https://bugzilla.redhat.com/1044466


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 5:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Amit Aviram has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 4:

(2 comments)

https://gerrit.ovirt.org/#/c/45077/4//COMMIT_MSG
Commit Message:

Line 12: the disk, otherwise the disk will remain in xmlDesc and be calculated in
Line 13: the vm's hash.
Line 14: This causes a bug in the engine's VM monitoring: the engine
Line 15: gets the same hash as before the disk was unplugged, thus does not
Line 16: update the disk's status.
>  Most likely (Amit can correct me if I'm wrong) because the
> saveState trigge 
Right, saveState changing the hash according to libvirt's
XMLDesc().
as for the proper fix, it should be done, but maybe in another patch, as this patch
targets specific bugs.
Line 17: 
Line 18: Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Line 19: Bug-Url:https://bugzilla.redhat.com/1206696
Line 20: Bug-Url: https://bugzilla.redhat.com/1044466


Line 16: update the disk's status.
Line 17: 
Line 18: Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Line 19: Bug-Url:https://bugzilla.redhat.com/1206696
Line 20: Bug-Url: https://bugzilla.redhat.com/1044466
>  This change looks good, but I don't think it is required for
> fixing this bu 
As for 1044466 you are right, it is not related to this fix and will
be removed, thanks.

As for 1206696:
The bug there is that the disk is shown plugged at engine, but actually it is not
connected to the VM. when the user is trying to unplug he gets this error message.

This patch moves state related actions to be after the device is detached. Among other
things, that means that the VM's _devices_hash will be calculated WITHOUT the detached
disk as it should. (saveState sets is based on the current dom.XMLDesc)

That means that the hash will be changed (as opposed to before), and the VM monitoring at
the engine will now notice  that the disk is changed, and will update it to be unplugged.


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Amit Aviram has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 5: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 5:

(2 comments)

https://gerrit.ovirt.org/#/c/45077/5//COMMIT_MSG
Commit Message:

Line 12: the disk, otherwise the disk will remain in xmlDesc and be calculated in
Line 13: the vm's hash.
Line 14: This causes a bug in the engine's VM monitoring: the engine
Line 15: gets the same hash as before the disk was unplugged, thus does not
Line 16: update the disk's status.
The root cause regarding bug 1206696 seems to be not updating domain xml after detaching
the disk, fooling engine monitoring code.

But this sounds like engine bug, not vdsm bug. How disk is considered "plugged"
by engine, after a successful request to unplug the disk? I don't see how monitoring
code effect this.

Can you explain how did you verify this? Can you reproduce this state in engine without
your patch, and show that with your patch this it does not happen?
Line 17: 
Line 18: Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Line 19: Bug-Url:https://bugzilla.redhat.com/1206696


Line 13: the vm's hash.
Line 14: This causes a bug in the engine's VM monitoring: the engine
Line 15: gets the same hash as before the disk was unplugged, thus does not
Line 16: update the disk's status.
Line 17: 
Please fix formatting:
- empty line between paragraphs
- wrap lines consistently (72 chars is a good limit for text).
Line 18: Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Line 19: Bug-Url:https://bugzilla.redhat.com/1206696


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Amit Aviram has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/45077/5//COMMIT_MSG
Commit Message:

Line 12: the disk, otherwise the disk will remain in xmlDesc and be calculated in
Line 13: the vm's hash.
Line 14: This causes a bug in the engine's VM monitoring: the engine
Line 15: gets the same hash as before the disk was unplugged, thus does not
Line 16: update the disk's status.
>  The root cause regarding bug 1206696 seems to be not updating domain
> xml af 
A disk can be plugged in cases where there is a race between the vm
monitoring and hotunpluging. Vm monitoring can be called while the disk is still attached,
then the hountplug is performed and succeed, then the vm monitoring will update the device
to be plugged again.

at the next vm monitoring, and with this patch, the VM's hash will be changed as it
should, and this the engine will update the disk to be unplugged as it really is.

This patch is for making the VM disk's status to be synced in the engine. Therefor
Verification of it was carried out as follows:
- Called hotunplug, make vdsm return (aka the operation succeeded)

- at the engine side, break on the part where we analyze the return value of vdsm- change
it to false (aka, the engine thinks the operation failed)

- now the disk's state is plugged at engine, but unplugged at VDSM. after 2-3 seconds
the monitoring will take action again and the disk's state will be changed to
hotunplug as it should.
Line 17: 
Line 18: Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Line 19: Bug-Url:https://bugzilla.redhat.com/1206696


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 6:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/45077/5//COMMIT_MSG
Commit Message:

Line 12: the disk, otherwise the disk will remain in xmlDesc and be calculated in
Line 13: the vm's hash.
Line 14: This causes a bug in the engine's VM monitoring: the engine
Line 15: gets the same hash as before the disk was unplugged, thus does not
Line 16: update the disk's status.
>  A disk can be plugged in cases where there is a race between the vm
> monitor 
Ok, please update the commit message to explain this flow.

Please also add note that this fix is only partial because deatchDevice is async, handled
in the next patch.
Line 17: 
Line 18: Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Line 19: Bug-Url:https://bugzilla.redhat.com/1206696


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Francesco Romani has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 6: Code-Review+1

(1 comment)

preliminary ACK. At glance patch looks OK, and code is nicer and shorter - and less
fragile.

Deeper review later.

https://gerrit.ovirt.org/#/c/45077/6/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2642: 
Line 2643:             self.saveState()
Line 2644:             hooks.after_disk_hotunplug(driveXml, self.conf,
Line 2645:                                        params=drive.custom)
Line 2646: 
unrelated, but I don't mind that much.
Line 2647:             self._cleanupDrives(drive)
Line 2648: 
Line 2649:         return {'status': doneCode, 'vmList': self.status()}
Line 2650: 


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 7:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Liron Aravot has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/45077/7//COMMIT_MSG
Commit Message:

Line 21: hotplugging is performed and succeeds, then monitoring is resumed and
Line 22: sets the disk to be still plugged, this patch is critical for the
Line 23: engine to be synced with the actual state of the VM's disks.
Line 24: 
Line 25: Please note that this patch is partial, as deatchDevice is async. That
/s/deatch/detach

please make sure to open a bug/fix hotunplugNic.
Line 26: means that still the hash can be calculated wrong in case the disk was
Line 27: not detached yet. This fix will be included in a following patch.
Line 28: 
Line 29: Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Amit Aviram has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/45077/7//COMMIT_MSG
Commit Message:

Line 21: hotplugging is performed and succeeds, then monitoring is resumed and
Line 22: sets the disk to be still plugged, this patch is critical for the
Line 23: engine to be synced with the actual state of the VM's disks.
Line 24: 
Line 25: Please note that this patch is partial, as deatchDevice is async. That
>  /s/deatch/detach 
Done, thanks!
Line 26: means that still the hash can be calculated wrong in case the disk was
Line 27: not detached yet. This fix will be included in a following patch.
Line 28: 
Line 29: Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 8:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 8: Code-Review+1

Waiting for Adam review.

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Adam Litke has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 8: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 8: Code-Review+2

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 8: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/45077/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2634: 
Line 2635:             # Find and remove disk device from vm's conf
Line 2636:             for dev in self.conf['devices'][:]:
Line 2637:                 if (dev['type'] == hwclass.DISK and
Line 2638:                             dev['path'] == drive.path):
Please fix pep8 error:

$ pep8 vdsm/virt/vm.py 
vdsm/virt/vm.py:2638:29: E127 continuation line over-indented for visual indent

You should run make check locally before posting patches to gerrit.
Line 2639:                     with self._confLock:
Line 2640:                         self.conf['devices'].remove(dev)
Line 2641:                     break
Line 2642: 


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Francesco Romani has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 8: -Code-Review

(2 comments)

https://gerrit.ovirt.org/#/c/45077/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2631:             return response.error('hotunplugDisk', e.message)
Line 2632:         else:
Line 2633:             self._devices[hwclass.DISK].remove(drive)
Line 2634: 
Line 2635:             # Find and remove disk device from vm's conf
this is almost identical to _lookupConfByPath (used into the live merge path). Any reason
not to use it?

  try:
      dev = self._lookupConfByPath(drive.path)
  except LookupError:
      self.log.error("log some critical error (not using this message verbatim):
config is inconsistent with device map")
  else:
      with self._confLock:
          self.conf['devices'].remove(dev)
Line 2636:             for dev in self.conf['devices'][:]:
Line 2637:                 if (dev['type'] == hwclass.DISK and
Line 2638:                             dev['path'] == drive.path):
Line 2639:                     with self._confLock:


Line 2642: 
Line 2643:             self.saveState()
Line 2644:             hooks.after_disk_hotunplug(driveXml, self.conf,
Line 2645:                                        params=drive.custom)
Line 2646: 
you need to resubmit anyway, so please remove unneeded althoug nice spurious blank line.
Line 2647:             self._cleanupDrives(drive)
Line 2648: 
Line 2649:         return {'status': doneCode, 'vmList': self.status()}
Line 2650: 


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Amit Aviram has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 8: -Verified

(2 comments)

https://gerrit.ovirt.org/#/c/45077/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2631:             return response.error('hotunplugDisk', e.message)
Line 2632:         else:
Line 2633:             self._devices[hwclass.DISK].remove(drive)
Line 2634: 
Line 2635:             # Find and remove disk device from vm's conf
>  this is almost identical to _lookupConfByPath (used into the live
> merge pat 
This patch is only about moving code around, I don't think this change
should be in that patch. It is a good idea to add it in a following one though.
Line 2636:             for dev in self.conf['devices'][:]:
Line 2637:                 if (dev['type'] == hwclass.DISK and
Line 2638:                             dev['path'] == drive.path):
Line 2639:                     with self._confLock:


Line 2634: 
Line 2635:             # Find and remove disk device from vm's conf
Line 2636:             for dev in self.conf['devices'][:]:
Line 2637:                 if (dev['type'] == hwclass.DISK and
Line 2638:                             dev['path'] == drive.path):
>  Please fix pep8 error: 
Done
Line 2639:                     with self._confLock:
Line 2640:                         self.conf['devices'].remove(dev)
Line 2641:                     break
Line 2642: 


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Francesco Romani has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 8:

(1 comment)

https://gerrit.ovirt.org/#/c/45077/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2631:             return response.error('hotunplugDisk', e.message)
Line 2632:         else:
Line 2633:             self._devices[hwclass.DISK].remove(drive)
Line 2634: 
Line 2635:             # Find and remove disk device from vm's conf
>  This patch is only about moving code around, I don't think this
> change shou 
fair enough.
Line 2636:             for dev in self.conf['devices'][:]:
Line 2637:                 if (dev['type'] == hwclass.DISK and
Line 2638:                             dev['path'] == drive.path):
Line 2639:                     with self._confLock:


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 8:

(1 comment)

https://gerrit.ovirt.org/#/c/45077/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2631:             return response.error('hotunplugDisk', e.message)
Line 2632:         else:
Line 2633:             self._devices[hwclass.DISK].remove(drive)
Line 2634: 
Line 2635:             # Find and remove disk device from vm's conf
>  this is almost identical to _lookupConfByPath (used into the live
> merge pat 
Good catch, but I think ot is not availble in 3.5, and we need to get this
fix there. We will remove the duplication in a future patch.
Line 2636:             for dev in self.conf['devices'][:]:
Line 2637:                 if (dev['type'] == hwclass.DISK and
Line 2638:                             dev['path'] == drive.path):
Line 2639:                     with self._confLock:


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 9:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

automation(a)ovirt.org has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 10:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Amit Aviram has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 10: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Francesco Romani has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 10: Code-Review+2

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

automation(a)ovirt.org has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 11:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 11: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/45077/11/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2609:             self._devices[hwclass.DISK].remove(drive)
Line 2610: 
Line 2611:             # Find and remove disk device from vm's conf
Line 2612:             for dev in self.conf['devices'][:]:
Line 2613:                 if (dev['type'] == hwclass.DISK and dev['path']
== drive.path):
The parenthesis are not needed now, please use:

     if dev['type'] == hwclass.DISK and dev['path'] == drive.path:
Line 2614:                     with self._confLock:
Line 2615:                         self.conf['devices'].remove(dev)
Line 2616:                     break
Line 2617: 


-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Francesco Romani has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 11: -Code-Review

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

automation(a)ovirt.org has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 12:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 12: Code-Review+2

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Amit Aviram has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 12: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Francesco Romani has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 12: Code-Review+2

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


vm.py: State saving in hotunplugDisk.

VM's state was saved before detatching a disk in hotunplugDisk.

Saving vm's state should be performed only AFTER vdsm is actually
detaching the disk, otherwise the disk will remain in xmlDesc and
be calculated in the vm's hash.

This causes a bug in the engine's VM monitoring: the engine
gets the same hash as before the disk was unplugged, thus does not
update the disk's status.

Due to a possible race while hotunplugging, where VM monitoring starts
updating the VM devices while the disk is still plugged- then
hotplugging is performed and succeeds, then monitoring is resumed and
sets the disk to be still plugged, this patch is critical for the
engine to be synced with the actual state of the VM's disks.

Please note that this patch is partial, as detachDevice is async. That
means that still the hash can be calculated wrong in case the disk was
not detached yet. This fix will be included in a following patch.

Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Bug-Url:https://bugzilla.redhat.com/1206696
Signed-off-by: Amit Aviram <aaviram(a)redhat.com&gt;
Reviewed-on: https://gerrit.ovirt.org/45077
Reviewed-by: Nir Soffer <nsoffer(a)redhat.com&gt;
Reviewed-by: Francesco Romani <fromani(a)redhat.com&gt;
Continuous-Integration: Jenkins CI
---
M vdsm/virt/vm.py
1 file changed, 10 insertions(+), 19 deletions(-)

Approvals:
  Nir Soffer: Looks good to me, approved
  Jenkins CI: Passed CI tests
  Amit Aviram: Verified
  Francesco Romani: Looks good to me, approved



-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org

automation(a)ovirt.org has posted comments on this change.

Change subject: vm.py: State saving in hotunplugDisk.
......................................................................


Patch Set 13:

* Update tracker::IGNORE, no Bug-Url found
* Set MODIFIED::IGNORE, no Bug-Url found.

-- 
To view, visit https://gerrit.ovirt.org/45077
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2cf18186cbba33d7e74fd15651ffec3149c98e1d
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Amit Aviram <aaviram(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No