[Freeipa-devel] [PATCH 0011, 0012, 0015] Replica promotion related changes in integration tests
Oleg Fayans
ofayans at redhat.com
Mon Nov 2 13:45:07 UTC 2015
Hi Martin,
On 11/02/2015 12:52 PM, Martin Basti wrote:
>
>
> On 02.11.2015 11:54, Oleg Fayans wrote:
>> Hi Martin,
>>
>> On 11/02/2015 10:39 AM, Martin Basti wrote:
>>>
>>>
>>> On 29.10.2015 18:32, Martin Basti wrote:
>>>>
>>>>
>>>> On 29.10.2015 18:31, Martin Basti wrote:
>>>>> NACK
>>>>>
>>>>> 1)
>>>>> DO NOT use tabs in code to indent
>> Fixed
>>>>>
>>>>> 2)
>>>>> Replica uninstallation does not work, uninstallation works different
>>>>> with domain level 0 and 1 (currently uninstallation with domain 1
>>>>> level will not work, it is known issue, but still the patch should
>>>>> solve the uninstallation)
>>> This is not valid, my bad, I was confused with new behaviour of replica
>>> uninstallation, but it is bug not a feature.
>>> So replica uninstallation is the same for level 0 and 1
>>> Sorry.
>>>>>
>>>>> 3)
>>>>> apply_common_fixes(host)
>>>>> Method for domain_level 1 is called twice, first time in replica
>>>>> install, second time in client install
>> Fixed
>>>>>
>>>>> 4)
>>>>> during testing this patch I used test_simple_replication and I found
>>>>> 4 bugs:
>>>> 3 bugs -----------------^^^
>>>>> #5419, #5420, #5421
>> Bug #5419 fixed, see patch N 15
>
> patch 0015 NACK
>
> 1)
> You fixed just half of the issue, there is also wrong dnsrecord-add
>
> 2)
> I do not think that your solution is the right, it can result in
> failures when the domain 'my.ipa.domain' is used.
>
> I prefer to add '.' to record name and use it as absolute name,
> dnsrecord-* command will handle it.
That's a good point. The problem though, is that dnsrecord-add handles
it correctly, while dnsrecord-find - does not.
$ ipa dnsrecord-add idm.lab.eng.brq.redhat.com.
vm-002.idm.lab.eng.brq.redhat.com. --a-rec 192.168.254.2
Record name: vm-002
A record: 192.168.254.2
$ ipa dnsrecord-find idm.lab.eng.brq.redhat.com.
vm-001.idm.lab.eng.brq.redhat.com.
----------------------------
Number of entries returned 0
----------------------------
$ ipa dnsrecord-find idm.lab.eng.brq.redhat.com. vm-001
Record name: vm-001
A record: 192.168.254.1
----------------------------
Number of entries returned 1
----------------------------
Should I file a ticket against it?
>
> if not host.hostname.endswith('.'):
> host.hostname += '{}.'.format(host.hostname)
>
> And I would replace dnsrecord-find with dnsrecord-show
>
> patches 11, 12 LGTM, I will test them later
>
> Martin^2
>>
>>>>> IMO it is related only to this one test case and to pass this test
>>>>> case #5419 or #5421 must be fixed.
>>>>>
>>>>>
>>>>> On 27.10.2015 16:34, Oleg Fayans wrote:
>>>>>> Hi Martin,
>>>>>>
>>>>>> The updated patch is attached
>>>>>>
>>>>>> On 10/27/2015 01:58 PM, Martin Basti wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 27.10.2015 13:56, Oleg Fayans wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 10/27/2015 01:22 PM, Martin Basti wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 27.10.2015 12:06, Oleg Fayans wrote:
>>>>>>>>>> Hi Martin,
>>>>>>>>>>
>>>>>>>>>> On 10/27/2015 10:50 AM, Martin Basti wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 27.10.2015 10:22, Martin Basti wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 27.10.2015 10:00, Oleg Fayans wrote:
>>>>>>>>>>>>> Hi Martin,
>>>>>>>>>>>>>
>>>>>>>>>>>>> The updated version of the patch is attached. Please, see my
>>>>>>>>>>>>> comments
>>>>>>>>>>>>> below
>>>>>>>>>>>> My comments inline, I may be completely wrong in how the test
>>>>>>>>>>>> suite
>>>>>>>>>>>> work, so feel free to correct me.
>>>>>>>>>>>>
>>>>>>>>>>>> Martin
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 10/26/2015 06:48 PM, Martin Basti wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 26.10.2015 08:59, Oleg Fayans wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 10/23/2015 03:10 PM, Martin Basti wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 23.10.2015 15:00, Oleg Fayans wrote:
>>>>>>>>>>>>>>>>> Hi Martin,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Here comes the updated version.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 10/22/2015 05:38 PM, Martin Basti wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 22.10.2015 15:23, Martin Basti wrote:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> On 22.10.2015 14:13, Oleg Fayans wrote:
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> thank you for the patch.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 1)
>>>>>>>>>>>>>>>>>>> please remove the added empty lines, they are
>>>>>>>>>>>>>>>>>>> unrelated to
>>>>>>>>>>>>>>>>>>> this
>>>>>>>>>>>>>>>>>>> ticket
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> done
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 2)
>>>>>>>>>>>>>>>>>>> -def install_master(host, setup_dns=True,
>>>>>>>>>>>>>>>>>>> setup_kra=False):
>>>>>>>>>>>>>>>>>>> +def install_master(host, setup_dns=True,
>>>>>>>>>>>>>>>>>>> setup_kra=False,
>>>>>>>>>>>>>>>>>>> domainlevel=1):
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I suggest to use default domainlevel=None, which will
>>>>>>>>>>>>>>>>>>> use the
>>>>>>>>>>>>>>>>>>> default
>>>>>>>>>>>>>>>>>>> domain level (specified in build)
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> done
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 3)
>>>>>>>>>>>>>>>>>>> + domain_level = domainlevel(master)
>>>>>>>>>>>>>>>>>>> I do not think that this meets expectations.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> We have to test, both domain level 0 and 1 for IPA 4.3,
>>>>>>>>>>>>>>>>>>> respectively
>>>>>>>>>>>>>>>>>>> new IPA must support all older domain levels, domain
>>>>>>>>>>>>>>>>>>> level is
>>>>>>>>>>>>>>>>>>> independent on IPA version, only admin can raise it up.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> So you have to find out way how to pass the domain
>>>>>>>>>>>>>>>>>>> level for
>>>>>>>>>>>>>>>>>>> which
>>>>>>>>>>>>>>>>>>> test will be running, we were talking about using config
>>>>>>>>>>>>>>>>>>> files,
>>>>>>>>>>>>>>>>>>> but
>>>>>>>>>>>>>>>>>>> feel free to find something new and better
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Fixed. Now, we declare domain level in config.yaml with
>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>> directive
>>>>>>>>>>>>>>>>> domain_level
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 4)
>>>>>>>>>>>>>>>>>>> Did you resolve the pytest fixtures which specifies
>>>>>>>>>>>>>>>>>>> which
>>>>>>>>>>>>>>>>>>> tests
>>>>>>>>>>>>>>>>>>> can be
>>>>>>>>>>>>>>>>>>> run under which domain level?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> In fact, we do not seem to have any tests yet that would
>>>>>>>>>>>>>>>>> require it.
>>>>>>>>>>>>>>>>> All the existing tests just use install_replica
>>>>>>>>>>>>>>>>> method, no matter how is it done.
>>>>>>>>>>>>>>>> How about topology CI test? This can be executed only with
>>>>>>>>>>>>>>>> domain
>>>>>>>>>>>>>>>> level
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> That's right. The topology test was updated. Patch is
>>>>>>>>>>>>>>> attached
>>>>>>>>>>>>>>> together with a proper version of 11-th patch (not a swap
>>>>>>>>>>>>>>> file,
>>>>>>>>>>>>>>> sorry
>>>>>>>>>>>>>>> about that).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 1, right?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 5)
>>>>>>>>>>>>>>>>>>> + '--ip-address', client.ip,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> why this change to client install?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Right, it found to be unnecessary.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Martin^2
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 6)
>>>>>>>>>>>>>>>>>> ************* Module ipatests.test_integration.tasks
>>>>>>>>>>>>>>>>>> ipatests/test_integration/tasks.py:85:
>>>>>>>>>>>>>>>>>> [E1123(unexpected-keyword-arg),
>>>>>>>>>>>>>>>>>> allow_sync_ptr] Unexpected keyword argument
>>>>>>>>>>>>>>>>>> 'raiseonerr' in
>>>>>>>>>>>>>>>>>> function
>>>>>>>>>>>>>>>>>> call)
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Fixed
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 1)
>>>>>>>>>>>>>> + if not host.config.domain_level == None:
>>>>>>>>>>>>>> + args.append("--domain-level=%i" %
>>>>>>>>>>>>>> host.config.domain_level)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> always use: variable *is not None*
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 2)
>>>>>>>>>>>>>> Why there is specified level 1 as default? IIRC we agreed
>>>>>>>>>>>>>> that the
>>>>>>>>>>>>>> default level is the one which is default in tested package.
>>>>>>>>>>>>>> These should be None and "":
>>>>>>>>>>>>>> + "domain_level": "1"
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> + "DOMAINLVL": "1",
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 3)
>>>>>>>>>>>>>> However, as I read the patch 12, and I'm right, the
>>>>>>>>>>>>>> pytest.fixture
>>>>>>>>>>>>>> needs
>>>>>>>>>>>>>> to know the value of domain level before we can do any
>>>>>>>>>>>>>> dynamic
>>>>>>>>>>>>>> detection
>>>>>>>>>>>>>> on master.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So we should use the constants MAX_DOMAIN_LEVEL as default,
>>>>>>>>>>>>>> for 2)
>>>>>>>>>>>>> Done
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Also I'm not sure if the values are inherited from the
>>>>>>>>>>>>>> DEFAULT_OUTPUT_DICT to code, I think it is not, so for this
>>>>>>>>>>>>>> part
>>>>>>>>>>>>>> you
>>>>>>>>>>>>>> need default value, or the fixture will not work as expected.
>>>>>>>>>>>>>> + self.domain_level = kwargs.get('domain_level',
>>>>>>>>>>>>>> MAX_DOMAIN_LEVEL)
>>>>>>>>>>>>>
>>>>>>>>>>>>> This won't work in cases when domainlevel is explicitly set
>>>>>>>>>>>>> to 0 in
>>>>>>>>>>>>> config.yaml. This default value will always override the
>>>>>>>>>>>>> explicit
>>>>>>>>>>>>> one.
>>>>>>>>>>>> wat? in case that kwargs is dict containing values from config
>>>>>>>>>>>> file:
>>>>>>>>>>>>
>>>>>>>>>>>> In [1]: kwargs = dict(domain_level=0)
>>>>>>>>>>>>
>>>>>>>>>>>> In [2]: kwargs.get('domain_level', 123)
>>>>>>>>>>>> Out[2]: 0
>>>>>>>>>>
>>>>>>>>>> Yep, right you are, it works as expected. Now the line looks
>>>>>>>>>> like:
>>>>>>>>>> self.domain_level = kwargs.get('domain_level', MAX_DOMAIN_LEVEL)
>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>> Respectively you should use this:
>>>>>>>>>>>
>>>>>>>>>>> self.domain_level = kwargs.get('domain_level')or
>>>>>>>>>>> MAX_DOMAIN_LEVEL
>>>>>>>>>> Now that would definitely not work in case of domain_level is 0:
>>>>>>>>>> 0 or 1 is always 1
>>>>>>>>> Oh right.
>>>>>>>>>
>>>>>>>>> I had discussion with Tomas, and we should add there check if it
>>>>>>>>> is not
>>>>>>>>> None, in case that kwargs contains {'domain_level': None} (One
>>>>>>>>> does not
>>>>>>>>> know with test when the None value appears there)
>>>>>>>>
>>>>>>>> I do not get this. If we do not specify domain_level in
>>>>>>>> config.yaml,
>>>>>>>> it will automatically be populated with a MAX_DOMAIN_LEVEL value.
>>>>>>>> That
>>>>>>>> means domain_level will never ever possibly be None.
>>>>>>> I do not know how pytest works inside, if you are 100% sure, that
>>>>>>> the
>>>>>>> case written above cannot happen, I'm fine with it.
>>>>>>> Martin
>>>>>>>>
>>>>>>>>>
>>>>>>>>> self.domain_level = kwargs.get('domain_level')
>>>>>>>>> if self.domain_level is None:
>>>>>>>>> self.domain_level = MAX_DOMAIN_LEVEL
>>>>>>>>>
>>>>>>>>> As we cannot user 'or' expression with integers, so this is the
>>>>>>>>> right
>>>>>>>>> way.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> because kwargs IMO contains undefined config values as None
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> freeipa-tests depends on freeipa-python so the constants
>>>>>>>>>>>>>> should be
>>>>>>>>>>>>>> available in tests.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So then you also need update this line
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> + if not host.config.domain_level != MAX_DOMAIN_LEVEL:
>>>>>>>>>>>>>> + args.append("--domain-level=%i" %
>>>>>>>>>>>>>> host.config.domain_level)
>>>>>>>>>>>>>
>>>>>>>>>>>>> This would not work if domainlevel is not set in
>>>>>>>>>>>>> config.yaml, in
>>>>>>>>>>>>> which case the host.config.domain_level is None.
>>>>>>>>>>>> Domain level will never be None because self.domain_level =
>>>>>>>>>>>> kwargs.get('domain_level', MAX_DOMAIN_LEVEL)
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 4)
>>>>>>>>>>>>>> Please add comment to function +def domainlevel(host): that
>>>>>>>>>>>>>> it is
>>>>>>>>>>>>>> useful
>>>>>>>>>>>>>> for test where domain level will be raised dynamically,
>>>>>>>>>>>>>> otherwise it
>>>>>>>>>>>>>> may
>>>>>>>>>>>>>> be lost after test refactoring as somebody may consider it as
>>>>>>>>>>>>>> unneeded
>>>>>>>>>>>>>> and replace it with config dict.
>>>>>>>>>>>>> Done
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So summary is the 1) and 2) are replaced by 3) :)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Martin^2
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
More information about the Freeipa-devel
mailing list