Persist port mapping and volume information for containers across WSLA sessions#14118
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for persisting WSLA container metadata (volumes, port mappings, and TTY settings) in Docker container labels, enabling containers to be recovered with full configuration across WSLA session restarts.
Changes:
- Introduced a JSON-based metadata schema stored in Docker labels to persist container configuration (volumes, ports, TTY)
- Changed WSLA_VOLUME.HostPath API from LPCWSTR to LPCSTR for consistency with other string fields
- Refactored port mapping logic to support both creation-time and recovery-time scenarios
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/windows/wslaservice/exe/WSLAContainerMetadata.h | New metadata schema with JSON serialization for volumes, ports, and TTY settings |
| src/windows/wslaservice/exe/WSLAContainer.h | Updated type signatures to use WSLAVolumeMount and WSLAPortMapping from metadata schema |
| src/windows/wslaservice/exe/WSLAContainer.cpp | Implemented metadata serialization on Create(), deserialization on Open(), and refactored port/volume handling |
| src/windows/inc/docker_schema.h | Added Labels field to CreateContainer request for metadata storage |
| src/windows/wslaservice/inc/wslaservice.idl | Breaking API change: WSLA_VOLUME.HostPath changed from LPCWSTR to LPCSTR |
| src/windows/common/WSLAContainerLauncher.h | Updated AddVolume and Create method signatures to use std::string instead of std::wstring |
| src/windows/common/WSLAContainerLauncher.cpp | Updated implementation to handle string-based paths and added Create method |
| test/windows/WSLATests.cpp | Added comprehensive recovery test and updated existing tests for string-based path API |
OneBlue
left a comment
There was a problem hiding this comment.
Change looks good, added a couple minor comments
| // Delete the host folder to simulate volume folder being missing on recovery | ||
| cleanup.reset(); | ||
|
|
||
| // Create a new session - this should succeed even though the volume folder is gone |
There was a problem hiding this comment.
The comment is slightly misleading. The session creation succeeds not because "the volume folder is gone" but because the container was already deleted in the previous session (line 2544). When OpenContainer is called, it returns ERROR_NOT_FOUND because the Docker container no longer exists, not because of the missing volume folder. Consider updating the comment to clarify that the container was deleted before the session was created.
| // Delete the host folder to simulate volume folder being missing on recovery | |
| cleanup.reset(); | |
| // Create a new session - this should succeed even though the volume folder is gone | |
| // Delete the host folder to simulate the volume folder being missing before recovery. | |
| cleanup.reset(); | |
| // Create a new session. This succeeds because the container was deleted in the previous | |
| // session; OpenContainer below is expected to return ERROR_NOT_FOUND for the missing | |
| // container, regardless of the missing volume folder. |
benhillis
left a comment
There was a problem hiding this comment.
I think some of these copilot comments are legit.
…kvega005/WSL into user/kevinve/container-metadata
| case ContainerState::Running: | ||
| case ContainerState::Restarting: | ||
| return WSLA_CONTAINER_STATE::WslaContainerStateRunning; | ||
| case ContainerState::Paused: |
There was a problem hiding this comment.
Should Paused state be clubbed with Created instead of Exited? I believe on both those types of states one could all container Start and be able to get a running container.
There was a problem hiding this comment.
We do not really support pausing containers today and have not looked into what we need to resume a paused container, so I figured it's probably not worth handling in this PR.
There was a problem hiding this comment.
Maybe leave a TODO here to revisit. I think a container in 'Restarting' state should also likely not be considered 'Running'. Though not sure what could cause those containers to go into these states if we don't support these operations (e.g. a container with a suspended process would be in Paused state)... in which case, maybe those should be considered invalid states?
There was a problem hiding this comment.
Thats a good point, we can consider them invalid states for now.
Summary of the Pull Request
Added support to persist WSLA container metadata (port mappings, volume mounts, TTY setting) in a Docker label. This enables running containers with their full configuration when opening them from previous sessions.
Changes
New Container Metadata Schema (
docker_schema.h)Added
WSLAContainerMetadataLabelconstant for the label keyAdded
ContainerMetadatastruct with JSON schema for storing:Container Creation (
WSLAContainer.cpp)Create(), serialize metadata to JSON and store in Docker labelContainer Recovery (
WSLAContainer.cpp)Open(), read metadata from Docker labelTests (
WSLATests.cpp)Added
ContainerVolumeAndPortRecoveryFromStoragetest that:PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed