Hi Martin,
The updated patches are attached. Patch 0017 includes all changes from
patch 0018, so, if you approve this one, there would be no need to
continue with the review of 0018. This one contains all changes related
to you remarks from 0018 review. Please see my explanation on the
stdout+stderr part in the thread from patch 0018.
With these two patches applied one of the tests fails due this bug:
https://fedorahosted.org/freeipa/ticket/5550
On 12/09/2015 12:17 PM, Martin Basti wrote:
>
>
> On 09.12.2015 12:10, Martin Basti wrote:
>>
>>
>> On 09.12.2015 11:14, Oleg Fayans wrote:
>>> Hi Martin
>>>
>>> On 12/09/2015 10:30 AM, Martin Basti wrote:
>>>>
>>>> On 08.12.2015 23:48, Oleg Fayans wrote:
>>>>> Substituted a hardcoded suffix name with a constant DOMAIN_SUFFIX_NAME
>>>>>
>>>>> On 12/08/2015 02:33 PM, Oleg Fayans wrote:
>>>>>> Hi all,
>>>>>>
>>>>>>
>>>>>> The patches are rebased against the current master.
>>>>>>
>>>>>> On 12/02/2015 05:10 PM, Martin Basti wrote:
>>>>>>> On 02.12.2015 16:18, Oleg Fayans wrote:
>>>>>>>> Hi Martin,
>>>>>>>>
>>>>>>>> On 12/01/2015 04:08 PM, Martin Basti wrote:
>>>>>>>>> On 27.11.2015 16:26, Oleg Fayans wrote:
>>>>>>>>>> And patch N 16 passes lint too:
>>>>>>>>>>
>>>>>>>>>> On 11/27/2015 04:03 PM, Oleg Fayans wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On 11/27/2015 03:26 PM, Martin Basti wrote:
>>>>>>>>>>>> On 27.11.2015 15:04, Oleg Fayans wrote:
>>>>>>>>>>>>> Hi Martin,
>>>>>>>>>>>>>
>>>>>>>>>>>>> All your suggestions were taken into account. Both patches are
>>>>>>>>>>>>> updated. Thank you for your help!
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 11/26/2015 10:50 AM, Martin Basti wrote:
>>>>>>>>>>>>>> On 26.11.2015 10:04, Oleg Fayans wrote:
>>>>>>>>>>>>>>> Hi Martin,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I agree to all your points but one. please, see my comment
>>>>>>>>>>>>>>> below
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 11/25/2015 07:42 PM, Martin Basti wrote:
>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 0) Note
>>>>>>>>>>>>>>>> Please be aware of
>>>>>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/5469
>>>>>>>>>>>>>>>> during
>>>>>>>>>>>>>>>> KRA testing
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 1)
>>>>>>>>>>>>>>>> Please do not use MIN and MAX_DOMAIN_LEVEL constants,
>>>>>>>>>>>>>>>> this may
>>>>>>>>>>>>>>>> change
>>>>>>>>>>>>>>>> over time, use DOMAIN_LEVEL_0 and DOMAIN_LEVEL_1 for domain
>>>>>>>>>>>>>>>> level 0
>>>>>>>>>>>>>>>> and 1
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 2)
>>>>>>>>>>>>>>>> Why uninstall KRA then server, is not enough just uninstall
>>>>>>>>>>>>>>>> server
>>>>>>>>>>>>>>>> which
>>>>>>>>>>>>>>>> covers KRA uninstall?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> + def teardown_method(self, method):
>>>>>>>>>>>>>>>> + for host in self.replicas:
>>>>>>>>>>>>>>>> + host.run_command(self.kra_uninstall,
>>>>>>>>>>>>>>>> raiseonerr=False)
>>>>>>>>>>>>>>>> + tasks.uninstall_master(host)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 3)
>>>>>>>>>>>>>>>> Can be this function more generic? It should allow specify
>>>>>>>>>>>>>>>> host
>>>>>>>>>>>>>>>> where
>>>>>>>>>>>>>>>> KRA should be installed not just master
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> + def test_kra_install_master(self):
>>>>>>>>>>>>>>>> + self.master.run_command(self.kra_install)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 4)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> TestLevel0(Dummy):
>>>>>>>>>>>>>>>> Can be the test name more specific, something like
>>>>>>>>>>>>>>>> TestReplicaPromotionLevel0
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 5)
>>>>>>>>>>>>>>>> please remove this, the patch is on review and it will be
>>>>>>>>>>>>>>>> pushed
>>>>>>>>>>>>>>>> sooner
>>>>>>>>>>>>>>>> than tests
>>>>>>>>>>>>>>>> + @pytest.mark.xfail # Ticket N 5455
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> and as I mentioned in ticket #5455, I cannot reproduce
>>>>>>>>>>>>>>>> it with
>>>>>>>>>>>>>>>> ipa-kra-install, so please provide steps to reproduce if
>>>>>>>>>>>>>>>> you
>>>>>>>>>>>>>>>> insist
>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>> this still does not work as expected with KRA.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 6) This is completely wrong, it removes everything that we
>>>>>>>>>>>>>>>> tried to
>>>>>>>>>>>>>>>> achieve with previous patches with domain level in CI
>>>>>>>>>>>>>>> Actually, being able to configure domain level per class
>>>>>>>>>>>>>>> is WAY
>>>>>>>>>>>>>>> more
>>>>>>>>>>>>>>> convenient, than to always have to think which domain
>>>>>>>>>>>>>>> level is
>>>>>>>>>>>>>>> appropriate for which particular test during jenkins job
>>>>>>>>>>>>>>> configuration. In fact, I should have thought about it
>>>>>>>>>>>>>>> from the
>>>>>>>>>>>>>>> very
>>>>>>>>>>>>>>> beginning. For example, in test_replica_promotion.py we
>>>>>>>>>>>>>>> have on
>>>>>>>>>>>>>>> class,
>>>>>>>>>>>>>>> which intiates with domain level = 1, while others - with
>>>>>>>>>>>>>>> domain
>>>>>>>>>>>>>>> level
>>>>>>>>>>>>>>> 0. With config-based approach, we would have to implement a
>>>>>>>>>>>>>>> separate
>>>>>>>>>>>>>>> step that raises domain level. Overall, I am against the
>>>>>>>>>>>>>>> approach,
>>>>>>>>>>>>>>> when you have to remember to set certain domain level in
>>>>>>>>>>>>>>> config
>>>>>>>>>>>>>>> for
>>>>>>>>>>>>>>> any particular test. The tests themselves should be aware of
>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>> domain level they need.
>>>>>>>>>>>>>> I do not say that we should not have something that overrides
>>>>>>>>>>>>>> settings
>>>>>>>>>>>>>> in from config in a particular test case, I say your patch is
>>>>>>>>>>>>>> doing it
>>>>>>>>>>>>>> wrong.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I agree it is useful to have param domain_level in
>>>>>>>>>>>>>> install_master,
>>>>>>>>>>>>>> and
>>>>>>>>>>>>>> intall_topo methods, but is cannot be MAX_DOMAIN_LEVEL by
>>>>>>>>>>>>>> default,
>>>>>>>>>>>>>> because with your current patch the domain_level in config is
>>>>>>>>>>>>>> not
>>>>>>>>>>>>>> used
>>>>>>>>>>>>>> at all, it will be always MAX_DOMAIN_LEVEL
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> For example I want to achieve this goal:
>>>>>>>>>>>>>> test_vault.py, this test suite can run on domain level1
>>>>>>>>>>>>>> and on
>>>>>>>>>>>>>> domain
>>>>>>>>>>>>>> level0, so with one test we can test 2 domain levels just
>>>>>>>>>>>>>> with
>>>>>>>>>>>>>> putting
>>>>>>>>>>>>>> domain level into config file.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I agree that with extraordinary test like replica
>>>>>>>>>>>>>> promotion test
>>>>>>>>>>>>>> is, we
>>>>>>>>>>>>>> need something that allows override the config file.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> As I said bellow, domain_level default value should be
>>>>>>>>>>>>>> None in
>>>>>>>>>>>>>> install_master and install_topo plugin. If domain level was
>>>>>>>>>>>>>> specified
>>>>>>>>>>>>>> use the specified one, if not (value is None) use the domain
>>>>>>>>>>>>>> level
>>>>>>>>>>>>>> from
>>>>>>>>>>>>>> config file.
>>>>>>>>>>>>> Agreed :)
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Martin
>>>>>>>>>>>>>>>> [PATCH] Enabled setting domain_level per class derived from
>>>>>>>>>>>>>>>> TestIntegration
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> When I configure domain level 0 in yaml config, how is this
>>>>>>>>>>>>>>>> supposed to
>>>>>>>>>>>>>>>> get into install methods when you removed that code?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> - "--domain-level=%i" % host.config.domain_level
>>>>>>>>>>>>>>>> + "--domain-level=%i" % domain_level
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> You always use MAX_DOMAIN_LEVEL in this case or whatever is
>>>>>>>>>>>>>>>> specified in
>>>>>>>>>>>>>>>> domain_level option.
>>>>>>>>>>>>>>>> I suggest to use domain_level=None, and when it is None use
>>>>>>>>>>>>>>>> 'host.config.domain_level', if it is not None, use
>>>>>>>>>>>>>>>> 'domain_level'
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> With this we can specify domain level in config file for
>>>>>>>>>>>>>>>> test
>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>> can
>>>>>>>>>>>>>>>> be used for both domain levels and you can manually specify
>>>>>>>>>>>>>>>> domain
>>>>>>>>>>>>>>>> level
>>>>>>>>>>>>>>>> for test that requires specific domain level.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Also this should go away
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> @classmethod
>>>>>>>>>>>>>>>> def install(cls, mh):
>>>>>>>>>>>>>>>> + if hasattr(cls, "domain_level") and cls.master:
>>>>>>>>>>>>>>>> + cls.master.config.domain_level = cls.domain_level
>>>>>>>>>>>>>>>> if cls.topology is None:
>>>>>>>>>>>>>>>> return
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I do not see reason why test should override
>>>>>>>>>>>>>>>> configuration in
>>>>>>>>>>>>>>>> config in
>>>>>>>>>>>>>>>> this case.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Martin
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 25.11.2015 16:44, Oleg Fayans wrote:
>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Here is the updated version of the patch (more tests +
>>>>>>>>>>>>>>>>> fixed the
>>>>>>>>>>>>>>>>> issues of the first one) + patch 0017, that implements the
>>>>>>>>>>>>>>>>> necessary
>>>>>>>>>>>>>>>>> changes in the background code, i. e. patch 16 does not
>>>>>>>>>>>>>>>>> work
>>>>>>>>>>>>>>>>> without
>>>>>>>>>>>>>>>>> patch 17
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 11/18/2015 05:20 PM, Martin Basti wrote:
>>>>>>>>>>>>>>>>>> On 09.11.2015 15:09, Oleg Fayans wrote:
>>>>>>>>>>>>>>>>>>> Hi guys,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Here are first two automated testcases from this (so far
>>>>>>>>>>>>>>>>>>> incomplete)
>>>>>>>>>>>>>>>>>>> testplan:
>>>>>>>>>>>>>>>>>>> http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Testplan review is highly appreciated
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> PATCH 16: NACK
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 1)
>>>>>>>>>>>>>>>>>> What is the reason to add an unused parameter to
>>>>>>>>>>>>>>>>>> 'domain_level' to
>>>>>>>>>>>>>>>>>> install_topo()?
>>>>>>>>>>>>>>>>>> Also it is good practise to add new option as the last
>>>>>>>>>>>>>>>>>> parameter.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 2)
>>>>>>>>>>>>>>>>>> cab you in both tests specify a domain level with
>>>>>>>>>>>>>>>>>> constant
>>>>>>>>>>>>>>>>>> instead of
>>>>>>>>>>>>>>>>>> number literal?
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 3)
>>>>>>>>>>>>>>>>>> both test call install_topo with custom domain level,
>>>>>>>>>>>>>>>>>> but it
>>>>>>>>>>>>>>>>>> cannot
>>>>>>>>>>>>>>>>>> work
>>>>>>>>>>>>>>>>>> because 1) (did you run the test?)
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 4)
>>>>>>>>>>>>>>>>>> How the test "TestLevel1" is supposed to work?
>>>>>>>>>>>>>>>>>> Respectively why there is call of install_topo() that
>>>>>>>>>>>>>>>>>> installs
>>>>>>>>>>>>>>>>>> replica.
>>>>>>>>>>>>>>>>>> As this test just tests that ipa-replica-prepare is not
>>>>>>>>>>>>>>>>>> working
>>>>>>>>>>>>>>>>>> anymore,
>>>>>>>>>>>>>>>>>> is it worth to spend 20 minutes with installing
>>>>>>>>>>>>>>>>>> replica and
>>>>>>>>>>>>>>>>>> then
>>>>>>>>>>>>>>>>>> just no
>>>>>>>>>>>>>>>>>> tot use it? IMO to install master in install step is
>>>>>>>>>>>>>>>>>> enough.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Martin^2
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>> ./make-lint
>>>>>>>>>>>> ************* Module ipatests.test_integration.base
>>>>>>>>>>>> ipatests/test_integration/base.py:66: [E1101(no-member),
>>>>>>>>>>>> IntegrationTest.install] Class 'IntegrationTest' has no
>>>>>>>>>>>> 'domain_level'
>>>>>>>>>>>> member)
>>>>>>>>>>>> ************* Module
>>>>>>>>>>>> ipatests.test_integration.test_replica_promotion
>>>>>>>>>>>> ipatests/test_integration/test_replica_promotion.py:16:
>>>>>>>>>>>> [E1101(no-member), Dummy.install] Class 'Dummy' has no
>>>>>>>>>>>> 'domain_level'
>>>>>>>>>>>> member)
>>>>>>>>>>>> ipatests/test_integration/test_replica_promotion.py:117:
>>>>>>>>>>>> [E1101(no-member),
>>>>>>>>>>>> TestCAInstall.test_ca_install_without_replica_file]
>>>>>>>>>>>> Module 'ipatests.test_integration.tasks' has no 'setup_replica'
>>>>>>>>>>>> member)
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Is it so hard to run pylint before patch is posted for review?
>>>>>>>>>>> Sorry, my bad. Fixed.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>> 1)
>>>>>>>>> Why is this change in the patch?
>>>>>>>>> - # Clean up the test directory
>>>>>>>>> - host.run_command(['rm', '-rvf', host.config.test_dir])
>>>>>>>> Otherwise 2 out of 8 tests fail with IOError at line 78 of
>>>>>>>> ipatests/test_integration/tasks.py
>>>>>>>>
>>>>>>>> I do not understand yet how does this happen, but if you remove
>>>>>>>> ipatests folder once, it then fails to be created again.
>>>>>>>>
>>>>>>> So this should be in separated patch and investigated properly.
>>>>>> Agree. Removed
>>>>>>>>> -
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 2)
>>>>>>>>> is enough to have this check only in install_master,
>>>>>>>>> install_topo can
>>>>>>>>> pass None to install_master
>>>>>>>>> + if domain_level is None:
>>>>>>>>> + domain_level = master.config.domain_level
>>>>>>>> Done
>>>>>>>>
>>>>>>>>> 3)
>>>>>>>>> IMO replica-manage del should cleanup hosts entry, so following
>>>>>>>>> code
>>>>>>>>> should not be needed.
>>>>>>>>> + if cleanhost:
>>>>>>>>> + kinit_admin(master)
>>>>>>>>> + master.run_command(["ipa", "host-del", "--updatedns",
>>>>>>>>> replica.hostname],
>>>>>>>>> + raiseonerr=False)
>>>>>>>> Well, in fact it does not. At least the corresponding dns record
>>>>>>>> stays
>>>>>>>> and causes the subsequent ipa-client-install to fail. Probably
>>>>>>>> it's a
>>>>>>>> bug. On the other hand, if I promote an existing client to
>>>>>>>> replica and
>>>>>>>> then delete this replica, then, I probably want the host record
>>>>>>>> (that
>>>>>>>> was created during client-install) to stay in the system. So,
>>>>>>>> does not
>>>>>>>> look like a bug to me.
>>>>>>> No you don't, because replica uninstallation also removes the
>>>>>>> client.
>>>>>>>
>>>>>>> I tried it with ipa42, ipa-replica-manage del removes host entry,
>>>>>>> and
>>>>>>> DNS A records, only SSHFP records stay there (I'm not sure if it
>>>>>>> is bug
>>>>>>> or feature)
>>>>>>>
>>>>>>> Also I received this message
>>>>>>> """
>>>>>>> Failed to cleanup replica1.ipa.test DNS entries: no matching entry
>>>>>>> found
>>>>>>> You may need to manually remove them from the tree
>>>>>>> """
>>>>>>> But, A record has been removed, so this is probably false
>>>>>>> positive and
>>>>>>> it needs to have a ticket.
>>>>>> Agree, that was an issue with my setup.
>>>>>> Removed
>>>>>>
>>>>>>>>> 4)
>>>>>>>>> This variable is not used
>>>>>>>>> + kra_uninstall = ["ipa-kra-install", "--uninstall", "-U"]
>>>>>>>> Removed
>>>>>>>>
>>>>>>>>> 5)
>>>>>>>>> Why do you need this
>>>>>>>>> + kra_install = ["ipa-kra-install", "-U", "-p",
>>>>>>>>> config.dirman_password]
>>>>>>>>> when you implemented tasks.install_kra that returns the exactly
>>>>>>>>> the same
>>>>>>>>> result?
>>>>>>>> Right. Removed
>>>>>>>>
>>>>>>>>> 6)
>>>>>>>>> PEP8
>>>>>>>>> ./ipatests/test_integration/tasks.py:928:1: E302 expected 2 blank
>>>>>>>>> lines,
>>>>>>>>> found 1
>>>>>>>>> ./ipatests/test_integration/tasks.py:934:1: E302 expected 2 blank
>>>>>>>>> lines,
>>>>>>>>> found 1
>>>>>>>>> ./ipatests/test_integration/tasks.py:939:1: E302 expected 2 blank
>>>>>>>>> lines,
>>>>>>>>> found 1
>>>>>>>>> ./ipatests/test_integration/tasks.py:943:1: E302 expected 2 blank
>>>>>>>>> lines,
>>>>>>>>> found 1
>>>>>>>>> ./ipatests/test_integration/tasks.py:950:80: E501 line too long
>>>>>>>>> (80 > 79
>>>>>>>>> characters)
>>>>>>>>>
>>>>>>>>> ./ipatests/test_integration/test_replica_promotion.py:29:80: E501
>>>>>>>>> line
>>>>>>>>> too long (85 > 79 characters)
>>>>>>>>> ./ipatests/test_integration/test_replica_promotion.py:64:80: E501
>>>>>>>>> line
>>>>>>>>> too long (85 > 79 characters)
>>>>>>>>> ./ipatests/test_integration/test_replica_promotion.py:67:80: E501
>>>>>>>>> line
>>>>>>>>> too long (88 > 79 characters)
>>>>>>>>> ./ipatests/test_integration/test_replica_promotion.py:93:80: E501
>>>>>>>>> line
>>>>>>>>> too long (80 > 79 characters)
>>>>>>>>> ./ipatests/test_integration/test_replica_promotion.py:94:80: E501
>>>>>>>>> line
>>>>>>>>> too long (83 > 79 characters)
>>>>>>>>> ./ipatests/test_integration/test_replica_promotion.py:118:80: E501
>>>>>>>>> line
>>>>>>>>> too long (81 > 79 characters)
>>>>>>>>> ./ipatests/test_integration/test_replica_promotion.py:128:80: E501
>>>>>>>>> line
>>>>>>>>> too long (80 > 79 characters)
>>>>>>>>> ./ipatests/test_integration/test_replica_promotion.py:129:80: E501
>>>>>>>>> line
>>>>>>>>> too long (82 > 79 characters)
>>>>>>>>> ./ipatests/test_integration/test_replica_promotion.py:181:80: E501
>>>>>>>>> line
>>>>>>>>> too long (80 > 79 characters)
>>>>>>>> Most of these complaints are unrelated to the current patches.
>>>>>>>> It's better to create a separate patch addressing PEP8 errors.
>>>>>>>>
>>>>>>> I beg for your pardon, those PEP8 errors have been introduced by
>>>>>>> your
>>>>>>> patches.
>>>>>> Fixed
>>>>>>
>>>>>>>>> 7)
>>>>>>>>> Why this must be stored in instance? IMO to have it stored as
>>>>>>>>> local
>>>>>>>>> variable is perfect
>>>>>>>>> TestKRAInstall, TestCAInstall
>>>>>>>>> self.replica1_filename =
>>>>>>>>> tasks.get_replica_filename(replica1)
>>>>>>>>> self.replica2_filename =
>>>>>>>>> tasks.get_replica_filename(replica2)
>>>>>>>> Agree. Fixed
>>>>>>>>
>>>>>>
>>>> This patch is missing something.
>>> I am sorry, I forgot to revert my previous change. The correct patch is
>>> attached
>>>
>> ************* Module ipatests.test_integration.test_replica_promotion
>> ipatests/test_integration/test_replica_promotion.py:15:
>> [E1123(unexpected-keyword-arg), Dummy.install] Unexpected keyword
>> argument 'domain_level' in function call)
>> ipatests/test_integration/test_replica_promotion.py:15:
>> [E1101(no-member), Dummy.install] Class 'Dummy' has no 'domain_level'
>> member)
>> ipatests/test_integration/test_replica_promotion.py:19:
>> [E1101(no-member), Dummy.teardown_method] Module
>> 'ipatests.test_integration.tasks' has no 'uninstall_replica' member)
>> ipatests/test_integration/test_replica_promotion.py:67:
>> [E1101(no-member), TestReplicaPromotionLevel0.test_backup_restore]
>> Module 'ipatests.test_integration.tasks' has no 'ipa_backup' member)
>> ipatests/test_integration/test_replica_promotion.py:72:
>> [E1101(no-member), TestReplicaPromotionLevel0.test_backup_restore]
>> Module 'ipatests.test_integration.tasks' has no 'ipa_restore' member)
>> ipatests/test_integration/test_replica_promotion.py:120:
>> [E1123(unexpected-keyword-arg), TestCAInstall.install] Unexpected
>> keyword argument 'domain_level' in function call)
>>
> Sorry I forgot to apply patch 17, my bad, I'm continuing with review
--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 8e6017645ca4fb5d7705321bfc707016724b8274 Mon Sep 17 00:00:00 2001
From: Oleg Fayans <ofay...@redhat.com>
Date: Wed, 9 Dec 2015 11:12:14 +0100
Subject: [PATCH] Prepared integration tests for replica promotion
---
.../test_integration/test_replica_promotion.py | 216 +++++++++++++++++++++
1 file changed, 216 insertions(+)
create mode 100644 ipatests/test_integration/test_replica_promotion.py
diff --git a/ipatests/test_integration/test_replica_promotion.py b/ipatests/test_integration/test_replica_promotion.py
new file mode 100644
index 0000000000000000000000000000000000000000..f2a9597d79c0e129c795e9cab4add813df8b6f3a
--- /dev/null
+++ b/ipatests/test_integration/test_replica_promotion.py
@@ -0,0 +1,216 @@
+from ipatests.test_integration.base import IntegrationTest
+from ipatests.test_integration import tasks
+from ipatests.test_integration.test_caless import assert_error
+from ipalib.constants import DOMAIN_LEVEL_0
+from ipalib.constants import DOMAIN_LEVEL_1
+from env_config import get_global_config
+from ipalib.constants import DOMAIN_SUFFIX_NAME
+
+
+class Dummy(IntegrationTest):
+ config = get_global_config()
+
+ @classmethod
+ def install(cls, mh):
+ tasks.install_master(cls.master, domain_level=cls.domain_level)
+
+ def teardown_method(self, method):
+ for host in self.replicas:
+ tasks.uninstall_replica(self.master, host)
+ tasks.uninstall_client(host)
+ self.master.run_command(["ipa",
+ "host-del",
+ "--updatedns",
+ host.hostname], raiseonerr=False)
+
+ def test_kra_install_master(self):
+ result1 = tasks.install_kra(self.master, raiseonerr=False)
+ assert result1.returncode == 0, result1.stderr_text
+ tasks.kinit_admin(self.master)
+ result2 = self.master.run_command(["ipa", "vault-find"],
+ raiseonerr=False)
+ found = result2.stdout_text.find("0 vaults matched")
+ assert(found > 0), result2.stdout_text
+
+
+class TestReplicaPromotionLevel0(Dummy):
+
+ topology = 'star'
+ num_replicas = 1
+ domain_level = DOMAIN_LEVEL_0
+
+ def test_promotion_disabled(self):
+ """
+ Testcase:
+ http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan#Test_case:
+ _Make_sure_the_feature_is_unavailable_under_domain_level_0
+ """
+ client = self.replicas[0]
+ tasks.install_client(self.master, client)
+ args = ['ipa-replica-install', '-U',
+ '-p', self.master.config.dirman_password,
+ '-w', self.master.config.admin_password,
+ '--ip-address', client.ip]
+ result = client.run_command(args, raiseonerr=False)
+ assert_error(result,
+ 'You must provide a file generated by ipa-replica-prepare'
+ ' to create a replica when the domain is at level 0', 1)
+
+ def test_backup_restore(self):
+ """
+ TestCase:
+ http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan#Test_case:
+ _ipa-restore_after_domainlevel_raise_restores_original_domain_level
+ """
+ command = ["ipa", "topologysegment-find", DOMAIN_SUFFIX_NAME]
+ tasks.install_replica(self.master, self.replicas[0])
+ backup_file = tasks.ipa_backup(self.master)
+ self.master.run_command(["ipa", "domainlevel-set", "1"])
+ result = self.master.run_command(command)
+ found1 = result.stdout_text.rfind("1 segment matched")
+ assert(found1 > 0), result.stdout_text
+ tasks.ipa_restore(self.master, backup_file)
+ result2 = self.master.run_command(command, raiseonerr=False)
+ found2 = result2.stdout_text.rfind("0 segments matched")
+ assert(found2 > 0), result2.stdout_text
+
+
+class TestKRAInstall(IntegrationTest):
+ """
+ TestCase: http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
+ #Test_case:_ipa-kra-install_with_replica_file_works_only_on_domain_level_0
+ """
+ config = get_global_config()
+ topology = 'star'
+ num_clients = 0
+ domain_level = DOMAIN_LEVEL_0
+ num_replicas = 2
+
+ def test_kra_install_without_replica_file(self):
+ master = self.master
+ replica1 = self.replicas[0]
+ replica2 = self.replicas[1]
+ tasks.install_kra(master, first_instance=True)
+ result1 = tasks.install_kra(replica1,
+ domain_level=DOMAIN_LEVEL_1,
+ raiseonerr=False)
+ assert_error(result1, "A replica file is required", 1)
+ tasks.install_kra(replica1,
+ domain_level=DOMAIN_LEVEL_0,
+ raiseonerr=True)
+ # Now prepare the replica file, copy it to the client and raise
+ # domain level on master to test the reverse situation
+ master.run_command(["ipa", "domainlevel-set", str(DOMAIN_LEVEL_1)])
+ result2 = tasks.install_kra(replica2,
+ domain_level=DOMAIN_LEVEL_0,
+ raiseonerr=False)
+ assert_error(result2, "No replica file is required", 1)
+ tasks.install_kra(replica2)
+
+
+class TestCAInstall(IntegrationTest):
+ config = get_global_config()
+ topology = 'star'
+ num_clients = 0
+ domain_level = DOMAIN_LEVEL_0
+ num_replicas = 2
+
+ @classmethod
+ def install(cls, mh):
+ tasks.install_master(cls.master, domain_level=cls.domain_level)
+
+ def test_ca_install_without_replica_file(self):
+ """
+ TestCase:
+ http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan#Test_case:
+ _ipa-ca-install_with_replica_file_works_only_on_domain_level_0
+ """
+ master = self.master
+ replica1 = self.replicas[0]
+ replica2 = self.replicas[1]
+ for i in self.replicas:
+ tasks.install_replica(master, i, setup_ca=False,
+ setup_dns=True)
+ result1 = tasks.install_ca(replica1, domain_level=1, raiseonerr=False)
+ assert_error(result1, "A replica file is required", 1)
+ tasks.install_ca(replica1, domain_level=0)
+ # Now prepare the replica file, copy it to the client and raise
+ # domain level on master to test the reverse situation
+ master.run_command(["ipa", "domainlevel-set", str(DOMAIN_LEVEL_1)])
+ result2 = tasks.install_ca(replica2, domain_level=0, raiseonerr=False)
+ assert_error(result2, 'Too many parameters provided.'
+ ' No replica file is required', 1)
+ tasks.install_ca(replica2, domain_level=1)
+
+
+class TestReplicaPromotionLevel1(Dummy):
+ """
+ TestCase: http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan#
+ Test_case:_Make_sure_the_old_workflow_is_disabled_at_domain_level_1
+ """
+
+ topology = 'star'
+ num_clients = 0
+ num_replicas = 1
+ domain_level = DOMAIN_LEVEL_1
+
+ def test_replica_prepare_disabled(self):
+ config = self.master.config
+ replica = self.replicas[0]
+ args = ['ipa-replica-prepare',
+ '-p', config.dirman_password,
+ '--ip-address', replica.ip,
+ replica.hostname]
+ result = self.master.run_command(args, raiseonerr=False)
+ assert_error(result, "Replica creation using 'ipa-replica-prepare'"
+ " to generate replica file\n"
+ "is supported only in 0-level IPA domain", 1)
+
+
+class TestReplicaManageCommands(IntegrationTest):
+ topology = "star"
+ num_replicas = 2
+ num_clients = 0
+ domain_level = DOMAIN_LEVEL_0
+
+ def test_replica_manage_commands(self):
+ """
+ TestCase: http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
+ #Test_case:_ipa-replica-manage_is_deprecated_in_domain_level_1
+ """
+ master = self.master
+ replica1 = self.replicas[0]
+ replica2 = self.replicas[1]
+
+ result1 = master.run_command(["ipa-replica-manage",
+ "connect",
+ replica1.hostname,
+ replica2.hostname],
+ raiseonerr=False)
+ assert result1.returncode == 0, result1.stderr_text
+ result2 = master.run_command(["ipa-replica-manage",
+ "disconnect",
+ replica1.hostname,
+ replica2.hostname],
+ raiseonerr=False)
+ assert result2.returncode == 0, result2.stderr_text
+ master.run_command(["ipa", "domainlevel-set", str(DOMAIN_LEVEL_1)])
+ result3 = master.run_command(["ipa-replica-manage",
+ "connect",
+ replica1.hostname,
+ replica2.hostname],
+ raiseonerr=False)
+ assert_error(result3, 'Creation of IPA replication agreement is'
+ ' deprecated with managed IPA replication'
+ ' topology. Please use `ipa topologysegment-*`'
+ ' commands to manage the topology', 1)
+ tasks.create_segment(master, replica1, replica2)
+ result4 = master.run_command(["ipa-replica-manage",
+ "disconnect",
+ replica1.hostname,
+ replica2.hostname],
+ raiseonerr=False)
+ assert_error(result4, 'Removal of IPA replication agreement is'
+ ' deprecated with managed IPA replication'
+ ' topology. Please use `ipa topologysegment-*`'
+ ' commands to manage the topology', 1)
--
2.4.3
From ed5e29f1f9e020aa34a6b7286d860d2e166f4d8a Mon Sep 17 00:00:00 2001
From: Oleg Fayans <ofay...@redhat.com>
Date: Mon, 14 Dec 2015 19:13:30 +0100
Subject: [PATCH] Enabled setting domain_level per class derived from
TestIntegration
---
ipatests/test_integration/base.py | 9 ++++++-
ipatests/test_integration/tasks.py | 51 +++++++++++++++++++++++++++-----------
2 files changed, 44 insertions(+), 16 deletions(-)
diff --git a/ipatests/test_integration/base.py b/ipatests/test_integration/base.py
index 4f57e959032a5fda0ad002fca95501da1160605b..a34c1502cf1c2a2d8ed73c05b1d706d71ac257b1 100644
--- a/ipatests/test_integration/base.py
+++ b/ipatests/test_integration/base.py
@@ -37,6 +37,7 @@ class IntegrationTest(object):
num_ad_domains = 0
required_extra_roles = []
topology = None
+ domain_level = None
@classmethod
def setup_class(cls):
@@ -62,11 +63,17 @@ class IntegrationTest(object):
@classmethod
def install(cls, mh):
+ if cls.domain_level is not None:
+ domain_level = cls.domain_level
+ else:
+ domain_level = cls.master.config.domain_level
if cls.topology is None:
return
else:
tasks.install_topo(cls.topology,
- cls.master, cls.replicas, cls.clients)
+ cls.master, cls.replicas,
+ cls.clients, domain_level)
+
@classmethod
def teardown_class(cls):
pass
diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index eb1378ef4ba8b5ec27034c8a0c429fd6114cd0a0..d1504070c819d3fdf4858734c1ddd0f750607b55 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -256,7 +256,9 @@ def enable_replication_debugging(host):
stdin_text=logging_ldif)
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False, domain_level=None):
+ if domain_level is None:
+ domain_level = host.config.domain_level
host.collect_log(paths.IPASERVER_INSTALL_LOG)
host.collect_log(paths.IPACLIENT_INSTALL_LOG)
inst = host.domain.realm.replace('.', '-')
@@ -266,13 +268,11 @@ def install_master(host, setup_dns=True, setup_kra=False):
apply_common_fixes(host)
fix_apache_semaphores(host)
- args = [
- 'ipa-server-install', '-U',
- '-r', host.domain.name,
- '-p', host.config.dirman_password,
- '-a', host.config.admin_password,
- "--domain-level=%i" % host.config.domain_level
- ]
+ args = ['ipa-server-install', '-U',
+ '-r', host.domain.name,
+ '-p', host.config.dirman_password,
+ '-a', host.config.admin_password,
+ "--domain-level=%i" % domain_level]
if setup_dns:
args.extend([
@@ -351,6 +351,7 @@ def install_replica(master, replica, setup_ca=True, setup_dns=False,
args.append(replica_filename)
else:
# install client on a replica machine and then promote it to replica
+ apply_common_fixes(replica)
install_client(master, replica)
fix_apache_semaphores(replica)
@@ -814,13 +815,13 @@ def tree2_topo(master, replicas):
master = replica
-def install_topo(topo, master, replicas, clients,
+def install_topo(topo, master, replicas, clients, domain_level=None,
skip_master=False, setup_replica_cas=True):
"""Install IPA servers and clients in the given topology"""
replicas = list(replicas)
installed = {master}
if not skip_master:
- install_master(master)
+ install_master(master, domain_level=domain_level)
add_a_records_for_hosts_in_master_domain(master)
@@ -932,10 +933,23 @@ def resolve_record(nameserver, query, rtype="SOA", retry=True, timeout=100):
time.sleep(1)
+def ipa_backup(master):
+ result = master.run_command(["ipa-backup"])
+ path_re = re.compile("^Backed up to (?P<backup>.*)$", re.MULTILINE)
+ matched = path_re.search(result.stdout_text + result.stderr_text)
+ return matched.group("backup")
+
+
+def ipa_restore(master, backup_path):
+ master.run_command(["ipa-restore", "-U",
+ "-p", master.config.dirman_password,
+ backup_path])
+
+
def install_kra(host, domain_level=None, first_instance=False, raiseonerr=True):
- if not domain_level:
- domain_level = domainlevel(host)
- command = ["ipa-kra-install", "-U"]
+ if domain_level is None:
+ domain_level = domainlevel(host)
+ command = ["ipa-kra-install", "-U", "-p", host.config.dirman_password]
if domain_level == DOMAIN_LEVEL_0 and not first_instance:
replica_file = get_replica_filename(host)
command.append(replica_file)
@@ -943,8 +957,8 @@ def install_kra(host, domain_level=None, first_instance=False, raiseonerr=True):
def install_ca(host, domain_level=None, first_instance=False, raiseonerr=True):
- if not domain_level:
- domain_level = domainlevel(host)
+ if domain_level is None:
+ domain_level = domainlevel(host)
command = ["ipa-ca-install", "-U", "-p", host.config.dirman_password,
"-P", 'admin', "-w", host.config.admin_password]
if domain_level == DOMAIN_LEVEL_0 and not first_instance:
@@ -960,3 +974,10 @@ def install_dns(host, raiseonerr=True):
"-U",
]
return host.run_command(args, raiseonerr=raiseonerr)
+
+
+def uninstall_replica(master, replica):
+ master.run_command(["ipa-replica-manage", "del", "--force",
+ "-p", master.config.dirman_password,
+ replica.hostname], raiseonerr=False)
+ uninstall_master(replica)
--
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
