Skip to content

Commit

Permalink
feat: wipe devices before using if enabled
Browse files Browse the repository at this point in the history
Signed-off-by: Suleyman Akbas <[email protected]>
  • Loading branch information
suleymanakbas91 committed Sep 28, 2023
1 parent bd11b2a commit 5f3b9e1
Show file tree
Hide file tree
Showing 19 changed files with 741 additions and 52 deletions.
6 changes: 6 additions & 0 deletions .mockery.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,9 @@ packages:
github.com/openshift/lvm-operator/pkg/lvm:
interfaces:
LVM:
github.com/openshift/lvm-operator/pkg/wipefs:
interfaces:
Wipefs:
github.com/openshift/lvm-operator/pkg/dmsetup:
interfaces:
Dmsetup:
19 changes: 8 additions & 11 deletions api/v1alpha1/lvmcluster_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!
// NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized.

// LVMClusterSpec defines the desired state of LVMCluster
type LVMClusterSpec struct {
// Important: Run "make" to regenerate code after modifying this file
Expand Down Expand Up @@ -98,6 +95,7 @@ type DeviceClass struct {
}

// DeviceSelector specifies the list of criteria that have to match before a device is assigned
// +kubebuilder:validation:XValidation:rule="!has(oldSelf.forceWipeDevicesAndDestroyAllData) || has(self.forceWipeDevicesAndDestroyAllData)", message="ForceWipeDevicesAndDestroyAllData is required once set"
type DeviceSelector struct {
// MinSize is the minimum size of the device which needs to be included. Defaults to `1Gi` if empty
// +optional
Expand All @@ -114,15 +112,14 @@ type DeviceSelector struct {
// We discourage using the device names as they can change over node restarts.
// +optional
OptionalPaths []string `json:"optionalPaths,omitempty"`
}

// type DeviceClassConfig struct {
// LVMConfig *LVMConfig `json:"lvmConfig,omitempty"`
// }

// type LVMConfig struct {
// thinProvision bool `json:"thinProvision,omitempty"`
// }
// ForceWipeDevicesAndDestroyAllData runs wipefs to wipe the devices.
// This can lead to data lose. Enable this only when you know that the disk
// does not contain any important data.
// +optional
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="ForceWipeDevicesAndDestroyAllData is immutable"
ForceWipeDevicesAndDestroyAllData *bool `json:"forceWipeDevicesAndDestroyAllData,omitempty"`
}

type LVMStateType string

Expand Down
14 changes: 14 additions & 0 deletions bundle/manifests/lvm.topolvm.io_lvmclusters.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,15 @@ spec:
description: DeviceSelector is a set of rules that should
match for a device to be included in the LVMCluster
properties:
forceWipeDevicesAndDestroyAllData:
description: ForceWipeDevicesAndDestroyAllData runs
wipefs to wipe the devices. This can lead to data
lose. Enable this only when you know that the disk
does not contain any important data.
type: boolean
x-kubernetes-validations:
- message: ForceWipeDevicesAndDestroyAllData is immutable
rule: self == oldSelf
optionalPaths:
description: A list of device paths which could be chosen
for creating Volume Group. For example "/dev/disk/by-path/pci-0000:04:00.0-nvme-1"
Expand All @@ -73,6 +82,11 @@ spec:
type: string
type: array
type: object
x-kubernetes-validations:
- message: ForceWipeDevicesAndDestroyAllData is required
once set
rule: '!has(oldSelf.forceWipeDevicesAndDestroyAllData)
|| has(self.forceWipeDevicesAndDestroyAllData)'
fstype:
default: xfs
description: FilesystemType sets the filesystem the device
Expand Down
11 changes: 11 additions & 0 deletions bundle/manifests/lvm.topolvm.io_lvmvolumegroups.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,14 @@ spec:
description: DeviceSelector is a set of rules that should match for
a device to be included in this TopoLVMCluster
properties:
forceWipeDevicesAndDestroyAllData:
description: ForceWipeDevicesAndDestroyAllData runs wipefs to
wipe the devices. This can lead to data lose. Enable this only
when you know that the disk does not contain any important data.
type: boolean
x-kubernetes-validations:
- message: ForceWipeDevicesAndDestroyAllData is immutable
rule: self == oldSelf
optionalPaths:
description: A list of device paths which could be chosen for
creating Volume Group. For example "/dev/disk/by-path/pci-0000:04:00.0-nvme-1"
Expand All @@ -59,6 +67,9 @@ spec:
type: string
type: array
type: object
x-kubernetes-validations:
- message: ForceWipeDevicesAndDestroyAllData is required once set
rule: '!has(oldSelf.forceWipeDevicesAndDestroyAllData) || has(self.forceWipeDevicesAndDestroyAllData)'
nodeSelector:
description: NodeSelector chooses nodes
properties:
Expand Down
4 changes: 4 additions & 0 deletions cmd/vgmanager/vgmanager.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@ import (
"os"

"github.com/go-logr/logr"
"github.com/openshift/lvm-operator/pkg/dmsetup"
"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"
"github.com/openshift/lvm-operator/pkg/wipefs"
"github.com/spf13/cobra"
"sigs.k8s.io/controller-runtime/pkg/cache"
metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
Expand Down Expand Up @@ -106,6 +108,8 @@ func run(_ *cobra.Command, _ []string, opts *Options) error {
LVMD: lvmd.DefaultConfigurator(),
Scheme: mgr.GetScheme(),
LSBLK: lsblk.NewDefaultHostLSBLK(),
Wipefs: wipefs.NewDefaultHostWipefs(),
Dmsetup: dmsetup.NewDefaultHostDmsetup(),
LVM: lvm.NewDefaultHostLVM(),
NodeName: os.Getenv("NODE_NAME"),
Namespace: os.Getenv("POD_NAMESPACE"),
Expand Down
14 changes: 14 additions & 0 deletions config/crd/bases/lvm.topolvm.io_lvmclusters.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,15 @@ spec:
description: DeviceSelector is a set of rules that should
match for a device to be included in the LVMCluster
properties:
forceWipeDevicesAndDestroyAllData:
description: ForceWipeDevicesAndDestroyAllData runs
wipefs to wipe the devices. This can lead to data
lose. Enable this only when you know that the disk
does not contain any important data.
type: boolean
x-kubernetes-validations:
- message: ForceWipeDevicesAndDestroyAllData is immutable
rule: self == oldSelf
optionalPaths:
description: A list of device paths which could be chosen
for creating Volume Group. For example "/dev/disk/by-path/pci-0000:04:00.0-nvme-1"
Expand All @@ -69,6 +78,11 @@ spec:
type: string
type: array
type: object
x-kubernetes-validations:
- message: ForceWipeDevicesAndDestroyAllData is required
once set
rule: '!has(oldSelf.forceWipeDevicesAndDestroyAllData)
|| has(self.forceWipeDevicesAndDestroyAllData)'
fstype:
default: xfs
description: FilesystemType sets the filesystem the device
Expand Down
11 changes: 11 additions & 0 deletions config/crd/bases/lvm.topolvm.io_lvmvolumegroups.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,14 @@ spec:
description: DeviceSelector is a set of rules that should match for
a device to be included in this TopoLVMCluster
properties:
forceWipeDevicesAndDestroyAllData:
description: ForceWipeDevicesAndDestroyAllData runs wipefs to
wipe the devices. This can lead to data lose. Enable this only
when you know that the disk does not contain any important data.
type: boolean
x-kubernetes-validations:
- message: ForceWipeDevicesAndDestroyAllData is immutable
rule: self == oldSelf
optionalPaths:
description: A list of device paths which could be chosen for
creating Volume Group. For example "/dev/disk/by-path/pci-0000:04:00.0-nvme-1"
Expand All @@ -59,6 +67,9 @@ spec:
type: string
type: array
type: object
x-kubernetes-validations:
- message: ForceWipeDevicesAndDestroyAllData is required once set
rule: '!has(oldSelf.forceWipeDevicesAndDestroyAllData) || has(self.forceWipeDevicesAndDestroyAllData)'
nodeSelector:
description: NodeSelector chooses nodes
properties:
Expand Down
53 changes: 53 additions & 0 deletions pkg/dmsetup/dmsetup.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package dmsetup

import (
"errors"
"fmt"
"strings"

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

var (
DefaultDMSetup = "/usr/sbin/dmsetup"
ErrReferenceNotFound = errors.New("device-mapper reference not found")
)

type Dmsetup interface {
Remove(deviceName string) error
}

type HostDmsetup struct {
exec.Executor
dmsetup string
}

func NewDefaultHostDmsetup() *HostDmsetup {
return NewHostDmsetup(&exec.CommandExecutor{}, DefaultDMSetup)
}

func NewHostDmsetup(executor exec.Executor, dmsetup string) *HostDmsetup {
return &HostDmsetup{
Executor: executor,
dmsetup: dmsetup,
}
}

// Remove removes the device's reference from the device-mapper
func (dmsetup *HostDmsetup) Remove(deviceName string) error {
if len(deviceName) == 0 {
return errors.New("failed to remove device-mapper reference. Device name is empty")
}

args := []string{"remove"}
args = append(args, deviceName)
output, err := dmsetup.ExecuteCommandWithOutputAsHost(dmsetup.dmsetup, args...)
if err != nil {
if strings.Contains(output, "not found") {
return ErrReferenceNotFound
}
return fmt.Errorf("failed to remove the reference from device-mapper %q: %v", deviceName, err)
}

return nil
}
47 changes: 47 additions & 0 deletions pkg/dmsetup/dmsetup_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package dmsetup

import (
"errors"
"fmt"
"testing"

mockExec "github.com/openshift/lvm-operator/pkg/internal/exec/test"
"github.com/stretchr/testify/assert"
)

func TestRemove(t *testing.T) {
tests := []struct {
name string
deviceName string
wantErr bool
}{
{"Empty device name", "", true},
{"Existing device name", "/dev/loop1", false},
{"Non-existing device name", "/dev/loop2", true},
}

executor := &mockExec.MockExecutor{
MockExecuteCommandWithOutputAsHost: func(command string, args ...string) (string, error) {
if args[0] != "remove" {
return "", fmt.Errorf("invalid args %q", args[0])
}
if args[1] == "/dev/loop1" {
return "", nil
} else if args[1] == "/dev/loop2" {
return "device loop2 not found", errors.New("device loop2 not found")
}
return "", fmt.Errorf("invalid args %q", args[1])
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := NewHostDmsetup(executor, DefaultDMSetup).Remove(tt.deviceName)
if tt.wantErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
})
}
}
74 changes: 74 additions & 0 deletions pkg/dmsetup/mocks/mock_dmsetup.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/internal/exec/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ var (
nsenterPath = "/usr/bin/nsenter"
)

// Executor is the interface for running exec commands
// Executor is the interface for running exec commands
type Executor interface {
ExecuteCommandWithOutput(command string, arg ...string) (string, error)
ExecuteCommandWithOutputAsHost(command string, arg ...string) (string, error)
Expand Down
Loading

0 comments on commit 5f3b9e1

Please sign in to comment.