LGTM, thanks.
On Mon, Sep 30, 2013 at 3:32 PM, Santi Raffa <[email protected]> wrote: > Additionally: > * fixed some spurious indentation > * removed the spurious import to storage/__init__.py at the source > > Interdiff: > > diff --git a/lib/hypervisor/hv_kvm.py b/lib/hypervisor/hv_kvm.py > index 309a177..8eb2b00 100644 > --- a/lib/hypervisor/hv_kvm.py > +++ b/lib/hypervisor/hv_kvm.py > @@ -1150,10 +1150,12 @@ class KVMHypervisor(hv_base.BaseHypervisor): > boot_val = ",boot=on" > > if cfdev.params[constants.LDP_ACCESS] == constants.DISK_USERSPACE: > - dev_path = device.GetUserspaceAccessUri(constants.HT_KVM) > + drive_uri = device.GetUserspaceAccessUri(constants.HT_KVM) > + else: > + drive_uri = dev_path > > - drive_val = "file=%s,format=raw%s%s%s" % (dev_path, if_val, > - boot_val, cache_val) > + drive_val = "file=%s,format=raw%s%s%s" % (drive_uri, if_val, > + boot_val, cache_val) > kvm_cmd.extend(["-drive", drive_val]) > > #Now we can specify a different device type for CDROM devices. > diff --git a/lib/storage/__init__.py b/lib/storage/__init__.py > index efe9600..9b9e38c 100644 > --- a/lib/storage/__init__.py > +++ b/lib/storage/__init__.py > @@ -22,5 +22,3 @@ > """Block device abstraction > > """ > - > -from ganeti.storage import base > diff --git a/test/py/ganeti.hypervisor.hv_xen_unittest.py > b/test/py/ganeti.hypervisor.hv_xen_unittest.py > index 75f22dc..c40e50c 100755 > --- a/test/py/ganeti.hypervisor.hv_xen_unittest.py > +++ b/test/py/ganeti.hypervisor.hv_xen_unittest.py > @@ -271,7 +271,7 @@ class TestGetConfigFileDiskData(unittest.TestCase): > for offset in [0, 1, 10]: > disks = [(objects.Disk(dev_type=constants.DT_PLAIN), > "/tmp/disk/%s" % idx, > - None) > + NotImplemented) > for idx in range(len(hv_xen._DISK_LETTERS) + offset)] > > if offset == 0: > @@ -292,10 +292,10 @@ class TestGetConfigFileDiskData(unittest.TestCase): > disks = [ > (objects.Disk(dev_type=constants.DT_PLAIN, > mode=constants.DISK_RDWR), > "/tmp/diskFirst", > - None), > + NotImplemented), > (objects.Disk(dev_type=constants.DT_PLAIN, > mode=constants.DISK_RDONLY), > "/tmp/diskLast", > - None), > + NotImplemented), > ] > > result = hv_xen._GetConfigFileDiskData(disks, "hd") > @@ -309,19 +309,19 @@ class TestGetConfigFileDiskData(unittest.TestCase): > (objects.Disk(dev_type=constants.DT_FILE, mode=constants.DISK_RDWR, > logical_id=[constants.FD_LOOP]), > "/tmp/diskFirst", > - None), > + NotImplemented), > (objects.Disk(dev_type=constants.DT_FILE, > mode=constants.DISK_RDONLY, > logical_id=[constants.FD_BLKTAP]), > "/tmp/diskTwo", > - None), > + NotImplemented), > (objects.Disk(dev_type=constants.DT_FILE, mode=constants.DISK_RDWR, > logical_id=[constants.FD_LOOP]), > "/tmp/diskThree", > - None), > + NotImplemented), > (objects.Disk(dev_type=constants.DT_FILE, mode=constants.DISK_RDWR, > logical_id=[constants.FD_BLKTAP]), > "/tmp/diskLast", > - None), > + NotImplemented), > ] > > result = hv_xen._GetConfigFileDiskData(disks, "sd") > @@ -337,7 +337,7 @@ class TestGetConfigFileDiskData(unittest.TestCase): > (objects.Disk(dev_type=constants.DT_FILE, mode=constants.DISK_RDWR, > logical_id=["#unknown#"]), > "/tmp/diskinvalid", > - None), > + NotImplemented), > ] > > self.assertRaises(KeyError, hv_xen._GetConfigFileDiskData, disks, > "sd") > @@ -689,17 +689,16 @@ class _TestXenHypervisor(object): > disks = [ > (objects.Disk(dev_type=constants.DT_PLAIN, > mode=constants.DISK_RDWR), > utils.PathJoin(self.tmpdir, "disk0"), > - None), > + NotImplemented), > (objects.Disk(dev_type=constants.DT_PLAIN, > mode=constants.DISK_RDONLY), > utils.PathJoin(self.tmpdir, "disk1"), > - None), > + NotImplemented), > ] > > inst = objects.Instance(name="server01.example.com", > hvparams=hvp, beparams=bep, > osparams={}, nics=[], os="deb1", > disks=map(compat.fst, disks)) > - > inst.UpgradeConfig() > > return (inst, disks) > > On Mon, Sep 30, 2013 at 10:54 AM, Thomas Thrainer <[email protected]> > wrote: > > > > > > > > On Fri, Sep 27, 2013 at 3:35 PM, Raffa Santi <[email protected]> wrote: > >> > >> * Add device class object in block_device tuple > >> * Update hv_xen.py for new block_devices format > >> * Fix tests broken by the change > >> > >> Signed-off-by: Raffa Santi <[email protected]> > >> --- > >> lib/backend.py | 2 +- > >> lib/hypervisor/hv_kvm.py | 9 +++++--- > >> lib/hypervisor/hv_xen.py | 2 +- > >> lib/storage/__init__.py | 2 ++ > >> test/py/ganeti.hypervisor.hv_xen_unittest.py | 32 > >> ++++++++++++++++++-------- > >> 5 files changed, 32 insertions(+), 15 deletions(-) > >> > >> diff --git a/lib/backend.py b/lib/backend.py > >> index 5e27048..a16b22a 100644 > >> --- a/lib/backend.py > >> +++ b/lib/backend.py > >> @@ -1636,7 +1636,7 @@ def _GatherAndLinkBlockDevs(instance): > >> raise errors.BlockDeviceError("Cannot create block device > symlink: > >> %s" % > >> e.strerror) > >> > >> - block_devices.append((disk, link_name)) > >> + block_devices.append((disk, link_name, device)) > >> > >> return block_devices > >> > >> diff --git a/lib/hypervisor/hv_kvm.py b/lib/hypervisor/hv_kvm.py > >> index 91a3b35..309a177 100644 > >> --- a/lib/hypervisor/hv_kvm.py > >> +++ b/lib/hypervisor/hv_kvm.py > >> @@ -1137,7 +1137,7 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> cache_val = ",cache=%s" % disk_cache > >> else: > >> cache_val = "" > >> - for cfdev, dev_path in block_devices: > >> + for cfdev, dev_path, device in block_devices: > >> if cfdev.mode != constants.DISK_RDWR: > >> raise errors.HypervisorError("Instance has read-only disks > which" > >> " are not supported by KVM") > >> @@ -1149,8 +1149,11 @@ class KVMHypervisor(hv_base.BaseHypervisor): > >> if needs_boot_flag and disk_type != constants.HT_DISK_IDE: > >> boot_val = ",boot=on" > >> > >> - drive_val = "file=%s,format=raw%s%s%s" % (dev_path, if_val, > >> boot_val, > >> - cache_val) > >> + if cfdev.params[constants.LDP_ACCESS] == > constants.DISK_USERSPACE: > >> + dev_path = device.GetUserspaceAccessUri(constants.HT_KVM) > > > > > > NIT: the dev_path variable does not hold a device path at this point. > > Consider introducing another variable (drive_uri, file_param or > something) > > which you can use instead. > > > >> > >> + > >> + drive_val = "file=%s,format=raw%s%s%s" % (dev_path, if_val, > >> + boot_val, cache_val) > >> kvm_cmd.extend(["-drive", drive_val]) > >> > >> #Now we can specify a different device type for CDROM devices. > >> diff --git a/lib/hypervisor/hv_xen.py b/lib/hypervisor/hv_xen.py > >> index cf697e9..a37bbc9 100644 > >> --- a/lib/hypervisor/hv_xen.py > >> +++ b/lib/hypervisor/hv_xen.py > >> @@ -287,7 +287,7 @@ def _GetConfigFileDiskData(block_devices, > >> blockdev_prefix, > >> > >> disk_data = [] > >> > >> - for sd_suffix, (cfdev, dev_path) in zip(_letters, block_devices): > >> + for sd_suffix, (cfdev, dev_path, _) in zip(_letters, block_devices): > >> sd_name = blockdev_prefix + sd_suffix > >> > >> if cfdev.mode == constants.DISK_RDWR: > >> diff --git a/lib/storage/__init__.py b/lib/storage/__init__.py > >> index 9b9e38c..efe9600 100644 > >> --- a/lib/storage/__init__.py > >> +++ b/lib/storage/__init__.py > >> @@ -22,3 +22,5 @@ > >> """Block device abstraction > >> > >> """ > >> + > >> +from ganeti.storage import base > >> diff --git a/test/py/ganeti.hypervisor.hv_xen_unittest.py > >> b/test/py/ganeti.hypervisor.hv_xen_unittest.py > >> index 4a3270b..75f22dc 100755 > >> --- a/test/py/ganeti.hypervisor.hv_xen_unittest.py > >> +++ b/test/py/ganeti.hypervisor.hv_xen_unittest.py > >> @@ -269,7 +269,9 @@ class TestGetConfigFileDiskData(unittest.TestCase): > >> > >> def testManyDisks(self): > >> for offset in [0, 1, 10]: > >> - disks = [(objects.Disk(dev_type=constants.DT_PLAIN), > "/tmp/disk/%s" > >> % idx) > >> + disks = [(objects.Disk(dev_type=constants.DT_PLAIN), > >> + "/tmp/disk/%s" % idx, > >> + None) > > > > > > We usually use NotImplemented for values which are not required in a test > > instead of None (as None could be a valid value too). Same for all > > occurrences below. > > > >> > >> for idx in range(len(hv_xen._DISK_LETTERS) + offset)] > >> > >> if offset == 0: > >> @@ -289,9 +291,11 @@ class TestGetConfigFileDiskData(unittest.TestCase): > >> def testTwoLvDisksWithMode(self): > >> disks = [ > >> (objects.Disk(dev_type=constants.DT_PLAIN, > >> mode=constants.DISK_RDWR), > >> - "/tmp/diskFirst"), > >> + "/tmp/diskFirst", > >> + None), > >> (objects.Disk(dev_type=constants.DT_PLAIN, > >> mode=constants.DISK_RDONLY), > >> - "/tmp/diskLast"), > >> + "/tmp/diskLast", > >> + None), > >> ] > >> > >> result = hv_xen._GetConfigFileDiskData(disks, "hd") > >> @@ -304,16 +308,20 @@ class > TestGetConfigFileDiskData(unittest.TestCase): > >> disks = [ > >> (objects.Disk(dev_type=constants.DT_FILE, > mode=constants.DISK_RDWR, > >> logical_id=[constants.FD_LOOP]), > >> - "/tmp/diskFirst"), > >> + "/tmp/diskFirst", > >> + None), > >> (objects.Disk(dev_type=constants.DT_FILE, > >> mode=constants.DISK_RDONLY, > >> logical_id=[constants.FD_BLKTAP]), > >> - "/tmp/diskTwo"), > >> + "/tmp/diskTwo", > >> + None), > >> (objects.Disk(dev_type=constants.DT_FILE, > mode=constants.DISK_RDWR, > >> logical_id=[constants.FD_LOOP]), > >> - "/tmp/diskThree"), > >> + "/tmp/diskThree", > >> + None), > >> (objects.Disk(dev_type=constants.DT_FILE, > mode=constants.DISK_RDWR, > >> logical_id=[constants.FD_BLKTAP]), > >> - "/tmp/diskLast"), > >> + "/tmp/diskLast", > >> + None), > >> ] > >> > >> result = hv_xen._GetConfigFileDiskData(disks, "sd") > >> @@ -328,7 +336,8 @@ class TestGetConfigFileDiskData(unittest.TestCase): > >> disks = [ > >> (objects.Disk(dev_type=constants.DT_FILE, > mode=constants.DISK_RDWR, > >> logical_id=["#unknown#"]), > >> - "/tmp/diskinvalid"), > >> + "/tmp/diskinvalid", > >> + None), > >> ] > >> > >> self.assertRaises(KeyError, hv_xen._GetConfigFileDiskData, disks, > >> "sd") > >> @@ -679,15 +688,18 @@ class _TestXenHypervisor(object): > >> > >> disks = [ > >> (objects.Disk(dev_type=constants.DT_PLAIN, > >> mode=constants.DISK_RDWR), > >> - utils.PathJoin(self.tmpdir, "disk0")), > >> + utils.PathJoin(self.tmpdir, "disk0"), > >> + None), > >> (objects.Disk(dev_type=constants.DT_PLAIN, > >> mode=constants.DISK_RDONLY), > >> - utils.PathJoin(self.tmpdir, "disk1")), > >> + utils.PathJoin(self.tmpdir, "disk1"), > >> + None), > >> ] > >> > >> inst = objects.Instance(name="server01.example.com", > >> hvparams=hvp, beparams=bep, > >> osparams={}, nics=[], os="deb1", > >> disks=map(compat.fst, disks)) > >> + > > > > > > Spurious new newline? > > > >> > >> inst.UpgradeConfig() > >> > >> return (inst, disks) > >> -- > >> 1.7.10.4 > >> > > > > > > > > -- > > Thomas Thrainer | Software Engineer | [email protected] | > > > > Google Germany GmbH > > Dienerstr. 12 > > 80331 München > > > > Registergericht und -nummer: Hamburg, HRB 86891 > > Sitz der Gesellschaft: Hamburg > > Geschäftsführer: Graham Law, Christine Elizabeth Flores > > > > -- > Raffa Santi > Google Germany GmbH > Dienerstr. 12 > 80331 München > > > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Geschäftsführer: Graham Law, Christine Elizabeth Flores > -- Thomas Thrainer | Software Engineer | [email protected] | Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
