Merge pull request #404 from jakobmoellerdev/OCPBUGS-18397-vgdelete

OCPBUGS-18397: fix: allow vg to be gone during deletion and pick up empty pvs

pkg/vgmanager/devices.go

@@ -190,7 +190,7 @@ func isDeviceAlreadyPartOfVG(vgs []VolumeGroup, diskName string, volumeGroup *lv
 	for _, vg := range vgs {
 		if vg.Name == volumeGroup.Name {
 			for _, pv := range vg.PVs {
-				if pv == diskName {
+				if pv.PvName == diskName {
 					return true
 				}
 			}

pkg/vgmanager/devices_test.go

@@ -313,9 +313,9 @@ func TestAvailableDevicesForVG(t *testing.T) {
 			existingVGs: []VolumeGroup{
 				{
 					Name: "vg1",
-					PVs: []string{
-						calculateDevicePath(t, "nvme1n1p1"),
-						calculateDevicePath(t, "nvme1n1p2"),
+					PVs: []PhysicalVolume{
+						{PvName: calculateDevicePath(t, "nvme1n1p1")},
+						{PvName: calculateDevicePath(t, "nvme1n1p2")},
 					},
 				},
 			},

pkg/vgmanager/filter.go

@@ -17,21 +17,22 @@ limitations under the License.
 package vgmanager
 
 import (
+	"fmt"
 	"strings"
 
 	"github.com/openshift/lvm-operator/pkg/internal"
 )
 
 const (
 	// filter names:
-	notReadOnly           = "notReadOnly"
-	notSuspended          = "notSuspended"
-	noBiosBootInPartLabel = "noBiosBootInPartLabel"
-	noReservedInPartLabel = "noReservedInPartLabel"
-	noFilesystemSignature = "noFilesystemSignature"
-	noBindMounts          = "noBindMounts"
-	noChildren            = "noChildren"
-	usableDeviceType      = "usableDeviceType"
+	notReadOnly                = "notReadOnly"
+	notSuspended               = "notSuspended"
+	noBiosBootInPartLabel      = "noBiosBootInPartLabel"
+	noReservedInPartLabel      = "noReservedInPartLabel"
+	noValidFilesystemSignature = "noValidFilesystemSignature"
+	noBindMounts               = "noBindMounts"
+	noChildren                 = "noChildren"
+	usableDeviceType           = "usableDeviceType"
 )
 
 // maps of function identifier (for logs) to filter function.
@@ -59,9 +60,37 @@ var FilterMap = map[string]func(internal.BlockDevice, internal.Executor) (bool,
 		return !reservedInPartLabel, nil
 	},
 
-	noFilesystemSignature: func(dev internal.BlockDevice, _ internal.Executor) (bool, error) {
-		matched := dev.FSType == ""
-		return matched, nil
+	noValidFilesystemSignature: func(dev internal.BlockDevice, e internal.Executor) (bool, error) {
+		// if no fs type is set, it's always okay
+		if dev.FSType == "" {
+			return true, nil
+		}
+
+		// if fstype is set to LVM2_member then it already was created as a PV
+		// this means that if the disk has no children, we can safely reuse it if it's a valid LVM PV.
+		if dev.FSType == "LVM2_member" && !dev.HasChildren() {
+			pvs, err := ListPhysicalVolumes(e, "")
+			if err != nil {
+				return false, fmt.Errorf("could not determine if block device has valid filesystem signature, since it is flagged as LVM2_member but physical volumes could not be verified: %w", err)
+			}
+
+			for _, pv := range pvs {
+				// a volume is a valid PV if it has the same name as the block device and no associated volume group and has available disk space
+				// however if there is a PV that matches the Device and there is a VG associated with it or no available space, we cannot use it
+				if pv.PvName == dev.KName {
+					if pv.VgName == "" && pv.PvFree != "0G" {
+						return true, nil
+					} else {
+						return false, nil
+					}
+				}
+			}
+
+			// if there was no PV that matched it and it still is flagged as LVM2_member, it is formatted but not recognized by LVM
+			// configuration. We can assume that in this case, the Volume can be reused by simply recalling the vgcreate command on it
+			return true, nil
+		}
+		return false, nil
 	},
 
 	noBindMounts: func(dev internal.BlockDevice, _ internal.Executor) (bool, error) {

pkg/vgmanager/filter_test.go

@@ -54,7 +54,7 @@ func TestNoFilesystemSignature(t *testing.T) {
 		{label: "tc swap", device: internal.BlockDevice{FSType: "swap"}, expected: false, expectErr: false},
 	}
 	for _, tc := range testcases {
-		result, err := FilterMap[noFilesystemSignature](tc.device, nil)
+		result, err := FilterMap[noValidFilesystemSignature](tc.device, nil)
 		assert.Equal(t, tc.expected, result)
 		if tc.expectErr {
 			assert.Error(t, err)

pkg/vgmanager/lvm.go

@@ -18,7 +18,9 @@ package vgmanager
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
+	"os/exec"
 
 	"github.com/openshift/lvm-operator/pkg/internal"
 )
@@ -32,15 +34,16 @@ var (
 )
 
 const (
-	lvmCmd      = "/usr/sbin/lvm"
-	vgCreateCmd = "/usr/sbin/vgcreate"
-	vgExtendCmd = "/usr/sbin/vgextend"
-	vgRemoveCmd = "/usr/sbin/vgremove"
-	pvRemoveCmd = "/usr/sbin/pvremove"
-	lvCreateCmd = "/usr/sbin/lvcreate"
-	lvExtendCmd = "/usr/sbin/lvextend"
-	lvRemoveCmd = "/usr/sbin/lvremove"
-	lvChangeCmd = "/usr/sbin/lvchange"
+	lvmCmd        = "/usr/sbin/lvm"
+	vgCreateCmd   = "/usr/sbin/vgcreate"
+	vgExtendCmd   = "/usr/sbin/vgextend"
+	vgRemoveCmd   = "/usr/sbin/vgremove"
+	pvRemoveCmd   = "/usr/sbin/pvremove"
+	lvCreateCmd   = "/usr/sbin/lvcreate"
+	lvExtendCmd   = "/usr/sbin/lvextend"
+	lvRemoveCmd   = "/usr/sbin/lvremove"
+	lvChangeCmd   = "/usr/sbin/lvchange"
+	lvmDevicesCmd = "/usr/sbin/lvmdevices"
 )
 
 // vgsOutput represents the output of the `vgs --reportformat json` command
@@ -56,10 +59,7 @@ type vgsOutput struct {
 // pvsOutput represents the output of the `pvs --reportformat json` command
 type pvsOutput struct {
 	Report []struct {
-		Pv []struct {
-			Name   string `json:"pv_name"`
-			VgName string `json:"vg_name"`
-		} `json:"pv"`
+		Pv []PhysicalVolume `json:"pv"`
 	} `json:"report"`
 }
 
@@ -86,7 +86,34 @@ type VolumeGroup struct {
 	VgSize string `json:"vg_size"`
 
 	// PVs is the list of physical volumes associated with the volume group
-	PVs []string `json:"pvs"`
+	PVs []PhysicalVolume `json:"pvs"`
+}
+
+// PhysicalVolume represents a physical volume of linux lvm.
+type PhysicalVolume struct {
+	// PvName is the name of the Physical Volume
+	PvName string `json:"pv_name"`
+
+	// UUID is the unique identifier of the Physical Volume used in the devices file
+	UUID string `json:"pv_uuid"`
+
+	// VgName is the name of the associated Volume Group, if any
+	VgName string `json:"vg_name"`
+
+	// PvFmt is the file format of the PhysicalVolume
+	PvFmt string `json:"pv_fmt"`
+
+	// PvAttr describes the attributes of the PhysicalVolume
+	PvAttr string `json:"pv_attr"`
+
+	// PvSize describes the total space of the PhysicalVolume
+	PvSize string `json:"pv_size"`
+
+	// PvFree describes the free space of the PhysicalVolume
+	PvFree string `json:"pv_free"`
+
+	// DevSize describes the size of the underlying device on which the PhysicalVolume was created
+	DevSize string `json:"dev_size"`
 }
 
 // Create creates a new volume group
@@ -132,20 +159,39 @@ func (vg VolumeGroup) Extend(exec internal.Executor, pvs []string) error {
 }
 
 // Delete deletes a volume group and the physical volumes associated with it
-func (vg VolumeGroup) Delete(exec internal.Executor) error {
+func (vg VolumeGroup) Delete(e internal.Executor) error {
 	// Remove Volume Group
 	vgArgs := []string{vg.Name}
-	_, err := exec.ExecuteCommandWithOutputAsHost(vgRemoveCmd, vgArgs...)
+	_, err := e.ExecuteCommandWithOutputAsHost(vgRemoveCmd, vgArgs...)
 	if err != nil {
-		return fmt.Errorf("failed to remove volume group %q. %v", vg.Name, err)
+		return fmt.Errorf("failed to remove volume group %s: %w", vg.Name, err)
 	}
 
 	// Remove physical volumes
-	pvArgs := vg.PVs
-	_, err = exec.ExecuteCommandWithOutputAsHost(pvRemoveCmd, pvArgs...)
+	pvArgs := make([]string, len(vg.PVs))
+	for i := range vg.PVs {
+		pvArgs[i] = vg.PVs[i].PvName
+	}
+	_, err = e.ExecuteCommandWithOutputAsHost(pvRemoveCmd, pvArgs...)
 	if err != nil {
-		return fmt.Errorf("failed to remove physical volumes for the volume group %q. %v", vg.Name, err)
+		return fmt.Errorf("failed to remove physical volumes for the volume group %s: %w", vg.Name, err)
 	}
+
+	for _, pv := range vg.PVs {
+		_, err = e.ExecuteCommandWithOutput(lvmDevicesCmd, "--delpvid", pv.UUID)
+		if err != nil {
+			var exitError *exec.ExitError
+			if errors.As(err, &exitError) {
+				switch exitError.ExitCode() {
+				// Exit Code 5 On lvmdevices --delpvid means that the PV with that UUID no longer exists
+				case 5:
+					continue
+				}
+			}
+			return fmt.Errorf("failed to delete PV %s from device file for the volume group %s: %w", pv.UUID, vg.Name, err)
+		}
+	}
+
 	return nil
 }
 
@@ -188,19 +234,32 @@ func GetVolumeGroup(exec internal.Executor, name string) (*VolumeGroup, error) {
 }
 
 // ListPhysicalVolumes returns list of physical volumes used to create the given volume group
-func ListPhysicalVolumes(exec internal.Executor, vgName string) ([]string, error) {
+func ListPhysicalVolumes(exec internal.Executor, vgName string) ([]PhysicalVolume, error) {
 	res := new(pvsOutput)
 	args := []string{
-		"pvs", "-S", fmt.Sprintf("vgname=%s", vgName), "--reportformat", "json",
+		"pvs", "--units", "g", "-v", "--reportformat", "json",
+	}
+	if vgName != "" {
+		args = append(args, "-S", fmt.Sprintf("vgname=%s", vgName))
 	}
+
 	if err := execute(exec, res, args...); err != nil {
-		return []string{}, err
+		return nil, err
 	}
 
-	pvs := []string{}
+	pvs := []PhysicalVolume{}
 	for _, report := range res.Report {
 		for _, pv := range report.Pv {
-			pvs = append(pvs, pv.Name)
+			pvs = append(pvs, PhysicalVolume{
+				PvName:  pv.PvName,
+				UUID:    pv.UUID,
+				VgName:  pv.VgName,
+				PvFmt:   pv.PvFmt,
+				PvAttr:  pv.PvAttr,
+				PvSize:  pv.PvSize,
+				PvFree:  pv.PvFree,
+				DevSize: pv.DevSize,
+			})
 		}
 	}
 	return pvs, nil
@@ -220,7 +279,7 @@ func ListVolumeGroups(exec internal.Executor) ([]VolumeGroup, error) {
 	vgList := []VolumeGroup{}
 	for _, report := range res.Report {
 		for _, vg := range report.Vg {
-			vgList = append(vgList, VolumeGroup{Name: vg.Name, PVs: []string{}})
+			vgList = append(vgList, VolumeGroup{Name: vg.Name, PVs: []PhysicalVolume{}})
 		}
 	}
 

pkg/vgmanager/lvm_test.go

@@ -79,9 +79,11 @@ func TestGetVolumeGroup(t *testing.T) {
 			if args[0] == "vgs" {
 				return mockVgsOutput, nil
 			} else if args[0] == "pvs" {
-				if strings.HasSuffix(args[2], "vg1") {
+				argsConcat := strings.Join(args, " ")
+				out := "pvs --units g -v --reportformat json -S vgname=%s"
+				if argsConcat == fmt.Sprintf(out, "vg1") {
 					return mockPvsOutputForVG1, nil
-				} else if strings.HasSuffix(args[2], "vg2") {
+				} else if argsConcat == fmt.Sprintf(out, "vg2") {
 					return mockPvsOutputForVG2, nil
 				}
 			}
@@ -115,9 +117,11 @@ func TestListVolumeGroup(t *testing.T) {
 			if args[0] == "vgs" {
 				return mockVgsOutput, nil
 			} else if args[0] == "pvs" {
-				if strings.HasSuffix(args[2], "vg1") {
+				argsConcat := strings.Join(args, " ")
+				out := "pvs --units g -v --reportformat json -S vgname=%s"
+				if argsConcat == fmt.Sprintf(out, "vg1") {
 					return mockPvsOutputForVG1, nil
-				} else if strings.HasSuffix(args[2], "vg2") {
+				} else if argsConcat == fmt.Sprintf(out, "vg2") {
 					return mockPvsOutputForVG2, nil
 				}
 			}

pkg/vgmanager/status.go

@@ -175,7 +175,10 @@ func (r *VGReconciler) setDevices(status *lvmv1alpha1.VGStatus) (bool, error) {
 		if status.Name == vg.Name {
 			if len(vg.PVs) > 0 {
 				devicesExist = true
-				status.Devices = vg.PVs
+				status.Devices = make([]string, len(vg.PVs))
+				for i := range vg.PVs {
+					status.Devices[i] = vg.PVs[i].PvName
+				}
 			}
 		}
 	}

pkg/vgmanager/vgmanager_controller.go

@@ -324,37 +324,40 @@ func (r *VGReconciler) processDelete(ctx context.Context, volumeGroup *lvmv1alph
 		if err != ErrVolumeGroupNotFound {
 			return fmt.Errorf("failed to get volume group %s, %w", volumeGroup.GetName(), err)
 		}
-		return nil
-	}
+		logger.Info("volume group not found, assuming it was already deleted and continuing")
+	} else {
+		// Delete thin pool
+		if volumeGroup.Spec.ThinPoolConfig != nil {
+			thinPoolName := volumeGroup.Spec.ThinPoolConfig.Name
+			logger := logger.WithValues("ThinPool", thinPoolName)
 
-	// Delete thin pool
-	if volumeGroup.Spec.ThinPoolConfig != nil {
-		thinPoolName := volumeGroup.Spec.ThinPoolConfig.Name
-		lvExists, err := LVExists(r.executor, thinPoolName, volumeGroup.Name)
-		if err != nil {
-			return fmt.Errorf("failed to check existence of thin pool %q in volume group %q. %v", thinPoolName, volumeGroup.Name, err)
-		}
+			thinPoolExists, err := LVExists(r.executor, thinPoolName, volumeGroup.Name)
+			if err != nil {
+				return fmt.Errorf("failed to check existence of thin pool %q in volume group %q. %v", thinPoolName, volumeGroup.Name, err)
+			}
 
-		if lvExists {
-			if err := DeleteLV(r.executor, thinPoolName, volumeGroup.Name); err != nil {
-				err := fmt.Errorf("failed to delete thin pool %s in volume group %s: %w", thinPoolName, volumeGroup.Name, err)
-				if err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, err); err != nil {
-					logger.Error(err, "failed to set status to failed", "VGName", volumeGroup.GetName())
+			if thinPoolExists {
+				if err := DeleteLV(r.executor, thinPoolName, volumeGroup.Name); err != nil {
+					err := fmt.Errorf("failed to delete thin pool %s in volume group %s: %w", thinPoolName, volumeGroup.Name, err)
+					if err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, err); err != nil {
+						logger.Error(err, "failed to set status to failed")
+					}
+					return err
 				}
-				return err
+				logger.Info("thin pool deleted")
+			} else {
+				logger.Info("thin pool not found, assuming it was already deleted and continuing")
 			}
-			logger.Info("thin pool deleted in the volume group", "VGName", volumeGroup.Name, "ThinPool", thinPoolName)
-		} else {
-			logger.Info("thin pool not found in the volume group", "VGName", volumeGroup.Name, "ThinPool", thinPoolName)
 		}
-	}
 
-	if err = vg.Delete(r.executor); err != nil {
-		err := fmt.Errorf("failed to delete volume group %s: %w", volumeGroup.Name, err)
-		if err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, err); err != nil {
-			logger.Error(err, "failed to set status to failed", "VGName", volumeGroup.GetName())
+		if err = vg.Delete(r.executor); err != nil {
+			err := fmt.Errorf("failed to delete volume group %s: %w", volumeGroup.Name, err)
+			if err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, err); err != nil {
+				logger.Error(err, "failed to set status to failed", "VGName", volumeGroup.GetName())
+			}
+			return err
 		}
-		return err
+		logger.Info("volume group deleted")
 	}
 
 	// Remove this vg from the lvmdconf file