[Freeipa-devel] [PATCH 0224] cainstance: Read CS.cfg for preop.pin in a loop
Nathaniel McCallum
npmccallum at redhat.com
Thu Jun 12 16:22:47 UTC 2014
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
More information about the Freeipa-devel
mailing list