Hi, Please try to split your patches into a patch series, where each commit performs one change. In this case, the following commits would come to my mind: * Extend the tuple passed to StartInstance by BlockDev instance - this must update all hypervisors, as they have to deal with 3-tuples now * Add `access` disk parameter - Add GetUserspaceAccessUri in BlockDev - Add support for calling it in hv_kvm.py * Finally implement custom RBD URI for RADOSBlockDevice.
It's ok if you leave it as is right now, but please keep this in mind for later patch series. On Sat, Jul 27, 2013 at 12:25 PM, Pulkit Singhal <[email protected]>wrote: > Changes: > - 'gnt-instance info' and 'gnt-cluster info' show the disk access type. > - Support for 'gnt-cluster modify -D rbd:access=userspace' is > implemented. > - Generic method GetUserspaceAccessURI() is added in RADOSBlockDevice > class. > > Signed-off-by: Pulkit Singhal <[email protected]> > --- > lib/backend.py | 2 +- > lib/client/gnt_instance.py | 1 + > lib/cmdlib/instance_query.py | 2 ++ > lib/constants.py | 10 ++++++++-- > lib/hypervisor/hv_kvm.py | 9 ++++++--- > lib/objects.py | 1 + > lib/storage/bdev.py | 8 ++++++++ > 7 files changed, 27 insertions(+), 6 deletions(-) > > diff --git a/lib/backend.py b/lib/backend.py > index 7063aa5..3e84106 100644 > --- a/lib/backend.py > +++ b/lib/backend.py > @@ -1628,7 +1628,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, device, link_name)) > > return block_devices > > diff --git a/lib/client/gnt_instance.py b/lib/client/gnt_instance.py > index 1334212..023ccd8 100644 > --- a/lib/client/gnt_instance.py > +++ b/lib/client/gnt_instance.py > @@ -1033,6 +1033,7 @@ def _FormatBlockDevInfo(idx, top_level, dev, roman): > if top_level: > if dev["spindles"] is not None: > data.append(("spindles", dev["spindles"])) > + data.append(("access type", dev["access_type"])) > I don't see why we should output this parameter here. We don't do it for all other disk parameters which are set at the cluster/node level, so I wouldn't do it for the access mode neither. > data.append(("access mode", dev["mode"])) > if dev["logical_id"] is not None: > try: > diff --git a/lib/cmdlib/instance_query.py b/lib/cmdlib/instance_query.py > index 5c55c7b..afbe0b2 100644 > --- a/lib/cmdlib/instance_query.py > +++ b/lib/cmdlib/instance_query.py > @@ -340,6 +340,7 @@ class LUInstanceQueryData(NoHooksLU): > @attention: The device has to be annotated already. > > """ > + > Why the additional newline? > drbd_info = None > if dev.dev_type in constants.LDS_DRBD: > # we change the snode then (otherwise we use the one passed in) > @@ -369,6 +370,7 @@ class LUInstanceQueryData(NoHooksLU): > dev_children = [] > > return { > + "access_type": dev.params[constants.LDP_ACCESS], > Same as above, I wouldn't include this single parameter in the output whereas we don't include all others. Additionally, this code probably won't work for other disk template which do not have the `access` parameter anymore. > "iv_name": dev.iv_name, > "dev_type": dev.dev_type, > "logical_id": dev.logical_id, > diff --git a/lib/constants.py b/lib/constants.py > index 5522e5a..02842cb 100644 > --- a/lib/constants.py > +++ b/lib/constants.py > @@ -1230,6 +1230,7 @@ LDP_DELAY_TARGET = "c-delay-target" > LDP_MAX_RATE = "c-max-rate" > LDP_MIN_RATE = "c-min-rate" > LDP_POOL = "pool" > +LDP_ACCESS = "access" > DISK_LD_TYPES = { > LDP_RESYNC_RATE: VTYPE_INT, > LDP_STRIPES: VTYPE_INT, > @@ -1246,6 +1247,7 @@ DISK_LD_TYPES = { > LDP_MAX_RATE: VTYPE_INT, > LDP_MIN_RATE: VTYPE_INT, > LDP_POOL: VTYPE_STRING, > + LDP_ACCESS: VTYPE_STRING, > } > DISK_LD_PARAMETERS = frozenset(DISK_LD_TYPES.keys()) > > @@ -1268,6 +1270,7 @@ DRBD_MAX_RATE = "c-max-rate" > DRBD_MIN_RATE = "c-min-rate" > LV_STRIPES = "stripes" > RBD_POOL = "pool" > +RBD_ACCESS = "access" > DISK_DT_TYPES = { > DRBD_RESYNC_RATE: VTYPE_INT, > DRBD_DATA_STRIPES: VTYPE_INT, > @@ -1286,6 +1289,7 @@ DISK_DT_TYPES = { > DRBD_MIN_RATE: VTYPE_INT, > LV_STRIPES: VTYPE_INT, > RBD_POOL: VTYPE_STRING, > + RBD_ACCESS: VTYPE_STRING, > } > > DISK_DT_PARAMETERS = frozenset(DISK_DT_TYPES.keys()) > @@ -2232,7 +2236,8 @@ DISK_LD_DEFAULTS = { > LD_FILE: {}, > LD_BLOCKDEV: {}, > LD_RBD: { > - LDP_POOL: "rbd" > + LDP_POOL: "rbd", > + LDP_ACCESS: "userspace", > }, > LD_EXT: {}, > } > @@ -2267,7 +2272,8 @@ DISK_DT_DEFAULTS = { > DT_SHARED_FILE: {}, > DT_BLOCK: {}, > DT_RBD: { > - RBD_POOL: DISK_LD_DEFAULTS[LD_RBD][LDP_POOL] > + RBD_POOL: DISK_LD_DEFAULTS[LD_RBD][LDP_POOL], > + RBD_ACCESS: DISK_LD_DEFAULTS[LD_RBD][LDP_ACCESS], > }, > DT_EXT: {}, > } > diff --git a/lib/hypervisor/hv_kvm.py b/lib/hypervisor/hv_kvm.py > index 14c5807..cfc6a92 100644 > --- a/lib/hypervisor/hv_kvm.py > +++ b/lib/hypervisor/hv_kvm.py > @@ -1125,7 +1125,7 @@ class KVMHypervisor(hv_base.BaseHypervisor): > cache_val = ",cache=%s" % disk_cache > else: > cache_val = "" > - for cfdev, dev_path in block_devices: > + for cfdev, device, dev_path in block_devices: > Probably all hypervisors use the block_devices tuple which you changed. But you only adapt the code handling it for KVM, essentially breaking the other ones. If you perform this change, you have to fix all other usages too. Ideally, such a change is one single commit in a patch series. > if cfdev.mode != constants.DISK_RDWR: > raise errors.HypervisorError("Instance has read-only disks which" > " are not supported by KVM") > @@ -1137,8 +1137,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] == 'userspace': > 'userspace' should be a constant. We always use " instead of ' for strings. Please read our style guide [0] for further style rules. > + dev_path = device.GetUserspaceAccessUri(constants.HT_KVM) > + > + 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/objects.py b/lib/objects.py > index 6105595..20cefb7 100644 > --- a/lib/objects.py > +++ b/lib/objects.py > @@ -896,6 +896,7 @@ class Disk(ConfigObject): > elif disk_template == constants.DT_RBD: > > result.append(FillDict(constants.DISK_LD_DEFAULTS[constants.LD_RBD], { > constants.LDP_POOL: dt_params[constants.RBD_POOL], > + constants.LDP_ACCESS: dt_params[constants.RBD_ACCESS], > })) > > elif disk_template == constants.DT_EXT: > diff --git a/lib/storage/bdev.py b/lib/storage/bdev.py > index 932bc98..4c7a23c 100644 > --- a/lib/storage/bdev.py > +++ b/lib/storage/bdev.py > @@ -999,6 +999,7 @@ class RADOSBlockDevice(base.BlockDev): > raise ValueError("Invalid configuration data %s" % str(unique_id)) > > self.driver, self.rbd_name = unique_id > + self.rbd_pool = params[constants.LDP_POOL] > > self.major = self.minor = None > self.Attach() > @@ -1335,6 +1336,13 @@ class RADOSBlockDevice(base.BlockDev): > base.ThrowError("rbd resize failed (%s): %s", > result.fail_reason, result.output) > > + def GetUserspaceAccessUri(self, hypervisor): > + """ For specific hypervisor type, return the disk URI for userspace > + access. > + """ > + if hypervisor == constants.HT_KVM: > + return "rbd:" + self.rbd_pool + "/" + self.rbd_name > + > The definition of this method has to go in the base class with a default implementation (raising an exception probably) and proper docstring. Also, this implementation must do something (raising or returning) in the else-branch. > > class ExtStorageDevice(base.BlockDev): > """A block device provided by an ExtStorage Provider. > -- > 1.7.9.5 > > Thanks, Thomas [0]: https://code.google.com/p/ganeti/wiki/StyleGuide -- 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
