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

[Rob Crittenden]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-1068-3-renewal.patch
Type: text/x-diff
Size: 6661 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20121101/b4fe4887/attachment.bin>