Change in vdsm[master]: Don't prepare image with illegal volumes

Adam Litke has uploaded a new change for review.

Change subject: Don't prepare image with illegal volumes
......................................................................

Don't prepare image with illegal volumes

The irs verb 'prepareImage' is used only by clientIF when preparing the
host to start a VM.  Currently, this operation succeeds even if one or
more of the volumes in the image is illegal.  Since we should never
permit a VM to start with an illegal disk, check for this and report an
error if any illegal volumes are found.

The pivot stage of a live merge operation depends on this behavior to
ensure that a VM is not accidentally started using a stale leaf volume.

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1206722
Change-Id: Ie543aeb8bdb52305419613ab6297681817124308
Signed-off-by: Adam Litke 
---
M vdsm/storage/hsm.py
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/02/39302/1

diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 8f07586..dc8ca77 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -3225,6 +3225,11 @@
         if leafUUID not in imgVolumes:
             raise se.VolumeDoesNotExist(leafUUID)

+        for volUUID in imgVolumes:
+            legality = dom.produceVolume(imgUUID, volUUID).getLegality()
+            if legality == volume.ILLEGAL_VOL:
+                raise se.prepareIllegalVolumeError(volUUID)
+
         imgPath = dom.activateVolumes(imgUUID, imgVolumes)
         if spUUID and spUUID != sd.BLANK_UUID:
             runImgPath = dom.linkBCImage(imgPath, imgUUID)

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

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 1:

* Update tracker::#1206722::OK
* Check Bug-Url::OK
* Check Public Bug::#1206722::OK, public bug
* Check Product::#1206722::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'])

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 1:

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 1:

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

Adam Litke has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 1: Verified+1

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 1:

Build Successful

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

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

Greg Padgett has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 1: Code-Review+1

Makes sense to me in the context of live merge flows.

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 1: Code-Review+1

[Allon Mureinik]

Allon Mureinik has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 1: Code-Review+1

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/39302/1/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 3225:         if leafUUID not in imgVolumes:
Line 3226:             raise se.VolumeDoesNotExist(leafUUID)
Line 3227: 
Line 3228:         for volUUID in imgVolumes:
Line 3229:             legality = dom.produceVolume(imgUUID, volUUID).getLegality()
we tried hard to avoid single-volume productions like these, as they cause serious performance hit.

Could you see if checking legality after image-wide activation is safe enough for this use case?
Line 3230:             if legality == volume.ILLEGAL_VOL:
Line 3231:                 raise se.prepareIllegalVolumeError(volUUID)
Line 3232: 
Line 3233:         imgPath = dom.activateVolumes(imgUUID, imgVolumes)

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/39302/1/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 3225:         if leafUUID not in imgVolumes:
Line 3226:             raise se.VolumeDoesNotExist(leafUUID)
Line 3227: 
Line 3228:         for volUUID in imgVolumes:
Line 3229:             legality = dom.produceVolume(imgUUID, volUUID).getLegality()
> we tried hard to avoid single-volume productions like these, as they cause

Lets time this with and without the legality check before trying to optimize this.
Line 3230:             if legality == volume.ILLEGAL_VOL:
Line 3231:                 raise se.prepareIllegalVolumeError(volUUID)
Line 3232: 
Line 3233:         imgPath = dom.activateVolumes(imgUUID, imgVolumes)

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

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 2:

* Update tracker::#1206722::OK
* Check Bug-Url::OK
* Check Public Bug::#1206722::OK, public bug
* Check Product::#1206722::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'])

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 2:

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 2:

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 2:

Build Successful

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

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

Francesco Romani has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 2: Code-Review+1

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/39302/2/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 3227: 
Line 3228:         for volUUID in imgVolumes:
Line 3229:             legality = dom.produceVolume(imgUUID, volUUID).getLegality()
Line 3230:             if legality == volume.ILLEGAL_VOL:
Line 3231:                 raise se.prepareIllegalVolumeError(volUUID)
Can you provide rough timing results of this function before and after this patch? We don't want to introduce performance regressions.
Line 3232: 
Line 3233:         imgPath = dom.activateVolumes(imgUUID, imgVolumes)
Line 3234:         if spUUID and spUUID != sd.BLANK_UUID:
Line 3235:             runImgPath = dom.linkBCImage(imgPath, imgUUID)

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 2:

(1 comment)

https://gerrit.ovirt.org/#/c/39302/2/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 3233:         imgPath = dom.activateVolumes(imgUUID, imgVolumes)
Line 3234:         if spUUID and spUUID != sd.BLANK_UUID:
Line 3235:             runImgPath = dom.linkBCImage(imgPath, imgUUID)
Line 3236:         else:
Line 3237:             runImgPath = imgPath
Could you see if checking legality after image-wide activation is safe enough for this use case?
Line 3238: 
Line 3239:         leafPath = os.path.join(runImgPath, leafUUID)
Line 3240:         for volUUID in imgVolumes:
Line 3241:             path = os.path.join(dom.domaindir, sd.DOMAIN_IMAGES, imgUUID,

Adam Litke has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 2:

(2 comments)

https://gerrit.ovirt.org/#/c/39302/2/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 3227: 
Line 3228:         for volUUID in imgVolumes:
Line 3229:             legality = dom.produceVolume(imgUUID, volUUID).getLegality()
Line 3230:             if legality == volume.ILLEGAL_VOL:
Line 3231:                 raise se.prepareIllegalVolumeError(volUUID)
> Can you provide rough timing results of this function before and after this

I measured the time the entire prepareImage function in hsm.py took using utils.monotonic_time().  To make the calls I created a VM with 4 disks where each disk had 0, 1, 2, and 4 snapshots respectively.  I started and stopped the VM twice in a row to ensure a reasonably stable measurement was being taken.  Here are the results (ordered in ascending elapsed time):

Without this patch:

0.00999999977648 s
0.00999999977648 s
0.0100000007078 s
0.0100000007078 s
0.019999999553 s
0.0200000004843 s
0.0300000002608 s
0.070000000298 s

With the patch:

0.0100000007078 s
0.0100000007078 s
0.019999999553 s
0.019999999553 s
0.0200000004843 s
0.0300000002608 s
0.0399999991059 s
0.0400000000373 s

My conclusion is that this patch affects performance slightly but not severely enough to justify the dangerously incorrect current behavior.
Line 3232: 
Line 3233:         imgPath = dom.activateVolumes(imgUUID, imgVolumes)
Line 3234:         if spUUID and spUUID != sd.BLANK_UUID:
Line 3235:             runImgPath = dom.linkBCImage(imgPath, imgUUID)

Line 3233:         imgPath = dom.activateVolumes(imgUUID, imgVolumes)
Line 3234:         if spUUID and spUUID != sd.BLANK_UUID:
Line 3235:             runImgPath = dom.linkBCImage(imgPath, imgUUID)
Line 3236:         else:
Line 3237:             runImgPath = imgPath
> Could you see if checking legality after image-wide activation is safe enou

It seems to be roughly the same code except that using Image.isLegal() includes an extra call to retrieve again all of the volumes in the image.  Is there a particular reason you would prefer your approach?
Line 3238: 
Line 3239:         leafPath = os.path.join(runImgPath, leafUUID)
Line 3240:         for volUUID in imgVolumes:
Line 3241:             path = os.path.join(dom.domaindir, sd.DOMAIN_IMAGES, imgUUID,

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 2:

There is no significant difference with or without this patch.

But we may like to test more snapshots, or domains with more volumes.

Adam Litke has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 2:

danken: ping.  Now would be a good time to put this into production and evaluate it.

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 2:

I agree with Adam, we must have this.

Adam Litke has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 2:

Dan?

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 2: Code-Review+2

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

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 3:

* Update tracker::#1206722::OK
* Check Bug-Url::OK
* Check Public Bug::#1206722::OK, public bug
* Check Product::#1206722::OK, Correct product oVirt
* Check TM::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'])

Adam Litke has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 3:

rebased.

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

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 4:

* Update tracker::#1206722::OK
* Check Bug-Url::OK
* Check Public Bug::#1206722::OK, public bug
* Check Product::#1206722::OK, Correct product oVirt
* Check TM::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'])

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Don't prepare image with illegal volumes
......................................................................

Patch Set 4:

Since Federico chose not to block this, I merge it in.

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Don't prepare image with illegal volumes
......................................................................

Don't prepare image with illegal volumes

The irs verb 'prepareImage' is used only by clientIF when preparing the
host to start a VM.  Currently, this operation succeeds even if one or
more of the volumes in the image is illegal.  Since we should never
permit a VM to start with an illegal disk, check for this and report an
error if any illegal volumes are found.

The pivot stage of a live merge operation depends on this behavior to
ensure that a VM is not accidentally started using a stale leaf volume.

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1206722
Change-Id: Ie543aeb8bdb52305419613ab6297681817124308
Signed-off-by: Adam Litke 
Reviewed-on: https://gerrit.ovirt.org/39302
Reviewed-by: Greg Padgett 
Reviewed-by: Allon Mureinik 
Reviewed-by: Francesco Romani 
Reviewed-by: Nir Soffer 
Continuous-Integration: Jenkins CI
---
M vdsm/storage/hsm.py
1 file changed, 5 insertions(+), 0 deletions(-)

Approvals:
  Nir Soffer: Looks good to me, approved
  Adam Litke: Verified
  Greg Padgett: Looks good to me, but someone else must approve
  Jenkins CI: Passed CI tests
  Allon Mureinik: 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: Don't prepare image with illegal volumes
......................................................................

Patch Set 5:

* Update tracker::#1206722::OK
* Check TR::#1206722::ERROR, 3.5.2 should not match .*