On 10/29/2013 01:00 PM, Petr Viktorin wrote:
On 10/24/2013 12:20 PM, Tomas Babej wrote:
On 10/22/2013 10:44 AM, Petr Viktorin wrote:
On 10/22/2013 10:09 AM, Tomas Babej wrote:
On 10/22/2013 09:54 AM, Petr Viktorin wrote:
On 10/22/2013 09:20 AM, Tomas Babej wrote:
Hi,

Adds support for host definition by a environment variables of the
following form:

KEYWORDHOST__envX, where X is the number of the environment
for which host referenced by a keyword should be defined.

You can also optionally use KEYWORDHOST__IP_envX to define
the IP address directly, otherwise the framework will try to resolve
the hostname.

Adds a required_keyword_hosts attribute to the IntegrationTest class,
which can test developer use to specify the keyword hosts that this
particular test requires. If not all required keyword hosts are
available, the test will be skipped.

All keyword hosts are accessible to the IntegrationTests via the
keyword_hosts attribute, which contains a dictionary keyed by the
keywords.


Why is this necessary?
What's wrong with just extending the current scheme with more roles?



The need for keyword hosts arised with the implementation of the legacy
client test suite.

As each of these tests needs a precise type (pre-defined and
pre-configured) of VM, and not a generic client, you need to restrict
the test case to specific host type.

One test might be restricted to RHEL 5.10 with nss-pam-ldapd, another to FreeBSD, next one to CentOS with nss-pam-ldapd, next to CentOS with old
version of SSSD...

Each testcase that needs a new type of preconfigured host, we would need
to introduce a new role, which would need to be integrated in the
framework. In such implementation, we would lose loose coupling between
the test framework and the test themselves, and make them less
pluggable.

The framework itself (excluding the configuration part) should be able
to handle this nicely using the existing role mechanism. It's true
that in some places it's currently hard-wired to just a few roles, but
supporting completely custom roles shouldn't be a problem.
I'd prefer if this system was used, rather than basically adding a
second role system, which we'd have to support even if we get rid of
the current config part.

The envvar-based configuration is not as flexible, but I'd rather make
this part somewhat unpleasant than making the framework complex. We
plan to move to a simpler configuration method anyway.
That said, you can basically keep the variable name scheme you use in
this patch; just maybe use TESTHOST rather than KEYWORDHOST.


I rewired the patch to use the current role system.

Please look if you have any additional issues.


I only have two naming nitpicks.
- The roles aren't really "dynamic"; eventually all the "dynamicness" should be specific just to the envvar configuration system, and a few shortcuts like `replicas` for `host_by_role('replica')`. I think "extra" would be a better term. - The environment variable names could be more descriptive. They store a hostname, not a role, so instead of $ROLE_<keyword>_envX I'd prefer $TESTHOST_<role>_envX

Other than that the changes look great!


Thanks, updated patch attached.

--
Tomas Babej
Associate Software Engeneer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org

From 55e4c104aa88f1e584bc1b91a5938fa6616f595a Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Wed, 16 Oct 2013 13:54:26 +0200
Subject: [PATCH] ipatests: Add support for extra roles referenced by a keyword

Adds support for host definition by a environment variables of the
following form:

ROLE_<keyword>_envX, where X is the number of the environment
for which host referenced by a role <keyword> should be defined.

Adds a required_extra_roles attribute to the IntegrationTest class,
which can test developer use to specify the extra roles that this
particular test requires. If not all required extra roles are
available, the test will be skipped.

All extra (and static) roles are accessible to the IntegrationTests
via the host_by_role method, which returns a host of given role.

Part of: https://fedorahosted.org/freeipa/ticket/3833
---
 ipatests/ipa-test-config            |  19 ++++++-
 ipatests/man/ipa-test-config.1      |  10 +++-
 ipatests/test_integration/base.py   |  41 +++++++++++---
 ipatests/test_integration/config.py | 109 ++++++++++++++++++++++++++++--------
 ipatests/test_integration/host.py   |   9 ++-
 ipatests/test_integration/tasks.py  |  10 ++--
 6 files changed, 158 insertions(+), 40 deletions(-)

diff --git a/ipatests/ipa-test-config b/ipatests/ipa-test-config
index ce1f55b5f6f81c337559eaccca42fdb942a1321a..fbaf3d57a4c7395afcf1ff81d3b29502563305cc 100755
--- a/ipatests/ipa-test-config
+++ b/ipatests/ipa-test-config
@@ -52,6 +52,9 @@ def main(argv):
     parser.add_argument('--client', type=int,
                         help='Print config for the client with this number')
 
+    parser.add_argument('--role', type=str,
+                        help='Print config for machine with this role')
+
     parser.add_argument('--no-simple', dest='simple', action='store_false',
                         help='Do not print Simple Vars '
                              '(normally included backwards-compatibility)')
@@ -106,18 +109,30 @@ def get_object(conf, args):
                 exit('No domains are configured.')
         if args.master:
             return domain.master
+
         elif args.replica:
             num = int(args.replica) - 1
             try:
                 return domain.replicas[args.replica]
             except LookupError:
-                exit('Domain %s not found in domain %s' % (args.replica, domain.name))
+                exit('Domain %s not found in domain %s' % (args.replica,
+                                                           domain.name))
+
         elif args.client:
             num = int(args.client) - 1
             try:
                 return domain.replicas[args.client]
             except LookupError:
-                exit('Client %s not found in domain %s' % (args.client, domain.name))
+                exit('Client %s not found in domain %s' % (args.client,
+                                                           domain.name))
+
+        elif args.role:
+            try:
+                return domain.get_host_by_role(args.role)
+            except LookupError:
+                exit('No host with role %s not found in domain %s'
+                     % (args.role, domain.name))
+
         else:
             return domain
 
diff --git a/ipatests/man/ipa-test-config.1 b/ipatests/man/ipa-test-config.1
index a2fa96b57129b5adad3c15cfe7e97b8a6c9724d0..317bd40d86a1e49c73f7e5d529617f40fc2ae564 100644
--- a/ipatests/man/ipa-test-config.1
+++ b/ipatests/man/ipa-test-config.1
@@ -62,6 +62,9 @@ Output configuration for the replica with the given number
 \fB\-\-replica\fR
 Output configuration for the client with the given number
 .TP
+\fB\-\-role\fR
+Output configuration for the host with the given role.
+.TP
 \fB\-\-no\-simple\fR
 Do not output Simple Vars.
 These are normally included for backwards compatibility.
@@ -91,8 +94,11 @@ Host configuration:
 .TP
 \fB$AD_env1\fR, \fB$AD_env2\fR, \fB$AD_env3\fR, \fB$AD_env4\fR, ...
     can be used to define Active Directory domains. Please note that these
-    domains are not separate from the IPA domains, so please use an unique
-    environment suffix for each of your Active Directory domains.
+    domains are not treated as separate from the IPA domains, so please use an
+    unique environment suffix for each of your Active Directory domains.
+.TP
+\fB$TESTHOST_\fR<keyword>\fB_env\fR<e>, e.g. \fB$TESTHOST_LEGACY_env1
+    Defines a host with extra role identified as lowercased <keyword>, e.g. 'legacy'.
 .TP
 \fB$BEAKER\fR<role><num>\fB_IP_env\fR<e>, e.g. \fB$BEAKERREPLICA1_IP_env1\fR
     the IP address of the given host
diff --git a/ipatests/test_integration/base.py b/ipatests/test_integration/base.py
index 1bed7d55b0e89f88507165807e01ca7e185f2c80..a24a577d61b17a46979bdb324faece8e5942d312 100644
--- a/ipatests/test_integration/base.py
+++ b/ipatests/test_integration/base.py
@@ -19,8 +19,6 @@
 
 """Base class for FreeIPA integration tests"""
 
-import os
-
 import nose
 
 from ipapython.ipa_log_manager import log_mgr
@@ -36,6 +34,7 @@ class IntegrationTest(object):
     num_replicas = 0
     num_clients = 0
     num_ad_domains = 0
+    required_extra_roles = []
     topology = None
 
     @classmethod
@@ -54,16 +53,29 @@ class IntegrationTest(object):
 
         cls.logs_to_collect = {}
 
-        domain = config.domains[0]
+        cls.domain = config.domains[0]
 
-        cls.master = domain.master
-        cls.replicas = get_resources(domain.replicas, 'replicas',
+        # Check that we have enough resources available
+        cls.master = cls.domain.master
+        cls.replicas = get_resources(cls.domain.replicas, 'replicas',
                                      cls.num_replicas)
-        cls.clients = get_resources(domain.clients, 'clients',
+        cls.clients = get_resources(cls.domain.clients, 'clients',
                                     cls.num_clients)
         cls.ad_domains = get_resources(config.ad_domains, 'AD domains',
                                        cls.num_ad_domains)
 
+        # Check that we have all required extra hosts at our disposal
+        available_extra_roles = [role for domain in cls.get_domains()
+                                        for role in domain.extra_roles]
+        missing_extra_roles = list(set(cls.required_extra_roles) -
+                                     set(available_extra_roles))
+
+        if missing_extra_roles:
+            raise nose.SkipTest("Not all required extra hosts available, "
+                                "missing: %s, available: %s"
+                                % (missing_extra_roles,
+                                   available_extra_roles))
+
         for host in cls.get_all_hosts():
             host.add_log_collector(cls.collect_log)
             cls.prepare_host(host)
@@ -75,8 +87,22 @@ class IntegrationTest(object):
             raise
 
     @classmethod
+    def host_by_role(cls, role):
+        for domain in cls.get_domains():
+            try:
+                return domain.host_by_role(role)
+            except LookupError:
+                pass
+        raise LookupError(role)
+
+    @classmethod
     def get_all_hosts(cls):
-        return [cls.master] + cls.replicas + cls.clients
+        return ([cls.master] + cls.replicas + cls.clients +
+                map(cls.host_by_role, cls.required_extra_roles))
+
+    @classmethod
+    def get_domains(cls):
+        return [cls.domain] + cls.ad_domains
 
     @classmethod
     def prepare_host(cls, host):
@@ -103,6 +129,7 @@ class IntegrationTest(object):
             del cls.replicas
             del cls.clients
             del cls.ad_domains
+            del cls.domain
 
     @classmethod
     def uninstall(cls):
diff --git a/ipatests/test_integration/config.py b/ipatests/test_integration/config.py
index 84228c736711de874984920458eba0e98d2814bf..3aa4d05d6cb5758cd0d6be64a1ac582adcc971b4 100644
--- a/ipatests/test_integration/config.py
+++ b/ipatests/test_integration/config.py
@@ -27,7 +27,7 @@ import random
 from ipapython import ipautil
 from ipapython.dn import DN
 from ipapython.ipa_log_manager import log_mgr
-from ipatests.test_integration.host import BaseHost
+from ipatests.test_integration.host import BaseHost, Host
 
 
 class Config(object):
@@ -96,7 +96,18 @@ class Config(object):
         OTHER_env1: space-separated FQDNs of other hosts
         (same for _env2, _env3, etc)
         BEAKERREPLICA1_IP_env1: IP address of replica 1 in env 1
-        (same for MASTER, CLIENT)
+        (same for MASTER, CLIENT, or any extra defined ROLE)
+
+        For each machine that should be accessible to tests via extra roles,
+        the following environment variable is necessary:
+
+            TESTHOST_<role>_env1: FQDN of the machine with the extra role <role>
+
+        You can also optionally specify the IP address of the host:
+            BEAKER<role>_IP_env1: IP address of the machine of the extra role
+
+        The framework will try to resolve the hostname to its IP address
+        if not passed via this environment variable.
 
         Also see env_normalize() for alternate variable names
         """
@@ -167,24 +178,25 @@ class Config(object):
             env['RELM%s' % domain._env] = domain.realm
             env['BASEDN%s' % domain._env] = str(domain.basedn)
 
-            for role, hosts in [('MASTER', domain.masters),
-                                ('REPLICA', domain.replicas),
-                                ('CLIENT', domain.clients),
-                                ('AD', domain.ads),
-                                ('OTHER', domain.other_hosts)]:
+            for role in domain.roles:
+                hosts = domain.hosts_by_role(role)
+
                 hostnames = ' '.join(h.hostname for h in hosts)
-                env['%s%s' % (role, domain._env)] = hostnames
+                env['%s%s' % (role.upper(), domain._env)] = hostnames
 
                 ext_hostnames = ' '.join(h.external_hostname for h in hosts)
-                env['BEAKER%s%s' % (role, domain._env)] = ext_hostnames
+                env['BEAKER%s%s' % (role.upper(), domain._env)] = ext_hostnames
 
                 ips = ' '.join(h.ip for h in hosts)
-                env['BEAKER%s_IP%s' % (role, domain._env)] = ips
+                env['BEAKER%s_IP%s' % (role.upper(), domain._env)] = ips
 
                 for i, host in enumerate(hosts, start=1):
-                    suffix = '%s%s' % (role, i)
+                    suffix = '%s%s' % (role.upper(), i)
+                    prefix = 'TESTHOST_' if role in domain.extra_roles else ''
+
                     ext_hostname = host.external_hostname
-                    env['%s%s' % (suffix, domain._env)] = host.hostname
+                    env['%s%s%s' % (prefix, suffix,
+                                    domain._env)] = host.hostname
                     env['BEAKER%s%s' % (suffix, domain._env)] = ext_hostname
                     env['BEAKER%s_IP%s' % (suffix, domain._env)] = host.ip
 
@@ -268,6 +280,49 @@ class Domain(object):
         self.realm = self.name.upper()
         self.basedn = DN(*(('dc', p) for p in name.split('.')))
 
+        self._extra_roles = tuple()  # Serves as a cache for the domain roles
+        self._session_env = None
+
+    @property
+    def roles(self):
+        return self.static_roles + self.extra_roles
+
+    @property
+    def static_roles(self):
+        # Specific roles for each domain type are hardcoded
+        if self.type == 'IPA':
+            return ('master', 'client', 'replica', 'other')
+        else:
+            return ('ad',)
+
+    @property
+    def extra_roles(self):
+        if self._extra_roles:
+            return self._extra_roles
+
+        roles = ()
+
+        # Extra roles can be defined via env variables of form TESTHOST_key_envX
+        for variable in self._session_env:
+            if variable.startswith('TESTHOST'):
+
+                variable_split = variable.split('_')
+
+                defines_extra_role = (
+                    variable.endswith(self._env) and
+                    # at least 3 parts, as in TESTHOST_key_env1
+                    len(variable_split) > 2 and
+                    # prohibit redefining roles
+                    variable_split[-2].lower() not in roles
+                    )
+
+                if defines_extra_role:
+                    key = '_'.join(variable_split[1:-1])
+                    roles += (key.lower(),)
+
+        self._extra_roles = roles
+        return roles
+
     @classmethod
     def from_env(cls, env, config, index, domain_type):
 
@@ -276,17 +331,17 @@ class Domain(object):
         # only to the AD domains
         if domain_type == 'IPA':
             master_role = 'MASTER'
-            domain_roles = 'master', 'replica', 'client', 'other'
         else:
             master_role = 'AD'
-            domain_roles = 'ad',
 
         master_env = '%s_env%s' % (master_role, index)
         hostname, dot, domain_name = env[master_env].partition('.')
         self = cls(config, domain_name, index, domain_type)
+        self._session_env = env
 
-        for role in domain_roles:
-            value = env.get('%s%s' % (role.upper(), self._env), '')
+        for role in self.roles:
+            prefix = 'TESTHOST_' if role in self.extra_roles else ''
+            value = env.get('%s%s%s' % (prefix, role.upper(), self._env), '')
 
             for index, hostname in enumerate(value.split(), start=1):
                 host = BaseHost.from_env(env, self, hostname, role, index)
@@ -307,30 +362,38 @@ class Domain(object):
 
         return env
 
+    def host_by_role(self, role):
+        if self.hosts_by_role(role):
+            return self.hosts_by_role(role)[0]
+        else:
+            raise LookupError(role)
+
+    def hosts_by_role(self, role):
+        return [h for h in self.hosts if h.role == role]
+
     @property
     def master(self):
-        return self.masters[0]
+        return self.host_by_role('master')
 
     @property
     def masters(self):
-        return [h for h in self.hosts if h.role == 'master']
+        return self.hosts_by_role('master')
 
     @property
     def replicas(self):
-        return [h for h in self.hosts if h.role == 'replica']
+        return self.hosts_by_role('replica')
 
     @property
     def clients(self):
-        return [h for h in self.hosts if h.role == 'client']
+        return self.hosts_by_role('client')
 
     @property
     def ads(self):
-        return [h for h in self.hosts if h.role == 'ad']
+        return self.hosts_by_role('ad')
 
     @property
     def other_hosts(self):
-        return [h for h in self.hosts
-                if h.role not in ('master', 'client', 'replica', 'ad')]
+        return self.hosts_by_role('other')
 
     def host_by_name(self, name):
         for host in self.hosts:
diff --git a/ipatests/test_integration/host.py b/ipatests/test_integration/host.py
index 157c5eda8bfc9ff11348e3a5aefd993f1bf168f8..02c82b372ce2805c0ca922319f5de1cd29b0ed82 100644
--- a/ipatests/test_integration/host.py
+++ b/ipatests/test_integration/host.py
@@ -97,7 +97,11 @@ class BaseHost(object):
         ip = env.get('BEAKER%s%s_IP_env%s' %
                         (role.upper(), index, domain.index), None)
 
-        if role == 'ad':
+        # We need to determine the type of the host, this depends on the domain
+        # type, as we assume all Unix machines are in the Unix domain and
+        # all Windows machine in a AD domain
+
+        if domain.type == 'AD':
             cls = WinHost
         else:
             cls = Host
@@ -121,7 +125,8 @@ class BaseHost(object):
         env['MYBEAKERHOSTNAME'] = self.external_hostname
         env['MYIP'] = self.ip
 
-        env['MYROLE'] = '%s%s' % (role, self.domain._env)
+        prefix = 'TESTHOST_' if self.role in self.domain.extra_roles else ''
+        env['MYROLE'] = '%s%s%s' % (prefix, role, self.domain._env)
         env['MYENV'] = str(self.domain.index)
 
         return env
diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index a93a583e966da720920c63c6db643f6c5b377c83..fc3ce67cf84617ad43e93c1bcdf868f631659584 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -34,15 +34,17 @@ from ipapython.dn import DN
 from ipapython.ipa_log_manager import log_mgr
 from ipatests.test_integration import util
 from ipatests.test_integration.config import env_to_script
+from ipatests.test_integration.host import Host
 
 log = log_mgr.get_logger(__name__)
 
 
 def prepare_host(host):
-    env_filename = os.path.join(host.config.test_dir, 'env.sh')
-    host.collect_log(env_filename)
-    host.transport.mkdir_recursive(host.config.test_dir)
-    host.put_file_contents(env_filename, env_to_script(host.to_env()))
+    if isinstance(host, Host):
+        env_filename = os.path.join(host.config.test_dir, 'env.sh')
+        host.collect_log(env_filename)
+        host.transport.mkdir_recursive(host.config.test_dir)
+        host.put_file_contents(env_filename, env_to_script(host.to_env()))
 
 
 def apply_common_fixes(host):
-- 
1.8.3.1

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to