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