[Freeipa-devel] [PATCH] First part of the replica promotion tests + testplan
Martin Basti
mbasti at redhat.com
Wed Dec 9 11:10:38 UTC 2015
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)
More information about the Freeipa-devel
mailing list