Add test for cgroups_relative_memory #2686

omprakaash · 2024-02-15T18:40:35Z

Adds an integration test for cgroups_relative_memory. The only difference from the non-relative test seems to be the cgroup_path of the specified spec while testing.

codecov-commenter · 2024-02-15T20:21:53Z

Codecov Report

Merging #2686 (44e7b59) into main (04f8f2d) will decrease coverage by 0.37%.
Report is 64 commits behind head on main.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2686      +/-   ##
==========================================
- Coverage   65.50%   65.13%   -0.37%     
==========================================
  Files         133      133              
  Lines       16916    17012      +96     
==========================================
  Hits        11081    11081              
- Misses       5835     5931      +96

Signed-off-by: omprakaash <omsuseela@gmail.com>

lengrongfu · 2024-02-18T02:36:16Z

LGTM

YJDoc2 · 2024-02-20T05:52:22Z

Hey, as @omprakaash had mentioned in discord we seem to be skipping a check for memory cgroups validation, as done here in runtime-tools. @tsturzl do you have any idea, why we might be intentionally skipping this, or we might have just missed it originally?

If so, @omprakaash may I ask you to add that in both this and the other test? (You can do it in separate PR, we can merge this one before it)

tsturzl · 2024-02-20T18:14:09Z

@YJDoc2 Hmm, if you compare any of the integration tests to runtime-tools it seems like we're missing a similar step a few cgroups integration test I think I might have assumed there was something in check_container_created that was checking the container against the spec somehow. It's been 3 years, so I can't recall my thinking from back then.

A simple solution would be to just read the respective cgroups files and comparing them. It looks like cpu, memory, and network are all missing these checks.

There is a lot of opportunity here to create a read/write abstraction for cgroups files. Right now we just have a constant for each file name and use a write_cgroup_file utility function to write to a given path. If reading (or writing) cgroups file is going to become more useful outside the context of libcgroups crate then maybe we want a generalized abstraction to handle read/write? This would clean up the implementation and unit tests, as well as provide obvious utility to these integration tests in checking cgroups values without having to implement a lot of boilerplate. It's something I thought about introducing to make it easier to more easily swap out the kernel IO API being used which is something I've experimented with in the past. I don't know if that effort is too large to block fixing the issues with the current cgroups integration tests, but it's something I'd be interested in working on if no one else is immediately interested.

omprakaash · 2024-02-21T00:17:13Z

I could just implement the additional checks for this test with current existing methods in this PR. Can the new abstractions be part of a separate PR later on ?

tsturzl · 2024-02-21T00:24:49Z

@omprakaash that's probably a better immediate solution. If we can just implement something similar to what runtime tools is doing.

Signed-off-by: omprakaash <omsuseela@gmail.com>

YJDoc2 · 2024-03-11T05:34:15Z

I think I have some changes to request in this, I'll try to get to a full review soon.

YJDoc2

Hey, Apologies it took me long for the review. There are some changed needed, so I have added comments, please take a look.

Is this test supposed to be cgroups v1 specific? If not we should not use the v1 functions explicitly, and use a generic version that can work on both : v1 and v2.

Also, for the functions added in libcgroup : if we are using them in only tests, and they might not be as useful otherwise, I'd prefer to move them in the test crate itself rather then libcgroup.

YJDoc2 · 2024-03-18T05:43:49Z

crates/libcgroups/src/v1/util.rs

@@ -41,6 +46,71 @@ pub fn list_supported_mount_points() -> Result<HashMap<ControllerType, PathBuf>,
    Ok(mount_paths)
 }

+pub fn get_memory_data(pid: i32) -> Result<LinuxMemory, Box<dyn std::error::Error>> {


Do not use dyn error, instead create a proper error type similar to how we have made specialized errors in enum for other functions and use that (or alternatively reuse some existing error type if that fits)

Another thing is that , I would like to confirm if we are similarly exposing the oci_spec objects from any other existing functions : as this fn is pub, it will be part of our public api, and if we are using another crate (oci_spec) type here, we need to be careful for this. If any other function already does this, then fine, else need to think on how to properly expose this.

I do not see any other functions that do this. Would it be a better idea to just move this into the testing crate.

Yep. While it is not a bad idea to have this sort of function, I'd prefer not to add this to a core crate in a PR related to tests. If you are interested, we can open another issue and discuss the API for this ; but for this case, I'll request you to move the functions into the tests itself.

YJDoc2 · 2024-03-18T05:46:03Z

crates/libcgroups/src/v1/util.rs

+    let cgroup_memory_files = vec![
+        "memory.limit_in_bytes",
+        "memory.soft_limit_in_bytes",
+        "memory.memsw.limit_in_bytes",
+        "memory.kmem.limit_in_bytes",
+        "memory.kmem.tcp.limit_in_bytes",
+        "memory.swappiness",
+        "memory.oom_control",
+    ];


Would it be possible to not hardcode these (I think we probably can't, but just confirming )?

YJDoc2 · 2024-03-18T05:47:31Z

crates/libcgroups/src/v1/util.rs

+                        .parse::<u64>()?;
+                    memory_data = memory_data.disable_oom_killer(oom_control == 1);
+                }
+                _ => {}


as we are controlling the files by hardcoding, we should add unreachable!() in the remaining case

Got it. Will change it

YJDoc2 · 2024-03-18T05:55:31Z

crates/libcgroups/src/v1/util.rs

+pub fn get_subsystem_path(pid: i32, subsystem: &str) -> Result<PathBuf, io::Error> {
+    let contents = fs::read_to_string(Path::new(&format!("/proc/{}/cgroup", pid)))
+        .unwrap_or_else(|_| panic!("failed to read /proc/{}/cgroup", pid));
+    for line in contents.lines() {
+        let parts: Vec<&str> = line.splitn(3, ':').collect();
+        if parts.len() < 3 {
+            continue;
+        }
+        let subparts: Vec<&str> = parts[1].split(',').collect();
+        for subpart in subparts {
+            if subpart == subsystem {
+                return Ok(PathBuf::from(parts[2].to_string()));
+            }
+        }
+    }
+    Err(io::Error::new(
+        io::ErrorKind::Other,
+        format!("subsystem {} not found", subsystem),
+    ))
+}


I think we already have something similar to this existing somewhere in the libcgroup crate. Can you check and confirm if we really need a new function for this?

YJDoc2 · 2024-03-18T06:01:15Z

tests/contest/contest/src/tests/cgroups/memory.rs


-use crate::utils::{test_outside_container, test_utils::check_container_created};
+use crate::utils::{self, test_outside_container};


nit: can we combine this import with the utils import above?

YJDoc2 · 2024-03-18T06:01:49Z

tests/contest/contest/src/tests/cgroups/relative_memory.rs

+    let spec = SpecBuilder::default()
+        .linux(
+            LinuxBuilder::default()
+                .cgroups_path(Path::new("/testdir/runtime-test/container").join(cgroup_name))


nit: extract this path at top as a const

YJDoc2 · 2024-03-18T06:02:46Z

tests/contest/contest/src/tests/cgroups/relative_memory.rs

+fn can_run() -> bool {
+    Path::new(CGROUP_MEMORY_LIMIT).exists() && Path::new(CGROUP_MEMORY_SWAPPINESS).exists()
+}


If this is cgroups v1 specific test, we should also check that cgroups is v1 here

YJDoc2 · 2024-03-18T06:03:46Z

tests/contest/contest/src/utils/linux_resource_memory.rs

+    let expected_memory = spec
+        .linux()
+        .as_ref()
+        .unwrap()
+        .resources()
+        .as_ref()
+        .unwrap()
+        .memory()
+        .as_ref();


I think we can do without the as_ref calls here, as spec is already a immutable ref? Please check once.

YJDoc2 · 2024-03-18T06:06:28Z

tests/contest/contest/src/utils/linux_resource_memory.rs

+    if memory.is_err() {
+        bail!("failed to get memory data: {:?}", memory.err().unwrap());
+    }


Suggested change

if memory.is_err() {

bail!("failed to get memory data: {:?}", memory.err().unwrap());

}

if let Err(e) = memory {

bail!("failed to get memory data: {:?}", e);

}

YJDoc2 · 2024-03-18T06:09:19Z

tests/contest/contest/src/utils/linux_resource_memory.rs

+    if expected_memory.limit().unwrap() != memory.as_ref().unwrap().limit().unwrap() {
+        bail!("expected memory {:?}, got {:?}", expected_memory, memory);
+    }
+
+    if expected_memory.swappiness().unwrap() != memory.as_ref().unwrap().swappiness().unwrap() {
+        bail!("expected memory {:?}, got {:?}", expected_memory, memory);
+    }


For both of these, can we extract the corresponding params in vars, and compare those? Additionally we should also report specifically what param mismatch was there, as potentially (although it shouldn't) some other param in memory data can be diff, making it confusing as to exactly why this failed. So in error message it should be something like expected memory limit {expected_limit} but got {actual_limit} instead .

omprakaash · 2024-03-20T23:46:53Z

Thank you for the review ! Will make the required changes.

YJDoc2 · 2024-04-29T12:09:44Z

@omprakaash ping! There are still some review changes pending, so pinging you

omprakaash force-pushed the main branch from f77c370 to 36b59a7 Compare February 15, 2024 19:15

tsturzl added the kind/test label Feb 15, 2024

omprakaash force-pushed the main branch from 36b59a7 to 5bdb8be Compare February 16, 2024 03:46

tsturzl approved these changes Feb 16, 2024

View reviewed changes

Add test for cgroups_relative_memory

fe60f11

Signed-off-by: omprakaash <omsuseela@gmail.com>

omprakaash force-pushed the main branch from 5bdb8be to fe60f11 Compare February 16, 2024 19:50

omprakaash force-pushed the main branch 3 times, most recently from 11a55cd to 8332eb0 Compare February 25, 2024 23:11

Add validate_linux_resource_memory test helper

44e7b59

Signed-off-by: omprakaash <omsuseela@gmail.com>

omprakaash force-pushed the main branch from 8332eb0 to 44e7b59 Compare February 25, 2024 23:37

YJDoc2 requested changes Mar 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test for cgroups_relative_memory #2686

Add test for cgroups_relative_memory #2686

omprakaash commented Feb 15, 2024 •

edited

codecov-commenter commented Feb 15, 2024 •

edited

lengrongfu commented Feb 18, 2024

YJDoc2 commented Feb 20, 2024

tsturzl commented Feb 20, 2024 •

edited

omprakaash commented Feb 21, 2024

tsturzl commented Feb 21, 2024

YJDoc2 commented Mar 11, 2024

YJDoc2 left a comment

YJDoc2 Mar 18, 2024

omprakaash Apr 1, 2024

YJDoc2 Apr 1, 2024

YJDoc2 Mar 18, 2024

YJDoc2 Mar 18, 2024

omprakaash Mar 20, 2024

YJDoc2 Mar 18, 2024

YJDoc2 Mar 18, 2024

omprakaash Mar 20, 2024

YJDoc2 Mar 18, 2024

YJDoc2 Mar 18, 2024

YJDoc2 Mar 18, 2024

YJDoc2 Mar 18, 2024

YJDoc2 Mar 18, 2024

omprakaash commented Mar 20, 2024

YJDoc2 commented Apr 29, 2024


		use crate::utils::{test_outside_container, test_utils::check_container_created};
		use crate::utils::{self, test_outside_container};

Add test for cgroups_relative_memory #2686

Are you sure you want to change the base?

Add test for cgroups_relative_memory #2686

Conversation

omprakaash commented Feb 15, 2024 • edited

codecov-commenter commented Feb 15, 2024 • edited

Codecov Report

lengrongfu commented Feb 18, 2024

YJDoc2 commented Feb 20, 2024

tsturzl commented Feb 20, 2024 • edited

omprakaash commented Feb 21, 2024

tsturzl commented Feb 21, 2024

YJDoc2 commented Mar 11, 2024

YJDoc2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omprakaash commented Mar 20, 2024

YJDoc2 commented Apr 29, 2024

omprakaash commented Feb 15, 2024 •

edited

codecov-commenter commented Feb 15, 2024 •

edited

tsturzl commented Feb 20, 2024 •

edited