Modularize container creation #121

Furisto · 2021-07-02T22:50:52Z

This attempts to make the container creation code more modular. Related to #20 and #118.

Furisto · 2021-07-02T23:02:06Z

Will check the test failure. Works fine for me locally.

utam0k · 2021-07-03T01:25:12Z

As for the overall direction, I agree with you. It's so cool. I'll review this when WIP comes off.

The current monolithic builder provides options that should only be called during init and not when creating a tenant and vice versa. This puts the burden on the user of the builder to know which methods are safe to call. Now the ContainerBuilder can be used to specify options that are common to both scenarios and afterwards as_init/as_tenant can be called to provide scenario specific options. This also simplifies the whole "if init then else" branching logic during container build.

Furisto · 2021-07-05T21:09:23Z

@utam0k This is now in a state where it can be reviewed

utam0k · 2021-07-06T06:54:25Z

.github/workflows/main.yml

@@ -31,6 +31,8 @@ jobs:
        run: ./build.sh
      - name: Run tests
        run: cargo test
+      - name: Run doc tests
+        run: cargo test --doc


utam0k · 2021-07-06T06:57:09Z

src/capabilities.rs

@@ -1,4 +1,4 @@
-use crate::command::Command;
+use crate::command::{Syscall};


The {} is probably unnecessary.

utam0k · 2021-07-06T07:03:40Z

src/container/builder.rs

+    /// let builder = ContainerBuilder::new("74f1a4cb3801".to_owned());
+    /// ```
+    pub fn new(container_id: String) -> Self {
+        let root_path = PathBuf::from("/run/youki");


It may be better to use Default Trait.

Also, it's better to use Default for other builders as well.

The default trait does not allow to specify arguments. I could do

ContainerBuilder::default() .with_id(id)

instead of with new(id) and generate an id automatically if none is specified in with_id if you like. The TenantContainerBuilder and InitContainerBuilder are not intended to be instantiated directly but by calling as_tenant/is_tenant on the ContainerBuilder, so you do not really have a default value for them because you always have a builder that will be provided in new(...)

I was planning to use default() in new, and I thought default would be better to use in case there are more constructor functions of different types.
However, it's a small thing, so I think it's fine to prepare it when it's needed.
What do you think?

We only have one constructor function at the moment, so I think we should do a default implementation when we start to get multiple.

utam0k · 2021-07-06T07:11:59Z

src/utils.rs

@@ -96,6 +96,11 @@ pub fn write_file<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) -> Resul
    Ok(())
 }

+pub fn create_dir_all<P: AsRef<Path>>(path: P) -> Result<()> {


I thought there might be no need to bother wrapping it in a function, is there any reason for this?

In the case of an error I want that the error message contains the path that should have been created, In the past it had been difficult to find out where the error is happening without this information. I could add with_context to every call, but having it in one place made more sense to me.

Furisto · 2021-07-06T20:53:47Z

There still seems to be some sporadic issue where the container is stopped during the kill integration test. Looking into this...

utam0k · 2021-07-07T02:33:01Z

There still seems to be some sporadic issue where the container is stopped during the kill integration test. Looking into this...

This may take a little time to find the cause. I've been trying to find time to fix it, but haven't been able to. If you find out anything, please let me know. In the worst case, just creating an issue is fine for this PR.

Furisto · 2021-07-08T16:45:08Z

@utam0k I have created #126 for the CI issues. If you have no objections we can merge this.

utam0k

awesome

add default handling when there isn't cgroup path in config.json. organize the logger. Merge pull request youki-dev#47 from utam0k/organize-log organize the logger. Merge pull request youki-dev#45 from utam0k/no-cgroup-path add default handling when there isn't cgroup path in config.json. ignore submodule artifacts. add a comment in oci_spec. make sure that config.json can be read regardless of where it is executed. add the tutorial on using youki. Merge pull request youki-dev#49 from utam0k/tutorial add the tutorial on using youki. update README. Merge pull request youki-dev#50 from utam0k/edit-readme update README. Reorganize cgroup modules cpu and mems is optional according to spec Spike cgroupv2 implementation Move CgroupManager to common module Implement cgroup manager trait Detect group version and create manager Improve file write options Implement string trait for controller types style Support cpu controller Add test module Style Enable controllers along the hierarchy Add tests for cpu controller Add modules for controllers Add tests for cpuset controller Use Result consistently Rework cgroup fs detection Support removal of cgroup Remove types module Formatting only Prefer v1 over v2 Fix integration test failure Control selection of cgroup fs with env variable Get rid of warnings Merge pull request youki-dev#48 from Furisto/cgroupv2 Initial support for cgroups v2 cargo clippy. make log level debug to get more information when ci failed. Merge pull request youki-dev#53 from utam0k/fix-unstable-ci make log level debug to get more information when ci failed. 4b260b0 Merge branch 'main' of github.com:utam0k/youki into HEAD Merge pull request youki-dev#52 from utam0k/cargo-clippy cargo clippy. Align cgroup controller implementations - Use common method to write to cgroup file - Ensure that pid is always written to the cgroup Merge pull request youki-dev#54 from Furisto/cg-common Align cgroup controller implementations add spec cli command resolve conflicts impl defaults for lots of structs Consolidate cgroup test methods Merge pull request youki-dev#57 from Furisto/cg-tests Consolidate cgroup test methods Update README.md - Change comment out sign `//` to `#` in shell script - Add new line at end of file formatting add back default attributes Update comment Merge branch 'main' of github.com:utam0k/youki into main Update comment Merge branch 'main' of github.com:YJDoc2/youki into main Add 'Community' section to README.md Merge pull request youki-dev#59 from nimrodshn/add_community_section_to_readme Add 'Community' section to README.md Update README: Split code section in tutorial - Split code section in tutorial. Merge pull request youki-dev#58 from aoki/main Update README.md Update comment, fix cargo-clippy warning Merge pull request youki-dev#43 from YJDoc2/main Add comments to create.rs utam0k -> containers Signed-off-by: Sora Morimoto <sora@morimoto.io> Merge pull request youki-dev#61 from smorimoto/utam0k-containers utam0k -> containers Add cgroup v1 cpu controller Add cgroup v1 cpuset controller Unit tests for cgroup v1 cpu controller Unit tests for cgroup v1 cpuset controller Cut down on boilerplate Activate cpu integration test Fix error while moving task into cgroup If a task is moved into the cgroup and a value has not been set for cpus and mems Errno 28 (no space left on device) will be returned. Therefore we set the value from the parent if required. Improve reliability of cgroup removal Merge pull request youki-dev#63 from Furisto/cg-cpu-v1 Support for cgroup v1 cpu and cpuset subsystem Add comments to process module and minor refactoring Changed hard coded mentions of poll time and event numbers to references of constants Merge pull request youki-dev#64 from YJDoc2/main Add comments to process module and minor refactoring Added install command for prerequisite in README Merge pull request youki-dev#66 from PeterYordanov/add-install-prerequisite Added install command for prerequisite in README Fixed spelling mistake in src/rootfs.rs Merge pull request youki-dev#67 from PeterYordanov/spelling-mistake-rootfs Fixed spelling mistake in src/rootfs.rs add handling of WouldBlock error. Use init pid instead of child pid Ensure controllers are enabled at the root Logging Added folder structure section in README Added folder structure section in README Added resources to folder structure section Fixed spelling mistakes Added documentation comment for cgroup module Added doc comments for modules Removed folder structure section Update doc comment Merge pull request youki-dev#68 from utam0k/handle_wouldblock add handling of WouldBlock error. Create a template for integration tests. refactore Run fmt Merge branch 'main' into add-spec-arg Added doc on how to run integration tests Update youki path Change build command Merge pull request youki-dev#73 from minakawa-daiki/fix-youki-path Change execution path and fix CI Merge branch 'main' into add-integration-template Merge pull request youki-dev#69 from Furisto/cg-escape Fix issues with cgroup v1 and v2 Merge pull request youki-dev#71 from minakawa-daiki/add-integration-template Added Integration test template basic device tests property testing on devices property testing hugetbl and memory controllers fix comount checks Updated doc comments migrate LinuxCapabilityType to enum Update caps_migrations.rs Fixed typo Unfinished sentence Merge pull request youki-dev#70 from PeterYordanov/added-doc-comments-modules Added doc comments modules Add comments to container module make LinuxCapabilityType add some widgets to README.md Merge pull request youki-dev#76 from utam0k/widget add some widgets to README.md Handle relative cgroup paths Ensure parents have value Merge pull request youki-dev#74 from Furisto/cg-relative Handle relative cgroup paths address requested changes Merge pull request youki-dev#75 from tsturzl/main Improved testing, property testing, device tests impl From for Capabilities and add tests Add comments to command module Add draft-doc for container and command module Merge branch 'main' of github.com:containers/youki into main Merge pull request youki-dev#79 from YJDoc2/main Document Container and Command modules Fix README badges fix github ci badge typo... Merge pull request youki-dev#80 from tsturzl/main Fix badges in README add create kill delete state in integration test Merge pull request youki-dev#81 from duduainankai/add-integration-test add create kill delete state in integration test Provide better error messages Merge pull request youki-dev#84 from Furisto/cg-better-errors Provide better error messages limit scope of unsafe get rid of unneeded unsafes make sure any values with `=` are not affected Merge pull request youki-dev#85 from tsturzl/main Clean up use of unsafe Add info command Merge pull request youki-dev#83 from Furisto/info-cmd Add info command Merge remote-tracking branch 'upstream/main' into add-spec-arg resolve conflict add lots of comments Add CODE-OF-CONDUCT.md and SECURITY.md Fix README link typo Merge pull request youki-dev#88 from sasurau4/fix/readme-link Fix README link typo Merge pull request youki-dev#86 from utam0k/coc-security Add CODE-OF-CONDUCT.md and SECURITY.md docs clean up around the tty. Renamed Cond to Pipe Merge pull request youki-dev#89 from utam0k/tty-refactor clean up around the tty. Merge pull request youki-dev#90 from YJDoc2/main Rename Cond to Pipe make sure to log any unimplemented controllers. Merge pull request youki-dev#91 from utam0k/unknown-controller make sure to log any unimplemented controllers. Add support for cpuacct in cgroup v1. Remove unnecessary processing. use bail! insted of anyhow Merge pull request youki-dev#92 from yjuba/v1-cpuacct [WIP] Add support for cpuacct in cgroup v1. Merge pull request youki-dev#94 from utam0k/bail use bail! insted of anyhow Add cgroup v1 freezer controller Add a test for applying CpuAcct. Merge pull request youki-dev#96 from yjuba/add-cpuacct-test Add a test for applying CpuAcct. improve build time in CI Check if rootless container is required and ensure prerequisites Ensure map binaries are available Implement protocol for identifier mapping Ensure root directory can be written by non root user Identifier mapping names were not correct Only one mapping needs to match Prevent panic when resources are not specified Fix wrong mapping Clippy and fmt Merge pull request youki-dev#93 from duduainankai/add-freezer Add cgroup v1 freezer controller remove the cargo-when dependency. Address review comments - should_use_rootless doesn't need Result type - add warning regarding current rootless limitations - make lookup_map_binary more concise Merge pull request youki-dev#98 from Furisto/rootless Experimental support for rootless containers Add unit tests for tty module Reuse tmpdir implementation from cgroup Extend info cmd with version and os Merge pull request youki-dev#102 from constreference/tty Add unit tests for tty module Merge pull request youki-dev#101 from Furisto/info-ex Extend info cmd with version and os Use `assert!` instead of `assert_eq!` when comparing a boolean. Merge pull request youki-dev#104 from utam0k/fix-clippy Use `assert!` instead of `assert_eq!` when comparing a boolean. Add support for systemd managed cgroups Merge pull request youki-dev#46 from nimrodshn/add_support_for_systemd_cgroups Add support for systemd managed cgroups update README.md Merge pull request youki-dev#105 from utam0k/update-README update README.md clean up comments Fix README.md Fedora & Centos instructions Merge pull request youki-dev#107 from nimrodshn/fix_readme Fix README.md Fedora & Centos instructions Add list command Merge pull request youki-dev#108 from Furisto/list-cmd Add list command fix conflicts. Merge pull request youki-dev#97 from utam0k/actions-cache improve build time in CI split the subcommands into their own files. Merge pull request youki-dev#110 from utam0k/refactor-cli-commands split the subcommands into their own files. Seperate adding tasks and applying resource restrictions Shorten names Update README.md Change `dnf` to `apt-get` for Debian based systems Address review comments - Add comments for functions - Use better naming in systemd cgroup manager Merge pull request youki-dev#112 from bkochendorfer/patch-1 Update README.md Merge pull request youki-dev#111 from Furisto/cg-add Seperate adding tasks to cgroups and applying resource restrictions Require only cgroups that are needed to fullfill the resource restrictions Merge pull request youki-dev#114 from Furisto/cg-only-required Require only requested cgroups to be present force delete container if it is running or created Merge pull request youki-dev#115 from TinySong/main force delete container if it is running or created add comments in intergration_test.sh about test case that runc no paas Merge pull request youki-dev#116 from TinySong/main add comments in intergration_test.sh about test case that runc no paas remove unnecessary clone() in create.rs Merge pull request youki-dev#117 from utam0k/refactor-create-remove-unnecessary-clone remove unnecessary clone() in create.rs Allow wider range of arguments for spec loading Rename command Provide context in case of errors during dir creation Ensure file info is captured Modularize create code add cgroup v2 pid controller Merge pull request youki-dev#119 from TinySong/main add cgroup v2 pids controller make String to signal conversion more user friendly by using a Trait. add tests for the signal. Split container builder into dedicated init and tenant builders The current monolithic builder provides options that should only be called during init and not when creating a tenant and vice versa. This puts the burden on the user of the builder to know which methods are safe to call. Now the ContainerBuilder can be used to specify options that are common to both scenarios and afterwards as_init/as_tenant can be called to provide scenario specific options. This also simplifies the whole "if init then else" branching logic during container build. Add documentation Remove tests Renaming Fix kill cmd test failures Execute doc tests Merge pull request youki-dev#122 from utam0k/refactor-signal make String to signal conversion more simplify by using a Trait. Review feedback and fmt Reduce binary size Merge pull request youki-dev#124 from Furisto/release Reduce size of binary Add cgroup v2 freezer controller Merge pull request youki-dev#123 from duduainankai/add-freezer-v2 Add cgroup v2 freezer controller Merge pull request youki-dev#121 from Furisto/builder Modularize container creation fix the warnings shown by cargo clippy support cgroupv2 io controller Merge pull request youki-dev#128 from TinySong/cgroupv2-io-controller Cgroupv2 io controller Merge pull request youki-dev#127 from utam0k/cargo-clippy-2 fix the warnings shown by cargo clippy Add code format check in CI format code to pass CI check Merge pull request youki-dev#129 from duduainankai/add-format-check-ci Add format check ci Fix spec path in delete Merge pull request youki-dev#130 from duduainankai/fix-path-in-delete Fix spec path in delete Document Capabilities and refactor its drop_privileges function Merge branch 'main' of github.com:containers/youki into comment-capabilities Fix same tmp dir in freezer v2 tests Merge pull request youki-dev#133 from duduainankai/fix-tmp-dir-in-test Fix same tmp dir in freezer v2 tests Merge pull request youki-dev#131 from YJDoc2/comment-capabilities Document capabilities rs and refactor its drop_privileges function cgroupsv2 hugetlb remove regex usage from hugetlb v2 use different temp dir for hugetlbv2 tests Document Info module Update doc-draft.md Merge pull request youki-dev#136 from YJDoc2/main Document Info module Document list module Merge pull request youki-dev#135 from 0xdco/cgroups-v2-hugetlb cgroupsv2 hugetlb Comment and modify Log module: Now the default log level is set according to type of compilation debug/production Merge branch 'main' of github.com:containers/youki into main Merge pull request youki-dev#137 from YJDoc2/main Document list and logger modules Implement exec command Support calling exec multiple times Fix test failure Remove todo Add comments Merge pull request youki-dev#138 from Furisto/exec Implement exec command Add pause and resume command conflicts Merge branch 'main' into add-spec-arg final issue split into multiple files add serde defaults to fields Merge pull request youki-dev#139 from duduainankai/support-pause-resume Add pause and resume command Merge branch 'main' into add-spec-arg

Furisto added 5 commits July 1, 2021 22:21

Allow wider range of arguments for spec loading

3d509e0

Rename command

8a13f82

Provide context in case of errors during dir creation

9e7b13c

Ensure file info is captured

a46faff

Modularize create code

a387a43

Furisto force-pushed the builder branch from 959c007 to 53c9b73 Compare July 4, 2021 21:05

Furisto added 3 commits July 5, 2021 19:40

Add documentation

5146d0e

Remove tests

9be94d9

Renaming

1643dd2

Furisto force-pushed the builder branch from 5dbe55e to 1643dd2 Compare July 5, 2021 18:59

Furisto added 2 commits July 5, 2021 22:32

Fix kill cmd test failures

d2ff085

Execute doc tests

0eaa906

Furisto changed the title ~~[WIP] Modularize container creation~~ Modularize container creation Jul 5, 2021

utam0k reviewed Jul 6, 2021

View reviewed changes

Review feedback and fmt

e76360b

Furisto force-pushed the builder branch from 4c288c5 to 6ce9bf8 Compare July 6, 2021 20:06

Furisto force-pushed the builder branch from 6ce9bf8 to e76360b Compare July 8, 2021 16:24

utam0k approved these changes Jul 9, 2021

View reviewed changes

utam0k merged commit 67972fe into youki-dev:main Jul 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modularize container creation #121

Modularize container creation #121

Furisto commented Jul 2, 2021

Furisto commented Jul 2, 2021

utam0k commented Jul 3, 2021

Furisto commented Jul 5, 2021

utam0k Jul 6, 2021

utam0k Jul 6, 2021

utam0k Jul 6, 2021

utam0k Jul 6, 2021

Furisto Jul 6, 2021

utam0k Jul 6, 2021

Furisto Jul 6, 2021

utam0k Jul 6, 2021

Furisto Jul 6, 2021

Furisto commented Jul 6, 2021

utam0k commented Jul 7, 2021 •

edited

Loading

Furisto commented Jul 8, 2021

utam0k left a comment

		@@ -1,4 +1,4 @@
		use crate::command::Command;
		use crate::command::{Syscall};

Modularize container creation #121

Modularize container creation #121

Conversation

Furisto commented Jul 2, 2021

Furisto commented Jul 2, 2021

utam0k commented Jul 3, 2021

Furisto commented Jul 5, 2021

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

Furisto commented Jul 6, 2021

utam0k commented Jul 7, 2021 • edited Loading

Furisto commented Jul 8, 2021

utam0k left a comment

Choose a reason for hiding this comment

utam0k commented Jul 7, 2021 •

edited

Loading