Skip to content

Commit

Permalink
chore: refactor vgmanager into different interfaces and introduce tests
Browse files Browse the repository at this point in the history
  • Loading branch information
jakobmoellerdev committed Sep 5, 2023
1 parent dd8b3b7 commit 1dbd4ff
Show file tree
Hide file tree
Showing 18 changed files with 863 additions and 509 deletions.
16 changes: 12 additions & 4 deletions cmd/vgmanager/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ import (
"os"

lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1"
"github.com/openshift/lvm-operator/pkg/exec"
"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"

"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -72,11 +76,15 @@ func main() {
os.Exit(1)
}

executor := &exec.CommandExecutor{}
if err = (&vgmanager.VGReconciler{
Client: mgr.GetClient(),
Scheme: mgr.GetScheme(),
NodeName: os.Getenv("NODE_NAME"),
Namespace: os.Getenv("POD_NAMESPACE"),
Client: mgr.GetClient(),
Scheme: mgr.GetScheme(),
NodeName: os.Getenv("NODE_NAME"),
Namespace: os.Getenv("POD_NAMESPACE"),
LVM: lvm.NewHostLVM(executor),
LSBLK: lsblk.NewHostLSBLK(executor, lsblk.DefaultMountinfo, lsblk.DefaultLosetup),
LVMDConfig: lvmd.NewDefaultLMVDFileConfig(),
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "VGManager")
os.Exit(1)
Expand Down
1 change: 0 additions & 1 deletion controllers/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
11 changes: 6 additions & 5 deletions controllers/topolvm_node.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand All @@ -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",
}

Expand All @@ -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
Expand Down
5 changes: 2 additions & 3 deletions pkg/internal/exec.go → pkg/exec/exec.go
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 All @@ -24,7 +24,6 @@ import (

var (
nsenterPath = "/usr/bin/nsenter"
losetupPath = "/usr/sbin/losetup"
)

// Executor is the interface for running exec commands
Expand All @@ -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...)
Expand Down
110 changes: 110 additions & 0 deletions pkg/lsblk/filter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/*
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 lsblk

import (
"strings"
)

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"

// filter names:
notReadOnly = "notReadOnly"
notSuspended = "notSuspended"
noBiosBootInPartLabel = "noBiosBootInPartLabel"
noReservedInPartLabel = "noReservedInPartLabel"
noFilesystemSignature = "noFilesystemSignature"
noBindMounts = "noBindMounts"
noChildren = "noChildren"
usableDeviceType = "usableDeviceType"
)

func (lsblk *HostLSBLK) ResetFilterMap() {
lsblk.filterMap = lsblk.defaultFilterMap()
}

func (lsblk *HostLSBLK) GetFilterMap() map[string]func(BlockDevice) (bool, error) {
return lsblk.filterMap
}

func (lsblk *HostLSBLK) SetFilterMap(newFilterMap map[string]func(BlockDevice) (bool, error)) {
lsblk.filterMap = newFilterMap
}

func (lsblk *HostLSBLK) defaultFilterMap() map[string]func(BlockDevice) (bool, error) {
return map[string]func(BlockDevice) (bool, error){
notReadOnly: func(dev BlockDevice) (bool, error) {
return !dev.ReadOnly, nil
},

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

noBiosBootInPartLabel: func(dev 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 BlockDevice) (bool, error) {
reservedInPartLabel := strings.Contains(strings.ToLower(dev.PartLabel), "reserved")
return !reservedInPartLabel, nil
},

noFilesystemSignature: func(dev BlockDevice) (bool, error) {
matched := dev.FSType == ""
return matched, nil
},

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

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

usableDeviceType: func(dev BlockDevice) (bool, error) {
switch dev.Type {
case DeviceTypeLoop:
// check loop device isn't being used by kubernetes
return lsblk.IsUsableLoopDev(dev)
case DeviceTypeROM:
return false, nil
case DeviceTypeLVM:
return false, nil
default:
return true, nil
}
},
}
}
131 changes: 131 additions & 0 deletions pkg/lsblk/filter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
package lsblk

import (
"testing"

"github.com/stretchr/testify/assert"
)

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

func TestNotReadOnly(t *testing.T) {
testcases := []filterTestCase{
{label: "tc false", device: BlockDevice{ReadOnly: false}, expected: true, expectErr: false},
{label: "tc true", device: BlockDevice{ReadOnly: true}, expected: false, expectErr: false},
}
for _, tc := range testcases {
result, err := NewHostLSBLK(nil, "", "").GetFilterMap()[notReadOnly](tc.device)
assert.Equal(t, tc.expected, result)
if tc.expectErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
}
}

func TestNotSuspended(t *testing.T) {
testcases := []filterTestCase{
{label: "tc suspended", device: BlockDevice{State: "suspended"}, expected: false, expectErr: false},
{label: "tc live", device: BlockDevice{State: "live"}, expected: true, expectErr: false},
{label: "tc running", device: BlockDevice{State: "running"}, expected: true, expectErr: false},
}
for _, tc := range testcases {
result, err := NewHostLSBLK(nil, "", "").GetFilterMap()[notSuspended](tc.device)
assert.Equal(t, tc.expected, result)
if tc.expectErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
}
}

func TestNoFilesystemSignature(t *testing.T) {
testcases := []filterTestCase{
{label: "tc no fs", device: BlockDevice{FSType: ""}, expected: true, expectErr: false},
{label: "tc xfs", device: BlockDevice{FSType: "xfs"}, expected: false, expectErr: false},
{label: "tc swap", device: BlockDevice{FSType: "swap"}, expected: false, expectErr: false},
}
for _, tc := range testcases {
result, err := NewHostLSBLK(nil, "", "").GetFilterMap()[noFilesystemSignature](tc.device)
assert.Equal(t, tc.expected, result)
if tc.expectErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
}
}

func TestNoChildren(t *testing.T) {
testcases := []filterTestCase{
{label: "tc child", device: BlockDevice{Name: "dev1", Children: []BlockDevice{{Name: "child1"}}}, expected: false, expectErr: false},
{label: "tc no child", device: BlockDevice{Name: "dev2", Children: []BlockDevice{}}, expected: true, expectErr: false},
}
for _, tc := range testcases {
result, err := NewHostLSBLK(nil, "", "").GetFilterMap()[noChildren](tc.device)
assert.Equal(t, tc.expected, result)
if tc.expectErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
}
}

func TestIsUsableDeviceType(t *testing.T) {
testcases := []filterTestCase{
{label: "tc ROM", device: BlockDevice{Name: "dev1", Type: "rom"}, expected: false, expectErr: false},
{label: "tc Disk", device: BlockDevice{Name: "dev2", Type: "disk"}, expected: true, expectErr: false},
}
for _, tc := range testcases {
result, err := NewHostLSBLK(nil, "", "").GetFilterMap()[usableDeviceType](tc.device)
assert.Equal(t, tc.expected, result)
if tc.expectErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
}
}

func TestNoBiosBootInPartLabel(t *testing.T) {
testcases := []filterTestCase{
{label: "tc 1", device: BlockDevice{Name: "dev1", PartLabel: ""}, expected: true, expectErr: false},
{label: "tc 2", device: BlockDevice{Name: "dev2", PartLabel: "abc"}, expected: true, expectErr: false},
{label: "tc 3", device: BlockDevice{Name: "dev3", PartLabel: "bios"}, expected: false, expectErr: false},
{label: "tc 4", device: BlockDevice{Name: "dev4", PartLabel: "BIOS"}, expected: false, expectErr: false},
{label: "tc 5", device: BlockDevice{Name: "dev5", PartLabel: "boot"}, expected: false, expectErr: false},
{label: "tc 6", device: BlockDevice{Name: "dev6", PartLabel: "BOOT"}, expected: false, expectErr: false},
}
for _, tc := range testcases {
result, err := NewHostLSBLK(nil, "", "").GetFilterMap()[noBiosBootInPartLabel](tc.device)
assert.Equal(t, tc.expected, result)
if tc.expectErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
}
}

func TestNoReservedInPartLabel(t *testing.T) {
testcases := []filterTestCase{
{label: "tc 1", device: BlockDevice{Name: "dev1", PartLabel: ""}, expected: true},
{label: "tc 2", device: BlockDevice{Name: "dev2", PartLabel: "abc"}, expected: true},
{label: "tc 3", device: BlockDevice{Name: "dev3", PartLabel: "reserved"}, expected: false},
{label: "tc 4", device: BlockDevice{Name: "dev4", PartLabel: "RESERVED"}, expected: false},
{label: "tc 5", device: BlockDevice{Name: "dev5", PartLabel: "Reserved"}, expected: false},
}
for _, tc := range testcases {
result, err := NewHostLSBLK(nil, "", "").GetFilterMap()[noReservedInPartLabel](tc.device)
assert.NoError(t, err)
assert.Equal(t, tc.expected, result)
}
}
Loading

0 comments on commit 1dbd4ff

Please sign in to comment.