✨ BestEffortRestartPolicy e2e - #1860
Open
hpannem wants to merge 1 commit into
Open
Conversation
hpannem
added
the
pro-ai-review-requested
|
📝 CL Summary: This pull request introduces the |
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-besteffort
branch
3 times, most recently
from
September 3, 2026 22:54
cf95240 to
8975be3
Compare
hpannem
changed the title
hpannem
force-pushed
the
maint-mode-policy-besteffort
branch
from
September 4, 2026 23:40
8975be3 to
3540c58
Compare
github-actions
Bot
added
size/XL
hpannem
added
pro-ai-review-requested
svc-vmop-bot
added
ai-reviewed
hpannem
force-pushed
the
maint-mode-policy-besteffort
branch
from
September 6, 2026 16:14
3540c58 to
6993c1a
Compare
test fix issue with condition message update
hpannem
force-pushed
the
maint-mode-policy-besteffort
branch
from
September 7, 2026 00:40
6993c1a to
4215de5
Compare
Contributor
Minimum allowed line rate is |
aruneshpa
approved these changes
Sep 8, 2026
aruneshpa
left a comment
aruneshpa
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Two minor comments.
| vmCtx.VM, | ||
| vmopv1.VirtualMachineConditionPlacementReady) { | ||
| vmopv1.VirtualMachineConditionPlacementReady) || | ||
| pkgcond.GetReason(vmCtx.VM, |
Collaborator
There was a problem hiding this comment.
I may be missing something here. :( But what you are adding seems redundant? The only way your code executes is when the VirtualMachineConditionPlacementReady is False AND the reason is NotReady -- upon which we set the reason to NotReady in the if block?
|
|
||
| // The object is still being processed. | ||
| msg := pkgcond.GetMessage(obj, vspherepolv1.ReadyConditionType) | ||
| if msg != "" { |
Collaborator
There was a problem hiding this comment.
This is confusing. Instead of creating this weird formatted string by sticking this next to the error, let's either have another key (e.g., "msg", ). Or, change the error to include the message somehow.
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.
What does this PR do, and why is it needed?
Adds the remaining E2E coverage for the
AutomaticVMEvictionPolicy/BestEffortRestartPolicycompute-policy CRDs (product code already merged in #1859), closing outtasks.mdT021/T022 and adding T040:BestEffortRestartPolicyviaspec.policiestags the VM.BestEffortRestartPolicysurfaces a not-readyPolicyEvaluationcondition (Ready=False, message containing "does not match").AutomaticVMEvictionPolicyand explicitly referencing an OptionalBestEffortRestartPolicysurfaces both instatus.policies.spec.policies.All new
Its are labeledexperimentalpere2e-testing.md, pending validation on real hardware.Also removes a standalone
ComputePolicye2e spec (computepolicy.go) and itsContext("COMPUTE-POLICY", ...)wiring that were added earlier under the mistaken assumptionComputePolicyhad no e2e coverage — it already has thorough mandatory/optional combination coverage through the real WCP admin API path intest/e2e/vmservice/vmservice/virtualmachine/virtualmachinelcm.go(Context("IaaS Policies", ...),Context("PLACEMENT-POLICY-COMBINATIONS", ...)), so the parallel direct-CRD-creation spec was redundant. This PR's scope is intentionally limited toAutomaticVMEvictionPolicy/BestEffortRestartPolicy..sdd/specs/007-vm-eviction-policy/plan.mdandtasks.mdupdated to match.Which issue(s) is/are addressed by this PR? (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Are there any special notes for your reviewer:
experimentallabel and are excluded from CI until validated on hardware.createAutomaticVMEvictionPolicy/createBestEffortRestartPolicygained anenforcementModeparameter (previously hardcoded per kind) so both Mandatory and Optional variants are exercisable from the same helpers; all existing call sites updated accordingly.Please add a release note if necessary: