URL: https://github.com/freeipa/freeipa/pull/258
Author: tiran
 Title: #258: Break ipaplatform / ipalib import cycle of hell
Action: opened

PR body:
"""
Here is an attempt to break the import cycle of hell between ipaplatform
and ipalib. All services now pass an ipalib.api object to
services.service(). RedHatServices.__init__() still needs to do a local
import because it initializes its wellknown service dict with service
instances.

Signed-off-by: Christian Heimes <chei...@redhat.com>
"""

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/258/head:pr258
git checkout pr258
From f89393b0ce3a9215cea3b17ce1247ab4e9435465 Mon Sep 17 00:00:00 2001
From: Christian Heimes <chei...@redhat.com>
Date: Fri, 18 Nov 2016 15:42:23 +0100
Subject: [PATCH] Break ipaplatform / ipalib import cycle of hell

Here is an attempt to break the import cycle of hell between ipaplatform
and ipalib. All services now pass an ipalib.api object to
services.service(). RedHatServices.__init__() still needs to do a local
import because it initializes its wellknown service dict with service
instances.

Signed-off-by: Christian Heimes <chei...@redhat.com>
---
 install/share/copy-schema-to-ca.py         |  2 +-
 ipaclient/install/client.py                |  8 ++++----
 ipaclient/ntpconf.py                       | 11 ++++++-----
 ipaplatform/base/services.py               | 22 +++++++++++++++-------
 ipaplatform/fedora/services.py             | 10 +++++-----
 ipaplatform/redhat/services.py             | 29 ++++++++++++++---------------
 ipaplatform/redhat/tasks.py                |  4 ++--
 ipaplatform/rhel/services.py               | 10 +++++-----
 ipaserver/install/adtrustinstance.py       |  6 +++---
 ipaserver/install/bindinstance.py          |  2 +-
 ipaserver/install/dns.py                   |  2 +-
 ipaserver/install/httpinstance.py          |  4 ++--
 ipaserver/install/installutils.py          |  2 +-
 ipaserver/install/ipa_restore.py           |  2 +-
 ipaserver/install/opendnssecinstance.py    |  2 +-
 ipaserver/install/server/replicainstall.py |  2 +-
 ipaserver/install/server/upgrade.py        | 10 +++++-----
 ipaserver/install/service.py               |  2 +-
 ipaserver/install/upgradeinstance.py       |  3 ++-
 ipaserver/plugins/dns.py                   |  2 +-
 ipaserver/plugins/server.py                |  2 +-
 21 files changed, 73 insertions(+), 64 deletions(-)

diff --git a/install/share/copy-schema-to-ca.py b/install/share/copy-schema-to-ca.py
index a6d09ec..eb2a306 100755
--- a/install/share/copy-schema-to-ca.py
+++ b/install/share/copy-schema-to-ca.py
@@ -104,7 +104,7 @@ def restart_pki_ds():
     """Restart the CA DS instance to pick up schema changes
     """
     root_logger.info('Restarting CA DS')
-    services.service('dirsrv').restart(SERVERID)
+    services.service('dirsrv', api).restart(SERVERID)
 
 
 def main():
diff --git a/ipaclient/install/client.py b/ipaclient/install/client.py
index b24a989..d18d8bb 100644
--- a/ipaclient/install/client.py
+++ b/ipaclient/install/client.py
@@ -2822,7 +2822,7 @@ def _install(options):
         root_logger.info("%s enabled", "SSSD" if options.sssd else "LDAP")
 
         if options.sssd:
-            sssd = services.service('sssd')
+            sssd = services.service('sssd', api)
             try:
                 sssd.restart()
             except CalledProcessError:
@@ -3139,7 +3139,7 @@ def uninstall(options):
 
         root_logger.info(
             "IPA domain removed from current one, restarting SSSD service")
-        sssd = services.service('sssd')
+        sssd = services.service('sssd', api)
         try:
             sssd.restart()
         except CalledProcessError:
@@ -3153,7 +3153,7 @@ def uninstall(options):
             "Other domains than IPA domain found, IPA domain was removed "
             "from /etc/sssd/sssd.conf.")
 
-        sssd = services.service('sssd')
+        sssd = services.service('sssd', api)
         try:
             sssd.restart()
         except CalledProcessError:
@@ -3172,7 +3172,7 @@ def uninstall(options):
             "Redundant SSSD configuration file "
             "/etc/sssd/sssd.conf was moved to /etc/sssd/sssd.conf.deleted")
 
-        sssd = services.service('sssd')
+        sssd = services.service('sssd', api)
         try:
             sssd.stop()
         except CalledProcessError:
diff --git a/ipaclient/ntpconf.py b/ipaclient/ntpconf.py
index 9a7db65..c78f807 100644
--- a/ipaclient/ntpconf.py
+++ b/ipaclient/ntpconf.py
@@ -16,11 +16,12 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
+import os
+import shutil
 
+from ipalib import api
 from ipapython import ipautil
 from ipapython.ipa_log_manager import root_logger
-import shutil
-import os
 from ipaplatform.tasks import tasks
 from ipaplatform import services
 from ipaplatform.paths import paths
@@ -189,7 +190,7 @@ def check_timedate_services():
         if service == 'ntpd':
             continue
         # Make sure that the service is not enabled
-        instance = services.service(service)
+        instance = services.service(service, api)
         if instance.is_enabled() or instance.is_running():
             raise NTPConflictingService(conflicting_service=instance.service_name)
 
@@ -201,7 +202,7 @@ def force_ntpd(statestore):
     for service in services.timedate_services:
         if service == 'ntpd':
             continue
-        instance = services.service(service)
+        instance = services.service(service, api)
         enabled = instance.is_enabled()
         running = instance.is_running()
 
@@ -224,7 +225,7 @@ def restore_forced_ntpd(statestore):
         if service == 'ntpd':
             continue
         if statestore.has_state(service):
-            instance = services.service(service)
+            instance = services.service(service, api)
             enabled = statestore.restore_state(instance.service_name, 'enabled')
             running = statestore.restore_state(instance.service_name, 'running')
             if enabled:
diff --git a/ipaplatform/base/services.py b/ipaplatform/base/services.py
index 750d979..071fe2a 100644
--- a/ipaplatform/base/services.py
+++ b/ipaplatform/base/services.py
@@ -27,10 +27,10 @@
 import json
 import time
 import collections
+import warnings
 
 import six
 
-import ipalib
 from ipapython import ipautil
 from ipaplatform.paths import paths
 
@@ -63,7 +63,6 @@ class KnownServices(collections.Mapping):
     instances as its own attributes on first access (or instance creation)
     and cache them.
     """
-
     def __init__(self, d):
         self.__d = d
 
@@ -93,9 +92,17 @@ class PlatformService(object):
 
     """
 
-    def __init__(self, service_name, api=ipalib.api):
+    def __init__(self, service_name, api=None):
         self.service_name = service_name
-        self.api = api
+        if api is not None:
+            self.api = api
+        else:
+            import ipalib  # FixMe: break import cycle
+            self.api = ipalib.api
+            warnings.warn(
+                "{s.__class__.__name__}('{s.service_name}', api=None) "
+                "is deprecated.".format(s=self),
+                RuntimeWarning, stacklevel=2)
 
     def start(self, instance_name="", capture_output=True, wait=True,
         update_service_list=True):
@@ -185,10 +192,11 @@ def get_config_dir(self, instance_name=""):
 class SystemdService(PlatformService):
     SYSTEMD_SRV_TARGET = "%s.target.wants"
 
-    def __init__(self, service_name, systemd_name, **kwargs):
-        super(SystemdService, self).__init__(service_name, **kwargs)
+    def __init__(self, service_name, systemd_name, api=None):
+        super(SystemdService, self).__init__(service_name, api=api)
         self.systemd_name = systemd_name
-        self.lib_path = os.path.join(paths.LIB_SYSTEMD_SYSTEMD_DIR, self.systemd_name)
+        self.lib_path = os.path.join(paths.LIB_SYSTEMD_SYSTEMD_DIR,
+                                     self.systemd_name)
         self.lib_path_exists = None
 
     def service_instance(self, instance_name, operation=None):
diff --git a/ipaplatform/fedora/services.py b/ipaplatform/fedora/services.py
index a3b951a..725d9ee 100644
--- a/ipaplatform/fedora/services.py
+++ b/ipaplatform/fedora/services.py
@@ -41,17 +41,17 @@ class FedoraService(redhat_services.RedHatService):
 # Function that constructs proper Fedora-specific server classes for services
 # of specified name
 
-def fedora_service_class_factory(name):
+def fedora_service_class_factory(name, api=None):
     if name == 'domainname':
-        return FedoraService(name)
-    return redhat_services.redhat_service_class_factory(name)
+        return FedoraService(name, api)
+    return redhat_services.redhat_service_class_factory(name, api)
 
 
 # Magicdict containing FedoraService instances.
 
 class FedoraServices(redhat_services.RedHatServices):
-    def service_class_factory(self, name):
-        return fedora_service_class_factory(name)
+    def service_class_factory(self, name, api=None):
+        return fedora_service_class_factory(name, api)
 
 
 # Objects below are expected to be exported by platform module
diff --git a/ipaplatform/redhat/services.py b/ipaplatform/redhat/services.py
index 2432534..99904ce 100644
--- a/ipaplatform/redhat/services.py
+++ b/ipaplatform/redhat/services.py
@@ -31,7 +31,6 @@
 
 from ipapython import ipautil, dogtag
 from ipapython.ipa_log_manager import root_logger
-from ipalib import api
 from ipaplatform.paths import paths
 
 # Mappings from service names as FreeIPA code references to these services
@@ -76,7 +75,7 @@
 class RedHatService(base_services.SystemdService):
     system_units = redhat_system_units
 
-    def __init__(self, service_name):
+    def __init__(self, service_name, api=None):
         systemd_name = service_name
         if service_name in self.system_units:
             systemd_name = self.system_units[service_name]
@@ -86,7 +85,7 @@ def __init__(self, service_name):
                 # and not a foo.target. Thus, not correct service name for
                 # systemd, default to foo.service style then
                 systemd_name = "%s.service" % (service_name)
-        super(RedHatService, self).__init__(service_name, systemd_name)
+        super(RedHatService, self).__init__(service_name, systemd_name, api)
 
 
 class RedHatDirectoryService(RedHatService):
@@ -195,12 +194,12 @@ def get_config_dir(self, instance_name=""):
 class RedHatCAService(RedHatService):
     def wait_until_running(self):
         root_logger.debug('Waiting until the CA is running')
-        timeout = float(api.env.startup_timeout)
+        timeout = float(self.api.env.startup_timeout)
         op_timeout = time.time() + timeout
         while time.time() < op_timeout:
             try:
                 # check status of CA instance on this host, not remote ca_host
-                status = dogtag.ca_status(api.env.host)
+                status = dogtag.ca_status(self.api.env.host)
             except Exception as e:
                 status = 'check interrupted due to error: %s' % e
             root_logger.debug('The CA status is: %s' % status)
@@ -244,31 +243,31 @@ def is_running(self, instance_name="", wait=True):
 # Function that constructs proper Red Hat OS family-specific server classes for
 # services of specified name
 
-def redhat_service_class_factory(name):
+def redhat_service_class_factory(name, api=None):
     if name == 'dirsrv':
-        return RedHatDirectoryService(name)
+        return RedHatDirectoryService(name, api)
     if name == 'ipa':
-        return RedHatIPAService(name)
+        return RedHatIPAService(name, api)
     if name == 'sshd':
-        return RedHatSSHService(name)
+        return RedHatSSHService(name, api)
     if name in ('pki-tomcatd', 'pki_tomcatd'):
-        return RedHatCAService(name)
-    return RedHatService(name)
+        return RedHatCAService(name, api)
+    return RedHatService(name, api)
 
 
 # Magicdict containing RedHatService instances.
 
 class RedHatServices(base_services.KnownServices):
-    def service_class_factory(self, name):
-        return redhat_service_class_factory(name)
-
     def __init__(self):
+        import ipalib  # FixMe: break import cycle
         services = dict()
         for s in base_services.wellknownservices:
-            services[s] = self.service_class_factory(s)
+            services[s] = self.service_class_factory(s, ipalib.api)
         # Call base class constructor. This will lock services to read-only
         super(RedHatServices, self).__init__(services)
 
+    def service_class_factory(self, name, api=None):
+        return redhat_service_class_factory(name, api)
 
 # Objects below are expected to be exported by platform module
 
diff --git a/ipaplatform/redhat/tasks.py b/ipaplatform/redhat/tasks.py
index dbe005a..11d1733 100644
--- a/ipaplatform/redhat/tasks.py
+++ b/ipaplatform/redhat/tasks.py
@@ -44,8 +44,6 @@
 from ipapython import ipautil
 import ipapython.errors
 
-from ipalib import x509 # FIXME: do not import from ipalib
-
 from ipaplatform.constants import constants
 from ipaplatform.paths import paths
 from ipaplatform.redhat.authconfig import RedHatAuthConfig
@@ -214,6 +212,8 @@ def reload_systemwide_ca_store(self):
             return True
 
     def insert_ca_certs_into_systemwide_ca_store(self, ca_certs):
+        from ipalib import x509  # FixMe: break import cycle
+
         new_cacert_path = paths.SYSTEMWIDE_IPA_CA_CRT
 
         if os.path.exists(new_cacert_path):
diff --git a/ipaplatform/rhel/services.py b/ipaplatform/rhel/services.py
index 980c84a..7918006 100644
--- a/ipaplatform/rhel/services.py
+++ b/ipaplatform/rhel/services.py
@@ -41,17 +41,17 @@ class RHELService(redhat_services.RedHatService):
 # Function that constructs proper RHEL-specific server classes for services
 # of specified name
 
-def rhel_service_class_factory(name):
+def rhel_service_class_factory(name, api=None):
     if name == 'domainname':
-        return RHELService(name)
-    return redhat_services.redhat_service_class_factory(name)
+        return RHELService(name, api)
+    return redhat_services.redhat_service_class_factory(name, api)
 
 
 # Magicdict containing RHELService instances.
 
 class RHELServices(redhat_services.RedHatServices):
-    def service_class_factory(self, name):
-        return rhel_service_class_factory(name)
+    def service_class_factory(self, name, api=None):
+        return rhel_service_class_factory(name, api)
 
 
 # Objects below are expected to be exported by platform module
diff --git a/ipaserver/install/adtrustinstance.py b/ipaserver/install/adtrustinstance.py
index cab5a72..136d26b 100644
--- a/ipaserver/install/adtrustinstance.py
+++ b/ipaserver/install/adtrustinstance.py
@@ -697,14 +697,14 @@ def __enable_compat_tree(self):
     def __start(self):
         try:
             self.start()
-            services.service('winbind').start()
+            services.service('winbind', api).start()
         except Exception:
             root_logger.critical("CIFS services failed to start")
 
     def __stop(self):
         self.backup_state("running", self.is_running())
         try:
-            services.service('winbind').stop()
+            services.service('winbind', api).stop()
             self.stop()
         except Exception:
             pass
@@ -856,7 +856,7 @@ def uninstall(self):
         self.restore_state("running")
         self.restore_state("enabled")
 
-        winbind = services.service("winbind")
+        winbind = services.service("winbind", api)
         # Always try to stop and disable smb service, since we do not leave
         # working configuration after uninstall
         try:
diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index 179eb68..24a77ff 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -617,7 +617,7 @@ def __init__(self, fstore=None, api=api):
         self.forwarders = None
         self.sub_dict = None
         self.reverse_zones = []
-        self.named_regular = services.service('named-regular')
+        self.named_regular = services.service('named-regular', api)
 
     suffix = ipautil.dn_attribute_property('_suffix')
 
diff --git a/ipaserver/install/dns.py b/ipaserver/install/dns.py
index 5b40c03..dd4ab7b 100644
--- a/ipaserver/install/dns.py
+++ b/ipaserver/install/dns.py
@@ -219,7 +219,7 @@ def install_check(standalone, api, replica, options, hostname):
                 "Only one DNSSEC key master is supported in current version.")
 
         if options.kasp_db_file:
-            dnskeysyncd = services.service('ipa-dnskeysyncd')
+            dnskeysyncd = services.service('ipa-dnskeysyncd', api)
 
             if not dnskeysyncd.is_installed():
                 raise RuntimeError("ipa-dnskeysyncd is not configured on this "
diff --git a/ipaserver/install/httpinstance.py b/ipaserver/install/httpinstance.py
index 4e8107e..b6affe6 100644
--- a/ipaserver/install/httpinstance.py
+++ b/ipaserver/install/httpinstance.py
@@ -463,7 +463,7 @@ def create_kdcproxy_conf(self):
         os.chmod(target_fname, 0o644)
 
     def enable_and_start_oddjobd(self):
-        oddjobd = services.service('oddjobd')
+        oddjobd = services.service('oddjobd', api)
         self.sstore.backup_state('oddjobd', 'running', oddjobd.is_running())
         self.sstore.backup_state('oddjobd', 'enabled', oddjobd.is_enabled())
 
@@ -484,7 +484,7 @@ def uninstall(self):
         enabled = self.restore_state("enabled")
 
         # Restore oddjobd to its original state
-        oddjobd = services.service('oddjobd')
+        oddjobd = services.service('oddjobd', api)
 
         if not self.sstore.restore_state('oddjobd', 'running'):
             try:
diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py
index 9e509e4..214d42c 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -984,7 +984,7 @@ def stopped_service(service, instance_name=""):
                       'the next set of commands is being executed.', service,
                       log_instance_name)
 
-    service_obj = services.service(service)
+    service_obj = services.service(service, api)
 
     # Figure out if the service is running, if not, yield
     if not service_obj.is_running(instance_name):
diff --git a/ipaserver/install/ipa_restore.py b/ipaserver/install/ipa_restore.py
index 21403af..3dc6522 100644
--- a/ipaserver/install/ipa_restore.py
+++ b/ipaserver/install/ipa_restore.py
@@ -410,7 +410,7 @@ def run(self):
                 self.log.info('Starting IPA services')
                 run(['ipactl', 'start'])
                 self.log.info('Restarting SSSD')
-                sssd = services.service('sssd')
+                sssd = services.service('sssd', api)
                 sssd.restart()
                 http.remove_httpd_ccache()
         finally:
diff --git a/ipaserver/install/opendnssecinstance.py b/ipaserver/install/opendnssecinstance.py
index 7f3269f..efaed8d 100644
--- a/ipaserver/install/opendnssecinstance.py
+++ b/ipaserver/install/opendnssecinstance.py
@@ -317,7 +317,7 @@ def uninstall(self):
         except Exception:
             pass
 
-        ods_exporter = services.service('ipa-ods-exporter')
+        ods_exporter = services.service('ipa-ods-exporter', api)
         try:
             ods_exporter.stop()
         except Exception:
diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index a7b333c..596d05e 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -451,7 +451,7 @@ def promote_sssd(host_name):
         sssdconfig.save_domain(domain)
         sssdconfig.write()
 
-        sssd = services.service('sssd')
+        sssd = services.service('sssd', api)
         try:
             sssd.restart()
         except CalledProcessError:
diff --git a/ipaserver/install/server/upgrade.py b/ipaserver/install/server/upgrade.py
index 5f61015..82bb6fb 100644
--- a/ipaserver/install/server/upgrade.py
+++ b/ipaserver/install/server/upgrade.py
@@ -1230,21 +1230,21 @@ def uninstall_dogtag_9(ds, http):
         dsdb.untrack_server_cert("Server-Cert")
 
     try:
-        services.service('pki-cad').disable('pki-ca')
+        services.service('pki-cad', api).disable('pki-ca')
     except Exception as e:
         root_logger.warning("Failed to disable pki-cad: %s", e)
     try:
-        services.service('pki-cad').stop('pki-ca')
+        services.service('pki-cad', api).stop('pki-ca')
     except Exception as e:
         root_logger.warning("Failed to stop pki-cad: %s", e)
 
     if serverid is not None:
         try:
-            services.service('dirsrv').disable(serverid)
+            services.service('dirsrv', api).disable(serverid)
         except Exception as e:
             root_logger.warning("Failed to disable dirsrv: %s", e)
         try:
-            services.service('dirsrv').stop(serverid)
+            services.service('dirsrv', api).stop(serverid)
         except Exception as e:
             root_logger.warning("Failed to stop dirsrv: %s", e)
 
@@ -1261,7 +1261,7 @@ def mask_named_regular():
 
     if bindinstance.named_conf_exists():
         root_logger.info('[Masking named]')
-        named = services.service('named-regular')
+        named = services.service('named-regular', api)
         try:
             named.stop()
         except Exception as e:
diff --git a/ipaserver/install/service.py b/ipaserver/install/service.py
index 62bd499..ee938d6 100644
--- a/ipaserver/install/service.py
+++ b/ipaserver/install/service.py
@@ -143,7 +143,7 @@ def __init__(self, service_name, service_desc=None, sstore=None,
                  keytab=None):
         self.service_name = service_name
         self.service_desc = service_desc
-        self.service = services.service(service_name)
+        self.service = services.service(service_name, api)
         self.steps = []
         self.output_fd = sys.stdout
 
diff --git a/ipaserver/install/upgradeinstance.py b/ipaserver/install/upgradeinstance.py
index 0d6013f..e606b38 100644
--- a/ipaserver/install/upgradeinstance.py
+++ b/ipaserver/install/upgradeinstance.py
@@ -91,7 +91,8 @@ def __init__(self, realm_name, files=[], schema_files=[]):
         self.schema_files = schema_files
 
     def __start(self):
-        services.service(self.service_name).start(self.serverid, ldapi=True)
+        srv = services.service(self.service_name, api)
+        srv.start(self.serverid, ldapi=True)
         api.Backend.ldap2.connect()
 
     def __stop_instance(self):
diff --git a/ipaserver/plugins/dns.py b/ipaserver/plugins/dns.py
index e37050a..3336e9e 100644
--- a/ipaserver/plugins/dns.py
+++ b/ipaserver/plugins/dns.py
@@ -2710,7 +2710,7 @@ def _warning_ttl_changed_reload_needed(self, result, **options):
                 options['version'],
                 result,
                 messages.ServiceRestartRequired(
-                    service=services.service('named').systemd_name,
+                    service=services.service('named', api).systemd_name,
                     server=_('<all IPA DNS servers>'), )
                 )
 
diff --git a/ipaserver/plugins/server.py b/ipaserver/plugins/server.py
index ded7661..08caa1c 100644
--- a/ipaserver/plugins/server.py
+++ b/ipaserver/plugins/server.py
@@ -262,7 +262,7 @@ def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
 
         if 'ipalocation_location' or 'ipaserviceweight' in options:
             self.add_message(messages.ServiceRestartRequired(
-                service=services.service('named').systemd_name,
+                service=services.service('named', api).systemd_name,
                 server=keys[0], ))
 
             result = self.api.Command.dns_update_system_records()
-- 
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

Reply via email to