Hi Martin, I am sorry, in my previous email I attached the old version of patch 0016. The correct on is attached.
On 12/16/2015 05:47 PM, Martin Basti wrote: > > > On 16.12.2015 15:39, Martin Basti wrote: >> >> >> On 15.12.2015 10:29, Oleg Fayans wrote: >>> 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 >> LGTM, I haven't had time to test it, but if you are sure that test is >> working, we may push this. >> > Is this expected due the bug you mentioned? > _____ > __________________________________________________________________________ > TestReplicaPromotionLevel0.test_kra_install_master > ________________________________________________________________________________ > > > self = > <ipatests.test_integration.test_replica_promotion.TestReplicaPromotionLevel0 > object at 0x7f5071a59e50> > > def test_kra_install_master(self): > result1 = tasks.install_kra(self.master, raiseonerr=False) >> assert result1.returncode == 0, result1.stderr_text > E AssertionError: Usage: ipa-kra-install [options] [replica_file] > E > E ipa-kra-install: error: Replica file > /root/ipatests/replica-info.gpg does not exist > E The ipa-kra-install command failed. See > /var/log/ipaserver-kra-install.log for more information > E > E assert 2 == 0 > E + where 2 = <pytest_multihost.transport.SSHCommand object at > 0x7f5071adbd50>.returncode > -- Oleg Fayans Quality Engineer FreeIPA team RedHat.
From cd883693fb7d07de662b57370440dae50fddfe5b Mon Sep 17 00:00:00 2001 From: Oleg Fayans <[email protected]> Date: Thu, 17 Dec 2015 08:50:13 +0100 Subject: [PATCH] Prepared integration tests for replica promotion --- .../test_integration/test_replica_promotion.py | 218 +++++++++++++++++++++ 1 file changed, 218 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..e790890db06e52fa155348e39ce4804ecdd1d3bf --- /dev/null +++ b/ipatests/test_integration/test_replica_promotion.py @@ -0,0 +1,218 @@ +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, + first_instance=True, + 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
-- 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
