Try to fix EventService test failures #154
Conversation
yitsushi
added
kind/bug
|
It does not fix it XD it does a painful crash after 2 minutes. Obviously I can't see this on local machine. |
Codecov Report
@@ Coverage Diff @@
## main #154 +/- ##
==========================================
- Coverage 40.09% 39.64% -0.46%
==========================================
Files 32 36 +4
Lines 1444 1695 +251
==========================================
+ Hits 579 672 +93
- Misses 789 926 +137
- Partials 76 97 +21
Continue to review full report at Codecov.
|
|
squash and better description: coming soon |
yitsushi
force-pushed
the
115-event-service-fix
branch
from
6f4201b to
b70bad3
Compare
|
Nope still not... why |
yitsushi
force-pushed
the
115-event-service-fix
branch
from
b70bad3 to
2d86efb
Compare
|
Now it seems it's happy. |
I was able to reproduce the issue with a helping had from cgroup. Limiting to one CPU with a heavily quota (0.01% of my CPU) revealed the issue. Note before I describe what happened: Containerd does not send messages to subscribers retrospectively, all subscribers will receive events from the point they subscribed. The original test created published N events and after that created subscribers. The connection between the test and containerd is much slower than the execution of the test, so when containrd wants to send out the events to all subscribers, they are already there to receive events. That's why it works on my machine and that's why it can pass on Github Actions sometimes. However, on a slow machine, with only one vcpu, the test and containerd are racing for their own cpu share. In this scenario, the events are already published before the subscribers are ready to receive them. Solution: Create subscribers first and then publish events. Disclaimer: There is a chance, all I wrote above is not entire correct, but that's how I understand it. It does not really matter if we only check the logic in the test. Originally it was publish->subscribe, but it's not correct, we need subscribers before we publish events. Fixes liquidmetal-dev#115
yitsushi
force-pushed
the
115-event-service-fix
branch
from
2d86efb to
b6f1379
Compare
|
So the |
|
|
||
| . "github.com/onsi/gomega" | ||
| "github.com/stretchr/testify/assert" |
There was a problem hiding this comment.
We don't need 2 assertion packages. If the consensus is testtify/assert then i'm good with that. I like gomega but not so much that i wouldn't be happy using something else.
There was a problem hiding this comment.
I used it because it was already in there (no addition in go.mod)
There was a problem hiding this comment.
My only issue with the way gomega is used, I don't like the . imports because they can mess with the environment, they are packages and should be used as packages and not a pack of function injected into our test module.
There was a problem hiding this comment.
Updated the test to use only Gomega.
I was able to reproduce the issue with a helping had from cgroup.
Limiting to one CPU with a heavily quota (0.01% of my CPU) revealed the
issue.
Note before I describe what happened:
Containerd does not send messages to subscribers retrospectively, all
subscribers will receive events from the point they subscribed.
The original test created published N events and after that created
subscribers. The connection between the test and containerd is much
slower than the execution of the test, so when containrd wants to send
out the events to all subscribers, they are already there to receive
events. That's why it works on my machine and that's why it can pass on
Github Actions sometimes.
However, on a slow machine, with only one vcpu, the test and containerd
are racing for their own cpu share. In this scenario, the events are
already published before the subscribers are ready to receive them.
Solution:
Create subscribers first and then publish events.
Disclaimer:
There is a chance, all I wrote above is not entire correct, but that's
how I understand it. It does not really matter if we only check the
logic in the test. Originally it was publish->subscribe, but it's not
correct, we need subscribers before we publish events.
Fixes #115
Checklist: