✨ Add power-state-synced condition for host maintenance mode - #1861
Open
hpannem wants to merge 2 commits into
Open
✨ Add power-state-synced condition for host maintenance mode#1861hpannem wants to merge 2 commits into
hpannem wants to merge 2 commits into
Conversation
hpannem
added
the
pro-ai-review-requested
|
📝 CL Summary: This changelist introduces two new compute-policy CRDs, |
svc-vmop-bot
added
cl-summary-reviewed
svc-vmop-bot
added
the
ai-reviewed
faisalabujabal
removed
the
pro-ai-review-requested
hpannem
force-pushed
the
maint-mode-policy-condition
branch
5 times, most recently
from
August 29, 2026 07:08
0912634 to
6a98778
Compare
test fix issue with condition message update
hpannem
force-pushed
the
maint-mode-policy-condition
branch
from
September 7, 2026 22:40
08c6cc4 to
282da2f
Compare
e2e tests
hpannem
force-pushed
the
maint-mode-policy-condition
branch
from
September 7, 2026 23:11
282da2f to
d465d33
Compare
hpannem
added
ai-review-requested
Contributor
Minimum allowed line rate is |
svc-vmop-bot
added
commit-msg-reviewed
svc-vmop-bot
added
ai-reviewed
aruneshpa
approved these changes
Sep 8, 2026
aruneshpa
left a comment
aruneshpa
left a comment
Collaborator
There was a problem hiding this comment.
A few comments, but none are absolute blockers.
| // Exported so its branches can be unit tested with a synthetic error, since | ||
| // vcsim cannot produce a real NoCompatibleHost/autoevac task fault. | ||
| func SetPowerStateSyncedCondition(vmCtx pkgctx.VirtualMachineContext, err error) { | ||
| if vmCtx.VM.Status.PowerState == vmCtx.VM.Spec.PowerState || errors.Is(err, ErrSetPowerState) { |
Collaborator
There was a problem hiding this comment.
Hmm. Why would we set PowerState synced = true if we get ErrSetPowerState?
| vmCtx.VM, | ||
| vmopv1.VirtualMachinePowerStateSynced, | ||
| vmopv1.VirtualMachineInfraInMaintenanceReason, | ||
| "VirtualMachine is powered off because the underlying infrastructure is in maintenance") |
Collaborator
There was a problem hiding this comment.
Thought: Do you want to keep this generic?
Like: "Unable to reconcile virtual machine power state because the underlying infrastructure is in maintenance"?
| } | ||
| for _, fm := range fault.FaultMessage { | ||
| switch fm.Key { | ||
| case FaultMessageKeyHostInMaintenanceMode, FaultMessageKeyRestartOnCurrentHostRequired: |
Collaborator
There was a problem hiding this comment.
Using a switch case for a single conditional is weird. Did linter complain against using a simple if statement here?
If not, let's switch to if. Otherwise, ignore.
| // power-state-change task when the failure is a NoCompatibleHost fault | ||
| // indicating the VM's host is in infrastructure maintenance. Callers use | ||
| // errors.Is to detect this condition from any call-stack depth. | ||
| var ErrInfraMaintenanceFault = errors.New("vm power op failed due to host infrastructure maintenance") |
Collaborator
There was a problem hiding this comment.
Let's get rid of "host" in the error message. That's leaking unnecessary detail to the user. Maybe call it:
var ErrInfraMaintenanceFault = errors.New("vm power op failed due to infrastructure in maintenance")
|
|
||
| // WrapHardPowerOpFailure builds the error returned by doAndWaitOnHardPowerOp | ||
| // for a failed hard power op, wrapping it with ErrInfraMaintenanceFault when | ||
| // ti indicates the failure is a NoCompatibleHost fault caused by the VM's |
Collaborator
There was a problem hiding this comment.
ti -> the provided task info.
For a second, I thought you meant "it".
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
4 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.
[on top of #1860 ]
What does this PR do, and why is it needed?
Adds a new
Reason,InfraInMaintenance, to the existingVirtualMachinePowerStateSyncedcondition, so tenants/VKS can tell a maintenance-mode-driven power-off apart from a genericNotSynced. When a power-state-change task fails with aNoCompatibleHostfault carrying vCenter's autoevac key (com.vmware.cp.autoevac.HostInMaintenanceMode/...RestartOnCurrentHostRequired), the condition is set accordingly instead of the generic reason. No new vCenter calls — the signal comes from the failed task's own fault. Gated behind theVMEvictionfeature flag; behavior is unchanged when it's off.pkg/util/vsphere/task:IsInfraMaintenanceFaultdetects the fault.pkg/util/vsphere/vm:WrapHardPowerOpFailurewraps it into sentinelErrInfraMaintenanceFault.pkg/providers/vsphere/vmprovider_vm.go:SetPowerStateSyncedConditionsets the condition fromreconcilePowerState's outcome.pkg/providers/vsphere/vmlifecycle/update_status.go:reconcileStatusPowerStatenow only setsStatus.PowerState(condition logic moved above).VirtualMachineInfraInMaintenanceReasonconstant inapi/v1alpha6.vmevictionpolicy.go): 3-VM spec — one unpinned VM gets relocated by DRS; two pinned to the same host (via a realvm_host_affinityComputePolicy, mimicking a VM DRS can't evacuate) get powered off by DRS's own autoevac and surfaceInfraInMaintenanceon VM Operator's retry, then recover once maintenance ends.Which issue(s) is/are addressed by this PR?
Fixes #
Are there any special notes for your reviewer:
experimental, excluded from CI until validated on real hardware.ComputePolicy/InfraPolicyhave no delete API, so they live until the suite's shared namespace is torn down at the end of the run rather than per-test — flagging in case a temp-namespace refactor is preferred.Please add a release note if necessary: