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.

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

tests/integration: nits #2592

Merged
merged 7 commits into from Sep 28, 2020
Merged

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Sep 16, 2020

Most notable one is spec.bats simplification and speedup.

The rest is nits and minor fixes. Please see individual commits for details.

Use update_config like in all the other places.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin marked this pull request as draft September 16, 2020 23:45
@kolyshkin kolyshkin force-pushed the tests-int-nits branch 2 times, most recently from 0c8b135 to b068da0 Compare September 19, 2020 02:30
@kolyshkin kolyshkin marked this pull request as ready for review September 19, 2020 02:40
@kolyshkin kolyshkin marked this pull request as draft September 19, 2020 02:57
@kolyshkin
Copy link
Contributor Author

Interesting... my changes resulting in spec generated by runc spec --rootless being validated (which was not performed before), and the validation fails with the following error:

The document is not valid. see errors :
 - linux.resources.pids: limit is required

Indeed, runtime-spec says pids.limit is required:
https://github.com/opencontainers/runtime-spec/blob/80db136d65fc1a90b1bbbcc964a4b354b7aad0b2/schema/config-linux.json#L53

@AkihiroSuda mind to take a look? Or should we NOT run this for the rootless case?

@AkihiroSuda
Copy link
Member

Shouldn't be required even for rootful, I guess

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Sep 21, 2020

OK this is caused by

update_config '.linux.resources += {"memory":{},"cpu":{},"blockio":{},"pids":{}}' $bundle

which is indeed an invalid config.json (if pids is specified, limit should be set). But since it is only generated during tests we're fine.

With some grep|awk help, found a few places where the containers
supposed to be removed in teardown() weren't.

To find container names:

	grep -E '^[[:space:]]*\<_*_*runc\>.*\<(create|run|restore)\>'

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit d78ae51 adds this line, but the file is never created.

This is probably a leftover from copy-pasting update.bats code.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Fix
 - whitespace at EOL
 - empty lines at EOF

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The purpose of the spec validation test is to make sure
that config.json generated by runc spec is valid. Therefore,
it does not depend on ROOTLESS_FEATURES.

Taking that into account, and given the fact that this test
involves cloning repos and building some code, it makes sense
to not run it 4 times for various rootless features.

Time it takes to execute spec.bats in rootless mode has improved.

Before:
	real	0m21.286s
	user	0m37.837s
	sys	0m5.745s

After:
	real	0m13.162s
	user	0m30.814s
	sys	0m4.050s

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Most of whatever is happening in the test cases is already available in
setup_hello and teardown_hello. Use these.

Rewrite the validation test to be more compact and not dependent
on `pwd` value. Also, pinning xeipuuv/gojsonschema to a particular
version is no longer required.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin marked this pull request as ready for review September 21, 2020 16:32
@kolyshkin
Copy link
Contributor Author

@mrunalp @AkihiroSuda PTAL

@mrunalp mrunalp merged commit 43d2b10 into opencontainers:master Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants