IIRC nova solved this problem by adding a very simple wrapper utility around the oslo call. Couldn't cinder do the same?
Michael On Tue, Dec 3, 2013 at 6:05 PM, Sean Dague <s...@dague.net> wrote: > On 12/03/2013 06:13 PM, Ben Nemec wrote: >> On 2013-12-03 17:09, Sean Dague wrote: >>> On 12/03/2013 05:50 PM, Mark McLoughlin wrote: >>>> On Tue, 2013-12-03 at 16:23 -0600, Ben Nemec wrote: >>>>> On 2013-12-03 15:56, Sean Dague wrote: >>>>>> This cinder patch - https://review.openstack.org/#/c/48935/ >>>>>> >>>>>> Is blocked on failing upgrade because the updated oslo lockutils won't >>>>>> function until there is a specific configuration variable added to the >>>>>> cinder.conf. >>>>>> >>>>>> That work around is proposed here - >>>>>> https://review.openstack.org/#/c/52070/3 >>>>>> >>>>>> However I think this is exactly the kind of forward breaks that we >>>>>> want >>>>>> to prevent with grenade, as cinder failing to function after a rolling >>>>>> upgrade because a config item wasn't added is exactly the kind of pain >>>>>> we are trying to prevent happening to ops. >>>>>> >>>>>> So the question is, how is this done correctly so that a default >>>>>> can be >>>>>> set in the cinder code for this value, and it not require a config >>>>>> change to work? >>>> >>>> You're absolutely correct, in principle - if the default value for >>>> lock_path worked for users before, we should absolutely continue to >>>> support it. >>>> >>>>> I don't know that I have a good answer on how to handle this, but for >>>>> context this change is the result of a nasty bug in lockutils that >>>>> meant >>>>> external locks were doing nothing if lock_path wasn't set. Basically >>>>> it's something we should never have allowed in the first place. >>>>> >>>>> As far as setting this in code, it's important that all of the >>>>> processes >>>>> for a service are using the same value to avoid the same bad situation >>>>> we were in before. For tests, we have a lockutils wrapper >>>>> (https://github.com/openstack/oslo-incubator/blob/master/openstack/common/lockutils.py#L282) >>>>> that sets an environment variable to address this, but that only >>>>> works if all of the processes are going to be spawned from within >>>>> the same wrapper, and I'm not sure how secure that is for production >>>>> deployments since it puts all of the lock files in a temporary >>>>> directory. >>>> >>>> Right, I don't think the previous default really "worked" - if you used >>>> the default, then external locking was broken. >>>> >>>> I suspect most distros do set a default - I see RDO has this in its >>>> default nova.conf: >>>> >>>> lock_path = /var/lib/nova/tmp >>>> >>>> So, yes - this is all terrible. >>>> >>>> IMHO, rather than raise an exception we should log a big fat warning >>>> about relying on the default and perhaps just treat the lock as an >>>> in-process lock in that case ... since that's essentially what it was >>>> before, right? >>> >>> So a default of lock_path = /tmp will work (FHS says that path has to be >>> there), even if not optimal. Could we make it a default value like that >>> instead of the current default which is null (and hence the problem). >> >> IIRC, my initial fix was something similar to that, but it got shot down >> because putting the lock files in a known world writeable location was a >> security issue. >> >> Although maybe if we put them in a subdirectory of /tmp and ensured that >> the permissions were such that only the user running the service could >> use that directory, it might be acceptable? We could still log a >> warning if we wanted. >> >> This seems like it would have implications for people running services >> on Windows too, but we can probably find a way to make that work if we >> decide on a solution. > > How is that a security issue? Are the lock files being written with some > sensitive data in them and have g or o permissions on? The sticky bit > (+t) on /tmp will prevent other users from deleting the file. > > (from the chmod man page): > > RESTRICTED DELETION FLAG OR STICKY BIT > The restricted deletion flag or sticky bit is a single bit, whose > interpretation depends on the file type. For directories, it prevents > unprivi‐ > leged users from removing or renaming a file in the directory > unless they own the file or the directory; this is called the restricted > deletion > flag for the directory, and is commonly found on world-writable > directories like /tmp. For regular files on some older systems, the bit > saves the > program's text image on the swap device so it will load more > quickly when run; this is called the sticky bit. > > > -Sean > > -- > Sean Dague > http://dague.net > > > _______________________________________________ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > -- Rackspace Australia _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
