[Freeipa-devel] [PATCH] 1068 wait for LDAP when renewing the RA

[Jan Cholasta]

Hi,

On 24.10.2012 21:24, Rob Crittenden wrote:
> All the certs are pretty critical in certificate renewal but the agent
> cert has the distinction of having to be updated in multiple places. It
> needs to exist in both LDAP servers.
>
> It is possible that one or both of these servers may be down briefly
> during renewal so we need to be a bit more robust in our handling. This
> will wait up to 5 minutes per server to try to update things, and syslog
> when failures occur.
>
> It is now also safe to re-run this in case something catastrophic
> happens. One would just need to manually run this to load the required
> data into LDAP.
>
> rob
>

I believe that there should be a break statement after the "updated = 
True" line:

+        updated = True
+    except errors.NetworkError:


It would be nice if this message said "30 s" instead of just "30":

+        syslog.syslog(syslog.LOG_ERR, 'Connection to %s failed, 
sleeping 30' % dogtag_uri)


The continue statement is redundant:

+        attempts += 1
+        continue
      except errors.EmptyModlist:


The variables you access in both of the finally blocks (conn and tmpdir) 
may be unbound. This can be fixed like this:

while attempts < 10:
     conn = None
     tmpdir = None
     try:
         ...
     finally:
         if conn is not None and conn.isconnected():
             conn.disconnect()
         if tmpdir is not None:
             shutil.rmtree(tmpdir)


It would be nice if this message (both instances of it) included 
sys.argv[0], so that it is obvious to the user what script is "this script":

+    syslog.syslog(syslog.LOG_ERR, 'Giving up. This script may be safely 
re-executed.')


No spaces in kwarg assignment please:

+        tmpdir = tempfile.mkdtemp(prefix = "tmp-")


You might want to include "sleeping 30 s" in this message as well:

+    except Exception, e:
+        syslog.syslog(syslog.LOG_ERR, 'Updating renewal certificate 
failed: %s' % e)
+        time.sleep(30)


Honza

-- 
Jan Cholasta