Skip to content

Commit

Permalink
Merge pull request #410 from jakobmoellerdev/OCPVE-677-lvm-filter-sep…
Browse files Browse the repository at this point in the history
…aration

OCPVE-677: chore: separate filter and lvm package from vgmanager
openshift-merge-robot authored Sep 8, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
2 parents 3386fe2 + e0a0dd1 commit 8208b3a
Showing 8 changed files with 152 additions and 74 deletions.
5 changes: 3 additions & 2 deletions pkg/vgmanager/filter.go → pkg/filter/filter.go
Original file line number Diff line number Diff line change
@@ -14,13 +14,14 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package vgmanager
package filter

import (
"fmt"
"strings"

"github.com/openshift/lvm-operator/pkg/internal"
"github.com/openshift/lvm-operator/pkg/lvm"
)

const (
@@ -69,7 +70,7 @@ var FilterMap = map[string]func(internal.BlockDevice, internal.Executor) (bool,
// 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, "")
pvs, err := lvm.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)
}
2 changes: 1 addition & 1 deletion pkg/vgmanager/filter_test.go → pkg/filter/filter_test.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package vgmanager
package filter

import (
"testing"
98 changes: 87 additions & 11 deletions pkg/vgmanager/lvm.go → pkg/lvm/lvm.go
Original file line number Diff line number Diff line change
@@ -14,13 +14,14 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package vgmanager
package lvm

import (
"encoding/json"
"errors"
"fmt"
"os/exec"
"strings"

"github.com/openshift/lvm-operator/pkg/internal"
)
@@ -34,16 +35,17 @@ 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"
lvmDevicesCmd = "/usr/sbin/lvmdevices"
defaultChunkSize = "128"
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
@@ -158,6 +160,55 @@ func (vg VolumeGroup) Extend(exec internal.Executor, pvs []string) error {
return nil
}

// CreateVG creates a new volume group
func (vg VolumeGroup) CreateVG(exec internal.Executor) error {
if vg.Name == "" {
return fmt.Errorf("failed to create volume group. Volume group name is empty")
}

if len(vg.PVs) == 0 {
return fmt.Errorf("failed to create volume group. Physical volume list is empty")
}

args := []string{vg.Name}

for _, pv := range vg.PVs {
args = append(args, pv.PvName)
}

_, err := exec.ExecuteCommandWithOutputAsHost(vgCreateCmd, args...)
if err != nil {
return fmt.Errorf("failed to create volume group %q. %v", vg.Name, err)
}

return nil
}

// ExtendVG extends the volume group only if new physical volumes are available
func (vg VolumeGroup) ExtendVG(exec internal.Executor, pvs []string) (VolumeGroup, error) {
if vg.Name == "" {
return VolumeGroup{}, fmt.Errorf("failed to extend volume group. Volume group name is empty")
}

if len(pvs) == 0 {
return VolumeGroup{}, fmt.Errorf("failed to extend volume group. Physical volume list is empty")
}

args := []string{vg.Name}
args = append(args, pvs...)

_, err := exec.ExecuteCommandWithOutputAsHost(vgExtendCmd, args...)
if err != nil {
return VolumeGroup{}, fmt.Errorf("failed to extend volume group %q. %v", vg.Name, err)
}

for _, pv := range pvs {
vg.PVs = append(vg.PVs, PhysicalVolume{PvName: pv})
}

return vg, nil
}

// Delete deletes a volume group and the physical volumes associated with it
func (vg VolumeGroup) Delete(e internal.Executor) error {
// Remove Volume Group
@@ -359,6 +410,31 @@ func DeleteLV(exec internal.Executor, lvName, vgName string) error {
return nil
}

// CreateLV creates the logical volume
func CreateLV(exec internal.Executor, lvName, vgName string, sizePercent int) error {
args := []string{"-l", fmt.Sprintf("%d%%FREE", sizePercent),
"-c", defaultChunkSize, "-Z", "y", "-T", fmt.Sprintf("%s/%s", vgName, lvName)}

if _, err := exec.ExecuteCommandWithOutputAsHost(lvCreateCmd, args...); err != nil {
return fmt.Errorf("failed to create logical volume %q in the volume group %q using command '%s': %w",
lvName, vgName, fmt.Sprintf("%s %s", lvCreateCmd, strings.Join(args, " ")), err)
}

return nil
}

// ExtendLV extends the logical volume
func ExtendLV(exec internal.Executor, lvName, vgName string, sizePercent int) error {
args := []string{"-l", fmt.Sprintf("%d%%Vg", sizePercent), fmt.Sprintf("%s/%s", vgName, lvName)}

if _, err := exec.ExecuteCommandWithOutputAsHost(lvExtendCmd, args...); err != nil {
return fmt.Errorf("failed to extend logical volume %q in the volume group %q using command '%s': %w",
lvName, vgName, fmt.Sprintf("%s %s", lvExtendCmd, strings.Join(args, " ")), err)
}

return nil
}

func execute(exec internal.Executor, v interface{}, args ...string) error {
output, err := exec.ExecuteCommandWithOutputAsHost(lvmCmd, args...)
if err != nil {
2 changes: 1 addition & 1 deletion pkg/vgmanager/lvm_test.go → pkg/lvm/lvm_test.go
Original file line number Diff line number Diff line change
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package vgmanager
package lvm

import (
"fmt"
56 changes: 29 additions & 27 deletions pkg/vgmanager/devices.go
Original file line number Diff line number Diff line change
@@ -21,40 +21,31 @@ import (
"errors"
"fmt"
"path/filepath"
"strings"

lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1"
"github.com/openshift/lvm-operator/pkg/filter"
"github.com/openshift/lvm-operator/pkg/internal"
"github.com/openshift/lvm-operator/pkg/lvm"
"sigs.k8s.io/controller-runtime/pkg/log"
)

// addDevicesToVG creates or extends a volume group using the provided devices.
func (r *VGReconciler) addDevicesToVG(ctx context.Context, vgs []VolumeGroup, vgName string, devices []internal.BlockDevice) error {
func (r *VGReconciler) addDevicesToVG(ctx context.Context, vgs []lvm.VolumeGroup, vgName string, devices []internal.BlockDevice) error {
logger := log.FromContext(ctx)

if len(devices) < 1 {
return fmt.Errorf("can't create vg %q with 0 devices", vgName)
}

// check if volume group is already present
vgFound := false
var existingVolumeGroup *lvm.VolumeGroup
for _, vg := range vgs {
if vg.Name == vgName {
vgFound = true
existingVolumeGroup = &vg
}
}

// TODO: Check if we can use functions from lvm.go here
var cmd string
if vgFound {
logger.Info("extending an existing volume group", "VGName", vgName)
cmd = "/usr/sbin/vgextend"
} else {
logger.Info("creating a new volume group", "VGName", vgName)
cmd = "/usr/sbin/vgcreate"
}

args := []string{vgName}
var args []string
for _, device := range devices {
if device.DevicePath != "" {
args = append(args, device.DevicePath)
@@ -63,16 +54,27 @@ func (r *VGReconciler) addDevicesToVG(ctx context.Context, vgs []VolumeGroup, vg
}
}

_, err := r.executor.ExecuteCommandWithOutputAsHost(cmd, args...)
if err != nil {
return fmt.Errorf("failed to create or extend volume group %q using command '%s': %v", vgName, fmt.Sprintf("%s %s", cmd, strings.Join(args, " ")), err)
if existingVolumeGroup != nil {
logger.Info("extending an existing volume group", "VGName", vgName)
if _, err := existingVolumeGroup.ExtendVG(r.executor, args); err != nil {
return fmt.Errorf("failed to extend volume group %s: %w", vgName, err)
}
} else {
logger.Info("creating a new volume group", "VGName", vgName)
var pvs []lvm.PhysicalVolume
for _, pvName := range args {
pvs = append(pvs, lvm.PhysicalVolume{PvName: pvName})
}
if err := (lvm.VolumeGroup{Name: vgName, PVs: pvs}).CreateVG(r.executor); err != nil {
return fmt.Errorf("failed to create volume group %s: %w", vgName, err)
}
}

return nil
}

// getAvailableDevicesForVG determines the available devices that can be used to create a volume group.
func (r *VGReconciler) getAvailableDevicesForVG(ctx context.Context, blockDevices []internal.BlockDevice, vgs []VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]internal.BlockDevice, error) {
func (r *VGReconciler) getAvailableDevicesForVG(ctx context.Context, blockDevices []internal.BlockDevice, vgs []lvm.VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]internal.BlockDevice, error) {
// filter devices based on DeviceSelector.Paths if specified
availableDevices, err := r.filterMatchingDevices(ctx, blockDevices, vgs, volumeGroup)
if err != nil {
@@ -98,14 +100,14 @@ DeviceLoop:
}

logger = logger.WithValues("Device.Name", blockDevice.Name)
for name, filter := range FilterMap {
logger := logger.WithValues("filter.Name", name)
valid, err := filter(blockDevice, r.executor)
for name, filterFunc := range filter.FilterMap {
logger := logger.WithValues("filterFunc.Name", name)
valid, err := filterFunc(blockDevice, r.executor)
if err != nil {
logger.Error(err, "filter error")
logger.Error(err, "filterFunc error")
continue DeviceLoop
} else if !valid {
logger.Info("does not match filter")
logger.Info("does not match filterFunc")
continue DeviceLoop
}
}
@@ -115,7 +117,7 @@ DeviceLoop:
}

// filterMatchingDevices filters devices based on DeviceSelector.Paths if specified.
func (r *VGReconciler) filterMatchingDevices(ctx context.Context, blockDevices []internal.BlockDevice, vgs []VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]internal.BlockDevice, error) {
func (r *VGReconciler) filterMatchingDevices(ctx context.Context, blockDevices []internal.BlockDevice, vgs []lvm.VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]internal.BlockDevice, error) {
logger := log.FromContext(ctx)

var filteredBlockDevices []internal.BlockDevice
@@ -186,7 +188,7 @@ func (r *VGReconciler) filterMatchingDevices(ctx context.Context, blockDevices [
return blockDevices, nil
}

func isDeviceAlreadyPartOfVG(vgs []VolumeGroup, diskName string, volumeGroup *lvmv1alpha1.LVMVolumeGroup) bool {
func isDeviceAlreadyPartOfVG(vgs []lvm.VolumeGroup, diskName string, volumeGroup *lvmv1alpha1.LVMVolumeGroup) bool {
for _, vg := range vgs {
if vg.Name == volumeGroup.Name {
for _, pv := range vg.PVs {
@@ -255,7 +257,7 @@ func checkDuplicateDeviceSelectorPaths(selector *lvmv1alpha1.DeviceSelector) err
//
// An error will be returned if the device is invalid
// No error and an empty BlockDevice object will be returned if this device should be skipped (ex: duplicate device)
func getValidDevice(devicePath string, blockDevices []internal.BlockDevice, vgs []VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) (internal.BlockDevice, error) {
func getValidDevice(devicePath string, blockDevices []internal.BlockDevice, vgs []lvm.VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) (internal.BlockDevice, error) {
// Make sure the symlink exists
diskName, err := filepath.EvalSymlinks(devicePath)
if err != nil {
14 changes: 8 additions & 6 deletions pkg/vgmanager/devices_test.go
Original file line number Diff line number Diff line change
@@ -9,7 +9,9 @@ import (

"github.com/go-logr/logr/testr"
"github.com/openshift/lvm-operator/api/v1alpha1"
"github.com/openshift/lvm-operator/pkg/filter"
"github.com/openshift/lvm-operator/pkg/internal"
"github.com/openshift/lvm-operator/pkg/lvm"
"github.com/stretchr/testify/assert"
"sigs.k8s.io/controller-runtime/pkg/log"

@@ -34,13 +36,13 @@ func TestAvailableDevicesForVG(t *testing.T) {
r := &VGReconciler{}

// remove noBindMounts filter as it reads `proc/1/mountinfo` file.
delete(FilterMap, "noBindMounts")
delete(filter.FilterMap, "noBindMounts")

testCases := []struct {
description string
volumeGroup v1alpha1.LVMVolumeGroup
existingBlockDevices []internal.BlockDevice
existingVGs []VolumeGroup
existingVGs []lvm.VolumeGroup
numOfAvailableDevices int
expectError bool
}{
@@ -310,10 +312,10 @@ func TestAvailableDevicesForVG(t *testing.T) {
},
},
},
existingVGs: []VolumeGroup{
existingVGs: []lvm.VolumeGroup{
{
Name: "vg1",
PVs: []PhysicalVolume{
PVs: []lvm.PhysicalVolume{
{PvName: calculateDevicePath(t, "nvme1n1p1")},
{PvName: calculateDevicePath(t, "nvme1n1p2")},
},
@@ -354,7 +356,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
},
},
},
existingVGs: []VolumeGroup{
existingVGs: []lvm.VolumeGroup{
{
Name: "vg1",
},
@@ -386,7 +388,7 @@ func TestAvailableDevicesForVG(t *testing.T) {
},
},
},
existingVGs: []VolumeGroup{
existingVGs: []lvm.VolumeGroup{
{
Name: "vg1",
},
3 changes: 2 additions & 1 deletion pkg/vgmanager/status.go
Original file line number Diff line number Diff line change
@@ -21,6 +21,7 @@ import (
"fmt"

lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1"
"github.com/openshift/lvm-operator/pkg/lvm"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/log"

@@ -165,7 +166,7 @@ func (r *VGReconciler) removeVolumeGroupStatus(ctx context.Context, vg *lvmv1alp
}

func (r *VGReconciler) setDevices(status *lvmv1alpha1.VGStatus) (bool, error) {
vgs, err := ListVolumeGroups(r.executor)
vgs, err := lvm.ListVolumeGroups(r.executor)
if err != nil {
return false, fmt.Errorf("failed to list volume groups. %v", err)
}
Loading

0 comments on commit 8208b3a

Please sign in to comment.