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.

-- 
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org 

>From 99bfd7ee23983f0e1fc1225f40571d4d69679c38 Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Wed, 11 Jun 2014 11:06:03 +0200
Subject: [PATCH] cainstance: Read CS.cfg for preop.pin in a loop

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
---
 ipaserver/install/cainstance.py | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 4bf915030b83983323adec043066baa726089493..483ec676ff6fd42d1b8ffe10db34109b532df314 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -39,7 +39,7 @@ import syslog
 from ipapython import dogtag
 from ipapython.certdb import get_ca_nickname
 from ipapython import certmonger
-from ipalib import pkcs10, x509
+from ipalib import pkcs10, x509, api
 from ipalib import errors
 from ipapython.dn import DN
 import subprocess
@@ -108,20 +108,29 @@ def get_preop_pin(instance_root, instance_name):
 
     filename = instance_root + "/" + instance_name + "/conf/CS.cfg"
 
-    # 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')
+    # Retry to read the preop.pin several times before timing out
+    # due to possible race conditions
+    # https://fedorahosted.org/freeipa/ticket/3382
+
+    op_timeout = time.time() + api.env.startup_timeout
     pattern = re.compile("preop.pin=(.*)" )
-    for line in data:
-        match = re.search(pattern, line)
-        if (match):
-            preop_pin=match.group(1)
-            break
+
+    while preop_pin is None and time.time() < op_timeout:
+        try:
+            with open(filename, 'r') as f:
+                for line in f:
+                    match = re.search(pattern, line.rstrip('\n'))
+                    if match:
+                        preop_pin = match.group(1)
+                        break
+
+        except IOError, e:
+            root_logger.error("Cannot open configuration file." + str(e))
+        finally:
+            if preop_pin is None:
+                root_logger.debug("Unable to find preop.pin.")
+                root_logger.debug("Possible race condition. Waiting for 1 second.")
+                time.sleep(1)
 
     if preop_pin is None:
         raise RuntimeError("Unable to find preop.pin in %s. Is your CA already configured?" % filename)
-- 
1.9.3

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

Reply via email to