Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Tomas Babej]

On 10/09/2015 09:15 AM, Jan Pazdziora wrote:
> On Fri, Oct 09, 2015 at 09:01:46AM +0200, Milan Kubík wrote:
>>
>> Perhaps we could use pytest's expected fail (xfail) or skip marker. [1] It
>> would prevent test from failing in the report and once the underlying issue
>> is fixed, it will raise as an unexpected pass.
>>
>> It could be used as a temporary solution, once the issue is fixed, we would
>> remove the mark from the test. This would probably need some workflow to be
>> defined for these cases.
> 
> That works but please note that this is not about test passing or
> failing, this is about some extra steps needed in the test body to
> achieve deterministic situation in which running that final check
> makes sense.
> 
> I can imagine that simple
> 
>   # workaround 5348
>   time.sleep(20)
> 
> and then some script which would find all these comments and compare
> them to resolved tickets might be enough.
> 

I like this idea the most. Keeping this information in Trac is not much
practical. Having a note in the comment annotating the particular
workaround, however, is quite neat.

Imho we can start such convention. Keeping a keyword in a comment is not
a heavy process. Also, I wouldn't be strict about it, as we already have
a couple of workarounds, and not every time a workaround has a exact
mapping to a particular ticket.

Tomas

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Jan Pazdziora]

On Fri, Oct 09, 2015 at 10:31:32AM +0200, Tomas Babej wrote:
> 
> a heavy process. Also, I wouldn't be strict about it, as we already have
> a couple of workarounds, and not every time a workaround has a exact
> mapping to a particular ticket.

Frankly, this worries me much more than not having ticket for some
trivial change to the code base.

If there's workaround in tests, it's some action that we do but
users/admins don't in their real setups. And that can cause failures
for our users that we don't see or even no longer know about because
in our tests, we've cleverly worked around them.

Either that workaround step is needed and needs to be documented, or
that step should not be needed and there should be a ticket describing
the issue.

-- 
Jan Pazdziora
Senior Principal Software Engineer, Identity Management Engineering, Red Hat

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Petr Spacek]

On 9.10.2015 11:03, Jan Pazdziora wrote:
> On Fri, Oct 09, 2015 at 10:31:32AM +0200, Tomas Babej wrote:
>>
>> a heavy process. Also, I wouldn't be strict about it, as we already have
>> a couple of workarounds, and not every time a workaround has a exact
>> mapping to a particular ticket.
> 
> Frankly, this worries me much more than not having ticket for some
> trivial change to the code base.
> 
> If there's workaround in tests, it's some action that we do but
> users/admins don't in their real setups. And that can cause failures
> for our users that we don't see or even no longer know about because
> in our tests, we've cleverly worked around them.
> 
> Either that workaround step is needed and needs to be documented, or
> that step should not be needed and there should be a ticket describing
> the issue.

+1

-- 
Petr^2 Spacek

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Jan Pazdziora]

On Fri, Oct 09, 2015 at 09:01:46AM +0200, Milan Kubík wrote:
>
> Perhaps we could use pytest's expected fail (xfail) or skip marker. [1] It
> would prevent test from failing in the report and once the underlying issue
> is fixed, it will raise as an unexpected pass.
> 
> It could be used as a temporary solution, once the issue is fixed, we would
> remove the mark from the test. This would probably need some workflow to be
> defined for these cases.

That works but please note that this is not about test passing or
failing, this is about some extra steps needed in the test body to
achieve deterministic situation in which running that final check
makes sense.

I can imagine that simple

# workaround 5348
time.sleep(20)

and then some script which would find all these comments and compare
them to resolved tickets might be enough.

-- 
Jan Pazdziora
Senior Principal Software Engineer, Identity Management Engineering, Red Hat

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Milan Kubík]

On 10/08/2015 02:50 PM, Martin Basti wrote:



On 10/08/2015 02:39 PM, Martin Kosek wrote:

On 10/08/2015 02:08 PM, Oleg Fayans wrote:

Hi,

On 10/08/2015 11:18 AM, Jan Pazdziora wrote:

On Thu, Oct 08, 2015 at 11:12:37AM +0200, Oleg Fayans wrote:

When the ticket is addressed and these workarounds are no longer
needed -- what is our process for finding these workarounds and
reverting them, so that the tests test the real, expected behaviour?
As per discussion with Martin Basti, it was decided that this 
workaround
will only be applied to the current 4-2 branch, not to the 
upstream. In

That sounds like a reasonable plan for this issue.


upstream the issue itself will (supposedly) be solved

Except currently it's not, so (IIUIC) you will keep having
nondeterministic failures in master.

I was mostly interested in the general approach that we have to
workarounds -- how do we track them, how do we make sure they don't
stick in tests forever, even after the issue was already properly
addressed.

That's actually a great point. I personally would like tickets to 
have one more
field: "workaround" containing the address of a workaround in the 
format
"path_to_the_file:line_number" or better even - a commit id of this 
workaround,
so that once the ticket is resolved, we could easily find what to 
reverse.


Please don't add any more trac fields, there is too many of them 
already :-)

Keyword may serve better for now...


+1

new trac field for a few workarounds per year is not worth it.

Perhaps we could use pytest's expected fail (xfail) or skip marker. [1] 
It would prevent test from failing in the report and once the underlying 
issue is fixed, it will raise as an unexpected pass.


It could be used as a temporary solution, once the issue is fixed, we 
would remove the mark from the test. This would probably need some 
workflow to be defined for these cases.


[1]: https://pytest.org/latest/skipping.html

--
Milan Kubik

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Milan Kubík]

On 10/09/2015 09:01 AM, Milan Kubík wrote:

On 10/08/2015 02:50 PM, Martin Basti wrote:



On 10/08/2015 02:39 PM, Martin Kosek wrote:

On 10/08/2015 02:08 PM, Oleg Fayans wrote:

Hi,

On 10/08/2015 11:18 AM, Jan Pazdziora wrote:

On Thu, Oct 08, 2015 at 11:12:37AM +0200, Oleg Fayans wrote:

When the ticket is addressed and these workarounds are no longer
needed -- what is our process for finding these workarounds and
reverting them, so that the tests test the real, expected 
behaviour?
As per discussion with Martin Basti, it was decided that this 
workaround
will only be applied to the current 4-2 branch, not to the 
upstream. In

That sounds like a reasonable plan for this issue.


upstream the issue itself will (supposedly) be solved

Except currently it's not, so (IIUIC) you will keep having
nondeterministic failures in master.

I was mostly interested in the general approach that we have to
workarounds -- how do we track them, how do we make sure they don't
stick in tests forever, even after the issue was already properly
addressed.

That's actually a great point. I personally would like tickets to 
have one more
field: "workaround" containing the address of a workaround in the 
format
"path_to_the_file:line_number" or better even - a commit id of this 
workaround,
so that once the ticket is resolved, we could easily find what to 
reverse.


Please don't add any more trac fields, there is too many of them 
already :-)

Keyword may serve better for now...


+1

new trac field for a few workarounds per year is not worth it.

Perhaps we could use pytest's expected fail (xfail) or skip marker. 
[1] It would prevent test from failing in the report and once the 
underlying issue is fixed, it will raise as an unexpected pass.


It could be used as a temporary solution, once the issue is fixed, we 
would remove the mark from the test. This would probably need some 
workflow to be defined for these cases.


[1]: https://pytest.org/latest/skipping.html

I write faster than I read. The issue at hand here is diffeerent use 
case. :)


--
Milan Kubik

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Martin Basti]

On 09.10.2015 22:04, Oleg Fayans wrote:



On 10/09/2015 11:03 AM, Jan Pazdziora wrote:

On Fri, Oct 09, 2015 at 10:31:32AM +0200, Tomas Babej wrote:


a heavy process. Also, I wouldn't be strict about it, as we already 
have

a couple of workarounds, and not every time a workaround has a exact
mapping to a particular ticket.


Frankly, this worries me much more than not having ticket for some
trivial change to the code base.

If there's workaround in tests, it's some action that we do but
users/admins don't in their real setups. And that can cause failures
for our users that we don't see or even no longer know about because
in our tests, we've cleverly worked around them.


I guess, the global question of whether to do workarounds in tests to 
make them pass or not is older than the very oldest profession on earth.
I must admit, I am on Jan's side here. I would prefer to implement the 
approach proposed by Milan: mark the test scenario as expected failure 
(if it is crucial to make the whole run pass), or better even to leave 
it as it is: just so that each red CI run would remind of the 
necessity to fix the original issue.


This all is a theory, however. What do we do with this particular 
case? It's an edge case (does anyone except root zone admins sign a 
root zone?). Should we probably disable the whole scenario? Or just 
leave it failing as it is?




This bug does not happen just for root zone. Other zones are affected too.
I would leave it failing, we have to fix it.



Either that workaround step is needed and needs to be documented, or
that step should not be needed and there should be a ticket describing
the issue.





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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Martin Basti]

On 10/08/2015 11:18 AM, Oleg Fayans wrote:

Done
On 10/08/2015 10:37 AM, Martin Basti wrote:



On 10/08/2015 09:13 AM, Oleg Fayans wrote:

Hi Martin

On 10/07/2015 04:30 PM, Martin Basti wrote:



On 10/07/2015 04:13 PM, Oleg Fayans wrote:

subj




Workaround looks good, but I prefer not to push it in upstream tests,
because it is not test failure.

I agree, we should rather fix the original issue. But as a temporary
solution, to satisfy downstream, it could do.


Why is there this sleep, this might be useful in upstream tests 
too, but

what is the reason to add sleep there?


Without it I kept getting this error:
E   CalledProcessError: Command '['drill', '@localhost', '-k',
'/etc/trusted-key.key', '-S', 'example.test.', 'SOA']' returned
non-zero exit status 29

with --pdb option, though, my attempts to re-run the command
succeeded, so I assumed it was a timing issue, and indeed, this 1
second sleep helped.



  # verify signatures
+time.sleep(1)
  args = [




Attached is an updated version of the patch with Martin's remarks
taken into account

Can you please send this as separate patch? I would like to push this 
one.



ACK

Pushed to:
master: 2b4354f37e7e775dae833d5e2e8824b43800855f
ipa-4-2: f076da99946c0405162c88174e771a5cecfe9664

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Oleg Fayans]

On 10/09/2015 11:03 AM, Jan Pazdziora wrote:

On Fri, Oct 09, 2015 at 10:31:32AM +0200, Tomas Babej wrote:


a heavy process. Also, I wouldn't be strict about it, as we already have
a couple of workarounds, and not every time a workaround has a exact
mapping to a particular ticket.


Frankly, this worries me much more than not having ticket for some
trivial change to the code base.

If there's workaround in tests, it's some action that we do but
users/admins don't in their real setups. And that can cause failures
for our users that we don't see or even no longer know about because
in our tests, we've cleverly worked around them.


I guess, the global question of whether to do workarounds in tests to 
make them pass or not is older than the very oldest profession on earth.
I must admit, I am on Jan's side here. I would prefer to implement the 
approach proposed by Milan: mark the test scenario as expected failure 
(if it is crucial to make the whole run pass), or better even to leave 
it as it is: just so that each red CI run would remind of the necessity 
to fix the original issue.


This all is a theory, however. What do we do with this particular case? 
It's an edge case (does anyone except root zone admins sign a root 
zone?). Should we probably disable the whole scenario? Or just leave it 
failing as it is?




Either that workaround step is needed and needs to be documented, or
that step should not be needed and there should be a ticket describing
the issue.



--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Oleg Fayans]

Hi Martin

On 10/07/2015 04:30 PM, Martin Basti wrote:



On 10/07/2015 04:13 PM, Oleg Fayans wrote:

subj




Workaround looks good, but I prefer not to push it in upstream tests,
because it is not test failure.
I agree, we should rather fix the original issue. But as a temporary 
solution, to satisfy downstream, it could do.


Why is there this sleep, this might be useful in upstream tests too, but
what is the reason to add sleep there?


Without it I kept getting this error:
E   CalledProcessError: Command '['drill', '@localhost', '-k', 
'/etc/trusted-key.key', '-S', 'example.test.', 'SOA']' returned non-zero 
exit status 29


with --pdb option, though, my attempts to re-run the command succeeded, 
so I assumed it was a timing issue, and indeed, this 1 second sleep helped.




  # verify signatures
+time.sleep(1)
  args = [




Attached is an updated version of the patch with Martin's remarks taken 
into account


--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 18c7fe38fcc2e064a77c257837775cfb6f5efe53 Mon Sep 17 00:00:00 2001
From: Oleg Fayans 
Date: Thu, 8 Oct 2015 09:10:52 +0200
Subject: [PATCH] Fixed failure in requesting signed root zone info

After creating signed root zone, the server requires named.service restart for dig
requests to this zone to start displaying the key.

https://fedorahosted.org/freeipa/ticket/5348
---
 ipatests/test_integration/test_dnssec.py | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/ipatests/test_integration/test_dnssec.py b/ipatests/test_integration/test_dnssec.py
index 098b227f6543fa221ed6c75d1e98e9f056761977..afcbcf130a614aa580feca4ae4a61c4d1e667243 100644
--- a/ipatests/test_integration/test_dnssec.py
+++ b/ipatests/test_integration/test_dnssec.py
@@ -280,7 +280,10 @@ class TestInstallDNSSECFirst(IntegrationTest):
 "--ns-rec=" + self.master.hostname
 ]
 self.master.run_command(args)
-
+# A workaround for https://fedorahosted.org/freeipa/ticket/5348
+time.sleep(20)
+self.master.run_command(["systemctl", "restart", "named-pkcs11.service"])
+# End of workaround
 # test master
 assert wait_until_record_is_signed(
 self.master.ip, root_zone, self.log, timeout=100
@@ -303,8 +306,10 @@ class TestInstallDNSSECFirst(IntegrationTest):
 ]
 
 self.master.run_command(args)
-
-# wait until zone is signed
+# A workaround for https://fedorahosted.org/freeipa/ticket/5348
+time.sleep(20)
+self.master.run_command(["systemctl", "restart", "named-pkcs11.service"])
+# End of workaround
 assert wait_until_record_is_signed(
 self.master.ip, example_test_zone, self.log, timeout=100
 ), "Zone %s is not signed (master)" % example_test_zone
@@ -382,6 +387,7 @@ class TestInstallDNSSECFirst(IntegrationTest):
root_keys_rrset.to_text() + '\n')
 
 # verify signatures
+time.sleep(1)
 args = [
 "drill", "@localhost", "-k",
 paths.DNSSEC_TRUSTED_KEY, "-S",
-- 
2.4.3

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Martin Basti]

On 10/08/2015 09:13 AM, Oleg Fayans wrote:

Hi Martin

On 10/07/2015 04:30 PM, Martin Basti wrote:



On 10/07/2015 04:13 PM, Oleg Fayans wrote:

subj




Workaround looks good, but I prefer not to push it in upstream tests,
because it is not test failure.
I agree, we should rather fix the original issue. But as a temporary 
solution, to satisfy downstream, it could do.


Why is there this sleep, this might be useful in upstream tests too, but
what is the reason to add sleep there?


Without it I kept getting this error:
E   CalledProcessError: Command '['drill', '@localhost', '-k', 
'/etc/trusted-key.key', '-S', 'example.test.', 'SOA']' returned 
non-zero exit status 29


with --pdb option, though, my attempts to re-run the command 
succeeded, so I assumed it was a timing issue, and indeed, this 1 
second sleep helped.




  # verify signatures
+time.sleep(1)
  args = [




Attached is an updated version of the patch with Martin's remarks 
taken into account



Can you please send this as separate patch? I would like to push this one.

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Jan Pazdziora]

On Thu, Oct 08, 2015 at 11:12:37AM +0200, Oleg Fayans wrote:
> >
> >When the ticket is addressed and these workarounds are no longer
> >needed -- what is our process for finding these workarounds and
> >reverting them, so that the tests test the real, expected behaviour?
> 
> As per discussion with Martin Basti, it was decided that this workaround
> will only be applied to the current 4-2 branch, not to the upstream. In

That sounds like a reasonable plan for this issue.

> upstream the issue itself will (supposedly) be solved

Except currently it's not, so (IIUIC) you will keep having
nondeterministic failures in master.

I was mostly interested in the general approach that we have to
workarounds -- how do we track them, how do we make sure they don't
stick in tests forever, even after the issue was already properly
addressed.

-- 
Jan Pazdziora
Senior Principal Software Engineer, Identity Management Engineering, Red Hat

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Oleg Fayans]

Done
On 10/08/2015 10:37 AM, Martin Basti wrote:



On 10/08/2015 09:13 AM, Oleg Fayans wrote:

Hi Martin

On 10/07/2015 04:30 PM, Martin Basti wrote:



On 10/07/2015 04:13 PM, Oleg Fayans wrote:

subj




Workaround looks good, but I prefer not to push it in upstream tests,
because it is not test failure.

I agree, we should rather fix the original issue. But as a temporary
solution, to satisfy downstream, it could do.


Why is there this sleep, this might be useful in upstream tests too, but
what is the reason to add sleep there?


Without it I kept getting this error:
E   CalledProcessError: Command '['drill', '@localhost', '-k',
'/etc/trusted-key.key', '-S', 'example.test.', 'SOA']' returned
non-zero exit status 29

with --pdb option, though, my attempts to re-run the command
succeeded, so I assumed it was a timing issue, and indeed, this 1
second sleep helped.



  # verify signatures
+time.sleep(1)
  args = [




Attached is an updated version of the patch with Martin's remarks
taken into account


Can you please send this as separate patch? I would like to push this one.


--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From ad6341499d25833986f097eeac1ae89b0ea2450b Mon Sep 17 00:00:00 2001
From: Oleg Fayans 
Date: Thu, 8 Oct 2015 11:14:15 +0200
Subject: [PATCH] Fixed a timing issue with drill returning non-zero exitcode

---
 ipatests/test_integration/test_dnssec.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ipatests/test_integration/test_dnssec.py b/ipatests/test_integration/test_dnssec.py
index 098b227f6543fa221ed6c75d1e98e9f056761977..66e67a6efbe1db767f8b7102d2928be775e723af 100644
--- a/ipatests/test_integration/test_dnssec.py
+++ b/ipatests/test_integration/test_dnssec.py
@@ -382,6 +382,7 @@ class TestInstallDNSSECFirst(IntegrationTest):
root_keys_rrset.to_text() + '\n')
 
 # verify signatures
+time.sleep(1)
 args = [
 "drill", "@localhost", "-k",
 paths.DNSSEC_TRUSTED_KEY, "-S",
-- 
2.4.3

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Oleg Fayans]

Hi Jan,

On 10/08/2015 10:44 AM, Jan Pazdziora wrote:

On Wed, Oct 07, 2015 at 04:13:25PM +0200, Oleg Fayans wrote:

subj

--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.



 From 7ab1afe5e9a8f6b28be2d5b92423eccec61248a0 Mon Sep 17 00:00:00 2001
From: Oleg Fayans 
Date: Wed, 7 Oct 2015 16:08:30 +0200
Subject: [PATCH] Added a workaround for ticket N 5348

After creating signed root zone, the server requires named.service restart for 
dig
requests to this zone to start displaying the key.
---
  ipatests/test_integration/test_dnssec.py | 12 +---
  1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/ipatests/test_integration/test_dnssec.py 
b/ipatests/test_integration/test_dnssec.py
index 
098b227f6543fa221ed6c75d1e98e9f056761977..b63c6ce4795c53c5c2dd604783c321835d8a689b
 100644
--- a/ipatests/test_integration/test_dnssec.py
+++ b/ipatests/test_integration/test_dnssec.py
@@ -280,7 +280,10 @@ class TestInstallDNSSECFirst(IntegrationTest):
  "--ns-rec=" + self.master.hostname
  ]
  self.master.run_command(args)
-
+# A workaround for ticket N 5348
+time.sleep(20)
+self.master.run_command(["systemctl", "restart", 
"named-pkcs11.service"])


When the ticket is addressed and these workarounds are no longer
needed -- what is our process for finding these workarounds and
reverting them, so that the tests test the real, expected behaviour?


As per discussion with Martin Basti, it was decided that this workaround 
will only be applied to the current 4-2 branch, not to the upstream. In 
upstream the issue itself will (supposedly) be solved




Also, instead of blind sleeps, wouldn't it be better to have some
polling for status of the services we are waiting for?


Since we still do not know what exactly causes the issue, it is really 
hard to figure out what is it that we should be polling. Otherwise I am 
really anti-blind-sleeps myself.






--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Jan Pazdziora]

On Wed, Oct 07, 2015 at 04:13:25PM +0200, Oleg Fayans wrote:
> subj
> 
> -- 
> Oleg Fayans
> Quality Engineer
> FreeIPA team
> RedHat.

> From 7ab1afe5e9a8f6b28be2d5b92423eccec61248a0 Mon Sep 17 00:00:00 2001
> From: Oleg Fayans 
> Date: Wed, 7 Oct 2015 16:08:30 +0200
> Subject: [PATCH] Added a workaround for ticket N 5348
> 
> After creating signed root zone, the server requires named.service restart 
> for dig
> requests to this zone to start displaying the key.
> ---
>  ipatests/test_integration/test_dnssec.py | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/ipatests/test_integration/test_dnssec.py 
> b/ipatests/test_integration/test_dnssec.py
> index 
> 098b227f6543fa221ed6c75d1e98e9f056761977..b63c6ce4795c53c5c2dd604783c321835d8a689b
>  100644
> --- a/ipatests/test_integration/test_dnssec.py
> +++ b/ipatests/test_integration/test_dnssec.py
> @@ -280,7 +280,10 @@ class TestInstallDNSSECFirst(IntegrationTest):
>  "--ns-rec=" + self.master.hostname
>  ]
>  self.master.run_command(args)
> -
> +# A workaround for ticket N 5348
> +time.sleep(20)
> +self.master.run_command(["systemctl", "restart", 
> "named-pkcs11.service"])

When the ticket is addressed and these workarounds are no longer
needed -- what is our process for finding these workarounds and
reverting them, so that the tests test the real, expected behaviour?

Also, instead of blind sleeps, wouldn't it be better to have some
polling for status of the services we are waiting for?

-- 
Jan Pazdziora
Senior Principal Software Engineer, Identity Management Engineering, Red Hat

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Oleg Fayans]

Hi,

On 10/08/2015 11:18 AM, Jan Pazdziora wrote:

On Thu, Oct 08, 2015 at 11:12:37AM +0200, Oleg Fayans wrote:


When the ticket is addressed and these workarounds are no longer
needed -- what is our process for finding these workarounds and
reverting them, so that the tests test the real, expected behaviour?


As per discussion with Martin Basti, it was decided that this workaround
will only be applied to the current 4-2 branch, not to the upstream. In


That sounds like a reasonable plan for this issue.


upstream the issue itself will (supposedly) be solved


Except currently it's not, so (IIUIC) you will keep having
nondeterministic failures in master.

I was mostly interested in the general approach that we have to
workarounds -- how do we track them, how do we make sure they don't
stick in tests forever, even after the issue was already properly
addressed.

That's actually a great point. I personally would like tickets to have 
one more field: "workaround" containing the address of a workaround in 
the format "path_to_the_file:line_number" or better even - a commit id 
of this workaround, so that once the ticket is resolved, we could easily 
find what to reverse.


--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Martin Basti]

On 10/08/2015 11:18 AM, Jan Pazdziora wrote:

On Thu, Oct 08, 2015 at 11:12:37AM +0200, Oleg Fayans wrote:

When the ticket is addressed and these workarounds are no longer
needed -- what is our process for finding these workarounds and
reverting them, so that the tests test the real, expected behaviour?

As per discussion with Martin Basti, it was decided that this workaround
will only be applied to the current 4-2 branch, not to the upstream. In

That sounds like a reasonable plan for this issue.


upstream the issue itself will (supposedly) be solved

Except currently it's not, so (IIUIC) you will keep having
nondeterministic failures in master.

I was mostly interested in the general approach that we have to
workarounds -- how do we track them, how do we make sure they don't
stick in tests forever, even after the issue was already properly
addressed.


I'm not sure if there is a formal process how to work with workarounds.

Usually, we open ticket, push workaround there, and leave ticket opened 
until the issue is fixed.
If there is no time to do proper fix and workaround works well we close 
ticket and open new ticket in further milestones.


I do not remember if we have a similar issue with test workaround in past.

Martin

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Martin Basti]

On 10/08/2015 02:41 PM, Martin Kosek wrote:

On 10/08/2015 02:06 PM, Martin Basti wrote:


On 10/08/2015 11:18 AM, Jan Pazdziora wrote:

On Thu, Oct 08, 2015 at 11:12:37AM +0200, Oleg Fayans wrote:

When the ticket is addressed and these workarounds are no longer
needed -- what is our process for finding these workarounds and
reverting them, so that the tests test the real, expected behaviour?

As per discussion with Martin Basti, it was decided that this workaround
will only be applied to the current 4-2 branch, not to the upstream. In

That sounds like a reasonable plan for this issue.


upstream the issue itself will (supposedly) be solved

Except currently it's not, so (IIUIC) you will keep having
nondeterministic failures in master.

I was mostly interested in the general approach that we have to
workarounds -- how do we track them, how do we make sure they don't
stick in tests forever, even after the issue was already properly
addressed.


I'm not sure if there is a formal process how to work with workarounds.

Usually, we open ticket, push workaround there, and leave ticket opened until
the issue is fixed.
If there is no time to do proper fix and workaround works well we close ticket
and open new ticket in further milestones.

I do not remember if we have a similar issue with test workaround in past.

Can we anyhow utilize "wait_for_dns" knob we added for making DNS tests 
reliable?

No,

I already do that when records in CI test are created, there is polling.

The first part of oleg's workaround has nothing common with timing 
issue, only restart of named process will help


The second part, I do not know why there is 1sec delay needed, because 
presence of signed records was detected in step before, so DNSKEY record 
must be available, and probably this is caused by named internals, that 
DNSKEY record is available later than signed records of zone.
I don think so that extending testing for all types of DNSSEC records is 
worth it and 1sec is enough for bind to be ready.


Martin

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Martin Kosek]

On 10/08/2015 02:08 PM, Oleg Fayans wrote:
> Hi,
> 
> On 10/08/2015 11:18 AM, Jan Pazdziora wrote:
>> On Thu, Oct 08, 2015 at 11:12:37AM +0200, Oleg Fayans wrote:

 When the ticket is addressed and these workarounds are no longer
 needed -- what is our process for finding these workarounds and
 reverting them, so that the tests test the real, expected behaviour?
>>>
>>> As per discussion with Martin Basti, it was decided that this workaround
>>> will only be applied to the current 4-2 branch, not to the upstream. In
>>
>> That sounds like a reasonable plan for this issue.
>>
>>> upstream the issue itself will (supposedly) be solved
>>
>> Except currently it's not, so (IIUIC) you will keep having
>> nondeterministic failures in master.
>>
>> I was mostly interested in the general approach that we have to
>> workarounds -- how do we track them, how do we make sure they don't
>> stick in tests forever, even after the issue was already properly
>> addressed.
>>
> That's actually a great point. I personally would like tickets to have one 
> more
> field: "workaround" containing the address of a workaround in the format
> "path_to_the_file:line_number" or better even - a commit id of this 
> workaround,
> so that once the ticket is resolved, we could easily find what to reverse.
> 

Please don't add any more trac fields, there is too many of them already :-)
Keyword may serve better for now...

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Martin Basti]

On 10/08/2015 02:39 PM, Martin Kosek wrote:

On 10/08/2015 02:08 PM, Oleg Fayans wrote:

Hi,

On 10/08/2015 11:18 AM, Jan Pazdziora wrote:

On Thu, Oct 08, 2015 at 11:12:37AM +0200, Oleg Fayans wrote:

When the ticket is addressed and these workarounds are no longer
needed -- what is our process for finding these workarounds and
reverting them, so that the tests test the real, expected behaviour?

As per discussion with Martin Basti, it was decided that this workaround
will only be applied to the current 4-2 branch, not to the upstream. In

That sounds like a reasonable plan for this issue.


upstream the issue itself will (supposedly) be solved

Except currently it's not, so (IIUIC) you will keep having
nondeterministic failures in master.

I was mostly interested in the general approach that we have to
workarounds -- how do we track them, how do we make sure they don't
stick in tests forever, even after the issue was already properly
addressed.


That's actually a great point. I personally would like tickets to have one more
field: "workaround" containing the address of a workaround in the format
"path_to_the_file:line_number" or better even - a commit id of this workaround,
so that once the ticket is resolved, we could easily find what to reverse.


Please don't add any more trac fields, there is too many of them already :-)
Keyword may serve better for now...


+1

new trac field for a few workarounds per year is not worth it.

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Martin Kosek]

On 10/08/2015 02:06 PM, Martin Basti wrote:
> 
> 
> On 10/08/2015 11:18 AM, Jan Pazdziora wrote:
>> On Thu, Oct 08, 2015 at 11:12:37AM +0200, Oleg Fayans wrote:
 When the ticket is addressed and these workarounds are no longer
 needed -- what is our process for finding these workarounds and
 reverting them, so that the tests test the real, expected behaviour?
>>> As per discussion with Martin Basti, it was decided that this workaround
>>> will only be applied to the current 4-2 branch, not to the upstream. In
>> That sounds like a reasonable plan for this issue.
>>
>>> upstream the issue itself will (supposedly) be solved
>> Except currently it's not, so (IIUIC) you will keep having
>> nondeterministic failures in master.
>>
>> I was mostly interested in the general approach that we have to
>> workarounds -- how do we track them, how do we make sure they don't
>> stick in tests forever, even after the issue was already properly
>> addressed.
>>
> I'm not sure if there is a formal process how to work with workarounds.
> 
> Usually, we open ticket, push workaround there, and leave ticket opened until
> the issue is fixed.
> If there is no time to do proper fix and workaround works well we close ticket
> and open new ticket in further milestones.
> 
> I do not remember if we have a similar issue with test workaround in past.

Can we anyhow utilize "wait_for_dns" knob we added for making DNS tests 
reliable?

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

[Freeipa-devel] [PATCH] Workaround for trac N 5348

[Oleg Fayans]

subj

--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 7ab1afe5e9a8f6b28be2d5b92423eccec61248a0 Mon Sep 17 00:00:00 2001
From: Oleg Fayans 
Date: Wed, 7 Oct 2015 16:08:30 +0200
Subject: [PATCH] Added a workaround for ticket N 5348

After creating signed root zone, the server requires named.service restart for dig
requests to this zone to start displaying the key.
---
 ipatests/test_integration/test_dnssec.py | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/ipatests/test_integration/test_dnssec.py b/ipatests/test_integration/test_dnssec.py
index 098b227f6543fa221ed6c75d1e98e9f056761977..b63c6ce4795c53c5c2dd604783c321835d8a689b 100644
--- a/ipatests/test_integration/test_dnssec.py
+++ b/ipatests/test_integration/test_dnssec.py
@@ -280,7 +280,10 @@ class TestInstallDNSSECFirst(IntegrationTest):
 "--ns-rec=" + self.master.hostname
 ]
 self.master.run_command(args)
-
+# A workaround for ticket N 5348
+time.sleep(20)
+self.master.run_command(["systemctl", "restart", "named-pkcs11.service"])
+# End of workaround
 # test master
 assert wait_until_record_is_signed(
 self.master.ip, root_zone, self.log, timeout=100
@@ -303,8 +306,10 @@ class TestInstallDNSSECFirst(IntegrationTest):
 ]
 
 self.master.run_command(args)
-
-# wait until zone is signed
+# A workaround for ticket N 5348
+time.sleep(20)
+self.master.run_command(["systemctl", "restart", "named-pkcs11.service"])
+# End of workaround
 assert wait_until_record_is_signed(
 self.master.ip, example_test_zone, self.log, timeout=100
 ), "Zone %s is not signed (master)" % example_test_zone
@@ -382,6 +387,7 @@ class TestInstallDNSSECFirst(IntegrationTest):
root_keys_rrset.to_text() + '\n')
 
 # verify signatures
+time.sleep(1)
 args = [
 "drill", "@localhost", "-k",
 paths.DNSSEC_TRUSTED_KEY, "-S",
-- 
2.4.3

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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Martin Basti]

On 10/07/2015 04:13 PM, Oleg Fayans wrote:

subj



Workaround looks good, but I prefer not to push it in upstream tests, 
because it is not test failure.


Why is there this sleep, this might be useful in upstream tests too, but 
what is the reason to add sleep there?


 # verify signatures
+time.sleep(1)
 args = [


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

Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348

[Martin Kosek]

On 10/07/2015 04:13 PM, Oleg Fayans wrote:
> subj

I would suggest using standard FreeIPA format of refering to tickets, i.e. URL.
I would also suggest including ticket URL in patch description so that people
can easily find it:

http://www.freeipa.org/page/Contribute/Patch_Format

Martin

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