[Freeipa-devel] [PATCH] First part of the replica promotion tests + testplan

[Martin Basti]

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.

>> -
>>
>>
>> 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.
>
>>
>> 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.
>>
>> 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
>