Opened 7 years ago

Closed 7 years ago

#2888 closed defect (fixed)

msgq unittest failure on OpenBSD

Reported by: jinmei Owned by: vorner
Priority: very high Milestone: Sprint-20130423
Component: ~msgq (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 1.71 Internal?: no

Description

I confirmed this can be reproducible.
http://git.bind10.isc.org/~tester/builder//BIND10/20130326104118-OpenBSD5-amd64/logs/unittests.out

Sometimes it succeeded, so it may be a timing issue. BTW: I think we
should try to avoid using different processes for unit tests. I'm not
sure that's a background reason for it, but in general that's a source
of troubles.

Subtickets

Change History (10)

comment:1 follow-up: Changed 7 years ago by vorner

As is commented in the test source, the goal is on the border between unit
tests and system tests. It does test specific behaviour, but of the whole msgq.
There's a lot of recv/send handling that is/(or at least was some time ago)
broken and it is needed to check it.

It seemed easier to write it as unit test than lettuce test. In lettuce, it
would be easier to start the msgq, but then, keeping a connection open and
sending and receiving data across it would be hell.

As for the error, maybe it would be better to have the while to check it can
connect instead of if the path exists. There may be a short time between the
socket is created and msgq starts listening on it.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 7 years ago by jinmei

Replying to vorner:

As is commented in the test source, the goal is on the border between unit
tests and system tests. It does test specific behaviour, but of the whole msgq.
There's a lot of recv/send handling that is/(or at least was some time ago)
broken and it is needed to check it.

It seemed easier to write it as unit test than lettuce test. In lettuce, it
would be easier to start the msgq, but then, keeping a connection open and
sending and receiving data across it would be hell.

I'm not convinced that abusing unit tests this way is the best
approach, but as the existing tests already passed the review-merge
process I won't fight against that at this moment.

The most important and urgent issue is to fix the occasional
failures. If we can do this by tweaking the test case I'm okay with
it for the purpose of this ticket. I reserve the opinion of
revisiting the approach itself, though.

BTW, this doesn't seem to be OpenBSD specific.

comment:3 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 3

comment:4 Changed 7 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130423

comment:5 Changed 7 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:6 in reply to: ↑ 2 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

Hello

Replying to jinmei:

I'm not convinced that abusing unit tests this way is the best
approach, but as the existing tests already passed the review-merge
process I won't fight against that at this moment.

Well, propose a better approach. As I say, we could implement it using lettuce.
But then, we would need the terrain to connect to msgq itself (not talk through
bindctl) and send messages and receive messages. And then the test would be
mostly the same, just with the additional layer of the terrain. The launching
could have the same bug as this one as well. And, once we start implementing
the stress tests, we need loops, which are not really available in lettuce, and
we will need some various broken scenarios for the error testing, which is
mostly a new lettuce statement for each error case. So I don't think we would
gain anything by using lettuce here.

Do we have any other approach?

And, yes, I believe this level of tests really does need to start full-featured msgq.

Anyway, I pushed the fix. I believe it should work, because it actually tries
to connect. I did run the test 1000 times and it didn't fail here (even without
the wait there, which could increase the chance of race condition). I'll also
ask for the branch to be put into the build bot. But if anyone had a reliable
way to reproduce, I won't say no for him trying it.

comment:7 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:8 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by jinmei

Replying to vorner:

I'm not convinced that abusing unit tests this way is the best
approach, but as the existing tests already passed the review-merge
process I won't fight against that at this moment.

Well, propose a better approach. [...]

Do we have any other approach?

I'm afraid we are talking about different things. My main concern was
that it's not good to use system level tests, whichever tools are
used, lettuce or python unittest framework or whatever, for things
that are supposed to be tested in unit tests, e.g., whether each
method works as expected, including all corner cases. A known bad
example against this principle is statistics tests, and I believe we
all know it's bad.

You seemed to talk about which tool we should use for system tests,
specifically whether to use lettuce or use the python unittest
framework. I don't have a strong opinion on that point.

This gap also made me realize I might misunderstand the purpose of
#1927 and msgq_run_test.py: I thought the subject of #1927 was to
write more detailed unit tests and the run_test.py was an attempt of
it like the statistics tests do.

In terms of how to *run* system tests, I have one suggestion: I'd like
to separate the current system test using from unit tests, preferably
if we type in 'make check' or something equivalently simply command
only unit tests will be run. Unit tests are supposed to be run very
frequently and should generally be completed very quickly. System
tests can often take more time, which makes us tend to run it less
frequently (although, current pure unit tests of msgq also takes some
time, probably because it also involves separate processes.)

Anyway, I pushed the fix. I believe it should work, because it actually tries
to connect. I did run the test 1000 times and it didn't fail here (even without
the wait there, which could increase the chance of race condition). I'll also
ask for the branch to be put into the build bot. But if anyone had a reliable
way to reproduce, I won't say no for him trying it.

The branch itself looks okay. As this is an urgent care issue I think
it should be merged now. I'd leave it to you whether to continue the
discussion above and possibly do some development work within this
ticket.

comment:9 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:10 in reply to: ↑ 8 Changed 7 years ago by vorner

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 1.71

Hello

Replying to jinmei:

This gap also made me realize I might misunderstand the purpose of
#1927 and msgq_run_test.py: I thought the subject of #1927 was to
write more detailed unit tests and the run_test.py was an attempt of
it like the statistics tests do.

Well, the ticket itself didn't really specify if it should be system or unit tests. However, when doing it, I considered:

  • The related ticket #1929 is probably implementable only as system tests, #1928 probably too.
  • When mocking the low-level operations, I would have mock out like 50% of msgq's code.
  • The „algorithmic“ parts (like subscription management) is already covered by unit tests.

So I decided it needs something like a system test, but not for the whole bind10 system, just for msgq. Therefore I didn't call it a system test, because it doesn't test the whole system. But unit test isn't enough for testing msgq works either. I don't think the unittest/systemtest distinction works well.

I'm OK with deciding to move these (module-level?) tests under system tests, so they are not run with make check. It just seems it is not easy to attach another test besides the lettuce ones to be just run.

For the record, I don't think the comparison with stats tests is very fitting. Stats tests start other than the tested processes, this one starts the testee. Also, statistics don't really need to test the communication with other processes, just their merging of the statistics data, while the communication is the main purpose of msgq.

The branch itself looks okay. As this is an urgent care issue I think
it should be merged now. I'd leave it to you whether to continue the
discussion above and possibly do some development work within this
ticket.

Merged.

I still don't see what concrete action should be taken to address your concerns, so I'm closing the ticket. If you have a concrete idea what to do with it, please create a new one.

Note: See TracTickets for help on using tickets.