Skip to content

Commit

Permalink
Merge pull request #423 from suleymanakbas91/wipefs
Browse files Browse the repository at this point in the history
OCPVE-600: Implement the ability to wipe the devices
  • Loading branch information
openshift-merge-robot authored Oct 2, 2023
2 parents bd11b2a + 56f7077 commit e7498c2
Show file tree
Hide file tree
Showing 22 changed files with 768 additions and 54 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:
38 changes: 38 additions & 0 deletions api/v1alpha1/lvmcluster_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/openshift/lvm-operator/pkg/cluster"

v1 "k8s.io/api/core/v1"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/ptr"
)

func generateUniqueNameForTestCase(ctx SpecContext) string {
Expand Down Expand Up @@ -401,4 +403,40 @@ var _ = Describe("webhook acceptance tests", func() {

Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
})

It("force wipe option cannot be added in update", func(ctx SpecContext) {
resource := defaultLVMClusterInUniqueNamespace(ctx)
resource.Spec.Storage.DeviceClasses[0].DeviceSelector = &DeviceSelector{Paths: []string{"/dev/newpath"}}
Expect(k8sClient.Create(ctx, resource)).To(Succeed())

updated := resource.DeepCopy()
updated.Spec.Storage.DeviceClasses[0].DeviceSelector.ForceWipeDevicesAndDestroyAllData = ptr.To[bool](true)

err := k8sClient.Update(ctx, updated)
Expect(err).To(HaveOccurred())
Expect(err).To(Satisfy(k8serrors.IsForbidden))
statusError := &k8serrors.StatusError{}
Expect(errors.As(err, &statusError)).To(BeTrue())
Expect(statusError.Status().Message).To(ContainSubstring(ErrForceWipeOptionCannotBeChanged.Error()))

Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
})

It("force wipe option cannot be changed in update", func(ctx SpecContext) {
resource := defaultLVMClusterInUniqueNamespace(ctx)
resource.Spec.Storage.DeviceClasses[0].DeviceSelector = &DeviceSelector{Paths: []string{"/dev/newpath"}, ForceWipeDevicesAndDestroyAllData: ptr.To[bool](false)}
Expect(k8sClient.Create(ctx, resource)).To(Succeed())

updated := resource.DeepCopy()
updated.Spec.Storage.DeviceClasses[0].DeviceSelector.ForceWipeDevicesAndDestroyAllData = ptr.To[bool](true)

err := k8sClient.Update(ctx, updated)
Expect(err).To(HaveOccurred())
Expect(err).To(Satisfy(k8serrors.IsForbidden))
statusError := &k8serrors.StatusError{}
Expect(errors.As(err, &statusError)).To(BeTrue())
Expect(statusError.Status().Message).To(ContainSubstring(ErrForceWipeOptionCannotBeChanged.Error()))

Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
})
})
17 changes: 6 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 @@ -114,15 +111,13 @@ 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
ForceWipeDevicesAndDestroyAllData *bool `json:"forceWipeDevicesAndDestroyAllData,omitempty"`
}

type LVMStateType string

Expand Down
16 changes: 14 additions & 2 deletions api/v1alpha1/lvmcluster_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ var (
ErrDuplicateLVMCluster = errors.New("duplicate LVMClusters are not allowed, remove the old LVMCluster or work with the existing instance")
ErrThinPoolConfigCannotBeChanged = errors.New("ThinPoolConfig can not be changed")
ErrDevicePathsCannotBeAddedInUpdate = errors.New("device paths can not be added after a device class has been initialized")
ErrForceWipeOptionCannotBeChanged = errors.New("ForceWipeDevicesAndDestroyAllData can not be changed")
)

//+kubebuilder:webhook:path=/validate-lvm-topolvm-io-v1alpha1-lvmcluster,mutating=false,failurePolicy=fail,sideEffects=None,groups=lvm.topolvm.io,resources=lvmclusters,verbs=create;update,versions=v1alpha1,name=vlvmcluster.kb.io,admissionReviewVersions=v1
Expand Down Expand Up @@ -156,6 +157,7 @@ func (v *lvmClusterValidator) ValidateUpdate(_ context.Context, old, new runtime
for _, deviceClass := range l.Spec.Storage.DeviceClasses {
var newThinPoolConfig, oldThinPoolConfig *ThinPoolConfig
var newDevices, newOptionalDevices, oldDevices, oldOptionalDevices []string
var oldForceWipeOption, newForceWipeOption *bool

newThinPoolConfig = deviceClass.ThinPoolConfig
oldThinPoolConfig, err = v.getThinPoolsConfigOfDeviceClass(oldLVMCluster, deviceClass.Name)
Expand All @@ -178,15 +180,24 @@ func (v *lvmClusterValidator) ValidateUpdate(_ context.Context, old, new runtime
if deviceClass.DeviceSelector != nil {
newDevices = deviceClass.DeviceSelector.Paths
newOptionalDevices = deviceClass.DeviceSelector.OptionalPaths
newForceWipeOption = deviceClass.DeviceSelector.ForceWipeDevicesAndDestroyAllData
}

oldDevices, oldOptionalDevices, err = v.getPathsOfDeviceClass(oldLVMCluster, deviceClass.Name)
oldDevices, oldOptionalDevices, oldForceWipeOption, err = v.getPathsOfDeviceClass(oldLVMCluster, deviceClass.Name)

// Is this a new device class?
if err == ErrDeviceClassNotFound {
continue
}

// Make sure ForceWipeDevicesAndDestroyAllData was not changed
if (oldForceWipeOption == nil && newForceWipeOption != nil) ||
(oldForceWipeOption != nil && newForceWipeOption == nil) ||
(oldForceWipeOption != nil && newForceWipeOption != nil &&
*oldForceWipeOption != *newForceWipeOption) {
return warnings, ErrForceWipeOptionCannotBeChanged
}

// Make sure a device path list was not added
if len(oldDevices) == 0 && len(newDevices) > 0 {
return warnings, ErrDevicePathsCannotBeAddedInUpdate
Expand Down Expand Up @@ -366,13 +377,14 @@ func (v *lvmClusterValidator) verifyNoDeviceOverlap(l *LVMCluster) error {
return nil
}

func (v *lvmClusterValidator) getPathsOfDeviceClass(l *LVMCluster, deviceClassName string) (required []string, optional []string, err error) {
func (v *lvmClusterValidator) getPathsOfDeviceClass(l *LVMCluster, deviceClassName string) (required []string, optional []string, forceWipe *bool, err error) {
required, optional, err = []string{}, []string{}, nil
for _, deviceClass := range l.Spec.Storage.DeviceClasses {
if deviceClass.Name == deviceClassName {
if deviceClass.DeviceSelector != nil {
required = deviceClass.DeviceSelector.Paths
optional = deviceClass.DeviceSelector.OptionalPaths
forceWipe = deviceClass.DeviceSelector.ForceWipeDevicesAndDestroyAllData
}

return
Expand Down
5 changes: 5 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go

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

6 changes: 6 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,12 @@ 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
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 Down
5 changes: 5 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,11 @@ 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
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 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
6 changes: 6 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,12 @@ 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
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 Down
5 changes: 5 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,11 @@ 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
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 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)
}
})
}
}
Loading

0 comments on commit e7498c2

Please sign in to comment.