[Freeipa-devel] [PATCH 0224] cainstance: Read CS.cfg for preop.pin in a loop
Tomas Babej
tbabej at redhat.com
Thu Jun 12 15:07:57 UTC 2014
On 06/12/2014 04:45 PM, Nathaniel McCallum wrote:
> On Thu, 2014-06-12 at 16:36 +0200, Tomas Babej wrote:
>> On 06/12/2014 04:27 PM, Nathaniel McCallum wrote:
>>> On Thu, 2014-06-12 at 16:20 +0200, Martin Kosek wrote:
>>>> On 06/12/2014 03:15 PM, Tomas Babej wrote:
>>>>> On 06/12/2014 02:37 PM, Nathaniel McCallum wrote:
>>>>>> On Thu, 2014-06-12 at 13:29 +0200, Tomas Babej wrote:
>>>>>>> On 06/12/2014 10:45 AM, Martin Kosek wrote:
>>>>>>>> On 06/11/2014 06:49 PM, Nathaniel McCallum wrote:
>>>>>>>>> On Wed, 2014-06-11 at 11:08 +0200, Tomas Babej wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> As due to possible race conditions, the preop.pin might not be
>>>>>>>>>> written in the CS.cfg at the time installer tries to read it.
>>>>>>>>>>
>>>>>>>>>> In case no value for preop.pin was found, retry until timeout
>>>>>>>>>> was reached.
>>>>>>>>>>
>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/3382
>>>>>>>>>>
>>>>>>>>>> (applies on ipa-3-0 branch)
>>>>>>>>> There is inconsistent spacing around '='. For instance:
>>>>>>>>> + f=open(filename, 'r')
>>>>>>>>> + data = f.read()
>>>>>>>>>
>>>>>>>>> Also, I'm fairly certain this is incorrect:
>>>>>>>>> + total_timeout =+ 1
>>>>>>>>>
>>>>>>>>> Nathaniel
>>>>>>>> +1, this is certainly is not a deterministic, constant measuring of a timeout.
>>>>>>> Fixed.
>>>>>>>
>>>>>>>> I would rather see something like
>>>>>>>>
>>>>>>>> op_timeout = time.time() + api.env.startup_timeout
>>>>>>>>
>>>>>>>> while preop_pin is None and time.time() < op_timeout:
>>>>>>>>
>>>>>>>>
>>>>>>>> Also, I do not think this will work, IOError is risen when a file is not found,
>>>>>>>> so this code would not solve the problem if waiting on file to appear.
>>>>>>> Yes, but the problem was not in being unable to open the file, but that
>>>>>>> certain file content
>>>>>>> is not in the file yet. It seems that pkicreate creates the file in
>>>>>>> time, but the content written
>>>>>>> later and that causes the race condition.
>>>>>>>
>>>>>>> If you inspect the original code, and bugzilla, you can see that
>>>>>>> installation fails with:
>>>>>>>
>>>>>>> 2013-01-25T02:58:44Z INFO The ipa-server-install command failed, exception:
>>>>>>> RuntimeError: Unable to find preop.pin in /var/lib/pki-ca/conf/CS.cfg. Is your CA already configured?
>>>>>>>
>>>>>>> While the original code
>>>>>>>
>>>>>>> # read the config file and get the preop pin
>>>>>>>
>>>>>>> try:
>>>>>>>
>>>>>>>
>>>>>>> f=open(filename)
>>>>>>>
>>>>>>> except IOError,
>>>>>>> e:
>>>>>>>
>>>>>>> root_logger.error("Cannot open configuration file." +
>>>>>>> str(e))
>>>>>>> raise
>>>>>>> e
>>>>>>>
>>>>>>> data =
>>>>>>> f.read()
>>>>>>>
>>>>>>> data =
>>>>>>> data.split('\n')
>>>>>>>
>>>>>>> pattern = re.compile("preop.pin=(.*)"
>>>>>>> )
>>>>>>> for line in
>>>>>>> data:
>>>>>>>
>>>>>>> match = re.search(pattern,
>>>>>>> line)
>>>>>>> if
>>>>>>> (match):
>>>>>>>
>>>>>>>
>>>>>>> preop_pin=match.group(1)
>>>>>>>
>>>>>>> break
>>>>>>> if preop_pin is None:
>>>>>>> raise RuntimeError("Unable to find preop.pin in %s. Is your CA
>>>>>>> already configured?" % filename)
>>>>>>>
>>>>>>> does raise the IOError in case the file was not able to be opened. Since
>>>>>>> we get the Runtime error,
>>>>>>> file must exist, only the desired content is missing.
>>>>>>>
>>>>>>> That said, I have no objections to amending the patch so that it
>>>>>>> properly handles this race condition
>>>>>>> we did not encounter. Better safe than sorry, it's a simple change and
>>>>>>> already included in the
>>>>>>> current iteration of the patch.
>>>>>>>
>>>>>>> Updated patch attached.
>>>>>> I think I would prefer something like inotify watching for
>>>>>> IN_CLOSE_WRITE rather than this polling.
>>>>>>
>>>>>> Nathaniel
>>>>>>
>>>>> Wouldn't that be a little bit of an overkill for that purpose?
>>>> If would. Nathaniel, this is a fix for ipa-3-0 branch only, we do not have this
>>>> problem in current master as this issue only affects Dogtag 9 installation.
>>>>
>>>> The target is to have small, contained and the non-intrusive patch. Thus the
>>>> proposed solution to just implement a little wait loop.
>>>>
>>>>> - we'll need to introduce an additional dependency for python-inotify
>>>>> - watching for IN_CLOSE_WRITE is not equivalent with the preop_pin
>>>>> written to CS.cfg (and we don't know that unless we inspect the code for
>>>>> pkicreate, which in turn would make our code dependant on code not
>>>>> located in our codebase), so we will still need to check if preop_pin is
>>>>> there, and loop over if not
>>> Sorry, I forgot this was for 3.0 branch.
>>>
>>> Nitpick: '=' spacing is still not fixed.
>> I fixed the spacing.
>>
>>> Also, is it your intention to keep looping through the loop with no
>>> delay on IOError? If there is a race condition where this file isn't
>>> created yet, you could very likely get log spam.
>> Read the patch more carefully please. There is delay on IOError
>> (preop_pin is still
>> None, since we jumped out of the try block when the exception occured at the
>> opening of the file and therefore had no chance to set it to something
>> else than None).
> Ah, good point.
>
> One more issue. In the case of an exception during f.read(), the file
> descriptor is leaked (in a loop). Why not do something like (is 3.x's
> python new enough for with?):
>
> with open(filename, 'r') as f:
> data = f.read()
> ...
>
> In any case, the file can be closed immediately after the read().
>
> Nathaniel
>
It is (available from python 2.6).
I did not mean to refactor the old code, but you raise a point so I did
convert the block to somewhat more pythonic code.
--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0224-4-cainstance-Read-CS.cfg-for-preop.pin-in-a-loop.patch
Type: text/x-patch
Size: 2743 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140612/99ecb111/attachment.bin>
More information about the Freeipa-devel
mailing list