Skip to content

Integration test: cgroup v1 network tests, fix to memory tests#516

Merged
utam0k merged 3 commits into
youki-dev:mainfrom
tsturzl:cgroups_network_integration_test
Dec 7, 2021
Merged

Integration test: cgroup v1 network tests, fix to memory tests#516
utam0k merged 3 commits into
youki-dev:mainfrom
tsturzl:cgroups_network_integration_test

Conversation

@tsturzl
Copy link
Copy Markdown
Collaborator

@tsturzl tsturzl commented Dec 5, 2021

I have implemented a similar test to the oci-spec runtime tools, and I've verified that it is passing consistently. I also noticed that the cgroups path for memory was not set correctly and included the fix in this PR. I also verified the change the memory cgroups test is passing consistently. I also made sure both are able to fail, as I suspect the memory cgroup test wasn't even running before because the wrong path was set for the can_run check.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 5, 2021

Codecov Report

Merging #516 (923c0c4) into main (82b9051) will decrease coverage by 0.24%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #516      +/-   ##
==========================================
- Coverage   61.08%   60.84%   -0.25%     
==========================================
  Files          85       87       +2     
  Lines       12416    12677     +261     
==========================================
+ Hits         7584     7713     +129     
- Misses       4832     4964     +132     

@tsturzl
Copy link
Copy Markdown
Collaborator Author

tsturzl commented Dec 5, 2021

It's very ironic that the code coverage drops when adding tests. I wonder if we can exclude the integration tests crate from the code coverage reports.

@tsturzl
Copy link
Copy Markdown
Collaborator Author

tsturzl commented Dec 5, 2021

I made another PR to ignore coverage reports for the integration tests: #517

const CGROUP_MEMORY_LIMIT: &str = "memory.limit_in_bytes";
const CGROUP_MEMORY_SWAPPINESS: &str = "memory.swappiness";
const CGROUP_MEMORY_LIMIT: &str = "/sys/fs/cgroup/memory/memory.limit_in_bytes";
const CGROUP_MEMORY_SWAPPINESS: &str = "/sys/fs/cgroup/memory/memory.swappiness";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

Comment on lines +67 to +74
let interfaces = datalink::interfaces();

// Gets the loopback interface, or defaults to "lo"
let lo_if_name = interfaces.get(0).map_or_else(|| "lo", |iface| &iface.name);
// Gets the first ethernet interface, or defaults to "eth0"
let eth_if_name = interfaces
.get(1)
.map_or_else(|| "eth0", |iface| &iface.name);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is better to check about interfaces in can_run()?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is what the runtime tools tests are currently doing, they check the interfaces and default them to lo and eth0. As far as I can tell the tests may still pass if the interfaces aren't found.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

See here: https://github.com/opencontainers/runtime-tools/blob/1684d131456a6bc99b8e96aa4a99783f21e58d79/validation/linux_cgroups_network/linux_cgroups_network.go#L21

I wish they explained the reasoning for setting a default more in the comment. It seems the rationale here, based on this commit message is to allow the test to run on systems with different interface names. For example as far as I can tell on every system the first interface is loopback (lo), and then on many systems the second interface may actually be something other than eth0. In fact on my system I do not have eth0, and the second interface is wlp0s20f3, probably because I don't have an ethernet interface on my laptop.

When I run the test with "eth0" explicitly set as the interface on my laptop the test does in fact fail. So I think you're right, we should probably add this to can_run.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Take a look at the latest revision.

Copy link
Copy Markdown
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

Looks good

@utam0k utam0k merged commit aff2d19 into youki-dev:main Dec 7, 2021
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.

3 participants