Add new exec package for host process containers - #1233
Merged
Conversation
Contributor
Author
|
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. |
Danny Canter (dcantah)
force-pushed
the
exec-pkg
branch
2 times, most recently
from
November 22, 2021 20:39
0a2e90d to
dbe2d29
Compare
Contributor
|
lgtm |
Contributor
Author
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 <dcanter@microsoft.com>
* Remove conpty comments * Terminate the process if os.FindProcess doesn't work. Signed-off-by: Daniel Canter <dcanter@microsoft.com>
This change adds pseudo console support to the exec package. Signed-off-by: Daniel Canter <dcanter@microsoft.com>
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 <dcanter@microsoft.com>
Danny Canter (dcantah)
force-pushed
the
exec-pkg
branch
from
December 16, 2021 19:02
f24aa0e to
49f270a
Compare
Contributor
Author
|
Kathryn Baldauf (@katiewasnothere) Maksim An (@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 <dcanter@microsoft.com>
| 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?
Contributor
Author
There was a problem hiding this comment.
Sure!
Contributor
Author
There was a problem hiding this comment.
Done
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
Contributor
Author
There was a problem hiding this comment.
Done!
Kathryn Baldauf (katiewasnothere)
approved these changes
Dec 18, 2021
Kathryn Baldauf (katiewasnothere)
left a comment
Kathryn Baldauf (katiewasnothere)
left a comment
There was a problem hiding this comment.
small requests otherwise lgtm
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 <dcanter@microsoft.com>
Danny Canter (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 <dcanter@microsoft.com> (cherry picked from commit 2314362) Signed-off-by: Daniel Canter <dcanter@microsoft.com>
Danny Canter (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 <dcanter@microsoft.com> (cherry picked from commit 2314362) Signed-off-by: Daniel Canter <dcanter@microsoft.com>
Prince Pereira (princepereira)
pushed a commit
to princepereira/hcsshim
that referenced
this pull request
Aug 29, 2024
* 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 <dcanter@microsoft.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 dcanter@microsoft.com