chore: refactor vgmanager into different interfaces and introduce tests

.mockery.yaml

@@ -0,0 +1,13 @@
+with-expecter: true
+filename: "{{.InterfaceName}}_mock.go"
+dir: "{{.InterfaceDir}}"
+mockname: "Mock{{.InterfaceName}}"
+outpkg: "{{.PackageName}}"
+inpackage: True
+packages:
+  github.com/openshift/lvm-operator/pkg/lsblk:
+      interfaces:
+        LSBLK:
+  github.com/openshift/lvm-operator/pkg/lvm:
+      interfaces:
+        LVM:

Makefile

@@ -141,6 +141,7 @@ manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and Cust
 
 generate: controller-gen ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations.
 	$(CONTROLLER_GEN) object:headerFile="hack/boilerplate.go.txt" paths="./..."
+    $(shell $(MOCKERY))
 
 fmt: ## Run go fmt against code.
 	go fmt ./...
@@ -319,6 +320,10 @@ GINKGO = $(shell pwd)/bin/ginkgo
 ginkgo: ## Download ginkgo and gomega locally if necessary.
 	$(call go-get-tool,$(GINKGO),github.com/onsi/ginkgo/v2/[email protected])
 
+MOCKERY = $(shell pwd)/bin/mockery
+mockery: ## Download mockery and add locally if necessary
+	$(call go-get-tool,$(MOCKERY),github.com/vektra/mockery/[email protected])
+
 # go-get-tool will 'go get' any package $2 and install it to $1.
 PROJECT_DIR := $(shell dirname $(abspath $(lastword $(MAKEFILE_LIST))))
 define go-get-tool

cmd/vgmanager/main.go

@@ -23,6 +23,7 @@ import (
 	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/lvm"
 	"github.com/openshift/lvm-operator/pkg/lvmd"
 	"github.com/openshift/lvm-operator/pkg/vgmanager"
 
@@ -85,6 +86,7 @@ func main() {
 		LVMD:          lvmd.DefaultConfigurator(),
 		Scheme:        mgr.GetScheme(),
 		LSBLK:         lsblk.NewDefaultHostLSBLK(),
+		LVM:           lvm.NewHostLVM(),
 		NodeName:      os.Getenv("NODE_NAME"),
 		Namespace:     os.Getenv("POD_NAMESPACE"),
 		Filters:       filter.DefaultFilters,

controllers/constants.go

@@ -81,7 +81,6 @@ const (
 	CSIKubeletRootDir               = "/var/lib/kubelet/"
 	NodeContainerName               = "topolvm-node"
 	TopolvmNodeContainerHealthzName = "healthz"
-	LvmdConfigFile                  = "/etc/topolvm/lvmd.yaml"
 
 	DefaultCSISocket  = "/run/topolvm/csi-topolvm.sock"
 	DefaultLVMdSocket = "/run/lvmd/lvmd.sock"

controllers/topolvm_node.go

@@ -23,6 +23,7 @@ import (
 	"strings"
 
 	lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1"
+	"github.com/openshift/lvm-operator/pkg/lvmd"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -137,7 +138,7 @@ func getNodeDaemonSet(lvmCluster *lvmv1alpha1.LVMCluster, namespace string, init
 		{Name: "lvmd-config-dir",
 			VolumeSource: corev1.VolumeSource{
 				HostPath: &corev1.HostPathVolumeSource{
-					Path: filepath.Dir(LvmdConfigFile),
+					Path: filepath.Dir(lvmd.DefaultLVMDConfigFilePath),
 					Type: &hostPathDirectory}}},
 		{Name: "lvmd-socket-dir",
 			VolumeSource: corev1.VolumeSource{
@@ -202,11 +203,11 @@ func getNodeInitContainer(initImage string) *corev1.Container {
 	command := []string{
 		"/usr/bin/bash",
 		"-c",
-		fmt.Sprintf("until [ -f %s ]; do echo waiting for lvmd config file; sleep 5; done", LvmdConfigFile),
+		fmt.Sprintf("until [ -f %s ]; do echo waiting for lvmd config file; sleep 5; done", lvmd.DefaultLVMDConfigFilePath),
 	}
 
 	volumeMounts := []corev1.VolumeMount{
-		{Name: "lvmd-config-dir", MountPath: filepath.Dir(LvmdConfigFile)},
+		{Name: "lvmd-config-dir", MountPath: filepath.Dir(lvmd.DefaultLVMDConfigFilePath)},
 	}
 
 	fileChecker := &corev1.Container{
@@ -228,7 +229,7 @@ func getNodeInitContainer(initImage string) *corev1.Container {
 func getLvmdContainer() *corev1.Container {
 	command := []string{
 		"/lvmd",
-		fmt.Sprintf("--config=%s", LvmdConfigFile),
+		fmt.Sprintf("--config=%s", lvmd.DefaultLVMDConfigFilePath),
 		"--container=true",
 	}
 
@@ -241,7 +242,7 @@ func getLvmdContainer() *corev1.Container {
 
 	volumeMounts := []corev1.VolumeMount{
 		{Name: "lvmd-socket-dir", MountPath: filepath.Dir(DefaultLVMdSocket)},
-		{Name: "lvmd-config-dir", MountPath: filepath.Dir(LvmdConfigFile)},
+		{Name: "lvmd-config-dir", MountPath: filepath.Dir(lvmd.DefaultLVMDConfigFilePath)},
 	}
 
 	privilege := true

go.mod

@@ -80,6 +80,7 @@ require (
 	github.com/spf13/jwalterweatherman v1.1.0 // indirect
 	github.com/spf13/pflag v1.0.5 // indirect
 	github.com/spf13/viper v1.10.1 // indirect
+	github.com/stretchr/objx v0.5.0 // indirect
 	github.com/subosito/gotenv v1.2.0 // indirect
 	go.uber.org/atomic v1.9.0 // indirect
 	go.uber.org/multierr v1.8.0 // indirect

go.sum

@@ -367,6 +367,7 @@ github.com/stoewer/go-strcase v1.2.0/go.mod h1:IBiWB2sKIp3wVVQ3Y035++gc+knqhUQag
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE=
 github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
+github.com/stretchr/objx v0.5.0 h1:1zr/of2m5FGMsad5YfcqgdqdWrIhu+EBEJRhR1U7z/c=
 github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo=
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=

pkg/filter/filter.go

@@ -20,7 +20,6 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/openshift/lvm-operator/pkg/internal/exec"
 	"github.com/openshift/lvm-operator/pkg/lsblk"
 	"github.com/openshift/lvm-operator/pkg/lvm"
 )
@@ -51,31 +50,31 @@ const (
 	usableDeviceType           = "usableDeviceType"
 )
 
-type Filters map[string]func(lsblk.BlockDevice, exec.Executor) (bool, error)
+type Filters map[string]func(lsblk.BlockDevice) (bool, error)
 
-func DefaultFilters(lsblkInstance lsblk.LSBLK) Filters {
+func DefaultFilters(lvm lvm.LVM, lsblkInstance lsblk.LSBLK) Filters {
 	return Filters{
-		notReadOnly: func(dev lsblk.BlockDevice, _ exec.Executor) (bool, error) {
+		notReadOnly: func(dev lsblk.BlockDevice) (bool, error) {
 			return !dev.ReadOnly, nil
 		},
 
-		notSuspended: func(dev lsblk.BlockDevice, _ exec.Executor) (bool, error) {
+		notSuspended: func(dev lsblk.BlockDevice) (bool, error) {
 			matched := dev.State != StateSuspended
 			return matched, nil
 		},
 
-		noBiosBootInPartLabel: func(dev lsblk.BlockDevice, _ exec.Executor) (bool, error) {
+		noBiosBootInPartLabel: func(dev lsblk.BlockDevice) (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) {
+		noReservedInPartLabel: func(dev lsblk.BlockDevice) (bool, error) {
 			reservedInPartLabel := strings.Contains(strings.ToLower(dev.PartLabel), "reserved")
 			return !reservedInPartLabel, nil
 		},
 
-		noValidFilesystemSignature: func(dev lsblk.BlockDevice, e exec.Executor) (bool, error) {
+		noValidFilesystemSignature: func(dev lsblk.BlockDevice) (bool, error) {
 			// if no fs type is set, it's always okay
 			if dev.FSType == "" {
 				return true, nil
@@ -84,7 +83,7 @@ func DefaultFilters(lsblkInstance lsblk.LSBLK) Filters {
 			// 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, "")
+				pvs, err := lvm.ListPVs("")
 				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)
 				}
@@ -108,17 +107,17 @@ func DefaultFilters(lsblkInstance lsblk.LSBLK) Filters {
 			return false, 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) {
+		noChildren: func(dev lsblk.BlockDevice) (bool, error) {
 			hasChildren := dev.HasChildren()
 			return !hasChildren, nil
 		},
 
-		usableDeviceType: func(dev lsblk.BlockDevice, executor exec.Executor) (bool, error) {
+		noBindMounts: func(dev lsblk.BlockDevice) (bool, error) {
+			hasBindMounts, _, err := lsblkInstance.HasBindMounts(dev)
+			return !hasBindMounts, err
+		},
+
+		usableDeviceType: func(dev lsblk.BlockDevice) (bool, error) {
 			switch dev.Type {
 			case DeviceTypeLoop:
 				// check loop device isn't being used by kubernetes

pkg/filter/filter_test.go

@@ -20,7 +20,7 @@ func TestNotReadOnly(t *testing.T) {
 		{label: "tc true", device: lsblk.BlockDevice{ReadOnly: true}, expected: false, expectErr: false},
 	}
 	for _, tc := range testcases {
-		result, err := DefaultFilters(nil)[notReadOnly](tc.device, nil)
+		result, err := DefaultFilters(nil, nil)[notReadOnly](tc.device)
 		assert.Equal(t, tc.expected, result)
 		if tc.expectErr {
 			assert.Error(t, err)
@@ -37,7 +37,7 @@ func TestNotSuspended(t *testing.T) {
 		{label: "tc running", device: lsblk.BlockDevice{State: "running"}, expected: true, expectErr: false},
 	}
 	for _, tc := range testcases {
-		result, err := DefaultFilters(nil)[notSuspended](tc.device, nil)
+		result, err := DefaultFilters(nil, nil)[notSuspended](tc.device)
 		assert.Equal(t, tc.expected, result)
 		if tc.expectErr {
 			assert.Error(t, err)
@@ -54,7 +54,7 @@ func TestNoFilesystemSignature(t *testing.T) {
 		{label: "tc swap", device: lsblk.BlockDevice{FSType: "swap"}, expected: false, expectErr: false},
 	}
 	for _, tc := range testcases {
-		result, err := DefaultFilters(nil)[noValidFilesystemSignature](tc.device, nil)
+		result, err := DefaultFilters(nil, nil)[noValidFilesystemSignature](tc.device)
 		assert.Equal(t, tc.expected, result)
 		if tc.expectErr {
 			assert.Error(t, err)
@@ -70,7 +70,7 @@ func TestNoChildren(t *testing.T) {
 		{label: "tc no child", device: lsblk.BlockDevice{Name: "dev2", Children: []lsblk.BlockDevice{}}, expected: true, expectErr: false},
 	}
 	for _, tc := range testcases {
-		result, err := DefaultFilters(nil)[noChildren](tc.device, nil)
+		result, err := DefaultFilters(nil, nil)[noChildren](tc.device)
 		assert.Equal(t, tc.expected, result)
 		if tc.expectErr {
 			assert.Error(t, err)
@@ -86,7 +86,7 @@ func TestIsUsableDeviceType(t *testing.T) {
 		{label: "tc Disk", device: lsblk.BlockDevice{Name: "dev2", Type: "disk"}, expected: true, expectErr: false},
 	}
 	for _, tc := range testcases {
-		result, err := DefaultFilters(nil)[usableDeviceType](tc.device, nil)
+		result, err := DefaultFilters(nil, nil)[usableDeviceType](tc.device)
 		assert.Equal(t, tc.expected, result)
 		if tc.expectErr {
 			assert.Error(t, err)
@@ -106,7 +106,7 @@ func TestNoBiosBootInPartLabel(t *testing.T) {
 		{label: "tc 6", device: lsblk.BlockDevice{Name: "dev6", PartLabel: "BOOT"}, expected: false, expectErr: false},
 	}
 	for _, tc := range testcases {
-		result, err := DefaultFilters(nil)[noBiosBootInPartLabel](tc.device, nil)
+		result, err := DefaultFilters(nil, nil)[noBiosBootInPartLabel](tc.device)
 		assert.Equal(t, tc.expected, result)
 		if tc.expectErr {
 			assert.Error(t, err)
@@ -125,7 +125,7 @@ func TestNoReservedInPartLabel(t *testing.T) {
 		{label: "tc 5", device: lsblk.BlockDevice{Name: "dev5", PartLabel: "Reserved"}, expected: false},
 	}
 	for _, tc := range testcases {
-		result, err := DefaultFilters(nil)[noReservedInPartLabel](tc.device, nil)
+		result, err := DefaultFilters(nil, nil)[noReservedInPartLabel](tc.device)
 		assert.NoError(t, err)
 		assert.Equal(t, tc.expected, result)
 	}

pkg/internal/exec/exec.go

@@ -24,7 +24,6 @@ import (
 
 var (
 	nsenterPath = "/usr/bin/nsenter"
-	losetupPath = "/usr/sbin/losetup"
 )
 
 // Executor is the  interface for running exec commands
@@ -42,7 +41,7 @@ func (*CommandExecutor) ExecuteCommandWithOutput(command string, arg ...string)
 	return runCommandWithOutput(cmd)
 }
 
-// ExecuteCommandWithOutput executes a command with output using nsenter
+// ExecuteCommandWithOutputAsHost executes a command with output using nsenter
 func (*CommandExecutor) ExecuteCommandWithOutputAsHost(command string, arg ...string) (string, error) {
 	args := append([]string{"-m", "-u", "-i", "-n", "-p", "-t", "1", command}, arg...)
 	cmd := exec.Command(nsenterPath, args...)