[Freeipa-devel] [PATCH 0224] cainstance: Read CS.cfg for preop.pin in a loop
Martin Kosek
mkosek at redhat.com
Fri Jun 13 07:11:01 UTC 2014
On 06/12/2014 06:22 PM, Nathaniel McCallum wrote:
> On Thu, 2014-06-12 at 17:07 +0200, Tomas Babej wrote:
>> 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.
>
> Yup, understood. The leak pre-existed your modifications. But putting
> the leak into a loop makes it worth fixing.
>
> Looks good to me now! ACK
>
Thanks for review! Pushed to ipa-3-0: 1ec270e5d2fee90605578d04047d675a51318245
Martin
More information about the Freeipa-devel
mailing list