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

[Jan Cholasta]

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

-- 
Jan Cholasta