[Freeipa-devel] [PATCH] 232 Increase service startup timeout default

[Jan Cholasta]

On 15.1.2014 12:17, Martin Kosek wrote:
> On 01/15/2014 11:20 AM, Alexander Bokovoy wrote:
>> On Wed, 15 Jan 2014, Jan Cholasta wrote:
>>> Hi,
>>>
>>> the attached patch should fix <https://fedorahosted.org/freeipa/ticket/4078>.
>>>
>>> I have also attached patch 179, which fixes a related bug in certificate
>>> renewal.
>>
>> NACK for this part:
>>> This fixes a possible NSS database corruption in renew_ca_cert.
>>> ---
>>> ipaserver/install/installutils.py | 3 ---
>>> 1 file changed, 3 deletions(-)
>>>
>>> diff --git a/ipaserver/install/installutils.py
>>> b/ipaserver/install/installutils.py
>>> index 67eabc2..0ba9c2e 100644
>>> --- a/ipaserver/install/installutils.py
>>> +++ b/ipaserver/install/installutils.py
>>> @@ -820,9 +820,6 @@ def stopped_service(service, instance_name=""):
>>>          root_logger.debug('Service %s%s is not running, continue.', service,
>>>                            log_instance_name)
>>>          yield
>>> -        root_logger.debug('Starting %s%s.', service, log_instance_name)
>>> -        ipaservices.knownservices[service].start(instance_name)
>>> -        return
>>>      else:
>>>          # Stop the service, do the required stuff and start it again
>>>          root_logger.debug('Stopping %s%s.', service, log_instance_name)
>> You need to wrap yield into try: finally: block. I have a patch for
>> similar case in private_cache() few lines above this code.

There is no cleanup needed for this particular yield. Also this is not 
really the concern of the patch, it does exactly what the commit message 
says. Since you are already fixing a similar case, I would suggest you 
amend your patch with any changes you deem necessary, I don't see why a 
single fix should be dispersed among multiple patches.

>>
>>>
>>> diff --git a/ipalib/constants.py b/ipalib/constants.py
>>> index d3e61ca..ae08277 100644
>>> --- a/ipalib/constants.py
>>> +++ b/ipalib/constants.py
>>> @@ -119,7 +119,7 @@ DEFAULT_CONFIG = (
>>>      ('rpc_protocol', 'jsonrpc'),
>>>
>>>      # Time to wait for a service to start, in seconds
>>> -    ('startup_timeout', 120),
>>> +    ('startup_timeout', 300),
>>>
>>>      # Web Application mount points
>>>      ('mount_ipa', '/ipa/'),
>> ACK for this one.
>>
>
>
> Additionally, shouldn't we make the renew_ca_cert script more robust and do the
> changes in LDAP datababase or certDB even if CA does not start and timeouts?
> (as indicated in #4078)
>
> IMO it is much easier for administrator to just start a CA manually, but with
> correct cert renewed, than figure out which part of renew procedure was not
> completed.
>
> I mean this part:
>
> # Done withing stopped_service context, CA restarted here
> update_cert_config(nickname, cert) <<< should not fail there if CA did not
> start, just report error and continue vvvvv

The CA should not be started here at all. Patch 179 fixes that.

>
> if nickname == 'subsystemCert cert-pki-ca':
>      update_people_entry('pkidbuser', cert)
>
> if nickname == 'auditSigningCert cert-pki-ca':
>      # Fix trust on the audit cert
>
> Martin
>


-- 
Jan Cholasta