fix: create workspace directory at startup and default execution dir to workspace#274
fix: create workspace directory at startup and default execution dir to workspace#274sicaario wants to merge 3 commits intovolcano-sh:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes PicoD workspace initialization and execution defaults so containers started with a non-/root --workspace behave correctly and consistently.
Changes:
- Ensure the configured workspace directory is created at server startup and fail fast if initialization fails.
- Default command execution working directory to the workspace when
working_diris omitted; auto-create requested subdirectories. - Improve Router upstream URL construction by validating/parsing sandbox endpoints and distinguishing “no entrypoint” from invalid endpoint configuration.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/router/handlers.go | Make upstream URL building return/propagate parse errors; refine error handling for missing entrypoints vs invalid endpoint URLs. |
| pkg/picod/server.go | Handle setWorkspace errors and ensure workspace is initialized before serving. |
| pkg/picod/files.go | Change setWorkspace to return an error and create the workspace directory via os.MkdirAll. |
| pkg/picod/execute.go | Default execution directory to workspace; create working_dir if provided. |
| pkg/picod/server_test.go | Add coverage that non-existent workspace paths are created during NewServer. |
| pkg/picod/execute_test.go | Add coverage for defaulting execution to workspace and auto-creating subdirectories. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements automatic directory creation for workspaces and execution working directories, and refactors URL construction in the router to improve error handling. Feedback was provided regarding potential resource exhaustion and performance overhead caused by calling os.MkdirAll on every execution request.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #274 +/- ##
==========================================
+ Coverage 35.60% 43.66% +8.05%
==========================================
Files 29 30 +1
Lines 2533 2638 +105
==========================================
+ Hits 902 1152 +250
+ Misses 1505 1359 -146
- Partials 126 127 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…kspace Signed-off-by: sicaario <hrmnp8@gmail.com>
…rrors in tests Signed-off-by: sicaario <hrmnp8@gmail.com>
sicaario
force-pushed
the
fix/workspace-dir-creation
branch
from
52629d3 to
184469b
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| @@ -397,6 +435,11 @@ func (s *Server) setWorkspace(dir string) { | |||
| s.workspaceDir = absDir | |||
| klog.Infof("Resolved workspace to absolute path: %q", s.workspaceDir) | |||
| } | |||
There was a problem hiding this comment.
setWorkspace now returns an error, but if filepath.Abs(dir) fails it only logs a warning and falls back to using the provided (potentially relative) path. This can lead to creating the workspace relative to the process CWD and can break the workspace-jail assumptions in sanitizePath/mkdirSafe. Consider returning the Abs error instead of continuing with a fallback path.
There was a problem hiding this comment.
I think this is a problem
This should return the Abs error instead of continuing.
| @@ -397,6 +435,11 @@ func (s *Server) setWorkspace(dir string) { | |||
| s.workspaceDir = absDir | |||
| klog.Infof("Resolved workspace to absolute path: %q", s.workspaceDir) | |||
| } | |||
There was a problem hiding this comment.
I think this is a problem
This should return the Abs error instead of continuing.
… exists Signed-off-by: sicaario <hrmnp8@gmail.com>
What type of PR is this?
/kind bug
What this PR does / why we need it:
When picod is started with
--workspaceset to a directory other than/root(e.g.--workspace=/workspace), two problems occur:The workspace directory is never created —
setWorkspaceonly resolves the path but never callsos.MkdirAll, so if the directory doesn't exist the container starts but all file and execution operations fail immediately.When a client sends an execute request without an explicit
working_dir, the command runs in the picod process's current working directory instead of the configured workspace, causing unexpected behavior depending on how the container is launched.Changes:
setWorkspace(files.go) now callsos.MkdirAllto create the workspace directory (including any parent directories) if it does not already exist, and returns an error on failureNewServer(server.go) handles the error fromsetWorkspaceand callsklog.Fatalfif workspace initialization failsExecuteHandler(execute.go) defaultscmd.Dirtos.workspaceDirwhen noworking_diris provided in the request; also auto-creates the requested subdirectory withos.MkdirAllso users do not need to pre-create subdirectories before executing inside themWhich issue(s) this PR fixes:
Fixes #154
Special notes for your reviewer:
The
TestSanitizePathfailure visible in local macOS runs is a pre-existing issue (present onmainbefore this PR) caused by macOS/tmp→/private/tmpsymlink resolution. It does not affect Linux CI and is unrelated to these changes.Does this PR introduce a user-facing change?: