picod: initialize and validate workspace when --workspace is set#196
picod: initialize and validate workspace when --workspace is set#196aaradhychinche-alt wants to merge 6 commits intovolcano-sh:mainfrom
Conversation
Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
volcano-sh-bot
requested review from
LiZhenCheng9527,
YaoZengzeng and
hzxuzhonghu
|
[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 |
Summary of ChangesHello @aaradhychinche-alt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines PicoD's workspace management by introducing robust initialization, validation, and isolation mechanisms. The changes ensure that PicoD operates within clearly defined and secure boundaries, preventing unauthorized file access and maintaining process integrity. This improves the overall stability and security of the PicoD service, especially when handling multiple execution requests. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces several improvements to workspace handling in PicoD, such as initializing the workspace from the --workspace flag, ensuring file operations are constrained, and adding relevant tests. The changes to the example configuration and documentation are clear and helpful.
However, there are a couple of significant issues in the implementation:
- There is a critical copy-paste error in
pkg/picod/files.gowhich results in duplicated code. - A major concern is the use of
os.Chdirto change the process's working directory. This contradicts one of the stated goals of the PR ("Avoid changing global process working directory") and is a risky practice for server applications due to its global nature, which can lead to concurrency issues. The application should rely on constructing absolute paths from the configured workspace directory rather than changing the process's CWD.
I've left specific comments on the relevant parts of the code with suggestions for improvement. Addressing these points will improve the robustness and maintainability of the code.
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #154 by implementing workspace initialization and validation in PicoD. The changes ensure that code execution works correctly when the --workspace argument is set to a directory other than /root.
Changes:
- Initialize and validate workspace directory at startup, creating it if it doesn't exist
- Change the process working directory to the workspace to ensure relative paths resolve correctly
- Ensure per-request working directories are created automatically when specified
- Add comprehensive unit tests for workspace handling scenarios
- Update documentation and example configurations to reflect the new
/workspacedefault
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/picod/server.go | Added originalWorkingDir field and RestoreWorkingDirectory() method for test cleanup |
| pkg/picod/files.go | Enhanced setWorkspace() to create, validate, and change to workspace directory at startup |
| pkg/picod/execute.go | Added automatic creation of working directories when specified in execution requests |
| pkg/picod/picod_test.go | Added proper cleanup for working directory changes in tests and new test cases for workspace behavior |
| example/code-interpreter/code-interpreter.yaml | Updated to use /workspace with an emptyDir volume mount instead of /root |
| docs/getting-started.md | Added note explaining automatic workspace directory creation and working directory changes |
pkg/picod/files.go
Outdated
| if !stat.IsDir() { | ||
| klog.Fatalf("workspace path %q is not a directory", absDir) | ||
| } | ||
|
|
There was a problem hiding this comment.
Line 425 appears to have trailing whitespace. Remove any trailing spaces or tabs at the end of this line.
pkg/picod/files.go
Outdated
| // Set workspace directory | ||
|
|
||
| // Set workspace directory | ||
| s.workspaceDir = absDir | ||
|
|
||
| // Change process working directory to workspace | ||
| if err := os.Chdir(absDir); err != nil { | ||
| klog.Fatalf("failed to change working directory to %q: %v", absDir, err) | ||
| } | ||
|
|
||
| klog.Infof("workspace directory initialized and working directory changed to: %q", s.workspaceDir) | ||
|
|
||
| // Change process working directory to workspace | ||
| if err := os.Chdir(absDir); err != nil { | ||
| klog.Fatalf("failed to change working directory to %q: %v", absDir, err) | ||
| } | ||
|
|
||
| klog.Infof("workspace directory initialized and working directory changed to: %q", s.workspaceDir) |
There was a problem hiding this comment.
This code is duplicated. Lines 426-429 set s.workspaceDir = absDir, and lines 431-436 call os.Chdir(absDir) and log a message. These same operations are then repeated on lines 438-443. Remove the duplicate code block (lines 438-443) and the duplicate comment on line 428.
|
|
||
|
|
||
| // Save original working directory before changing it (only once) | ||
| if s.originalWorkingDir == "" { | ||
| cwd, err := os.Getwd() | ||
| if err != nil { | ||
| klog.Warningf("failed to get current working directory: %v", err) | ||
| } else { | ||
| s.originalWorkingDir = cwd | ||
| } | ||
| } | ||
|
|
||
| // Resolve to absolute path | ||
|
|
||
| absDir, err := filepath.Abs(dir) | ||
| if err != nil { | ||
| klog.Warningf("Failed to resolve absolute path for workspace '%s': %v", dir, err) | ||
| s.workspaceDir = dir // Fallback to provided path | ||
| } else { | ||
| s.workspaceDir = absDir | ||
| klog.Infof("Resolved workspace to absolute path: %q", s.workspaceDir) | ||
| klog.Fatalf("failed to resolve absolute path for workspace %q: %v", dir, err) | ||
| } | ||
|
|
||
| // Create directory if it doesn't exist | ||
| if err := os.MkdirAll(absDir, 0755); err != nil { | ||
| klog.Fatalf("failed to create workspace directory %q: %v", absDir, err) | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Extra blank lines should be removed for consistency with the rest of the codebase. There are unnecessary blank lines at 392-393, 404-405, and 416.
pkg/picod/files.go
Outdated
| // Change process working directory to workspace | ||
| if err := os.Chdir(absDir); err != nil { | ||
| klog.Fatalf("failed to change working directory to %q: %v", absDir, err) | ||
| } |
There was a problem hiding this comment.
The PR description states "Avoid changing global process working directory" but this code explicitly changes the global process working directory using os.Chdir(absDir). If the intent is truly to avoid changing the global working directory, this implementation contradicts that goal. The originalWorkingDir field and RestoreWorkingDirectory() method suggest an attempt to make this change reversible, but the working directory is still being changed globally during normal operation. Consider whether this aligns with the intended design, or if the PR description needs to be updated to reflect that the working directory IS being changed.
pkg/picod/files.go
Outdated
|
|
||
| // Verify path exists and is a directory | ||
| stat, err := os.Stat(absDir) | ||
| if err != nil { | ||
| klog.Fatalf("failed to stat workspace directory %q: %v", absDir, err) | ||
| } | ||
| if !stat.IsDir() { | ||
| klog.Fatalf("workspace path %q is not a directory", absDir) | ||
| } | ||
|
|
There was a problem hiding this comment.
The stat verification on lines 417-424 is redundant after os.MkdirAll on line 412 successfully completes. If os.MkdirAll succeeds, the directory is guaranteed to exist and be a directory. This verification adds no value and can be safely removed.
| // Verify path exists and is a directory | |
| stat, err := os.Stat(absDir) | |
| if err != nil { | |
| klog.Fatalf("failed to stat workspace directory %q: %v", absDir, err) | |
| } | |
| if !stat.IsDir() { | |
| klog.Fatalf("workspace path %q is not a directory", absDir) | |
| } | |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #196 +/- ##
==========================================
+ Coverage 35.60% 35.76% +0.15%
==========================================
Files 29 29
Lines 2533 2572 +39
==========================================
+ Hits 902 920 +18
- Misses 1505 1519 +14
- Partials 126 133 +7
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:
|
Signed-off-by: Aaradhy Chinche <aaradhychinche@gmail.com>
| memory: "128Mi" | ||
| volumes: | ||
| - name: workspace | ||
| emptyDir: {} |
There was a problem hiding this comment.
we have frequenlty commented on this
There was a problem hiding this comment.
@hzxuzhonghu workspace=/workspace is in the CodeInterpreter example, consistent with earlier feedback. Please let me know if anything else is missing.
There was a problem hiding this comment.
Which comment?
Donot mount an emptyDir, confusing to users. Why not using a existing dir?
4 participants
This PR implements reviewer-requested improvements to PicoD workspace handling.
Changes included:
Initialize and validate workspace when --workspace is provided
Ensure all file operations are constrained within the workspace
Avoid changing global process working directory
Create per-request working directories safely
Add unit tests covering default and custom workspace behavior
Update documentation and example configuration accordingly
Notes:
All PicoD unit tests pass (go test ./pkg/picod/...)
Failures observed in router / workloadmanager / e2e tests are unrelated to PicoD changes and are consistent with existing open issues (e.g. CodeInterpreter warm pool behavior)
No functional changes were made outside PicoD scope
fixes #154