-
Notifications
You must be signed in to change notification settings - Fork 40
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.
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
OCPVE-600: Implement the ability to wipe the devices #423
Conversation
@suleymanakbas91: This pull request references OCPVE-600 which is a valid jira issue. 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 kubernetes/test-infra repository. |
Skipping CI for Draft Pull Request. |
5561eae
to
cd8b6c6
Compare
18fcf17
to
b05a2a6
Compare
pkg/vgmanager/wipe_devices.go
Outdated
allPaths := append(volumeGroup.Spec.DeviceSelector.Paths, volumeGroup.Spec.DeviceSelector.OptionalPaths...) | ||
wiped := false | ||
for _, path := range allPaths { | ||
if isDeviceAlreadyPartOfVG(vgs, path, volumeGroup) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe if the device is part of the vg and the vg was already creatd once and is discoverable in the node status, we can just skip this path. However, if the device is part of a vg that is not in the node status, we have to decide if we should risk killing the existing vg that is not managed by us by wiping the disk, or even erroring. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the implementation to check isDeviceAlreadyPartOfVG
by verifying the existence from the LVMVolumeGroupNodeStatus
instead of from vgs
call. This will prevent a case where the name of a user-created volume group is the same as the one created in LVMS, and we assume that it is created by LVMS, and skip these devices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we force wipe devices, shouldn't we also force wipe the devices that were created by LVMS and leftover?
b05a2a6
to
ac0befe
Compare
ac0befe
to
84eefba
Compare
7d3abc8
to
bfebc93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits, however I think we should make the new field immutable.
volumeGroup := &v1alpha1.LVMVolumeGroup{ | ||
ObjectMeta: metav1.ObjectMeta{Name: "vg1"}, | ||
Spec: v1alpha1.LVMVolumeGroupSpec{DeviceSelector: &v1alpha1.DeviceSelector{ | ||
Paths: tt.devicePaths, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are missing optionalDevicePath cases I think. Did we test what happens when you pass a non existing devicePath to wipefs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the implementation, I just append optionalDevicePaths to devicePaths: https://github.com/openshift/lvm-operator/pull/423/files#diff-df5f27c889c2546de41e7ad40a6dbad4e9c7933a22593c529fd23977c25b9e54R19 So, I didn't think it'd make a difference to add optionalDevicePath cases to the test.
If the devicePath is non-existing, wipefs returns a failure like "No such file or directory". We just fail the reconciliation at that point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the devicePath is non-existing, wipefs returns a failure like "No such file or directory". We just fail the reconciliation at that point.
In a multi-node scenario, it is normal to have certain optionalDevices that are present on one node but not on another (imagine 2 devices with 2 nodes where each node has one of the devices, but not the other). In this case, it would always fail to wipe the devices on all nodes because at least one device would not be present.
@@ -56,6 +56,12 @@ spec: | |||
description: DeviceSelector is a set of rules that should | |||
match for a device to be included in the LVMCluster | |||
properties: | |||
forceWipeDevicesAndDestroyAllData: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should make this property immutable after creation, see https://kubernetes.io/blog/2022/09/29/enforce-immutability-using-cel/#immutability-upon-object-creation or we can use the webhook, whatever you like more
bfebc93
to
cc589f6
Compare
5f3b9e1
to
0c926a6
Compare
/retest-required |
Signed-off-by: Suleyman Akbas <[email protected]>
0c926a6
to
56f7077
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #423 +/- ##
==========================================
+ Coverage 65.80% 66.11% +0.31%
==========================================
Files 28 31 +3
Lines 2404 2497 +93
==========================================
+ Hits 1582 1651 +69
- Misses 677 694 +17
- Partials 145 152 +7
|
@suleymanakbas91: all tests passed! Full PR test history. Your PR dashboard. 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 kubernetes/test-infra repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@suleymanakbas91: This pull request references OCPVE-600 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.15.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 kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jakobmoellerdev, suleymanakbas91 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:
Approvers can indicate their approval by writing |
OCPVE-600: Implement the ability to wipe the devices
No description provided.