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
--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 3569551e68b4f02a0ea9161c47d8c1ec2bdbf041 Mon Sep 17 00:00:00 2001
From: Oleg Fayans <ofay...@redhat.com>
Date: Tue, 11 Aug 2015 15:34:38 +0200
Subject: [PATCH] Added first part of integration tests for topology plugin
---
ipatests/test_integration/tasks.py | 58 +++++++++++-
ipatests/test_integration/test_topology.py | 115 ++++++++++++++++++++++++
ipatests/test_ipaserver/test_topology_plugin.py | 13 +--
3 files changed, 178 insertions(+), 8 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 2b83f274619fcb18bcb87118d7df35a85ce52c1a..44b9a78b8a82e721817251f6b0e0bc45a8145a2e 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -40,6 +40,23 @@ 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))
+ 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')
@@ -243,7 +260,6 @@ def install_replica(master, replica, setup_ca=True, setup_dns=False):
'--forwarder', replica.config.dns_forwarder
])
replica.run_command(args)
-
enable_replication_debugging(replica)
setup_sssd_debugging(replica)
@@ -507,7 +523,8 @@ def uninstall_master(host):
paths.SYSCONFIG_PKI_TOMCAT,
paths.SYSCONFIG_PKI_TOMCAT_PKI_TOMCAT_DIR,
paths.VAR_LIB_PKI_TOMCAT_DIR,
- paths.PKI_TOMCAT],
+ paths.PKI_TOMCAT,
+ paths.REPLICA_INFO_GPG_TEMPLATE % host.hostname],
raiseonerr=False)
unapply_fixes(host)
@@ -519,6 +536,43 @@ def uninstall_client(host):
raiseonerr=False)
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 on
+ :returns: a hash object containing segment's name, leftnode, rightnode information
+ """
+ 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])
+ return {'leftnode': lefthost,
+ 'rightnode': righthost,
+ 'name': segment_name}
+
+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)
+ return master.run_command(["ipa",
+ "topologysegment-del",
+ "realm",
+ segment_name])
+
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..395e9cf3c0ee1c4adb71611c8236fa972791dc3a
--- /dev/null
+++ b/ipatests/test_integration/test_topology.py
@@ -0,0 +1,115 @@
+#
+# 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'
+ segname_re = re.compile("Segment name: ([\w\.\-]+)")
+ noentries_re = re.compile("Number of entries returned (\d+)")
+ leftnode_re = re.compile("Left node: ([\w\.]+)")
+ rightnode_re = re.compile("Right node: ([\w\.]+)")
+ connectivity_re = re.compile("Connectivity: ([\w\-]+)")
+
+ @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:
+ if self.segname_re.search(i):
+ result.append({'leftnode': self.leftnode_re.search(i).group(),
+ 'rightnode': self.rightnode_re.search(i).group(),
+ 'name': self.segname_re.search(i).group(),
+ 'connectivity': self.connectivity_re.search(i).group()})
+ 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
+ """
+ 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")
+ 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")
+ def test_add_remove_segment(self):
+ """
+ Make sure a topology segment can be manually created and deleted
+ with the influence on the real topology
+ The corresponding testcase can be found at
+ 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)
+ result1 = self.master.run_command(['ipa', 'topologysegment-find',
+ 'realm'])
+ # turn a star into a ring
+ segment = tasks.create_segment(self.master,
+ self.replicas[0],
+ self.replicas[1])
+ # Make sure the new segment is shown by `ipa topologysegment-find`
+ result2 = self.master.run_command(['ipa', 'topologysegment-find',
+ 'realm'])
+ assert(result2.stdout_text.find(segment['name']) > 0)
+ # Remove master <-> replica2 segment and make sure that the changes get
+ # there through replica1
+ deleteme = self.segname_re.findall(result1.stdout_text)[1]
+ tasks.destroy_segment(self.master, deleteme)
+ # 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)
+ 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
diff --git a/ipatests/test_ipaserver/test_topology_plugin.py b/ipatests/test_ipaserver/test_topology_plugin.py
index 6678974993cb1762abb01e89a30caa3ebd94e3d0..6d4efc6bc15e77bab94f6c5135cb24c28f9c50e4 100644
--- a/ipatests/test_ipaserver/test_topology_plugin.py
+++ b/ipatests/test_ipaserver/test_topology_plugin.py
@@ -2,18 +2,20 @@
# 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
"""
+ pwfile = os.path.join(api.env.dot_ipa, ".dmpw")
def setup(self):
"""
@@ -25,6 +27,8 @@ class TestTopologyPlugin(object):
if self.conn and self.conn.isconnected():
self.conn.disconnect()
+ @pytest.mark.skipif(ipautil.file_exists(pwfile) == 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 +60,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
