On Sun, May 08, 2016 at 12:19:28PM -0700, David Mohr wrote: > 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.
Excellent, thanks. > >> @@ -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. Fair enough. > 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? Hmm. As far as I can tell, there doesn't appear to be any documentation of the specific options supported by the ceph backend. The closest we come to it is the 'ceph:option=value...' in the Updated Commands section in doc/design-ceph-ganeti-support.rst. If you're feeling extra keen, you could add a list of all the supported options to that document. [snip] > 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! I don't think so, but it's not a problem. In Ganeti we arrange it so that you can mix and match daemons from versions x.y.z1 and x.y.z2 on the same cluster without problems, but x.y1 and x.y2 is not allowed and may break. The idea around this is to keep RPC stable enough to allow a certain amount of flexibility when rolling out bugfix 'point releases' to a cluster, but also to allow us to improve the RPC protocol over time. The RPC checking code verifies that it gets exactly the arguments it expects, no more and no less, so RPC changes that add arguments (or change argument types) are not compatible. However, you're adding functionality that will go into the next release, where we've no compatibility guarantees. Cheers, Brian.
