On 06/12/2014 06:22 PM, Nathaniel McCallum wrote:
> 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
> 

Thanks for review! Pushed to ipa-3-0: 1ec270e5d2fee90605578d04047d675a51318245

Martin

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to