[Freeipa-devel] [TEST][patch-0037]Fixes of dnssec tests

[Oleg Fayans]

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

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

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

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

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

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0037.1-A-workaround-for-ticket-N-5348.patch
Type: text/x-patch
Size: 8037 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160506/f99118e8/attachment.bin>