-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Initial implementation for CreateContainer() #13791
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
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…oft/WSL into user/ptrivedi/create-cont
…ptrivedi/create-cont
…crosoft/WSL into user/oneblue/create-container
| IWSLAContainer& Get(); | ||
|
|
||
| WSLA_CONTAINER_STATE State(); | ||
| ClientRunningWSLAProcess GetInitProcess(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It would be good to add a GetPid() either here or in ClientRunningWSLAProcess(). Otherwise, to get Container's entry process pid, we would be doing a RunningWSLAContainer->GetInitProcess()->Get()->GetPid(). Could do that in a followup PR, oc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PID can be retrieved today RunningWSLAContainer.Get().GetPid().
We could add a convenience method to make it a bit easier though
| RETURN_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), !socket.is_valid()); | ||
|
|
||
| *Handle = HandleToUlong(common::wslutil::DuplicateHandleToCallingProcess(socket.get())); | ||
| WSL_LOG( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this just for debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It originally was, but I'd like to keep it for now, since it helps diagnose hangs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces an initial native implementation of CreateContainer() for the WSLA (Windows Subsystem for Linux Architecture) service, enabling container creation and management through the WSL infrastructure. The implementation builds on nerdctl as the underlying container runtime and extends the WSL service with new APIs and client-side launcher classes.
Key Changes:
- Added
CreateContainer()implementation with support for basic container lifecycle operations - Introduced
--imageand--debug-shellcommand-line options for the WSLA shell - Extended IDL with new container-related APIs including
OpenSessionByName(), updated container/volume structures, and GPU flags
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| test/windows/WSLATests.cpp | Added comprehensive test for CreateContainer with multiple scenarios including environment variables, stdin/stdout, and ARM64 conditional skipping |
| src/windows/wslaservice/inc/wslaservice.idl | Updated API structures: made Executable optional, renamed ContainerHostPath to ContainerPath, changed InitProcessOptions from pointer to value, added GPU flags and OpenSessionByName method |
| src/windows/wslaservice/exe/WSLAUserSession.h/cpp | Implemented OpenSessionByName to find sessions by display name |
| src/windows/wslaservice/exe/WSLASession.h/cpp | Updated CreateContainer to use new WSLAContainer::Create factory method with proper validation and locking |
| src/windows/wslaservice/exe/WSLAProcess.cpp | Fixed OnVmTerminated to properly signal exit event and added logging for GetStdHandle |
| src/windows/wslaservice/exe/WSLAContainer.h/cpp | Implemented container creation using nerdctl run with support for stdin/tty detection, environment variables, entrypoint override, and GPU flags |
| src/windows/common/WSLClient.cpp | Added container and debug-shell support to WslaShell with unified optional process handling |
| src/windows/common/WSLAProcessLauncher.h/cpp | Made RunningWSLAProcess and ClientRunningWSLAProcess movable, removed std::move on return |
| src/windows/common/WSLAContainerLauncher.h/cpp | New launcher class for creating and managing container instances through the WSLA session |
| src/windows/common/ServiceProcessLauncher.h | Made ServiceRunningProcess movable to support container process management |
| src/windows/common/WslCoreFilesystem.cpp | Changed VHD extension validation from assertion to exception for better error handling |
| src/windows/common/wslutil.cpp | Added ERROR_INVALID_SID to common error map |
| src/windows/common/CMakeLists.txt | Added WSLAContainerLauncher source and header files |
| src/shared/inc/defs.h | Introduced DEFAULT_MOVABLE macro for declaring default move semantics |
| msipackage/package.wix.in | Added temporary wslarootfs.vhd to MSI package for x64 development |
| CMakeLists.txt | Fixed OFFICIAL_BUILD condition to evaluate boolean correctly |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| WSLA_CONTAINER_FLAG_ENABLE_GPU = 1 | ||
| } ; | ||
|
|
||
|
|
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Extra blank line between line 136 and 138. While not critical, this is inconsistent with the formatting style used elsewhere in the file (single blank line between structures).
| if (hasStdin) | ||
| { | ||
| // For now return a proper error if the caller tries to pass stdin without a TTY to prevent hangs. | ||
| THROW_WIN32_IF(ERROR_NOT_SUPPORTED, hasTty == false); |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The condition hasTty == false should use the more idiomatic !hasTty for consistency with C++ best practices.
| THROW_WIN32_IF(ERROR_NOT_SUPPORTED, hasTty == false); | |
| THROW_WIN32_IF(ERROR_NOT_SUPPORTED, !hasTty); |
test/windows/WSLATests.cpp
Outdated
| if (result.Code != expectedResult) | ||
| { | ||
| LogError( | ||
| "Comman didn't return expected code (%i). ExitCode: %i, Stdout: '%hs', Stderr: '%hs'", |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in error message: "Comman" should be "Command".
| "Comman didn't return expected code (%i). ExitCode: %i, Stdout: '%hs', Stderr: '%hs'", | |
| "Command didn't return expected code (%i). ExitCode: %i, Stdout: '%hs', Stderr: '%hs'", |
| VERIFY_ARE_EQUAL(hresult, expectedError); | ||
|
|
||
| return process; | ||
| return std::move(process); |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The std::move() is unnecessary here. When returning a local variable, the compiler will automatically apply copy elision (RVO/NRVO) or move semantics. Explicitly using std::move() can actually prevent RVO optimization and may reduce performance.
| return std::move(process); | |
| return process; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not true. This fails with:
repos\wsl\test\windows\WSLATests.cpp(452,13): error C2280: 'std::optional<wsl::windows::common::ClientRunningWSLAProcess>::optional(const std::optional<wsl::windows::common::ClientRunningWSLAProcess> &)': attempting to reference a deleted function
| LPCSTR ContainerPath; | ||
| BOOL ReadOnly; |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the IDL definition, ContainerHostPath has been renamed to ContainerPath, but the field ReadOnly was added. This is a breaking change to the API structure. If this is intentional for the new implementation, ensure that any existing usages of WSLA_VOLUME are updated accordingly.
| } | ||
|
|
||
| return process.value(); | ||
| return std::move(process.value()); |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Similarly, std::move() is unnecessary here. The compiler will automatically apply move semantics when returning a local variable.
| args.push_back(options.Name); | ||
| if (options.ShmSize > 0) | ||
| { | ||
| args.push_back("--shm-size=" + std::to_string(options.ShmSize) + 'm'); |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shm-size argument is constructed as "--shm-size=" + std::to_string(options.ShmSize) + 'm'. However, the 'm' suffix is a char literal, which will be converted to its ASCII value (109) and concatenated as a number. To append the character 'm', use a string literal: "--shm-size=" + std::to_string(options.ShmSize) + "m".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::format might be better anyway
| This file contains the definition for WSLAContainerLauncher. | ||
| --*/ | ||
| #include "WSLAprocessLauncher.h" |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This header file is missing include guards (#pragma once or traditional #ifndef/#define/#endif). Without include guards, this header may cause compilation errors if included multiple times in the same translation unit.
| endif() | ||
|
|
||
| if (NOT DEFINED OFFICIAL_BUILD AND ${TARGET_PLATFORM} STREQUAL "x64") | ||
| if (NOT OFFICIAL_BUILD AND ${TARGET_PLATFORM} STREQUAL "x64") |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition DEFINED OFFICIAL_BUILD was changed to OFFICIAL_BUILD without checking if it's defined first. If OFFICIAL_BUILD is not defined, this will cause a CMake warning or error. The correct pattern should be if (NOT DEFINED OFFICIAL_BUILD OR NOT OFFICIAL_BUILD) or keep the original if (NOT DEFINED OFFICIAL_BUILD ...) form.
|
|
||
| Microsoft::WRL::ComPtr<WSLAContainer> WSLAContainer::Create(const WSLA_CONTAINER_OPTIONS& containerOptions, WSLAVirtualMachine& parentVM) | ||
| { | ||
| // TODO: Switch to nerdctl create, and call nerdctl start in Start(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will have to look a bit more into this, but we may have to end up calling nerdctl run in Start(). nerdctl start options are very limited
Co-authored-by: Pooja Trivedi <poojatrivedi@gmail.com>
Co-authored-by: Pooja Trivedi <poojatrivedi@gmail.com>
Co-authored-by: Pooja Trivedi <poojatrivedi@gmail.com>
Co-authored-by: Pooja Trivedi <poojatrivedi@gmail.com>
ptrivedi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, approving so we can make progress and keep adding functionality to this.
Summary of the Pull Request
This change introduces an initial "native" implementation of CreateContainer(), along with --container and --debug-shell options for wsl --wsla.
A lot still needs to be done in WSLAContainer, but this is a good starting point.
This change is based on @ptrivedi's initial CreateContainer PR: #13758
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed