Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348
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
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
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
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
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
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
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
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
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
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
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
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
Re: [Freeipa-devel] [PATCH] Workaround for trac N 5348
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
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
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
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
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
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
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
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
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
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
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
