Jan Cholasta wrote:
On 1.11.2012 16:54, Rob Crittenden wrote:
Jan Cholasta wrote:
On 1.11.2012 16:32, Rob Crittenden wrote:
Jan Cholasta wrote:
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:

Sure is, added.


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)

Sure.


The continue statement is redundant:

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

Yup. I used to have code executed after the try/except/finally.
Removed.


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)

Good catch, added.


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.')

It is included by syslog:

Nov  1 11:13:24 edsel renew_ra_cert: Connection to
ldap://localhost:7389
failed, sleeping 30

Yep, but it might be nice to show the full path to the script, since it
is not in $PATH.

Ok, added.

rob

Thanks.

Please also add the "conn = None" bit to the first loop:

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

and it's ACK.

Honza


Done, pushed to master and ipa-3-0

rob

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

Reply via email to