Change in vdsm[master]: vm: Delete Drive.diskReplicate before saving config

[Nir Soffer]

Nir Soffer has uploaded a new change for review.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

vm: Delete Drive.diskReplicate before saving config

When drive replication is finished, either by pivoting to the
destination volume or keeping the source volume, Drive.diskReplicate
must be deleted, to ensure that high watermark monitoring is correct.

The current code could fail to delete diskReplicate if the disk was not
found in Vm.conf, or saving state failed. This could lead to incorrect
extend requests for non-existent volume.

Change-Id: I3112f94a4877e28057497749938aa8c3d7771d30
Signed-off-by: Nir Soffer 
---
M vdsm/virt/vm.py
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/20/40220/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 5b82bdf..2896c30 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -3019,12 +3019,12 @@
         This utility method is the inverse of _setDiskReplica, look at the
         _setDiskReplica description for more information.
         """
+        del drive.diskReplicate
+
         disk = self._findConfDisk(drive.name)
         with self._confLock:
             del disk['diskReplicate']
         self.saveState()
-
-        del drive.diskReplicate

def diskReplicateStart(self, srcDisk, dstDisk):
         try:

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 1:

* 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'])

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 1:

Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/18142/

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 1:

Build Started (2/2)

0 -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1372/

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 1:

Build Successful

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

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1372/ : 0

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 1: Verified+1

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 2:

* 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'])

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 2:

Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/18158/

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 2:

Build Started (2/2)

0 -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1388/

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 2:

Build Successful

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

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1388/ : 0

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 3:

* 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'])

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 3:

Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/18186/

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 3:

Build Started (2/2)

0 -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1416/

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 3:

Build Failed

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

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1416/ : 0

Adam Litke has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 3: Code-Review+2

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 4:

* 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'])

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 4:

Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/18385/

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 4:

Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1615/

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 4:

Build Successful

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

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

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

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'])

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

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'])

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

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'])

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 7: Code-Review+1

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

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'])

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

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'])

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

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'])

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

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'])

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 11: Verified+1

Francesco Romani has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 11: Code-Review+1

(1 comment)

I'm fine with the concept, and codewise looks OK.

https://gerrit.ovirt.org/#/c/40220/11//COMMIT_MSG
Commit Message:

Line 10: destination volume or keeping the source volume, Drive.diskReplicate
Line 11: must be deleted, to ensure that high watermark monitoring is correct.
Line 12: 
Line 13: The current code could fail to delete diskReplicate if the disk was not
Line 14: found in Vm.conf, or saving state failed. This could lead to incorrect
correct, even though I'd be already worried by the mismatch between drive data and Vm.conf.
saveState failure is definitely possible.
Line 15: extend requests for non-existent volume.
Line 16: 
Line 17: Change-Id: I3112f94a4877e28057497749938aa8c3d7771d30

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 11:

(1 comment)

https://gerrit.ovirt.org/#/c/40220/11//COMMIT_MSG
Commit Message:

Line 10: destination volume or keeping the source volume, Drive.diskReplicate
Line 11: must be deleted, to ensure that high watermark monitoring is correct.
Line 12: 
Line 13: The current code could fail to delete diskReplicate if the disk was not
Line 14: found in Vm.conf, or saving state failed. This could lead to incorrect
> correct, even though I'd be already worried by the mismatch between drive d

Francesco, what can we do if saving state failed other than fail the current request?

We have two options - have incorrect in-memory state, which can lead to breaking of extension mechanism, but being consistent with on-disk state, or have correct in-memory state synchronized with libvirt state, and incorrect on-disk state.

The only way out of this is eliminating the on-disk state, and using libvirt state, or minimizing the on-disk state only to the state which is not kept by libvirt.
Line 15: extend requests for non-existent volume.
Line 16: 
Line 17: Change-Id: I3112f94a4877e28057497749938aa8c3d7771d30

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

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'])

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 13:

* 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'])

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 14:

* 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'])

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 15:

* 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'])

Freddy Rolland has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 15: Code-Review+1

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 15: Code-Review+2

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

vm: Delete Drive.diskReplicate before saving config

When drive replication is finished, either by pivoting to the
destination volume or keeping the source volume, Drive.diskReplicate
must be deleted, to ensure that high watermark monitoring is correct.

The current code could fail to delete diskReplicate if the disk was not
found in Vm.conf, or saving state failed. This could lead to incorrect
extend requests for non-existent volume.

Change-Id: I3112f94a4877e28057497749938aa8c3d7771d30
Signed-off-by: Nir Soffer 
Reviewed-on: https://gerrit.ovirt.org/40220
Reviewed-by: Francesco Romani 
Continuous-Integration: Jenkins CI
Reviewed-by: Freddy Rolland 
Reviewed-by: Dan Kenigsberg 
---
M vdsm/virt/vm.py
1 file changed, 2 insertions(+), 2 deletions(-)

Approvals:
  Nir Soffer: Verified
  Jenkins CI: Passed CI tests
  Dan Kenigsberg: Looks good to me, approved
  Freddy Rolland: Looks good to me, but someone else must approve
  Francesco Romani: Looks good to me, but someone else must approve

[e-mail] has posted comments on this change.

Change subject: vm: Delete Drive.diskReplicate before saving config
......................................................................

Patch Set 16:

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