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

[Alexander Bokovoy]

On Wed, 15 Jan 2014, Jan Cholasta wrote:
>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.
Patch attached, it obsoletes your patch 179.

-- 
/ Alexander Bokovoy
-------------- next part --------------
>From a201bd425d988257769f096093198e16c23e7065 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <abokovoy at redhat.com>
Date: Wed, 15 Jan 2014 14:16:49 +0200
Subject: [PATCH 2/2] ipaserver/install/installutils: clean up properly after
 yield

When a context to which we yield generates exception, the code in
private_ccache() and stopped_service() didn't get called for cleanup.

Additionally, in stopped_service() one should not start a previously
stopped service after the context returned control to us because the
original state was with stopped service already.

Fixes https://fedorahosted.org/freeipa/ticket/4078
---
 ipaserver/install/installutils.py | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py
index cc53f75..67216f1 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -786,15 +786,16 @@ def private_ccache(path=None):
 
     os.environ['KRB5CCNAME'] = path
 
-    yield
-
-    if original_value is not None:
-        os.environ['KRB5CCNAME'] = original_value
-    else:
-        os.environ.pop('KRB5CCNAME')
+    try:
+        yield
+    finally:
+        if original_value is not None:
+            os.environ['KRB5CCNAME'] = original_value
+        else:
+            os.environ.pop('KRB5CCNAME')
 
-    if os.path.exists(path):
-        os.remove(path)
+        if os.path.exists(path):
+            os.remove(path)
 
 
 @contextmanager
@@ -820,13 +821,13 @@ 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)
         ipaservices.knownservices[service].stop(instance_name)
-        yield
-        root_logger.debug('Starting %s%s.', service, log_instance_name)
-        ipaservices.knownservices[service].start(instance_name)
+        try:
+            yield
+        finally:
+            root_logger.debug('Starting %s%s.', service, log_instance_name)
+            ipaservices.knownservices[service].start(instance_name)
-- 
1.8.4.2