Open Stack

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 <sean at 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 at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>



-- 
Rackspace Australia