Skip to content

Commit

Permalink
Merge pull request #415 from jakobmoellerdev/OCPVE-677-separate-lsblk
Browse files Browse the repository at this point in the history 
OCPVE-677: chore: separate device filtering from vgmanager in lsblk package
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot committed Sep 11, 2023
2 parents 8a0a648 + 3407add commit d147fcb
Show file tree
Hide file tree
Showing 12 changed files with 252 additions and 226 deletions.
4 changes: 4 additions & 0 deletions cmd/vgmanager/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import (
"os"

lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1"
"github.com/openshift/lvm-operator/pkg/filter"
"github.com/openshift/lvm-operator/pkg/lsblk"
"github.com/openshift/lvm-operator/pkg/lvmd"
"github.com/openshift/lvm-operator/pkg/vgmanager"

Expand Down Expand Up @@ -82,8 +84,10 @@ func main() {
EventRecorder: mgr.GetEventRecorderFor(vgmanager.ControllerName),
LVMD: lvmd.DefaultConfigurator(),
Scheme: mgr.GetScheme(),
LSBLK: lsblk.NewDefaultHostLSBLK(),
NodeName: os.Getenv("NODE_NAME"),
Namespace: os.Getenv("POD_NAMESPACE"),
Filters: filter.DefaultFilters,
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "VGManager")
os.Exit(1)
Expand Down
167 changes: 91 additions & 76 deletions pkg/filter/filter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,25 @@ import (
"fmt"
"strings"

"github.com/openshift/lvm-operator/pkg/internal"
"github.com/openshift/lvm-operator/pkg/internal/exec"
"github.com/openshift/lvm-operator/pkg/lsblk"
"github.com/openshift/lvm-operator/pkg/lvm"
)

const (
// StateSuspended is a possible value of BlockDevice.State
StateSuspended = "suspended"

// DeviceTypeLoop is the device type for loop devices in lsblk output
DeviceTypeLoop = "loop"

// DeviceTypeROM is the device type for ROM devices in lsblk output
DeviceTypeROM = "rom"

// DeviceTypeLVM is the device type for lvm devices in lsblk output
DeviceTypeLVM = "lvm"
)

const (
// filter names:
notReadOnly = "notReadOnly"
Expand All @@ -36,85 +51,85 @@ const (
usableDeviceType = "usableDeviceType"
)

// maps of function identifier (for logs) to filter function.
// These are passed the localv1alpha1.DeviceInclusionSpec to make testing easier,
// but they aren't expected to use it
// they verify that the device itself is good to use
var FilterMap = map[string]func(internal.BlockDevice, internal.Executor) (bool, error){
notReadOnly: func(dev internal.BlockDevice, _ internal.Executor) (bool, error) {
return !dev.ReadOnly, nil
},

notSuspended: func(dev internal.BlockDevice, _ internal.Executor) (bool, error) {
matched := dev.State != internal.StateSuspended
return matched, nil
},

noBiosBootInPartLabel: func(dev internal.BlockDevice, _ internal.Executor) (bool, error) {
biosBootInPartLabel := strings.Contains(strings.ToLower(dev.PartLabel), strings.ToLower("bios")) ||
strings.Contains(strings.ToLower(dev.PartLabel), strings.ToLower("boot"))
return !biosBootInPartLabel, nil
},

noReservedInPartLabel: func(dev internal.BlockDevice, _ internal.Executor) (bool, error) {
reservedInPartLabel := strings.Contains(strings.ToLower(dev.PartLabel), "reserved")
return !reservedInPartLabel, nil
},

noValidFilesystemSignature: func(dev internal.BlockDevice, e internal.Executor) (bool, error) {
// if no fs type is set, it's always okay
if dev.FSType == "" {
return true, nil
}

// if fstype is set to LVM2_member then it already was created as a PV
// this means that if the disk has no children, we can safely reuse it if it's a valid LVM PV.
if dev.FSType == "LVM2_member" && !dev.HasChildren() {
pvs, err := lvm.ListPhysicalVolumes(e, "")
if err != nil {
return false, fmt.Errorf("could not determine if block device has valid filesystem signature, since it is flagged as LVM2_member but physical volumes could not be verified: %w", err)
type Filters map[string]func(lsblk.BlockDevice, exec.Executor) (bool, error)

func DefaultFilters(lsblkInstance lsblk.LSBLK) Filters {
return Filters{
notReadOnly: func(dev lsblk.BlockDevice, _ exec.Executor) (bool, error) {
return !dev.ReadOnly, nil
},

notSuspended: func(dev lsblk.BlockDevice, _ exec.Executor) (bool, error) {
matched := dev.State != StateSuspended
return matched, nil
},

noBiosBootInPartLabel: func(dev lsblk.BlockDevice, _ exec.Executor) (bool, error) {
biosBootInPartLabel := strings.Contains(strings.ToLower(dev.PartLabel), strings.ToLower("bios")) ||
strings.Contains(strings.ToLower(dev.PartLabel), strings.ToLower("boot"))
return !biosBootInPartLabel, nil
},

noReservedInPartLabel: func(dev lsblk.BlockDevice, _ exec.Executor) (bool, error) {
reservedInPartLabel := strings.Contains(strings.ToLower(dev.PartLabel), "reserved")
return !reservedInPartLabel, nil
},

noValidFilesystemSignature: func(dev lsblk.BlockDevice, e exec.Executor) (bool, error) {
// if no fs type is set, it's always okay
if dev.FSType == "" {
return true, nil
}

for _, pv := range pvs {
// a volume is a valid PV if it has the same name as the block device and no associated volume group and has available disk space
// however if there is a PV that matches the Device and there is a VG associated with it or no available space, we cannot use it
if pv.PvName == dev.KName {
if pv.VgName == "" && pv.PvFree != "0G" {
return true, nil
} else {
return false, nil
// if fstype is set to LVM2_member then it already was created as a PV
// this means that if the disk has no children, we can safely reuse it if it's a valid LVM PV.
if dev.FSType == "LVM2_member" && !dev.HasChildren() {
pvs, err := lvm.ListPhysicalVolumes(e, "")
if err != nil {
return false, fmt.Errorf("could not determine if block device has valid filesystem signature, since it is flagged as LVM2_member but physical volumes could not be verified: %w", err)
}

for _, pv := range pvs {
// a volume is a valid PV if it has the same name as the block device and no associated volume group and has available disk space
// however if there is a PV that matches the Device and there is a VG associated with it or no available space, we cannot use it
if pv.PvName == dev.KName {
if pv.VgName == "" && pv.PvFree != "0G" {
return true, nil
} else {
return false, nil
}
}
}
}

// if there was no PV that matched it and it still is flagged as LVM2_member, it is formatted but not recognized by LVM
// configuration. We can assume that in this case, the Volume can be reused by simply recalling the vgcreate command on it
return true, nil
}
return false, nil
},

noBindMounts: func(dev internal.BlockDevice, _ internal.Executor) (bool, error) {
hasBindMounts, _, err := dev.HasBindMounts()
return !hasBindMounts, err
},

noChildren: func(dev internal.BlockDevice, _ internal.Executor) (bool, error) {
hasChildren := dev.HasChildren()
return !hasChildren, nil
},

usableDeviceType: func(dev internal.BlockDevice, executor internal.Executor) (bool, error) {
switch dev.Type {
case internal.DeviceTypeLoop:
// check loop device isn't being used by kubernetes
return dev.IsUsableLoopDev(executor)
case internal.DeviceTypeROM:
return false, nil
case internal.DeviceTypeLVM:
// if there was no PV that matched it and it still is flagged as LVM2_member, it is formatted but not recognized by LVM
// configuration. We can assume that in this case, the Volume can be reused by simply recalling the vgcreate command on it
return true, nil
}
return false, nil
default:
return true, nil
}
},
},

noBindMounts: func(dev lsblk.BlockDevice, _ exec.Executor) (bool, error) {
hasBindMounts, _, err := lsblkInstance.HasBindMounts(dev)
return !hasBindMounts, err
},

noChildren: func(dev lsblk.BlockDevice, _ exec.Executor) (bool, error) {
hasChildren := dev.HasChildren()
return !hasChildren, nil
},

usableDeviceType: func(dev lsblk.BlockDevice, executor exec.Executor) (bool, error) {
switch dev.Type {
case DeviceTypeLoop:
// check loop device isn't being used by kubernetes
return lsblkInstance.IsUsableLoopDev(dev)
case DeviceTypeROM:
return false, nil
case DeviceTypeLVM:
return false, nil
default:
return true, nil
}
},
}
}
64 changes: 32 additions & 32 deletions pkg/filter/filter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,24 @@ package filter
import (
"testing"

"github.com/openshift/lvm-operator/pkg/internal"
"github.com/openshift/lvm-operator/pkg/lsblk"
"github.com/stretchr/testify/assert"
)

type filterTestCase struct {
label string
device internal.BlockDevice
device lsblk.BlockDevice
expected bool
expectErr bool
}

func TestNotReadOnly(t *testing.T) {
testcases := []filterTestCase{
{label: "tc false", device: internal.BlockDevice{ReadOnly: false}, expected: true, expectErr: false},
{label: "tc true", device: internal.BlockDevice{ReadOnly: true}, expected: false, expectErr: false},
{label: "tc false", device: lsblk.BlockDevice{ReadOnly: false}, expected: true, expectErr: false},
{label: "tc true", device: lsblk.BlockDevice{ReadOnly: true}, expected: false, expectErr: false},
}
for _, tc := range testcases {
result, err := FilterMap[notReadOnly](tc.device, nil)
result, err := DefaultFilters(nil)[notReadOnly](tc.device, nil)
assert.Equal(t, tc.expected, result)
if tc.expectErr {
assert.Error(t, err)
Expand All @@ -32,12 +32,12 @@ func TestNotReadOnly(t *testing.T) {

func TestNotSuspended(t *testing.T) {
testcases := []filterTestCase{
{label: "tc suspended", device: internal.BlockDevice{State: "suspended"}, expected: false, expectErr: false},
{label: "tc live", device: internal.BlockDevice{State: "live"}, expected: true, expectErr: false},
{label: "tc running", device: internal.BlockDevice{State: "running"}, expected: true, expectErr: false},
{label: "tc suspended", device: lsblk.BlockDevice{State: "suspended"}, expected: false, expectErr: false},
{label: "tc live", device: lsblk.BlockDevice{State: "live"}, expected: true, expectErr: false},
{label: "tc running", device: lsblk.BlockDevice{State: "running"}, expected: true, expectErr: false},
}
for _, tc := range testcases {
result, err := FilterMap[notSuspended](tc.device, nil)
result, err := DefaultFilters(nil)[notSuspended](tc.device, nil)
assert.Equal(t, tc.expected, result)
if tc.expectErr {
assert.Error(t, err)
Expand All @@ -49,12 +49,12 @@ func TestNotSuspended(t *testing.T) {

func TestNoFilesystemSignature(t *testing.T) {
testcases := []filterTestCase{
{label: "tc no fs", device: internal.BlockDevice{FSType: ""}, expected: true, expectErr: false},
{label: "tc xfs", device: internal.BlockDevice{FSType: "xfs"}, expected: false, expectErr: false},
{label: "tc swap", device: internal.BlockDevice{FSType: "swap"}, expected: false, expectErr: false},
{label: "tc no fs", device: lsblk.BlockDevice{FSType: ""}, expected: true, expectErr: false},
{label: "tc xfs", device: lsblk.BlockDevice{FSType: "xfs"}, expected: false, expectErr: false},
{label: "tc swap", device: lsblk.BlockDevice{FSType: "swap"}, expected: false, expectErr: false},
}
for _, tc := range testcases {
result, err := FilterMap[noValidFilesystemSignature](tc.device, nil)
result, err := DefaultFilters(nil)[noValidFilesystemSignature](tc.device, nil)
assert.Equal(t, tc.expected, result)
if tc.expectErr {
assert.Error(t, err)
Expand All @@ -66,11 +66,11 @@ func TestNoFilesystemSignature(t *testing.T) {

func TestNoChildren(t *testing.T) {
testcases := []filterTestCase{
{label: "tc child", device: internal.BlockDevice{Name: "dev1", Children: []internal.BlockDevice{{Name: "child1"}}}, expected: false, expectErr: false},
{label: "tc no child", device: internal.BlockDevice{Name: "dev2", Children: []internal.BlockDevice{}}, expected: true, expectErr: false},
{label: "tc child", device: lsblk.BlockDevice{Name: "dev1", Children: []lsblk.BlockDevice{{Name: "child1"}}}, expected: false, expectErr: false},
{label: "tc no child", device: lsblk.BlockDevice{Name: "dev2", Children: []lsblk.BlockDevice{}}, expected: true, expectErr: false},
}
for _, tc := range testcases {
result, err := FilterMap[noChildren](tc.device, nil)
result, err := DefaultFilters(nil)[noChildren](tc.device, nil)
assert.Equal(t, tc.expected, result)
if tc.expectErr {
assert.Error(t, err)
Expand All @@ -82,11 +82,11 @@ func TestNoChildren(t *testing.T) {

func TestIsUsableDeviceType(t *testing.T) {
testcases := []filterTestCase{
{label: "tc ROM", device: internal.BlockDevice{Name: "dev1", Type: "rom"}, expected: false, expectErr: false},
{label: "tc Disk", device: internal.BlockDevice{Name: "dev2", Type: "disk"}, expected: true, expectErr: false},
{label: "tc ROM", device: lsblk.BlockDevice{Name: "dev1", Type: "rom"}, expected: false, expectErr: false},
{label: "tc Disk", device: lsblk.BlockDevice{Name: "dev2", Type: "disk"}, expected: true, expectErr: false},
}
for _, tc := range testcases {
result, err := FilterMap[usableDeviceType](tc.device, nil)
result, err := DefaultFilters(nil)[usableDeviceType](tc.device, nil)
assert.Equal(t, tc.expected, result)
if tc.expectErr {
assert.Error(t, err)
Expand All @@ -98,15 +98,15 @@ func TestIsUsableDeviceType(t *testing.T) {

func TestNoBiosBootInPartLabel(t *testing.T) {
testcases := []filterTestCase{
{label: "tc 1", device: internal.BlockDevice{Name: "dev1", PartLabel: ""}, expected: true, expectErr: false},
{label: "tc 2", device: internal.BlockDevice{Name: "dev2", PartLabel: "abc"}, expected: true, expectErr: false},
{label: "tc 3", device: internal.BlockDevice{Name: "dev3", PartLabel: "bios"}, expected: false, expectErr: false},
{label: "tc 4", device: internal.BlockDevice{Name: "dev4", PartLabel: "BIOS"}, expected: false, expectErr: false},
{label: "tc 5", device: internal.BlockDevice{Name: "dev5", PartLabel: "boot"}, expected: false, expectErr: false},
{label: "tc 6", device: internal.BlockDevice{Name: "dev6", PartLabel: "BOOT"}, expected: false, expectErr: false},
{label: "tc 1", device: lsblk.BlockDevice{Name: "dev1", PartLabel: ""}, expected: true, expectErr: false},
{label: "tc 2", device: lsblk.BlockDevice{Name: "dev2", PartLabel: "abc"}, expected: true, expectErr: false},
{label: "tc 3", device: lsblk.BlockDevice{Name: "dev3", PartLabel: "bios"}, expected: false, expectErr: false},
{label: "tc 4", device: lsblk.BlockDevice{Name: "dev4", PartLabel: "BIOS"}, expected: false, expectErr: false},
{label: "tc 5", device: lsblk.BlockDevice{Name: "dev5", PartLabel: "boot"}, expected: false, expectErr: false},
{label: "tc 6", device: lsblk.BlockDevice{Name: "dev6", PartLabel: "BOOT"}, expected: false, expectErr: false},
}
for _, tc := range testcases {
result, err := FilterMap[noBiosBootInPartLabel](tc.device, nil)
result, err := DefaultFilters(nil)[noBiosBootInPartLabel](tc.device, nil)
assert.Equal(t, tc.expected, result)
if tc.expectErr {
assert.Error(t, err)
Expand All @@ -118,14 +118,14 @@ func TestNoBiosBootInPartLabel(t *testing.T) {

func TestNoReservedInPartLabel(t *testing.T) {
testcases := []filterTestCase{
{label: "tc 1", device: internal.BlockDevice{Name: "dev1", PartLabel: ""}, expected: true},
{label: "tc 2", device: internal.BlockDevice{Name: "dev2", PartLabel: "abc"}, expected: true},
{label: "tc 3", device: internal.BlockDevice{Name: "dev3", PartLabel: "reserved"}, expected: false},
{label: "tc 4", device: internal.BlockDevice{Name: "dev4", PartLabel: "RESERVED"}, expected: false},
{label: "tc 5", device: internal.BlockDevice{Name: "dev5", PartLabel: "Reserved"}, expected: false},
{label: "tc 1", device: lsblk.BlockDevice{Name: "dev1", PartLabel: ""}, expected: true},
{label: "tc 2", device: lsblk.BlockDevice{Name: "dev2", PartLabel: "abc"}, expected: true},
{label: "tc 3", device: lsblk.BlockDevice{Name: "dev3", PartLabel: "reserved"}, expected: false},
{label: "tc 4", device: lsblk.BlockDevice{Name: "dev4", PartLabel: "RESERVED"}, expected: false},
{label: "tc 5", device: lsblk.BlockDevice{Name: "dev5", PartLabel: "Reserved"}, expected: false},
}
for _, tc := range testcases {
result, err := FilterMap[noReservedInPartLabel](tc.device, nil)
result, err := DefaultFilters(nil)[noReservedInPartLabel](tc.device, nil)
assert.NoError(t, err)
assert.Equal(t, tc.expected, result)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/internal/exec.go → pkg/internal/exec/exec.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package internal
package exec

import (
"fmt"
Expand Down
0 pkg/internal/test/mock_exec.go → pkg/internal/exec/test/mock_exec.go
View file
File renamed without changes.
Loading

0 comments on commit d147fcb

Please sign in to comment.