From 04251c9dab90a8af60bcec398c0957afc7ec4588 Mon Sep 17 00:00:00 2001 From: Blake Devcich Date: Wed, 20 Nov 2024 11:45:26 -0600 Subject: [PATCH 1/2] Fix aggressive lvRemove and vgRemove The default storage profile was only using the $VG_NAME, causing lvRemove to remove all logical volumes in the volume group. Additionally, volume groups were being removed without first checking to see if logical volumes exist in the volume group. While both of these issues have no real affect on the removal of the LVs/VGs, this was causing issues with the new PreUnmount commands. The first logical volume to run the lvRemove command was the only fileysystem to run the PreUnmount commands, since it destroys all the other filesystems on the rabbit in that single volume group. With this change, the PreUnmmount command runs on each filesystem before it is destroyed. Signed-off-by: Blake Devcich --- config/examples/nnf_nnfstorageprofile.yaml | 6 +++--- pkg/blockdevice/lvm.go | 12 +++++++++++- pkg/blockdevice/lvm/logical_volumes.go | 10 +++++----- pkg/blockdevice/lvm/lvs.go | 2 +- pkg/blockdevice/lvm/volume_groups.go | 4 ++-- 5 files changed, 22 insertions(+), 12 deletions(-) diff --git a/config/examples/nnf_nnfstorageprofile.yaml b/config/examples/nnf_nnfstorageprofile.yaml index 73b0a676b..d8c39dec5 100644 --- a/config/examples/nnf_nnfstorageprofile.yaml +++ b/config/examples/nnf_nnfstorageprofile.yaml @@ -50,7 +50,7 @@ data: lvChange: activate: --activate ys $VG_NAME/$LV_NAME deactivate: --activate n $VG_NAME/$LV_NAME - lvRemove: $VG_NAME + lvRemove: $VG_NAME/$LV_NAME mkfs: -j2 -p $PROTOCOL -t $CLUSTER_NAME:$LOCK_SPACE $DEVICE mountRabbit: $DEVICE $MOUNT_PATH mountCompute: $DEVICE $MOUNT_PATH @@ -70,7 +70,7 @@ data: lvChange: activate: --activate y $VG_NAME/$LV_NAME deactivate: --activate n $VG_NAME/$LV_NAME - lvRemove: $VG_NAME + lvRemove: $VG_NAME/$LV_NAME mkfs: $DEVICE mountRabbit: $DEVICE $MOUNT_PATH mountCompute: $DEVICE $MOUNT_PATH @@ -90,4 +90,4 @@ data: lvChange: activate: --activate y $VG_NAME/$LV_NAME deactivate: --activate n $VG_NAME/$LV_NAME - lvRemove: $VG_NAME + lvRemove: $VG_NAME/$LV_NAME diff --git a/pkg/blockdevice/lvm.go b/pkg/blockdevice/lvm.go index f02f48e8b..eed4dd978 100644 --- a/pkg/blockdevice/lvm.go +++ b/pkg/blockdevice/lvm.go @@ -140,12 +140,22 @@ func (l *Lvm) Destroy(ctx context.Context) (bool, error) { destroyed, err := l.LogicalVolume.Remove(ctx, l.CommandArgs.LvArgs.Remove) if err != nil { return false, err - } if destroyed { objectDestroyed = true } + // Check to ensure the VG has no LVs before removing + lvs, err := lvm.LvsListVolumes(ctx, l.VolumeGroup.Log) + if err != nil { + return false, err + } + for _, lv := range lvs { + if lv.VGName == l.VolumeGroup.Name { + return false, nil + } + } + destroyed, err = l.VolumeGroup.Remove(ctx, l.CommandArgs.VgArgs.Remove) if err != nil { return false, err diff --git a/pkg/blockdevice/lvm/logical_volumes.go b/pkg/blockdevice/lvm/logical_volumes.go index 799c67eec..1bfe51bc7 100644 --- a/pkg/blockdevice/lvm/logical_volumes.go +++ b/pkg/blockdevice/lvm/logical_volumes.go @@ -51,7 +51,7 @@ func NewLogicalVolume(ctx context.Context, name string, vg *VolumeGroup, size in // Exists determines if the LV exists in the OS func (lv *LogicalVolume) Exists(ctx context.Context) (bool, error) { - existingLVs, err := lvsListVolumes(ctx, lv.Log) + existingLVs, err := LvsListVolumes(ctx, lv.Log) if err != nil { return false, err } @@ -98,7 +98,7 @@ func (lv *LogicalVolume) Create(ctx context.Context, rawArgs string) (bool, erro return false, err } - existingLVs, err := lvsListVolumes(ctx, lv.Log) + existingLVs, err := LvsListVolumes(ctx, lv.Log) if err != nil { return false, err } @@ -126,7 +126,7 @@ func (lv *LogicalVolume) Remove(ctx context.Context, rawArgs string) (bool, erro return false, err } - existingLVs, err := lvsListVolumes(ctx, lv.Log) + existingLVs, err := LvsListVolumes(ctx, lv.Log) if err != nil { return false, err } @@ -166,7 +166,7 @@ func (lv *LogicalVolume) Activate(ctx context.Context, rawArgs string) (bool, er return false, nil } - existingLVs, err := lvsListVolumes(ctx, lv.Log) + existingLVs, err := LvsListVolumes(ctx, lv.Log) if err != nil { return false, err } @@ -189,7 +189,7 @@ func (lv *LogicalVolume) Deactivate(ctx context.Context, rawArgs string) (bool, return false, nil } - existingLVs, err := lvsListVolumes(ctx, lv.Log) + existingLVs, err := LvsListVolumes(ctx, lv.Log) if err != nil { return false, err } diff --git a/pkg/blockdevice/lvm/lvs.go b/pkg/blockdevice/lvm/lvs.go index 958343068..8e8934082 100644 --- a/pkg/blockdevice/lvm/lvs.go +++ b/pkg/blockdevice/lvm/lvs.go @@ -43,7 +43,7 @@ type lvsLogicalVolume struct { Size string `json:"lv_size"` } -func lvsListVolumes(ctx context.Context, log logr.Logger) ([]lvsLogicalVolume, error) { +func LvsListVolumes(ctx context.Context, log logr.Logger) ([]lvsLogicalVolume, error) { output, err := command.Run("lvs --nolock --reportformat json", log) if err != nil { return nil, fmt.Errorf("could not list logical volumes: %w", err) diff --git a/pkg/blockdevice/lvm/volume_groups.go b/pkg/blockdevice/lvm/volume_groups.go index acff13d2c..1c5d11a20 100644 --- a/pkg/blockdevice/lvm/volume_groups.go +++ b/pkg/blockdevice/lvm/volume_groups.go @@ -174,11 +174,11 @@ func (vg *VolumeGroup) LockStop(ctx context.Context, rawArgs string) (bool, erro return false, err } - if exists == false { + if !exists { return false, nil } - lvs, err := lvsListVolumes(ctx, vg.Log) + lvs, err := LvsListVolumes(ctx, vg.Log) for _, lv := range lvs { if lv.VGName == vg.Name && lv.Attrs[4] == 'a' { return false, nil From 1e4a84cb25fb6402f4df51641601aa668b861f1d Mon Sep 17 00:00:00 2001 From: Blake Devcich Date: Wed, 20 Nov 2024 14:14:57 -0600 Subject: [PATCH 2/2] Address review comments --- pkg/blockdevice/lvm.go | 10 +++------- pkg/blockdevice/lvm/logical_volumes.go | 10 +++++----- pkg/blockdevice/lvm/lvs.go | 2 +- pkg/blockdevice/lvm/volume_groups.go | 18 +++++++++++++++++- 4 files changed, 26 insertions(+), 14 deletions(-) diff --git a/pkg/blockdevice/lvm.go b/pkg/blockdevice/lvm.go index eed4dd978..52a2e9d52 100644 --- a/pkg/blockdevice/lvm.go +++ b/pkg/blockdevice/lvm.go @@ -146,14 +146,10 @@ func (l *Lvm) Destroy(ctx context.Context) (bool, error) { } // Check to ensure the VG has no LVs before removing - lvs, err := lvm.LvsListVolumes(ctx, l.VolumeGroup.Log) - if err != nil { + if count, err := l.VolumeGroup.NumLVs(ctx); err != nil { return false, err - } - for _, lv := range lvs { - if lv.VGName == l.VolumeGroup.Name { - return false, nil - } + } else if count != 0 { + return objectDestroyed, nil } destroyed, err = l.VolumeGroup.Remove(ctx, l.CommandArgs.VgArgs.Remove) diff --git a/pkg/blockdevice/lvm/logical_volumes.go b/pkg/blockdevice/lvm/logical_volumes.go index 1bfe51bc7..799c67eec 100644 --- a/pkg/blockdevice/lvm/logical_volumes.go +++ b/pkg/blockdevice/lvm/logical_volumes.go @@ -51,7 +51,7 @@ func NewLogicalVolume(ctx context.Context, name string, vg *VolumeGroup, size in // Exists determines if the LV exists in the OS func (lv *LogicalVolume) Exists(ctx context.Context) (bool, error) { - existingLVs, err := LvsListVolumes(ctx, lv.Log) + existingLVs, err := lvsListVolumes(ctx, lv.Log) if err != nil { return false, err } @@ -98,7 +98,7 @@ func (lv *LogicalVolume) Create(ctx context.Context, rawArgs string) (bool, erro return false, err } - existingLVs, err := LvsListVolumes(ctx, lv.Log) + existingLVs, err := lvsListVolumes(ctx, lv.Log) if err != nil { return false, err } @@ -126,7 +126,7 @@ func (lv *LogicalVolume) Remove(ctx context.Context, rawArgs string) (bool, erro return false, err } - existingLVs, err := LvsListVolumes(ctx, lv.Log) + existingLVs, err := lvsListVolumes(ctx, lv.Log) if err != nil { return false, err } @@ -166,7 +166,7 @@ func (lv *LogicalVolume) Activate(ctx context.Context, rawArgs string) (bool, er return false, nil } - existingLVs, err := LvsListVolumes(ctx, lv.Log) + existingLVs, err := lvsListVolumes(ctx, lv.Log) if err != nil { return false, err } @@ -189,7 +189,7 @@ func (lv *LogicalVolume) Deactivate(ctx context.Context, rawArgs string) (bool, return false, nil } - existingLVs, err := LvsListVolumes(ctx, lv.Log) + existingLVs, err := lvsListVolumes(ctx, lv.Log) if err != nil { return false, err } diff --git a/pkg/blockdevice/lvm/lvs.go b/pkg/blockdevice/lvm/lvs.go index 8e8934082..958343068 100644 --- a/pkg/blockdevice/lvm/lvs.go +++ b/pkg/blockdevice/lvm/lvs.go @@ -43,7 +43,7 @@ type lvsLogicalVolume struct { Size string `json:"lv_size"` } -func LvsListVolumes(ctx context.Context, log logr.Logger) ([]lvsLogicalVolume, error) { +func lvsListVolumes(ctx context.Context, log logr.Logger) ([]lvsLogicalVolume, error) { output, err := command.Run("lvs --nolock --reportformat json", log) if err != nil { return nil, fmt.Errorf("could not list logical volumes: %w", err) diff --git a/pkg/blockdevice/lvm/volume_groups.go b/pkg/blockdevice/lvm/volume_groups.go index 1c5d11a20..334d4fd8a 100644 --- a/pkg/blockdevice/lvm/volume_groups.go +++ b/pkg/blockdevice/lvm/volume_groups.go @@ -178,7 +178,7 @@ func (vg *VolumeGroup) LockStop(ctx context.Context, rawArgs string) (bool, erro return false, nil } - lvs, err := LvsListVolumes(ctx, vg.Log) + lvs, err := lvsListVolumes(ctx, vg.Log) for _, lv := range lvs { if lv.VGName == vg.Name && lv.Attrs[4] == 'a' { return false, nil @@ -211,3 +211,19 @@ func (vg *VolumeGroup) Remove(ctx context.Context, rawArgs string) (bool, error) return false, nil } + +func (vg *VolumeGroup) NumLVs(ctx context.Context) (int, error) { + count := 0 + + lvs, err := lvsListVolumes(ctx, vg.Log) + if err != nil { + return count, err + } + for _, lv := range lvs { + if lv.VGName == vg.Name { + count += 1 + } + } + + return count, nil +}