On 04.03.2016 08:37, Martin Basti wrote:
On 03.03.2016 18:38, Martin Basti wrote:On 02.03.2016 13:47, Oleg Fayans wrote:Hi Martin, I've made the requested changes. The full set of necessary patches is attached. On 03/02/2016 10:05 AM, Martin Basti wrote:On 02.03.2016 00:12, Oleg Fayans wrote:Hi Martin, On 03/01/2016 07:04 PM, Martin Basti wrote:On 01.03.2016 14:56, Martin Basti wrote:On 01.03.2016 12:37, Martin Basti wrote:On 01.03.2016 12:32, Martin Basti wrote:On 29.02.2016 13:16, Oleg Fayans wrote:Hi all, Finally the tests pass. The patch 0024 applies on top of patch 0022 (please, consider reviewing it also). Besides, the whole functionality depends on Martin's patch N 0421 All patches pass pylint.hello, I cannot apply patches on master branch Martin^2My bad I applied wrong patchOn 12/19/2015 11:56 PM, Martin Basti wrote:On 17.12.2015 10:04, Oleg Fayans wrote: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:LGTM, I haven't had time to test it, but if you are sure thatHi 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 tocontinue with the review of 0018. This one contains all changesrelated 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 thisbug: 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:Sorry I forgot to apply patch 17, my bad, I'm continuing withOn 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:So this should be in separated patch and investigatedHi Martin, On 12/01/2015 04:08 PM, Martin Basti wrote:Otherwise 2 out of 8 tests fail with IOError at lineOn 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. Bothpatches 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:I do not say that we should not have somethingHi 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 ofhttps://fedorahosted.org/freeipa/ticket/5469during 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 enoughjust 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 shouldallow 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, somethinglike TestReplicaPromotionLevel0 5) please remove this, the patch is on review and it will be pushed sooner than tests + @pytest.mark.xfail # Ticket N 5455and as I mentioned in ticket #5455, I cannotreproduce it with ipa-kra-install, so please provide steps to reproduce if you insist thatthis 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 CIActually, being able to configure domain level per class is WAY moreconvenient, than to always have to think whichdomain level is appropriate for which particular test during jenkins jobconfiguration. In fact, I should have thoughtabout 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 level0. With config-based approach, we would have toimplement a separate step that raises domain level. Overall, I am against the approach,when you have to remember to set certain domainlevel in config for any particular test. The tests themselves should be aware of the domain level they need.that overrides settingsin from config in a particular test case, I sayyour 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 domainlevel1 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 replicapromotion test is, weneed something that allows override the configfile. 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 classderived from TestIntegration When I configure domain level 0 in yaml config, how is this supposed toget into install methods when you removed thatcode? - "--domain-level=%i" % host.config.domain_level + "--domain-level=%i" % domain_levelYou always use MAX_DOMAIN_LEVEL in this case orwhatever is specified in domain_level option.I suggest to use domain_level=None, and whenit is None use'host.config.domain_level', if it is not None,use 'domain_level'With this we can specify domain level in configfile for test that can be used for both domain levels and you can manually specify domain levelfor test that requires specific domain level.Also this should go away @classmethod def install(cls, mh):+ if hasattr(cls, "domain_level") andcls.master: + cls.master.config.domain_level = cls.domain_level if cls.topology is None: returnI do not see reason why test should overrideconfiguration in config in this case. Martin On 25.11.2015 16:44, Oleg Fayans wrote:Hi,Here is the updated version of the patch (moretests + fixed theissues of the first one) + patch 0017, thatimplements the necessarychanges in the background code, i. e. patch 16does not work without patch 17On 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 fromthis (so far incomplete) testplan:http://www.freeipa.org/page/V4/Replica_Promotion/Test_planTestplan review is highly appreciatedPATCH 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 optionas the last parameter. 2)cab you in both tests specify a domain levelwith 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 notot use it? IMO to install master in installstep is enough. Martin^2./make-lint************* Module ipatests.test_integration.baseipatests/test_integration/base.py:66: [E1101(no-member),IntegrationTest.install] Class 'IntegrationTest'has no 'domain_level' member) ************* Moduleipatests.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 postedfor 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])78 of ipatests/test_integration/tasks.pyI do not understand yet how does this happen, but ifyou remove ipatests folder once, it then fails to be created again.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_levelDone3)IMO replica-manage del should cleanup hosts entry, sofollowing 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 staysand 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 removeshost entry, andDNS A records, only SSHFP records stay there (I'm notsure 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. Removed4) This variable is not used + kra_uninstall = ["ipa-kra-install", "--uninstall", "-U"]Removed5) Why do you need this + kra_install = ["ipa-kra-install", "-U", "-p", config.dirman_password]when you implemented tasks.install_kra that returnsthe exactly the same result?Right. RemovedMost of these complaints are unrelated to the current6) 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)patches.It's better to create a separate patch addressing PEP8errors.I beg for your pardon, those PEP8 errors have been introduced by your patches.Fixed7)Why this must be stored in instance? IMO to have itstored as local variable is perfect TestKRAInstall, TestCAInstall self.replica1_filename = tasks.get_replica_filename(replica1) self.replica2_filename = tasks.get_replica_filename(replica2)Agree. FixedThis 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)reviewtest 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.TestReplicaPromotionLevel0object at 0x7f5071a59e50> def test_kra_install_master(self): result1 = tasks.install_kra(self.master, raiseonerr=False)assert result1.returncode == 0, result1.stderr_textE 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>.returncodeIMO the test needs fix, KRA on replica file needs KRA related certificates in replica file[ipa.ipatests.test_integration.host.Host.replica2.ParamikoTransport]RUN ['ipa-kra-install', '-U', '-p', 'Secret123', '/root/ipatests/replica-info.gpg'] [ipa.ipatests.test_integration.host.Host.replica2.cmd27] RUN ['ipa-kra-install', '-U', '-p', 'Secret123', '/root/ipatests/replica-info.gpg'][ipa.ipatests.test_integration.host.Host.replica2.cmd27] MissingKRA certificates, please create a new replica file. [ipa.ipatests.test_integration.host.Host.replica2.cmd27] The ipa-kra-install command failed. See /var/log/ipaserver-kra-install.log for more information [ipa.ipatests.test_integration.host.Host.replica2.cmd27] Exit code: 1 FAILED traceback self =<ipatests.test_integration.test_replica_promotion.TestKRAInstallobject at 0x7f660bc1a590>I just read the code. PATCH 16: 0) PEP8 ./ipatests/test_integration/test_replica_promotion.py:24:14: E111 indentation is not a multiple of four ./ipatests/test_integration/test_replica_promotion.py:24:14: E113 unexpected indentation ./ipatests/test_integration/test_replica_promotion.py:148:80: E501 line too long (80 > 79 characters) ./ipatests/test_integration/test_replica_promotion.py:150:80: E501 line too long (81 > 79 characters) 1) workaround is not workaround, because the host entry is removed anyway, the error is raised from POST callback, please remove it + # Workaround for 5627 + if "host not found" in result.stderr_text: + self.master.run_command(["ipa", + "host-del", + host.hostname], raiseonerr=False)sorry, I was wrong with this, check is in pre_callback, but please remove it anyway, I will send patch to fix it ASAPDoneI realized that the fix I'm working on is for 4.4 only, so for 4.3 add this as separated patch.Done, patch 00272) Please name it better, for example "replica" instead of "i" + for i in self.replicas: + tasks.install_replica(master, i, setup_ca=False, + setup_dns=True)Done3) Please use constant for domain level (multiple times) + result1 = tasks.install_ca(replica1, domain_level=1, raiseonerr=False) + tasks.install_ca(replica1, domain_level=0) + result2 = tasks.install_ca(replica2, domain_level=0, raiseonerr=False) ... more timesDone4) This link does not exists, only connect is deprecated not ipa-replica-manage at all + 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 + """Fixed5) Missing testcases: Test case: Unprivileged users are not allowed to enroll and promote clients Test case: Replica created using old workflow is functional after domain upgradeTest case: ipa-csreplica-manage connect is deprecated in domain level 1Test case: Replica can be installed using one command Test case: Prohibit ipa server uninstallation from disconnecting topology segmentThey are on the way, not fully ready yetPATCH 24: 1) why there is this change, how it is related to this patch?: def apply_common_fixes(host): + prepare_host(host) fix_etc_hosts(host) fix_hostname(host) - prepare_host(host)Good catch! That was one of my attempts to address the issue that wassuccessfully resolved in patch 0025. Will remove it once we agree on therest of the changesRemovedJust to make this call independent from domain level (at domain_level 12) Why is there this change, how it is related to this patch?: def replica_prepare(master, replica): - apply_common_fixes(replica) fix_apache_semaphores(replica) ...def install_replica(master, replica, setup_ca=True, setup_dns=False,... + apply_common_fixes(replica)replica_prepare never gets called)It should be in separate commit, because it is not related to adding domain_level in class functionalityDone. Patch 0026It should be in separate commit, as this is not related to domain levels3) why is there this change, how it is related to this patch?: - + args.extend(['-n', replica.domain.name, + '-r', replica.domain.realm])At least -r is a required parameter. -n was added for further robustness. Can be safely removed, thoughDone. Patch 00264) why there force, how is this change related to this patch (domain levels)? '-w', client.config.admin_password, - '--server', master.hostname] + '--server', master.hostname, + '--force'] + list(extra_args))client refuses to install unless everything is super clear in the dns setup (including reverse zone). Otherwise the installer fails andinforms you that you may use '--force' at your own risk. I can rerun the tests without this option to provide you with the exact output, if youlike.It should be in separated commit, because it is not related to domain levelsI've run the tests without this option again at it passed. Must have been some temporary issue. Removed this change.Otherwise domain level related changes LGTM PATCH 25 LGTM Martin^21) this method is unused please remove it def test_kra_install_master(self): 2) Why are these there? I do not see any usage from env_config import get_global_config config = get_global_config() 3) nitpick + num_clients = 0 this is set by default otherwise LGTM Results of testing tomorrow. Martin^2I applied all patches including workarounds, but test failed.ipatests.test_integration.test_replica_promotion.TestReplicaPromotionLevel0[ipa.ipatests.test_integration.host.Host.replica1.cmd51] RUN ['ipa-replica-install', '-U', '-p', 'Secret123', '-w', 'Secret123', '--setup-ca', '--ip-address', '192.168.144.102', '/root/ipatests/replica-info.gpg'] [ipa.ipatests.test_integration.host.Host.replica1.cmd51] The host replica1.ipa.test already exists on the master server. [ipa.ipatests.test_integration.host.Host.replica1.cmd51] You should remove it before proceeding: [ipa.ipatests.test_integration.host.Host.replica1.cmd51] % ipa host-del replica1.ipa.test [ipa.ipatests.test_integration.host.Host.replica1.cmd51] ipa.ipapython.install.cli.install_tool(Replica): ERROR The ipa-replica-install command failed. See /var/log/ipareplica-install.log for more information[ipa.ipatests.test_integration.host.Host.replica1.cmd51] Exit code: 3 FAILED
And it needs ticket, otherwise it will not be in 4-3 branch. -- 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
