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

[Rob Crittenden]

Jan Cholasta wrote:
> 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
>

Done, pushed to master and ipa-3-0

rob