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

[Martin Kosek]

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