Add new exec package for host process containers by dcantah · Pull Request #1233 · microsoft/hcsshim

dcantah · 2021-11-20T03:40:28Z

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]

dcantah · 2021-11-20T03:41:12Z

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.

helsaawy · 2021-11-29T23:54:31Z

lgtm

dcantah · 2021-11-30T19:06:27Z

@ambarve PTAL

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]>

dcantah · 2021-12-16T19:14:50Z

@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]>

katiewasnothere · 2021-12-18T02:21:25Z

+		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


can we change this to

defer func() { _ = windows.CloseHandle(windows.Handle(pi.Thread)) }()

instead?

katiewasnothere · 2021-12-18T02:24:52Z

+// Either: New() -> e.Start() -> e.Wait()
+//
+// or: New() -> e.Run()


We should add something about ExitCode and Pid, etc I think

katiewasnothere

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 <[email protected]>

* 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]>

* 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]>

dcantah requested a review from a team as a code owner November 20, 2021 03:40

dcantah commented Nov 20, 2021

View reviewed changes

Comment thread internal/exec/options.go Outdated

dcantah force-pushed the exec-pkg branch 2 times, most recently from 0a2e90d to dbe2d29 Compare November 22, 2021 20:39

dcantah assigned ambarve and helsaawy Nov 23, 2021

helsaawy reviewed Nov 23, 2021

View reviewed changes

Comment thread internal/exec/exec.go Outdated

helsaawy reviewed Nov 23, 2021

View reviewed changes

Comment thread internal/exec/exec.go

helsaawy reviewed Nov 23, 2021

View reviewed changes

Comment thread internal/exec/exec.go Outdated

dcantah mentioned this pull request Nov 30, 2021

Add forked version of os related stdlib packages #1134

Closed

ambarve reviewed Dec 6, 2021

View reviewed changes

Comment thread internal/winapi/process.go Outdated

Comment thread internal/winapi/process.go Outdated

Comment thread internal/exec/exec.go

Comment thread internal/exec/exec.go Outdated

Comment thread internal/exec/exec_test.go Outdated

Comment thread internal/exec/exec_test.go

dcantah mentioned this pull request Dec 14, 2021

Add conpty (pseudo console) package #1228

Merged

dcantah added 4 commits December 16, 2021 06:36

PR feedback

46e016f

* Remove conpty comments * Terminate the process if os.FindProcess doesn't work. Signed-off-by: Daniel Canter <[email protected]>

Add pseudo console support to exec package

6861cd6

This change adds pseudo console support to the exec package. Signed-off-by: Daniel Canter <[email protected]>

Change to x/sys/windows for /internal/exec package

49f270a

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]>

dcantah force-pushed the exec-pkg branch from f24aa0e to 49f270a Compare December 16, 2021 19:02