Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix NsJail Tests #93

Merged
merged 7 commits into from Mar 8, 2021
Merged

Fix NsJail Tests #93

merged 7 commits into from Mar 8, 2021

Conversation

MarkKoz
Copy link
Member

@MarkKoz MarkKoz commented Feb 5, 2021

#88 left tests in a broken state. This wasn't noticed since CI had a bug that prevented the step from failing despite tests failing. This PR fixes both of these issues.

@MarkKoz
CI: fix test step not failing when tests fail
e12e403 
Generating the report in the same step resulted in the report exit code
overriding the exit code of the test runner.
@MarkKoz MarkKoz added type: bug Something isn't working area: CI Related to continuous intergration and deployment area: tests Related to tests (e.g. unit tests) labels Feb 5, 2021
Akarys42
Akarys42 approved these changes Feb 6, 2021
View reviewed changes
Copy link
Contributor

@Akarys42 Akarys42 left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (except for the failing test)

@MarkKoz
Copy link
Member Author

MarkKoz commented Feb 6, 2021

Yeah this wasn't ready for review. I became too busy to continue and fix those issues.

Akarys42
Akarys42 reviewed Feb 25, 2021
View reviewed changes
.github/workflows/lint-test-build-push.yaml Outdated
Comment on lines 146 to 147
sudo pip install coveralls~=2.1
sudo coveralls
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason why we don't make a user install rather than a system install?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC it's cause the user site is not on PATH. It's more convenient to use sudo in this case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though maybe it's bad practice, even in CI, and I should have kept using the Python version task...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't yell at me, but I forced push to get rid of it. Turns out I had already dropped the commit locally last month, forgot about it, and commited changes over it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@MarkKoz
Docker: improve caching & install numpy in container
3f474ff 
CI was building the image twice: once with dev dependencies and again
without. Separating the pipenv command into separate layers allows the
second build in CI to take advantage of the cache for the base
dependencies that it will share across both builds.

Install numpy along with the dev dependencies within the container.
Previously it was installed in CI only, but this meant extra work for
those running tests locally.

Install numpy to the correct site.
@coveralls
Copy link

coveralls commented Mar 8, 2021 •
edited

Coverage Status

Coverage increased (+34.7%) to 91.566% when pulling 46fc728 on bug/tests/nsjail into 59a1bf4 on master.

@MarkKoz
Merge master into bug/tests/nsjail
46fc728 
The branch needs the fixes from #94 to make the tests pass.
@MarkKoz
Copy link
Member Author

MarkKoz commented Mar 8, 2021 •
edited

@jb3 One of the tests is failing. This is a similar issue to what #94 was trying to fix, but the test still fails despite merging that into this branch.

======================================================================
ERROR: test_stdout_flood_results_in_graceful_sigterm (tests.test_nsjail.NsJailTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/snekbox/snekbox/tests/test_nsjail.py", line 188, in test_stdout_flood_results_in_graceful_sigterm
    result = self.nsjail.python3(stdout_flood)
  File "/home/runner/work/snekbox/snekbox/snekbox/nsjail.py", line 236, in python3
    child.rmdir()
  File "/usr/local/lib/python3.9/pathlib.py", line 1352, in rmdir
    self._accessor.rmdir(self)
OSError: [Errno 16] Device or resource busy: '/sys/fs/cgroup/memory/snekbox-976324a6-c789-49eb-b88e-4702e4ac0626/NSJAIL.901'

Do you have any ideas? Oddly enough, the tests all pass locally.

@jb3
Copy link
Member

jb3 commented Mar 8, 2021

So I guess the child cgroups are still busy. I'll check it out.

@MarkKoz
Copy link
Member Author

MarkKoz commented Mar 8, 2021

Turns out that killing the parent process does not kill its children. Therefore, we have nsjail's children (which are Python processes in this case) sticking around preventing child cgroups from being removed. This will be addressed in a separate PR.

@MarkKoz MarkKoz merged commit bc05ed1 into master Mar 8, 2021
@MarkKoz MarkKoz deleted the bug/tests/nsjail branch March 8, 2021 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Akarys42 Akarys42 Akarys42 approved these changes

@Den4200 Den4200 Awaiting requested review from Den4200

@jb3 jb3 Awaiting requested review from jb3

Assignees
No one assigned
Labels
area: CI Related to continuous intergration and deployment area: tests Related to tests (e.g. unit tests) type: bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MarkKoz @coveralls @jb3 @Akarys42