Skip to content

MGMT-24155: Use LVM operator instead of LSO for CNV dependency #10285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shay23bra wants to merge 1 commit into
base: master
Choose a base branch
from
+68 −41
Open

MGMT-24155: Use LVM operator instead of LSO for CNV dependency #10285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions internal/operators/cnv/cnv_operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,30 +67,30 @@ func (o *operator) GetFullName() string {

// GetDependencies provides a list of dependencies of the Operator
func (o *operator) GetDependencies(cluster *common.Cluster) ([]string, error) {
lsoOperator := []string{lso.Operator.Name}
lvmOperator := []string{lvm.Operator.Name}

// Disable lso for ARM deployment as it's not supported
// 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) {
Copy link
Copy Markdown
Contributor

@rccrdpccl rccrdpccl May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@rccrdpccl rccrdpccl May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, it's architecture specific 😅
sorry

return []string{}, nil
}

if cluster.OpenshiftVersion == "" {
return lsoOperator, nil
return []string{lvm.Operator.Name}, nil
}

// SNO
if common.IsSingleNodeCluster(cluster) {
if isGreaterOrEqual, _ := common.BaseVersionGreaterOrEqual(lvm.LvmsMinOpenshiftVersion4_12, cluster.OpenshiftVersion); isGreaterOrEqual {
return lvmOperator, nil
if isSupported, _ := common.BaseVersionGreaterOrEqual(lvm.LvmsMinOpenshiftVersion4_12, cluster.OpenshiftVersion); isSupported {
return []string{lvm.Operator.Name}, nil
}
return []string{lso.Operator.Name}, nil
}

if isSupported, _ := common.BaseVersionGreaterOrEqual(lvm.LvmMinMultiNodeSupportVersion, cluster.OpenshiftVersion); isSupported {
return []string{lvm.Operator.Name}, nil
}
return lsoOperator, nil

return []string{lso.Operator.Name}, nil
Comment thread
shay23bra marked this conversation as resolved.
}

func (o *operator) GetDependenciesFeatureSupportID() []models.FeatureSupportLevelID {
return []models.FeatureSupportLevelID{models.FeatureSupportLevelIDLSO, models.FeatureSupportLevelIDLVM}
return []models.FeatureSupportLevelID{models.FeatureSupportLevelIDLVM}
}
Comment on lines 92 to 94
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find how GetDependenciesFeatureSupportID is used to understand if this inconsistency matters
rg -n "GetDependenciesFeatureSupportID" --type=go -C5

Repository: openshift/assisted-service

Length of output: 34862

🏁 Script executed:

sed -n '60,95p' internal/operators/cnv/cnv_operator.go

Repository: 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).


// GetClusterValidationIDs returns cluster validation IDs for the Operator
Expand Down
83 changes: 55 additions & 28 deletions internal/operators/cnv/cnv_operator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,23 +37,43 @@ var _ = Describe("CNV operator", func() {
operator = cnv.NewCNVOperator(log, cfg)
})

DescribeTable("getDependencies", func(ocpVersion string, haMode int64, expectedOperator string) {
cluster := common.Cluster{
Cluster: models.Cluster{ControlPlaneCount: haMode, OpenshiftVersion: ocpVersion},
}

requirements, err := operator.GetDependencies(&cluster)
Expect(err).ToNot(HaveOccurred())
Expect(requirements).ToNot(BeNil())

Expect(requirements[0]).To(BeEquivalentTo(expectedOperator))
},
DescribeTable("getDependencies",
func(ocpVersion string, cpuArch string, haMode int64, expectedDeps []string) {
cluster := common.Cluster{
Cluster: models.Cluster{
ControlPlaneCount: haMode,
OpenshiftVersion: ocpVersion,
CPUArchitecture: cpuArch,
},
}

Entry("LVM, Single node 4.12", "4.12", int64(1), lvm.Operator.Name),
Entry("LSO, Multi node 4.15", "4.15", int64(common.MinMasterHostsNeededForInstallationInHaMode), lso.Operator.Name),
Entry("LSO, Multi node 4.21", "4.21", int64(common.MinMasterHostsNeededForInstallationInHaMode), lso.Operator.Name),
Entry("LSO, Multi node 4.12", "4.12", int64(common.MinMasterHostsNeededForInstallationInHaMode), lso.Operator.Name),
Entry("LSO, Single node 4.11", "4.11", int64(1), lso.Operator.Name),
deps, err := operator.GetDependencies(&cluster)
Expect(err).ToNot(HaveOccurred())
Expect(deps).To(Equal(expectedDeps))
},
// SNO: LVM from 4.12+, LSO fallback for older
Entry("LVM, Single node 4.12", "4.12", common.DefaultCPUArchitecture, int64(1), []string{lvm.Operator.Name}),
Entry("LVM, Single node 4.15", "4.15", common.DefaultCPUArchitecture, int64(1), []string{lvm.Operator.Name}),
Entry("LSO, Single node 4.11", "4.11", common.DefaultCPUArchitecture, int64(1), []string{lso.Operator.Name}),
Entry("LSO, Single node 4.10", "4.10", common.DefaultCPUArchitecture, int64(1), []string{lso.Operator.Name}),

// Multi-node: LVM from 4.15+, LSO fallback for older
Entry("LVM, Multi node 4.15", "4.15", common.DefaultCPUArchitecture, int64(common.MinMasterHostsNeededForInstallationInHaMode), []string{lvm.Operator.Name}),
Entry("LVM, Multi node 4.21", "4.21", common.DefaultCPUArchitecture, int64(common.MinMasterHostsNeededForInstallationInHaMode), []string{lvm.Operator.Name}),
Entry("LSO, Multi node 4.14", "4.14", common.DefaultCPUArchitecture, int64(common.MinMasterHostsNeededForInstallationInHaMode), []string{lso.Operator.Name}),
Entry("LSO, Multi node 4.12", "4.12", common.DefaultCPUArchitecture, int64(common.MinMasterHostsNeededForInstallationInHaMode), []string{lso.Operator.Name}),
Comment thread
shay23bra marked this conversation as resolved.

// Unsupported architectures: no deps regardless of version
Entry("No deps, s390x architecture", "4.15", common.S390xCPUArchitecture, int64(1), []string{}),
Entry("No deps, ppc64le architecture", "4.15", common.PowerCPUArchitecture, int64(1), []string{}),

// ARM64 and multi-arch: LVM is supported
Entry("LVM, ARM64 SNO 4.15", "4.15", common.ARM64CPUArchitecture, int64(1), []string{lvm.Operator.Name}),
Entry("LVM, ARM64 Multi node 4.15", "4.15", common.ARM64CPUArchitecture, int64(common.MinMasterHostsNeededForInstallationInHaMode), []string{lvm.Operator.Name}),
Entry("LVM, Multi arch Multi node 4.15", "4.15", common.MultiCPUArchitecture, int64(common.MinMasterHostsNeededForInstallationInHaMode), []string{lvm.Operator.Name}),

// Empty OCP version defaults to LVM
Entry("LVM, empty OCP version", "", common.DefaultCPUArchitecture, int64(1), []string{lvm.Operator.Name}),
)

Context("host requirements", func() {
Expand Down Expand Up @@ -297,18 +317,17 @@ var _ = Describe("CNV operator", func() {
)
})

DescribeTable("GetPreflightRequirements, should be returned", func(cfg cnv.Config, cluster common.Cluster) {
DescribeTable("GetPreflightRequirements, should be returned", func(cfg cnv.Config, cluster common.Cluster, expectedDeps []string) {
cnvOperator := cnv.NewCNVOperator(log, cfg)
requirements, err := cnvOperator.GetPreflightRequirements(context.TODO(), &cluster)
Expect(err).ToNot(HaveOccurred())
Expect(requirements.Dependencies).To(ConsistOf(lso.Operator.Name))
Expect(requirements.Dependencies).To(Equal(expectedDeps))
Expect(requirements.OperatorName).To(BeEquivalentTo(cnv.Operator.Name))
numQualitative := 3
workerRequirements := newRequirements(cnv.WorkerCPU, cnv.WorkerMemory)
masterRequirements := newRequirements(cnv.MasterCPU, cnv.MasterMemory)

if common.IsSingleNodeCluster(&cluster) {
// CNV+SNO installs HPP storage; additional discoverable disk req
if cfg.SNOInstallHPP {
numQualitative += 1
}
Expand All @@ -321,18 +340,26 @@ var _ = Describe("CNV operator", func() {
Expect(requirements.Requirements.Master.Quantitative).To(BeEquivalentTo(masterRequirements))
Expect(requirements.Requirements.Master.Qualitative).To(BeEquivalentTo(requirements.Requirements.Worker.Qualitative))
},
Entry("for non-SNO", cnv.Config{SNOPoolSizeRequestHPPGib: 50}, common.Cluster{Cluster: models.Cluster{
OpenshiftVersion: "4.10",
Entry("for non-SNO 4.15", cnv.Config{SNOPoolSizeRequestHPPGib: 50}, common.Cluster{Cluster: models.Cluster{
OpenshiftVersion: "4.15",
ControlPlaneCount: common.MinMasterHostsNeededForInstallationInHaMode,
}}),
Entry("for SNO", cnv.Config{SNOPoolSizeRequestHPPGib: 50, SNOInstallHPP: true}, common.Cluster{Cluster: models.Cluster{
OpenshiftVersion: "4.10",
}}, []string{lvm.Operator.Name}),
Entry("for SNO 4.12", cnv.Config{SNOPoolSizeRequestHPPGib: 50}, common.Cluster{Cluster: models.Cluster{
OpenshiftVersion: "4.12",
ControlPlaneCount: sno,
}}),
Entry("for SNO and opt out of HPP via env var", cnv.Config{SNOPoolSizeRequestHPPGib: 50, SNOInstallHPP: false}, common.Cluster{Cluster: models.Cluster{
OpenshiftVersion: "4.10",
}}, []string{lvm.Operator.Name}),
Entry("for SNO 4.11 with HPP", cnv.Config{SNOPoolSizeRequestHPPGib: 50, SNOInstallHPP: true}, common.Cluster{Cluster: models.Cluster{
OpenshiftVersion: "4.11",
ControlPlaneCount: sno,
}}),
}}, []string{lso.Operator.Name}),
Entry("for SNO 4.11 opt out HPP", cnv.Config{SNOPoolSizeRequestHPPGib: 50, SNOInstallHPP: false}, common.Cluster{Cluster: models.Cluster{
OpenshiftVersion: "4.11",
ControlPlaneCount: sno,
}}, []string{lso.Operator.Name}),
Entry("for multi-node OCP < 4.15 with LSO fallback", cnv.Config{SNOPoolSizeRequestHPPGib: 50}, common.Cluster{Cluster: models.Cluster{
OpenshiftVersion: "4.14",
ControlPlaneCount: common.MinMasterHostsNeededForInstallationInHaMode,
}}, []string{lso.Operator.Name}),
)

DescribeTable("Validate Cluster", func(ocpVersion []string, cpuArch string, expectedApiStatus api.ValidationStatus, errorMessage string) {
Expand Down