[Freeipa-devel] [PATCH 016, 024, 025] First part of the replica promotion tests + testplan
Martin Basti
mbasti at redhat.com
Tue Mar 1 13:56:08 UTC 2016
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^2
> My bad I applied wrong patch
>
>>>
>>>
>>> On 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:
>>>>>>>> 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
>>>>>>
>>>> IMO 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] Missing KRA
>>>> 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.TestKRAInstall
>>>> object 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)
2)
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)
3)
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 times
4)
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
+ """
5)
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
upgrade
Test case: ipa-csreplica-manage connect is deprecated in domain level 1
Test case: Replica can be installed using one command
Test case: Prohibit ipa server uninstallation from disconnecting
topology segment
PATCH 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)
2)
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)
3)
why is there this change, how it is related to this patch?:
-
+ args.extend(['-n', replica.domain.name,
+ '-r', replica.domain.realm])
4)
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))
Otherwise domain level related changes LGTM
PATCH 25
LGTM
Martin^2
More information about the Freeipa-devel
mailing list