diff --git a/pkg/vgmanager/devices.go b/pkg/vgmanager/devices.go index 5de7d2d7b..1f91afcc7 100644 --- a/pkg/vgmanager/devices.go +++ b/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 } } diff --git a/pkg/vgmanager/devices_test.go b/pkg/vgmanager/devices_test.go index 21db2f788..b29062df2 100644 --- a/pkg/vgmanager/devices_test.go +++ b/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")}, }, }, }, diff --git a/pkg/vgmanager/filter.go b/pkg/vgmanager/filter.go index c0991f79a..f842a8757 100644 --- a/pkg/vgmanager/filter.go +++ b/pkg/vgmanager/filter.go @@ -17,6 +17,7 @@ limitations under the License. package vgmanager import ( + "fmt" "strings" "github.com/openshift/lvm-operator/pkg/internal" @@ -24,14 +25,14 @@ import ( 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) { diff --git a/pkg/vgmanager/filter_test.go b/pkg/vgmanager/filter_test.go index f6af872d2..56f2788fe 100644 --- a/pkg/vgmanager/filter_test.go +++ b/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) diff --git a/pkg/vgmanager/lvm.go b/pkg/vgmanager/lvm.go index 12a2426ab..5377e554f 100644 --- a/pkg/vgmanager/lvm.go +++ b/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{}}) } } diff --git a/pkg/vgmanager/lvm_test.go b/pkg/vgmanager/lvm_test.go index 49ea8c63f..366898ac1 100644 --- a/pkg/vgmanager/lvm_test.go +++ b/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 } } diff --git a/pkg/vgmanager/status.go b/pkg/vgmanager/status.go index 6caeddd81..334f92073 100644 --- a/pkg/vgmanager/status.go +++ b/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 + } } } } diff --git a/pkg/vgmanager/vgmanager_controller.go b/pkg/vgmanager/vgmanager_controller.go index 633ab9757..d47106b2e 100644 --- a/pkg/vgmanager/vgmanager_controller.go +++ b/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