fix: correct wiping behavior

for given lsblk -o Kname,name (vdb was created with one partition (type 8E), after which a vg and lv thin pool have been created on it) vdb vdb vdb1 `-vdb1 dm-0 |-some--other--vg-some--lv_tmeta dm-2 | `-some--other--vg-some--lv dm-1 `-some--other--vg-some--lv_tdata dm-2 `-some--other--vg-some--lv the commands will now be dmsetup remove --force /dev/dm-2 dmsetup remove --force /dev/dm-0 dmsetup remove --force /dev/dm-2 //no-op dmsetup remove --force /dev/dm-1 wipefs --all --force /dev/vdb // will do ioctl reload and cause partition table refresh Signed-off-by: Jakob Möller <[email protected]>
openshift · Aug 7, 2024 · d80e311 · d80e311
1 parent d6989bc
commit d80e311
Show file tree

Hide file tree

Showing 6 changed files with 68 additions and 45 deletions.
diff --git a/internal/controllers/vgmanager/dmsetup/dmsetup.go b/internal/controllers/vgmanager/dmsetup/dmsetup.go
@@ -5,9 +5,8 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	exec2 "os/exec"
 
-	"github.com/openshift/lvm-operator/v4/internal/controllers/vgmanager/exec"
+	vgmanagerexec "github.com/openshift/lvm-operator/v4/internal/controllers/vgmanager/exec"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
@@ -21,15 +20,15 @@ type Dmsetup interface {
 }
 
 type HostDmsetup struct {
-	exec.Executor
+	vgmanagerexec.Executor
 	dmsetup string
 }
 
 func NewDefaultHostDmsetup() *HostDmsetup {
-	return NewHostDmsetup(&exec.CommandExecutor{}, DefaultDMSetup)
+	return NewHostDmsetup(&vgmanagerexec.CommandExecutor{}, DefaultDMSetup)
 }
 
-func NewHostDmsetup(executor exec.Executor, dmsetup string) *HostDmsetup {
+func NewHostDmsetup(executor vgmanagerexec.Executor, dmsetup string) *HostDmsetup {
 	return &HostDmsetup{
 		Executor: executor,
 		dmsetup:  dmsetup,
@@ -42,12 +41,7 @@ func (dmsetup *HostDmsetup) Remove(ctx context.Context, deviceName string) error
 		return errors.New("failed to remove device-mapper reference. Device name is empty")
 	}
 
-	output, err := exec2.CommandContext(ctx, "nsenter",
-		append(
-			[]string{"-m", "-u", "-i", "-n", "-p", "-t", "1"},
-			[]string{dmsetup.dmsetup, "remove", "--force", deviceName}...,
-		)...,
-	).CombinedOutput()
+	output, err := dmsetup.Executor.CombinedOutputCommandAsHost(ctx, dmsetup.dmsetup, "remove", "--force", deviceName)
 
 	if err == nil {
 		log.FromContext(ctx).Info(fmt.Sprintf("successfully removed the reference from device-mapper %q: %s", deviceName, string(output)))

diff --git a/internal/controllers/vgmanager/dmsetup/dmsetup_test.go b/internal/controllers/vgmanager/dmsetup/dmsetup_test.go
@@ -4,8 +4,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"io"
-	"strings"
 	"testing"
 
 	"github.com/go-logr/logr/testr"
@@ -26,16 +24,19 @@ func TestRemove(t *testing.T) {
 	}
 
 	executor := &mockExec.MockExecutor{
-		MockExecuteCommandWithOutputAsHost: func(ctx context.Context, command string, args ...string) (io.ReadCloser, error) {
+		MockCombinedOutputCommandAsHost: func(ctx context.Context, command string, args ...string) ([]byte, error) {
 			if args[0] != "remove" {
-				return io.NopCloser(strings.NewReader("")), fmt.Errorf("invalid args %q", args[0])
+				return nil, fmt.Errorf("invalid args %q", args[0])
 			}
-			if args[1] == "/dev/loop1" {
-				return io.NopCloser(strings.NewReader("")), nil
-			} else if args[1] == "/dev/loop2" {
-				return io.NopCloser(strings.NewReader("device loop2 not found")), errors.New("device loop2 not found")
+			if args[1] != "--force" {
+				return nil, fmt.Errorf("invalid args %q", args[1])
 			}
-			return io.NopCloser(strings.NewReader("")), fmt.Errorf("invalid args %q", args[1])
+			if args[2] == "/dev/loop1" {
+				return nil, nil
+			} else if args[2] == "/dev/loop2" {
+				return []byte("device loop2 not found"), errors.New("device loop2 not found")
+			}
+			return nil, fmt.Errorf("invalid args %q", args[1])
 		},
 	}
 

diff --git a/internal/controllers/vgmanager/exec/exec.go b/internal/controllers/vgmanager/exec/exec.go
@@ -31,14 +31,17 @@ import (
 )
 
 var (
-	nsenterPath = "/usr/bin/nsenter"
+	nsEnterPath  = "/usr/bin/nsenter"
+	nsEnterFlags = []string{"-m", "-u", "-i", "-n", "-p", "-t", "1"}
 )
 
 // Executor is the interface for running exec commands
 type Executor interface {
 	StartCommandWithOutputAsHost(ctx context.Context, command string, arg ...string) (io.ReadCloser, error)
 	RunCommandAsHost(ctx context.Context, command string, arg ...string) error
+	CombinedOutputCommandAsHost(ctx context.Context, command string, arg ...string) ([]byte, error)
 	RunCommandAsHostInto(ctx context.Context, into any, command string, arg ...string) error
+	WrapCommandWithNSenter(command string, arg ...string) (string, []string)
 }
 
 // CommandExecutor is an Executor type
@@ -50,6 +53,15 @@ func (e *CommandExecutor) RunCommandAsHost(ctx context.Context, command string,
 	return e.RunCommandAsHostInto(ctx, nil, command, arg...)
 }
 
+// CombinedOutputCommandAsHost executes a command as host and returns an error if the command fails.
+// it finishes the run and the output will be printed to the log.
+func (e *CommandExecutor) CombinedOutputCommandAsHost(ctx context.Context, command string, arg ...string) ([]byte, error) {
+	command, arg = e.WrapCommandWithNSenter(command, arg...)
+	cmd := exec.Command(command, arg...)
+	log.FromContext(ctx).Info("executing", "command", cmd.String())
+	return cmd.CombinedOutput()
+}
+
 // RunCommandAsHostInto executes a command as host and returns an error if the command fails.
 // it finishes the run and decodes the output via JSON into the provided struct pointer.
 // if the struct pointer is nil, the output will be printed to the log instead.
@@ -77,23 +89,25 @@ func (e *CommandExecutor) RunCommandAsHostInto(ctx context.Context, into any, co
 // StartCommandWithOutputAsHost executes a command with output as host and returns the output as a ReadCloser.
 // The caller is responsible for closing the ReadCloser.
 // Not calling close on this method will result in a resource leak.
-func (*CommandExecutor) StartCommandWithOutputAsHost(ctx context.Context, command string, arg ...string) (io.ReadCloser, error) {
-	args := append([]string{"-m", "-u", "-i", "-n", "-p", "-t", "1", command}, arg...)
-	cmd := exec.Command(nsenterPath, args...)
+func (e *CommandExecutor) StartCommandWithOutputAsHost(ctx context.Context, command string, arg ...string) (io.ReadCloser, error) {
+	command, arg = e.WrapCommandWithNSenter(command, arg...)
+	cmd := exec.Command(command, arg...)
 	log.FromContext(ctx).Info("executing", "command", cmd.String())
 	return runCommandWithOutput(cmd)
 }
 
+// WrapCommandWithNSenter wraps the command and arguments with nsenter arguments.
+func (*CommandExecutor) WrapCommandWithNSenter(command string, arg ...string) (string, []string) {
+	return nsEnterPath, append(append(nsEnterFlags, command), arg...)
+}
+
 type pipeClosingReadCloser struct {
 	pipeclose func() error
 	io.ReadCloser
 	stderr io.ReadCloser
 }
 
 func (p pipeClosingReadCloser) Close() error {
-	if err := p.ReadCloser.Close(); err != nil {
-		return err
-	}
 	// Read the stderr output after the read has finished since we are sure by then the command must have run.
 	stderr, err := io.ReadAll(p.stderr)
 	if err != nil {
@@ -121,9 +135,12 @@ func runCommandWithOutput(cmd *exec.Cmd) (io.ReadCloser, error) {
 	}
 	stderr, err := cmd.StderrPipe()
 	if err != nil {
+		_ = stdout.Close()
 		return nil, err
 	}
 	if err := cmd.Start(); err != nil {
+		_ = stdout.Close()
+		_ = stderr.Close()
 		return nil, err
 	}
 

diff --git a/internal/controllers/vgmanager/exec/test/mock_exec.go b/internal/controllers/vgmanager/exec/test/mock_exec.go
@@ -21,15 +21,20 @@ import (
 	"errors"
 	"io"
 	"strings"
+
+	vgmanagerexec "github.com/openshift/lvm-operator/v4/internal/controllers/vgmanager/exec"
 )
 
 type MockExecutor struct {
 	MockExecuteCommandWithOutputAsHost func(ctx context.Context, command string, arg ...string) (io.ReadCloser, error)
 
-	MockRunCommandAsHost     func(ctx context.Context, command string, arg ...string) error
-	MockRunCommandAsHostInto func(ctx context.Context, into any, command string, arg ...string) error
+	MockRunCommandAsHost            func(ctx context.Context, command string, arg ...string) error
+	MockRunCommandAsHostInto        func(ctx context.Context, into any, command string, arg ...string) error
+	MockCombinedOutputCommandAsHost func(ctx context.Context, command string, arg ...string) ([]byte, error)
 }
 
+var _ vgmanagerexec.Executor = &MockExecutor{}
+
 // StartCommandWithOutputAsHost mocks StartCommandWithOutputAsHost
 func (e *MockExecutor) StartCommandWithOutputAsHost(ctx context.Context, command string, arg ...string) (io.ReadCloser, error) {
 	if e.MockExecuteCommandWithOutputAsHost != nil {
@@ -56,3 +61,15 @@ func (e *MockExecutor) RunCommandAsHostInto(ctx context.Context, into any, comma
 
 	return errors.New("RunCommandAsHostInto not mocked")
 }
+
+func (e *MockExecutor) CombinedOutputCommandAsHost(ctx context.Context, command string, arg ...string) ([]byte, error) {
+	if e.MockCombinedOutputCommandAsHost != nil {
+		return e.MockCombinedOutputCommandAsHost(ctx, command, arg...)
+	}
+
+	return nil, errors.New("CombinedOutputCommandAsHost not mocked")
+}
+
+func (e *MockExecutor) WrapCommandWithNSenter(command string, arg ...string) (string, []string) {
+	return (&vgmanagerexec.CommandExecutor{}).WrapCommandWithNSenter(command, arg...)
+}
diff --git a/internal/controllers/vgmanager/wipefs/wipefs.go b/internal/controllers/vgmanager/wipefs/wipefs.go
@@ -4,9 +4,8 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	exec2 "os/exec"
 
-	"github.com/openshift/lvm-operator/v4/internal/controllers/vgmanager/exec"
+	vgmanagerexec "github.com/openshift/lvm-operator/v4/internal/controllers/vgmanager/exec"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
@@ -19,15 +18,15 @@ type Wipefs interface {
 }
 
 type HostWipefs struct {
-	exec.Executor
+	vgmanagerexec.Executor
 	wipefs string
 }
 
 func NewDefaultHostWipefs() *HostWipefs {
-	return NewHostWipefs(&exec.CommandExecutor{}, DefaultWipefs)
+	return NewHostWipefs(&vgmanagerexec.CommandExecutor{}, DefaultWipefs)
 }
 
-func NewHostWipefs(executor exec.Executor, wipefs string) *HostWipefs {
+func NewHostWipefs(executor vgmanagerexec.Executor, wipefs string) *HostWipefs {
 	return &HostWipefs{
 		Executor: executor,
 		wipefs:   wipefs,
@@ -39,12 +38,7 @@ func (wipefs *HostWipefs) Wipe(ctx context.Context, deviceName string) error {
 	if len(deviceName) == 0 {
 		return fmt.Errorf("failed to wipe the device. Device name is empty")
 	}
-	if output, err := exec2.CommandContext(ctx, "nsenter",
-		append(
-			[]string{"-m", "-u", "-i", "-n", "-p", "-t", "1"},
-			[]string{wipefs.wipefs, "--all", "--force", deviceName}...,
-		)...,
-	).CombinedOutput(); err != nil {
+	if output, err := wipefs.CombinedOutputCommandAsHost(ctx, wipefs.wipefs, "--all", "--force", deviceName); err != nil {
 		return fmt.Errorf("failed to wipe the device %q. %v", deviceName, errors.Join(err, errors.New(string(output))))
 	} else {
 		log.FromContext(ctx).Info(fmt.Sprintf("successfully wiped the device %q: %s", deviceName, string(output)))

diff --git a/internal/controllers/vgmanager/wipefs/wipefs_test.go b/internal/controllers/vgmanager/wipefs/wipefs_test.go
@@ -24,16 +24,16 @@ func TestWipe(t *testing.T) {
 	}
 
 	executor := &mockExec.MockExecutor{
-		MockRunCommandAsHost: func(ctx context.Context, command string, args ...string) error {
+		MockCombinedOutputCommandAsHost: func(ctx context.Context, command string, args ...string) ([]byte, error) {
 			if args[0] != "--all" || args[1] != "--force" {
-				return fmt.Errorf("invalid args %q", args[0:2])
+				return nil, fmt.Errorf("invalid args %q", args[0:2])
 			}
 			if args[2] == "/dev/loop1" {
-				return nil
+				return nil, nil
 			} else if args[2] == "/dev/loop2" {
-				return errors.New("no such file or directory")
+				return nil, errors.New("no such file or directory")
 			}
-			return fmt.Errorf("invalid args %q", args[2])
+			return nil, fmt.Errorf("invalid args %q", args[2])
 		},
 	}