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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to