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

[Jan Cholasta]

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.

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

Honza

-- 
Jan Cholasta