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

implement the update subcommand(partially) #536

Merged
merged 4 commits into from
Dec 16, 2021

Conversation

knight42
Copy link
Contributor

Ref #147

This PR did several things:

  • Update the version of oci-spec to the latest
  • Extract a common function create_cgroup_manager which could be used in ps and update command
  • Add the update subcommand and make it be abled to changed the pids limit in the container

@knight42
Copy link
Contributor Author

/cc @utam0k

One thing I am curious is that now we read the cgroup path from the config.json under container root path https://github.com/containers/youki/blob/cbba0bc641ae2cb3aa3f0a060676532296c5dd97/crates/youki/src/commands/ps.rs#L14, but config.json is not copied to the root dir when creating container, so the config.json is missing when I run the ps command, is this intended?

crates/youki/src/commands/mod.rs Outdated Show resolved Hide resolved
@Furisto
Copy link
Collaborator

Furisto commented Dec 13, 2021

The CI failure is due to some clippy warnings that need to be fixed

@Furisto
Copy link
Collaborator

Furisto commented Dec 13, 2021

Also the clippy warnings still need to be fixed and the pnet package seems to be specified twice in the cargo.lock file now.

@codecov-commenter
Copy link

Codecov Report

Merging #536 (7332eb4) into main (529ca67) will decrease coverage by 0.58%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #536      +/-   ##
==========================================
- Coverage   61.09%   60.51%   -0.59%     
==========================================
  Files          85       99      +14     
  Lines       12532    12643     +111     
==========================================
- Hits         7657     7651       -6     
- Misses       4875     4992     +117     

@utam0k
Copy link
Member

utam0k commented Dec 14, 2021

@knight42
Oh I forget to take care of ps command 😭
When this PR #447 merged, we stopped copying config.rs for improving performance.
Now, we use YoukiConfig instead of config.rs
https://github.com/containers/youki/blob/781f9fa49b1fc6aa2534c28fb2c97a4b5a5bc47b/crates/libcontainer/src/container/container_start.rs#L43
If you like, I will ask you to fix it. What do you think? Of course, if you are busy, I'll fix it on my end.

@knight42
Copy link
Contributor Author

@knight42 Oh I forget to take care of ps command 😭 When this PR #447 merged, we stopped copying config.rs for improving performance. Now, we use YoukiConfig instead of config.rs

@utam0k Hi! I believe this issue has been fixed in this PR based on @Furisto 's suggestion here.

@Furisto Would you like to take another look?

@Furisto
Copy link
Collaborator

Furisto commented Dec 15, 2021

@knight42 Looks good! One last thing: Can you please squash the last 4 commits into one and name it "Address review comments" or something like that? Otherwise LGTM!

@knight42
Copy link
Contributor Author

@Furisto Hi! I have already squashed the commits.

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.

Looks good. Thanks!

@Furisto
Copy link
Collaborator

Furisto commented Dec 15, 2021

@knight42 Can you resolve the merge conflicts?

Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>
Signed-off-by: Jian Zeng <anonymousknight96@gmail.com>
@knight42
Copy link
Contributor Author

@Furisto Fixed now.

@utam0k
Copy link
Member

utam0k commented Dec 16, 2021

@knight42 Awesome!

@utam0k utam0k merged commit a01252a into youki-dev:main Dec 16, 2021
@knight42 knight42 deleted the feat/add-update-cmd branch December 16, 2021 08:48
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.

4 participants