add schedule entity

[lengrongfu]

issue #1706

[codecov-commenter]

Codecov Report

Merging #2495 (378aa91) into main (8db3599) will decrease coverage by 0.39%.
Report is 5 commits behind head on main.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2495      +/-   ##
==========================================
- Coverage   65.89%   65.51%   -0.39%     
==========================================
  Files         133      133              
  Lines       16819    16916      +97     
==========================================
- Hits        11083    11082       -1     
- Misses       5736     5834      +98     

[utam0k]

@lengrongfu May I ask you to add the e2e test?
https://containers.github.io/youki/developer/e2e/rust_oci_test.html

[YJDoc2]

Hey @lengrongfu , if there is any issue with the e2e test addition, or any other problems that you might be facing, feel free to ping me!

[YJDoc2]

Hey @lengrongfu , are you still working on this? In case you're busy with something else, and cannot add the tests right now, can you post a comment with what the changes are and how one would approach testing them? That way we can either make and push tests to this branch ourselves, or merge this for now, and get another PR for the test. Let me know, and thanks for the work you have already done :)

[lengrongfu]

Hey @lengrongfu , are you still working on this? In case you're busy with something else, and cannot add the tests right now, can you post a comment with what the changes are and how one would approach testing them? That way we can either make and push tests to this branch ourselves, or merge this for now, and get another PR for the test. Let me know, and thanks for the work you have already done :)

I'm so sorry. please take look.

[YJDoc2]

I'm so sorry. please take look.

Hey, no worries, thanks for getting back 👍

A couple of quick questions :

I don't think the comment ////////// ANCHOR: create_spec policy: u32 is needed in the test, can it be removed?

Also in the integration tests, we are only checking that the containers are getting created / running successfully. Is there a way to check specifically that the policy we specified is getting applied correctly? That would be a better test.

Another thing is that this adds dependency of nc to do a raw syscall ; does nix or libc libraries does not support this? And if that is the case, can you add a comment in the code, with TODO saying to replace it when those library supports those?

Thanks!

[lengrongfu]

Also in the integration tests, we are only checking that the containers are getting created / running successfully. Is there a way to check specifically that the policy we specified is getting applied correctly? That would be a better test.

By calling the sched_getattr function, we can obtain the scheduling strategy of the current process and compare it with the expected value. What do you think of this solution?

does nix or libc libraries does not support this?

What I know is that nix does not have it. I need to check libc.

[YJDoc2]

By calling the sched_getattr function, we can obtain the scheduling strategy of the current process and compare it with the expected value. What do you think of this solution?

This seems like a good idea. This will be done from inside the container right (i.e. in runtimetests) ? We can set the sched strategy in the integration tests, maybe with some specific values, and check that inside the container.

What I know is that nix does not have it. I need to check libc.

👍

[lengrongfu]

i am in run just validate-rust-oci-runc command, how to watch container_init_process.rs file print log, can you help me?
@YJDoc2

[YJDoc2]

i am in run just validate-rust-oci-runc command, how to watch container_init_process.rs file print log, can you help me?

Hey, the validate-rust-oci-runc runs the rust oci test code (i.e. the ones we are changing here) on runc , not youki. So the container_init_process from youki is not involved at all. If the tests are failing, then is is possible that something we are expecting in the test is not supported / implemented differently by runc. What issue are you facing?

[YJDoc2]

Can you once check if sched policy is supported by runc yet or not? If it is not, then the issue is not with your code.

Also, don't mind the pending CI checks, that is due to changes from some other PR.

[YJDoc2]

Also, may I ask you to rebase on latest main, as we have made some changes regarding CI and build there

[lengrongfu]

runc is having this function，but not sure it is valid. i will try again.

[YJDoc2]

@lengrongfu Hey! Let me know if you need any help with this 👍
Also please rebase on the main to make the CI pass properly!

[lengrongfu]

@lengrongfu Hey! Let me know if you need any help with this 👍 Also please rebase on the main to make the CI pass properly!

current only runc integration_test can't run success.

[YJDoc2]

current only runc integration_test can't run success.

Thanks for rebasing and updating! Did you check if the runc release we are using in CI supports the sched entity as expected? If that is the case, I'll try to take a more detailed look on why this might be failing.

[YJDoc2]

@lengrongfu I checked this, and I think the issue is even though the sched support PR is merged in runc, its still not in any release. I tested this with main branch of runc, and the tests are passing with it.
I'll discuss and decide what should be done in such case, but for now can you take a look at my previous comments regarding the pid arg to set/get attr calls?
Thanks!

[YJDoc2]

@lengrongfu may I ask you to rebase this to current master, and for the failing runc test, for now do this :
keep everything the same, just comment out the addition of test in the main.rs . This will skip the test for now, but I'll do a fix in a separate PR to make it work for both.
Thanks a lot 🙏

[lengrongfu]

@YJDoc2 Do you know why this ci error? https://github.com/containers/youki/actions/runs/7538727425/job/20519819284?pr=2495

Run just runtimetest rust-oci-tests-bin
error: Justfile does not contain recipe `rust-oci-tests-bin`.
Error: Process completed with exit code 1.

[YJDoc2]

Do you know why this ci error?

We recently changed the file structure and the test tool name in a PR. If you rebased on recent main, you might have missed that change in it. Make sure that 1. just file is correctly picked from the main ; and 2. the test you added here is in correct dir after the rebase.

[YJDoc2]

Also, you can ref 16dedc2 for commenting out the test I mentioned

[YJDoc2]

@lengrongfu , apologies, the failing CI was an issue from our side, we missed a change in that PR. I have pused a fix, can you merge/rebase master and see 😓

[lengrongfu]

maybe need wait this pr merge #2615

[YJDoc2]

Hey @lengrongfu , I updated the nice value checking condition, similar to how you did it on runc . I'll wait for a bit and merge this after sometime, as otherwise this is ok. In case you think there is some mistake, please let me know ; even after merge, so we can correct it accordingly. Thanks!

[YJDoc2]

lgtm 👍