OCPBUGS-98217: fix(kubevirt): stop hardcoding evictionStrategy=External on KubeVirt VMs#8974
OCPBUGS-98217: fix(kubevirt): stop hardcoding evictionStrategy=External on KubeVirt VMs#8974tiraboschi wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@tiraboschi: This pull request references Jira Issue OCPBUGS-98217, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a KubeVirt eviction strategy enum and an optional 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/content/how-to/kubevirt/gpu-devices.md (1)
91-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: cluster default also depends on node count.
The doc states the cluster-level default is "typically
LiveMigrateon HA clusters managed by HCO," which is accurate for multi-node clusters, but KubeVirt/HCO defaults toNoneon single-node OpenShift. Worth a brief mention for completeness, though not essential for this PR's GPU-focused scope.🤖 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 `@docs/content/how-to/kubevirt/gpu-devices.md` around lines 91 - 114, Update the eviction strategy documentation in the GPU NodePools section to mention that the cluster-level KubeVirt default is not always LiveMigrate; note the single-node OpenShift case where the default is None. Keep the guidance anchored around the evictionStrategy explanation and the supported values table, and add a short clarifying sentence near the description of the default behavior.api/hypershift/v1beta1/kubevirt.go (1)
194-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a dedicated named type instead of raw
*string.
NetworkInterfaceMultiQueuein this same struct uses a dedicated type (*MultiQueueSetting) for its enum.EvictionStrategyinstead uses*string, which loses compile-time type safety and forces the controller to blindly castkubevirtv1.EvictionStrategy(*kvPlatform.EvictionStrategy)without any type-level guarantee that the value belongs to the enum.♻️ Suggested approach
+// KubevirtEvictionStrategy defines the eviction behavior for KubeVirt VMs. +// +kubebuilder:validation:Enum=LiveMigrate;LiveMigrateIfPossible;External;None +type KubevirtEvictionStrategy string + // +optional - // +kubebuilder:validation:Enum=LiveMigrate;LiveMigrateIfPossible;External;None - EvictionStrategy *string `json:"evictionStrategy,omitempty"` + EvictionStrategy *KubevirtEvictionStrategy `json:"evictionStrategy,omitempty"`🤖 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 `@api/hypershift/v1beta1/kubevirt.go` around lines 194 - 210, `EvictionStrategy` is modeled as a raw `*string`, which loses enum type safety compared with `NetworkInterfaceMultiQueue`’s dedicated `*MultiQueueSetting`. Update the `KubevirtPlatformSpec` field to use a named enum type for eviction strategy, add the corresponding type definition and JSON/schema tags, and adjust any controller/serialization code that currently casts `*kvPlatform.EvictionStrategy` so it works directly with the typed value instead of an unchecked string.
🤖 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 `@api/hypershift/v1beta1/kubevirt.go`:
- Around line 194-210: `EvictionStrategy` is modeled as a raw `*string`, which
loses enum type safety compared with `NetworkInterfaceMultiQueue`’s dedicated
`*MultiQueueSetting`. Update the `KubevirtPlatformSpec` field to use a named
enum type for eviction strategy, add the corresponding type definition and
JSON/schema tags, and adjust any controller/serialization code that currently
casts `*kvPlatform.EvictionStrategy` so it works directly with the typed value
instead of an unchecked string.
In `@docs/content/how-to/kubevirt/gpu-devices.md`:
- Around line 91-114: Update the eviction strategy documentation in the GPU
NodePools section to mention that the cluster-level KubeVirt default is not
always LiveMigrate; note the single-node OpenShift case where the default is
None. Keep the guidance anchored around the evictionStrategy explanation and the
supported values table, and add a short clarifying sentence near the description
of the default behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6e91eb64-982c-4bfc-9fc0-ac7182bb54b6
⛔ Files ignored due to path filters (3)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/kubevirt.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (4)
api/hypershift/v1beta1/kubevirt.godocs/content/how-to/kubevirt/gpu-devices.mdhypershift-operator/controllers/nodepool/kubevirt/kubevirt.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.go
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tiraboschi 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 please review again |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/hypershift/v1beta1/kubevirt.go (1)
206-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew optional field correctly modeled; consider adding a serialization compatibility test.
The pointer +
omitemptychoice correctly models "unset → cluster default" as a distinct valid state, and the enum values match the upstream KubeVirtEvictionStrategytype used by the controller cast. As per API module guidelines, when modifying API types you should add serialization compatibility tests verifying the field round-trips correctly between old/new struct versions (seeapi/hypershift/v1beta1/nodepool_types_test.gofor the pattern). This isn't strictly required for a purely additive optional field, but would tighten N+1/N-1 guarantees for downstream vendors like ARO-HCP.🤖 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 `@api/hypershift/v1beta1/kubevirt.go` around lines 206 - 221, The new optional EvictionStrategy field in KubevirtPlatformSpec should be covered by a serialization compatibility test. Add a round-trip test in the same API test pattern as nodepool_types_test.go that verifies the field stays unset when omitted, preserves explicit enum values when set, and remains compatible across old/new struct versions for KubevirtPlatformSpec and KubevirtEvictionStrategy.Source: Coding guidelines
🤖 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 `@api/hypershift/v1beta1/kubevirt.go`:
- Around line 206-221: The new optional EvictionStrategy field in
KubevirtPlatformSpec should be covered by a serialization compatibility test.
Add a round-trip test in the same API test pattern as nodepool_types_test.go
that verifies the field stays unset when omitted, preserves explicit enum values
when set, and remains compatible across old/new struct versions for
KubevirtPlatformSpec and KubevirtEvictionStrategy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a12e764d-5960-41e3-a28d-aa42ebda7fcf
⛔ Files ignored due to path filters (3)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/kubevirt.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (4)
api/hypershift/v1beta1/kubevirt.godocs/content/how-to/kubevirt/gpu-devices.mdhypershift-operator/controllers/nodepool/kubevirt/kubevirt.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/content/how-to/kubevirt/gpu-devices.md
🚧 Files skipped from review as they are similar to previous changes (2)
- hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go
- hypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8974 +/- ##
=======================================
Coverage 43.60% 43.60%
=======================================
Files 771 771
Lines 95806 95809 +3
=======================================
+ Hits 41778 41781 +3
Misses 51119 51119
Partials 2909 2909
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
The background agent for the lint job has completed and confirms the same findings I already included in the report above. Both agents' analyses are consistent with the root cause I identified:
The full report with all details, recommendations, and evidence is in my previous message. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/hypershift/v1beta1/nodepool_types_test.go (1)
199-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSubtest names should follow "When ... it should ..." format.
t.Run(string(strategy), ...)produces subtest names likeLiveMigrate,External, etc., which don't follow the required pattern used elsewhere in this file (e.g.,TestKubevirtEvictionStrategySerializationCompatibility's subtests).🔧 Proposed fix
- t.Run(string(strategy), func(t *testing.T) { + t.Run(fmt.Sprintf("When EvictionStrategy is %s it should round-trip", strategy), func(t *testing.T) {As per path instructions,
**/*_test.go: "Always use "When ... it should ..." format for describing test cases when creating unit tests."🤖 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 `@api/hypershift/v1beta1/nodepool_types_test.go` around lines 199 - 201, The subtest naming in the Kubevirt eviction strategy test is too generic and does not match the required “When ... it should ...” style. Update the `t.Run` call inside the loop over `expected` in `TestKubevirtEvictionStrategySerializationCompatibility` to use a descriptive scenario-based name that includes the strategy and expected behavior, consistent with the other tests in `nodepool_types_test.go`. Ensure the generated subtest names follow the “When ... it should ...” convention instead of raw values like `LiveMigrate` or `External`.Source: Path instructions
🤖 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 `@api/hypershift/v1beta1/nodepool_types_test.go`:
- Around line 199-201: The subtest naming in the Kubevirt eviction strategy test
is too generic and does not match the required “When ... it should ...” style.
Update the `t.Run` call inside the loop over `expected` in
`TestKubevirtEvictionStrategySerializationCompatibility` to use a descriptive
scenario-based name that includes the strategy and expected behavior, consistent
with the other tests in `nodepool_types_test.go`. Ensure the generated subtest
names follow the “When ... it should ...” convention instead of raw values like
`LiveMigrate` or `External`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 94d6e027-49a3-448b-a613-b8a25c40d329
⛔ Files ignored due to path filters (11)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OSStreams.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/kubevirtnodepoolplatform.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/kubevirt.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (5)
api/hypershift/v1beta1/kubevirt.goapi/hypershift/v1beta1/nodepool_types_test.godocs/content/how-to/kubevirt/gpu-devices.mdhypershift-operator/controllers/nodepool/kubevirt/kubevirt.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- api/hypershift/v1beta1/kubevirt.go
- hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go
- docs/content/how-to/kubevirt/gpu-devices.md
- hypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.go
PR openshift#6380 unconditionally set evictionStrategy=External on all KubeVirt VMs created for HCP NodePools. This overrides the cluster-level default set by HCO (LiveMigrate on HA clusters), forcing CAPK to drain the guest node and delete the VMI on every infra node drain — even when the VM is live-migratable and KubeVirt could transparently migrate it with zero guest disruption. With External strategy, KubeVirt's evacuation controller skips the VM entirely (VMIMigratableOnEviction returns false for External), so no live migration is ever attempted. Instead, CAPK cordons and drains the guest node, deletes the VMI, and KubeVirt recreates it on another host. This is a full disruption to all workloads on the guest node. The External strategy is only appropriate for non-migratable VMs (e.g., with GPU passthrough or SR-IOV) where live migration is impossible and CAPK's drain-then-delete is the only graceful option. For the majority of VMs that are migratable, it is strictly worse than letting KubeVirt handle migration automatically via the cluster default. This commit removes the hardcoded External strategy and adds an optional EvictionStrategy field to KubevirtNodePoolPlatform. When not set, the cluster-level KubeVirt default applies. Users with non-migratable VMs can explicitly opt into External when needed. Fixes: https://redhat.atlassian.net/browse/OCPBUGS-98217 Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/hypershift/v1beta1/nodepool_types_test.go (1)
97-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider defining an explicit N-1 struct, mirroring the existing pattern.
TestNodePoolAutoScalingSerializationCompatibilityabove defines anodePoolAutoScalingNMinus1struct to model the previous API version explicitly. This new test instead simulates N-1 by deleting a key from a raw JSON map, which is functionally valid but diverges from the established pattern in this file and is slightly less explicit about what "N-1" actually looked like for this type.🤖 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 `@api/hypershift/v1beta1/nodepool_types_test.go` around lines 97 - 178, The N-1 compatibility part of TestKubevirtEvictionStrategySerializationCompatibility should use an explicit previous-version struct instead of deleting evictionStrategy from a raw JSON map. Add a KubevirtNodePoolPlatformNMinus1-style type that mirrors the older API shape, marshal that type for the N-1 case, and unmarshal into KubevirtNodePoolPlatform to verify the zero-value behavior. Keep the existing round-trip checks for KubevirtNodePoolPlatform and EvictionStrategy, but align the test with the established pattern used by TestNodePoolAutoScalingSerializationCompatibility.
🤖 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 `@api/hypershift/v1beta1/nodepool_types_test.go`:
- Around line 180-207: The subtest names in
TestKubevirtEvictionStrategyEnumValues do not follow the required “When...it
should...” convention. Update the t.Run label in this test to use a descriptive
sentence in that format, consistent with the other test cases in
nodepool_types_test.go, while keeping the existing enum round-trip assertions
unchanged.
---
Nitpick comments:
In `@api/hypershift/v1beta1/nodepool_types_test.go`:
- Around line 97-178: The N-1 compatibility part of
TestKubevirtEvictionStrategySerializationCompatibility should use an explicit
previous-version struct instead of deleting evictionStrategy from a raw JSON
map. Add a KubevirtNodePoolPlatformNMinus1-style type that mirrors the older API
shape, marshal that type for the N-1 case, and unmarshal into
KubevirtNodePoolPlatform to verify the zero-value behavior. Keep the existing
round-trip checks for KubevirtNodePoolPlatform and EvictionStrategy, but align
the test with the established pattern used by
TestNodePoolAutoScalingSerializationCompatibility.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: be6655b1-7711-4806-9fe8-bf74a68f5fe0
⛔ Files ignored due to path filters (11)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OSStreams.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/kubevirtnodepoolplatform.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/kubevirt.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (5)
api/hypershift/v1beta1/kubevirt.goapi/hypershift/v1beta1/nodepool_types_test.godocs/content/how-to/kubevirt/gpu-devices.mdhypershift-operator/controllers/nodepool/kubevirt/kubevirt.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/content/how-to/kubevirt/gpu-devices.md
🚧 Files skipped from review as they are similar to previous changes (3)
- api/hypershift/v1beta1/kubevirt.go
- hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go
- hypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.go
|
@tiraboschi: all tests passed! 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. |
What this PR does / why we need it:
PR #6380 unconditionally set evictionStrategy=External on all KubeVirt VMs created for HCP NodePools. This overrides the cluster-level default set by HCO (LiveMigrate on HA clusters), forcing CAPK to drain the guest node and delete the VMI on every infra node drain — even when the VM is live-migratable and KubeVirt could transparently migrate it with zero guest disruption.
With External strategy, KubeVirt's evacuation controller skips the VM entirely (VMIMigratableOnEviction returns false for External), so no live migration is ever attempted. Instead, CAPK cordons and drains the guest node, deletes the VMI, and KubeVirt recreates it on another host. This is a full disruption to all workloads on the guest node.
The External strategy is only appropriate for non-migratable VMs (e.g., with GPU passthrough or SR-IOV) where live migration is impossible and CAPK's drain-then-delete is the only graceful option. For the majority of VMs that are migratable, it is strictly worse than letting KubeVirt handle migration automatically via the cluster default.
This commit removes the hardcoded External strategy and adds an optional EvictionStrategy field to KubevirtNodePoolPlatform. When not set, the cluster-level KubeVirt default applies. Users with non-migratable VMs can explicitly opt into External when needed.
Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/OCPBUGS-98217
Special notes for your reviewer:
Exposing this to the user is probably not the smartest or more automated solution. On the other side trying to automate it would require changes on:
Coordinating three releases.
So in terms of effectiveness this is definitively the low hanging fruit solution.
Checklist:
Summary by CodeRabbit
evictionStrategyoption with values:LiveMigrate,LiveMigrateIfPossible,External, andNone.evictionStrategyaffects VM eviction (includingExternalvsNone).