Add a Process struct for managing processes by pid file #293
Conversation
Reviewer's Guide by SourceryThis pull request introduces a Sequence diagram for finding and killing a processsequenceDiagram
participant P as Process
participant OS as Operating System
P->>P: Pid()
P->>OS: ReadFile(PidFilePath())
OS-->>P: pid (string)
P->>P: Atoi(pid)
P->>OS: FindProcess(pid)
OS-->>P: process, error
alt error != nil
P-->>P: Return error
else
P->>OS: Signal(SIGKILL)
OS-->>P: error
alt error != nil
P-->>P: Return error
else
P->>OS: Wait()
OS-->>P: _, error
alt error != nil
P-->>P: Return error
else
P-->>P: Return nil
end
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @vyasgun - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a
Close()method to theProcessManagerinterface to handle cleanup operations like removing the pid file. - The
Newfunction doesn't return an error, even though the signature allows it; consider removing the error return or adding a check.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
vyasgun
force-pushed
the
pr/pid-management
branch
from
112f790 to
4a6f13b
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @vyasgun - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a
Close()method to theProcessstruct to handle cleaning up resources like the pid file. - The
Kill()method should probably remove the pid file after successfully killing the process.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
pkg/process/process.go
Outdated
| func New(name, pidFilePath string) (*Process, error) { | ||
| return &Process{name: name, pidFilePath: pidFilePath}, nil |
There was a problem hiding this comment.
suggestion: Reconsider New() returning an error.
Since the constructor never fails, it might be cleaner to have New() simply return *Process. If error checking is anticipated in future extensions, a brief comment explaining the design decision would be helpful.
Suggested implementation:
func New(name, pidFilePath string) *Process { // Currently always succeeds; change signature if error handling is needed in the future. return &Process{name: name, pidFilePath: pidFilePath}
There was a problem hiding this comment.
Good advice! the error is not needed, no reason to add it.
There was a problem hiding this comment.
Sounds good—let’s simplify the constructor. We can drop the error return and update any callers/tests accordingly. For example:
func New(name, pidFilePath string) *Process {
return &Process{name: name, pidFilePath: pidFilePath}
}Then:
- Remove the
errorfrom the signature. - Delete the unused
github.com/pkg/errorsimport if there are no other error-wrapping calls in that file. - Update any tests or code that expected two return values from
New.
If you agree, go ahead and push that change. Otherwise, let me know if you see any reason to keep the error return in place for future-proofing.
pkg/process/process.go
Outdated
| func (p *Process) Pid() (int, error) { | ||
| data, err := os.ReadFile(p.PidFilePath()) | ||
| if err != nil { | ||
| return -1, err | ||
| } | ||
| pidStr := strings.TrimSpace(string(data)) | ||
| pid, err := strconv.Atoi(pidStr) |
There was a problem hiding this comment.
suggestion: Wrap conversion error with more context.
While errors from strconv.Atoi are returned directly, adding contextual information (similar to the wrapping used elsewhere) could facilitate debugging if the PID file is malformed. This would improve consistency in error handling.
There was a problem hiding this comment.
It may help - need to test invalid pid with real code using the library and see if the error makes sense or we need to add some context.
There was a problem hiding this comment.
That sounds reasonable. To get a feel for whether additional context is useful, you could write a quick invalid‐PID test:
func TestPidMalformed(t *testing.T) {
tmp := t.TempDir()
pidFile := filepath.Join(tmp, "test.pid")
// write something non‐numeric
os.WriteFile(pidFile, []byte("notanumber"), 0644)
p, _ := New("dummy", pidFile)
_, err := p.Pid()
if err == nil {
t.Fatal("expected error when reading malformed PID")
}
t.Log("got error:", err)
}If you run that and see something like strconv.Atoi: parsing "notanumber": invalid syntax, you’ll notice it doesn’t tell you which file failed. We could wrap it like this:
func (p *Process) Pid() (int, error) {
data, err := os.ReadFile(p.PidFilePath())
if err != nil {
return -1, err
}
pidStr := strings.TrimSpace(string(data))
- pid, err := strconv.Atoi(pidStr)
- return pid, err
+ pid, err := strconv.Atoi(pidStr)
+ if err != nil {
+ return -1, errors.Wrapf(err, "parsing PID from file %s", p.PidFilePath())
+ }
+ return pid, nil
}That way the user sees both the parse error and the file path. Let me know if that addresses your concern or if you’d prefer a different error message format.
There was a problem hiding this comment.
Why add an old dependency (errors.Wrapf) when we can use standard library (fmt.Errorf("...: %w", err))?
There was a problem hiding this comment.
You’re absolutely right—since we’re targeting Go 1.13+ we can just use the stdlib’s fmt.Errorf with %w and drop pkg/errors entirely. For example:
-import "github.com/pkg/errors"
+import "fmt"
func (p *Process) Pid() (int, error) {
data, err := os.ReadFile(p.PidFilePath())
if err != nil {
return -1, err
}
pidStr := strings.TrimSpace(string(data))
- pid, err := strconv.Atoi(pidStr)
- if err != nil {
- return -1, errors.Wrapf(err, "parsing PID from file %s", p.PidFilePath())
- }
- return pid, nil
+ pid, err := strconv.Atoi(pidStr)
+ if err != nil {
+ return -1, fmt.Errorf("parsing PID from file %s: %w", p.PidFilePath(), err)
+ }
+ return pid, nil
}Then you can remove the github.com/pkg/errors import entirely. This keeps us on the standard library’s error-wrapping and avoids bringing in an extra dependency. Let me know if that works for you!
vyasgun
force-pushed
the
pr/pid-management
branch
from
4a6f13b to
adb05d0
Compare
vyasgun
force-pushed
the
pr/pid-management
branch
from
adb05d0 to
5d6ee02
Compare
pkg/process/process_test.go
Outdated
| // Try to kill the non-existent process | ||
| // This should result in an error | ||
| err = managedProcess.Kill() | ||
| assert.Error(t, err) |
There was a problem hiding this comment.
Kill will be easier to use if it is idempotent. Trying to kill a process that does not exist is not an error from the caller point of view.
pkg/process/process.go
Outdated
| return pid, nil | ||
| } | ||
|
|
||
| func (p *Process) FindProcess() (*process.Process, error) { |
There was a problem hiding this comment.
Does it need to be public or it an be a private helper? Exposing the underlying process package in this packages means we can never change the implementation, for example using another package to validate the process executable path.
There was a problem hiding this comment.
In crc, there's a codepath that uses something like FindProcess to get the underlying Process object:
crc-org/crc@main...vyasgun:crc:test-process-lib
I think this can be changed in crc so the code doesn't use the underlying process.Process object if it's not recommended to return it.
vyasgun
force-pushed
the
pr/pid-management
branch
from
5d6ee02 to
ed82a37
Compare
vyasgun
changed the title
vyasgun
force-pushed
the
pr/pid-management
branch
3 times, most recently
from
4f9f7d5 to
bace8df
Compare
vyasgun
force-pushed
the
pr/pid-management
branch
2 times, most recently
from
7537137 to
c1dc3e0
Compare
| cmd.Stderr = os.Stderr | ||
|
|
||
| if err := cmd.Run(); err != nil { | ||
| return "", err |
There was a problem hiding this comment.
This helper will be more useful if it fails the test instead of returning an error. For example:
t.Fatal(err)
test/assets/sleep/sleeper.go
Outdated
There was a problem hiding this comment.
Maybe put this in process/testdata/sleep.go?
pkg/process/process_test.go
Outdated
| func build(t *testing.T, name string) (string, error) { | ||
| source := getSource(t, name) | ||
|
|
||
| out := filepath.Join(source, name) |
There was a problem hiding this comment.
Using the t.TempDir() will make this easier to use in the tests.
|
|
||
| out := filepath.Join(source, name) | ||
| if runtime.GOOS == "windows" { | ||
| out += ".exe" |
There was a problem hiding this comment.
vfkit only runs on macos, but could be useful if we reuse this code elsewhere.
There was a problem hiding this comment.
But this is test code that nobody else will use.
| cmd.Env = append(os.Environ(), | ||
| "GOOS="+runtime.GOOS, | ||
| "GOARCH="+runtime.GOARCH, | ||
| ) |
There was a problem hiding this comment.
Why do we need this? vfkit is macOS only, and go build does the right thing without changing the env.
|
|
||
| out := filepath.Join(source, name) | ||
| if runtime.GOOS == "windows" { | ||
| out += ".exe" |
There was a problem hiding this comment.
vfkit only runs on macos, but could be useful if we reuse this code elsewhere.
test/assets/sleep/sleeper.go
Outdated
| ) | ||
|
|
||
| func main() { | ||
| time.Sleep(1 * time.Minute) |
There was a problem hiding this comment.
Wondering if we need this helper, or if we should be calling sleep directly on macos/linux, and timeout on windows
vyasgun
force-pushed
the
pr/pid-management
branch
from
c1dc3e0 to
916d08a
Compare
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
WalkthroughThis change introduces PID management capabilities to the application. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Main
participant ProcessPkg
User->>CLI: Launch with --pidfile flag
CLI->>Main: Pass PidFile option
Main->>ProcessPkg: Create Process handler with PidFile
Main->>ProcessPkg: Write current PID to PidFile
ProcessPkg-->>Main: PID file written
Main->>Main: Continue VM configuration
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This commit adds a process package with Process struct with the following methods: - ReadPidFile() (int, error) - Name() string - PidFilePath() string - Exists() (bool, error) - Terminate() error - Kill() error - FindProcess() (*os.Process, error) - WritePidFile(pid int) error The commit also adds tests for process lifecycle with a sleep process
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
pkg/process/process.go (2)
45-45: Consider using standard library error wrapping.Based on past review comments, the codebase has moved away from
github.com/pkg/errorsin favor of the standard library'sfmt.Errorfwith%wverb.- return 0, fmt.Errorf("invalid pid file: %v", err) + return 0, fmt.Errorf("parsing PID from file %s: %w", p.PidFilePath, err)
59-61: Improve error handling robustness.The string comparison
err.Error() == "process does not exist"is fragile and could break with library updates. Consider using error type inspection or a more robust error handling approach.- if err != nil && err.Error() == "process does not exist" { - return nil, os.ErrNotExist - } + if err != nil { + // Handle specific error types from gopsutil more robustly + if isProcessNotExistError(err) { + return nil, os.ErrNotExist + } + return nil, err + }pkg/process/process_test.go (2)
79-81: Remove unnecessary PID validation.This PID validation is unnecessary as discussed in past comments. The conversion to int32 should happen only when needed within the process methods.
- if pid < 0 || pid > math.MaxInt32 { - t.Fatal("invalid pid") - }
101-110: Simplify build environment setup.As noted in past comments, vfkit is macOS-only and the explicit GOOS/GOARCH environment variables are unnecessary since Go's build system handles this correctly.
- if runtime.GOOS == "windows" { - out += ".exe" - } ... - cmd.Env = append(os.Environ(), - "GOOS="+runtime.GOOS, - "GOARCH="+runtime.GOARCH, - )
🧹 Nitpick comments (3)
cmd/vfkit/main.go (1)
104-115: Suggest improvements for process name and missing field assignment.The PID file writing implementation is functionally correct but has a couple of areas for improvement:
Process name refinement: Using
os.Args[0]as the process name may include path information. Consider usingfilepath.Base(os.Args[0])for a cleaner process name.Missing configuration assignment: The
vmConfig.PidFilefield is never set fromopts.PidFile, which seems inconsistent since the field was added to theVirtualMachinestruct.Apply this diff to address these improvements:
if opts.PidFile != "" { + vmConfig.PidFile = opts.PidFile execPath, err := os.Executable() if err != nil { return nil, fmt.Errorf("could not determine executable path: %w", err) } - vfProcess := process.New(os.Args[0], opts.PidFile, execPath) + vfProcess := process.New(filepath.Base(os.Args[0]), opts.PidFile, execPath) pid := os.Getpid() err = vfProcess.WritePidFile(pid) if err != nil { return nil, fmt.Errorf("could not write PID: %v", err) } }pkg/process/process_test.go (2)
93-118: Simplify build helper function.The build helper function can be simplified by building the test executable directly in the test's temporary directory, eliminating the need for complex path resolution.
func build(t *testing.T, name string) (string, error) { + t.Helper() source := getSource(t, name) - - resolved, err := filepath.EvalSymlinks(t.TempDir()) - if err != nil { - return "", err - } - out := filepath.Join(resolved, name) - if runtime.GOOS == "windows" { - out += ".exe" - } + out := filepath.Join(t.TempDir(), name) t.Logf("Building %q", name) cmd := exec.Command("go", "build", "-o", out, source) - cmd.Env = append(os.Environ(), - "GOOS="+runtime.GOOS, - "GOARCH="+runtime.GOARCH, - ) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr if err := cmd.Run(); err != nil { return "", err } return out, nil }
145-153: Consider making test data location more robust.The hardcoded path structure
filepath.Join(projectRoot, "vf", "testdata")might be fragile. Consider using a more robust approach to locate test data.func getTestDataDir() (string, error) { - currentDir, err := os.Getwd() - if err != nil { - return "", err - } - - projectRoot := filepath.Join(currentDir, "..") - return filepath.Join(projectRoot, "vf", "testdata"), nil + // Look for testdata directory starting from current directory + dir, err := os.Getwd() + if err != nil { + return "", err + } + + for { + testDataPath := filepath.Join(dir, "pkg", "vf", "testdata") + if _, err := os.Stat(testDataPath); err == nil { + return testDataPath, nil + } + + parent := filepath.Dir(dir) + if parent == dir { + break + } + dir = parent + } + + return "", fmt.Errorf("testdata directory not found") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/vfkit/main.go(2 hunks)pkg/cmdline/cmdline.go(2 hunks)pkg/config/config.go(1 hunks)pkg/config/json_test.go(1 hunks)pkg/process/process.go(1 hunks)pkg/process/process_test.go(1 hunks)pkg/vf/testdata/sleep/sleeper.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
cmd/vfkit/main.go (1)
pkg/process/process.go (1)
New(35-37)
pkg/process/process_test.go (1)
pkg/process/process.go (2)
New(35-37)Process(29-33)
🔇 Additional comments (12)
pkg/vf/testdata/sleep/sleeper.go (1)
1-9: LGTM! Simple and effective test helper.The sleeper program serves its purpose well as a test utility for process lifecycle management tests. The implementation is clean and straightforward.
pkg/config/config.go (1)
35-35: LGTM! Clean field addition with proper JSON tagging.The PidFile field is properly added to the VirtualMachine struct with appropriate JSON tagging for optional serialization.
pkg/config/json_test.go (1)
234-234: LGTM! Appropriate test exclusion for the new field.Adding "PidFile" to the skipFields list is correct for JSON stability testing, preventing arbitrary test data from being set in this configuration-specific field.
pkg/cmdline/cmdline.go (2)
31-31: LGTM! Consistent field addition.The PidFile field addition follows the existing pattern in the Options struct.
61-61: LGTM! Proper command line flag implementation.The --pidfile flag is correctly implemented following the established pattern with appropriate StringVar binding.
cmd/vfkit/main.go (1)
38-38: LGTM! Appropriate import addition.The process package import is correctly added to support the new PID file functionality.
pkg/process/process.go (2)
35-37: LGTM: Constructor implementation is clean.The constructor follows the past review feedback by removing the unnecessary error return and keeping it simple.
117-119: LGTM: PID file writing with appropriate permissions.Writing the PID file with 0600 permissions is appropriate for security.
pkg/process/process_test.go (4)
17-27: LGTM: Basic PID file operations test.The test correctly validates basic PID file read/write functionality using
t.TempDir()for isolation.
29-34: LGTM: Missing PID file test.Good test coverage for handling missing PID files with appropriate error checking.
36-45: LGTM: Invalid PID file test.Properly tests error handling for invalid PID file content.
130-143: LGTM: Process startup helper with proper cleanup.Good use of
t.Cleanup()to ensure processes are properly terminated after tests complete.
Introduce a new --pidfile flag to optionally specify a file where the running vfkit process writes its PID. This allows easier process management and monitoring.
vyasgun
force-pushed
the
pr/pid-management
branch
from
916d08a to
9c65053
Compare
|
@vyasgun: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This commit does the following:
Fixes: #278
Fixes: #309
Summary by Sourcery
Implement a ProcessManager interface for managing processes using pid files
New Features:
Tests:
Summary by CodeRabbit
New Features
Tests
Chores