Reduce vCenter API load for large vSphere clusters#1515
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
/test ? |
|
[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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughAdds configurable sync-period and reconciliation-concurrency flags to the vSphere controller, migrates actuator caches to concurrent storage, introduces stable-machine reconcile skipping and scope reuse, memoizes VM lookups, returns timed task requeues, and configures vSphere operator concurrency defaults. ChangesvSphere reconciliation and concurrency
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MachineController
participant Actuator
participant ScopeCache
participant vCenter
MachineController->>Actuator: Exists(machine)
Actuator->>Actuator: canSkipFullReconcile(machine)
Actuator->>vCenter: findVM()
Actuator->>ScopeCache: Store(machineUID, machineScope)
MachineController->>Actuator: Update(machine)
Actuator->>ScopeCache: getOrCreateScope(machineUID)
Actuator->>vCenter: reconcile VM
Actuator->>Actuator: markFullReconcileComplete(machine)
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/test e2e-vsphere-ovn-multi-vcenter |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/controller/vsphere/reconciler.go (1)
1498-1517: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor inconsistency: empty-string sentinel for
cachedUUID.Unlike
cachedPowerState(pointer, unambiguous "unset" state),cachedUUIDuses""as the sentinel. Ifvm.Obj.UUID()legitimately returns empty (VM not fully provisioned), caching is silently skipped every call — safe, but inconsistent with the pattern used for power state.🤖 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 `@pkg/controller/vsphere/reconciler.go` around lines 1498 - 1517, The getUUID cache in virtualMachine currently uses an empty string sentinel, which makes repeated calls re-fetch UUIDs when object UUID() legitimately returns empty. Update getUUID and the cachedUUID field to use an explicit “set/unset” pattern consistent with cachedPowerState (for example, a separate boolean or pointer-like sentinel) so a real empty UUID is still cached and returned without re-querying. Keep the change localized to virtualMachine and getUUID so the memoization behavior matches the existing caching style.cmd/vsphere/main.go (1)
38-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming
timeoutnow that its only use is as the--sync-perioddefault.The generic name risks confusion with other unrelated timeouts (context/HTTP) as the file grows.
✏️ Suggested rename
-const timeout = 60 * time.Minute +const defaultSyncPeriod = 60 * time.MinuteAnd update the reference at Line 105 accordingly.
🤖 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/vsphere/main.go` at line 38, Rename the generic timeout constant used by the vSphere CLI default so it clearly reflects that it is the default sync period rather than a request or context timeout. Update the constant in the main package and the `--sync-period` default reference in the CLI setup so both use the new name consistently.
🤖 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 `@cmd/vsphere/main.go`:
- Line 38: Rename the generic timeout constant used by the vSphere CLI default
so it clearly reflects that it is the default sync period rather than a request
or context timeout. Update the constant in the main package and the
`--sync-period` default reference in the CLI setup so both use the new name
consistently.
In `@pkg/controller/vsphere/reconciler.go`:
- Around line 1498-1517: The getUUID cache in virtualMachine currently uses an
empty string sentinel, which makes repeated calls re-fetch UUIDs when object
UUID() legitimately returns empty. Update getUUID and the cachedUUID field to
use an explicit “set/unset” pattern consistent with cachedPowerState (for
example, a separate boolean or pointer-like sentinel) so a real empty UUID is
still cached and returned without re-querying. Keep the change localized to
virtualMachine and getUUID so the memoization behavior matches the existing
caching style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fb6d1f73-584b-4e2d-8b5a-47b4eb629123
📒 Files selected for processing (12)
cmd/vsphere/main.godocs/proposals/vsphere-perf-improvements.mdpkg/controller/vsphere/actuator.gopkg/controller/vsphere/actuator_cache_test.gopkg/controller/vsphere/actuator_concurrency_test.gopkg/controller/vsphere/actuator_test.gopkg/controller/vsphere/machine_scope.gopkg/controller/vsphere/reconcile_skip_test.gopkg/controller/vsphere/reconciler.gopkg/controller/vsphere/virtualmachine_cache_test.gopkg/operator/sync.gopkg/operator/sync_test.go
Machines that are fully stable (Phase=Running, ProviderID set, NodeRef present, addresses populated, not deleting) and whose provider spec has not changed since the last full reconciliation are short-circuited in both Exists() and Update(), avoiding all vCenter API calls. A SHA-256 hash of the provider spec is stored in an annotation to detect spec changes, and an RFC3339 timestamp annotation enforces a 1-hour TTL to guarantee periodic drift detection regardless of spec changes. For a 200-machine stable cluster this eliminates ~30,000 unnecessary vCenter API calls per hour, reducing new-machine queue wait from 10+ minutes to near-zero. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change the informer resync interval from 10 minutes to 60 minutes, reducing the frequency of resync-driven reconciliation 6x. Combined with the per-machine 1-hour TTL from the short-circuit annotation, stable machines get exactly 1 full reconciliation per hour. A --sync-period flag is added for operational flexibility. Watch-driven reconciliation (spec changes, status updates) still fires immediately regardless of this setting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cache machineScope between Exists() and Update() via a sync.Map-based scopeCache on the Actuator, avoiding redundant session validation and credential reads. Cache the VM reference from exists() for reuse in update(), eliminating a duplicate FindByUuid call. Cache UUID() and getPowerState() results on the virtualMachine struct to avoid duplicate property collector calls within reconcileMachineWithCloudState. Convert TaskIDCache from map[string]string to *sync.Map to make it safe for concurrent reconciliation. Combined savings: ~5 SOAP + 2 REST calls per full reconciliation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add a --max-concurrent-reconciles flag to the vSphere machine controller binary, defaulting to 1, and pass it through to controller-runtime via AddWithActuatorOpts. The operator (sync.go) sets it to 3 for vSphere, conservative compared to Azure/GCP's 10 since vCenter is a shared appliance with lower API throughput. Combined with the short-circuit from commit 1, stable machines complete reconciliation in microseconds, so 3 workers handles concurrent creation bursts without multiplying vCenter API load. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change the default value of --max-concurrent-reconciles from 1 to 3 so the concurrency improvement takes effect without requiring the operator to pass the flag. This commit should be dropped before merge. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rename the generic 'timeout' constant to 'defaultSyncPeriod' to clarify its purpose as the informer resync interval default. Change cachedUUID from string to *string so that a legitimately empty UUID is cached and not re-fetched, matching the cachedPowerState pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jcpowermac
force-pushed
the
vsphere-perf-improvements
branch
from
07c6232 to
66146f4
Compare
|
/test e2e-vsphere-ovn-multi-vcenter |
|
@jcpowermac: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
When a vCenter task (clone, power-on, delete) is still running, reconciler.go returned a plain error, triggering controller-runtime's 5ms exponential backoff. This produced rapid-fire GetTask() API calls and spurious FailedCreate/Update/Delete Warning events for normal task-in-progress polling. Return RequeueAfterError (20s fixed interval) instead, and short-circuit before handleMachineError in the Actuator to suppress Warning events and Error logs for expected intermediate state — mirroring the existing TaskIDCache pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/vsphere/reconciler.go (1)
1509-1524: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear cached power state after
powerOffVM(). In the delete flow,setProviderStatus(..., vm)reuses the same wrapper after a power transition, sogetPowerState()can return the pre-off value fromcachedPowerState. Reset the cache after a successful power-off, or recreate the wrapper before reading state again.🤖 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 `@pkg/controller/vsphere/reconciler.go` around lines 1509 - 1524, Clear the stale power-state cache after a successful power transition in the delete flow: update powerOffVM() to set the virtualMachine.cachedPowerState cache to nil once the VM is powered off, before setProviderStatus(..., vm) reads it again. Preserve the cache on failure and use the existing cachedPowerState field and powerOffVM symbol to locate the change.
🧹 Nitpick comments (1)
pkg/controller/vsphere/actuator_test.go (1)
429-452: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftExercise the actuator path, not only
%wunwrapping.These subtests only construct
reconcilerFailFmtand callerrors.As; they never invokeActuator.Create,Update, orDelete, nor verify thathandleMachineErroris bypassed. The test would still pass if the short-circuit were removed. Either rename it to reflect the narrower wrapping contract or add an actuator-level regression assertion.🤖 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 `@pkg/controller/vsphere/actuator_test.go` around lines 429 - 452, The test only verifies fmt.Errorf wrapping and does not exercise the actuator short-circuit behavior. Extend TestRequeueAfterErrorShortCircuitsHandleMachineError to invoke the relevant Actuator Create, Update, or Delete path with a RequeueAfterError and assert handleMachineError is bypassed, or rename the test to describe only the reconcilerFailFmt unwrapping contract.
🤖 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 `@pkg/controller/vsphere/reconciler_test.go`:
- Around line 3708-3710: Guard the RequeueAfterError assertion in the test
before dereferencing requeueErr: make errors.As a fatal assertion or explicitly
verify requeueErr is non-nil, then assert RequeueAfter only after that check.
- Line 3714: Bound waitForTaskToComplete with a timeout so simulator regressions
cannot hang the test suite indefinitely. Update waitForTaskToComplete to accept
and use a timeout-bearing context for both session.GetTask and taskObj.Wait, or
wrap its invocation with Eventually using an explicit timeout.
- Around line 3592-3594: Update the clone-task test setup around
simulator.TaskDelay.MethodDelay to avoid relying on the 500ms delay: save the
existing MethodDelay map, register t.Cleanup or defer to restore it after the
test, and use a deterministic mechanism to keep the clone task pending until the
second create() rather than elapsed timing. Remove the post-t.Run assignment to
nil and ensure the original simulator state is restored even when the test
fails.
---
Outside diff comments:
In `@pkg/controller/vsphere/reconciler.go`:
- Around line 1509-1524: Clear the stale power-state cache after a successful
power transition in the delete flow: update powerOffVM() to set the
virtualMachine.cachedPowerState cache to nil once the VM is powered off, before
setProviderStatus(..., vm) reads it again. Preserve the cache on failure and use
the existing cachedPowerState field and powerOffVM symbol to locate the change.
---
Nitpick comments:
In `@pkg/controller/vsphere/actuator_test.go`:
- Around line 429-452: The test only verifies fmt.Errorf wrapping and does not
exercise the actuator short-circuit behavior. Extend
TestRequeueAfterErrorShortCircuitsHandleMachineError to invoke the relevant
Actuator Create, Update, or Delete path with a RequeueAfterError and assert
handleMachineError is bypassed, or rename the test to describe only the
reconcilerFailFmt unwrapping contract.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 89164d3f-44f6-42ca-a95b-e867bf963cf1
📒 Files selected for processing (4)
pkg/controller/vsphere/actuator.gopkg/controller/vsphere/actuator_test.gopkg/controller/vsphere/reconciler.gopkg/controller/vsphere/reconciler_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/vsphere/actuator.go
| simulator.TaskDelay.MethodDelay = map[string]int{ | ||
| "CloneVM": 500, | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
# Inspect the relevant sections of the test file
sed -n '3540,3665p' pkg/controller/vsphere/reconciler_test.go
printf '\n---\n'
sed -n '3680,3755p' pkg/controller/vsphere/reconciler_test.go
printf '\n=== SEARCH ===\n'
rg -n "TaskDelay\.MethodDelay|CloneVM|SetDelay|MethodDelay =" pkg/controller/vsphere/reconciler_test.go pkg -g '!**/vendor/**'Repository: openshift/machine-api-operator
Length of output: 5910
🏁 Script executed:
set -euo pipefail
# Inspect simulator.TaskDelay definition and any other use patterns.
rg -n "type .*TaskDelay|var TaskDelay|MethodDelay|TaskDelay" "$(go env GOPATH 2>/dev/null || true)" -g '!**/vendor/**' || true
# Search within the module cache is unreliable; inspect repo references too.
rg -n "TaskDelay|MethodDelay" . -g '!**/vendor/**' -g '!**/dist/**' -g '!**/build/**'Repository: openshift/machine-api-operator
Length of output: 2489
🏁 Script executed:
set -euo pipefail
# Search the repository for all direct references to simulator.TaskDelay / MethodDelay.
rg -n "TaskDelay|MethodDelay" . -g '!**/vendor/**' -g '!**/dist/**' -g '!**/build/**'
# If the simulator package is vendored, inspect the declaration there.
fd -a "simulator" vendor . 2>/dev/null | head -n 20Repository: openshift/machine-api-operator
Length of output: 1716
🏁 Script executed:
set -euo pipefail
# Inspect the nearby test that also mutates MethodDelay and whether it restores state.
sed -n '240,330p' pkg/controller/vsphere/session/session_test.go
# Inspect the TaskDelay implementation in vendored govmomi to understand nil/restore semantics.
sed -n '1,120p' vendor/github.com/vmware/govmomi/simulator/task.go
sed -n '1,120p' vendor/github.com/vmware/govmomi/simulator/model.goRepository: openshift/machine-api-operator
Length of output: 10869
🏁 Script executed:
set -euo pipefail
# Check whether the vsphere reconciler tests use parallelism or other shared-state patterns.
rg -n "Parallel\(|t\.Parallel\(|RunParallel|TaskDelay\.MethodDelay" pkg/controller/vsphere -g '!**/vendor/**'
# Show the exact reconciler test block with line numbers for context.
cat -n pkg/controller/vsphere/reconciler_test.go | sed -n '3580,3725p'Repository: openshift/machine-api-operator
Length of output: 5851
🏁 Script executed:
set -euo pipefail
# Inspect the nearby test that also mutates MethodDelay and whether it restores state.
cat -n pkg/controller/vsphere/session/session_test.go | sed -n '260,310p'
# Inspect the vendored implementation to confirm what nil means for MethodDelay.
cat -n vendor/github.com/vmware/govmomi/simulator/task.go | sed -n '1,140p'
cat -n vendor/github.com/vmware/govmomi/simulator/model.go | sed -n '1,80p'Repository: openshift/machine-api-operator
Length of output: 10309
🏁 Script executed:
set -euo pipefail
# Confirm whether this package's tests run sequentially and whether cleanup should be deferred here.
cat -n pkg/controller/vsphere/session/session_test.go | sed -n '260,310p'
printf '\n---\n'
cat -n vendor/github.com/vmware/govmomi/simulator/task.go | sed -n '1,140p'
printf '\n---\n'
cat -n vendor/github.com/vmware/govmomi/simulator/model.go | sed -n '1,80p'Repository: openshift/machine-api-operator
Length of output: 10319
Make the clone-task test less timing-sensitive and restore simulator state in cleanup.
The 500ms CloneVM delay still depends on elapsed time to keep the task running, so a slow CI worker can let the task finish before the second create(). Save the previous simulator.TaskDelay.MethodDelay and restore it with defer/t.Cleanup instead of assigning nil after t.Run.
🤖 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 `@pkg/controller/vsphere/reconciler_test.go` around lines 3592 - 3594, Update
the clone-task test setup around simulator.TaskDelay.MethodDelay to avoid
relying on the 500ms delay: save the existing MethodDelay map, register
t.Cleanup or defer to restore it after the test, and use a deterministic
mechanism to keep the clone task pending until the second create() rather than
elapsed timing. Remove the post-t.Run assignment to nil and ensure the original
simulator state is restored even when the test fails.
| var requeueErr *machinecontroller.RequeueAfterError | ||
| g.Expect(errors.As(err, &requeueErr)).To(BeTrue(), "expected RequeueAfterError, got: %v", err) | ||
| g.Expect(requeueErr.RequeueAfter).To(Equal(time.Duration(requeueAfterSeconds) * time.Second)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '3675,3725p' pkg/controller/vsphere/reconciler_test.go | cat -nRepository: openshift/machine-api-operator
Length of output: 2351
Guard the RequeueAfterError assertion before dereferencing it. If errors.As fails, requeueErr stays nil and the next line can panic; use a fatal assertion or check requeueErr before reading RequeueAfter.
🤖 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 `@pkg/controller/vsphere/reconciler_test.go` around lines 3708 - 3710, Guard
the RequeueAfterError assertion in the test before dereferencing requeueErr:
make errors.As a fatal assertion or explicitly verify requeueErr is non-nil,
then assert RequeueAfter only after that check.
|
|
||
| // Wait for the clone task to complete before exiting, so the simulator | ||
| // goroutine finishes and doesn't race with clearing TaskDelay. | ||
| g.Expect(waitForTaskToComplete(session, reconciler)).To(Succeed()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the helper and its usages
rg -n "waitForTaskToComplete" pkg/controller/vsphere/reconciler_test.go
# Show the function implementation and nearby context
line=$(rg -n "func waitForTaskToComplete" pkg/controller/vsphere/reconciler_test.go | head -n1 | cut -d: -f1)
if [ -n "${line:-}" ]; then
start=$((line-30))
end=$((line+120))
sed -n "${start},${end}p" pkg/controller/vsphere/reconciler_test.go
fiRepository: openshift/machine-api-operator
Length of output: 4638
Bound the task wait. waitForTaskToComplete uses context.TODO() for both session.GetTask and taskObj.Wait, so a simulator regression can stall the suite indefinitely. Pass a timeout-bearing context through this helper or wrap the wait in Eventually.
🤖 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 `@pkg/controller/vsphere/reconciler_test.go` at line 3714, Bound
waitForTaskToComplete with a timeout so simulator regressions cannot hang the
test suite indefinitely. Update waitForTaskToComplete to accept and use a
timeout-bearing context for both session.GetTask and taskObj.Wait, or wrap its
invocation with Eventually using an explicit timeout.
Source: Path instructions
1 participant
Stable Running machines were fully re-reconciled every 10 minutes with ~25-30 serial vCenter API calls, starving newly created machines of queue time on large clusters. Implements the plan in docs/proposals/vsphere-perf-improvements.md:
Summary by CodeRabbit
New Features
Bug Fixes
Tests