Re: [Freeipa-devel] [PATCH 0043] Stop uninstaller from failing if a service can't be started

[Stanislav Laznicka]

On 06/24/2016 04:04 PM, Martin Basti wrote:

On 24.06.2016 15:50, Stanislav Laznicka wrote:

On 06/21/2016 04:39 PM, Martin Basti wrote:



On 14.06.2016 17:26, Stanislav Laznicka wrote:

-signerd_service.start()
+try:
+signerd_service.start()
+except Exception as e:
+root_logger.error("Unable to start '{svcname}': 
{err}"

+ .format(svcname=signerd_service.service_name,
+  err=e))


why is signerd so special?

Martin^2


From ODSExporterInstance.uninstall():

signerd_service = services.knownservices.ods_signerd

This means that signerd_service here is not an instance of the 
service.Service class or of its child class but is rather an instance 
of the RedHatService class, a child class of the 
services.SystemdService class. Thus it has to be treated with special 
care.




Well then I prefer to put this option to systemdservice, and only pass 
it from service.restart() to systemdService.restart()


You forgot to handle regular_named service, which is the same as 
ods_signerd


The passing of the option is not as easy as it may seem. The 
service.restart() method calls self.service.restart(). However, 
self.service is not directly of SystemdService class but rather of one 
of RedHat*Service classes, some of which override the 
SystemdService.restart() method (and call it somewhere in the process of 
their overridden method).


Would you then suggest:
1) Adding the option to all the methods on the way from Service to 
SystemdService classes just for the sake of passing => exceptions still 
may occur and suppress_errors option therefore does not do what you'd 
expect it to do
2) Adding the option to all the methods and treat all the possible 
exceptions on the way with a try: .. except: .. blocks ===> veeery ugly 
copy-pasta.
3) Leaving it as is => some .restart() calls offer suppress_errors 
option, some don't.


--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0043] Stop uninstaller from failing if a service can't be started

[Stanislav Laznicka]

On 06/21/2016 04:39 PM, Martin Basti wrote:



On 14.06.2016 17:26, Stanislav Laznicka wrote:

-signerd_service.start()
+try:
+signerd_service.start()
+except Exception as e:
+root_logger.error("Unable to start '{svcname}': {err}"
+ .format(svcname=signerd_service.service_name,
+  err=e))


why is signerd so special?

Martin^2


From ODSExporterInstance.uninstall():

signerd_service = services.knownservices.ods_signerd

This means that signerd_service here is not an instance of the 
service.Service class or of its child class but is rather an instance of 
the RedHatService class, a child class of the services.SystemdService 
class. Thus it has to be treated with special care.


--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0043] Stop uninstaller from failing if a service can't be started

[Martin Basti]

On 14.06.2016 17:26, Stanislav Laznicka wrote:

-signerd_service.start()
+try:
+signerd_service.start()
+except Exception as e:
+root_logger.error("Unable to start '{svcname}': {err}"
+  .format(svcname=signerd_service.service_name,
+  err=e))


why is signerd so special?

Martin^2

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0043] Stop uninstaller from failing if a service can't be started

[Stanislav Laznicka]

On 06/14/2016 09:25 AM, Stanislav Laznicka wrote:

On 06/13/2016 02:51 PM, Martin Babinsky wrote:

On 06/07/2016 10:14 AM, Stanislav Laznicka wrote:

https://fedorahosted.org/freeipa/ticket/5775



Umm, wouldn't it be better to augment the `Service.start()/restart()` 
methods themselves with parameters that will suppress exception 
raising and log an error instead of copy-pasting try: ... except: 
blocks?


A good point. SystemdService start()/restart() methods will need 
augmenting as well (signerd service) but I suppose that's about it for 
this issue. I'll send a patch updated accordingly.


Actually, adding an argument to SystemdService's start()/restart() 
methods turned out to be very, very bad idea for this purpose (it would 
end up with way worse copy-paste than the original patch). Attached is 
probably the most minimal solution.
From 33f9b79a68431b54e3a047f3f4a67dec5554803c Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Tue, 14 Jun 2016 17:17:37 +0200
Subject: [PATCH] Uninstaller won't fail if service can't be started

https://fedorahosted.org/freeipa/ticket/5775
---
 ipaserver/install/bindinstance.py|  2 +-
 ipaserver/install/httpinstance.py|  2 +-
 ipaserver/install/krbinstance.py |  2 +-
 ipaserver/install/ntpinstance.py |  2 +-
 ipaserver/install/odsexporterinstance.py |  7 ++-
 ipaserver/install/opendnssecinstance.py  |  2 +-
 ipaserver/install/service.py | 28 +++-
 7 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index 78e75359266bbefe7954242b98922272fb0c9194..b9b9a7d7f3bef5fb461182e23a4b45d39a3f0405 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -1273,7 +1273,7 @@ class BindInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+self.restart(suppress_errors=True)
 
 self.named_regular.unmask()
 if named_regular_enabled:
diff --git a/ipaserver/install/httpinstance.py b/ipaserver/install/httpinstance.py
index 00f890175ae583f485797da6f913a7f83b302df3..cd81bbed3830e239b6cf50f765f176547ee788fa 100644
--- a/ipaserver/install/httpinstance.py
+++ b/ipaserver/install/httpinstance.py
@@ -549,7 +549,7 @@ class HTTPInstance(service.Service):
 self.print_msg('WARNING: ' + str(e))
 
 if running:
-self.restart()
+self.restart(suppress_errors=True)
 
 # disabled by default, by ldap_enable()
 if enabled:
diff --git a/ipaserver/install/krbinstance.py b/ipaserver/install/krbinstance.py
index f560a6ec4c2e4ce931cc1552976db5900a3fa5cd..61d02f9fd346d7b78e5d8682038d9b1a1a149c74 100644
--- a/ipaserver/install/krbinstance.py
+++ b/ipaserver/install/krbinstance.py
@@ -405,7 +405,7 @@ class KrbInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+self.restart(suppress_errors=True)
 
 self.kpasswd = KpasswdInstance()
 self.kpasswd.uninstall()
diff --git a/ipaserver/install/ntpinstance.py b/ipaserver/install/ntpinstance.py
index 8b0f0e5395dae3c058fc31bd8914741e4d158830..8b2b1c1ec5102355eb6461bcfa5b9985ba02bc12 100644
--- a/ipaserver/install/ntpinstance.py
+++ b/ipaserver/install/ntpinstance.py
@@ -183,4 +183,4 @@ class NTPInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+self.restart(suppress_errors=True)
diff --git a/ipaserver/install/odsexporterinstance.py b/ipaserver/install/odsexporterinstance.py
index e9f7bf853d98237aa19aace384b8ff7021c3a85a..4625df81988ba654ac844f93812fc6802b48dee8 100644
--- a/ipaserver/install/odsexporterinstance.py
+++ b/ipaserver/install/odsexporterinstance.py
@@ -193,7 +193,12 @@ class ODSExporterInstance(service.Service):
 signerd_service.enable()
 
 if signerd_running:
-signerd_service.start()
+try:
+signerd_service.start()
+except Exception as e:
+root_logger.error("Unable to start '{svcname}': {err}"
+  .format(svcname=signerd_service.service_name,
+  err=e))
 
 installutils.remove_keytab(paths.IPA_ODS_EXPORTER_KEYTAB)
 installutils.remove_ccache(ccache_path=paths.IPA_ODS_EXPORTER_CCACHE)
diff --git a/ipaserver/install/opendnssecinstance.py b/ipaserver/install/opendnssecinstance.py
index f0c512ba04129d08b5874f58c7a25620f7435b2a..c4de0eb4614c235981840db0041a62f4db42ccef 100644
--- a/ipaserver/install/opendnssecinstance.py
+++ b/ipaserver/install/opendnssecinstance.py
@@ -386,4 +386,4 @@ class OpenDNSSECInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+self.restart(suppress_errors=True)
diff --git a/ipaserver/install/service.py 

Re: [Freeipa-devel] [PATCH 0043] Stop uninstaller from failing if a service can't be started

[Stanislav Laznicka]

On 06/13/2016 02:51 PM, Martin Babinsky wrote:

On 06/07/2016 10:14 AM, Stanislav Laznicka wrote:

https://fedorahosted.org/freeipa/ticket/5775



Umm, wouldn't it be better to augment the `Service.start()/restart()` 
methods themselves with parameters that will suppress exception 
raising and log an error instead of copy-pasting try: ... except: blocks?


A good point. SystemdService start()/restart() methods will need 
augmenting as well (signerd service) but I suppose that's about it for 
this issue. I'll send a patch updated accordingly.


--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0043] Stop uninstaller from failing if a service can't be started

[Martin Babinsky]

On 06/07/2016 10:14 AM, Stanislav Laznicka wrote:

https://fedorahosted.org/freeipa/ticket/5775



Umm, wouldn't it be better to augment the `Service.start()/restart()` 
methods themselves with parameters that will suppress exception raising 
and log an error instead of copy-pasting try: ... except: blocks?


--
Martin^3 Babinsky

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0043] Stop uninstaller from failing if a service can't be started

[Florence Blanc-Renaud]

On 06/07/2016 10:14 AM, Stanislav Laznicka wrote:

https://fedorahosted.org/freeipa/ticket/5775




LGTM,
ACK
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code