[Freeipa-devel] [PATCH 0224] cainstance: Read CS.cfg for preop.pin in a loop

[Nathaniel McCallum]

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.

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.

Nathaniel