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

Upgrade oci-spec-rs to 0.4.0 #266

Merged
merged 12 commits into from
Sep 12, 2021
Merged

Conversation

guni1192
Copy link
Contributor

@guni1192 guni1192 commented Sep 5, 2021

fix: #225

Common

  • Fix import path: use oci_spec::XXX -> use oci_spec::runtime::XXX

Upgrade oci-spec-rs in cgroups

  • Port FreezerState to cgroups crate
    • Controller::apply() receive ContainerOpt instead of LinuxResources
  • Remove A from LinuxDeviceType

Upgrade oci-spec-rs in youki

  • Fix capability type (Capability type change: Vec -> HashSet)
  • Implement functions equivalent to LinuxDeviceType::to_sflag in youki.

Upgrade oci-spec-rs to v0.5.1

Signed-off-by: Takashi IIGUNI <iiguni.tks@gmail.com>
1. Fix capability type (Capability type change: Vec -> HashSet)
2. Implement functions equivalent to LinuxDeviceType::to_sflag in youki.
3. Fix crate path: use oci_spec::XXX -> use oci_spec::runtime::XXX

Signed-off-by: Takashi IIGUNI <iiguni.tks@gmail.com>
@guni1192 guni1192 marked this pull request as ready for review September 7, 2021 00:05
@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2021

Codecov Report

Merging #266 (472d646) into main (c247c90) will decrease coverage by 0.68%.
The diff coverage is 28.13%.

@@            Coverage Diff             @@
##             main     #266      +/-   ##
==========================================
- Coverage   69.62%   68.93%   -0.69%     
==========================================
  Files          46       46              
  Lines        5660     5837     +177     
==========================================
+ Hits         3941     4024      +83     
- Misses       1719     1813      +94     

@yihuaf
Copy link
Collaborator

yihuaf commented Sep 7, 2021

@guni1192 Hi not sure if merge conflicts are becoming a lot of work with all the other PR going in. If you have a good idea on when you can get the PR ready, may be we can stop merging other PR for a day or two to get this PR merged. This is an important PR and we probably want this to go in as soon as possible. Let us know how we can help you and/or ping us on Discord.

@guni1192
Copy link
Contributor Author

guni1192 commented Sep 8, 2021

@yihuaf Thank you very much for your kindness.
I'm currently dealing with an issue where conflicts and integration tests are failing.
I hope to address this issue by the end of the week, but I need a favor.

  • Please wait a bit before merging PRs that might conflict.
  • I don't know why the tests in linux_cgroups_devices/linux_cgroups_devices.t fail, so I need your help.
    Unfortunately, I don't have enough knowledge about cgroup device.

Signed-off-by: Takashi IIGUNI <iiguni.tks@gmail.com>
@Furisto
Copy link
Collaborator

Furisto commented Sep 8, 2021

@guni1192 What is the error? When I run the test with your changes it is passing.

@guni1192
Copy link
Contributor Author

guni1192 commented Sep 8, 2021

@Furisto Thanks for trying it out.
Both CI and local fail with the same test cases.
I am using the Cgroup v2 systemd driver environment.
The result is as follows.

 $   ./integration_test.sh
Running create/create.t
[sudo] password for guni:
Running default/default.t
Running delete/delete.t
Running delete_only_create_resources/delete_only_create_resources.t
Running delete_resources/delete_resources.t
Running hooks/hooks.t
Running hooks_stdin/hooks_stdin.t
Running hostname/hostname.t
Running kill/kill.t
Running kill_no_effect/kill_no_effect.t
Running killsig/killsig.t
Running linux_cgroups_cpus/linux_cgroups_cpus.t
Running linux_cgroups_devices/linux_cgroups_devices.t
TAP version 13
ok 1 - find devices
ok 2 - get devices data
not ok 3 - devices c 10:229 rwm is set correctly
# expect: c 10:229 rwm, actual: a 0:0 rwm
# The runtime MUST apply entries in the listed order
# Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.2-dev/config-linux.md#device-whitelist
1..3

Signed-off-by: Takashi IIGUNI <iiguni.tks@gmail.com>
Signed-off-by: Takashi IIGUNI <iiguni.tks@gmail.com>
Signed-off-by: Takashi IIGUNI <iiguni.tks@gmail.com>
@utam0k
Copy link
Member

utam0k commented Sep 8, 2021

I have a feeling that this is probably an effect of this.
youki-dev/oci-spec-rs#16
@Furisto
What do you think?

@Furisto
Copy link
Collaborator

Furisto commented Sep 8, 2021

@utam0k Yes, that was also my suspicion.

@utam0k
Copy link
Member

utam0k commented Sep 8, 2021

@Furisto
It seemed we needed the A. I'm sorry, I forgot. I'll create a PR to fix it.
https://www.kernel.org/doc/Documentation/cgroup-v1/devices.txt

A whitelist entry has 4 fields.
'type' is a (all), c (char), or b (block).

@utam0k
Copy link
Member

utam0k commented Sep 8, 2021

@guni1192 @Furisto I got the 0.4.0 oci-spec-rs locally and applied the same fix as this PR and it passed the integration test, so this is probably the cause of the problem.
youki-dev/oci-spec-rs#65

@utam0k
Copy link
Member

utam0k commented Sep 8, 2021

@guni1192 I'm going to ask them to release a fixed version, 0.5.1. However, it may be difficult to address this issue without upgrading the version to 0.5.1. Sorry...

@guni1192
Copy link
Contributor Author

guni1192 commented Sep 8, 2021

@utam0k I understand. I'll try as much as I can.

@guni1192 guni1192 changed the title [WIP] Upgrade oci-spec-rs to 0.4.0 [WIP] Upgrade oci-spec-rs to 0.5.1 Sep 8, 2021
@utam0k
Copy link
Member

utam0k commented Sep 10, 2021

@guni1192 The change to the builder pattern would not have been that exciting. Very helpful!

@yihuaf
Copy link
Collaborator

yihuaf commented Sep 10, 2021

Not sure if this is helpful, but maybe we can bump commit up instead of version, if the changes are too big to handle in one PR? Then there may be smaller bits to bite off? Just a suggestion :)

@utam0k
Copy link
Member

utam0k commented Sep 10, 2021

@yihuaf
This is my mistake, but there were some bugs in oci-spec-rs, so CI won't go through unless the current oci-spec-rs is the latest version.
@guni1192
However, if you need it, I can fork it and prepare a backported version to 0.4.0 with bug fixes. What do you think?

@guni1192
Copy link
Contributor Author

@utam0k I thought it was a good idea too.
It looks like supporting the builder pattern for youki will be a bigger task than I thought.
In particular, I expect the changes to tenant_builder.rs to be significant.
I would like you to backport your patch to 0.4.0.

@utam0k
Copy link
Member

utam0k commented Sep 11, 2021

@guni1192 I prepared v0.4.0 with a bug fix.
https://github.com/utam0k/oci-spec-rs/tree/v0.4.0-with-bugfix

And I have tried using it and did integration test. I applied it and pass all tests.

diff --git a/src/rootfs.rs b/src/rootfs.rs
index f5884d8..0b63456 100644
--- a/src/rootfs.rs
+++ b/src/rootfs.rs
@@ -224,6 +224,7 @@ fn bind_dev(rootfs: &Path, dev: &LinuxDevice) -> Result<()> {
 
 fn to_sflag(dev_type: LinuxDeviceType) -> SFlag {
     match dev_type {
+        LinuxDeviceType::A => SFlag::S_IFBLK | SFlag::S_IFCHR | SFlag::S_IFIFO,
         LinuxDeviceType::B => SFlag::S_IFBLK,
         LinuxDeviceType::C | LinuxDeviceType::U => SFlag::S_IFCHR,
         LinuxDeviceType::P => SFlag::S_IFIFO,

Signed-off-by: Takashi IIGUNI <iiguni.tks@gmail.com>
@guni1192 guni1192 changed the title [WIP] Upgrade oci-spec-rs to 0.5.1 [WIP] Upgrade oci-spec-rs to 0.4.0 Sep 12, 2021
Signed-off-by: Takashi IIGUNI <iiguni.tks@gmail.com>
@guni1192
Copy link
Contributor Author

@utam0k 0.4.0-with-bugfix has passed all tests, what about support for 0.5.1?
Do you want to continue with this PR or split the PR?

@utam0k
Copy link
Member

utam0k commented Sep 12, 2021

@utam0k 0.4.0-with-bugfix has passed all tests, what about support for 0.5.1?
Do you want to continue with this PR or split the PR?

@guni1192
Thank you, the upgrade to 0.5.1 should be a separate PR considering the conflicts with other PRs.

Cargo.toml Outdated
@@ -38,9 +38,10 @@ fastrand = "1.4.1"
crossbeam-channel = "0.5"

[dev-dependencies]
oci_spec = { git = "https://github.com/containers/oci-spec-rs", rev = "e0de21b89dc1e65f69a5f45a08bbe426787c7fa1", features = ["proptests"]}
# oci-spec = { version = "0.4.0", features = ["proptests"] }
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason to keep this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry ... I forgot to delete that line.

Signed-off-by: Takashi IIGUNI <iiguni.tks@gmail.com>
@guni1192
Copy link
Contributor Author

Thank you, the upgrade to 0.5.1 should be a separate PR considering the conflicts with other PRs.

All right. I think so too.
I'll delete [WIP].

@guni1192 guni1192 changed the title [WIP] Upgrade oci-spec-rs to 0.4.0 Upgrade oci-spec-rs to 0.4.0 Sep 12, 2021
Copy link
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.

lgtm

@utam0k utam0k merged commit 7aaa2fb into youki-dev:main Sep 12, 2021
@yihuaf yihuaf mentioned this pull request Sep 15, 2021
4 tasks
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.

Upgradeoci-spec-rs
5 participants