Integration test: cgroup v1 relative-cpus tests

[posutsai]

Hi, I have implemented the test_relative_cpus function to contribute to #361 by referencing linux_cgroups_relative_cpus.go and cpu/v2.rs. However, I encountered a few issues:

The current implementation closely mirrors the logic in v2.rs. Since there isn't a clear distinction between v1 and v2 logic in linux_cgroups_relative_cpus.go, would it be better to extract the common logic into a shared module?

I am also unsure about the correct way to run the tests. The just test-contest command on the main branch fails on Ubuntu 20.04. Could you provide guidance on resolving this issue?

Thank you for your help. I have learned a lot from this process.

[utam0k]

@YJDoc2 May I ask you to review this PR?

[YJDoc2]

Sure, I'll take a look at this 👍

[posutsai]

Sorry, I would like to make some refinement. @YJDoc2 , Could you please review later?

[YJDoc2]

@posutsai , sure. May I request you to mark this as draft and ping me when this is ready to review?
Also instead of having multiple functions or dealing with any, I can suggest two ways -

1. make function generic with type parameter constrained by PartialEq,Eq , that way you can use == operator. Might need some more bounds
2. just take everything as string. So instead of giving 1000 as number we give it as a string for expected value. Not the best way, but I think good enough for test result evaluation.

[posutsai]

Thank you. I have turned it to draft. Also I would like to know should I do the check in test_cpu_cgroups instead of declaring another function.

[posutsai]

Hi, @YJDoc2 I believe the review can resume. I use ToString input type to handle different and check equality between string. Thank you.

[YJDoc2]

Some changes, overall ok 👍
Thanks :)

[YJDoc2]

Hey, so I should have asked this a long time back, but even I'm catching this right now - What you have implemented is still using the absolute cgroup path right? This does not have anything corresponding to https://github.com/opencontainers/runtime-tools/blob/master/validation/linux_cgroups_relative_cpus/linux_cgroups_relative_cpus.go#L24 which sets the relative cgroup path.

[posutsai]

Hi @YJDoc2, please correct me if I am wrong. According to my understanding, the point of the unit test is to check correctness of the config related to relative CPU resource allocation e.g. shares, quota, period etc. instead of taking relative path as argument.

[YJDoc2]

According to my understanding, the point of the unit test is to check correctness of the config related to relative CPU resource allocation e.g. shares, quota, period etc. instead of taking relative path as argument.

Hey so from what I saw, if you check other relative* cgroup tests, the common thing is setting the relative cgroup path. If you see https://github.com/opencontainers/runtime-tools/blob/master/validation/linux_cgroups_cpus/linux_cgroups_cpus.go , which is non-relative, it still uses shares , quotas etc. So I think the aim of this test is to check correct usage of relative cgroup path.

[posutsai]

Hi, sorry for late reply. I've modified the code to use get_subsystem_mount_point and take relative path as input. Not sure if I do it the right way.

[YJDoc2]

Hi, sorry for late reply.

No worries! I'll try to take a look at this today or so.

[YJDoc2]

Hey, sorry it took me a while to get to this, got busy with other stuff. So in your recent changes, you are correctly taking it using the function however you are not setting the cgroup path as relative in the spec. For reference I'd suggest to take a look at https://github.com/containers/youki/pull/2686/files# , and see how it is done there, specifically https://github.com/containers/youki/pull/2686/files#diff-682de1209b41c713baac9258d0206875be877e6453cba0834357f52050d0520eR18-R21

I'll also suggest to take a look at discussion there, it seems the original author might not be continuing with the PR, so if you're fine with it, you might be able to take that one up after this (if you want).

[YJDoc2]

@posutsai , ping!

[YJDoc2]

Hey @posutsai , let me know if you have any issues, or cannot work this anymore. It is ok if you are busy with something else, just let me know so we can proceed accordingly. Thanks :)

[posutsai]

Hi, @YJDoc2. Sorry for reply lately. I've checked your reference url. I would like to know if the key point is to specify cgroups path as following.

    let spec = SpecBuilder::default()
        .linux(
            LinuxBuilder::default()
                // key here
                .cgroups_path(Path::new(RELATIVE_CGROUPS_PATH).join(cgroup_name))
                .resources(...)
                .build()
                .context("failed to build linux spec")?,
        )
        .build()
        .context("failed to build spec")?;

Thus, in our case we should avoid using the utility create_cpu_spec since there is no cgroups_path argument to pass in.

[YJDoc2]

I would like to know if the key point is to specify cgroups path as following.

Yes, we should be specifying the cgroups path similar to that.

Thus, in our case we should avoid using the utility create_cpu_spec since there is no cgroups_path argument to pass in.

Yes, we can either create a new fn to take a path param, or modify the original one to optinally take a param (Option or something like that) , and default to current behavior if no path is passed (i.e. None is passed)

[posutsai]

Please correct me if I am wrong. After checking the definition of the create_spec function, seems like I already specify the cgroups_path by calling it.

fn create_spec(cgroup_name: &str, case: LinuxCpu) -> Result<Spec> {
    let spec = SpecBuilder::default()
        .linux(
            LinuxBuilder::default()
                .cgroups_path(Path::new("/runtime-test").join(cgroup_name))
                .resources(
                    LinuxResourcesBuilder::default()
                        .cpu(case)
                        .build()
                        .context("failed to build resource spec")?,
                )
                .build()
                .context("failed to build linux spec")?,
        )
        .build()
        .context("failed to build spec")?;

    Ok(spec)
}

If that's the case, I guess I have no need to modify create_cpu_spec interface. Am I right?

[utam0k]

Is your intention to edit Spec after calling create_spec? I prefer passing a cgroup path to create_cpu_spec.

[posutsai]

Thank you for reply. I totally miss the point. I plan to modify create_spec and add an optional argument, since the original code already calls the builder there. Is it better than modify create_cpu_spec?

[utam0k]

Yes, it'd be better because create_cpu_spec should only be editing related CPU field,s not the cgroup path.

[posutsai]

I've add a is_relative argument in create_spec. But I have no idea why the CI fail.