Hi Martin,
On 11/02/2015 10:39 AM, Martin Basti wrote:
On 29.10.2015 18:32, Martin Basti wrote:
On 29.10.2015 18:31, Martin Basti wrote:
NACK
1)
DO NOT use tabs in code to indent
Fixed
2)
Replica uninstallation does not work, uninstallation works different
with domain level 0 and 1 (currently uninstallation with domain 1
level will not work, it is known issue, but still the patch should
solve the uninstallation)
This is not valid, my bad, I was confused with new behaviour of replica
uninstallation, but it is bug not a feature.
So replica uninstallation is the same for level 0 and 1
Sorry.
3)
apply_common_fixes(host)
Method for domain_level 1 is called twice, first time in replica
install, second time in client install
Fixed
4)
during testing this patch I used test_simple_replication and I found
4 bugs:
3 bugs -----------------^^^
#5419, #5420, #5421
Bug #5419 fixed, see patch N 15
IMO it is related only to this one test case and to pass this test
case #5419 or #5421 must be fixed.
On 27.10.2015 16:34, Oleg Fayans wrote:
Hi Martin,
The updated patch is attached
On 10/27/2015 01:58 PM, Martin Basti wrote:
On 27.10.2015 13:56, Oleg Fayans wrote:
On 10/27/2015 01:22 PM, Martin Basti wrote:
On 27.10.2015 12:06, Oleg Fayans wrote:
Hi Martin,
On 10/27/2015 10:50 AM, Martin Basti wrote:
On 27.10.2015 10:22, Martin Basti wrote:
On 27.10.2015 10:00, Oleg Fayans wrote:
Hi Martin,
The updated version of the patch is attached. Please, see my
comments
below
My comments inline, I may be completely wrong in how the test
suite
work, so feel free to correct me.
Martin
On 10/26/2015 06:48 PM, Martin Basti wrote:
On 26.10.2015 08:59, Oleg Fayans wrote:
On 10/23/2015 03:10 PM, Martin Basti wrote:
On 23.10.2015 15:00, Oleg Fayans wrote:
Hi Martin,
Here comes the updated version.
On 10/22/2015 05:38 PM, Martin Basti wrote:
On 22.10.2015 15:23, Martin Basti wrote:
On 22.10.2015 14:13, Oleg Fayans wrote:
Hello,
thank you for the patch.
1)
please remove the added empty lines, they are unrelated to
this
ticket
done
2)
-def install_master(host, setup_dns=True,
setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False,
domainlevel=1):
I suggest to use default domainlevel=None, which will
use the
default
domain level (specified in build)
done
3)
+ domain_level = domainlevel(master)
I do not think that this meets expectations.
We have to test, both domain level 0 and 1 for IPA 4.3,
respectively
new IPA must support all older domain levels, domain
level is
independent on IPA version, only admin can raise it up.
So you have to find out way how to pass the domain
level for
which
test will be running, we were talking about using config
files,
but
feel free to find something new and better
Fixed. Now, we declare domain level in config.yaml with the
directive
domain_level
4)
Did you resolve the pytest fixtures which specifies which
tests
can be
run under which domain level?
In fact, we do not seem to have any tests yet that would
require it.
All the existing tests just use install_replica
method, no matter how is it done.
How about topology CI test? This can be executed only with
domain
level
That's right. The topology test was updated. Patch is attached
together with a proper version of 11-th patch (not a swap
file,
sorry
about that).
1, right?
5)
+ '--ip-address', client.ip,
why this change to client install?
Right, it found to be unnecessary.
Martin^2
6)
************* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85:
[E1123(unexpected-keyword-arg),
allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in
function
call)
Fixed
1)
+ if not host.config.domain_level == None:
+ args.append("--domain-level=%i" %
host.config.domain_level)
always use: variable *is not None*
2)
Why there is specified level 1 as default? IIRC we agreed
that the
default level is the one which is default in tested package.
These should be None and "":
+ "domain_level": "1"
+ "DOMAINLVL": "1",
3)
However, as I read the patch 12, and I'm right, the
pytest.fixture
needs
to know the value of domain level before we can do any dynamic
detection
on master.
So we should use the constants MAX_DOMAIN_LEVEL as default,
for 2)
Done
Also I'm not sure if the values are inherited from the
DEFAULT_OUTPUT_DICT to code, I think it is not, so for this
part
you
need default value, or the fixture will not work as expected.
+ self.domain_level = kwargs.get('domain_level',
MAX_DOMAIN_LEVEL)
This won't work in cases when domainlevel is explicitly set
to 0 in
config.yaml. This default value will always override the
explicit
one.
wat? in case that kwargs is dict containing values from config
file:
In [1]: kwargs = dict(domain_level=0)
In [2]: kwargs.get('domain_level', 123)
Out[2]: 0
Yep, right you are, it works as expected. Now the line looks like:
self.domain_level = kwargs.get('domain_level', MAX_DOMAIN_LEVEL)
Respectively you should use this:
self.domain_level = kwargs.get('domain_level')or MAX_DOMAIN_LEVEL
Now that would definitely not work in case of domain_level is 0:
0 or 1 is always 1
Oh right.
I had discussion with Tomas, and we should add there check if it
is not
None, in case that kwargs contains {'domain_level': None} (One
does not
know with test when the None value appears there)
I do not get this. If we do not specify domain_level in config.yaml,
it will automatically be populated with a MAX_DOMAIN_LEVEL value.
That
means domain_level will never ever possibly be None.
I do not know how pytest works inside, if you are 100% sure, that the
case written above cannot happen, I'm fine with it.
Martin
self.domain_level = kwargs.get('domain_level')
if self.domain_level is None:
self.domain_level = MAX_DOMAIN_LEVEL
As we cannot user 'or' expression with integers, so this is the
right
way.
because kwargs IMO contains undefined config values as None
freeipa-tests depends on freeipa-python so the constants
should be
available in tests.
So then you also need update this line
+ if not host.config.domain_level != MAX_DOMAIN_LEVEL:
+ args.append("--domain-level=%i" %
host.config.domain_level)
This would not work if domainlevel is not set in config.yaml, in
which case the host.config.domain_level is None.
Domain level will never be None because self.domain_level =
kwargs.get('domain_level', MAX_DOMAIN_LEVEL)
4)
Please add comment to function +def domainlevel(host): that
it is
useful
for test where domain level will be raised dynamically,
otherwise it
may
be lost after test refactoring as somebody may consider it as
unneeded
and replace it with config dict.
Done
So summary is the 1) and 2) are replaced by 3) :)
Martin^2
--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 6d0fca9b6029fb3f5af1e84b9e7fc32b67eb37c3 Mon Sep 17 00:00:00 2001
From: Oleg Fayans <ofay...@redhat.com>
Date: Mon, 2 Nov 2015 11:51:34 +0100
Subject: [PATCH] Updated the tests according to the new replica installation
workflow
As of 4.3 the replica installation is performed without preparing a gpg file on
master, but rather enrolling a future replica as a client with subsequent
promotion of the client. This required the corresponding change in the
integration tests
https://fedorahosted.org/freeipa/ticket/5379
---
ipatests/test_integration/config.py | 6 +++-
ipatests/test_integration/tasks.py | 43 ++++++++++++++++++++++------
ipatests/test_integration/test_testconfig.py | 5 +++-
3 files changed, 44 insertions(+), 10 deletions(-)
diff --git a/ipatests/test_integration/config.py b/ipatests/test_integration/config.py
index 785a9bb8c420f99980c098887e0bd31422119564..0e4ac1f1d99c8c9cba8dfbb497cfde0075c6a84a 100644
--- a/ipatests/test_integration/config.py
+++ b/ipatests/test_integration/config.py
@@ -26,6 +26,7 @@ import pytest_multihost.config
from ipapython.dn import DN
from ipapython.ipa_log_manager import log_mgr
+from ipalib.constants import MAX_DOMAIN_LEVEL
class Config(pytest_multihost.config.Config):
@@ -39,6 +40,7 @@ class Config(pytest_multihost.config.Config):
'ad_admin_name',
'ad_admin_password',
'dns_forwarder',
+ 'domain_level',
}
def __init__(self, **kwargs):
@@ -56,10 +58,12 @@ class Config(pytest_multihost.config.Config):
'%s.pool.ntp.org' % random.randint(0, 3)))
self.ad_admin_name = kwargs.get('ad_admin_name') or 'Administrator'
self.ad_admin_password = kwargs.get('ad_admin_password') or 'Secret123'
-
+ self.domain_level = kwargs.get('domain_level', MAX_DOMAIN_LEVEL)
# 8.8.8.8 is probably the best-known public DNS
self.dns_forwarder = kwargs.get('dns_forwarder') or '8.8.8.8'
self.debug = False
+ if self.domain_level is None:
+ self.domain_level = MAX_DOMAIN_LEVEL
def get_domain_class(self):
return Domain
diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index e241454a984aac97eb2d0955f55bb83d85bf9d4c..307563af3dfa9d37ee308f2348d95c4c0f3ae44c 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -79,6 +79,12 @@ def prepare_host(host):
host.put_file_contents(env_filename, env_to_script(host.to_env()))
+def allow_sync_ptr(host):
+ kinit_admin(host)
+ host.run_command(["ipa", "dnsconfig-mod", "--allow-sync-ptr=true"],
+ raiseonerr=False)
+
+
def apply_common_fixes(host):
fix_etc_hosts(host)
fix_hostname(host)
@@ -260,7 +266,8 @@ def install_master(host, setup_dns=True, setup_kra=False):
'ipa-server-install', '-U',
'-r', host.domain.name,
'-p', host.config.dirman_password,
- '-a', host.config.admin_password
+ '-a', host.config.admin_password,
+ "--domain-level=%i" % host.config.domain_level
]
if setup_dns:
@@ -288,6 +295,18 @@ def get_replica_filename(replica):
return os.path.join(replica.config.test_dir, 'replica-info.gpg')
+def domainlevel(host):
+ # Dynamically determines the domainlevel on master. Needed for scenarios
+ # when domainlevel is changed during the test execution.
+ result = host.run_command(['ipa', 'domainlevel-get'], raiseonerr=False)
+ level = 0
+ domlevel_re = re.compile('.*(\d)')
+ if result.returncode == 0:
+ # "domainlevel-get" command doesn't exist on ipa versions prior to 4.3
+ level = int(domlevel_re.findall(result.stdout_text)[0])
+ return level
+
+
def replica_prepare(master, replica):
apply_common_fixes(replica)
fix_apache_semaphores(replica)
@@ -306,15 +325,13 @@ def install_replica(master, replica, setup_ca=True, setup_dns=False,
setup_kra=False):
replica.collect_log(paths.IPAREPLICA_INSTALL_LOG)
replica.collect_log(paths.IPAREPLICA_CONNCHECK_LOG)
-
- replica_prepare(master, replica)
-
- replica_filename = get_replica_filename(replica)
+ allow_sync_ptr(master)
+ # Otherwise ipa-client-install would not create a PTR
+ # and replica installation would fail
args = ['ipa-replica-install', '-U',
'-p', replica.config.dirman_password,
'-w', replica.config.admin_password,
- '--ip-address', replica.ip,
- replica_filename]
+ '--ip-address', replica.ip]
if setup_ca:
args.append('--setup-ca')
if setup_dns:
@@ -322,8 +339,18 @@ def install_replica(master, replica, setup_ca=True, setup_dns=False,
'--setup-dns',
'--forwarder', replica.config.dns_forwarder
])
+ if domainlevel(master) == 0:
+ apply_common_fixes(replica)
+ # prepare the replica file on master and put it to replica, AKA "old way"
+ replica_prepare(master, replica)
+ replica_filename = get_replica_filename(replica)
+ args.append(replica_filename)
+ else:
+ # install client on a replica machine and then promote it to replica
+ install_client(master, replica)
+ fix_apache_semaphores(replica)
+
replica.run_command(args)
-
enable_replication_debugging(replica)
setup_sssd_debugging(replica)
diff --git a/ipatests/test_integration/test_testconfig.py b/ipatests/test_integration/test_testconfig.py
index 8d146fcff0dd9729393a30bf45e37c581b528e68..5c40522ed6bf0e4715e3b7ad160fbf7fbfdca9bc 100644
--- a/ipatests/test_integration/test_testconfig.py
+++ b/ipatests/test_integration/test_testconfig.py
@@ -23,6 +23,7 @@ import copy
from ipatests.test_integration import config
from ipapython.ipautil import write_tmp_file
from ipatests.util import assert_deepequal
+from ipalib.constants import MAX_DOMAIN_LEVEL
DEFAULT_OUTPUT_DICT = {
"nis_domain": "ipatest",
@@ -39,7 +40,8 @@ DEFAULT_OUTPUT_DICT = {
"dirman_dn": "cn=Directory Manager",
"dirman_password": "Secret123",
"ntp_server": "ntp.clock.test",
- "admin_password": "Secret123"
+ "admin_password": "Secret123",
+ "domain_level": MAX_DOMAIN_LEVEL
}
DEFAULT_OUTPUT_ENV = {
@@ -57,6 +59,7 @@ DEFAULT_OUTPUT_ENV = {
"ADADMINPW": "Secret123",
"IPv6SETUP": "",
"IPADEBUG": "",
+ "DOMAINLVL": MAX_DOMAIN_LEVEL
}
DEFAULT_INPUT_ENV = {
--
2.4.3
From 0305af62add5733dbbaee9468caa1b6fd41e57df Mon Sep 17 00:00:00 2001
From: Oleg Fayans <ofay...@redhat.com>
Date: Mon, 2 Nov 2015 11:48:43 +0100
Subject: [PATCH] Fixed A record creation bug
When creating an A record we used to provide full hostname as a record name,
while we should have provided only the first part of the hostname
https://fedorahosted.org/freeipa/ticket/5419
---
ipatests/test_integration/tasks.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index e241454a984aac97eb2d0955f55bb83d85bf9d4c..ab56720500418fc9c839676f72a8368914ea49dd 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -861,7 +861,7 @@ def add_a_record(master, host):
cmd = master.run_command(['ipa',
'dnsrecord-find',
master.domain.name,
- host.hostname,
+ host.hostname.split(".")[0],
'--a-rec', host.ip],
raiseonerr=False)
--
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
