[Freeipa-devel] [PATCH 016 - 017] First part of the replica promotion tests + testplan
Martin Basti
mbasti at redhat.com
Tue Mar 1 11:37:48 UTC 2016
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>
>>>
>
More information about the Freeipa-devel
mailing list