Change in vdsm[master]: vm: Require format attribute for drives

[Nir Soffer]

Nir Soffer has uploaded a new change for review.

Change subject: vm: Require format attribute for drives
......................................................................

vm: Require format attribute for drives

We have seen sampling errors where a drive has no "format" attribute.
These errors spam the system logs, and does not help to debug the real
issue - why the required format attribute is not set?

This patch ensure that a drive cannot be created without a format
attribute, avoding log spam by sampling errors, and hopefully revealing
the real reason for this bug.

One broken test creating a drive without a format was fixed and new test
ensure that creating drive without a format raises.

Change-Id: I01ab1e071ecb76f383cc6dc7d99782e10cc90136
Relates-To: http://gerrit.ovirt.org/22551
Relates-To: https://bugzilla.redhat.com/994534
Relates-To: http://lists.ovirt.org/pipermail/users/2014-February/021007.html
Bug-Url: https://bugzilla.redhat.com/1055437
Signed-off-by: Nir Soffer 
---
M tests/vmTests.py
M vdsm/vm.py
2 files changed, 28 insertions(+), 3 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/34/24234/1

diff --git a/tests/vmTests.py b/tests/vmTests.py
index 1e0a3f6..724d458 100644
--- a/tests/vmTests.py
+++ b/tests/vmTests.py
@@ -449,6 +449,18 @@
         redir = vm.RedirDevice(self.conf, self.log, **dev)
         self.assertXML(redir.getXML(), redirXML)

+    def testDriveRequiredParameters(self):
+        # TODO: It is not clear what are the other required parameters, and it
+        # the parameters depend on the type of drive. We probbaly need bigger
+        # test here that test many combinations.
+        # Currently this test only missing "format" attribute.
+        conf = {'index': '2', 'propagateErrors': 'off', 'iface': 'ide',
+                'name': 'hdc', 'device': 'cdrom', 'path': '/tmp/fedora.iso',
+                'type': 'disk', 'readonly': 'True', 'shared': 'none',
+                'serial': '54-a672-23e5b495a9ea'}
+        self.assertRaises(vm.InvalidDeviceParameters, vm.Drive, {}, self.log,
+                          **conf)
+
     def testDriveSharedStatus(self):
         sharedConfigs = [
             # Backward compatibility
@@ -470,7 +482,8 @@
             'exclusive', 'shared', 'none', 'transient',
         ]

-        driveConfig = {'index': '0', 'iface': 'virtio', 'device': 'disk'}
+        driveConfig = {'index': '0', 'iface': 'virtio', 'device': 'disk',
+                       'format': 'raw'}

for driveInput, driveOutput in zip(sharedConfigs, expectedStates):
             driveInput.update(driveConfig)
diff --git a/vdsm/vm.py b/vdsm/vm.py
index aae8bd6..b7151b1 100644
--- a/vdsm/vm.py
+++ b/vdsm/vm.py
@@ -80,6 +80,10 @@
 SMARTCARD_DEVICES = 'smartcard'

+class InvalidDeviceParameters(Exception):
+    """ Raised when creating device with invalid parameters """
+
+
 def isVdsmImage(drive):
     """
     Tell if drive looks like a vdsm image
@@ -1438,6 +1442,9 @@
     VOLWM_CHUNK_REPLICATE_MULT = 2  # Chunk multiplier during replication

def __init__(self, conf, log, **kwargs):
+        if 'format' not in kwargs:
+            raise InvalidDeviceParameters('"format" attribute is required:'
+                                          ' %r' % kwargs)
         if not kwargs.get('serial'):
             self.serial = kwargs.get('imageID'[-20:]) or ''
         VmDevice.__init__(self, conf, log, **kwargs)
@@ -3108,8 +3115,13 @@

for devType, devClass in self.DeviceMapping:
             for dev in devices[devType]:
-                self._devices[devType].append(devClass(self.conf, self.log,
-                                                       **dev))
+                try:
+                    drive = devClass(self.conf, self.log, **dev)
+                except InvalidDeviceParameters as e:
+                    self.log.error('Ignoring device with invalid parameters:'
+                                   ' %s', e, exc_info=True)
+                else:
+                    self._devices[devType].append(drive)

# We should set this event as a last part of drives initialization
         self._pathsPreparedEvent.set()

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: Require format attribute for drives
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6257/ : SUCCESS

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7147/ : SUCCESS

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm: Require format attribute for drives
......................................................................

Patch Set 1:

(1 comment)

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

Line 1441:     VOLWM_FREE_PCT = 100 - config.getint('irs', 'volume_utilization_percent')
Line 1442:     VOLWM_CHUNK_REPLICATE_MULT = 2  # Chunk multiplier during replication
Line 1443: 
Line 1444:     def __init__(self, conf, log, **kwargs):
Line 1445:         if 'format' not in kwargs:
> can't we put a default value instead of raising an exception?

We can, but this will hide the faulty code calling us without a required parameter, which we like to fix.
Line 1446:             raise InvalidDeviceParameters('"format" attribute is required:'
Line 1447:                                           ' %r' % kwargs)
Line 1448:         if not kwargs.get('serial'):
Line 1449:             self.serial = kwargs.get('imageID'[-20:]) or ''

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: Require format attribute for drives
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

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

Line 1441:     VOLWM_FREE_PCT = 100 - config.getint('irs', 'volume_utilization_percent')
Line 1442:     VOLWM_CHUNK_REPLICATE_MULT = 2  # Chunk multiplier during replication
Line 1443: 
Line 1444:     def __init__(self, conf, log, **kwargs):
Line 1445:         if 'format' not in kwargs:
> We can, but this will hide the faulty code calling us without a required pa

Setting a default is wrong and failing is harsh.

There's also another case that we want to fix: what if the parameter is wrong? (e.g. format passed by engine == 'raw' but volume is cow? or the other way around).

On creation:

* if the parameter is missing it should be filled in with a value taken from the metadata (e.g. getVolumeInfo)
* if the parameter is present and wrong we want to fail

Later on (e.g. migration or vdsm restart):

* if the parameter is missing it should be filled in with a value taken from getVolumeInfo or using what libvirt reports in its xml (but we probably want to log this as there's a bug somewhere)

Basically the format passed by engine instead of being mandatory should be optional and validated for correctness if present.
Line 1446:             raise InvalidDeviceParameters('"format" attribute is required:'
Line 1447:                                           ' %r' % kwargs)
Line 1448:         if not kwargs.get('serial'):
Line 1449:             self.serial = kwargs.get('imageID'[-20:]) or ''

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm: Require format attribute for drives
......................................................................

Patch Set 1:

(1 comment)

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

Line 1441:     VOLWM_FREE_PCT = 100 - config.getint('irs', 'volume_utilization_percent')
Line 1442:     VOLWM_CHUNK_REPLICATE_MULT = 2  # Chunk multiplier during replication
Line 1443: 
Line 1444:     def __init__(self, conf, log, **kwargs):
Line 1445:         if 'format' not in kwargs:
> Setting a default is wrong and failing is harsh.

This is too complicated and has too much heuristics. Do you see any reason why the parameter is missing when creating a drive?
Line 1446:             raise InvalidDeviceParameters('"format" attribute is required:'
Line 1447:                                           ' %r' % kwargs)
Line 1448:         if not kwargs.get('serial'):
Line 1449:             self.serial = kwargs.get('imageID'[-20:]) or ''

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: Require format attribute for drives
......................................................................

Patch Set 1:

(1 comment)

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

Line 1441:     VOLWM_FREE_PCT = 100 - config.getint('irs', 'volume_utilization_percent')
Line 1442:     VOLWM_CHUNK_REPLICATE_MULT = 2  # Chunk multiplier during replication
Line 1443: 
Line 1444:     def __init__(self, conf, log, **kwargs):
Line 1445:         if 'format' not in kwargs:
> This is too complicated and has too much heuristics. Do you see any reason

And most importantly, for base images guessing the format is a known security problem (the owner of a VM with raw format can mess with his image too look like a qcow, and gain read/write on any other image on next boot).
Line 1446:             raise InvalidDeviceParameters('"format" attribute is required:'
Line 1447:                                           ' %r' % kwargs)
Line 1448:         if not kwargs.get('serial'):
Line 1449:             self.serial = kwargs.get('imageID'[-20:]) or ''

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vm: Require format attribute for drives
......................................................................

Patch Set 1:

(1 comment)

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

Line 1441:     VOLWM_FREE_PCT = 100 - config.getint('irs', 'volume_utilization_percent')
Line 1442:     VOLWM_CHUNK_REPLICATE_MULT = 2  # Chunk multiplier during replication
Line 1443: 
Line 1444:     def __init__(self, conf, log, **kwargs):
Line 1445:         if 'format' not in kwargs:
> And most importantly, for base images guessing the format is a known securi

Dan, in fact I proposed to use getVolumeInfo, after that for simplicity if you want you *can* use what's in the xml.
Line 1446:             raise InvalidDeviceParameters('"format" attribute is required:'
Line 1447:                                           ' %r' % kwargs)
Line 1448:         if not kwargs.get('serial'):
Line 1449:             self.serial = kwargs.get('imageID'[-20:]) or ''

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm: Require format attribute for drives
......................................................................

Patch Set 1:

Turns out that Federico has this patch http://gerrit.ovirt.org/24202 which should fix the root cause of this error, so we don't need this patch for debugging this issue.

More then that, Federico suggest that using the image format from __init__ is not good enough, and we must always check the format using the disk meta data, so requiring the format from in __init__ is not what we want.

So the proper solution will:

1. Initialize format to None if not set in __init__
2. In prepareVolumePath, use getVolumeInfo to get the real format, and update disk format, logging a warning if the previous format was different
3. In vm.Vm._getUnderlyingDriveInfo, we will get the format from libvirt, and update drive format, logging a warning if the format was different

Itamar Heim has posted comments on this change.

Change subject: vm: Require format attribute for drives
......................................................................

Patch Set 1:

ping

Itamar Heim has abandoned this change.

Change subject: vm: Require format attribute for drives
......................................................................

Abandoned

no activity. please restore if relevant.

[Nir Soffer]

Nir Soffer has restored this change.

Change subject: vm: Require format attribute for drives
......................................................................

Restored

This is not resolved yet, we should implement the fix suggested by Federico.

[Nir Soffer]

Nir Soffer has abandoned this change.

Change subject: vm: Require format attribute for drives
......................................................................

Abandoned

Should be handled in a more general patch, part of input validation for vdsm verbs.

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

Change subject: vm: Require format attribute for drives
......................................................................

Patch Set 1:

* Update tracker::#1055437::OK