feat: add crc ssh and crc vm stats commands#5189
Open
sebrandon1 wants to merge 1 commit intocrc-org:mainfrom
Open
feat: add crc ssh and crc vm stats commands#5189sebrandon1 wants to merge 1 commit intocrc-org:mainfrom
sebrandon1 wants to merge 1 commit intocrc-org:mainfrom
Conversation
|
[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 |
|
Hi @sebrandon1. Thanks for your PR. I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
WalkthroughAdds a top-level Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as "crc CLI"
participant Client as "machine client"
participant SSH as "local ssh binary"
participant VM as "CRC VM (remote)"
User->>CLI: run `crc ssh [-- cmd]` or `crc vm stats`
CLI->>Client: newMachine() / ConnectionDetails()
Client-->>CLI: connection details (IP, port, SSHKeys)
CLI->>SSH: LookPath("ssh") and select identity file
CLI->>SSH: exec/run ssh with args
SSH->>VM: open SSH session / run remote stats script
VM-->>SSH: stdout (labeled sections / JSON)
SSH-->>CLI: exit status + output
CLI->>User: print JSON or formatted report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/crc/cmd/vm/stats.go (1)
129-137: Consider extracting machine existence check to avoid duplication.This logic duplicates
checkIfMachineMissing()fromroot.go. While thevmpackage can't directly accesscmdpackage internals, consider refactoring shared utilities to a common location to avoid divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/crc/cmd/vm/stats.go` around lines 129 - 137, The runStats function duplicates machine existence logic found in checkIfMachineMissing; extract the existence check into a shared utility (e.g., MachineExists or EnsureMachineExists) in a common package accessible to both cmd and vm, then replace the inline block in runStats (which constructs machine.NewSynchronizedMachine / client.Exists()) with a call to that new utility and return the same crcErr.VMNotExist or propagated errors to avoid divergence.cmd/crc/cmd/ssh.go (1)
49-51: Consider improving the error message to aid debugging.When no SSH key is found, listing the paths that were checked would help users troubleshoot.
💡 Suggested improvement
if sshKey == "" { - return fmt.Errorf("no SSH key found") + return fmt.Errorf("no SSH key found in: %v", connectionDetails.SSHKeys) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/crc/cmd/ssh.go` around lines 49 - 51, The error message for the SSH key absence (the check using sshKey) is too generic; update the fmt.Errorf call to include the list of file paths that were inspected so users can debug which locations were checked. Locate the logic that computes or holds the candidate paths (e.g., the variable or helper that produces the checked paths used when resolving sshKey) and change the return to something like fmt.Errorf("no SSH key found; checked paths: %v", checkedPaths) so the error carries the inspected paths alongside the existing message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/crc/cmd/ssh.go`:
- Around line 63-70: The current ssh branch uses exec.Command(...).Run() and
returns its error directly, which yields a *exec.ExitError that doesn't match
the exec.CodeExitError the caller (root.go) expects; change the post-run error
handling around cmd.Run() so that if the error is an *exec.ExitError (use
errors.As) you extract the OS exit status (via syscall.WaitStatus) and return a
k8s.io/client-go/util/exec.CodeExitError populated with that status (otherwise
return the original error); update imports to include "errors" (and syscall and
k8s exec package as needed) and reference sshArgs, exec.Command, cmd.Run(),
exec.ExitError, errors.As, syscall.WaitStatus and exec.CodeExitError in the new
handling.
- Line 72: The code calls the Unix-only syscall.Exec(sshPath, sshArgs,
os.Environ()) which fails on Windows; either restrict the file to Unix builds or
replace the syscall.Exec path with the same exec.Command approach used earlier.
Change the syscall.Exec call to construct exec.Command(sshPath, sshArgs...)
(matching how exec.Command is used in the earlier branch), set its
Stdout/Stderr/Stdin as needed, and Run/Start it and return its error, or
alternatively add a Unix build tag (e.g., // +build unix) at the top of ssh.go
to exclude the file from Windows; reference symbols: syscall.Exec, sshPath,
sshArgs, exec.Command.
---
Nitpick comments:
In `@cmd/crc/cmd/ssh.go`:
- Around line 49-51: The error message for the SSH key absence (the check using
sshKey) is too generic; update the fmt.Errorf call to include the list of file
paths that were inspected so users can debug which locations were checked.
Locate the logic that computes or holds the candidate paths (e.g., the variable
or helper that produces the checked paths used when resolving sshKey) and change
the return to something like fmt.Errorf("no SSH key found; checked paths: %v",
checkedPaths) so the error carries the inspected paths alongside the existing
message.
In `@cmd/crc/cmd/vm/stats.go`:
- Around line 129-137: The runStats function duplicates machine existence logic
found in checkIfMachineMissing; extract the existence check into a shared
utility (e.g., MachineExists or EnsureMachineExists) in a common package
accessible to both cmd and vm, then replace the inline block in runStats (which
constructs machine.NewSynchronizedMachine / client.Exists()) with a call to that
new utility and return the same crcErr.VMNotExist or propagated errors to avoid
divergence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2dbcc0e3-8146-450a-b381-37503b181d77
📒 Files selected for processing (5)
cmd/crc/cmd/root.gocmd/crc/cmd/root_test.gocmd/crc/cmd/ssh.gocmd/crc/cmd/vm/stats.gocmd/crc/cmd/vm/vm.go
Comment on lines
+63
to
+70
| if len(args) > 0 { | ||
| sshArgs = append(sshArgs, args...) | ||
| cmd := exec.Command(sshPath, sshArgs[1:]...) | ||
| cmd.Stdin = os.Stdin | ||
| cmd.Stdout = os.Stdout | ||
| cmd.Stderr = os.Stderr | ||
| return cmd.Run() | ||
| } |
There was a problem hiding this comment.
Exit code from remote command may not propagate correctly.
exec.Command.Run() returns *exec.ExitError on non-zero exit, but root.go checks for exec.CodeExitError (from k8s.io/client-go/util/exec). This type mismatch means crc ssh -- false will exit with code 1 instead of preserving the remote command's actual exit code.
🔧 Proposed fix to propagate exit codes
if len(args) > 0 {
sshArgs = append(sshArgs, args...)
cmd := exec.Command(sshPath, sshArgs[1:]...)
cmd.Stdin = os.Stdin
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
- return cmd.Run()
+ if err := cmd.Run(); err != nil {
+ var exitErr *exec.ExitError
+ if errors.As(err, &exitErr) {
+ os.Exit(exitErr.ExitCode())
+ }
+ return err
+ }
+ return nil
}Add "errors" to imports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/crc/cmd/ssh.go` around lines 63 - 70, The current ssh branch uses
exec.Command(...).Run() and returns its error directly, which yields a
*exec.ExitError that doesn't match the exec.CodeExitError the caller (root.go)
expects; change the post-run error handling around cmd.Run() so that if the
error is an *exec.ExitError (use errors.As) you extract the OS exit status (via
syscall.WaitStatus) and return a k8s.io/client-go/util/exec.CodeExitError
populated with that status (otherwise return the original error); update imports
to include "errors" (and syscall and k8s exec package as needed) and reference
sshArgs, exec.Command, cmd.Run(), exec.ExitError, errors.As, syscall.WaitStatus
and exec.CodeExitError in the new handling.
| return cmd.Run() | ||
| } | ||
|
|
||
| return syscall.Exec(sshPath, sshArgs, os.Environ()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if CRC has Windows-specific build constraints or platform checks
rg -l 'windows' --type go | head -20
rg -n 'runtime.GOOS|GOOS.*windows' --type go | head -20Repository: crc-org/crc
Length of output: 3083
🏁 Script executed:
#!/bin/bash
# Check build tags and constraints on ssh.go
echo "=== Build tags on ssh.go ==="
head -15 cmd/crc/cmd/ssh.go
echo -e "\n=== Check for Windows exclusions in CRC ==="
rg '\+build|//go:build' cmd/crc/cmd/ --type go
echo -e "\n=== Check CRC's supported platforms ==="
find . -maxdepth 2 -name 'README*' -o -name 'go.mod' | head -5Repository: crc-org/crc
Length of output: 364
🏁 Script executed:
#!/bin/bash
# Check README for Windows support
echo "=== CRC README - Platform Support Section ==="
head -100 README.md | grep -i -A5 -B5 "windows\|platform\|support"
echo -e "\n=== Full README first 150 lines ==="
head -150 README.md
echo -e "\n=== Check for Windows-specific ssh handling ==="
rg -n 'windows|Windows' cmd/crc/cmd/ssh.go
rg -r 'func.*ssh|Windows.*ssh' cmd/crc/cmd/ --type goRepository: crc-org/crc
Length of output: 3379
🏁 Script executed:
#!/bin/bash
# Check how ssh command is used and if there's Windows handling
echo "=== Full ssh.go file ==="
cat cmd/crc/cmd/ssh.go
echo -e "\n=== Check test coverage for ssh.go ==="
find . -name '*ssh*test*.go' -o -name '*test*ssh*.go' | grep -v vendor | head -10
echo -e "\n=== Search for Exec usage in codebase ==="
rg 'syscall\.Exec|exec\.Exec' --type go | grep -v vendor | head -20Repository: crc-org/crc
Length of output: 1795
🏁 Script executed:
#!/bin/bash
# Check if syscall.Exec is available on Windows
echo "=== Check syscall.Exec availability on Windows ==="
rg 'func Exec' tools/vendor/golang.org/x/sys/windows/ --type go
echo -e "\n=== Check Go's syscall package for Exec on Windows ==="
python3 << 'EOF'
# Check if the code would compile on Windows
# syscall.Exec is only available on Unix-like systems
# On Windows, the syscall package doesn't provide Exec
print("syscall.Exec is a Unix-specific function.")
print("It is NOT available in the Windows version of the syscall package.")
print("On Windows, attempting to call syscall.Exec would result in:")
print("- Compilation error: undefined: syscall.Exec")
print("or")
print("- Build failure when cross-compiling for windows/amd64")
EOF
echo -e "\n=== Check if CRC has any Windows-specific build handling ==="
rg 'build windows|GOOS.*windows' cmd/crc/cmd/ --type go
echo -e "\n=== Check if there are any platform-specific alternatives ==="
rg -B3 -A3 'syscall.Exec' cmd/crc/cmd/ssh.goRepository: crc-org/crc
Length of output: 970
🏁 Script executed:
#!/bin/bash
# Let's verify the compilation issue - check if ssh.go should have build tags
echo "=== Check first line of ssh.go to confirm no build tags ==="
head -1 cmd/crc/cmd/ssh.go
echo -e "\n=== Check other cmd files for build tag patterns ==="
head -1 cmd/crc/cmd/*.go | grep -B1 'build'
echo -e "\n=== Verify: Does syscall.Exec compilation fail on Windows? ==="
python3 << 'EOF'
# Check if Go compiler recognizes syscall.Exec on Windows
# This is a fact about Go's standard library
print("FACT: syscall.Exec is ONLY defined for Unix-like systems.")
print("Go's syscall package uses build tags internally:")
print(" - syscall.Exec is in syscall/exec_unix.go (with // +build unix)")
print(" - It is NOT available in Windows builds")
print("")
print("THEREFORE: Compiling ssh.go on Windows WILL result in:")
print(" 'undefined: syscall.Exec'")
print("")
print("SOLUTION: ssh.go needs a build tag like '// +build !windows'")
print("or use exec.Command for all paths (already used for args > 0 case)")
EOF
echo -e "\n=== Check if Windows CI actually runs ssh command tests ==="
find . -name '.github' -type d | head -2Repository: crc-org/crc
Length of output: 743
🏁 Script executed:
#!/bin/bash
# Check the Windows CI workflow to see what it actually builds
echo "=== Check Windows CI workflow ==="
cat .github/workflows/make-check-win.yml | head -50
echo -e "\n=== Check what make-check does on Windows ==="
grep -r 'ssh' .github/workflows/ | grep -i windows
echo -e "\n=== Check if cmd/crc/cmd/ssh.go is excluded from Windows builds ==="
rg 'ssh.go' Makefile build.sh .github/ --type text 2>/dev/null || echo "No references found in build files"Repository: crc-org/crc
Length of output: 903
🏁 Script executed:
#!/bin/bash
# Check if make check actually compiles the full binary on Windows
echo "=== Check Makefile to see what 'make check' does ==="
grep -A 20 'check:' Makefile | head -30
echo -e "\n=== Look for any conditional compilation logic for ssh.go ==="
cat .golangci.yml | grep -A5 -B5 'skip-files\|exclude-files' 2>/dev/null || echo "No file exclusions in linter config"
echo -e "\n=== Check if ssh command is tested anywhere ==="
rg 'sshCmd|runSSH' cmd/crc/cmd/ --type goRepository: crc-org/crc
Length of output: 1543
🏁 Script executed:
#!/bin/bash
# Check what the 'cross' target actually does
echo "=== Check 'cross' build target in Makefile ==="
rg -A 20 '^cross:' Makefile
echo -e "\n=== Check if Windows build target is actually invoked ==="
rg 'windows.*crc\.exe|crc\.exe.*windows' Makefile | head -10
echo -e "\n=== Search for any build script that might handle Windows differently ==="
ls -la scripts/ build* 2>/dev/null | head -20
echo -e "\n=== Final verification: Does the Windows build actually compile ssh.go? ==="
python3 << 'EOF'
print("ANALYSIS:")
print("=" * 60)
print("")
print("1. ssh.go has NO build tags (// +build unix or // +build !windows)")
print("2. Makefile has Windows build target: $(BUILD_DIR)/windows-amd64/crc.exe")
print("3. ssh.go contains: syscall.Exec(sshPath, sshArgs, os.Environ())")
print("4. syscall.Exec is ONLY defined for Unix in Go stdlib")
print("")
print("CONCLUSION:")
print("- If Windows target is compiled, it MUST fail on syscall.Exec")
print("- The original review comment is CORRECT")
print("- This is a CRITICAL BUG: code cannot compile for Windows")
print("")
print("However, code also shows exec.Command() usage for the case")
print("when args > 0, which works on all platforms.")
print("The fix is to either:")
print("1. Add // +build !windows to ssh.go, OR")
print("2. Replace syscall.Exec with exec.Command for all paths")
EOFRepository: crc-org/crc
Length of output: 2469
🏁 Script executed:
#!/bin/bash
# One final check: Let's verify if maybe there's special handling we missed
echo "=== Check if ssh.go actually gets compiled by looking at go build output ==="
rg -A 5 -B 5 'undefined.*Exec' . 2>/dev/null || echo "No existing build errors found in repo"
echo -e "\n=== Verify the exact issue with syscall.Exec on Windows ==="
python3 << 'EOF'
# Check Go documentation/source
# syscall.Exec signature and availability
print("VERIFICATION COMPLETE:")
print("=" * 70)
print("")
print("Issue: syscall.Exec is not available on Windows")
print("")
print("Evidence:")
print("1. Makefile includes Windows build: $(BUILD_DIR)/windows-amd64/crc.exe")
print("2. 'make cross' target builds for: macos-arm64, macos-amd64,")
print(" linux-amd64, linux-arm64, AND windows-amd64")
print("3. ssh.go line 72 uses: syscall.Exec(sshPath, sshArgs, os.Environ())")
print("4. ssh.go has NO build tags (no // +build !windows)")
print("5. Go's syscall package only defines Exec for Unix platforms")
print("")
print("Impact: Windows binary CANNOT be built with current code")
print("This is a COMPILATION ERROR, not a runtime error")
print("")
print("Current code already uses exec.Command for conditional branch,")
print("showing that cross-platform approach is possible.")
EOFRepository: crc-org/crc
Length of output: 7224
Fix Windows build failure: syscall.Exec is undefined on Windows.
syscall.Exec is Unix-specific and will cause a compilation error when building for Windows. The Makefile targets Windows ($(BUILD_DIR)/windows-amd64/crc.exe), but ssh.go lacks a build constraint. The code already uses exec.Command successfully in the conditional branch (lines 62-66); apply the same approach for the syscall.Exec path or add // +build unix to exclude ssh.go from Windows builds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/crc/cmd/ssh.go` at line 72, The code calls the Unix-only
syscall.Exec(sshPath, sshArgs, os.Environ()) which fails on Windows; either
restrict the file to Unix builds or replace the syscall.Exec path with the same
exec.Command approach used earlier. Change the syscall.Exec call to construct
exec.Command(sshPath, sshArgs...) (matching how exec.Command is used in the
earlier branch), set its Stdout/Stderr/Stdin as needed, and Run/Start it and
return its error, or alternatively add a Unix build tag (e.g., // +build unix)
at the top of ssh.go to exclude the file from Windows; reference symbols:
syscall.Exec, sshPath, sshArgs, exec.Command.
Member
|
@sebrandon1 we had an old issue #2079 about having About the I am putting this PR on hold as of now and will discuss with team to see if we need something around it. /hold |
sebrandon1
force-pushed
the
feat/ssh-and-vm-stats
branch
from
c052b74 to
48eb7dd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cmd/crc/cmd/ssh.go (2)
72-72:⚠️ Potential issue | 🔴 CriticalIsolate Unix-only
syscall.Execto avoid Windows build breakage.Line 72 uses
syscall.Exec, which is not available on Windows builds. This should be moved behind OS-specific files/build tags (or replaced with a cross-platform execution path).Minimal direction (delegate to OS-specific helper)
-import ( +import ( "fmt" "os" "os/exec" "strconv" - "syscall" @@ - return syscall.Exec(sshPath, sshArgs, os.Environ()) + return runInteractiveSSH(sshPath, sshArgs) }#!/bin/bash # Verify Unix-only API usage and Windows target presence. rg -nP --type=go '\bsyscall\.Exec\(' cmd/crc/cmd/ssh.go rg -nP 'windows-amd64|crc\.exe' Makefile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/crc/cmd/ssh.go` at line 72, The syscall.Exec call using sshPath and sshArgs is Unix-only and must be isolated into OS-specific files: create a helper function (e.g., execReplace(path string, args []string, env []string) error) and replace the direct syscall.Exec(sshPath, sshArgs, os.Environ()) call with execReplace(sshPath, sshArgs, os.Environ()) in the existing code; implement execReplace in ssh_unix.go with the current syscall.Exec call and a // +build !windows tag, and provide a windows stub in ssh_windows.go (or an alternative cross-platform exec flow using exec.Command.Start/Wait or return a clear error) with // +build windows so Windows builds no longer reference syscall.Exec.
63-69:⚠️ Potential issue | 🟠 MajorPreserve remote command exit status for CLI callers.
At Line 69, returning
cmd.Run()directly yields*os/exec.ExitError; this does not match thek8s.io/client-go/util/exec.CodeExitErrorhandling pattern in the caller, so remote exit codes may not be propagated correctly.Proposed fix
import ( + "errors" "fmt" "os" "os/exec" "strconv" "syscall" "github.com/spf13/cobra" + k8sexec "k8s.io/client-go/util/exec" ) @@ if len(args) > 0 { sshArgs = append(sshArgs, args...) cmd := exec.Command(sshPath, sshArgs[1:]...) cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr - return cmd.Run() + if err := cmd.Run(); err != nil { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + if ws, ok := exitErr.Sys().(syscall.WaitStatus); ok { + return k8sexec.CodeExitError{Err: err, Code: ws.ExitStatus()} + } + } + return err + } + return nil }#!/bin/bash # Verify current mismatch between returned error type and caller handling. rg -nP --type=go 'return cmd\.Run\(\)' cmd/crc/cmd/ssh.go rg -nP --type=go 'CodeExitError|ExitStatus\(' cmd/crc/cmd/root.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/crc/cmd/ssh.go` around lines 63 - 69, The current return of cmd.Run() in the SSH command path can produce an *os/exec.ExitError which the caller expects as k8s.io/client-go/util/exec.CodeExitError; change the error handling around exec.Command(sshPath, sshArgs[1:]...) so that you run cmd.Run(), check if the error is an *exec.ExitError (or has an exit code via ExitError.ExitCode()/cmd.ProcessState.ExitCode()), and if so wrap/convert it into a k8s.io/client-go/util/exec.CodeExitError with the original error and the numeric exit status before returning; for non-exit errors return them unchanged. Ensure you reference sshPath, sshArgs, cmd.Run and produce a CodeExitError on remote-command non-zero exits.
🧹 Nitpick comments (1)
cmd/crc/cmd/vm/stats.go (1)
232-239: Consider debug-level logging for parse failures.The best-effort parsing approach is reasonable for a stats command, but silently discarding conversion errors makes debugging harder when output looks wrong. Adding debug-level logging would help diagnose issues without affecting normal operation.
💡 Example for one section (apply pattern elsewhere)
if lines, ok := sections["CPU"]; ok && len(lines) >= 2 { - stats.CPU.Count, _ = strconv.Atoi(strings.TrimSpace(lines[0])) + var err error + stats.CPU.Count, err = strconv.Atoi(strings.TrimSpace(lines[0])) + if err != nil { + logging.Debugf("failed to parse CPU count: %v", err) + } fields := strings.Fields(lines[1])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/crc/cmd/vm/stats.go` around lines 232 - 239, The CPU parsing silently ignores conversion errors; capture and log parse failures at debug level so malformed input is diagnosable. Wrap the strconv.Atoi and strconv.ParseFloat calls in error checks in the CPU block (sections["CPU"], stats.CPU.Count, fields) and emit a debug log with context (section name "CPU", the offending string/field and the parse error) using the repository's debug logger; apply the same pattern to other parsing blocks so parse errors are logged but do not change the best-effort behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/crc/cmd/ssh.go`:
- Line 72: The syscall.Exec call using sshPath and sshArgs is Unix-only and must
be isolated into OS-specific files: create a helper function (e.g.,
execReplace(path string, args []string, env []string) error) and replace the
direct syscall.Exec(sshPath, sshArgs, os.Environ()) call with
execReplace(sshPath, sshArgs, os.Environ()) in the existing code; implement
execReplace in ssh_unix.go with the current syscall.Exec call and a // +build
!windows tag, and provide a windows stub in ssh_windows.go (or an alternative
cross-platform exec flow using exec.Command.Start/Wait or return a clear error)
with // +build windows so Windows builds no longer reference syscall.Exec.
- Around line 63-69: The current return of cmd.Run() in the SSH command path can
produce an *os/exec.ExitError which the caller expects as
k8s.io/client-go/util/exec.CodeExitError; change the error handling around
exec.Command(sshPath, sshArgs[1:]...) so that you run cmd.Run(), check if the
error is an *exec.ExitError (or has an exit code via
ExitError.ExitCode()/cmd.ProcessState.ExitCode()), and if so wrap/convert it
into a k8s.io/client-go/util/exec.CodeExitError with the original error and the
numeric exit status before returning; for non-exit errors return them unchanged.
Ensure you reference sshPath, sshArgs, cmd.Run and produce a CodeExitError on
remote-command non-zero exits.
---
Nitpick comments:
In `@cmd/crc/cmd/vm/stats.go`:
- Around line 232-239: The CPU parsing silently ignores conversion errors;
capture and log parse failures at debug level so malformed input is diagnosable.
Wrap the strconv.Atoi and strconv.ParseFloat calls in error checks in the CPU
block (sections["CPU"], stats.CPU.Count, fields) and emit a debug log with
context (section name "CPU", the offending string/field and the parse error)
using the repository's debug logger; apply the same pattern to other parsing
blocks so parse errors are logged but do not change the best-effort behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3653a5f5-67be-4e80-9774-f10171543ca4
📒 Files selected for processing (5)
cmd/crc/cmd/root.gocmd/crc/cmd/root_test.gocmd/crc/cmd/ssh.gocmd/crc/cmd/vm/stats.gocmd/crc/cmd/vm/vm.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/crc/cmd/root_test.go
- cmd/crc/cmd/root.go
- cmd/crc/cmd/vm/vm.go
Add direct SSH access to the CRC VM via 'crc ssh' with support for
interactive shells (syscall.Exec) and remote commands (exec.Command).
Add 'crc vm stats' to display comprehensive VM diagnostics including
OS info, CPU/memory/disk usage, network config, container stats,
service health, and OOM/fault counters. Supports -o json output
for scripting.
Usage examples:
$ crc ssh -- hostname
crc
$ crc ssh -- cat /etc/redhat-release
Red Hat Enterprise Linux release 9.6 (Plow)
$ crc ssh # opens interactive shell
$ crc vm stats
System
────────
OS: Red Hat Enterprise Linux CoreOS 9.6.20260225-1 (Plow)
Kernel: 5.14.0-570.94.1.el9_6.aarch64 (aarch64)
Uptime: 4h 0m
CPU
─────
Cores: 4
Load Avg: 2.62 (1m) 2.06 (5m) 1.81 (15m)
Memory
────────
RAM: 6.953GB / 10.93GB [############........] 64%
Swap: disabled
Disk (/sysroot)
─────────────────
Usage: 25.51GB / 36.98GB [#############.......] 69%
Free: 11.47GB
Network
─────────
Node IP: 127.0.0.1
Cluster IP: 192.168.127.2
DNS: 192.168.127.1
Workload
──────────
Pods: 66
Containers: 104
Images: 66
Top Containers (by memory)
────────────────────────────
kube-apiserver: 1.46GB
ovnkube-controller: 262.2MB
openshift-apiserver: 255.7MB
etcd: 247.4MB
kube-controller-manager: 185MB
Services
──────────
kubelet: ok
crio: ok
Health
────────
OOM Kills: 0
Major Page Faults: 258166
$ crc vm stats -o json # JSON output for scripting
sebrandon1
force-pushed
the
feat/ssh-and-vm-stats
branch
from
48eb7dd to
58b2a0c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/crc/cmd/vm/stats.go (1)
218-219: Keep JSON list fields stable for scripting.Because
statsstarts with nil slices andparseContainerStatsreturns nil on unmarshal failure,crc vm stats -o jsoncan emitnullfor list fields liketopinstead of[]. For a scripting-facing command, consistent array types are much easier to consume.Suggested patch
func parseStats(output string) (*vmStats, error) { - stats := &vmStats{} + stats := &vmStats{ + Containers: containerInfo{Top: []containerStat{}}, + Services: []serviceInfo{}, + } sections := splitSections(output) @@ func parseContainerStats(jsonData string) []containerStat { var resp crictlStatsResponse if err := json.Unmarshal([]byte(jsonData), &resp); err != nil { - return nil + return []containerStat{} }Also applies to: 288-289, 312-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/crc/cmd/vm/stats.go` around lines 218 - 219, The JSON output can contain null for list fields because vmStats and its nested container stats may have nil slices and parseContainerStats can return nil on unmarshal failure; update parseStats and where parseContainerStats is used (and the parseContainerStats implementation itself) to ensure slice fields are initialized to empty slices instead of nil. Concretely: in parseStats, after creating stats := &vmStats{} and after any unmarshalling, initialize list fields (e.g., stats.Top, stats.Containers[].Top, and any other slice fields) to empty slices if nil; modify parseContainerStats to return empty slices/empty struct slice fields on unmarshal failure rather than nil; ensure any code paths at or around the parseStats, parseContainerStats, and consumption points (the functions named parseStats and parseContainerStats and the vmStats struct and its Containers elements) normalize nil slices to [] before encoding to JSON so the CLI always emits [] for lists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/crc/cmd/vm/stats.go`:
- Around line 190-192: The NET probe emits plain positional lines (echo
"===NET===", the `ip -br addr show br-ex ...` call, and the `awk
'/^nameserver/{print $2; exit}' /etc/resolv.conf` call) so a blank output from
one command can shift/drop other metrics when later code trims empty lines and
discards sections; change the probe to emit key=value pairs for each metric
(e.g., "bridge_ip=<value>" and "dns=<value>" — emit an empty value like "dns="
when nothing found) and update the downstream trimming logic that currently
removes blank lines/discards whole sections to preserve lines with empty values
rather than deleting the whole NET section. Ensure the emitters and the consumer
use the key names consistently so blank probes no longer erase unrelated
metrics.
- Around line 129-166: The outputFormat validation is performed too late in
runStats (after creating the VM client and establishing SSH), so typos like "-o
jsno" open the VM unnecessarily; validate outputFormat (accepting "json" or
empty string) at the start of runStats and return an error for invalid values
before calling machine.NewSynchronizedMachine, client.Exists, ssh.CreateRunner,
or collectStats; update runStats to perform this early check (or factor a helper
validateOutputFormat(outputFormat)) so no VM/SSH work happens for an invalid
--output.
- Around line 25-32: The command's RunE ignores positional args so unexpected
arguments (e.g. "crc vm stats typo") are currently accepted; update the cobra
command definition (the cmd variable constructed in the stats command) to set
Args: cobra.NoArgs so Cobra validates and rejects unexpected positional
arguments before calling RunE/runStats, ensuring the command fails fast on extra
args.
---
Nitpick comments:
In `@cmd/crc/cmd/vm/stats.go`:
- Around line 218-219: The JSON output can contain null for list fields because
vmStats and its nested container stats may have nil slices and
parseContainerStats can return nil on unmarshal failure; update parseStats and
where parseContainerStats is used (and the parseContainerStats implementation
itself) to ensure slice fields are initialized to empty slices instead of nil.
Concretely: in parseStats, after creating stats := &vmStats{} and after any
unmarshalling, initialize list fields (e.g., stats.Top, stats.Containers[].Top,
and any other slice fields) to empty slices if nil; modify parseContainerStats
to return empty slices/empty struct slice fields on unmarshal failure rather
than nil; ensure any code paths at or around the parseStats,
parseContainerStats, and consumption points (the functions named parseStats and
parseContainerStats and the vmStats struct and its Containers elements)
normalize nil slices to [] before encoding to JSON so the CLI always emits []
for lists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d6a0415-d10d-4a3e-8945-84ed1f359cdf
📒 Files selected for processing (5)
cmd/crc/cmd/root.gocmd/crc/cmd/root_test.gocmd/crc/cmd/ssh.gocmd/crc/cmd/vm/stats.gocmd/crc/cmd/vm/vm.go
✅ Files skipped from review due to trivial changes (3)
- cmd/crc/cmd/root.go
- cmd/crc/cmd/root_test.go
- cmd/crc/cmd/vm/vm.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/crc/cmd/ssh.go
Comment on lines
+25
to
+32
| cmd := &cobra.Command{ | ||
| Use: "stats", | ||
| Short: "Display detailed virtual machine statistics", | ||
| Long: "Display detailed statistics about the CRC virtual machine including OS, CPU, memory, disk, network, containers, and service health.", | ||
| RunE: func(_ *cobra.Command, _ []string) error { | ||
| return runStats(cfg, outputFormat) | ||
| }, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how other CRC Cobra commands reject unexpected positional args.
# Expectation: commands that accept no args typically set `Args: cobra.NoArgs`.
rg -n --type=go 'Args:\s*cobra\.(NoArgs|ExactArgs|MaximumNArgs|MinimumNArgs)' cmd/crc/cmdRepository: crc-org/crc
Length of output: 37
🏁 Script executed:
# Check if stats.go exists and read the relevant lines
head -40 cmd/crc/cmd/vm/stats.goRepository: crc-org/crc
Length of output: 1157
🏁 Script executed:
# Look for other Cobra command definitions to understand the codebase pattern
fd -t f '\.go$' cmd/crc/cmd | head -20 | xargs -I {} sh -c 'echo "=== {} ===" && grep -A 5 "cmd := &cobra.Command" {} | head -10'Repository: crc-org/crc
Length of output: 775
🏁 Script executed:
# Count how many command definitions exist and check if any use Args
find cmd/crc/cmd -name "*.go" -type f -exec grep -l "cmd := &cobra.Command" {} \; | wc -lRepository: crc-org/crc
Length of output: 56
🏁 Script executed:
# Search more broadly for any Args usage in command definitions
rg -n "Args\s*:" cmd/crc/cmd -A 1 -B 1 | head -50Repository: crc-org/crc
Length of output: 152
🏁 Script executed:
# Search for all cobra.Command definitions to count them
rg -n "cmd := &cobra\.Command" cmd/crc/cmd --type=go | head -20Repository: crc-org/crc
Length of output: 105
🏁 Script executed:
# Check what the actual pattern is for commands in this codebase
# Look at a sample of command definitions
fd -t f '\.go$' cmd/crc/cmd | head -10 | xargs -I {} sh -c 'grep -n "&cobra.Command\|Args:" {} | head -5 && echo "---"'Repository: crc-org/crc
Length of output: 384
🏁 Script executed:
# Look specifically at how many commands define Args field
rg "&cobra\.Command\{" -A 10 cmd/crc/cmd --type=go | grep -c "Args:"Repository: crc-org/crc
Length of output: 56
Reject unexpected positional arguments.
This command ignores the args parameter in its RunE function and does not declare an Args validator, so crc vm stats typo will execute the SSH probe instead of failing fast. Add cobra.NoArgs to reject unexpected positional arguments.
Suggested patch
cmd := &cobra.Command{
Use: "stats",
Short: "Display detailed virtual machine statistics",
Long: "Display detailed statistics about the CRC virtual machine including OS, CPU, memory, disk, network, containers, and service health.",
+ Args: cobra.NoArgs,
RunE: func(_ *cobra.Command, _ []string) error {
return runStats(cfg, outputFormat)
},
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cmd := &cobra.Command{ | |
| Use: "stats", | |
| Short: "Display detailed virtual machine statistics", | |
| Long: "Display detailed statistics about the CRC virtual machine including OS, CPU, memory, disk, network, containers, and service health.", | |
| RunE: func(_ *cobra.Command, _ []string) error { | |
| return runStats(cfg, outputFormat) | |
| }, | |
| } | |
| cmd := &cobra.Command{ | |
| Use: "stats", | |
| Short: "Display detailed virtual machine statistics", | |
| Long: "Display detailed statistics about the CRC virtual machine including OS, CPU, memory, disk, network, containers, and service health.", | |
| Args: cobra.NoArgs, | |
| RunE: func(_ *cobra.Command, _ []string) error { | |
| return runStats(cfg, outputFormat) | |
| }, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/crc/cmd/vm/stats.go` around lines 25 - 32, The command's RunE ignores
positional args so unexpected arguments (e.g. "crc vm stats typo") are currently
accepted; update the cobra command definition (the cmd variable constructed in
the stats command) to set Args: cobra.NoArgs so Cobra validates and rejects
unexpected positional arguments before calling RunE/runStats, ensuring the
command fails fast on extra args.
Comment on lines
+129
to
+166
| func runStats(cfg *config.Config, outputFormat string) error { | ||
| client := machine.NewSynchronizedMachine(machine.NewClient(constants.DefaultName, logging.IsDebug(), cfg)) | ||
| exists, err := client.Exists() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !exists { | ||
| return crcErr.VMNotExist | ||
| } | ||
|
|
||
| details, err := client.ConnectionDetails() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| runner, err := ssh.CreateRunner(details.IP, details.SSHPort, details.SSHKeys...) | ||
| if err != nil { | ||
| return fmt.Errorf("cannot create SSH connection: %w", err) | ||
| } | ||
| defer runner.Close() | ||
|
|
||
| stats, err := collectStats(runner) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| stats.Network.NodeIP = details.IP | ||
|
|
||
| switch outputFormat { | ||
| case "json": | ||
| encoder := json.NewEncoder(os.Stdout) | ||
| encoder.SetIndent("", " ") | ||
| return encoder.Encode(stats) | ||
| case "": | ||
| printStats(stats) | ||
| return nil | ||
| default: | ||
| return fmt.Errorf("invalid output format: %s", outputFormat) | ||
| } |
There was a problem hiding this comment.
Validate --output before touching the VM.
Line 156 is the first time outputFormat is checked, after Line 150 has already done the SSH round-trip. A typo like -o jsno should fail immediately instead of opening the VM connection and potentially returning an unrelated SSH error.
Suggested patch
func runStats(cfg *config.Config, outputFormat string) error {
+ switch outputFormat {
+ case "", "json":
+ default:
+ return fmt.Errorf("invalid output format %q", outputFormat)
+ }
+
client := machine.NewSynchronizedMachine(machine.NewClient(constants.DefaultName, logging.IsDebug(), cfg))
exists, err := client.Exists()
if err != nil {
@@
- switch outputFormat {
- case "json":
- encoder := json.NewEncoder(os.Stdout)
- encoder.SetIndent("", " ")
- return encoder.Encode(stats)
- case "":
- printStats(stats)
- return nil
- default:
- return fmt.Errorf("invalid output format: %s", outputFormat)
- }
+ if outputFormat == "json" {
+ encoder := json.NewEncoder(os.Stdout)
+ encoder.SetIndent("", " ")
+ return encoder.Encode(stats)
+ }
+ printStats(stats)
+ return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/crc/cmd/vm/stats.go` around lines 129 - 166, The outputFormat validation
is performed too late in runStats (after creating the VM client and establishing
SSH), so typos like "-o jsno" open the VM unnecessarily; validate outputFormat
(accepting "json" or empty string) at the start of runStats and return an error
for invalid values before calling machine.NewSynchronizedMachine, client.Exists,
ssh.CreateRunner, or collectStats; update runStats to perform this early check
(or factor a helper validateOutputFormat(outputFormat)) so no VM/SSH work
happens for an invalid --output.
Comment on lines
+190
to
+192
| `echo "===NET==="`, | ||
| `ip -br addr show br-ex 2>/dev/null | awk '{print $3}' | cut -d/ -f1`, | ||
| `awk '/^nameserver/{print $2; exit}' /etc/resolv.conf`, |
There was a problem hiding this comment.
Don't let one blank probe erase the rest of the section.
Line 350 drops empty lines, so if Line 191 returns nothing, the DNS value from Line 192 becomes the only NET entry and Line 277 discards the entire section. The same position-based format is fragile anywhere one probe can legitimately emit no output. Emitting key=value pairs per field would keep unrelated metrics from disappearing when one command is blank.
Illustrative direction
- `echo "===NET==="`,
- `ip -br addr show br-ex 2>/dev/null | awk '{print $3}' | cut -d/ -f1`,
- `awk '/^nameserver/{print $2; exit}' /etc/resolv.conf`,
+ `echo "===NET==="`,
+ `printf 'cluster_ip=%s\n' "$(ip -br addr show br-ex 2>/dev/null | awk '{print $3}' | cut -d/ -f1)"`,
+ `printf 'dns_server=%s\n' "$(awk '/^nameserver/{print $2; exit}' /etc/resolv.conf)"`,- if lines, ok := sections["NET"]; ok && len(lines) >= 2 {
- stats.Network.ClusterIP = strings.TrimSpace(lines[0])
- stats.Network.DNSServer = strings.TrimSpace(lines[1])
- }
+ for _, line := range sections["NET"] {
+ key, value, ok := strings.Cut(line, "=")
+ if !ok {
+ continue
+ }
+ switch key {
+ case "cluster_ip":
+ stats.Network.ClusterIP = strings.TrimSpace(value)
+ case "dns_server":
+ stats.Network.DNSServer = strings.TrimSpace(value)
+ }
+ }Also applies to: 277-280, 344-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/crc/cmd/vm/stats.go` around lines 190 - 192, The NET probe emits plain
positional lines (echo "===NET===", the `ip -br addr show br-ex ...` call, and
the `awk '/^nameserver/{print $2; exit}' /etc/resolv.conf` call) so a blank
output from one command can shift/drop other metrics when later code trims empty
lines and discards sections; change the probe to emit key=value pairs for each
metric (e.g., "bridge_ip=<value>" and "dns=<value>" — emit an empty value like
"dns=" when nothing found) and update the downstream trimming logic that
currently removes blank lines/discards whole sections to preserve lines with
empty values rather than deleting the whole NET section. Ensure the emitters and
the consumer use the key names consistently so blank probes no longer erase
unrelated metrics.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
crc sshfor direct SSH access to the CRC VM — supports interactive shells (crc ssh) and remote commands (crc ssh -- <cmd>)crc vm statsto display comprehensive VM diagnostics with usage bars, structured sections, and-o jsonsupport for scriptingcrc vmsubcommand group as a home for future VM inspection commandsUsage examples
Implementation notes
crc sshusessyscall.Execfor interactive mode (replaces process for proper TTY handling) andexec.Commandfor remote commandscrc vm statscollects all data in a single SSH round-trip for efficiency, parsescrictl statsJSON natively in Gomachine.Clientinterface — uses existingConnectionDetails()andssh.CreateRunner()Test plan
make lintpassesgo test ./cmd/crc/cmd/...passes (man page test updated)crc sshopens interactive shellcrc ssh -- hostnameprints VM hostnamecrc vm statsdisplays all sections with correct datacrc vm stats -o jsonoutputs valid JSONSummary by CodeRabbit
New Features
Tests