From d1535e14e48e81c6a68f824754701b3409816c22 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jakob=20M=C3=B6ller?= <jmoller@redhat.com>
Date: Mon, 11 Sep 2023 10:55:34 +0200
Subject: [PATCH 1/2] chore: separate device filtering from vgmanager in lsblk
 package

---
 cmd/vgmanager/main.go                         |   4 +
 pkg/filter/filter.go                          | 165 ++++++++++--------
 pkg/filter/filter_test.go                     |  64 +++----
 .../block_device.go => lsblk/lsblk.go}        |  96 +++++-----
 pkg/vgmanager/devices.go                      |  29 ++-
 pkg/vgmanager/devices_test.go                 |  53 +++---
 pkg/vgmanager/vgmanager_controller.go         |   8 +-
 7 files changed, 222 insertions(+), 197 deletions(-)
 rename pkg/{internal/block_device.go => lsblk/lsblk.go} (66%)

diff --git a/cmd/vgmanager/main.go b/cmd/vgmanager/main.go
index e9747c543..6a79a9183 100644
--- a/cmd/vgmanager/main.go
+++ b/cmd/vgmanager/main.go
@@ -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"
 
@@ -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)
diff --git a/pkg/filter/filter.go b/pkg/filter/filter.go
index bcf2be86d..6ecb2efc3 100644
--- a/pkg/filter/filter.go
+++ b/pkg/filter/filter.go
@@ -21,9 +21,24 @@ import (
 	"strings"
 
 	"github.com/openshift/lvm-operator/pkg/internal"
+	"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"
@@ -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, internal.Executor) (bool, error)
+
+func DefaultFilters(lsblkInstance lsblk.LSBLK) Filters {
+	return Filters{
+		notReadOnly: func(dev lsblk.BlockDevice, _ internal.Executor) (bool, error) {
+			return !dev.ReadOnly, nil
+		},
+
+		notSuspended: func(dev lsblk.BlockDevice, _ internal.Executor) (bool, error) {
+			matched := dev.State != StateSuspended
+			return matched, nil
+		},
+
+		noBiosBootInPartLabel: func(dev lsblk.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 lsblk.BlockDevice, _ internal.Executor) (bool, error) {
+			reservedInPartLabel := strings.Contains(strings.ToLower(dev.PartLabel), "reserved")
+			return !reservedInPartLabel, nil
+		},
+
+		noValidFilesystemSignature: func(dev lsblk.BlockDevice, e internal.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, _ internal.Executor) (bool, error) {
+			hasBindMounts, _, err := lsblkInstance.HasBindMounts(dev)
+			return !hasBindMounts, err
+		},
+
+		noChildren: func(dev lsblk.BlockDevice, _ internal.Executor) (bool, error) {
+			hasChildren := dev.HasChildren()
+			return !hasChildren, nil
+		},
+
+		usableDeviceType: func(dev lsblk.BlockDevice, executor internal.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
+			}
+		},
+	}
 }
diff --git a/pkg/filter/filter_test.go b/pkg/filter/filter_test.go
index f4b7854a9..899e98ccd 100644
--- a/pkg/filter/filter_test.go
+++ b/pkg/filter/filter_test.go
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
 	}
diff --git a/pkg/internal/block_device.go b/pkg/lsblk/lsblk.go
similarity index 66%
rename from pkg/internal/block_device.go
rename to pkg/lsblk/lsblk.go
index 73c514121..b3c917a2d 100644
--- a/pkg/internal/block_device.go
+++ b/pkg/lsblk/lsblk.go
@@ -1,20 +1,4 @@
-/*
-Copyright © 2023 Red Hat, Inc.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package internal
+package lsblk
 
 import (
 	"encoding/json"
@@ -22,25 +6,17 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+
+	"github.com/openshift/lvm-operator/pkg/internal"
 )
 
 var (
-	mountFile = "/proc/1/mountinfo"
+	DefaultMountinfo = "/proc/1/mountinfo"
+	DefaultLosetup   = "/usr/sbin/losetup"
+	DefaultLsblk     = "/usr/bin/lsblk"
 )
 
 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"
-
 	// mount string to find if a path is part of kubernetes
 	pluginString = "plugins/kubernetes.io"
 )
@@ -66,14 +42,46 @@ type BlockDevice struct {
 	DevicePath string
 }
 
+type LSBLK interface {
+	ListBlockDevices() ([]BlockDevice, error)
+	IsUsableLoopDev(b BlockDevice) (bool, error)
+	HasBindMounts(b BlockDevice) (bool, string, error)
+}
+
+type HostLSBLK struct {
+	internal.Executor
+	lsblk     string
+	mountInfo string
+	losetup   string
+}
+
+func NewDefaultHostLSBLK() *HostLSBLK {
+	return NewHostLSBLK(&internal.CommandExecutor{}, DefaultLsblk, DefaultMountinfo, DefaultLosetup)
+}
+
+func NewHostLSBLK(executor internal.Executor, lsblk, mountInfo, losetup string) *HostLSBLK {
+	hostLsblk := &HostLSBLK{
+		lsblk:     lsblk,
+		Executor:  executor,
+		mountInfo: mountInfo,
+		losetup:   losetup,
+	}
+	return hostLsblk
+}
+
+// HasChildren checks if the disk has partitions
+func (b BlockDevice) HasChildren() bool {
+	return len(b.Children) > 0
+}
+
 // ListBlockDevices lists the block devices using the lsblk command
-func ListBlockDevices(exec Executor) ([]BlockDevice, error) {
+func (lsblk *HostLSBLK) ListBlockDevices() ([]BlockDevice, error) {
 	// var output bytes.Buffer
 	var blockDeviceMap map[string][]BlockDevice
 	columns := "NAME,ROTA,TYPE,SIZE,MODEL,VENDOR,RO,STATE,KNAME,SERIAL,PARTLABEL,FSTYPE"
 	args := []string{"--json", "--paths", "-o", columns}
 
-	output, err := exec.ExecuteCommandWithOutput("lsblk", args...)
+	output, err := lsblk.ExecuteCommandWithOutput(lsblk.lsblk, args...)
 	if err != nil {
 		return []BlockDevice{}, err
 	}
@@ -89,46 +97,40 @@ func ListBlockDevices(exec Executor) ([]BlockDevice, error) {
 // IsUsableLoopDev returns true if the loop device isn't in use by Kubernetes
 // by matching the back file path against a standard string used to mount devices
 // from host into pods
-func (b BlockDevice) IsUsableLoopDev(exec Executor) (bool, error) {
+func (lsblk *HostLSBLK) IsUsableLoopDev(b BlockDevice) (bool, error) {
 	// holds back-file string of the loop device
 	var loopDeviceMap map[string][]struct {
 		BackFile string `json:"back-file"`
 	}
 
-	usable := true
 	args := []string{b.Name, "-O", "BACK-FILE", "--json"}
-	output, err := exec.ExecuteCommandWithOutput(losetupPath, args...)
+	output, err := lsblk.ExecuteCommandWithOutput(lsblk.losetup, args...)
 	if err != nil {
-		return usable, err
+		return true, err
 	}
 
 	err = json.Unmarshal([]byte(output), &loopDeviceMap)
 	if err != nil {
-		return usable, err
+		return false, err
 	}
 
 	for _, backFile := range loopDeviceMap["loopdevices"] {
 		if strings.Contains(backFile.BackFile, pluginString) {
 			// this loop device is being used by kubernetes and can't be
 			// added to volume group
-			usable = false
+			return false, nil
 		}
 	}
 
-	return usable, nil
-}
-
-// HasChildren checks if the disk has partitions
-func (b BlockDevice) HasChildren() bool {
-	return len(b.Children) > 0
+	return true, nil
 }
 
 // HasBindMounts checks for bind mounts and returns mount point for a device by parsing `proc/1/mountinfo`.
 // HostPID should be set to true inside the POD spec to get details of host's mount points inside `proc/1/mountinfo`.
-func (b BlockDevice) HasBindMounts() (bool, string, error) {
-	data, err := os.ReadFile(mountFile)
+func (lsblk *HostLSBLK) HasBindMounts(b BlockDevice) (bool, string, error) {
+	data, err := os.ReadFile(lsblk.mountInfo)
 	if err != nil {
-		return false, "", fmt.Errorf("failed to read file %s: %v", mountFile, err)
+		return false, "", fmt.Errorf("failed to read file %s: %v", lsblk.mountInfo, err)
 	}
 
 	mountString := string(data)
diff --git a/pkg/vgmanager/devices.go b/pkg/vgmanager/devices.go
index c23e69566..81298c9bf 100644
--- a/pkg/vgmanager/devices.go
+++ b/pkg/vgmanager/devices.go
@@ -23,14 +23,13 @@ import (
 	"path/filepath"
 
 	lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1"
-	"github.com/openshift/lvm-operator/pkg/filter"
-	"github.com/openshift/lvm-operator/pkg/internal"
+	"github.com/openshift/lvm-operator/pkg/lsblk"
 	"github.com/openshift/lvm-operator/pkg/lvm"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
 // addDevicesToVG creates or extends a volume group using the provided devices.
-func (r *VGReconciler) addDevicesToVG(ctx context.Context, vgs []lvm.VolumeGroup, vgName string, devices []internal.BlockDevice) error {
+func (r *VGReconciler) addDevicesToVG(ctx context.Context, vgs []lvm.VolumeGroup, vgName string, devices []lsblk.BlockDevice) error {
 	logger := log.FromContext(ctx)
 
 	if len(devices) < 1 {
@@ -74,7 +73,7 @@ func (r *VGReconciler) addDevicesToVG(ctx context.Context, vgs []lvm.VolumeGroup
 }
 
 // getAvailableDevicesForVG determines the available devices that can be used to create a volume group.
-func (r *VGReconciler) getAvailableDevicesForVG(ctx context.Context, blockDevices []internal.BlockDevice, vgs []lvm.VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]internal.BlockDevice, error) {
+func (r *VGReconciler) getAvailableDevicesForVG(ctx context.Context, blockDevices []lsblk.BlockDevice, vgs []lvm.VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]lsblk.BlockDevice, error) {
 	// filter devices based on DeviceSelector.Paths if specified
 	availableDevices, err := r.filterMatchingDevices(ctx, blockDevices, vgs, volumeGroup)
 	if err != nil {
@@ -86,10 +85,10 @@ func (r *VGReconciler) getAvailableDevicesForVG(ctx context.Context, blockDevice
 
 // filterAvailableDevices returns:
 // availableDevices: the list of blockdevices considered available
-func (r *VGReconciler) filterAvailableDevices(ctx context.Context, blockDevices []internal.BlockDevice) []internal.BlockDevice {
+func (r *VGReconciler) filterAvailableDevices(ctx context.Context, blockDevices []lsblk.BlockDevice) []lsblk.BlockDevice {
 	logger := log.FromContext(ctx)
 
-	var availableDevices []internal.BlockDevice
+	var availableDevices []lsblk.BlockDevice
 	// using a label so `continue DeviceLoop` can be used to skip devices
 DeviceLoop:
 	for _, blockDevice := range blockDevices {
@@ -100,7 +99,7 @@ DeviceLoop:
 		}
 
 		logger = logger.WithValues("Device.Name", blockDevice.Name)
-		for name, filterFunc := range filter.FilterMap {
+		for name, filterFunc := range r.Filters(r.LSBLK) {
 			logger := logger.WithValues("filterFunc.Name", name)
 			valid, err := filterFunc(blockDevice, r.executor)
 			if err != nil {
@@ -117,10 +116,10 @@ DeviceLoop:
 }
 
 // filterMatchingDevices filters devices based on DeviceSelector.Paths if specified.
-func (r *VGReconciler) filterMatchingDevices(ctx context.Context, blockDevices []internal.BlockDevice, vgs []lvm.VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]internal.BlockDevice, error) {
+func (r *VGReconciler) filterMatchingDevices(ctx context.Context, blockDevices []lsblk.BlockDevice, vgs []lvm.VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]lsblk.BlockDevice, error) {
 	logger := log.FromContext(ctx)
 
-	var filteredBlockDevices []internal.BlockDevice
+	var filteredBlockDevices []lsblk.BlockDevice
 	devicesAlreadyInVG := false
 
 	if volumeGroup.Spec.DeviceSelector != nil {
@@ -202,7 +201,7 @@ func isDeviceAlreadyPartOfVG(vgs []lvm.VolumeGroup, diskName string, volumeGroup
 	return false
 }
 
-func hasExactDisk(blockDevices []internal.BlockDevice, deviceName string) (internal.BlockDevice, bool) {
+func hasExactDisk(blockDevices []lsblk.BlockDevice, deviceName string) (lsblk.BlockDevice, bool) {
 	for _, blockDevice := range blockDevices {
 		if blockDevice.KName == deviceName {
 			return blockDevice, true
@@ -213,7 +212,7 @@ func hasExactDisk(blockDevices []internal.BlockDevice, deviceName string) (inter
 			}
 		}
 	}
-	return internal.BlockDevice{}, false
+	return lsblk.BlockDevice{}, false
 }
 
 func checkDuplicateDeviceSelectorPaths(selector *lvmv1alpha1.DeviceSelector) error {
@@ -257,22 +256,22 @@ func checkDuplicateDeviceSelectorPaths(selector *lvmv1alpha1.DeviceSelector) err
 //
 //	An error will be returned if the device is invalid
 //	No error and an empty BlockDevice object will be returned if this device should be skipped (ex: duplicate device)
-func getValidDevice(devicePath string, blockDevices []internal.BlockDevice, vgs []lvm.VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) (internal.BlockDevice, error) {
+func getValidDevice(devicePath string, blockDevices []lsblk.BlockDevice, vgs []lvm.VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) (lsblk.BlockDevice, error) {
 	// Make sure the symlink exists
 	diskName, err := filepath.EvalSymlinks(devicePath)
 	if err != nil {
-		return internal.BlockDevice{}, fmt.Errorf("unable to find symlink for disk path %s: %v", devicePath, err)
+		return lsblk.BlockDevice{}, fmt.Errorf("unable to find symlink for disk path %s: %v", devicePath, err)
 	}
 
 	// Make sure this isn't a duplicate in the VG
 	if isDeviceAlreadyPartOfVG(vgs, diskName, volumeGroup) {
-		return internal.BlockDevice{}, nil // No error, we just don't want a duplicate
+		return lsblk.BlockDevice{}, nil // No error, we just don't want a duplicate
 	}
 
 	// Make sure the block device exists
 	blockDevice, ok := hasExactDisk(blockDevices, diskName)
 	if !ok {
-		return internal.BlockDevice{}, fmt.Errorf("can not find device name %s in the available block devices", devicePath)
+		return lsblk.BlockDevice{}, fmt.Errorf("can not find device name %s in the available block devices", devicePath)
 	}
 
 	blockDevice.DevicePath = devicePath
diff --git a/pkg/vgmanager/devices_test.go b/pkg/vgmanager/devices_test.go
index 7ec619a67..b4f0bb826 100644
--- a/pkg/vgmanager/devices_test.go
+++ b/pkg/vgmanager/devices_test.go
@@ -10,7 +10,7 @@ import (
 	"github.com/go-logr/logr/testr"
 	"github.com/openshift/lvm-operator/api/v1alpha1"
 	"github.com/openshift/lvm-operator/pkg/filter"
-	"github.com/openshift/lvm-operator/pkg/internal"
+	"github.com/openshift/lvm-operator/pkg/lsblk"
 	"github.com/openshift/lvm-operator/pkg/lvm"
 	"github.com/stretchr/testify/assert"
 	"sigs.k8s.io/controller-runtime/pkg/log"
@@ -34,14 +34,17 @@ func TestAvailableDevicesForVG(t *testing.T) {
 	}
 
 	r := &VGReconciler{}
-
-	// remove noBindMounts filter as it reads `proc/1/mountinfo` file.
-	delete(filter.FilterMap, "noBindMounts")
+	r.Filters = func(instance lsblk.LSBLK) filter.Filters {
+		filters := filter.DefaultFilters(instance)
+		// remove noBindMounts filter as it reads `proc/1/mountinfo` file.
+		delete(filters, "noBindMounts")
+		return filters
+	}
 
 	testCases := []struct {
 		description           string
 		volumeGroup           v1alpha1.LVMVolumeGroup
-		existingBlockDevices  []internal.BlockDevice
+		existingBlockDevices  []lsblk.BlockDevice
 		existingVGs           []lvm.VolumeGroup
 		numOfAvailableDevices int
 		expectError           bool
@@ -53,7 +56,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					Name: "vg1",
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:     "/dev/nvme1n1",
 					Type:     "disk",
@@ -72,7 +75,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					Name: "vg1",
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:     "/dev/nvme1n1",
 					Type:     "disk",
@@ -91,7 +94,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					Name: "vg1",
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:     "/dev/nvme1n1",
 					Type:     "disk",
@@ -110,7 +113,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					Name: "vg1",
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:      "/dev/nvme1n1",
 					Type:      "disk",
@@ -130,7 +133,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					Name: "vg1",
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:      "/dev/nvme1n1",
 					Type:      "disk",
@@ -150,7 +153,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					Name: "vg1",
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:     "/dev/nvme1n1",
 					Type:     "disk",
@@ -170,7 +173,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					Name: "vg1",
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:     "/dev/nvme1n1",
 					Type:     "disk",
@@ -178,7 +181,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					ReadOnly: false,
 					State:    "live",
 					KName:    "/dev/nvme1n1",
-					Children: []internal.BlockDevice{
+					Children: []lsblk.BlockDevice{
 						{
 							Name:     "/dev/nvme1n1p1",
 							ReadOnly: true,
@@ -195,7 +198,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					Name: "vg1",
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:     "/dev/nvme1n1",
 					Type:     "disk",
@@ -203,7 +206,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					ReadOnly: false,
 					State:    "live",
 					KName:    "/dev/nvme1n1",
-					Children: []internal.BlockDevice{
+					Children: []lsblk.BlockDevice{
 						{
 							Name:     "/dev/nvme1n1p1",
 							Type:     "disk",
@@ -239,7 +242,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					},
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:     "nvme1n1p1",
 					KName:    calculateDevicePath(t, "nvme1n1p1"),
@@ -265,7 +268,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					},
 				},
 			},
-			existingBlockDevices:  []internal.BlockDevice{},
+			existingBlockDevices:  []lsblk.BlockDevice{},
 			numOfAvailableDevices: 0,
 			expectError:           true,
 		},
@@ -283,7 +286,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					},
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:     "nvme1n1p1",
 					KName:    calculateDevicePath(t, "nvme1n1p1"),
@@ -321,7 +324,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					},
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:     "nvme1n1p1",
 					KName:    calculateDevicePath(t, "nvme1n1p1"),
@@ -361,7 +364,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					Name: "vg1",
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:     "nvme1n1p1",
 					KName:    calculateDevicePath(t, "nvme1n1p1"),
@@ -393,7 +396,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					Name: "vg1",
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:     "nvme1n1p1",
 					KName:    calculateDevicePath(t, "nvme1n1p1"),
@@ -401,7 +404,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					Size:     "279.4G",
 					ReadOnly: false,
 					State:    "live",
-					Children: []internal.BlockDevice{
+					Children: []lsblk.BlockDevice{
 						{
 							Name:     "nvme1n1p2",
 							KName:    calculateDevicePath(t, "nvme1n1p2"),
@@ -433,7 +436,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					},
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:     "nvme1n1p1",
 					KName:    calculateDevicePath(t, "nvme1n1p1"),
@@ -469,7 +472,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					},
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:     "nvme1n1p1",
 					KName:    calculateDevicePath(t, "nvme1n1p1"),
@@ -496,7 +499,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
 					},
 				},
 			},
-			existingBlockDevices: []internal.BlockDevice{
+			existingBlockDevices: []lsblk.BlockDevice{
 				{
 					Name:     "nvme1n1p1",
 					KName:    calculateDevicePath(t, "nvme1n1p1"),
diff --git a/pkg/vgmanager/vgmanager_controller.go b/pkg/vgmanager/vgmanager_controller.go
index 8b272f9f6..153f348fc 100644
--- a/pkg/vgmanager/vgmanager_controller.go
+++ b/pkg/vgmanager/vgmanager_controller.go
@@ -27,7 +27,9 @@ import (
 
 	lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1"
 	"github.com/openshift/lvm-operator/controllers"
+	"github.com/openshift/lvm-operator/pkg/filter"
 	"github.com/openshift/lvm-operator/pkg/internal"
+	"github.com/openshift/lvm-operator/pkg/lsblk"
 	"github.com/openshift/lvm-operator/pkg/lvm"
 	"github.com/openshift/lvm-operator/pkg/lvmd"
 
@@ -87,8 +89,10 @@ type VGReconciler struct {
 	Scheme    *runtime.Scheme
 	executor  internal.Executor
 	LVMD      lvmd.Configurator
+	lsblk.LSBLK
 	NodeName  string
 	Namespace string
+	Filters   func(lsblk.LSBLK) filter.Filters
 }
 
 func (r *VGReconciler) getFinalizer() string {
@@ -126,8 +130,6 @@ func (r *VGReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Re
 			return ctrl.Result{}, fmt.Errorf("could not determine if LVMVolumeGroupNodeStatus still needs to be created: %w", err)
 		}
 	}
-
-	r.executor = &internal.CommandExecutor{}
 	return r.reconcile(ctx, volumeGroup)
 }
 
@@ -168,7 +170,7 @@ func (r *VGReconciler) reconcile(ctx context.Context, volumeGroup *lvmv1alpha1.L
 		return ctrl.Result{}, fmt.Errorf("failed to list volume groups: %w", err)
 	}
 
-	blockDevices, err := internal.ListBlockDevices(r.executor)
+	blockDevices, err := r.ListBlockDevices()
 	if err != nil {
 		return ctrl.Result{}, fmt.Errorf("failed to list block devices: %w", err)
 	}

From 3407addf661da0c4dd345af3ffc608c5f326e7ca Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jakob=20M=C3=B6ller?= <jmoller@redhat.com>
Date: Mon, 11 Sep 2023 13:16:22 +0200
Subject: [PATCH 2/2] chore: move and initialize exec correctly

---
 pkg/filter/filter.go                       | 20 ++++++------
 pkg/internal/{ => exec}/exec.go            |  2 +-
 pkg/internal/{ => exec}/test/mock_exec.go  |  0
 pkg/lsblk/lsblk.go                         |  8 ++---
 pkg/lvm/lvm.go                             | 36 +++++++++++-----------
 pkg/lvm/lvm_test.go                        |  2 +-
 pkg/vgmanager/vgmanager_controller.go      |  9 +++---
 pkg/vgmanager/vgmanager_controller_test.go |  8 ++---
 8 files changed, 43 insertions(+), 42 deletions(-)
 rename pkg/internal/{ => exec}/exec.go (99%)
 rename pkg/internal/{ => exec}/test/mock_exec.go (100%)

diff --git a/pkg/filter/filter.go b/pkg/filter/filter.go
index 6ecb2efc3..a9eb4df25 100644
--- a/pkg/filter/filter.go
+++ b/pkg/filter/filter.go
@@ -20,7 +20,7 @@ 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"
 )
@@ -51,31 +51,31 @@ const (
 	usableDeviceType           = "usableDeviceType"
 )
 
-type Filters map[string]func(lsblk.BlockDevice, internal.Executor) (bool, error)
+type Filters map[string]func(lsblk.BlockDevice, exec.Executor) (bool, error)
 
 func DefaultFilters(lsblkInstance lsblk.LSBLK) Filters {
 	return Filters{
-		notReadOnly: func(dev lsblk.BlockDevice, _ internal.Executor) (bool, error) {
+		notReadOnly: func(dev lsblk.BlockDevice, _ exec.Executor) (bool, error) {
 			return !dev.ReadOnly, nil
 		},
 
-		notSuspended: func(dev lsblk.BlockDevice, _ internal.Executor) (bool, error) {
+		notSuspended: func(dev lsblk.BlockDevice, _ exec.Executor) (bool, error) {
 			matched := dev.State != StateSuspended
 			return matched, nil
 		},
 
-		noBiosBootInPartLabel: func(dev lsblk.BlockDevice, _ internal.Executor) (bool, error) {
+		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, _ internal.Executor) (bool, error) {
+		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 internal.Executor) (bool, error) {
+		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
@@ -108,17 +108,17 @@ func DefaultFilters(lsblkInstance lsblk.LSBLK) Filters {
 			return false, nil
 		},
 
-		noBindMounts: func(dev lsblk.BlockDevice, _ internal.Executor) (bool, error) {
+		noBindMounts: func(dev lsblk.BlockDevice, _ exec.Executor) (bool, error) {
 			hasBindMounts, _, err := lsblkInstance.HasBindMounts(dev)
 			return !hasBindMounts, err
 		},
 
-		noChildren: func(dev lsblk.BlockDevice, _ internal.Executor) (bool, error) {
+		noChildren: func(dev lsblk.BlockDevice, _ exec.Executor) (bool, error) {
 			hasChildren := dev.HasChildren()
 			return !hasChildren, nil
 		},
 
-		usableDeviceType: func(dev lsblk.BlockDevice, executor internal.Executor) (bool, error) {
+		usableDeviceType: func(dev lsblk.BlockDevice, executor exec.Executor) (bool, error) {
 			switch dev.Type {
 			case DeviceTypeLoop:
 				// check loop device isn't being used by kubernetes
diff --git a/pkg/internal/exec.go b/pkg/internal/exec/exec.go
similarity index 99%
rename from pkg/internal/exec.go
rename to pkg/internal/exec/exec.go
index 44ea9ae3c..c1ddcad48 100644
--- a/pkg/internal/exec.go
+++ b/pkg/internal/exec/exec.go
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package internal
+package exec
 
 import (
 	"fmt"
diff --git a/pkg/internal/test/mock_exec.go b/pkg/internal/exec/test/mock_exec.go
similarity index 100%
rename from pkg/internal/test/mock_exec.go
rename to pkg/internal/exec/test/mock_exec.go
diff --git a/pkg/lsblk/lsblk.go b/pkg/lsblk/lsblk.go
index b3c917a2d..91ca12c1e 100644
--- a/pkg/lsblk/lsblk.go
+++ b/pkg/lsblk/lsblk.go
@@ -7,7 +7,7 @@ import (
 	"path/filepath"
 	"strings"
 
-	"github.com/openshift/lvm-operator/pkg/internal"
+	"github.com/openshift/lvm-operator/pkg/internal/exec"
 )
 
 var (
@@ -49,17 +49,17 @@ type LSBLK interface {
 }
 
 type HostLSBLK struct {
-	internal.Executor
+	exec.Executor
 	lsblk     string
 	mountInfo string
 	losetup   string
 }
 
 func NewDefaultHostLSBLK() *HostLSBLK {
-	return NewHostLSBLK(&internal.CommandExecutor{}, DefaultLsblk, DefaultMountinfo, DefaultLosetup)
+	return NewHostLSBLK(&exec.CommandExecutor{}, DefaultLsblk, DefaultMountinfo, DefaultLosetup)
 }
 
-func NewHostLSBLK(executor internal.Executor, lsblk, mountInfo, losetup string) *HostLSBLK {
+func NewHostLSBLK(executor exec.Executor, lsblk, mountInfo, losetup string) *HostLSBLK {
 	hostLsblk := &HostLSBLK{
 		lsblk:     lsblk,
 		Executor:  executor,
diff --git a/pkg/lvm/lvm.go b/pkg/lvm/lvm.go
index 517fab2ca..a198e7c91 100644
--- a/pkg/lvm/lvm.go
+++ b/pkg/lvm/lvm.go
@@ -20,10 +20,10 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"os/exec"
+	osexec "os/exec"
 	"strings"
 
-	"github.com/openshift/lvm-operator/pkg/internal"
+	"github.com/openshift/lvm-operator/pkg/internal/exec"
 )
 
 type lvmError string
@@ -119,7 +119,7 @@ type PhysicalVolume struct {
 }
 
 // Create creates a new volume group
-func (vg VolumeGroup) Create(exec internal.Executor, pvs []string) error {
+func (vg VolumeGroup) Create(exec exec.Executor, pvs []string) error {
 	if vg.Name == "" {
 		return fmt.Errorf("failed to create volume group. Volume group name is empty")
 	}
@@ -140,7 +140,7 @@ func (vg VolumeGroup) Create(exec internal.Executor, pvs []string) error {
 }
 
 // Extend extends the volume group only if new physical volumes are available
-func (vg VolumeGroup) Extend(exec internal.Executor, pvs []string) error {
+func (vg VolumeGroup) Extend(exec exec.Executor, pvs []string) error {
 	if vg.Name == "" {
 		return fmt.Errorf("failed to extend volume group. Volume group name is empty")
 	}
@@ -161,7 +161,7 @@ func (vg VolumeGroup) Extend(exec internal.Executor, pvs []string) error {
 }
 
 // CreateVG creates a new volume group
-func (vg VolumeGroup) CreateVG(exec internal.Executor) error {
+func (vg VolumeGroup) CreateVG(exec exec.Executor) error {
 	if vg.Name == "" {
 		return fmt.Errorf("failed to create volume group. Volume group name is empty")
 	}
@@ -185,7 +185,7 @@ func (vg VolumeGroup) CreateVG(exec internal.Executor) error {
 }
 
 // ExtendVG extends the volume group only if new physical volumes are available
-func (vg VolumeGroup) ExtendVG(exec internal.Executor, pvs []string) (VolumeGroup, error) {
+func (vg VolumeGroup) ExtendVG(exec exec.Executor, pvs []string) (VolumeGroup, error) {
 	if vg.Name == "" {
 		return VolumeGroup{}, fmt.Errorf("failed to extend volume group. Volume group name is empty")
 	}
@@ -210,7 +210,7 @@ func (vg VolumeGroup) ExtendVG(exec internal.Executor, pvs []string) (VolumeGrou
 }
 
 // Delete deletes a volume group and the physical volumes associated with it
-func (vg VolumeGroup) Delete(e internal.Executor) error {
+func (vg VolumeGroup) Delete(e exec.Executor) error {
 	// Remove Volume Group
 	vgArgs := []string{vg.Name}
 	_, err := e.ExecuteCommandWithOutputAsHost(vgRemoveCmd, vgArgs...)
@@ -231,7 +231,7 @@ func (vg VolumeGroup) Delete(e internal.Executor) error {
 	for _, pv := range vg.PVs {
 		_, err = e.ExecuteCommandWithOutput(lvmDevicesCmd, "--delpvid", pv.UUID)
 		if err != nil {
-			var exitError *exec.ExitError
+			var exitError *osexec.ExitError
 			if errors.As(err, &exitError) {
 				switch exitError.ExitCode() {
 				// Exit Code 5 On lvmdevices --delpvid means that the PV with that UUID no longer exists
@@ -247,7 +247,7 @@ func (vg VolumeGroup) Delete(e internal.Executor) error {
 }
 
 // GetVolumeGroup returns a volume group along with the associated physical volumes
-func GetVolumeGroup(exec internal.Executor, name string) (*VolumeGroup, error) {
+func GetVolumeGroup(exec exec.Executor, name string) (*VolumeGroup, error) {
 	res := new(vgsOutput)
 
 	args := []string{
@@ -285,7 +285,7 @@ func GetVolumeGroup(exec internal.Executor, name string) (*VolumeGroup, error) {
 }
 
 // ListPhysicalVolumes returns list of physical volumes used to create the given volume group
-func ListPhysicalVolumes(exec internal.Executor, vgName string) ([]PhysicalVolume, error) {
+func ListPhysicalVolumes(exec exec.Executor, vgName string) ([]PhysicalVolume, error) {
 	res := new(pvsOutput)
 	args := []string{
 		"pvs", "--units", "g", "-v", "--reportformat", "json",
@@ -317,7 +317,7 @@ func ListPhysicalVolumes(exec internal.Executor, vgName string) ([]PhysicalVolum
 }
 
 // ListVolumeGroups lists all volume groups and the physical volumes associated with them.
-func ListVolumeGroups(exec internal.Executor) ([]VolumeGroup, error) {
+func ListVolumeGroups(exec exec.Executor) ([]VolumeGroup, error) {
 	res := new(vgsOutput)
 	args := []string{
 		"vgs", "--reportformat", "json",
@@ -347,7 +347,7 @@ func ListVolumeGroups(exec internal.Executor) ([]VolumeGroup, error) {
 }
 
 // ListLogicalVolumes returns list of logical volumes for a volume group
-func ListLogicalVolumes(exec internal.Executor, vgName string) ([]string, error) {
+func ListLogicalVolumes(exec exec.Executor, vgName string) ([]string, error) {
 	res, err := GetLVSOutput(exec, vgName)
 	if err != nil {
 		return []string{}, err
@@ -363,7 +363,7 @@ func ListLogicalVolumes(exec internal.Executor, vgName string) ([]string, error)
 }
 
 // GetLVSOutput returns the output for `lvs` command in json format
-func GetLVSOutput(exec internal.Executor, vgName string) (*lvsOutput, error) {
+func GetLVSOutput(exec exec.Executor, vgName string) (*lvsOutput, error) {
 	res := new(lvsOutput)
 	args := []string{
 		"lvs", "-S", fmt.Sprintf("vgname=%s", vgName), "--units", "g", "--reportformat", "json",
@@ -376,7 +376,7 @@ func GetLVSOutput(exec internal.Executor, vgName string) (*lvsOutput, error) {
 }
 
 // LVExists checks if a logical volume exists in a volume group
-func LVExists(exec internal.Executor, lvName, vgName string) (bool, error) {
+func LVExists(exec exec.Executor, lvName, vgName string) (bool, error) {
 	lvs, err := ListLogicalVolumes(exec, vgName)
 	if err != nil {
 		return false, err
@@ -392,7 +392,7 @@ func LVExists(exec internal.Executor, lvName, vgName string) (bool, error) {
 }
 
 // DeleteLV deactivates the logical volume and deletes it
-func DeleteLV(exec internal.Executor, lvName, vgName string) error {
+func DeleteLV(exec exec.Executor, lvName, vgName string) error {
 	lv := fmt.Sprintf("%s/%s", vgName, lvName)
 
 	// deactivate logical volume
@@ -411,7 +411,7 @@ func DeleteLV(exec internal.Executor, lvName, vgName string) error {
 }
 
 // CreateLV creates the logical volume
-func CreateLV(exec internal.Executor, lvName, vgName string, sizePercent int) error {
+func CreateLV(exec exec.Executor, lvName, vgName string, sizePercent int) error {
 	args := []string{"-l", fmt.Sprintf("%d%%FREE", sizePercent),
 		"-c", defaultChunkSize, "-Z", "y", "-T", fmt.Sprintf("%s/%s", vgName, lvName)}
 
@@ -424,7 +424,7 @@ func CreateLV(exec internal.Executor, lvName, vgName string, sizePercent int) er
 }
 
 // ExtendLV extends the logical volume
-func ExtendLV(exec internal.Executor, lvName, vgName string, sizePercent int) error {
+func ExtendLV(exec exec.Executor, lvName, vgName string, sizePercent int) error {
 	args := []string{"-l", fmt.Sprintf("%d%%Vg", sizePercent), fmt.Sprintf("%s/%s", vgName, lvName)}
 
 	if _, err := exec.ExecuteCommandWithOutputAsHost(lvExtendCmd, args...); err != nil {
@@ -435,7 +435,7 @@ func ExtendLV(exec internal.Executor, lvName, vgName string, sizePercent int) er
 	return nil
 }
 
-func execute(exec internal.Executor, v interface{}, args ...string) error {
+func execute(exec exec.Executor, v interface{}, args ...string) error {
 	output, err := exec.ExecuteCommandWithOutputAsHost(lvmCmd, args...)
 	if err != nil {
 		return fmt.Errorf("failed to execute command. %v", err)
diff --git a/pkg/lvm/lvm_test.go b/pkg/lvm/lvm_test.go
index 89d7dd0ff..1faa42688 100644
--- a/pkg/lvm/lvm_test.go
+++ b/pkg/lvm/lvm_test.go
@@ -21,7 +21,7 @@ import (
 	"strings"
 	"testing"
 
-	mockExec "github.com/openshift/lvm-operator/pkg/internal/test"
+	mockExec "github.com/openshift/lvm-operator/pkg/internal/exec/test"
 	"github.com/stretchr/testify/assert"
 )
 
diff --git a/pkg/vgmanager/vgmanager_controller.go b/pkg/vgmanager/vgmanager_controller.go
index 153f348fc..da19fac84 100644
--- a/pkg/vgmanager/vgmanager_controller.go
+++ b/pkg/vgmanager/vgmanager_controller.go
@@ -28,7 +28,7 @@ import (
 	lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1"
 	"github.com/openshift/lvm-operator/controllers"
 	"github.com/openshift/lvm-operator/pkg/filter"
-	"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"
 	"github.com/openshift/lvm-operator/pkg/lvmd"
@@ -77,6 +77,7 @@ var (
 
 // SetupWithManager sets up the controller with the Manager.
 func (r *VGReconciler) SetupWithManager(mgr ctrl.Manager) error {
+	r.executor = &exec.CommandExecutor{}
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&lvmv1alpha1.LVMVolumeGroup{}).
 		Owns(&lvmv1alpha1.LVMVolumeGroupNodeStatus{}, builder.MatchEveryOwner).
@@ -86,9 +87,9 @@ func (r *VGReconciler) SetupWithManager(mgr ctrl.Manager) error {
 type VGReconciler struct {
 	client.Client
 	record.EventRecorder
-	Scheme    *runtime.Scheme
-	executor  internal.Executor
-	LVMD      lvmd.Configurator
+	Scheme   *runtime.Scheme
+	executor exec.Executor
+	LVMD     lvmd.Configurator
 	lsblk.LSBLK
 	NodeName  string
 	Namespace string
diff --git a/pkg/vgmanager/vgmanager_controller_test.go b/pkg/vgmanager/vgmanager_controller_test.go
index 468414e62..eb3c4c209 100644
--- a/pkg/vgmanager/vgmanager_controller_test.go
+++ b/pkg/vgmanager/vgmanager_controller_test.go
@@ -7,8 +7,8 @@ import (
 
 	"github.com/go-logr/logr/testr"
 	lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1"
-	"github.com/openshift/lvm-operator/pkg/internal"
-	mockExec "github.com/openshift/lvm-operator/pkg/internal/test"
+	"github.com/openshift/lvm-operator/pkg/internal/exec"
+	mockExec "github.com/openshift/lvm-operator/pkg/internal/exec/test"
 	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/strings/slices"
@@ -92,7 +92,7 @@ var mockLvsOutputRAID = `
 
 func TestVGReconciler_validateLVs(t *testing.T) {
 	type fields struct {
-		executor internal.Executor
+		executor exec.Executor
 	}
 	type args struct {
 		volumeGroup *lvmv1alpha1.LVMVolumeGroup
@@ -108,7 +108,7 @@ func TestVGReconciler_validateLVs(t *testing.T) {
 		"json",
 	}
 
-	mockExecutorForLVSOutput := func(output string) internal.Executor {
+	mockExecutorForLVSOutput := func(output string) exec.Executor {
 		return &mockExec.MockExecutor{
 			MockExecuteCommandWithOutputAsHost: func(command string, args ...string) (string, error) {
 				if !slices.Equal(args, lvsCommandForVG1) {