SPLAT-2830: Add monitoring for Nutanix CI/dev cluster capacity - #6
Conversation
|
@nischawl: This pull request references SPLAT-2830 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. |
|
Warning Review limit reached
Next review available in: 32 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds Nutanix capacity monitoring with configurable thresholds, resource calculations, severity reporting, CLI output, build integration, and a four-hour periodic job. ChangesCapacity monitoring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PeriodicJob
participant CapacityMonitorCLI
participant Config
participant NutanixClient
participant NutanixAPI
PeriodicJob->>CapacityMonitorCLI: Run capacity-monitor
CapacityMonitorCLI->>Config: Load credentials and Nutanix configuration
CapacityMonitorCLI->>NutanixClient: Run monitor with configured clusters
NutanixClient->>NutanixAPI: Get cluster, host, and CI VM data
NutanixAPI-->>NutanixClient: Return capacity inputs
NutanixClient-->>CapacityMonitorCLI: Return CapacityReport
CapacityMonitorCLI-->>PeriodicJob: Print report and return status code
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 (8)
prow/capacity-monitor-periodics.yaml (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider raising the log verbosity for triage.
The binary writes the Prism request details and result counts at
kloglevel 2. The command passes no-vflag, so those lines are suppressed. When the job alerts, the log will show only the summary and the error. Adding-v=2makes the cluster, host, and VM counts visible without a re-run.♻️ Proposed change
./bin/capacity-monitor --config config/nutanix.yaml --credentials-dir /tmp/secret + -v=2🤖 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 `@prow/capacity-monitor-periodics.yaml` around lines 16 - 18, Update the capacity-monitor command in the commands block to pass the klog verbosity flag -v=2, preserving the existing config and credentials arguments so Prism request details and resource counts are emitted for triage.cmd/capacity-monitor/main.go (1)
66-67: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd an overall timeout to the context.
signal.NotifyContextreacts toSIGTERMand interrupts only. It sets no deadline. This binary runs as a four-hour periodic job. If a Prism API call hangs, the job stays blocked until the Prow-level timeout, and it produces no report and no useful log.♻️ Proposed refactor
ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer stop() + + ctx, cancel := context.WithTimeout(ctx, 10*time.Minute) + defer cancel()Import
time. Consider exposing the duration as a-timeoutflag.🤖 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/capacity-monitor/main.go` around lines 66 - 67, Update the context setup in main around signal.NotifyContext to add an overall timeout for the four-hour job, using a configurable duration such as a -timeout flag with an appropriate default. Ensure the timeout context is the one used by downstream Prism API calls and retain deferred cancellation for both signal and timeout resources.internal/config/nutanix_config_test.go (1)
89-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the equality boundary and an out-of-range case.
Both case names say
>=, but both fixtures only cover>(90 vs 80, 90 vs 85). The code rejects equality with>=, and that boundary is untested. An out-of-range value such as101is also untested.💚 Proposed additional cases
{ name: "memory warning >= critical", content: ` prism_central: endpoint: "pc.example.com" capacity_monitoring: thresholds: memory_warning_percent: 90 memory_critical_percent: 85 `, }, + { + name: "cpu warning equals critical", + content: ` +prism_central: + endpoint: "pc.example.com" +capacity_monitoring: + thresholds: + cpu_warning_percent: 80 + cpu_critical_percent: 80 +`, + }, + { + name: "cpu critical above 100", + content: ` +prism_central: + endpoint: "pc.example.com" +capacity_monitoring: + thresholds: + cpu_warning_percent: 70 + cpu_critical_percent: 101 +`, + },🤖 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/config/nutanix_config_test.go` around lines 89 - 108, Update the table-driven validation cases in the Nutanix config tests to add equality fixtures where each warning threshold equals its corresponding critical threshold, and add an out-of-range threshold fixture using a value such as 101. Keep the existing greater-than cases and assert the expected validation failure for all invalid configurations.internal/nutanix/client.go (2)
182-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared VM mapping loop.
This loop duplicates lines 79-100 in
ListCIVMs, except that it omits theCategoriesmapping. The divergence is not obvious from the function signature. A future caller that readsCategoriesfrom this method will receive an empty slice. Extract onetoVMInfo(vm)helper and use it in both methods.♻️ Proposed refactor
+func toVMInfo(vm *vmconfig.Vm) VMInfo { + info := VMInfo{ + ExtID: derefString(vm.ExtId), + Name: derefString(vm.Name), + } + if vm.CreateTime != nil { + info.CreateTime = *vm.CreateTime + } + if vm.PowerState != nil { + info.PowerState = PowerState(vm.PowerState.GetName()) + } + if vm.Cluster != nil { + info.ClusterID = derefString(vm.Cluster.ExtId) + } + for _, cat := range vm.Categories { + if cat.ExtId != nil { + info.Categories = append(info.Categories, *cat.ExtId) + } + } + return info +}Then in both
ListCIVMsandListCIVMsForCluster:results := make([]VMInfo, 0, len(vms)) for i := range vms { - vm := &vms[i] - info := VMInfo{ - ExtID: derefString(vm.ExtId), - Name: derefString(vm.Name), - } - if vm.CreateTime != nil { - info.CreateTime = *vm.CreateTime - } - if vm.PowerState != nil { - info.PowerState = PowerState(vm.PowerState.GetName()) - } - if vm.Cluster != nil { - info.ClusterID = derefString(vm.Cluster.ExtId) - } - results = append(results, info) + results = append(results, toVMInfo(&vms[i])) }Adjust the parameter type to match the element type returned by
c.converged.VMs.List.🤖 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/nutanix/client.go` around lines 182 - 199, Extract the duplicated VM-to-VMInfo mapping from ListCIVMs and ListCIVMsForCluster into a shared toVMInfo helper, using the element type returned by c.converged.VMs.List. Ensure the helper includes Categories mapping, then have both methods call it so all VM fields remain consistent.
173-174: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEscape single quotes before building the OData filter.
namePrefixandclusterUUIDare interpolated directly into the filter string. If either value contains a single quote, the filter becomes malformed or changes meaning. The result is a failed request or a silently wrong VM set, which corrupts the capacity numbers. The same pattern already exists at Line 70 inListCIVMs; apply the fix in both places.🛡️ Proposed fix
+func escapeODataLiteral(s string) string { + return strings.ReplaceAll(s, "'", "''") +} + func (c *Client) ListCIVMsForCluster(ctx context.Context, clusterUUID, namePrefix string) ([]VMInfo, error) { - filter := fmt.Sprintf("startswith(name, '%s') and cluster/extId eq '%s'", namePrefix, clusterUUID) + filter := fmt.Sprintf("startswith(name, '%s') and cluster/extId eq '%s'", + escapeODataLiteral(namePrefix), escapeODataLiteral(clusterUUID))🤖 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/nutanix/client.go` around lines 173 - 174, Escape single quotes in both namePrefix and clusterUUID before constructing OData filters in ListCIVMsForCluster and the existing ListCIVMs implementation. Reuse the established escaping approach from the corresponding filter logic so embedded quotes remain literal values and cannot alter or invalidate the query.internal/config/nutanix_config.go (1)
105-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated threshold checks into one helper.
The CPU block and the memory block apply the same three rules. A shared helper removes the duplication and keeps future resource types consistent.
♻️ Proposed refactor
func (t *CapacityThresholds) validate() error { - if t.CPUWarningPercent <= 0 || t.CPUWarningPercent > 100 { - return fmt.Errorf("cpu_warning_percent must be between 1 and 100") - } - if t.CPUCriticalPercent <= 0 || t.CPUCriticalPercent > 100 { - return fmt.Errorf("cpu_critical_percent must be between 1 and 100") - } - if t.CPUWarningPercent >= t.CPUCriticalPercent { - return fmt.Errorf("cpu_warning_percent (%d) must be less than cpu_critical_percent (%d)", - t.CPUWarningPercent, t.CPUCriticalPercent) - } - if t.MemoryWarningPercent <= 0 || t.MemoryWarningPercent > 100 { - return fmt.Errorf("memory_warning_percent must be between 1 and 100") - } - if t.MemoryCriticalPercent <= 0 || t.MemoryCriticalPercent > 100 { - return fmt.Errorf("memory_critical_percent must be between 1 and 100") - } - if t.MemoryWarningPercent >= t.MemoryCriticalPercent { - return fmt.Errorf("memory_warning_percent (%d) must be less than memory_critical_percent (%d)", - t.MemoryWarningPercent, t.MemoryCriticalPercent) - } + if err := validateThresholdPair("cpu", t.CPUWarningPercent, t.CPUCriticalPercent); err != nil { + return err + } + if err := validateThresholdPair("memory", t.MemoryWarningPercent, t.MemoryCriticalPercent); err != nil { + return err + } return nil } + +func validateThresholdPair(name string, warning, critical int) error { + if warning <= 0 || warning > 100 { + return fmt.Errorf("%s_warning_percent must be between 1 and 100", name) + } + if critical <= 0 || critical > 100 { + return fmt.Errorf("%s_critical_percent must be between 1 and 100", name) + } + if warning >= critical { + return fmt.Errorf("%s_warning_percent (%d) must be less than %s_critical_percent (%d)", + name, warning, name, critical) + } + return nil +}🤖 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/config/nutanix_config.go` around lines 105 - 125, Refactor CapacityThresholds.validate to use a shared helper for validating a warning/critical threshold pair: enforce both values are between 1 and 100, then require warning to be less than critical while preserving the existing field-specific error messages. Invoke the helper for both the CPU and memory threshold fields and return its errors unchanged.internal/capacitymonitor/monitor.go (1)
31-40: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOne cluster failure discards the whole report.
Runreturns on the firstcollectClusterCapacityerror. With several cluster UUIDs configured, a transient API error on the last cluster loses the successfully collected data for all earlier clusters. The CLI then exits 1 and reports nothing.Collect per-cluster errors and continue. Return the partial report together with the combined error.
♻️ Proposed refactor
for _, clusterUUID := range m.cfg.CapacityMonitoring.ClusterUUIDs { klog.Infof("Checking capacity for cluster %s", clusterUUID) cc, err := m.collectClusterCapacity(ctx, clusterUUID) if err != nil { - return nil, fmt.Errorf("collecting capacity for cluster %s: %w", clusterUUID, err) + errs = append(errs, fmt.Errorf("collecting capacity for cluster %s: %w", clusterUUID, err)) + continue } report.Clusters = append(report.Clusters, *cc) } - return report, nil + return report, errors.Join(errs...)Declare
var errs []errorabove the loop and importerrors. Update the caller incmd/capacity-monitor/main.goto print the partial report whenreport != nil.🤖 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/capacitymonitor/monitor.go` around lines 31 - 40, Update Run’s cluster iteration to collect each collectClusterCapacity error in an errs slice and continue processing remaining clusterUUIDs, preserving successfully collected entries in report.Clusters; after the loop, return the partial report with errors.Join(errs...) when failures occurred. Update the capacity-monitor CLI caller to print the report whenever report is non-nil before exiting based on the combined error.internal/capacitymonitor/monitor_test.go (1)
120-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
estimateVMMemoryBytesand makeMonitortestable.Two gaps:
estimateVMMemoryByteshas no test.estimateVMCPUHzhas one. Both feed the used-capacity calculation.collectClusterCapacityandRunhave no tests.Monitorholds a concrete*nutanix.Client, so no fake can be injected. Define a small interface withGetCluster,ListClusterHosts, andListCIVMsForCluster, and accept that interface inNew. The aggregation logic then becomes testable.The second point matters here because the used-capacity aggregation is the part of this package that carries the correctness risk flagged in
internal/capacitymonitor/monitor.go.💚 Proposed interface for injection
+type capacityClient interface { + GetCluster(ctx context.Context, uuid string) (*nutanix.ClusterInfo, error) + ListClusterHosts(ctx context.Context, clusterUUID string) ([]nutanix.HostInfo, error) + ListCIVMsForCluster(ctx context.Context, clusterUUID, namePrefix string) ([]nutanix.VMInfo, error) +} + type Monitor struct { - client *nutanix.Client + client capacityClient cfg *config.NutanixConfig } -func New(client *nutanix.Client, cfg *config.NutanixConfig) *Monitor { +func New(client capacityClient, cfg *config.NutanixConfig) *Monitor {
*nutanix.Clientsatisfies the interface, socmd/capacity-monitor/main.goneeds no change.🤖 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/capacitymonitor/monitor_test.go` around lines 120 - 156, Expand capacity monitor coverage by adding table-driven tests for estimateVMMemoryBytes and tests for collectClusterCapacity and Run using a fake client. Introduce a minimal client interface exposing GetCluster, ListClusterHosts, and ListCIVMsForCluster; change Monitor and New to depend on that interface instead of *nutanix.Client, while preserving the existing concrete-client compatibility in main.go.
🤖 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/capacity-monitor/main.go`:
- Around line 95-103: Update run in cmd/capacity-monitor/main.go at lines 95-103
to return a distinct exit code for HasWarning instead of sharing the operational
failure code 1; preserve the critical breach and success codes. Update
prow/capacity-monitor-periodics.yaml lines 22-28 to reword report_template so it
accurately covers both capacity threshold breaches and monitor failures reported
through the error job state.
- Around line 201-208: Update outputJSON to return the JSON encoding error
instead of calling os.Exit(1). Propagate that error through run, where the
existing deferred stop and klog.Flush can execute before handling the failure
and returning the appropriate nonzero exit status.
In `@internal/capacitymonitor/monitor.go`:
- Around line 97-127: Clamp the free-capacity calculations in buildCPUUsage and
buildMemoryUsage so freeHz and freeBytes never fall below zero when used
capacity exceeds total capacity. Use the available Go 1.21+ max support while
preserving the existing conversions and severity calculations.
- Around line 71-77: Update VMInfo and ListCIVMsForCluster in the Nutanix client
to expose and populate each VM’s actual vCPU and memory specifications, then
modify the capacity calculation in the monitor to sum those per-VM values for
powered-on CI VMs. Remove the invariant estimateVMMemoryBytes usage and avoid
recomputing host-based estimates inside the loop; preserve the existing CI VM
filtering while using real specifications for used-capacity totals.
---
Nitpick comments:
In `@cmd/capacity-monitor/main.go`:
- Around line 66-67: Update the context setup in main around
signal.NotifyContext to add an overall timeout for the four-hour job, using a
configurable duration such as a -timeout flag with an appropriate default.
Ensure the timeout context is the one used by downstream Prism API calls and
retain deferred cancellation for both signal and timeout resources.
In `@internal/capacitymonitor/monitor_test.go`:
- Around line 120-156: Expand capacity monitor coverage by adding table-driven
tests for estimateVMMemoryBytes and tests for collectClusterCapacity and Run
using a fake client. Introduce a minimal client interface exposing GetCluster,
ListClusterHosts, and ListCIVMsForCluster; change Monitor and New to depend on
that interface instead of *nutanix.Client, while preserving the existing
concrete-client compatibility in main.go.
In `@internal/capacitymonitor/monitor.go`:
- Around line 31-40: Update Run’s cluster iteration to collect each
collectClusterCapacity error in an errs slice and continue processing remaining
clusterUUIDs, preserving successfully collected entries in report.Clusters;
after the loop, return the partial report with errors.Join(errs...) when
failures occurred. Update the capacity-monitor CLI caller to print the report
whenever report is non-nil before exiting based on the combined error.
In `@internal/config/nutanix_config_test.go`:
- Around line 89-108: Update the table-driven validation cases in the Nutanix
config tests to add equality fixtures where each warning threshold equals its
corresponding critical threshold, and add an out-of-range threshold fixture
using a value such as 101. Keep the existing greater-than cases and assert the
expected validation failure for all invalid configurations.
In `@internal/config/nutanix_config.go`:
- Around line 105-125: Refactor CapacityThresholds.validate to use a shared
helper for validating a warning/critical threshold pair: enforce both values are
between 1 and 100, then require warning to be less than critical while
preserving the existing field-specific error messages. Invoke the helper for
both the CPU and memory threshold fields and return its errors unchanged.
In `@internal/nutanix/client.go`:
- Around line 182-199: Extract the duplicated VM-to-VMInfo mapping from
ListCIVMs and ListCIVMsForCluster into a shared toVMInfo helper, using the
element type returned by c.converged.VMs.List. Ensure the helper includes
Categories mapping, then have both methods call it so all VM fields remain
consistent.
- Around line 173-174: Escape single quotes in both namePrefix and clusterUUID
before constructing OData filters in ListCIVMsForCluster and the existing
ListCIVMs implementation. Reuse the established escaping approach from the
corresponding filter logic so embedded quotes remain literal values and cannot
alter or invalidate the query.
In `@prow/capacity-monitor-periodics.yaml`:
- Around line 16-18: Update the capacity-monitor command in the commands block
to pass the klog verbosity flag -v=2, preserving the existing config and
credentials arguments so Prism request details and resource counts are emitted
for triage.
🪄 Autofix
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: 4c0c93d8-281c-40aa-ad37-cb0eff51da17
📒 Files selected for processing (10)
Makefilecmd/capacity-monitor/main.goconfig/nutanix.yamlinternal/capacitymonitor/monitor.gointernal/capacitymonitor/monitor_test.gointernal/capacitymonitor/types.gointernal/config/nutanix_config.gointernal/config/nutanix_config_test.gointernal/nutanix/client.goprow/capacity-monitor-periodics.yaml
|
/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
New Features
Automation
Configuration