SPLAT-2829: Add vm-monitor for orphaned CI VM detection and cleanup - #3
Conversation
|
@nischawl: This pull request references SPLAT-2829 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "5.0.0" version, but no target version was set. DetailsIn response to this: 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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Nutanix VM monitoring with YAML configuration, Prism Central access, TTL-based orphan detection, optional cleanup, table or JSON output, signal handling, exit codes, tests, and build support. ChangesVM monitoring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VMMonitorCLI
participant NutanixConfig
participant VMMonitor
participant PrismCentral
VMMonitorCLI->>NutanixConfig: LoadNutanix
VMMonitorCLI->>VMMonitor: Run
VMMonitor->>PrismCentral: ListCIVMs
PrismCentral-->>VMMonitor: VM metadata
VMMonitor-->>VMMonitorCLI: VMReport
VMMonitorCLI->>VMMonitor: Cleanup
VMMonitor->>PrismCentral: DeleteVM
PrismCentral-->>VMMonitor: Delete result
VMMonitor-->>VMMonitorCLI: CleanupReport
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 4
🧹 Nitpick comments (3)
cmd/vm-monitor/main.go (2)
25-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFor consideration:
formataccepts any string and silently falls back to table output for anything other than"json"(Line 76-81), so a typo like--format=jsoproduces table output with no warning. Validating the flag value and failing fast would make misconfiguration visible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/vm-monitor/main.go` at line 25, Validate the format flag in the main command flow after parsing, allowing only "table" and "json"; for any other value, report the invalid option and terminate with a non-zero status instead of silently using table output. Keep the existing output handling unchanged for valid values.
160-167: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
outputJSONexits the process directly, bypassing cleanup inmain.On encode failure,
outputJSONcallsos.Exit(1)(Line 165) instead of returning an error torun. This skipsklog.Flush()and the deferredstop()call for the signal context thatmain/runotherwise perform, so buffered log output can be lost on this failure path.Return an error from
outputJSONand letrunpropagate the failing exit code through the normal path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/vm-monitor/main.go` around lines 160 - 167, Update outputJSON to return an error from json.Encoder.Encode failures instead of logging and calling os.Exit; adjust its callers, including run, to propagate the error through the normal return path so main performs deferred stop and klog.Flush cleanup.internal/vmmonitor/monitor.go (1)
79-97: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winStop the cleanup loop early on context cancellation.
Cleanupkeeps callingm.client.DeleteVMfor every remaining orphan even afterctxis canceled (for example, by SIGTERM throughsignal.NotifyContextincmd/vm-monitor/main.go). Each subsequent call fails immediately, and its error gets recorded inreport.ResultsasDeleteStatusFailed, indistinguishable from a genuine deletion failure.Check
ctx.Err()at the top of the loop and stop attempting further deletions once the context is canceled, so the report distinguishes "not attempted due to shutdown" from "attempted and failed."♻️ Proposed early-exit guard
report.Attempted = len(orphans) for i := range orphans { + if ctx.Err() != nil { + klog.Warningf("Cleanup canceled, %d VM(s) not attempted", len(orphans)-i) + break + } vm := &orphans[i]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/vmmonitor/monitor.go` around lines 79 - 97, Update the orphan-deletion loop in Cleanup to check ctx.Err() before each iteration and break when the context is canceled. Leave already processed results unchanged and avoid calling m.client.DeleteVM or recording failed results for remaining orphans after cancellation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/vm-monitor/main.go`:
- Around line 83-99: Update the exit-code flow in the cleanup handling around
mon.Cleanup so a cleanup that removes all orphaned VMs with zero failures does
not fall through to the generic orphan-detected return. Track the successful
cleanup outcome and return the intended distinct or zero status, while
preserving the existing failure return and behavior when cleanup is not
performed.
- Around line 76-93: Move the JSON emission in the main report flow so
outputJSON(report) runs after the cleanup block assigns report.CleanupReport.
Preserve table output behavior and cleanup failure handling, ensuring JSON runs
with --cleanup, including dry-run, contains the completed cleanup_report.
In `@config/nutanix.yaml`:
- Around line 1-4: Update the default prism_central configuration to document
that insecure mode is only for the dev/staging endpoint with a self-signed
certificate, and explicitly state that production deployments must override
insecure to false.
In `@internal/config/nutanix_config.go`:
- Around line 22-25: Wire VMFilters.CategoryPattern into the VM selection path
so configured category patterns actually narrow results, updating Monitor.Run or
Client.ListCIVMs and preserving NamePrefixes filtering. Alternatively, remove
CategoryPattern from VMFilters, applyDefaults, and the sample configuration
until filtering is implemented.
---
Nitpick comments:
In `@cmd/vm-monitor/main.go`:
- Line 25: Validate the format flag in the main command flow after parsing,
allowing only "table" and "json"; for any other value, report the invalid option
and terminate with a non-zero status instead of silently using table output.
Keep the existing output handling unchanged for valid values.
- Around line 160-167: Update outputJSON to return an error from
json.Encoder.Encode failures instead of logging and calling os.Exit; adjust its
callers, including run, to propagate the error through the normal return path so
main performs deferred stop and klog.Flush cleanup.
In `@internal/vmmonitor/monitor.go`:
- Around line 79-97: Update the orphan-deletion loop in Cleanup to check
ctx.Err() before each iteration and break when the context is canceled. Leave
already processed results unchanged and avoid calling m.client.DeleteVM or
recording failed results for remaining orphans after cancellation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c058404-20c2-4e48-9c4c-13e6255224e8
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
Makefilecmd/vm-monitor/main.goconfig/nutanix.yamlgo.modinternal/config/nutanix_config.gointernal/config/nutanix_config_test.gointernal/nutanix/client.gointernal/vmmonitor/monitor.gointernal/vmmonitor/monitor_test.gointernal/vmmonitor/types.go
| prism_central: | ||
| endpoint: "prismcentral.sts2-cluster.internal.nutanix-dev.devcluster.openshift.com" | ||
| port: "9440" | ||
| insecure: true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Default config disables TLS certificate verification.
insecure: true is the value shipped in the default config path (config/nutanix.yaml is the default --config flag value in cmd/vm-monitor/main.go). This turns off certificate validation for Prism Central connections unless an operator overrides it.
If this endpoint is a dev/staging cluster with a self-signed certificate, add a comment stating that and confirm production deployments override insecure to false.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/nutanix.yaml` around lines 1 - 4, Update the default prism_central
configuration to document that insecure mode is only for the dev/staging
endpoint with a self-signed certificate, and explicitly state that production
deployments must override insecure to false.
There was a problem hiding this comment.
This is the dev Prism Central endpoint which uses a self-signed certificate. Production deployments will override insecure: false with a proper CA bundle. Added a comment to clarify.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/build.yaml (1)
46-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the exact
golangci-lintpatch version.Line 48 changes the selector from
v2.12.2tov2.12. The shorter selector can resolve a different patch release later and change CI results without a workflow change. The action supports both minor and patch selectors, so restorev2.12.2unless floating patch updates are intentional. (github.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yaml around lines 46 - 48, Update the version selector in the golangci-lint action configuration to the exact patch version v2.12.2 instead of the floating minor selector v2.12, while leaving the action and surrounding workflow unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/build.yaml:
- Around line 46-48: Update the version selector in the golangci-lint action
configuration to the exact patch version v2.12.2 instead of the floating minor
selector v2.12, while leaving the action and surrounding workflow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a42e8d4b-4824-4957-af6c-5be8ea5f42c4
📒 Files selected for processing (5)
.github/workflows/build.yamlcmd/vm-monitor/main.goconfig/nutanix.yamlinternal/config/nutanix_config.gointernal/config/nutanix_config_test.go
💤 Files with no reviewable changes (2)
- internal/config/nutanix_config_test.go
- config/nutanix.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/vm-monitor/main.go
|
/retest |
1 similar comment
|
/retest |
|
/override ci/prow/lint |
|
@nischawl: Overrode contexts on behalf of nischawl: ci/prow/lint DetailsIn response to this:
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. |
|
/override ci/prow/unit |
|
@nischawl: Overrode contexts on behalf of nischawl: ci/prow/unit DetailsIn response to this:
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. |
3 participants
Summary by CodeRabbit