[Freeipa-devel] [PATCH 0224] cainstance: Read CS.cfg for preop.pin in a loop
Tomas Babej
tbabej at redhat.com
Thu Jun 12 14:36:29 UTC 2014
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).
> Nathaniel
>
>
--
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-3-cainstance-Read-CS.cfg-for-preop.pin-in-a-loop.patch
Type: text/x-patch
Size: 2860 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140612/d8661a37/attachment.bin>
More information about the Freeipa-devel
mailing list