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

Add shim command for launching hostprocess cntr processes #1443

Closed
wants to merge 1 commit into from

Conversation

dcantah
Copy link
Contributor

@dcantah dcantah commented Jun 27, 2022

Due to the rootfs for a hostprocess container being a bind mount that is
only viewable for a process currently running in the containers silo, this
poses some challenges for launching a process in the container image via
CreateProcess from outside of the container. There's no usermode mechanism
to join a job object/silo and then leave afterwards, so that isn't viable either.

How we were working around this fact today is by launching container processes
via cmd /c C:\hpc\mybinary.exe rest of the commandline. The reason this
works is because cmd exists outside of the bind mount and should always
exist, and then once cmd starts up it is able to view the bind mount as
it's running in the containers silo. We can get the same functionality
and keep full control of launching and managing the process by exposing a
new command on our shim that we can invoke instead.

This change adds in a new hidden hpc command to our shim that host process
containers will make use of. It looks for a file in its working directory
that contains all the info needed to launch and manage a container process.
This new shim instance will join the containers silo so it can successfully
see the rootfs (generally at C:\hpc) and launch processes. This also allows
us to get some backwards compatible behavior for how bind mounts are handled
on systems that bindflt isn't available. In addition to binding mounts to
the path requested, we can additionally bind them to a relative path under
the containers rootfs, which is what was done for non-bindflt machines.

In addition to this change I've added some safeguards around trying to kill
a process that has just recently exited and removed an unncessary environment
variable workaround from the initial process launch for a container.

@dcantah
Copy link
Contributor Author

dcantah commented Jun 27, 2022

@kevpar Leaving this on draft as it builds on #1437 so I'll need to rebase after that gets in, but I'd love to get early feedback if possible. Edit: Sorry will add a description also soon

@dcantah dcantah force-pushed the hpc-launchprocess-shim branch 4 times, most recently from 191ea3b to 5b9de5e Compare July 1, 2022 02:43
@dcantah dcantah marked this pull request as ready for review July 1, 2022 02:56
@dcantah dcantah requested a review from a team as a code owner July 1, 2022 02:56
@dcantah dcantah force-pushed the hpc-launchprocess-shim branch 2 times, most recently from 1fea41d to 2ac8a49 Compare July 1, 2022 13:26
@dcantah
Copy link
Contributor Author

dcantah commented Jul 1, 2022

@kevpar Okay, this is ready for review

@dcantah
Copy link
Contributor Author

dcantah commented Jul 5, 2022

@marosset @jsturtevant fyi. This should get us backwards compatibility with mounts in beta

cmd/containerd-shim-runhcs-v1/hpc.go Show resolved Hide resolved
}

// Strip the drive letter (if there is one) so we don't end up with "%CONTAINER_SANDBOX_MOUNT_POINT%"\C:\path\to\mount
func stripDriveLetter(name string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we care about pathological cases like \\?\C:\path\to\mount as well, or, worse yet, \\?\Volume{GUID}\path\?

internal/jobcontainers/jobcontainer.go Show resolved Hide resolved
internal/jobcontainers/process.go Outdated Show resolved Hide resolved
Due to the rootfs for a hostprocess container being a bind mount that is
only viewable for a process currently running in the containers silo, this
poses some challenges for launching a process in the container image via
CreateProcess from outside of the container. There's no usermode mechanism
to join a job object/silo and then leave afterwards, so that isn't viable either.

How we were working around this fact today is by launching container processes
via `cmd /c C:\hpc\mybinary.exe rest of the commandline`. The reason this
works is because cmd exists outside of the bind mount and should always
exist, and then once cmd starts up it is able to view the bind mount as
it's running in the containers silo. We can get the same functionality
and keep full control of launching and managing the process by exposing a
new command on our shim that we can invoke instead.

This change adds in a new hidden hpc command to our shim that host process
containers will make use of. It looks for a file in its working directory
that contains all the info needed to launch and manage a container process.
This new shim instance will join the containers silo so it can successfully
see the rootfs (generally at C:\hpc) and launch processes. This also allows
us to get some backwards compatible behavior for how bind mounts are handled
on systems that bindflt isn't available. In addition to binding mounts to
the path requested, we can additionally bind them to a relative path under
the containers rootfs, which is what was done for non-bindflt machines.

In addition to this change I've added some safeguards around trying to kill
a process that has just recently exited and removed an unncessary environment
variable workaround from the initial process launch for a container.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@dcantah
Copy link
Contributor Author

dcantah commented Jul 14, 2022

Turns out this actually won't work with nested paths, and my test cases were getting lucky as they built on each other (C:\foo, and C:\foo\bar). This change makes things really ugly right now also.. I think there's an easier way we can get backwards compatibility a lot easier that I'll push shortly. We can just use symlinks like we do now for mounts for backwards compat, and use bindflt for where the mounts are actually being asked to show up

@dcantah dcantah closed this Jul 14, 2022
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.

3 participants