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.

Sign up for GitHub

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

Jump to bottom

Get the result of exec command #1018

Merged
merged 11 commits into from
Sep 20, 2022
Merged

Get the result of exec command #1018

merged 11 commits into from
Sep 20, 2022

Conversation

utam0k
Copy link
Member

@utam0k utam0k commented Jun 29, 2022 •
edited by gitpod-io bot
Loading

Fix: #953

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2022 •
edited
Loading

Codecov Report

Merging #1018 (df0c6f0) into main (3a29e2d) will decrease coverage by 0.13%.
The diff coverage is 19.60%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1018      +/-   ##
==========================================
- Coverage   69.41%   69.28%   -0.14%     
==========================================
  Files         118      118              
  Lines       12446    12474      +28     
==========================================
+ Hits         8640     8643       +3     
- Misses       3806     3831      +25

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jul 14, 2022

Hey @utam0k , sorry I haven't been much active here recently 😅 Let me know if you need any help with this!

@utam0k
Copy link
Member Author

utam0k commented Jul 19, 2022

@YJDoc2 Thanks for your help. Can I ask you to help me with this pr? I am currently having trouble getting fifo's fd to pass to the init process, and I plan to get the results of exec's command execution through fifo.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jul 20, 2022

Hey, sure, I'll take a look and see if I can do anything by this weekend :)

@YJDoc2 YJDoc2 mentioned this pull request Jul 25, 2022
Closed
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Aug 30, 2022

@utam0k should we make this ready for review and get review?
The conflict is minor and can be easily resolved

utam0k and others added 5 commits September 1, 2022 21:06
@utam0k
Wait for the terminated of the command ran by exec command.
4bafaab 
Signed-off-by: utam0k <k0ma@utam0k.jp>
@utam0k
wip.
dacc737 
Signed-off-by: utam0k <k0ma@utam0k.jp>
@utam0k
fix conflicts.
3de7458 
Signed-off-by: utam0k <k0ma@utam0k.jp>
@YJDoc2 @utam0k
Make the intermediate process wait conditionally, only for exec
aa5d9c3 
Now the intermediate process waits for the main process only
when exec is called, not when the container is created. This
prevents it from waiting for created contianers, for which
exec might not be called.
This also fixes some formatting issues
@YJDoc2 @utam0k
Remove the is_exec_builder as it is redundant, replace with !init
4f2e27d
@utam0k utam0k marked this pull request as ready for review September 1, 2022 12:15
@utam0k
Copy link
Member Author

utam0k commented Sep 4, 2022

@Furisto @yihuaf Can I ask you to review this PR ;)

yihuaf
yihuaf approved these changes Sep 6, 2022
View reviewed changes
Copy link
Collaborator

@yihuaf yihuaf left a comment

Choose a reason for hiding this comment

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

Some nits and requests for comments. Otherwise, LGTM.

crates/libcontainer/src/process/fork.rs Outdated Show resolved Hide resolved
crates/libcontainer/src/process/fork.rs Show resolved Hide resolved
crates/libcontainer/src/process/container_main_process.rs Outdated Show resolved Hide resolved
crates/libcontainer/src/container/builder_impl.rs Show resolved Hide resolved
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Sep 6, 2022

Hey @yihuaf , thanks for the review!
@utam0k all of these are from my changes, so I'll fix and open another PR at your branch

I'll make the changes as well as reply here, to keep the track.

Furisto
Furisto reviewed Sep 6, 2022
View reviewed changes
crates/libcontainer/src/process/container_intermediate_process.rs Outdated
@@ -115,7 +118,13 @@ pub fn container_intermediate_process(
.close()
.context("failed to close unused init sender")?;

Ok(())
if wait {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just return the pid here and then do the wait in container_main_process (line 36)? I would like to handle this as high up as possible and leave the lower layers untouched.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey, @Furisto that was the original way I tried to do it, but the issue with it is that the final process (container process) is child of the intermediate process, so only that can wait for it. Trying to waitpid on the final process from the main process fails, as it is not main process's child. Thus we need to make intermediate wait for the final process, and main process wait for intermediate

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand that. What I am asking is to do the wait here, that's in the main_process file, but the code in that location is executed by the intermediate process.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, ok, sorry I misunderstood earlier. Can you take a look at utam0k#116 and see if that is what you expected?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@YJDoc2 Thanks, that is what I was thinking of

@YJDoc2 YJDoc2 mentioned this pull request Sep 12, 2022
Merged
@utam0k
Merge pull request #116 from YJDoc2/exec-wait-pid
75482bb 
Move wait logic from intermediate process to main process
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Sep 12, 2022 •
edited
Loading

Hey, there's a clippy lint failing, I'll fix that after @Furisto has seen utam0k#116. That way if there are any other changes, I can make them as well.

@utam0k
Copy link
Member Author

utam0k commented Sep 20, 2022

Thanks!! @YJDoc2

@utam0k utam0k merged commit 281c0a9 into youki-dev:main Sep 20, 2022
@utam0k
Copy link
Member Author

utam0k commented Sep 20, 2022

@YJDoc2 Have you tried the integration test of containerd yet?

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Sep 20, 2022

@utam0k Unfortunately I haven't tried that yet, and am quite busy until this weekend, so probably will do it after that. Apologies for the delay. Also thanks a lot for helping with this PR! 😄

@utam0k
Copy link
Member Author

utam0k commented Sep 27, 2022

@YJDoc2 We passed 💯

cat result.txt | grep TestContainerExecNoBinaryExists
=== RUN   TestContainerExecNoBinaryExists
=== PAUSE TestContainerExecNoBinaryExists
=== CONT  TestContainerExecNoBinaryExists
--- PASS: TestContainerExecNoBinaryExists (0.27s)

@utam0k
Copy link
Member Author

utam0k commented Sep 27, 2022

hmm... but we got failed another test case TestContainerExec
https://github.com/containerd/containerd/blob/902212651b12e4d81b7bc779632b2e4da0a8e673/integration/client/container_test.go#L245

=== CONT  TestContainerExec
    container_test.go:272: failed to create shim: OCI runtime create failed: runc did not terminate successfully: exit status 1: unknown
--- FAIL: TestContainerExec (0.04s)

@YJDoc2 YJDoc2 mentioned this pull request Oct 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YJDoc2 YJDoc2 YJDoc2 left review comments

@Furisto Furisto Furisto approved these changes

@yihuaf yihuaf yihuaf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

exec command returns failure when the command doesn't exist
5 participants
@utam0k @codecov-commenter @YJDoc2 @yihuaf @Furisto