Feat/podman rootless #2370

YJDoc2 · 2023-09-19T13:30:48Z

This switches from dbus-rs to natively implemented dbus connection implementation, in order to make rootless invocation with podman work. ~~This is almost done but still few final touches are remaining, so marked as WIP.~~

This PR can be split into 4 parts :

dbus_native implementation : commits b678337 , e3c6f57 , c36983a
Changing rest of stuff (tests, types etc) to use dbus_native : commits 982ec4b , 82add24 , 1e88851
Implementing the original dbus interface code into dbus native : commits e45d483 , a4169ba , 66fea6f
misc and documentation : commits 8f67d30 , a650514

Apologies for such large PR, but to validate that the code works, I have to completely switch over to dbus_native, and test. If required, I can split the PR into multiple ones according to above splits, but then the individual PRs may not be testable.

There are several places where I'm not particularly happy with my implementation, so any and all comments, suggestions and feedback is welcome to improve this!

Thank you :)

codecov-commenter · 2023-09-19T13:36:22Z

Codecov Report

Merging #2370 (263e6b8) into main (4b04abd) will increase coverage by 1.16%.
Report is 71 commits behind head on main.
The diff coverage is 72.27%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2370      +/-   ##
==========================================
+ Coverage   64.83%   65.99%   +1.16%     
==========================================
  Files         129      133       +4     
  Lines       15201    16814    +1613     
==========================================
+ Hits         9855    11096    +1241     
- Misses       5346     5718     +372

Signed-off-by: Yashodhan Joshi <[email protected]>

systemdClientInterface into dbus_native Signed-off-by: Yashodhan Joshi <[email protected]>

Signed-off-by: Yashodhan Joshi <[email protected]>

YJDoc2 · 2023-09-22T09:40:21Z

crates/libcgroups/src/systemd/cpuset.rs

+            let cpu_mask: Vec<_> = to_bitmask(cpus)
+                .map_err(SystemdCpuSetError::CpusBitmask)?
+                .into_iter()
+                .map(|v| v as u64)
+                .collect();
+            properties.insert(ALLOWED_CPUS, Variant::ArrayU64(cpu_mask));
        }

        if let Some(mems) = cpu.mems() {
-            let mems_mask = to_bitmask(mems).map_err(SystemdCpuSetError::MemoryNodesBitmask)?;
-            properties.insert(ALLOWED_NODES, Box::new(mems_mask));
+            let mems_mask: Vec<_> = to_bitmask(mems)
+                .map_err(SystemdCpuSetError::MemoryNodesBitmask)?
+                .into_iter()
+                .map(|v| v as u64)
+                .collect();
+            properties.insert(ALLOWED_NODES, Variant::ArrayU64(mems_mask));


In tests as well as other uses, the original code expected u64, so changed this to type u64 as well, instead of casting there

YJDoc2 · 2023-09-22T09:41:23Z

crates/libcgroups/src/systemd/dbus_native/dbus.rs

+    /// socket fd
+    socket: i32,
+    /// name id assigned by dbus for the connection
+    id: Option<String>,


it is not necessary to send ID, but I have added it as sending it is a better practice

YJDoc2 · 2023-09-22T09:43:29Z

crates/libcgroups/src/systemd/dbus_native/message.rs

Unit tests in this test the combined functionality of message serialization and individual type serialization. The "original" message in all tests are taken from zbus calls, so we know that those are valid messages.

YJDoc2 · 2023-09-22T10:10:36Z

With these changes, the failing tests in podman rootless are 136, 4 more that current main (132). I'll check and fix them in a separate PR, this only fixes the running with podman issues.
The binary size is also slightly large, but not much, which I think is acceptable.

The primary test for validating is the following command should run with and without root permissions, both

podman create --runtime $PWD/youki --name test hello-world
podman start -ia test # this should print the hello world message on terminal
podman rm test

podman run -it fedora /bin/bash # verify you can run commands inside the container

utam0k

Awesome.
Especially, I think it is good to have a good unit tests Why not divert some unit tests and add doc test?

docs/src/user/basic_setup.md

docs/src/user/dbus-native.md

yihuaf · 2023-09-24T17:33:11Z

.github/workflows/e2e.yaml

@@ -42,7 +42,7 @@ jobs:
          go-version: '1.20'
          cache: true
      - run: sudo apt-get -y update
-      - run: sudo apt-get install -y pkg-config libsystemd-dev libdbus-glib-1-dev libelf-dev libseccomp-dev btrfs-progs libbtrfs-dev


yihuaf · 2023-09-24T17:37:16Z

This is technically a breaking change. I would include this change in a minor version bump.

YJDoc2 · 2023-09-25T06:22:23Z

This is technically a breaking change. I would include this change in a minor version bump.

Yes, I will be adding a migration guide for this as @utam0k has suggested, just haven't bumped the version in this PR, as we usually do that in the release PRs when we do a new release.

YJDoc2 · 2023-09-25T06:24:57Z

Especially, I think it is good to have a good unit tests Why not divert some unit tests and add doc test?

@utam0k , I'm not sure what you mean by this? Do you want me to change some of the current unit tests that I have added to doc tests instead, or do you want to keep unit tests as they are, and add more tests as doc tests?

utam0k · 2023-09-25T11:25:56Z

Especially, I think it is good to have a good unit tests Why not divert some unit tests and add doc test?

@utam0k , I'm not sure what you mean by this? Do you want me to change some of the current unit tests that I have added to doc tests instead, or do you want to keep unit tests as they are, and add more tests as doc tests?

May I ask you to use documentation tests in simple util functions? For example:
https://github.com/containers/youki/pull/2370/files#diff-9347c2507177cda90306312df09422eccd4d3a259bc837ecb73a78c963a5c0f5R81

YJDoc2 · 2023-10-05T06:23:37Z

Hey @utam0k apologies for the delay. I have updated the migration guide and docs as you commented.
I tried converting some utils tests to doctests, but as the dbus_native module is private, they cannot be imported in the doctest.

When compiling a crate for use in doctests (with --test option), rustdoc will set #[cfg(doctest)]. Note that they will still link against only the public items of your crate; if you need to test private items, you need to write a unit test.

https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html#include-items-only-when-collecting-doctests

So for now I have kept the unit tests as they were.
Please take a look : )

Signed-off-by: Yashodhan Joshi <[email protected]>

utam0k

May I ask you to add e2e test for rootless podmain in another PR if possible?

utam0k · 2023-10-05T12:08:25Z

@orimanabu May I ask you to give it a try?

YJDoc2 · 2023-10-05T14:57:45Z

May I ask you to add e2e test for rootless podmain in another PR if possible?

Yes, I'll do that 👍

YJDoc2 force-pushed the feat/podman-rootless branch from 8293065 to 78bb880 Compare September 19, 2023 13:31

YJDoc2 added the kind/feature label Sep 19, 2023

YJDoc2 added 11 commits September 21, 2023 14:27

Move dbus_native code to youki, add tests

b678337

Signed-off-by: Yashodhan Joshi <[email protected]>

(incomplete) move individual systemd components to dbus_native

982ec4b

Signed-off-by: Yashodhan Joshi <[email protected]>

(incomplete) Change tests to use dbus_native, move

82add24

systemdClientInterface into dbus_native Signed-off-by: Yashodhan Joshi <[email protected]>

(incomplete) Change variant to actual enum, Fix types for systemd

1e88851

Signed-off-by: Yashodhan Joshi <[email protected]>

Fix bug in vector ser/de

e3c6f57

Signed-off-by: Yashodhan Joshi <[email protected]>

Fix mut requirements, minor lints etc., add client id in dbus

c36983a

Signed-off-by: Yashodhan Joshi <[email protected]>

Add implementation for proxy methods

e45d483

Signed-off-by: Yashodhan Joshi <[email protected]>

Split dbus error from SystemdClientError

a4169ba

Signed-off-by: Yashodhan Joshi <[email protected]>

implement system and session connection creation

66fea6f

Signed-off-by: Yashodhan Joshi <[email protected]>

Fix typos and failing feature test build

8f67d30

Signed-off-by: Yashodhan Joshi <[email protected]>

Completely remove dbus dependency and add docs regarding dbus-native

a650514

Signed-off-by: Yashodhan Joshi <[email protected]>

YJDoc2 force-pushed the feat/podman-rootless branch from 9a31896 to a650514 Compare September 21, 2023 08:57

YJDoc2 commented Sep 22, 2023

View reviewed changes

YJDoc2 marked this pull request as ready for review September 22, 2023 09:44

YJDoc2 requested review from utam0k and a team September 22, 2023 09:45

utam0k reviewed Sep 23, 2023

View reviewed changes

docs/src/user/basic_setup.md Show resolved Hide resolved

docs/src/user/dbus-native.md Outdated Show resolved Hide resolved

yihuaf approved these changes Sep 24, 2023

View reviewed changes

YJDoc2 force-pushed the feat/podman-rootless branch from b579273 to 998a715 Compare October 5, 2023 06:19

Update migration guide, move documentation od dbus_native to cgroups

263e6b8

Signed-off-by: Yashodhan Joshi <[email protected]>

YJDoc2 force-pushed the feat/podman-rootless branch from 998a715 to 263e6b8 Compare October 5, 2023 06:29

YJDoc2 requested a review from utam0k October 5, 2023 07:25

utam0k approved these changes Oct 5, 2023

View reviewed changes

utam0k merged commit cc39179 into youki-dev:main Oct 5, 2023

This was referenced Oct 5, 2023

Change dbus interfacing for making rootless podman containers work #2208

Closed

Check youki is started in user namespace #1447

Closed

Add initial tests for rootless podman #2406

Merged

YJDoc2 added the breaking change label Oct 9, 2023

Pvlerick mentioned this pull request Oct 27, 2023

Unable to start rootless container under Podman when in WSL #2484

Open

YJDoc2 mentioned this pull request Nov 6, 2023

[WIP] Allow youki to run with podman in rootless #1171

Closed

YJDoc2 deleted the feat/podman-rootless branch January 2, 2024 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/podman rootless #2370

Feat/podman rootless #2370

YJDoc2 commented Sep 19, 2023 •

edited

Loading

codecov-commenter commented Sep 19, 2023 •

edited

Loading

YJDoc2 Sep 22, 2023

YJDoc2 Sep 22, 2023

YJDoc2 Sep 22, 2023

YJDoc2 commented Sep 22, 2023

utam0k left a comment

yihuaf Sep 24, 2023

yihuaf commented Sep 24, 2023

YJDoc2 commented Sep 25, 2023

YJDoc2 commented Sep 25, 2023

utam0k commented Sep 25, 2023

YJDoc2 commented Oct 5, 2023

utam0k left a comment

utam0k commented Oct 5, 2023

YJDoc2 commented Oct 5, 2023

Feat/podman rootless #2370

Feat/podman rootless #2370

Conversation

YJDoc2 commented Sep 19, 2023 • edited Loading

codecov-commenter commented Sep 19, 2023 • edited Loading

Codecov Report

YJDoc2 Sep 22, 2023

Choose a reason for hiding this comment

YJDoc2 Sep 22, 2023

Choose a reason for hiding this comment

YJDoc2 Sep 22, 2023

Choose a reason for hiding this comment

YJDoc2 commented Sep 22, 2023

utam0k left a comment

Choose a reason for hiding this comment

yihuaf Sep 24, 2023

Choose a reason for hiding this comment

yihuaf commented Sep 24, 2023

YJDoc2 commented Sep 25, 2023

YJDoc2 commented Sep 25, 2023

utam0k commented Sep 25, 2023

YJDoc2 commented Oct 5, 2023

utam0k left a comment

Choose a reason for hiding this comment

utam0k commented Oct 5, 2023

YJDoc2 commented Oct 5, 2023

YJDoc2 commented Sep 19, 2023 •

edited

Loading

codecov-commenter commented Sep 19, 2023 •

edited

Loading