On Tuesday, May 3, 2016 at 6:20:43 AM UTC-6, Brian Foley wrote:
> 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.
>
Hi Brian,
thank you for evaluating the patch and giving me some feedback. Yes, we are
using this patch internally and it is working well for us.
I hope the small patch update that I sent is appropriate, of course I will
send a squashed patch once everything is fixed.
>> @@ -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?
>
As far as I can tell these are valid characters for ceph. I'm not sure what
the best mechanism would be to escape them in ganeti. Maybe a note in the
documenation would be appropriate that these characters are not supported
as part of the user id. While not idea, this doesn't seem overly
restrictive to me.
Speaking of documentation, my patch is obviously lacking that so far. Any
pointers for me where in the source tree I'd need to add that to?
>
> > 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.
>
Ok. No way around that, is there? I'm also asking because I'll be sending
some more patches soon which also touch the RPC protocol.
Again thanks for the feedback!
