This repository has been archived by the owner on May 12, 2021. It is now read-only.
virtcontainers: Fix unit tests #209
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
April 13, 2018 15:22
When using noopShim type from the unit tests, we were ending up getting a PID 1000, and when checking if the shim was around, we were always expecting the shim to be "not running", based on the fact that the process was not there anymore. Unfortunately, this was a very wrong assumption because we cannot control which PIDs are running or not on the system. The way to simplify this is to return a PID 0 in case of noopShim, processed as a special case by the function waitForShim(). Fixes kata-containers#208 Signed-off-by: Sebastien Boeuf <[email protected]>
Because of the bad design of the cc_proxy_mock go routine, we were leaving an infinite loop running into this go routine behind. This was consuming a lot of resources and it was obviously slowing down the tests being run in parallel. That's one of the reason we were hitting the 10 seconds timeout when running go tests. Fixes kata-containers#208 Signed-off-by: Sebastien Boeuf <[email protected]>
Those different files were all calling into a go routine that was eventually reporting some result through a go channel. The problem was the way those routine were implemented, as they were hanging around forever. Indeed, nothing was actually listening to the channel in some cases, and those routines never ended. This was one of the problem detected by the fact that our unit tests needed more time to pass because when they were all run in parallel, the resources consumed by those routines were increasing the time for other tests to complete. Fixes kata-containers#208 Signed-off-by: Sebastien Boeuf <[email protected]>
|
@devimc @grahamwhaley @egernst @bergwolf PTAL and let's merge this ;) |
grahamwhaley
approved these changes
Apr 16, 2018
| proxy.startListening() | ||
| go func() { | ||
| for { | ||
| proxy.serve() | ||
|
|
||
| proxy.Lock() |
There was a problem hiding this comment.
Just curious - do you need the locking if you only have one reader and one writer?
There was a problem hiding this comment.
Yes you do, otherwise Golang complains. I agree it is semantically overkill here.
|
Couple of strange things on the failed 17.10 CI - I'll give it a nudge (or is this a known issue @chavafg ?). |
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
- Coverage 65.33% 65.33% -0.01%
==========================================
Files 73 73
Lines 7702 7707 +5
==========================================
+ Hits 5032 5035 +3
- Misses 2127 2128 +1
- Partials 543 544 +1
Continue to review full report at Codecov.
|
egernst
reviewed
Apr 16, 2018
egernst
approved these changes
Apr 16, 2018
zklei
pushed a commit
to zklei/runtime
that referenced
this pull request
Jun 13, 2019
The go netlink package has added more netlink socket protocols than we need. Specify one to avoid failure due to unsupported protocols. Fixes: kata-containers#209 Signed-off-by: Peng Tao <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Several failures have been reported through kata-containers/tests#225 regarding our CI. A panic was triggered after our unit tests were reaching the defined timeout of 10s, and we could have bumped this timeout to a bigger value, but the root cause was our bad management of the go routines from our code and from our tests, leaving a lot of go routines behind, and slowing down our tests.
Fixes #208