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

[Rob Crittenden]

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

>
>
> No spaces in kwarg assignment please:
>
> +        tmpdir = tempfile.mkdtemp(prefix = "tmp-")

OK.

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

Sure, added that.

I also added a missing update. I added handling for ldap.SERVER_DOWN as 
a NetworkError instead of a DatabaseError.

rob

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-1068-2-renewal.patch
Type: text/x-diff
Size: 6625 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20121101/d1bdd67b/attachment.bin>