Hi Martin,

My bad, forgot to do git add.


On 08/27/2015 06:27 PM, Martin Basti wrote:


On 08/27/2015 05:41 PM, Oleg Fayans wrote:
Hi,

I am sorry I have missed that.
Fixed. The test fails now due to this bug:

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

The test output is attached together with the updated patch

On 08/26/2015 05:53 PM, Martin Basti wrote:


On 08/26/2015 05:42 PM, Martin Basti wrote:


On 08/26/2015 02:53 PM, Oleg Fayans wrote:
Hi,

No more short links :)

On 08/26/2015 11:50 AM, Tomas Babej wrote:


On 08/26/2015 11:44 AM, Oleg Fayans wrote:
Hi Martin,

On 08/20/2015 11:18 AM, Martin Basti wrote:


On 08/20/2015 10:26 AM, Martin Basti wrote:


On 08/19/2015 04:17 PM, Martin Basti wrote:
I got this:

https://paste.fedoraproject.org/256746/43999380/
FYI replica install failure. (I will retest it, but I'm pretty
sure
that it was clean VM, test for some reason install client first)

   File
"/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py",



line 295, in decorated
     func(installer)
   File
"/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py",



line 319, in install_check
     sys.exit("IPA client is already configured on this system.\n"

2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed,
exception: SystemExit: IPA client is already configured on this
system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.
2015-08-19T14:14:15Z ERROR IPA client is already configured on
this
system.
Please uninstall it first before configuring the replica, using
'ipa-client-install --uninstall'.
Confirm I got same error.
Fixed. It was a regex error.



On 08/19/2015 09:00 AM, Oleg Fayans wrote:
Hi Martin,

As discussed, here is a new version with pep8-related fixes


On 08/14/2015 10:44 AM, Oleg Fayans wrote:
Hi Martin,

Already noticed that. Implemented the named groups as Tomas
advised.
Added the third test for
http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica








On 08/13/2015 05:06 PM, Martin Basti wrote:


On 08/11/2015 03:36 PM, Oleg Fayans wrote:
Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:
NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:
Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:
Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:
+def create_segment(master, leftnode, rightnode):
+    """create_segment(master, leftnode, rightnode)
Why do you add the name of method in docstring?
My bad, fixed.

still there

+ tokenize_topologies(command_output)
+        takes an output of `ipa topologysegment-find` and
returns an
array of

Fixed, sorry.



2)

+def create_segment(master, leftnode, rightnode):
+    """create_segment(master, leftnode, rightnode)
+    creates a topology segment. The first argument is a
node to
run the
command on
+    The first 3 arguments should be objects of class Host
+    Returns a hash object containing segment's name,
leftnode,
rightnode information
+    """

I would prefer to add assert there instead of just
document
that a
Host
object is needed
assert(isinstance(master, Host))
...

Fixed. Created a decorator that checks the type of
arguments

This does not scale well.
If we will want to add new argument that is not host
object, you
need
change it again.

Agreed. Modified the decorator so that you can specify a
slice of
arguments to be checked and a class to compare to. This does
scale :)

This might be used as function with specified variables that
have to be
host objects


3)
+def destroy_segment(master, segment_name):
+    """
+    destroy_segment(master, segment_name)
+    Destroys topology segment. First argument should be
object of
class
Host

Instead of description of params as first, second etc.,
you
may use
following:

+def destroy_segment(master, segment_name):
+    """
+    Destroys topology segment.
+    :param master: reference to master object of class
Host
+    :param segment: name fo segment
and eventually this in other methods
+    :returns: Lorem ipsum sit dolor mit amet
+    :raises NotFound: if segment is not found

Fixed

4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]

Fixed

5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)

in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The
first
one is
even shorter a bit, so why change? :)

It depends on point of view,  because when I reviewed it
yesterday my
brain raises exception that you are trying to add multiple
values to
single value attribute, because you used findall, I expected
that you
need multiple values, and then I saw that index [0] at the
end,
and I
was pretty confused what are you trying to achieve.

And because findall is not effective in case when you
need to
find just
one occurrence.

I got it. Fixed.






Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3
compability
already comes, but just for record:
instead of open(file, 'r'), please use io.open(file, 'r')
(import io
before)

Done.





1)

+#

empty comment here (several times)

Removed



NACK

you changed it wrong

group() returns everything, you need use group(1) to return
content in
braces.

















Please, do not use third-party URL shorteners from within our source
code.

Tomas



I received 2 errors

___________________________________________________________________________________

TestTopologyOptions.test_add_remove_segment
___________________________________________________________________________________



self = <ipatests.test_integration.test_topology.TestTopologyOptions
object at 0x7f6ab95b3110>

    def test_add_remove_segment(self):
        """
            Make sure a topology segment can be manually created and
deleted
            with the influence on the real topology
            Testcase
http://www.freeipa.org/page/V4/Manage_replication_topology/
            Test_plan#Test_case:_Basic_CRUD_test
            """
        tasks.kinit_admin(self.master)
        # Install the second replica
        tasks.install_replica(self.master, self.replicas[1],
setup_ca=False,
                              setup_dns=False)
        # turn a star into a ring
        segment, err = tasks.create_segment(self.master,
                                            self.replicas[0],
> self.replicas[1])
E       TypeError: 'NoneType' object is not iterable

test_integration/test_topology.py:102: TypeError

Maybe tasks.create_segment returns None?
In [3]: a, b = None
---------------------------------------------------------------------------


TypeError                                 Traceback (most recent call
last)
<ipython-input-3-c020227e755d> in <module>()
----> 1 a, b = None

TypeError: 'NoneType' object is not iterable


--------------

self = <pytest_multihost.transport.SSHCommand object at
0x7f399b9ea710>, raiseonerr = True

    def wait(self, raiseonerr=True):
        """Wait for the remote process to exit

            Raises an excption if the exit code is not 0, unless
raiseonerr is
            true.
            """
        if self._done:
            return self.returncode

        self._end_process()

        self._done = True

        if raiseonerr and self.returncode:
            self.log.error('Exit code: %s', self.returncode)
>           raise subprocess.CalledProcessError(self.returncode,
self.argv)
E           CalledProcessError: Command '['ipa',
'topologysegment-del', 'realm',
'vm-152.abc.idm.lab.eng.brq.redhat.com-to-vm-160.abc.idm.lab.eng.brq.redhat.com']'

returned non-zero exit status 2

Honza found where the problem is.

def check_arguments_are(slice, instanceof):
...
     def wrapper(func):
         def wrapped(*args):
             for i in args[slice[0]:slice[1]]:
                 assert isinstance(i, instanceof), "Wrong type: %s: %s"
% (i, type(i))
             func(*args)
         return wrapped
     return wrapper

there should be
return func(*args)

otherwise None will be always returned


It looks nice, but I miss the integration test in the patch.

--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 47c0c9133f4cc50c979fa079ac31130ab89f5b1a Mon Sep 17 00:00:00 2001
From: Oleg Fayans <ofay...@redhat.com>
Date: Thu, 27 Aug 2015 19:19:28 +0200
Subject: [PATCH] Integration tests for topology plugin

---
 ipatests/test_integration/tasks.py              |  98 +++++++++++++---
 ipatests/test_integration/test_topology.py      | 148 ++++++++++++++++++++++++
 ipatests/test_ipaserver/test_topology_plugin.py |  16 ++-
 3 files changed, 241 insertions(+), 21 deletions(-)
 create mode 100644 ipatests/test_integration/test_topology.py

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index fc81d46f779e698e381f7902bbb280f5dcb7cee3..820507022e6b5e8cc7a57c66c7f9e8e8b1500c7e 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -40,6 +40,24 @@ from ipatests.test_integration.host import Host
 log = log_mgr.get_logger(__name__)
 
 
+def check_arguments_are(slice, instanceof):
+    """
+    :param: slice - tuple of integers denoting the beginning and the end
+    of argument list to be checked
+    :param: instanceof - name of the class the checked arguments should be
+    instances of
+    Example: @check_arguments_are((1, 3), int) will check that the second
+    and third arguments are integers
+    """
+    def wrapper(func):
+        def wrapped(*args):
+            for i in args[slice[0]:slice[1]]:
+                assert isinstance(i, instanceof), "Wrong type: %s: %s" % (i, type(i))
+            return func(*args)
+        return wrapped
+    return wrapper
+
+
 def prepare_host(host):
     if isinstance(host, Host):
         env_filename = os.path.join(host.config.test_dir, 'env.sh')
@@ -118,7 +136,8 @@ def fix_apache_semaphores(master):
     if systemd_available:
         master.run_command(['systemctl', 'stop', 'httpd'], raiseonerr=False)
     else:
-        master.run_command([paths.SBIN_SERVICE, 'httpd', 'stop'], raiseonerr=False)
+        master.run_command([paths.SBIN_SERVICE, 'httpd', 'stop'],
+                           raiseonerr=False)
 
     master.run_command('for line in `ipcs -s | grep apache | cut -d " " -f 2`; '
                        'do ipcrm -s $line; done', raiseonerr=False)
@@ -261,7 +280,7 @@ def install_client(master, client, extra_args=()):
                         '-p', client.config.admin_name,
                         '-w', client.config.admin_password,
                         '--server', master.hostname]
-                        + list(extra_args))
+                       + list(extra_args))
 
     setup_sssd_debugging(client)
     kinit_admin(client)
@@ -298,10 +317,9 @@ def install_adtrust(host):
     else:
         host.run_command(['systemctl', 'restart', 'named'])
 
-
     # Check that named is running and has loaded the information from LDAP
     dig_command = ['dig', 'SRV', '+short', '@localhost',
-               '_ldap._tcp.%s' % host.domain.name]
+                   '_ldap._tcp.%s' % host.domain.name]
     dig_output = '0 100 389 %s.' % host.hostname
     dig_test = lambda x: re.search(re.escape(dig_output), x)
 
@@ -373,9 +391,9 @@ def establish_trust_with_ad(master, ad, extra_args=()):
     master.run_command(['smbcontrol', 'all', 'debug', '100'])
     util.run_repeatedly(master,
                         ['ipa', 'trust-add',
-                        '--type', 'ad', ad.domain.name,
-                        '--admin', 'Administrator',
-                        '--password'] + list(extra_args),
+                         '--type', 'ad', ad.domain.name,
+                         '--admin', 'Administrator',
+                         '--password'] + list(extra_args),
                         stdin_text=master.config.ad_admin_password)
     master.run_command(['smbcontrol', 'all', 'debug', '1'])
     clear_sssd_cache(master)
@@ -428,15 +446,14 @@ def setup_sssd_debugging(host):
     # First, remove any previous occurences
     host.run_command(['sed', '-i',
                       '/debug_level = 7/d',
-                      paths.SSSD_CONF
-                     ], raiseonerr=False)
+                      paths.SSSD_CONF],
+                     raiseonerr=False)
 
     # Add the debug directive to each section
     host.run_command(['sed', '-i',
                       '/\[*\]/ a\debug_level = 7',
-                      paths.SSSD_CONF
-                     ], raiseonerr=False)
-
+                      paths.SSSD_CONF],
+                     raiseonerr=False)
 
     host.collect_log('/var/log/sssd/*')
 
@@ -492,7 +509,7 @@ def disconnect_replica(master, replica):
 
 def kinit_admin(host):
     host.run_command(['kinit', 'admin'],
-                      stdin_text=host.config.admin_password)
+                     stdin_text=host.config.admin_password)
 
 
 def uninstall_master(host):
@@ -507,8 +524,9 @@ def uninstall_master(host):
                       paths.SYSCONFIG_PKI_TOMCAT,
                       paths.SYSCONFIG_PKI_TOMCAT_PKI_TOMCAT_DIR,
                       paths.VAR_LIB_PKI_TOMCAT_DIR,
-                      paths.PKI_TOMCAT],
-                      raiseonerr=False)
+                      paths.PKI_TOMCAT,
+                      paths.REPLICA_INFO_GPG_TEMPLATE % host.hostname],
+                     raiseonerr=False)
     unapply_fixes(host)
 
 
@@ -520,6 +538,56 @@ def uninstall_client(host):
     unapply_fixes(host)
 
 
+@check_arguments_are((0, 2), Host)
+def clean_replication_agreement(master, replica):
+    """
+    Performs `ipa-replica-manage del replica_hostname --force`.
+    """
+    master.run_command(['ipa-replica-manage',
+                        'del',
+                        replica.hostname,
+                        '--force'])
+
+
+@check_arguments_are((0, 3), Host)
+def create_segment(master, leftnode, rightnode):
+    """
+    creates a topology segment. The first argument is a node to run the command
+    :returns: a hash object containing segment's name, leftnode, rightnode
+    information and an error string.
+    """
+    kinit_admin(master)
+    lefthost = leftnode.hostname
+    righthost = rightnode.hostname
+    segment_name = "%s-to-%s" % (lefthost, righthost)
+    result = master.run_command(["ipa", "topologysegment-add", "realm",
+                                 segment_name,
+                                 "--leftnode=%s" % lefthost,
+                                 "--rightnode=%s" % righthost], raiseonerr=False)
+    if result.returncode == 0:
+        return {'leftnode': lefthost,
+                'rightnode': righthost,
+                'name': segment_name}, ""
+    else:
+        return {}, result.stderr_text
+
+
+def destroy_segment(master, segment_name):
+    """
+    Destroys topology segment.
+    :param master: reference to master object of class Host
+    :param segment_name: name of the segment to be created
+    """
+    assert isinstance(master, Host), "master should be an instance of Host"
+    kinit_admin(master)
+    command = ["ipa",
+               "topologysegment-del",
+               "realm",
+               segment_name]
+    result = master.run_command(command, raiseonerr=False)
+    return result.returncode, result.stderr_text
+
+
 def get_topo(name_or_func):
     """Get a topology function by name
 
diff --git a/ipatests/test_integration/test_topology.py b/ipatests/test_integration/test_topology.py
new file mode 100644
index 0000000000000000000000000000000000000000..0d304fcb3f8736090788a3e1eebff52245155964
--- /dev/null
+++ b/ipatests/test_integration/test_topology.py
@@ -0,0 +1,148 @@
+#
+# Copyright (C) 2015  FreeIPA Contributors see COPYING for license
+#
+
+import re
+import time
+
+from ipatests.test_integration.base import IntegrationTest
+from ipatests.test_integration import tasks
+
+
+class TestTopologyOptions(IntegrationTest):
+    num_replicas = 2
+    topology = 'star'
+    rawsegment_re = ('Segment name: (?P<name>.*?)',
+                     '\s+Left node: (?P<lnode>.*?)',
+                     '\s+Right node: (?P<rnode>.*?)',
+                     '\s+Connectivity: (?P<connectivity>\S+)')
+    segment_re = re.compile("\n".join(rawsegment_re))
+    noentries_re = re.compile("Number of entries returned (\d+)")
+
+    @classmethod
+    def install(cls, mh):
+        tasks.install_topo(cls.topology, cls.master,
+                           cls.replicas[:-1],
+                           cls.clients)
+
+    def tokenize_topologies(self, command_output):
+        """
+        takes an output of `ipa topologysegment-find` and returns an array of
+        segment hashes
+        """
+        segments = command_output.split("-----------------")[2]
+        raw_segments = segments.split('\n\n')
+        result = []
+        for i in raw_segments:
+            matched = self.segment_re.search(i)
+            if matched:
+                result.append({'leftnode': matched.group('lnode'),
+                               'rightnode': matched.group('rnode'),
+                               'name': matched.group('name'),
+                               'connectivity': matched.group('connectivity')
+                               }
+                              )
+        return result
+
+    def test_topology_updated_on_replica_install_remove(self):
+        """
+        Install and remove a replica and make sure topology information is
+        updated on all other replicas
+        Testcase: http://www.freeipa.org/page/V4/Manage_replication_topology/
+        Test_plan#Test_case:
+        _Replication_topology_should_be_saved_in_the_LDAP_tree
+        """
+        tasks.kinit_admin(self.master)
+        result1 = self.master.run_command(['ipa', 'topologysegment-find',
+                                           'realm'])
+        first_segment_name = "%s-to-%s" % (self.master.hostname,
+                                           self.replicas[0].hostname)
+        output1 = result1.stdout_text
+        firstsegment = self.tokenize_topologies(output1)[0]
+        assert(firstsegment['name'] == first_segment_name)
+        assert(self.noentries_re.search(output1).group(1) == "1")
+        assert(firstsegment['leftnode'] == self.master.hostname)
+        assert(firstsegment['rightnode'] == self.replicas[0].hostname)
+        tasks.install_replica(self.master, self.replicas[1], setup_ca=False,
+                              setup_dns=False)
+        # We need to make sure topology information is consistent across all
+        # replicas
+        result2 = self.master.run_command(['ipa', 'topologysegment-find',
+                                           'realm'])
+        result3 = self.replicas[0].run_command(['ipa', 'topologysegment-find',
+                                                'realm'])
+        result4 = self.replicas[1].run_command(['ipa', 'topologysegment-find',
+                                                'realm'])
+        segments = self.tokenize_topologies(result2.stdout_text)
+        assert(len(segments) == 2)
+        assert(result2.stdout_text == result3.stdout_text)
+        assert(result3.stdout_text == result4.stdout_text)
+        # Now let's check that uninstalling the replica will update the topology
+        # info on the rest of replicas.
+        tasks.uninstall_master(self.replicas[1])
+        tasks.clean_replication_agreement(self.master, self.replicas[1])
+        result5 = self.master.run_command(['ipa', 'topologysegment-find',
+                                           'realm'])
+        assert(self.noentries_re.search(result5.stdout_text).group(1) == "1")
+
+    def test_add_remove_segment(self):
+        """
+        Make sure a topology segment can be manually created and deleted
+        with the influence on the real topology
+        Testcase http://www.freeipa.org/page/V4/Manage_replication_topology/
+        Test_plan#Test_case:_Basic_CRUD_test
+        """
+        tasks.kinit_admin(self.master)
+        # Install the second replica
+        tasks.install_replica(self.master, self.replicas[1], setup_ca=False,
+                              setup_dns=False)
+        # turn a star into a ring
+        segment, err = tasks.create_segment(self.master,
+                                            self.replicas[0],
+                                            self.replicas[1])
+        assert err == "", err
+        # Make sure the new segment is shown by `ipa topologysegment-find`
+        result1 = self.master.run_command(['ipa', 'topologysegment-find',
+                                           'realm'])
+        assert(result1.stdout_text.find(segment['name']) > 0)
+        # Remove master <-> replica2 segment and make sure that the changes get
+        # there through replica1
+        deleteme = "%s-to-%s" % (self.master.hostname,
+                                 self.replicas[1].hostname)
+        returncode, error = tasks.destroy_segment(self.master, deleteme)
+        assert returncode == 0, error
+        # make sure replica1 does not have segment that was deleted on master
+        result3 = self.replicas[0].run_command(['ipa', 'topologysegment-find',
+                                               'realm'])
+        assert(result3.stdout_text.find(deleteme) < 0)
+        # Create test data on master and make sure it gets all the way down to
+        # replica2 through replica1
+        self.master.run_command(['ipa', 'user-add', 'someuser',
+                                 '--first', 'test',
+                                 '--last', 'user'])
+        time.sleep(60)  # replication requires some time
+        users_on_replica2 = self.replicas[1].run_command(['ipa',
+                                                         'user-find'])
+        assert(users_on_replica2.find('someuser') > 0)
+        # We end up having a line topology: master <-> replica1 <-> replica2
+
+    def test_remove_the_only_connection(self):
+        """
+        Testcase: http://www.freeipa.org/page/V4/Manage_replication_topology/
+        Test_plan#Test_case:
+        _Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica
+        """
+        text = "Removal of Segment disconnects topology"
+        error1 = "The system should not have let you remove the segment"
+        error2 = "Wrong error message thrown during segment removal: \"%s\""
+        replicas = (self.replicas[0].hostname, self.replicas[1].hostname)
+
+        returncode, error = tasks.destroy_segment(self.master, "%s-to-%s" % replicas)
+        assert returncode != 0, error1
+        assert error.count(text) == 1, error2 % error
+        newseg, err = tasks.create_segment(self.master,
+                                           self.master,
+                                           self.replicas[1])
+        assert err == "", err
+        returncode, error = tasks.destroy_segment(self.master, "%s-to-%s" % replicas)
+        assert returncode == 0, error
diff --git a/ipatests/test_ipaserver/test_topology_plugin.py b/ipatests/test_ipaserver/test_topology_plugin.py
index 6678974993cb1762abb01e89a30caa3ebd94e3d0..b162822e0466798b13b49cbd4d30a36780e77e4f 100644
--- a/ipatests/test_ipaserver/test_topology_plugin.py
+++ b/ipatests/test_ipaserver/test_topology_plugin.py
@@ -2,18 +2,23 @@
 # Copyright (C) 2015  FreeIPA Contributors see COPYING for license
 #
 
+import io
 import os
 from ipaserver.plugins.ldap2 import ldap2
 from ipalib import api
 from ipapython import ipautil
 from ipapython.dn import DN
-import nose
+import pytest
 
 
 class TestTopologyPlugin(object):
     """
     Test Topology plugin from the DS point of view
+    Testcase: http://www.freeipa.org/page/V4/Manage_replication_topology/
+    Test_plan#Test_case:
+    _Replication_Topology_is_listed_among_directory_server_plugins
     """
+    pwfile = os.path.join(api.env.dot_ipa, ".dmpw")
 
     def setup(self):
         """
@@ -25,6 +30,8 @@ class TestTopologyPlugin(object):
         if self.conn and self.conn.isconnected():
             self.conn.disconnect()
 
+    @pytest.mark.skipif(ipautil.file_exists(pwfile) is False,
+                        reason="You did not provide a .dmpw file with the DM password")
     def test_topologyplugin(self):
         pluginattrs = {
             u'nsslapd-pluginPath': [u'libtopology'],
@@ -56,11 +63,8 @@ class TestTopologyPlugin(object):
                           ('cn', 'plugins'),
                           ('cn', 'config'))
         pwfile = os.path.join(api.env.dot_ipa, ".dmpw")
-        if ipautil.file_exists(pwfile):
-            with open(pwfile, "r") as f:
-                dm_password = f.read().rstrip()
-        else:
-            raise nose.SkipTest("No directory manager password in %s" % pwfile)
+        with io.open(pwfile, "r") as f:
+            dm_password = f.read().rstrip()
         self.conn = ldap2(api)
         self.conn.connect(bind_dn=DN(('cn', 'directory manager')),
                           bind_pw=dm_password)
-- 
2.4.3

-- 
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