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