On Mon, May 02, 2016 at 06:10:28PM -0600, David Mohr wrote:
> The user id is used by ceph to determine the keyring to use for
> authentication. By default the admin keyring is used, which may
> not be desirable.
>
> Signed-off-by: David Mohr <[email protected]>
Hi David,
Thanks for the patch.
I'm afraid we don't use Ceph internally, so we don't really have a good way
of testing this. I assume you've tested it and it's working for you locally.
Just eyeballing the code, it seems sane to me, but a couple of questions
(see inline)...
Cheers,
Brian.
> ---
> lib/storage/bdev.py | 45 ++++++++++++++++++++++++++++-----------------
> src/Ganeti/Constants.hs | 18 +++++++++++++++++-
> 2 files changed, 45 insertions(+), 18 deletions(-)
>
> diff --git a/lib/storage/bdev.py b/lib/storage/bdev.py
> index e3a48de..4892ec2 100644
> --- a/lib/storage/bdev.py
> +++ b/lib/storage/bdev.py
> @@ -886,6 +886,13 @@ class RADOSBlockDevice(base.BlockDev):
> self.Attach()
>
> @classmethod
> + def MakeRbdCmd(cls, params, cmd):
> + if constants.RBD_USER_ID in params and params[constants.RBD_USER_ID]:
> + cmd.append("--id")
> + cmd.append("%s" % params[constants.RBD_USER_ID])
> + return [constants.RBD_CMD] + cmd
Nit: This might be slightly neater as
if params.get(constants.RBD_USER_ID, ""):
cmd.extend(["--id", str(params[constants.RBD_USER_ID])])
> +
> + @classmethod
> def Create(cls, unique_id, children, size, spindles, params, excl_stor,
> dyn_params, *args):
> """Create a new rbd device.
> @@ -903,8 +910,8 @@ class RADOSBlockDevice(base.BlockDev):
> rbd_name = unique_id[1]
>
> # Provision a new rbd volume (Image) inside the RADOS cluster.
> - cmd = [constants.RBD_CMD, "create", "-p", rbd_pool,
> - rbd_name, "--size", "%s" % size]
> + cmd = cls.MakeRbdCmd(params, ["create", "-p", rbd_pool, rbd_name,
> + "--size", "%s" % size])
Nit: Generally there's a preference in the code for str(foo) vs "%s" % foo.
> result = utils.RunCmd(cmd)
> if result.failed:
> base.ThrowError("rbd creation failed (%s): %s",
> @@ -928,7 +935,7 @@ class RADOSBlockDevice(base.BlockDev):
> self.Shutdown()
>
> # Remove the actual Volume (Image) from the RADOS cluster.
> - cmd = [constants.RBD_CMD, "rm", "-p", rbd_pool, rbd_name]
> + cmd = self.__class__.MakeRbdCmd(self.params, ["rm", "-p", rbd_pool,
> rbd_name])
> result = utils.RunCmd(cmd)
> if result.failed:
> base.ThrowError("Can't remove Volume from cluster with rbd rm: %s -
> %s",
> @@ -987,7 +994,7 @@ class RADOSBlockDevice(base.BlockDev):
> return rbd_dev
>
> # The mapping doesn't exist. Create it.
> - map_cmd = [constants.RBD_CMD, "map", "-p", pool, name]
> + map_cmd = self.__class__.MakeRbdCmd(self.params, ["map", "-p", pool,
> name])
> result = utils.RunCmd(map_cmd)
> if result.failed:
> base.ThrowError("rbd map failed (%s): %s",
> @@ -1017,14 +1024,13 @@ class RADOSBlockDevice(base.BlockDev):
> try:
> # Newer versions of the rbd tool support json output formatting. Use it
> # if available.
> - showmap_cmd = [
> - constants.RBD_CMD,
> + showmap_cmd = cls.MakeRbdCmd({}, [
> "showmapped",
> "-p",
> pool,
> "--format",
> "json"
> - ]
> + ])
> result = utils.RunCmd(showmap_cmd)
> if result.failed:
> logging.error("rbd JSON output formatting returned error (%s): %s,"
> @@ -1036,7 +1042,7 @@ class RADOSBlockDevice(base.BlockDev):
> except RbdShowmappedJsonError:
> # For older versions of rbd, we have to parse the plain / text output
> # manually.
> - showmap_cmd = [constants.RBD_CMD, "showmapped", "-p", pool]
> + showmap_cmd = cls.MakeRbdCmd({}, ["showmapped", "-p", pool])
> result = utils.RunCmd(showmap_cmd)
> if result.failed:
> base.ThrowError("rbd showmapped failed (%s): %s",
> @@ -1168,7 +1174,7 @@ class RADOSBlockDevice(base.BlockDev):
>
> if rbd_dev:
> # The mapping exists. Unmap the rbd device.
> - unmap_cmd = [constants.RBD_CMD, "unmap", "%s" % rbd_dev]
> + unmap_cmd = self.__class__.MakeRbdCmd(self.params, ["unmap", "%s" %
> rbd_dev])
Nit: Again str(rbd_dev).
> result = utils.RunCmd(unmap_cmd)
> if result.failed:
> base.ThrowError("rbd unmap failed (%s): %s",
> @@ -1212,8 +1218,8 @@ class RADOSBlockDevice(base.BlockDev):
> new_size = self.size + amount
>
> # Resize the rbd volume (Image) inside the RADOS cluster.
> - cmd = [constants.RBD_CMD, "resize", "-p", rbd_pool,
> - rbd_name, "--size", "%s" % new_size]
> + cmd = self.__class__.MakeRbdCmd(self.params, ["resize", "-p", rbd_pool,
> + rbd_name, "--size", "%s" % new_size])
Nit: str(new_size)
> result = utils.RunCmd(cmd)
> if result.failed:
> base.ThrowError("rbd resize failed (%s): %s",
> @@ -1248,16 +1254,17 @@ class RADOSBlockDevice(base.BlockDev):
> self._UnmapVolumeFromBlockdev(self.unique_id)
>
> # Remove the actual Volume (Image) from the RADOS cluster.
> - cmd = [constants.RBD_CMD, "rm", "-p", rbd_pool, rbd_name]
> + cmd = self.__class__.MakeRbdCmd(self.params, ["rm", "-p", rbd_pool,
> rbd_name])
> result = utils.RunCmd(cmd)
> if result.failed:
> base.ThrowError("Can't remove Volume from cluster with rbd rm: %s -
> %s",
> result.fail_reason, result.output)
>
> # We use "-" for importing from stdin
> - return [constants.RBD_CMD, "import",
> + return self.__class__.MakeRbdCmd(self.params, [
> + "import",
> "-p", rbd_pool,
> - "-", rbd_name]
> + "-", rbd_name])
>
> def Export(self):
> """Builds the shell command for exporting data from device.
> @@ -1273,9 +1280,10 @@ class RADOSBlockDevice(base.BlockDev):
> rbd_name = self.unique_id[1]
>
> # We use "-" for exporting to stdout.
> - return [constants.RBD_CMD, "export",
> + return self.__class__.MakeRbdCmd(self.params, [
> + "export",
> "-p", rbd_pool,
> - rbd_name, "-"]
> + rbd_name, "-"])
>
> def GetUserspaceAccessUri(self, hypervisor):
> """Generate KVM userspace URIs to be used as `-drive file` settings.
> @@ -1284,7 +1292,10 @@ class RADOSBlockDevice(base.BlockDev):
>
> """
> if hypervisor == constants.HT_KVM:
> - return "rbd:" + self.rbd_pool + "/" + self.rbd_name
> + uri = "rbd:" + self.rbd_pool + "/" + self.rbd_name
> + if constants.RBD_USER_ID in self.params and
> self.params[constants.RBD_USER_ID]:
> + uri += ":id=%s" % self.params[constants.RBD_USER_ID]
> + return uri
A question here about escaping parameters. What happens if the RBD pool,
device names, or user id have a : or = in them? Does ceph have a defined
escaping mechanism for this? Should we be using it?
> else:
> base.ThrowError("Hypervisor %s doesn't support RBD userspace access" %
> hypervisor)
> diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs
> index 09ca78d..c186877 100644
> --- a/src/Ganeti/Constants.hs
> +++ b/src/Ganeti/Constants.hs
> @@ -2330,6 +2330,9 @@ ldpPlanAhead = "c-plan-ahead"
> ldpPool :: String
> ldpPool = "pool"
>
> +ldpUserId :: String
> +ldpUserId = "user-id"
> +
> ldpProtocol :: String
> ldpProtocol = "protocol"
>
> @@ -2357,7 +2360,8 @@ diskLdTypes =
> (ldpDelayTarget, VTypeInt),
> (ldpMaxRate, VTypeInt),
> (ldpMinRate, VTypeInt),
> - (ldpPool, VTypeString)]
> + (ldpPool, VTypeString),
> + (ldpUserId, VTypeString)]
>
> diskLdParameters :: FrozenSet String
> diskLdParameters = ConstantUtils.mkSet (Map.keys diskLdTypes)
> @@ -2418,6 +2422,9 @@ lvStripes = "stripes"
> rbdAccess :: String
> rbdAccess = "access"
>
> +rbdUserId :: String
> +rbdUserId = "user-id"
> +
> rbdPool :: String
> rbdPool = "pool"
>
> @@ -2440,6 +2447,7 @@ diskDtTypes =
> (drbdMinRate, VTypeInt),
> (lvStripes, VTypeInt),
> (rbdAccess, VTypeString),
> + (rbdUserId, VTypeString),
> (rbdPool, VTypeString),
> (glusterHost, VTypeString),
> (glusterVolume, VTypeString),
> @@ -4240,6 +4248,9 @@ defaultPlanAhead = 20
> defaultRbdPool :: String
> defaultRbdPool = "rbd"
>
> +defaultRbdUserId :: String
> +defaultRbdUserId = ""
> +
> diskLdDefaults :: Map DiskTemplate (Map String PyValueEx)
> diskLdDefaults =
> Map.fromList
> @@ -4266,11 +4277,13 @@ diskLdDefaults =
> , (DTPlain, Map.fromList [(ldpStripes, PyValueEx lvmStripecount)])
> , (DTRbd, Map.fromList
> [ (ldpPool, PyValueEx defaultRbdPool)
> + , (ldpUserId, PyValueEx defaultRbdUserId)
> , (ldpAccess, PyValueEx diskKernelspace)
> ])
> , (DTSharedFile, Map.empty)
> , (DTGluster, Map.fromList
> [ (rbdAccess, PyValueEx diskKernelspace)
> + , (rbdUserId, PyValueEx defaultRbdUserId)
> , (glusterHost, PyValueEx glusterHostDefault)
> , (glusterVolume, PyValueEx glusterVolumeDefault)
> , (glusterPort, PyValueEx glusterPortDefault)
> @@ -4301,16 +4314,19 @@ diskDtDefaults =
> ])
> , (DTExt, Map.fromList
> [ (rbdAccess, PyValueEx diskKernelspace)
> + , (rbdUserId, PyValueEx defaultRbdUserId)
> ])
> , (DTFile, Map.empty)
> , (DTPlain, Map.fromList [(lvStripes, PyValueEx lvmStripecount)])
> , (DTRbd, Map.fromList
> [ (rbdPool, PyValueEx defaultRbdPool)
> , (rbdAccess, PyValueEx diskKernelspace)
> + , (rbdUserId, PyValueEx defaultRbdUserId)
> ])
> , (DTSharedFile, Map.empty)
> , (DTGluster, Map.fromList
> [ (rbdAccess, PyValueEx diskKernelspace)
> + , (rbdUserId, PyValueEx defaultRbdUserId)
> , (glusterHost, PyValueEx glusterHostDefault)
> , (glusterVolume, PyValueEx glusterVolumeDefault)
> , (glusterPort, PyValueEx glusterPortDefault)
These are a backwards-incompatible change in the RPC protocol, but that's
OK since master will be a new version, and we don't require compatibilty
across versions.
