[Freeipa-devel] [TEST][patch-0037]Fixes of dnssec tests
Martin Basti
mbasti at redhat.com
Wed May 11 11:20:43 UTC 2016
On 11.05.2016 12:27, Oleg Fayans wrote:
> Hi Martin,
>
> Here as per discussion on monday meeting, I removed from the patch
> everything unrelated to the workaround itself and added a separate test
> that runs a standard dnssec-enabled zone and queries it without
> restarting of named. The test is marked as xfail with strict=True, which
> means, once the bug is fixed it will start to unexpectedly pass causing
> the whole testrun to fail, so we will know precisely when to revert this
> change. This test is (apart from restarting named) an exact copy of the
> existing one, so it would be safe to remove it.
>
> Patch 0038 was rebased.
>
> On 05/06/2016 04:11 PM, Martin Basti wrote:
>>
>> On 06.05.2016 14:58, Oleg Fayans wrote:
>>> On 05/06/2016 11:42 AM, Martin Basti wrote:
>>>> On 06.05.2016 11:14, Oleg Fayans wrote:
>>>>> On 05/06/2016 09:48 AM, Martin Basti wrote:
>>>>>> On 06.05.2016 09:36, Oleg Fayans wrote:
>>>>>>> Tests are finally stable:
>>>>>>>
>>>>>>> ============================= test session starts
>>>>>>> ==============================
>>>>>>> platform linux2 -- Python 2.7.11 -- py-1.4.30 -- pytest-2.7.3
>>>>>>> rootdir: /usr/lib/python2.7/site-packages/ipatests, inifile:
>>>>>>> pytest.ini
>>>>>>> plugins: multihost, sourceorder
>>>>>>> collected 8 items
>>>>>>>
>>>>>>> test_integration/test_dnssec.py ........
>>>>>>>
>>>>>>> ========================= 8 passed in 5561.48 seconds
>>>>>>> ==========================
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> PATCH 38 LGTM
>>>>>>
>>>>>> PATCH 37 IIRC I refused to accept workaround for this issue when you
>>>>>> send this (almost the same) patch for first time, are you sure that we
>>>>>> want to hide real issues in tests, to just have green color there?
>>>>>>
>>>>> The underlying issue is 7 months old. Latest update in the issue from
>>>>> Peter Spacek is: "I do not have time to investigate this issue now",
>>>>> which means, that it will stay there for unpredictable amount of time
>>>>> more. If we want to have a "green" jenkins that actually tests existing
>>>>> features, we have to accept workarounds for such long-term issues
>>>>>
>>>>>> Martin
>>>> I leave decision if push this or not to different people, however I will
>>>> do review on this.
>>>>
>>>>
>>>> NACK
>>>>
>>>> 1)
>>>> Why do you change sleep time? How is it related to workaround?
>>>>
>>>> - time.sleep(20) # sleep a bit until LDAP changes are applied to
>>>> DNS
>>>> + time.sleep(10) # sleep a bit until LDAP changes are applied to
>>>> DNS
>>> 10 seconds proved to be enough even in our super-slow brno rhevm lab
>> Unrelated to workaround, send it as new patch
>>
>>>> 2)
>>>> why do you removes sleep from several places? How is this related to
>>>> workaround?
>>>> @@ -281,13 +302,19 @@ class TestInstallDNSSECFirst(IntegrationTest):
>>>> "--a-rec=" + self.master.ip
>>>> ]
>>>> self.master.run_command(args)
>>>> - time.sleep(10) # sleep a bit until data are provided by
>>>> bind-dyndb-ldap
>>>>
>>>> args = [
>>>> "ipa", "dnsrecord-add", root_zone,
>>>> self.master.domain.name,
>>>> "--ns-rec=" + self.master.hostname
>>>> ]
>>>> self.master.run_command(args)
>>> Because it's more reasonable to make changes on all hosts and then wait.
>> Now, this is NACK.
>> Yes, that is true wait when all changes are done, but you completely
>> misunderstood why that sleep is there.
>> Sleep is there because an A record was added, and it took time until
>> this change is propagated to DNS, without A record following command
>> (adding NS record) will fail.
>> Bind-dyndb-ldap feels happy so it can work today, but it may not work
>> tomorrow.
>>
>>>> 3)
>>>> You restart the same replica twice
>>>> + time.sleep(10) # sleep a bit until LDAP changes are applied to
>>>> DNS
>>>> + # A workaround for ticket N 5348
>>>> + self.replicas[0].run_command(["systemctl", "restart",
>>>> + "named-pkcs11.service"])
>>>> + self.replicas[0].run_command(["systemctl", "restart",
>>>> + "named-pkcs11.service"])
>>>> + # End of workaround
>>> My bad.
>>>
>>>> 4)
>>>> Can you create a function doing workaround instead of copying the same
>>>> code several times?
>>>>
>>>> something like
>>>> def restart_named(*args):
>>>> for host in args:
>>>> # host$ systemctl restart named-pkcs11
>>>>
>>>> where args are instances self.host, or self.master, or self.replica
>>> Now, that's a marvelous idea! Implemented. And put the sleep interval in
>>> it too just to keep it in one place
>> I wanted *args to have
>> restart_named(self.replicas[0], self.replicas[1])
>> instead creating an additional list
>> restart_named([self.replicas[0], self.replicas[1]])
>> but whatever it works.
>>
>>>> 5)
>>>> Why did you removed this comment?
>>>> - # wait until zone is signed
>>> Because this is kin of obvious from the name of the function:
>>> wait_until_record_is_signed
>> Unrelated to this workaround, send it as separate patch "Removing
>> mbasti's useless DNSSEC comments"
>>
>>>> 6)
>>>> I'm not sure, but is sleep there needed? Because restart of named will
>>>> download all LDAP data again.
>>> It turns out, without this interval the restart does not always help
>>>
>>>> If timeout is required maybe reason why
>>>> time is there should be rephrased, to something like, make sure the
>>>> dnssec keys were exported for named, or so.
>>> Rephrased
>>>
>>>> + time.sleep(10) # sleep a bit until LDAP changes are applied to
>>>> DNS
>>>> + # A workaround for ticket N 5348
>>>> + self.master.run_command(["systemctl", "restart",
>>>> + "named-pkcs11.service"])
>>>> + self.replicas[0].run_command(["systemctl", "restart",
>>>> + "named-pkcs11.service"])
>>>> + # End of workaround
>>>>
>>>> Martin^2
Patch 0037:
Pushed to:
ipa-4-3: 377d75b98b3e62a6c224940420b61c6e8a7846a1
master: 5567dff4b46cc05bf0ea44dd03afdd12645143a5
Patch 0038:
ACK
Pushed to master: 84e5065b398eec722b30123319dc7725286a2a74
More information about the Freeipa-devel
mailing list