From d6989bc9899dca038cac6566593efd5f35ffeea1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20M=C3=B6ller?= Date: Wed, 7 Aug 2024 14:00:03 +0200 Subject: [PATCH 1/2] fix: correct wiping behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../lvmcluster/resource/csi_node.go | 5 +++- .../controllers/vgmanager/dmsetup/dmsetup.go | 25 ++++++++----------- internal/controllers/vgmanager/exec/exec.go | 7 ++++-- .../controllers/vgmanager/wipe_devices.go | 22 +++++++++++----- .../controllers/vgmanager/wipefs/wipefs.go | 18 ++++++++----- 5 files changed, 48 insertions(+), 29 deletions(-) diff --git a/internal/controllers/lvmcluster/resource/csi_node.go b/internal/controllers/lvmcluster/resource/csi_node.go index 0cca3a2c1..427f7c7b4 100644 --- a/internal/controllers/lvmcluster/resource/csi_node.go +++ b/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) } } diff --git a/internal/controllers/vgmanager/dmsetup/dmsetup.go b/internal/controllers/vgmanager/dmsetup/dmsetup.go index 333e9d57a..6057b5f2e 100644 --- a/internal/controllers/vgmanager/dmsetup/dmsetup.go +++ b/internal/controllers/vgmanager/dmsetup/dmsetup.go @@ -5,9 +5,10 @@ import ( "context" "errors" "fmt" - "io" + exec2 "os/exec" "github.com/openshift/lvm-operator/v4/internal/controllers/vgmanager/exec" + "sigs.k8s.io/controller-runtime/pkg/log" ) var ( @@ -41,23 +42,19 @@ 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 := exec2.CommandContext(ctx, "nsenter", + append( + []string{"-m", "-u", "-i", "-n", "-p", "-t", "1"}, + []string{dmsetup.dmsetup, "remove", "--force", deviceName}..., + )..., + ).CombinedOutput() + 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) diff --git a/internal/controllers/vgmanager/exec/exec.go b/internal/controllers/vgmanager/exec/exec.go index a5962c369..530066985 100644 --- a/internal/controllers/vgmanager/exec/exec.go +++ b/internal/controllers/vgmanager/exec/exec.go @@ -80,7 +80,7 @@ func (e *CommandExecutor) RunCommandAsHostInto(ctx context.Context, into any, co 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()) + log.FromContext(ctx).Info("executing", "command", cmd.String()) return runCommandWithOutput(cmd) } @@ -94,13 +94,16 @@ 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{ diff --git a/internal/controllers/vgmanager/wipe_devices.go b/internal/controllers/vgmanager/wipe_devices.go index 46fc2786a..205f8fe8f 100644 --- a/internal/controllers/vgmanager/wipe_devices.go +++ b/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) diff --git a/internal/controllers/vgmanager/wipefs/wipefs.go b/internal/controllers/vgmanager/wipefs/wipefs.go index b97f6cb0c..40174e66f 100644 --- a/internal/controllers/vgmanager/wipefs/wipefs.go +++ b/internal/controllers/vgmanager/wipefs/wipefs.go @@ -2,9 +2,12 @@ package wipefs import ( "context" + "errors" "fmt" + exec2 "os/exec" "github.com/openshift/lvm-operator/v4/internal/controllers/vgmanager/exec" + "sigs.k8s.io/controller-runtime/pkg/log" ) var ( @@ -36,12 +39,15 @@ 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 := exec2.CommandContext(ctx, "nsenter", + append( + []string{"-m", "-u", "-i", "-n", "-p", "-t", "1"}, + []string{wipefs.wipefs, "--all", "--force", deviceName}..., + )..., + ).CombinedOutput(); 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 } From d80e31177f0be72752e9ab763564c2002e0b178f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20M=C3=B6ller?= Date: Wed, 7 Aug 2024 14:55:52 +0200 Subject: [PATCH 2/2] fix: correct wiping behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../controllers/vgmanager/dmsetup/dmsetup.go | 16 +++------- .../vgmanager/dmsetup/dmsetup_test.go | 19 ++++++------ internal/controllers/vgmanager/exec/exec.go | 31 ++++++++++++++----- .../vgmanager/exec/test/mock_exec.go | 21 +++++++++++-- .../controllers/vgmanager/wipefs/wipefs.go | 16 +++------- .../vgmanager/wipefs/wipefs_test.go | 10 +++--- 6 files changed, 68 insertions(+), 45 deletions(-) diff --git a/internal/controllers/vgmanager/dmsetup/dmsetup.go b/internal/controllers/vgmanager/dmsetup/dmsetup.go index 6057b5f2e..1990cf0c7 100644 --- 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 index b813ee356..9f48b4fca 100644 --- 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 index 530066985..afcfd71ba 100644 --- 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,13 +89,18 @@ 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 @@ -91,9 +108,6 @@ type pipeClosingReadCloser struct { } 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 index ac16ca875..0f7b1726a 100644 --- 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 index 40174e66f..2c9ad5232 100644 --- 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 index 8a123cf24..207e28cff 100644 --- 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]) }, }