MGMT-24155: Use LVM operator instead of LSO for CNV dependency#10285
MGMT-24155: Use LVM operator instead of LSO for CNV dependency#10285shay23bra wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@shay23bra: This pull request references MGMT-24155 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 bug 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. |
openshift-ci
Bot
added
the
size/L
WalkthroughGetDependencies now checks LVM feature compatibility by CPU architecture and OpenShift version, returning no dependencies when LVM is incompatible. For compatible clusters it prefers LVM (including when OCP version is empty), but falls back to LSO for SNO or multi-node clusters that don't meet LVM version/support thresholds. GetDependenciesFeatureSupportID reports only IDLVM. Tests were converted to table-driven cases covering OCP versions, architectures, and SNO vs multi-node scenarios. ChangesCNV Dependency Selection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/operators/cnv/cnv_operator.go`:
- Around line 77-88: The version-compare errors from
common.BaseVersionGreaterOrEqual are currently ignored in the CNV dependency
logic; update the checks in the function (the block using
common.IsSingleNodeCluster and the subsequent multi-node check) to capture the
error return from common.BaseVersionGreaterOrEqual and propagate it back to the
caller instead of discarding it—i.e., for both calls that reference
lvm.LvmsMinOpenshiftVersion4_12 and lvm.LvmMinMultiNodeSupportVersion, check the
error and return nil, err when non-nil; keep the existing behavior of returning
[]string{lvm.Operator.Name}, nil when isSupported is true, otherwise continue
flow.
🪄 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: 9737446d-b865-468d-9f15-fcb642d2d1e0
📒 Files selected for processing (2)
internal/operators/cnv/cnv_operator.gointernal/operators/cnv/cnv_operator_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10285 +/- ##
==========================================
+ Coverage 44.35% 44.42% +0.07%
==========================================
Files 416 417 +1
Lines 72792 73213 +421
==========================================
+ Hits 32284 32522 +238
- Misses 37577 37736 +159
- Partials 2931 2955 +24
🚀 New features to boost your workflow:
|
|
@shay23bra should we drop dependencies in older OCP versions too? |
@rccrdpccl The ticket said: "If an architecture does not have it, it's fine to pull nothing." - So I assumed it was for all scenarios that do not have it, but maybe I was wrong and it should be only for 4.15+ ? |
shay23bra
force-pushed
the
MGMT-24155-Use-LVM-operator-instead-of-LSO-for-CNV-dependency
branch
2 times, most recently
from
7b8104c to
4d5b82a
Compare
|
For older versions that didn’t have LVM - multi-node OCP < 4.15 and SNO < 4.12, the existing LSO dependency is preserved as a fallback since LVM is not supported for those configurations. |
…upported configurations
shay23bra
force-pushed
the
MGMT-24155-Use-LVM-operator-instead-of-LSO-for-CNV-dependency
branch
from
4d5b82a to
bc2dcb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/operators/cnv/cnv_operator_test.go`:
- Around line 54-64: The test entries that expect LSO should be changed to
expect no storage dependency: update each Entry that currently uses
[]string{lso.Operator.Name} to an empty slice (e.g., []string{}) for both
single-node and multi-node cases so GetDependencies and GetPreflightRequirements
assert no dependency; likewise update the duplicate block (lines around 351-362)
the same way and then remove the now-unused lso test import. Ensure you modify
the specific test entries that reference lso.Operator.Name and any related
import cleanup.
In `@internal/operators/cnv/cnv_operator.go`:
- Around line 78-89: The method currently falls back to returning
lso.Operator.Name for older OCP versions; change the logic so that when LVM is
not supported for the cluster (both single-node and multi-node checks using
common.BaseVersionGreaterOrEqual against lvm.LvmsMinOpenshiftVersion4_12 and
lvm.LvmMinMultiNodeSupportVersion) the function returns an empty slice (or nil)
instead of []string{lso.Operator.Name}, and then remove the unused lso import;
ensure this keeps the behavior aligned with GetDependenciesFeatureSupportID
which only reports LVM support.
🪄 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: 620c67e7-a6d9-4a56-b313-5d37e3b68dbf
📒 Files selected for processing (2)
internal/operators/cnv/cnv_operator.gointernal/operators/cnv/cnv_operator_test.go
| // to allow CNV ARM operator | ||
| if cluster.CPUArchitecture == common.ARM64CPUArchitecture || cluster.CPUArchitecture == common.MultiCPUArchitecture { | ||
| return make([]string, 0), nil | ||
| if !featuresupport.IsFeatureCompatibleWithArchitecture(models.FeatureSupportLevelIDLVM, cluster.OpenshiftVersion, cluster.CPUArchitecture) { |
There was a problem hiding this comment.
If LVM not supported, return no dependencies? Isn't this contradicting For older versions that didn’t have LVM - multi-node OCP < 4.15 and SNO < 4.12, the existing LSO dependency is preserved as a fallback since LVM is not supported for those configurations. or am I reading it wrong?
There was a problem hiding this comment.
Nevermind, it's architecture specific 😅
sorry
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rccrdpccl, shay23bra The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/operators/cnv/cnv_operator.go`:
- Around line 92-94: GetDependenciesFeatureSupportID currently only returns
FeatureSupportLevelIDLVM but GetDependencies can also return lso.Operator.Name
for older OCP versions, so update the operator.GetDependenciesFeatureSupportID
implementation to include both feature IDs; modify the return so it includes
models.FeatureSupportLevelIDLVM and models.FeatureSupportLevelIDLSO (ensure you
reference the GetDependenciesFeatureSupportID function and the
models.FeatureSupportLevelIDLSO constant when making the change).
🪄 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: cad88009-4799-477a-ba02-7b171e063f83
📒 Files selected for processing (2)
internal/operators/cnv/cnv_operator.gointernal/operators/cnv/cnv_operator_test.go
| func (o *operator) GetDependenciesFeatureSupportID() []models.FeatureSupportLevelID { | ||
| return []models.FeatureSupportLevelID{models.FeatureSupportLevelIDLSO, models.FeatureSupportLevelIDLVM} | ||
| return []models.FeatureSupportLevelID{models.FeatureSupportLevelIDLVM} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find how GetDependenciesFeatureSupportID is used to understand if this inconsistency matters
rg -n "GetDependenciesFeatureSupportID" --type=go -C5Repository: openshift/assisted-service
Length of output: 34862
🏁 Script executed:
sed -n '60,95p' internal/operators/cnv/cnv_operator.goRepository: openshift/assisted-service
Length of output: 1261
Add FeatureSupportLevelIDLSO to GetDependenciesFeatureSupportID to match all possible dependencies.
GetDependencies returns lso.Operator.Name for older OCP versions (SNO < 4.12, multi-node < 4.15), but GetDependenciesFeatureSupportID returns only FeatureSupportLevelIDLVM. This inconsistency breaks the contract that GetDependenciesFeatureSupportID should return all potential feature IDs that GetDependencies could return. Callers using this method for feature support validation will incorrectly pass compatibility checks when LSO is required but not included.
Update line 93 to:
return []models.FeatureSupportLevelID{models.FeatureSupportLevelIDLVM, models.FeatureSupportLevelIDLSO}🤖 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/operators/cnv/cnv_operator.go` around lines 92 - 94,
GetDependenciesFeatureSupportID currently only returns FeatureSupportLevelIDLVM
but GetDependencies can also return lso.Operator.Name for older OCP versions, so
update the operator.GetDependenciesFeatureSupportID implementation to include
both feature IDs; modify the return so it includes
models.FeatureSupportLevelIDLVM and models.FeatureSupportLevelIDLSO (ensure you
reference the GetDependenciesFeatureSupportID function and the
models.FeatureSupportLevelIDLSO constant when making the change).
|
/retest |
|
/hold Revision bc2dcb1 was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/retest-required |
|
/retest |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/retest-required |
|
The
I've made a dev-scripts fix here openshift-metal3/dev-scripts#1891 and confirmed the |
|
I wasn't expecting a failure in this job (one that consumes operators btw) /test e2e-agent-compact-ipv4-iso-no-registry |
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
@shay23bra: The following tests 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. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/test e2e-agent-compact-ipv4-iso-no-registry |
6 participants
Replaces the LSO dependency with LVM for CNV across all cluster topologies, as requested by the Virtualization team.
CNV now pulls LVM on SNO (OCP 4.12+), multi-node (OCP 4.15+), and returns no storage dependency for unsupported architectures (s390x, ppc64le) or older OCP versions — LSO is never pulled as a CNV dependency anymore.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Before changes:
After changes:
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit
Refactor
Tests