[Freeipa-devel] [PATCH 0011, 0012, 0015] Replica promotion related changes in integration tests

Oleg Fayans ofayans at redhat.com
Mon Nov 2 15:05:20 UTC 2015 


On 11/02/2015 03:45 PM, Martin Basti wrote:
>
>
> On 02.11.2015 14:45, Oleg Fayans wrote:
>> 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.

Done

>> 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?
>>
> I do not think that this is a bug, dnsrecord-find does search in
> multiple attributes not only in record names
>
> I suggest to use dnsrecord-show instead of dnsrecord-find
>>
>>>
>>> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0015.1-Fixed-A-record-creation-bug.patch
Type: text/x-patch
Size: 1654 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151102/0487cfaf/attachment.bin>

More information about the Freeipa-devel mailing list