Re: [Freeipa-devel] [PATCH] 1074 limit service list
Simo Sorce wrote: On Tue, 2012-12-04 at 21:32 -0500, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2012-12-04 at 15:56 -0500, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2012-12-04 at 15:14 -0500, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2012-12-04 at 14:03 -0500, Rob Crittenden wrote: Only touch the service list in the server installer and ipactl. Nack, comments inline. [..] This break the fallback we have in ipa_stop() We expect an exception or a non empty list there. Ok, I can move the handling so ipactl ignores the exception. Is the problem that we are printing an error to stdout/stderr ? Or do you actually want to change behavior somehow ? We need to change the behavior. If you run: ipactl stop then: ipactl status you get a backtrace because the service list doesn't exist. Ok, what about defining our own exception and then simply pass on it except for the stop() case that treats it differently ? I think checking for file existence is probably going to hit the majority of the cases. Using a separate exception is probably overkill. ACK Simo. pushed to master and ipa-3-0 rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1074 limit service list
On Tue, 2012-12-04 at 14:03 -0500, Rob Crittenden wrote: Only touch the service list in the server installer and ipactl. Nack, comments inline. diff --git a/install/tools/ipactl b/install/tools/ipactl index f931a27257aaca987db46c7295cbb4708a6801f7..2a60b9eaf4e1ffb536fd389d17ff747c99492a35 100755 --- a/install/tools/ipactl +++ b/install/tools/ipactl @@ -175,6 +175,9 @@ def get_config_from_file(): svc_list = [] +if not os.path.exists(ipaservices.get_svc_list_file()): +return svc_list + This break the fallback we have in ipa_stop() We expect an exception or a non empty list there. try: f = open(ipaservices.get_svc_list_file(), 'r') svc_list = json.load(f) @@ -469,7 +472,7 @@ def main(): else: raise e -api.bootstrap(context='cli', debug=options.debug) +api.bootstrap(context='ipactl', debug=options.debug) api.finalize() if '.' not in api.env.host: diff --git a/ipapython/platform/base.py b/ipapython/platform/base.py index 8385e1038c0609ae06a7a4a25d844de48360f19e..d1d42b3259d735d88df4e9c1698d5f8781dd1124 100644 --- a/ipapython/platform/base.py +++ b/ipapython/platform/base.py @@ -136,12 +136,15 @@ class PlatformService(object): def __init__(self, service_name): self.service_name = service_name -def start(self, instance_name=, capture_output=True, wait=True): +def start(self, instance_name=, capture_output=True, wait=True, +update_list=True): Can we call this something like 'store_action' or 'remember_action' ? 'update_list' is quite opaque as name. Or maybe at least qualify: 'update_stop_list' When a service is started record the fact in a special file. This allows ipactl stop to always stop all services that have been started via ipa tools +if not update_list: +return svc_list = [] try: f = open(SVC_LIST_FILE, 'r') @@ -159,10 +162,12 @@ class PlatformService(object): f.close() return -def stop(self, instance_name=, capture_output=True): +def stop(self, instance_name=, capture_output=True, update_list=True): When a service is stopped remove it from the service list file. +if not update_list: +return svc_list = [] try: f = open(SVC_LIST_FILE, 'r') diff --git a/ipapython/platform/systemd.py b/ipapython/platform/systemd.py index bb6c009299adc9ca8488308afffdd767975fc2ae..359b593594f2db2b1a1810abbd71deebbf33677e 100644 --- a/ipapython/platform/systemd.py +++ b/ipapython/platform/systemd.py @@ -91,13 +91,21 @@ class SystemdService(base.PlatformService): def stop(self, instance_name=, capture_output=True): ipautil.run([/bin/systemctl, stop, self.service_instance(instance_name)], capture_output=capture_output) -super(SystemdService, self).stop(instance_name) +if 'context' in api.env and api.env.context in ['ipactl', 'installer']: Will this trigger also when ipa-client-install is run ? We have a patch on the list to restart sssd via ipa-client-install. sssd *should* not end in the stop-list though. +update_list = True +else: +update_list = False +super(SystemdService, self).stop(instance_name,update_list=update_list) def start(self, instance_name=, capture_output=True, wait=True): ipautil.run([/bin/systemctl, start, self.service_instance(instance_name)], capture_output=capture_output) +if 'context' in api.env and api.env.context in ['ipactl', 'installer']: +update_list = True +else: +update_list = False if wait and self.is_running(instance_name): self.__wait_for_open_ports(self.service_instance(instance_name)) -super(SystemdService, self).start(instance_name) +super(SystemdService, self).start(instance_name, update_list=update_list) def restart(self, instance_name=, capture_output=True, wait=True): # Restart command is broken before systemd-36-3.fc16 In general looks good, otherwise. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1074 limit service list
On Tue, 2012-12-04 at 15:14 -0500, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2012-12-04 at 14:03 -0500, Rob Crittenden wrote: Only touch the service list in the server installer and ipactl. Nack, comments inline. [..] This break the fallback we have in ipa_stop() We expect an exception or a non empty list there. Ok, I can move the handling so ipactl ignores the exception. Is the problem that we are printing an error to stdout/stderr ? Or do you actually want to change behavior somehow ? [..] Can we call this something like 'store_action' or 'remember_action' ? 'update_list' is quite opaque as name. Or maybe at least qualify: 'update_stop_list' Yes, I'm not completely happy with the variable name either. How about update_service_list? Sounds ok. [..] Will this trigger also when ipa-client-install is run ? We have a patch on the list to restart sssd via ipa-client-install. sssd *should* not end in the stop-list though. No, the only services we care about for ipactl are those started by the server itself. I don't think a user would expect that certmonger, messagebus, sssd, etc would stop if they executed ipactl stop. Yes this is what I am saying, so ipa-client-install is identified as 'cli' and not as 'installer' I guess ? [..] I'll work up a new patch soon. Thanks, Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1074 limit service list
Simo Sorce wrote: On Tue, 2012-12-04 at 15:14 -0500, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2012-12-04 at 14:03 -0500, Rob Crittenden wrote: Only touch the service list in the server installer and ipactl. Nack, comments inline. [..] This break the fallback we have in ipa_stop() We expect an exception or a non empty list there. Ok, I can move the handling so ipactl ignores the exception. Is the problem that we are printing an error to stdout/stderr ? Or do you actually want to change behavior somehow ? We need to change the behavior. If you run: ipactl stop then: ipactl status you get a backtrace because the service list doesn't exist. [..] Can we call this something like 'store_action' or 'remember_action' ? 'update_list' is quite opaque as name. Or maybe at least qualify: 'update_stop_list' Yes, I'm not completely happy with the variable name either. How about update_service_list? Sounds ok. [..] Will this trigger also when ipa-client-install is run ? We have a patch on the list to restart sssd via ipa-client-install. sssd *should* not end in the stop-list though. No, the only services we care about for ipactl are those started by the server itself. I don't think a user would expect that certmonger, messagebus, sssd, etc would stop if they executed ipactl stop. Yes this is what I am saying, so ipa-client-install is identified as 'cli' and not as 'installer' I guess ? [..] Exactly. When the IPA api is bootstrapped you supply a context name. This is generally unique in the IPA universe and different names make things do different things. I'll work up a new patch soon. Thanks, Simo. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1074 limit service list
On Tue, 2012-12-04 at 15:56 -0500, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2012-12-04 at 15:14 -0500, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2012-12-04 at 14:03 -0500, Rob Crittenden wrote: Only touch the service list in the server installer and ipactl. Nack, comments inline. [..] This break the fallback we have in ipa_stop() We expect an exception or a non empty list there. Ok, I can move the handling so ipactl ignores the exception. Is the problem that we are printing an error to stdout/stderr ? Or do you actually want to change behavior somehow ? We need to change the behavior. If you run: ipactl stop then: ipactl status you get a backtrace because the service list doesn't exist. Ok, what about defining our own exception and then simply pass on it except for the stop() case that treats it differently ? Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1074 limit service list
Simo Sorce wrote: On Tue, 2012-12-04 at 15:56 -0500, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2012-12-04 at 15:14 -0500, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2012-12-04 at 14:03 -0500, Rob Crittenden wrote: Only touch the service list in the server installer and ipactl. Nack, comments inline. [..] This break the fallback we have in ipa_stop() We expect an exception or a non empty list there. Ok, I can move the handling so ipactl ignores the exception. Is the problem that we are printing an error to stdout/stderr ? Or do you actually want to change behavior somehow ? We need to change the behavior. If you run: ipactl stop then: ipactl status you get a backtrace because the service list doesn't exist. Ok, what about defining our own exception and then simply pass on it except for the stop() case that treats it differently ? I think checking for file existence is probably going to hit the majority of the cases. Using a separate exception is probably overkill. rob From 8cddaf2468b883685f711d50e47d69ccd8d1a89b Mon Sep 17 00:00:00 2001 From: Rob Crittenden rcrit...@redhat.com Date: Tue, 4 Dec 2012 13:55:30 -0500 Subject: [PATCH] Only update the list of running services in the installer or ipactl. The file is only present in the case of a server installation. It should only be touched by the server installer and ipactl. https://fedorahosted.org/freeipa/ticket/3277 --- install/tools/ipactl | 10 -- ipapython/platform/base.py| 9 +++-- ipapython/platform/systemd.py | 12 ++-- 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/install/tools/ipactl b/install/tools/ipactl index f931a27257aaca987db46c7295cbb4708a6801f7..67a9ee4486ffd5ce09fbff3426bd6d046b68f8b6 100755 --- a/install/tools/ipactl +++ b/install/tools/ipactl @@ -416,7 +416,10 @@ def ipa_status(options): try: svc_list = get_config_from_file() except IpactlError, e: -raise e +if os.path.exists(ipaservices.get_svc_list_file()): +raise e +else: +svc_list = [] except Exception, e: raise IpactlError(Failed to get list of services to probe status: + str(e)) @@ -426,6 +429,9 @@ def ipa_status(options): print Directory Service: RUNNING else: print Directory Service: STOPPED +if len(svc_list) == 0: +print (Directory Service must be running in order to + + obtain status of other services) except: raise IpactlError(Failed to get Directory Service status) @@ -469,7 +475,7 @@ def main(): else: raise e -api.bootstrap(context='cli', debug=options.debug) +api.bootstrap(context='ipactl', debug=options.debug) api.finalize() if '.' not in api.env.host: diff --git a/ipapython/platform/base.py b/ipapython/platform/base.py index 8385e1038c0609ae06a7a4a25d844de48360f19e..41a9c83e703512583d6ad3afcb1c87337f575926 100644 --- a/ipapython/platform/base.py +++ b/ipapython/platform/base.py @@ -136,12 +136,15 @@ class PlatformService(object): def __init__(self, service_name): self.service_name = service_name -def start(self, instance_name=, capture_output=True, wait=True): +def start(self, instance_name=, capture_output=True, wait=True, +update_service_list=True): When a service is started record the fact in a special file. This allows ipactl stop to always stop all services that have been started via ipa tools +if not update_service_list: +return svc_list = [] try: f = open(SVC_LIST_FILE, 'r') @@ -159,10 +162,12 @@ class PlatformService(object): f.close() return -def stop(self, instance_name=, capture_output=True): +def stop(self, instance_name=, capture_output=True, update_service_list=True): When a service is stopped remove it from the service list file. +if not update_service_list: +return svc_list = [] try: f = open(SVC_LIST_FILE, 'r') diff --git a/ipapython/platform/systemd.py b/ipapython/platform/systemd.py index bb6c009299adc9ca8488308afffdd767975fc2ae..4e8a03f2f1ce06371c8252e3c7d2071089bebba9 100644 --- a/ipapython/platform/systemd.py +++ b/ipapython/platform/systemd.py @@ -91,13 +91,21 @@ class SystemdService(base.PlatformService): def stop(self, instance_name=, capture_output=True): ipautil.run([/bin/systemctl, stop, self.service_instance(instance_name)], capture_output=capture_output) -super(SystemdService, self).stop(instance_name) +if 'context' in api.env and api.env.context in ['ipactl', 'installer']: +update_service_list = True +else: +update_service_list = False +super(SystemdService,
Re: [Freeipa-devel] [PATCH] 1074 limit service list
On Tue, 2012-12-04 at 21:32 -0500, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2012-12-04 at 15:56 -0500, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2012-12-04 at 15:14 -0500, Rob Crittenden wrote: Simo Sorce wrote: On Tue, 2012-12-04 at 14:03 -0500, Rob Crittenden wrote: Only touch the service list in the server installer and ipactl. Nack, comments inline. [..] This break the fallback we have in ipa_stop() We expect an exception or a non empty list there. Ok, I can move the handling so ipactl ignores the exception. Is the problem that we are printing an error to stdout/stderr ? Or do you actually want to change behavior somehow ? We need to change the behavior. If you run: ipactl stop then: ipactl status you get a backtrace because the service list doesn't exist. Ok, what about defining our own exception and then simply pass on it except for the stop() case that treats it differently ? I think checking for file existence is probably going to hit the majority of the cases. Using a separate exception is probably overkill. ACK Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
