[Freeipa-devel] [PATCH] 1068 wait for LDAP when renewing the RA
Jan Cholasta
jcholast at redhat.com
Thu Nov 1 16:45:27 UTC 2012
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
More information about the Freeipa-devel
mailing list