Hello Nir Soffer, Freddy Rolland,
I'd like you to do a code review. Please visit
https://gerrit.ovirt.org/56274
to review the following change.
Change subject: virt: storage: avoid empty serials to domain XML
......................................................................
virt: storage: avoid empty serials to domain XML
The VM drives in the libvirt domain XML may have
one 'serial' attribute.
Vdsm used to always sends this attribute.
The value is either received as drive configuration
parameter, or it is filled from the drive backing imageID.
Should Vdsm not receive the serial attribute, or should
the drive not have a backing image, the serial will be
empty, and will be sent anyway to libvirt.
Starting from libvirt 1.3.0, libvirt enforces the fact
that the serial must not be empty (see
https://bugzilla.redhat.com/1322842#c4).
This exposed an old bug of Vdsm, and the effect is that
libvirt rejects the domain XML and the VM could not start.
To trigger this bug, just configure one VM with payload
device (e.g. cloud init) this will send a new cdrom device
alongside the default one, and both will have empty serial.
Looking again at the libvirt domain XML specifications
(http://libvirt.org/formatdomain.html#elementsDisks)
we see that the 'serial' attribute is not mandatory.
Thus, the simplest fix is just not to add the 'serial'
attribute in the domain XML if it is empty.
Change-Id: I175954321faef8fb33ee31b6b367bfad9ec87137
Bug-Url: https://bugzilla.redhat.com/1322842
Backport-To: 3.6
Signed-off-by: Francesco Romani <[email protected]>
Reviewed-on: https://gerrit.ovirt.org/56014
Reviewed-by: Freddy Rolland <[email protected]>
Reviewed-by: Nir Soffer <[email protected]>
Continuous-Integration: Jenkins CI
---
M tests/vmStorageTests.py
M tests/vmTests.py
M vdsm/virt/vmdevices/storage.py
3 files changed, 41 insertions(+), 5 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/74/56274/1
diff --git a/tests/vmStorageTests.py b/tests/vmStorageTests.py
index 4310bd2..2886a0a 100644
--- a/tests/vmStorageTests.py
+++ b/tests/vmStorageTests.py
@@ -37,6 +37,7 @@
index='2',
path='/path/to/fedora.iso',
readonly='True',
+ serial='54-a672-23e5b495a9ea',
)
xml = """
<disk device="cdrom" snapshot="no" type="file">
@@ -52,6 +53,7 @@
conf = drive_config(
format='cow',
propagateErrors='on',
+ serial='54-a672-23e5b495a9ea',
shared='shared',
specParams={
'ioTune': {
@@ -78,7 +80,9 @@
self.check(vm_conf, conf, xml, is_block_device=False)
def test_disk_block(self):
- conf = drive_config()
+ conf = drive_config(
+ serial='54-a672-23e5b495a9ea',
+ )
xml = """
<disk device="disk" snapshot="no" type="block">
<source dev="/path/to/volume"/>
@@ -91,7 +95,9 @@
self.check({}, conf, xml, is_block_device=True)
def test_disk_file(self):
- conf = drive_config()
+ conf = drive_config(
+ serial='54-a672-23e5b495a9ea',
+ )
xml = """
<disk device="disk" snapshot="no" type="file">
<source file="/path/to/volume"/>
@@ -108,6 +114,7 @@
device='lun',
iface='scsi',
path='/dev/mapper/lun1',
+ serial='54-a672-23e5b495a9ea',
sgio='unfiltered',
)
xml = """
@@ -129,6 +136,7 @@
],
path='poolname/volumename',
protocol='rbd',
+ serial='54-a672-23e5b495a9ea',
)
xml = """
<disk device="disk" snapshot="no" type="network">
@@ -154,6 +162,7 @@
],
path='poolname/volumename',
protocol='rbd',
+ serial='54-a672-23e5b495a9ea',
)
xml = """
<disk device="disk" snapshot="no" type="network">
@@ -171,6 +180,35 @@
</disk>
"""
self.check({}, conf, xml, is_block_device=None)
+
+ def test_cdrom_without_serial(self):
+ conf = drive_config(
+ device='cdrom',
+ iface='ide',
+ index='2',
+ path='/path/to/fedora.iso',
+ readonly='True',
+ )
+ xml = """
+ <disk device="cdrom" snapshot="no" type="file">
+ <source file="/path/to/fedora.iso" startupPolicy="optional"/>
+ <target bus="ide" dev="hdc"/>
+ <readonly/>
+ </disk>
+ """
+ self.check({}, conf, xml, is_block_device=False)
+
+ def test_disk_without_serial(self):
+ conf = drive_config()
+ xml = """
+ <disk device="disk" snapshot="no" type="file">
+ <source file="/path/to/volume"/>
+ <target bus="virtio" dev="vda"/>
+ <driver cache="none" error_policy="stop"
+ io="threads" name="qemu" type="raw"/>
+ </disk>
+ """
+ self.check({}, conf, xml, is_block_device=False)
def check(self, vm_conf, device_conf, xml, is_block_device=False):
drive = Drive(vm_conf, self.log, **device_conf)
@@ -473,7 +511,6 @@
'path': '/path/to/volume',
'propagateErrors': 'off',
'readonly': 'False',
- 'serial': '54-a672-23e5b495a9ea',
'shared': 'none',
'type': 'disk',
}
diff --git a/tests/vmTests.py b/tests/vmTests.py
index 5471c03..6b054f1 100644
--- a/tests/vmTests.py
+++ b/tests/vmTests.py
@@ -890,7 +890,6 @@
<disk device="hdd" snapshot="no" type="block">
<source dev="/dev/dummy"/>
<target bus="ide" dev="hda"/>
- <serial></serial>
<iotune>
<read_bytes_sec>2</read_bytes_sec>
<write_bytes_sec>1</write_bytes_sec>
diff --git a/vdsm/virt/vmdevices/storage.py b/vdsm/virt/vmdevices/storage.py
index 2ffb63a..2e47e94 100644
--- a/vdsm/virt/vmdevices/storage.py
+++ b/vdsm/virt/vmdevices/storage.py
@@ -364,7 +364,7 @@
# they are readonly unless explicitely stated otherwise
diskelem.appendChildWithArgs('readonly')
- if hasattr(self, 'serial') and self.device != 'lun':
+ if getattr(self, 'serial', False) and self.device != 'lun':
diskelem.appendChildWithArgs('serial', text=self.serial)
if hasattr(self, 'bootOrder'):
--
To view, visit https://gerrit.ovirt.org/56274
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I175954321faef8fb33ee31b6b367bfad9ec87137
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Francesco Romani <[email protected]>
Gerrit-Reviewer: Freddy Rolland <[email protected]>
Gerrit-Reviewer: Nir Soffer <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
