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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel