Merge pull request #687 from jakobmoellerdev/wipe-command-fix

OCPBUGS-37711: fix: ensure wipefs / dmsetup remove use CombinedOutput to wait for both STDERR and STDOUT before continuing

internal/controllers/lvmcluster/resource/csi_node.go

@@ -72,7 +72,10 @@ func (c csiNode) EnsureDeleted(r Reconciler, ctx context.Context, cluster *lvmv1
 			}
 		}
 		if found {
-			return fmt.Errorf("csi node %s does not have driver %s", csiNode.Name, constants.TopolvmCSIDriverName)
+			return fmt.Errorf("csi node %s still has driver %s, "+
+				"if you think this is by mistake, "+
+				"manually login to the node and remove the driver from the kubelets plugin directory",
+				csiNode.Name, constants.TopolvmCSIDriverName)
 		}
 	}
 

internal/controllers/vgmanager/dmsetup/dmsetup.go

@@ -5,9 +5,9 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"io"
 
-	"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"
 )
 
 var (
@@ -20,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,
@@ -41,23 +41,14 @@ func (dmsetup *HostDmsetup) Remove(ctx context.Context, deviceName string) error
 		return errors.New("failed to remove device-mapper reference. Device name is empty")
 	}
 
-	args := []string{"remove"}
-	args = append(args, deviceName)
-	output, err := dmsetup.StartCommandWithOutputAsHost(ctx, dmsetup.dmsetup, args...)
+	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)))
 		return nil
 	}
 
-	// if err != nil (ExitCode != 0), we can check the cmd output to verify if we have a non-found device
-	data, err := io.ReadAll(output)
-	if err != nil {
-		return fmt.Errorf("failed to read output from device-mapper %q: %w", deviceName, err)
-	}
-	if err := output.Close(); err != nil {
-		return fmt.Errorf("failed to close output from device-mapper %q: %w", deviceName, err)
-	}
-
-	if bytes.Contains(data, []byte("not found")) {
+	if bytes.Contains(output, []byte("not found")) {
 		return ErrReferenceNotFound
 	}
 	return fmt.Errorf("failed to remove the reference from device-mapper %q: %w", deviceName, err)

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])
 		},
 	}
 

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,30 +89,35 @@ 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...)
-	log.FromContext(ctx).V(1).Info("executing", "command", cmd.String())
+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 {
 		return err
 	}
 
+	if err := p.stderr.Close(); err != nil {
+		return err
+	}
+
 	if err := p.pipeclose(); err != nil {
 		// wait can result in an exit code error
 		return &internalError{
@@ -118,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
 	}
 

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...)
+}

internal/controllers/vgmanager/wipe_devices.go

@@ -97,16 +97,19 @@ func (r *Reconciler) wipeDevice(ctx context.Context, deviceName string, blockDev
 	wiped := false
 	for _, device := range blockDevices {
 		if device.KName == deviceName {
+			// remove all references that were just orphaned
+			for _, child := range device.Children {
+				// all mapper references must be removed before wiping the device
+				r.removeMapperReference(ctx, child)
+			}
+			logger.Info("wipe device", "deviceName", deviceName)
+			// wipe all signatures once more and cause ioctl reload
 			if err := r.Wipefs.Wipe(ctx, device.KName); err != nil {
 				return false, err
 			}
-			wiped = true
 			logger.Info("device wiped successfully")
-			for _, child := range device.Children {
-				// If the device was used as a Physical Volume before, wipefs does not remove the child LVs.
-				// So, a device-mapper reference removal is necessary to further remove the child LV references.
-				r.removeMapperReference(ctx, child)
-			}
+			wiped = true
+			break
 		} else if device.HasChildren() {
 			childWiped, err := r.wipeDevice(ctx, deviceName, device.Children)
 			if err != nil {
@@ -128,6 +131,13 @@ func (r *Reconciler) removeMapperReference(ctx context.Context, device lsblk.Blo
 			r.removeMapperReference(ctx, child)
 		}
 	}
+	if device.Type == "part" {
+		logger.Info("skipping the removal of device-mapper reference as the device is a partition", "childName", device.KName)
+		return
+	} else {
+		logger.Info("removing device-mapper reference", "childName", device.KName, "deviceType", device.Type)
+	}
+
 	if err := r.Dmsetup.Remove(ctx, device.KName); err != nil {
 		if errors.Is(err, dmsetup.ErrReferenceNotFound) {
 			logger.Info("skipping the removal of device-mapper reference as the reference does not exist", "childName", device.KName)

internal/controllers/vgmanager/wipefs/wipefs.go

@@ -2,9 +2,11 @@ package wipefs
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
-	"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"
 )
 
 var (
@@ -16,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,
@@ -36,12 +38,10 @@ 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")
 	}
-
-	args := []string{"--all", "--force"}
-	args = append(args, deviceName)
-	if err := wipefs.RunCommandAsHost(ctx, wipefs.wipefs, args...); err != nil {
-		return fmt.Errorf("failed to wipe the device %q. %v", deviceName, err)
+	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)))
 	}
-
 	return nil
 }

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])
 		},
 	}