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.

>
> Martin

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

>From 7a3750fec43b5988f181ecdcf6ade892d3ee76a3 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 | 45 +++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 4bf915030b83983323adec043066baa726089493..27b7e5b2c7e759169fe74eda7d899487a7682d68 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,35 @@ 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')
-    pattern = re.compile("preop.pin=(.*)" )
-    for line in data:
-        match = re.search(pattern, line)
-        if (match):
-            preop_pin=match.group(1)
-            break
+    # 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
+
+    while preop_pin is None and time.time() < op_timeout:
+        try:
+            f=open(filename, 'r')
+
+            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
+
+            f.close()
+
+        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