Skip to content

Conversation

@OneBlue
Copy link
Collaborator

@OneBlue OneBlue commented Dec 24, 2025

Summary of the Pull Request

PR Checklist

  • Closes: Link to issue #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@OneBlue OneBlue changed the title POC: Use dockerd as a backend Switch to dockerd Jan 6, 2026
@OneBlue OneBlue marked this pull request as ready for review January 6, 2026 02:31
@OneBlue OneBlue requested a review from a team as a code owner January 6, 2026 02:31
Copilot AI review requested due to automatic review settings January 6, 2026 02:31
Copy link
Contributor

Copilot AI left a 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 pull request switches the WSLA (WSL Application) service from using containerd/nerdctl to dockerd for container management. This represents a significant architectural change in how the service interacts with container runtimes.

Key changes:

  • Replaces containerd/nerdctl with dockerd as the container runtime
  • Introduces a new DockerHTTPClient to communicate with Docker's HTTP API
  • Refactors container and process lifecycle management to work with Docker
  • Updates cgroup mounting from cgroup v2 to cgroup v1 (multiple hierarchies)

Reviewed changes

Copilot reviewed 34 out of 34 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
wslaservice.idl Breaking API changes: Hash field type change, ContentLength parameters added, new methods (ResizeTty, Start, Inspect)
WSLAVirtualMachine.cpp/h Changed cgroup mounting from v2 to v1, added ConnectUnixSocket method
WSLASession.cpp/h Replaced containerd with dockerd, integrated DockerHTTPClient, refactored image management
DockerHTTPClient.cpp/h New HTTP client for Docker API communication with comprehensive REST operations
WSLAContainer.cpp/h Major refactoring: removed nerdctl CLI calls, uses Docker API directly, simplified state management
WSLAContainerProcess.cpp/h New class to manage container processes with I/O relaying and event tracking
ContainerEventTracker.cpp/h Changed from nerdctl events to Docker events API, updated event parsing
relay.cpp/hpp Added new relay handles: HTTPChunkBasedReadHandle, DockerIORelayHandle, RelayHandle
docker_schema.h New file with Docker API JSON schemas
WSLATests.cpp Updated tests for Docker-based implementation, added validation for new features
WSLAInit.cpp Added WSLA_UNIX_CONNECT handler for Unix socket connections
CMakeLists.txt Added Boost dependency (version 1.90.0) for HTTP/networking
packages.config Updated WslaRootfs package to 0.1.3

{
// Otherwise, the session is shutting down; terminate containerd before exiting.
process.Get().Signal(15); // SIGTERM
LOG_IF_FAILED(process.Get().Signal(15)); // SIGKILL
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The comment says 'SIGKILL' but the signal being sent is 15 (SIGTERM). The comment should be corrected to match the actual signal.

Suggested change
LOG_IF_FAILED(process.Get().Signal(15)); // SIGKILL
LOG_IF_FAILED(process.Get().Signal(15)); // SIGTERM

Copilot uses AI. Check for mistakes.
Comment on lines +390 to +410
Mount(m_initChannel, nullptr, "/sys/fs/cgroup", "tmpfs", "uid=0,gid=0,mode=0755", 0);

std::vector<const char*> cgroups = {
"cpuset",
"cpu",
"cpuacct",
"blkio",
"memory",
"devices",
"freezer",
"net_cls",
"perf_event",
"net_prio",
"hugetlb",
"pids",
"rdma"};

for (const auto* e : cgroups)
{
Mount(m_initChannel, nullptr, std::format("/sys/fs/cgroup/{}", e).c_str(), "cgroup", e, 0);
}
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The cgroup mounting has been changed from cgroup v2 (single unified hierarchy) to cgroup v1 (multiple hierarchies). This is a significant behavioral change. The comment "// TODO: re-think how GPU settings should work at the session level API" on line 412 suggests this may need further consideration. Verify that dockerd requires cgroup v1 and that all functionality (including GPU support) works correctly with this configuration.

Copilot uses AI. Check for mistakes.
Comment on lines +1057 to +1060
if (!GetOverlappedResult(Handle.Get(), &Overlapped, &bytesRead, true))
{
auto error = GetLastError();
LOG_LAST_ERROR_IF(error != ERROR_CONNECTION_ABORTED && error != ERROR_OPERATION_ABORTED);
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The error variable shadows the parameter name 'error'. This could lead to confusion and potential bugs. Consider using a different variable name.

Copilot uses AI. Check for mistakes.
PendingBuffer.insert(PendingBuffer.end(), buffer.data(), buffer.data() + consumedBytes);
advance(consumedBytes);

WI_ASSERT(PendingChunkSize >= PendingChunkSize);
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

This assertion appears to be incorrect. It's checking 'PendingChunkSize >= PendingChunkSize' which is always true. This should likely be 'consumedBytes <= PendingChunkSize' or 'PendingChunkSize >= consumedBytes'.

Suggested change
WI_ASSERT(PendingChunkSize >= PendingChunkSize);
WI_ASSERT(PendingChunkSize >= consumedBytes);

Copilot uses AI. Check for mistakes.
Comment on lines +301 to +302
HRESULT LoadImage([in] ULONG ImageHandle, [in, unique] IProgressCallback* ProgressCallback, [in] ULONGLONG ContentLength);
HRESULT ImportImage([in] ULONG ImageHandle, [in] LPCSTR ImageName, [in, unique] IProgressCallback* ProgressCallback, [in] ULONGLONG ContentLength);
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The ContentLength parameter is added to LoadImage and ImportImage methods but there's no validation that the actual content read matches this length. Consider adding validation to prevent buffer overruns or mismatched content length issues.

Copilot uses AI. Check for mistakes.
}
}

// Consumme the buffer from the socket.
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The typo 'Consumme' should be 'Consume'.

Suggested change
// Consumme the buffer from the socket.
// Consume the buffer from the socket.

Copilot uses AI. Check for mistakes.
}
}

// Feed the parser up to the end of the heaer.
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The typo 'heaer' should be 'header'.

Suggested change
// Feed the parser up to the end of the heaer.
// Feed the parser up to the end of the header.

Copilot uses AI. Check for mistakes.
NLOHMANN_DEFINE_TYPE_INTRUSIVE_WITH_DEFAULT(ErrorResponse, message);
};

struct EmtpyRequest
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The typo 'EmtpyRequest' should be 'EmptyRequest' (missing 'p').

Suggested change
struct EmtpyRequest
struct EmptyRequest

Copilot uses AI. Check for mistakes.
size_t separator = image.find(':');
THROW_HR_IF_MSG(E_INVALIDARG, separator == std::string::npos || separator >= Input.size() - 1, "Invalid image: %hs", Input.c_str());

return {image.substr(0, separator), image.substr(separator + 1)};
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

Missing validation for empty image names. The ParseImage function throws on invalid format but doesn't check if the image/repo name is empty after parsing. Consider adding validation to ensure both repo and tag are non-empty.

Suggested change
return {image.substr(0, separator), image.substr(separator + 1)};
std::string repo = image.substr(0, separator);
std::string tag = image.substr(separator + 1);
THROW_HR_IF_MSG(E_INVALIDARG, repo.empty() || tag.empty(), "Invalid image: %hs", Input.c_str());
return {std::move(repo), std::move(tag)};

Copilot uses AI. Check for mistakes.
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.

2 participants