OCPEDGE-2109: Allow device deletion on day2 #1380

qJkee · 2025-09-25T11:41:21Z

This is a POC of device deletion for LVMS operator.
Currently we do allow to user to delete required devices and optional devices from the CR, but there are few limitations:

You can't delete all devices from device class, at least one required or optional device must present in CR
If device is no longer available on the node, user must fix LVM's VG on the node first and then modify the CR to reflect current state of the VG on the node. The reason for that is because we can't remove exact missing device, only all of them if there are more than one device is missing
You can't delete device which has data on it. If LVMS detects that there are data on the disk, it will ask user to do pvmove command manually on the node to remove all the data from the disk in VG

The standard user workflow might be defined in 2 ways.

If the disk is lost, fix the VG on the node first and then delete disk from the CR
Delete the disk from CR, but make sure that there are no data on this disk, otherwise use pvmove command on the host to remove any data leftowers

openshift-ci-robot · 2025-09-25T11:41:24Z

@qJkee: This pull request references OCPEDGE-2109 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 story to target the "4.21.0" version, but no target version was set.

In 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 · 2025-09-25T11:41:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qJkee

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [qJkee]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2025-09-30T11:48:26Z

@qJkee: This pull request references OCPEDGE-2109 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 story to target the "4.21.0" version, but no target version was set.

In response to this:

This is a POC of device deletion for LVMS operator.
Currently we do allow to user to delete required devices and optional devices from the CR, but there are few limitations:

You can't delete all devices from device class, at least one required or optional device must present in CR

If device is no longer available on the node, user must fix LVM's VG on the node first and then modify the CR to reflect current state of the VG on the node. The reason for that is because we can't remove exact missing device, only all of them if there are more than one device is missing

You can't delete device which has data on it. If LVMS detects that there are data on the disk, it will ask user to do pvmove command manually on the node to remove all the data from the disk in VG

The standard user workflow might be defined in 2 ways.

If the disk is lost, fix the VG on the node first and then delete disk from the CR

Delete the disk from CR, but make sure that there are no data on this disk, otherwise use pvmove command on the host to remove any data leftowers

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.

codecov-commenter · 2025-10-01T08:05:55Z

Codecov Report

❌ Patch coverage is 40.00000% with 54 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.78%. Comparing base (44c57f7) to head (5c736d5).
⚠️ Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/controllers/vgmanager/controller.go	22.50%	25 Missing and 6 partials ⚠️
internal/controllers/vgmanager/lvm/lvm.go	46.15%	14 Missing ⚠️
...nal/controllers/vgmanager/device_mapping_helper.go	63.15%	4 Missing and 3 partials ⚠️
api/v1alpha1/lvmcluster_webhook.go	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1380      +/-   ##
==========================================
- Coverage   49.62%   48.78%   -0.85%     
==========================================
  Files          51       52       +1     
  Lines        3609     3663      +54     
==========================================
- Hits         1791     1787       -4     
- Misses       1680     1726      +46     
- Partials      138      150      +12

Files with missing lines	Coverage Δ
api/v1alpha1/lvmcluster_webhook.go	`78.14% <60.00%> (-0.93%)`	⬇️
...nal/controllers/vgmanager/device_mapping_helper.go	`63.15% <63.15%> (ø)`
internal/controllers/vgmanager/lvm/lvm.go	`87.19% <46.15%> (-5.58%)`	⬇️
internal/controllers/vgmanager/controller.go	`66.73% <22.50%> (-4.16%)`	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

qJkee · 2025-10-02T13:55:16Z

/hold for reviews