[Freeipa-devel] [PATCH] 1014 configurable service timeout

[Rob Crittenden]

Martin Kosek wrote:
> On 07/02/2012 03:57 PM, Rob Crittenden wrote:
>> Martin Kosek wrote:
>>> On 06/29/2012 07:52 PM, Rob Crittenden wrote:
>>>> Martin Kosek wrote:
>>>>> On 05/29/2012 04:31 PM, Rob Crittenden wrote:
>>>>>> Martin Kosek wrote:
>>>>>>> On Thu, 2012-05-24 at 11:38 -0400, Rob Crittenden wrote:
>>>>>>>> Petr Viktorin wrote:
>>>>>>>>> On 05/18/2012 10:03 PM, Rob Crittenden wrote:
>>>>>>>>>> Rob Crittenden wrote:
>>>>>>>>>>> A hardcoded timeout was used in ipactl for service restarts, set rather
>>>>>>>>>>> low. A separate timeout was hardcoded into the installer.
>>>>>>>>>>>
>>>>>>>>>>> I centralized them into a single timeout, configurable in the standard
>>>>>>>>>>> way in /etc/ipa/*.conf.
>>>>>>>>>>>
>>>>>>>>>>> On install it will always default to 120 seconds and remain there unless
>>>>>>>>>>> changed in default.conf (not replicated either).
>>>>>>>>>>>
>>>>>>>>>>> I tested this on systemd systems and sysV systems and it works ok for
>>>>>>>>>>> me. You'll also want to double-check that this works when other 389-ds
>>>>>>>>>>> instances are installed.
>>>>>>>>>>>
>>>>>>>>>>> Getting the naming of instances right was a bit tricky.
>>>>>>>>>>
>>>>>>>>>> Noticed a problem on upgrades and fixed that. Updated patch attached.
>>>>>>>>>>
>>>>>>>>>> rob
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Please rebase the patch onto current master.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> Done
>>>>>>>
>>>>>>> This is a good start. I just found few places where I found that the
>>>>>>> remaining wait function calls are redundant:
>>>>>>>
>>>>>>> 1) install/tools/ipactl:
>>>>>>>
>>>>>>>             if lurl.urlscheme == 'ldapi':
>>>>>>> -            wait_for_open_socket(lurl.hostport, timeout=6)
>>>>>>> +            wait_for_open_socket(lurl.hostport,
>>>>>>> timeout=api.env.startup_timeout)
>>>>>>>              else:
>>>>>>>                  (host,port) = lurl.hostport.split(':')
>>>>>>> -            wait_for_open_ports(host, [int(port)], timeout=6)
>>>>>>> +            wait_for_open_ports(host, [int(port)],
>>>>>>> timeout=api.env.startup_timeout)
>>>>>>>
>>>>>>> Aren't these calls redundant? We already wait for ports when dirsrv is
>>>>>>> started (dirsrv.start()) or restarted (dirsrv.restart()).
>>>>>>
>>>>>> It is redundant in some cases but there are some calls we make where this is
>>>>>> used to determine the availability of the service. This call is needed.
>>>>>>
>>>>>>> 2) ipaserver/install/replication.py:
>>>>>>> -        installutils.wait_for_open_ports('localhost', [389, 636], 300)
>>>>>>> +        ipautil.wait_for_open_ports('localhost', [389, 636], 300)
>>>>>>>
>>>>>>> Isn't this now redundant? Port check should be done in service restart.
>>>>>>
>>>>>> Yes, looks like this call can go.
>>>>>>
>>>>>>> 3) ipaserver/install/plugins/updateclient.py:
>>>>>>>
>>>>>>> -            installutils.wait_for_open_socket(socket_name)
>>>>>>> +            wait_for_open_socket(socket_name)
>>>>>>>
>>>>>>> Also seems redundant, dirsrv should be already up as it was restarted
>>>>>>> via our Service framework. Though we only check for ports in the Service
>>>>>>> framework, I wonder if this is enough and we can be sure that when ports
>>>>>>> are up, the LDAPI socket is also up.
>>>>>>
>>>>>> No, sockets and ports are separate, particularly when updating. In fact, we
>>>>>> disable the ports so a wait_for_port() will always fail which is why I added
>>>>>> the wait flag. This may be a case I missed with upgrades. Let me test
>>>>>> upgrades
>>>>>> again...
>>>>>>
>>>>>> rob
>>>>>
>>>>> I think we want to either send a revised patch to this ticket to get it to
>>>>> Beta
>>>>> 1 or to defer it to some future version...
>>>>>
>>>>> Martin
>>>>>
>>>>
>>>> Here is a rebased patch.
>>>>
>>>> rob
>>>
>>>
>>> This rather looks as a SELinux user map test patch...
>>>
>>> Martin
>>
>> Ouch, off-by-one error. This is the right one.
>>
>> rob
>
> I did not find any further issue, so ACK.
>
> Martin
>

pushed to master