Add new exec package for host process containers #1233
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Don't be scared of the 347 changed files haha, 90% is just new files from bumping golang.org/x/sys in the main go.mod and in /test. |
dcantah
commented
Nov 20, 2021
dcantah
force-pushed
the
exec-pkg
branch
2 times, most recently
from
0a2e90d
to
dbe2d29
Compare
helsaawy
reviewed
Nov 23, 2021
helsaawy
reviewed
Nov 23, 2021
helsaawy
reviewed
Nov 23, 2021
|
lgtm |
|
@ambarve PTAL |
ambarve
reviewed
Dec 6, 2021
This change adds a new exec package thats main goal is to run external processes on Windows. Unfortunately due to a couple things that can't be accomplished with the stdlib os/exec package, this new package is meant to replace how processes for host process containers are launched. The main shortcomings are not being able to pass in a pseudo console to use for tty scenarios, and not being able to start a process assigned to a job object instead of doing the Create -> Assign dance. Both of these issue are centered around not having access to the process thread attribute list that is setup inside of syscall.StartProcess. This is needed to be able to properly setup both cases, as it requires calling UpdateProcThreadAttribute and passing in what's necessary for both scenarios. Signed-off-by: Daniel Canter <[email protected]>
* Remove conpty comments * Terminate the process if os.FindProcess doesn't work. Signed-off-by: Daniel Canter <[email protected]>
This change adds pseudo console support to the exec package. Signed-off-by: Daniel Canter <[email protected]>
x/sys/windows has the Proc thread attribute work we need already, so swap to this instead of our own definitions. Signed-off-by: Daniel Canter <[email protected]>
|
@katiewasnothere @anmaxvl Anyone able to take a look at this? |
1. Add comment regarding thread safety 2. Add check on job object test to make sure the pids slice has at least one element before testing what's in [0]. 3. Change stdioOurEnd -> stdioPipesOurSide and stdioProcEnd -> stdioPipesProcSide. 4. Misc. clarification comments on fields and methods. 5. Change from raw string literal in powershell test to string with carriage return at the end. Signed-off-by: Daniel Canter <[email protected]>
internal/exec/exec.go
Outdated
| return fmt.Errorf("failed to create process: %w", err) | ||
| } | ||
| // Don't need the thread handle for anything. | ||
| defer windows.CloseHandle(windows.Handle(pi.Thread)) //nolint:errcheck |
There was a problem hiding this comment.
can we change this to
defer func() {
_ = windows.CloseHandle(windows.Handle(pi.Thread))
}()
instead?
internal/exec/exec.go
Outdated
Comment on lines
28
to
30
| // Either: New() -> e.Start() -> e.Wait() | ||
| // | ||
| // or: New() -> e.Run() |
There was a problem hiding this comment.
We should add something about ExitCode and Pid, etc I think
katiewasnothere
approved these changes
Dec 18, 2021
1. Add nod to using e.ExitCode() in the process lifecycle comment 2. Change to different way to defer close the thread handle to avoid nolint comment Signed-off-by: Daniel Canter <[email protected]>
dcantah
added a commit
to dcantah/hcsshim
that referenced
this pull request
Jan 4, 2022
* Add new exec package for host process containers This change adds a new exec package thats main goal is to run external processes on Windows. Unfortunately due to a couple things that can't be accomplished with the stdlib os/exec package, this new package is meant to replace how processes for host process containers are launched. The main shortcomings are not being able to pass in a pseudo console to use for tty scenarios, and not being able to start a process assigned to a job object instead of doing the Create -> Assign dance. Both of these issue are centered around not having access to the process thread attribute list that is setup inside of syscall.StartProcess. This is needed to be able to properly setup both cases, as it requires calling UpdateProcThreadAttribute and passing in what's necessary for both scenarios. This change ends up bumping x/sys/windows as well to grab some fixes for the attribute list functionality. Signed-off-by: Daniel Canter <[email protected]> (cherry picked from commit 2314362) Signed-off-by: Daniel Canter <[email protected]>
dcantah
added a commit
to dcantah/hcsshim
that referenced
this pull request
Jan 4, 2022
* Add new exec package for host process containers This change adds a new exec package thats main goal is to run external processes on Windows. Unfortunately due to a couple things that can't be accomplished with the stdlib os/exec package, this new package is meant to replace how processes for host process containers are launched. The main shortcomings are not being able to pass in a pseudo console to use for tty scenarios, and not being able to start a process assigned to a job object instead of doing the Create -> Assign dance. Both of these issue are centered around not having access to the process thread attribute list that is setup inside of syscall.StartProcess. This is needed to be able to properly setup both cases, as it requires calling UpdateProcThreadAttribute and passing in what's necessary for both scenarios. This change ends up bumping x/sys/windows as well to grab some fixes for the attribute list functionality. Signed-off-by: Daniel Canter <[email protected]> (cherry picked from commit 2314362) Signed-off-by: Daniel Canter <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change adds a new exec package thats main goal is to run
external processes on Windows. Unfortunately due to a couple
things that can't be accomplished with the stdlib os/exec package,
this new package is meant to replace how processes for host process
containers are launched.
The main shortcomings for os/exec are not being able to pass in a pseudo console to use
for tty scenarios, and not being able to start a process assigned to a job object
instead of doing the Create -> Assign dance. Both of these issue are centered
around not having access to the process thread attribute list that is setup inside
of syscall.StartProcess. This is needed to be able to properly setup both cases,
as it requires calling UpdateProcThreadAttribute and passing in what's necessary
for both scenarios.
Signed-off-by: Daniel Canter [email protected]