From b5572c9c387d08d988f24741bd7ee1c1a68fe3c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20M=C3=B6ller?= Date: Mon, 18 Sep 2023 16:13:23 +0200 Subject: [PATCH] feat: report filter status into vgstatus --- api/v1alpha1/lvmcluster_types.go | 9 +- api/v1alpha1/lvmvolumegroup_webhook.go | 81 +++++++ .../lvmvolumegroupnodestatus_types.go | 20 +- api/v1alpha1/webhook_suite_test.go | 4 +- api/v1alpha1/zz_generated.deepcopy.go | 62 +++++- ...topolvm.io_lvmvolumegroupnodestatuses.yaml | 38 ++++ .../lvms-operator.clusterserviceversion.yaml | 20 ++ .../crd/bases/lvm.topolvm.io_lvmclusters.yaml | 47 +++- ...topolvm.io_lvmvolumegroupnodestatuses.yaml | 38 ++++ config/webhook/manifests.yaml | 20 ++ controllers/lvmcluster_controller.go | 6 +- main.go | 4 + pkg/filter/filter.go | 99 ++++++--- pkg/filter/filter_test.go | 136 ++++++------ pkg/vgmanager/devices.go | 205 ++++++++---------- pkg/vgmanager/devices_test.go | 25 ++- pkg/vgmanager/status.go | 56 +++-- pkg/vgmanager/vgmanager_controller.go | 81 ++++--- pkg/vgmanager/vgmanager_controller_test.go | 21 +- 19 files changed, 677 insertions(+), 295 deletions(-) create mode 100644 api/v1alpha1/lvmvolumegroup_webhook.go diff --git a/api/v1alpha1/lvmcluster_types.go b/api/v1alpha1/lvmcluster_types.go index 8d02b3b95..a8ea461f5 100644 --- a/api/v1alpha1/lvmcluster_types.go +++ b/api/v1alpha1/lvmcluster_types.go @@ -220,13 +220,8 @@ type Storage struct { // NodeStatus defines the observed state of the deviceclass on the node type NodeStatus struct { // Node is the name of the node - Node string `json:"node,omitempty"` - // Status is the status of the VG on the node - Status VGStatusType `json:"status,omitempty"` - // Reason provides more detail on the VG creation status - Reason string `json:"reason,omitempty"` - // Devices is the list of devices used by the deviceclass - Devices []string `json:"devices,omitempty"` + Node string `json:"node,omitempty"` + VGStatus `json:",inline"` } //+kubebuilder:object:root=true diff --git a/api/v1alpha1/lvmvolumegroup_webhook.go b/api/v1alpha1/lvmvolumegroup_webhook.go new file mode 100644 index 000000000..328c4ac84 --- /dev/null +++ b/api/v1alpha1/lvmvolumegroup_webhook.go @@ -0,0 +1,81 @@ +package v1alpha1 + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// log is for logging in this package. +var lvmvolumegrouplog = logf.Log.WithName("lvmvolumegroup-webhook") + +var _ webhook.Validator = &LVMVolumeGroup{} + +//+kubebuilder:webhook:path=/validate-lvm-topolvm-io-v1alpha1-lvmvolumegroup,mutating=false,failurePolicy=fail,sideEffects=None,groups=lvm.topolvm.io,resources=lvmvolumegroups,verbs=create;update,versions=v1alpha1,name=vlvmvolumegroup.kb.io,admissionReviewVersions=v1 + +func (in *LVMVolumeGroup) SetupWebhookWithManager(mgr ctrl.Manager) error { + return ctrl.NewWebhookManagedBy(mgr). + For(in). + Complete() +} + +func (in *LVMVolumeGroup) ValidateCreate() (warnings admission.Warnings, err error) { + lvmvolumegrouplog.Info("validate create", "name", in.Name) + return in.ValidateUpdate(nil) +} + +func (in *LVMVolumeGroup) ValidateUpdate(_ runtime.Object) (warnings admission.Warnings, err error) { + lvmvolumegrouplog.Info("validate update", "name", in.Name) + if err := in.validateDeviceSelector(); err != nil { + return nil, fmt.Errorf(".Spec.DeviceSelector is invalid: %w", err) + } + return nil, nil +} + +func (in *LVMVolumeGroup) validateDeviceSelector() error { + selector := in.Spec.DeviceSelector + + uniquePaths := make(map[string]bool) + duplicatePaths := make(map[string]bool) + + // Check for duplicate required paths + for _, path := range selector.Paths { + if _, exists := uniquePaths[path]; exists { + duplicatePaths[path] = true + continue + } + + uniquePaths[path] = true + } + + // Check for duplicate optional paths + for _, path := range selector.OptionalPaths { + if _, exists := uniquePaths[path]; exists { + duplicatePaths[path] = true + continue + } + + uniquePaths[path] = true + } + + // Report any duplicate paths + if len(duplicatePaths) > 0 { + keys := make([]string, 0, len(duplicatePaths)) + for k := range duplicatePaths { + keys = append(keys, k) + } + + return fmt.Errorf("duplicate device paths found: %v", keys) + } + + return nil +} + +func (in *LVMVolumeGroup) ValidateDelete() (warnings admission.Warnings, err error) { + lvmvolumegrouplog.Info("validate delete", "name", in.Name) + return []string{}, nil +} diff --git a/api/v1alpha1/lvmvolumegroupnodestatus_types.go b/api/v1alpha1/lvmvolumegroupnodestatus_types.go index 0623fa7c5..accbda62a 100644 --- a/api/v1alpha1/lvmvolumegroupnodestatus_types.go +++ b/api/v1alpha1/lvmvolumegroupnodestatus_types.go @@ -48,8 +48,26 @@ type VGStatus struct { Status VGStatusType `json:"status,omitempty"` // Reason provides more detail on the VG creation status Reason string `json:"reason,omitempty"` - //Devices is the list of devices used by the VG + // Devices is the list of devices used by the VG Devices []string `json:"devices,omitempty"` + // FilterStatus contains the per node status of applied filters and their respective filtered devices. + // It can be consulted in case the device is not picked up as a device for LVM to use. + FilterStatus *FilterStatus `json:"filterStatus,omitempty"` +} + +type FilteredDevice struct { + // Name is a human-readable Identifier for a generic function that is ran on every block devices + // before it is considered by LVMS to be valid and usable. + Name string `json:"name"` + // FilteredReason is the human-readable reason why the device was filtered and could not be used + FilteredReason string `json:"filteredReason"` +} + +type FilterStatus struct { + // Registered lists all filters that are run on the node + Registered []string `json:"registered"` + // Devices lists a map that relates all currently filtered devices to the filter that excluded them. + Devices map[string][]FilteredDevice `json:"devices,omitempty"` } // LVMVolumeGroupNodeStatusStatus defines the observed state of LVMVolumeGroupNodeStatus diff --git a/api/v1alpha1/webhook_suite_test.go b/api/v1alpha1/webhook_suite_test.go index d677650da..db25accbf 100644 --- a/api/v1alpha1/webhook_suite_test.go +++ b/api/v1alpha1/webhook_suite_test.go @@ -104,8 +104,8 @@ var _ = BeforeSuite(func() { }) Expect(err).NotTo(HaveOccurred()) - err = (&LVMCluster{}).SetupWebhookWithManager(mgr) - Expect(err).NotTo(HaveOccurred()) + Expect((&LVMCluster{}).SetupWebhookWithManager(mgr)).NotTo(HaveOccurred()) + Expect((&LVMVolumeGroup{}).SetupWebhookWithManager(mgr)).NotTo(HaveOccurred()) //+kubebuilder:scaffold:webhook diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index ec661ba50..894efa5c2 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -119,6 +119,57 @@ func (in *DeviceSelector) DeepCopy() *DeviceSelector { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *FilterStatus) DeepCopyInto(out *FilterStatus) { + *out = *in + if in.Registered != nil { + in, out := &in.Registered, &out.Registered + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.Devices != nil { + in, out := &in.Devices, &out.Devices + *out = make(map[string][]FilteredDevice, len(*in)) + for key, val := range *in { + var outVal []FilteredDevice + if val == nil { + (*out)[key] = nil + } else { + inVal := (*in)[key] + in, out := &inVal, &outVal + *out = make([]FilteredDevice, len(*in)) + copy(*out, *in) + } + (*out)[key] = outVal + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FilterStatus. +func (in *FilterStatus) DeepCopy() *FilterStatus { + if in == nil { + return nil + } + out := new(FilterStatus) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *FilteredDevice) DeepCopyInto(out *FilteredDevice) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FilteredDevice. +func (in *FilteredDevice) DeepCopy() *FilteredDevice { + if in == nil { + return nil + } + out := new(FilteredDevice) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *LVMCluster) DeepCopyInto(out *LVMCluster) { *out = *in @@ -433,11 +484,7 @@ func (in *LVMVolumeGroupStatus) DeepCopy() *LVMVolumeGroupStatus { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *NodeStatus) DeepCopyInto(out *NodeStatus) { *out = *in - if in.Devices != nil { - in, out := &in.Devices, &out.Devices - *out = make([]string, len(*in)) - copy(*out, *in) - } + in.VGStatus.DeepCopyInto(&out.VGStatus) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NodeStatus. @@ -495,6 +542,11 @@ func (in *VGStatus) DeepCopyInto(out *VGStatus) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.FilterStatus != nil { + in, out := &in.FilterStatus, &out.FilterStatus + *out = new(FilterStatus) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new VGStatus. diff --git a/bundle/manifests/lvm.topolvm.io_lvmvolumegroupnodestatuses.yaml b/bundle/manifests/lvm.topolvm.io_lvmvolumegroupnodestatuses.yaml index ea855e76d..b505a1f10 100644 --- a/bundle/manifests/lvm.topolvm.io_lvmvolumegroupnodestatuses.yaml +++ b/bundle/manifests/lvm.topolvm.io_lvmvolumegroupnodestatuses.yaml @@ -45,6 +45,44 @@ spec: items: type: string type: array + filterStatus: + description: FilterStatus contains the per node status of applied + filters and their respective filtered devices. It can be consulted + in case the device is not picked up as a device for LVM to + use. + properties: + devices: + additionalProperties: + items: + properties: + filteredReason: + description: FilteredReason is the human-readable + reason why the device was filtered and could not + be used + type: string + name: + description: Name is a human-readable Identifier + for a generic function that is ran on every block + devices before it is considered by LVMS to be + valid and usable. + type: string + required: + - filteredReason + - name + type: object + type: array + description: Devices lists a map that relates all currently + filtered devices to the filter that excluded them. + type: object + registered: + description: Registered lists all filters that are run on + the node + items: + type: string + type: array + required: + - registered + type: object name: description: Name is the name of the VG type: string diff --git a/bundle/manifests/lvms-operator.clusterserviceversion.yaml b/bundle/manifests/lvms-operator.clusterserviceversion.yaml index ceda1a10a..b5ee8a920 100644 --- a/bundle/manifests/lvms-operator.clusterserviceversion.yaml +++ b/bundle/manifests/lvms-operator.clusterserviceversion.yaml @@ -888,3 +888,23 @@ spec: targetPort: 9443 type: ValidatingAdmissionWebhook webhookPath: /validate-lvm-topolvm-io-v1alpha1-lvmcluster + - admissionReviewVersions: + - v1 + containerPort: 443 + deploymentName: lvms-operator + failurePolicy: Fail + generateName: vlvmvolumegroup.kb.io + rules: + - apiGroups: + - lvm.topolvm.io + apiVersions: + - v1alpha1 + operations: + - CREATE + - UPDATE + resources: + - lvmvolumegroups + sideEffects: None + targetPort: 9443 + type: ValidatingAdmissionWebhook + webhookPath: /validate-lvm-topolvm-io-v1alpha1-lvmvolumegroup diff --git a/config/crd/bases/lvm.topolvm.io_lvmclusters.yaml b/config/crd/bases/lvm.topolvm.io_lvmclusters.yaml index 9dd8a935f..3fa72c1f5 100644 --- a/config/crd/bases/lvm.topolvm.io_lvmclusters.yaml +++ b/config/crd/bases/lvm.topolvm.io_lvmclusters.yaml @@ -297,10 +297,52 @@ spec: properties: devices: description: Devices is the list of devices used by the - deviceclass + VG items: type: string type: array + filterStatus: + description: FilterStatus contains the per node status + of applied filters and their respective filtered devices. + It can be consulted in case the device is not picked + up as a device for LVM to use. + properties: + devices: + additionalProperties: + items: + properties: + filteredReason: + description: FilteredReason is the human-readable + reason why the device was filtered and could + not be used + type: string + name: + description: Name is a human-readable Identifier + for a generic function that is ran on every + block devices before it is considered by + LVMS to be valid and usable. + type: string + required: + - filteredReason + - name + type: object + type: array + description: Devices lists a map that relates all + currently filtered devices to the filter that excluded + them. + type: object + registered: + description: Registered lists all filters that are + run on the node + items: + type: string + type: array + required: + - registered + type: object + name: + description: Name is the name of the VG + type: string node: description: Node is the name of the node type: string @@ -309,7 +351,8 @@ spec: status type: string status: - description: Status is the status of the VG on the node + description: Status tells if the VG was created on the + node type: string type: object type: array diff --git a/config/crd/bases/lvm.topolvm.io_lvmvolumegroupnodestatuses.yaml b/config/crd/bases/lvm.topolvm.io_lvmvolumegroupnodestatuses.yaml index 4567715f2..46d345422 100644 --- a/config/crd/bases/lvm.topolvm.io_lvmvolumegroupnodestatuses.yaml +++ b/config/crd/bases/lvm.topolvm.io_lvmvolumegroupnodestatuses.yaml @@ -45,6 +45,44 @@ spec: items: type: string type: array + filterStatus: + description: FilterStatus contains the per node status of applied + filters and their respective filtered devices. It can be consulted + in case the device is not picked up as a device for LVM to + use. + properties: + devices: + additionalProperties: + items: + properties: + filteredReason: + description: FilteredReason is the human-readable + reason why the device was filtered and could not + be used + type: string + name: + description: Name is a human-readable Identifier + for a generic function that is ran on every block + devices before it is considered by LVMS to be + valid and usable. + type: string + required: + - filteredReason + - name + type: object + type: array + description: Devices lists a map that relates all currently + filtered devices to the filter that excluded them. + type: object + registered: + description: Registered lists all filters that are run on + the node + items: + type: string + type: array + required: + - registered + type: object name: description: Name is the name of the VG type: string diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index c92958ea7..6362bfc2d 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -24,3 +24,23 @@ webhooks: resources: - lvmclusters sideEffects: None +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /validate-lvm-topolvm-io-v1alpha1-lvmvolumegroup + failurePolicy: Fail + name: vlvmvolumegroup.kb.io + rules: + - apiGroups: + - lvm.topolvm.io + apiVersions: + - v1alpha1 + operations: + - CREATE + - UPDATE + resources: + - lvmvolumegroups + sideEffects: None diff --git a/controllers/lvmcluster_controller.go b/controllers/lvmcluster_controller.go index c2c8fcee8..20bc655f5 100644 --- a/controllers/lvmcluster_controller.go +++ b/controllers/lvmcluster_controller.go @@ -255,10 +255,8 @@ func (r *LVMClusterReconciler) updateLVMClusterStatus(ctx context.Context, insta vgNodeMap[item.Name] = append(vgNodeMap[item.Name], lvmv1alpha1.NodeStatus{ - Node: nodeItem.Name, - Reason: item.Reason, - Status: item.Status, - Devices: item.Devices, + Node: nodeItem.Name, + VGStatus: *item.DeepCopy(), }, ) } diff --git a/main.go b/main.go index 7643e493f..23eafedd5 100644 --- a/main.go +++ b/main.go @@ -190,6 +190,10 @@ func main() { setupLog.Error(err, "unable to create webhook", "webhook", "LVMCluster") os.Exit(1) } + if err = (&lvmv1alpha1.LVMVolumeGroup{}).SetupWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create webhook", "webhook", "LVMVolumeGroup ") + os.Exit(1) + } pvController := persistent_volume.NewPersistentVolumeReconciler(mgr.GetClient(), mgr.GetAPIReader(), mgr.GetEventRecorderFor("lvms-pv-controller")) if err := pvController.SetupWithManager(mgr); err != nil { diff --git a/pkg/filter/filter.go b/pkg/filter/filter.go index ccdd35316..2a9d94cff 100644 --- a/pkg/filter/filter.go +++ b/pkg/filter/filter.go @@ -50,34 +50,53 @@ const ( usableDeviceType = "usableDeviceType" ) -type Filters map[string]func(lsblk.BlockDevice) (bool, error) +type Filter func(lsblk.BlockDevice) error + +type Filters map[string]Filter func DefaultFilters(lvm lvm.LVM, lsblkInstance lsblk.LSBLK) Filters { return Filters{ - notReadOnly: func(dev lsblk.BlockDevice) (bool, error) { - return !dev.ReadOnly, nil + notReadOnly: func(dev lsblk.BlockDevice) error { + if dev.ReadOnly { + return fmt.Errorf("device %s cannot be read-only", dev.KName) + } + return nil }, - notSuspended: func(dev lsblk.BlockDevice) (bool, error) { - matched := dev.State != StateSuspended - return matched, nil + notSuspended: func(dev lsblk.BlockDevice) error { + if dev.State == StateSuspended { + return fmt.Errorf("device %s cannot be in suspended", dev.KName) + } + return nil }, - noBiosBootInPartLabel: func(dev lsblk.BlockDevice) (bool, error) { - biosBootInPartLabel := strings.Contains(strings.ToLower(dev.PartLabel), strings.ToLower("bios")) || - strings.Contains(strings.ToLower(dev.PartLabel), strings.ToLower("boot")) - return !biosBootInPartLabel, nil + noBiosBootInPartLabel: func(dev lsblk.BlockDevice) error { + biosPartLabel := "bios" + bootPartLabel := "boot" + biosPartLabelExists := strings.Contains(strings.ToLower(dev.PartLabel), biosPartLabel) + bootPartLabelExists := strings.Contains(strings.ToLower(dev.PartLabel), bootPartLabel) + + if biosPartLabelExists || bootPartLabelExists { + return fmt.Errorf("device %s has the %q part-label set and cannot be used", dev.KName, dev.PartLabel) + } + + return nil }, - noReservedInPartLabel: func(dev lsblk.BlockDevice) (bool, error) { - reservedInPartLabel := strings.Contains(strings.ToLower(dev.PartLabel), "reserved") - return !reservedInPartLabel, nil + noReservedInPartLabel: func(dev lsblk.BlockDevice) error { + reservedPartLabel := "reserved" + reservedPartLabelExists := strings.Contains(strings.ToLower(dev.PartLabel), reservedPartLabel) + + if reservedPartLabelExists { + return fmt.Errorf("device %s has the %q part-label set and cannot be used", dev.KName, reservedPartLabel) + } + return nil }, - noValidFilesystemSignature: func(dev lsblk.BlockDevice) (bool, error) { + noValidFilesystemSignature: func(dev lsblk.BlockDevice) error { // if no fs type is set, it's always okay if dev.FSType == "" { - return true, nil + return nil } // if fstype is set to LVM2_member then it already was created as a PV @@ -85,50 +104,66 @@ func DefaultFilters(lvm lvm.LVM, lsblkInstance lsblk.LSBLK) Filters { if dev.FSType == "LVM2_member" && !dev.HasChildren() { pvs, err := lvm.ListPVs("") 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) + return 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 pv.VgName != "" { + return fmt.Errorf("device %s is already part of the volume group %s and cannot be used", dev.KName, pv.VgName) } + if pv.PvFree != "0G" { + return fmt.Errorf("device %s was reported as having no free capacity as a physical volume and cannot be used", dev.KName) + } + break } } // 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 nil } - return false, nil + return fmt.Errorf("device %s has invalid filesystem signature %s and cannot be used", dev.KName, dev.FSType) }, - noChildren: func(dev lsblk.BlockDevice) (bool, error) { + noChildren: func(dev lsblk.BlockDevice) error { hasChildren := dev.HasChildren() - return !hasChildren, nil + if hasChildren { + return fmt.Errorf("device %s has children devices and cannot be used", dev.KName) + } + return nil }, - noBindMounts: func(dev lsblk.BlockDevice) (bool, error) { + noBindMounts: func(dev lsblk.BlockDevice) error { hasBindMounts, _, err := lsblkInstance.HasBindMounts(dev) - return !hasBindMounts, err + if err != nil { + return fmt.Errorf("could not determine if device %s had bind mounts: %w", dev.KName, err) + } + if hasBindMounts { + return fmt.Errorf("device %s has bind mounts and cannot be used", dev.KName) + } + return nil }, - usableDeviceType: func(dev lsblk.BlockDevice) (bool, error) { + usableDeviceType: func(dev lsblk.BlockDevice) error { switch dev.Type { case DeviceTypeLoop: // check loop device isn't being used by kubernetes - return lsblkInstance.IsUsableLoopDev(dev) + usable, err := lsblkInstance.IsUsableLoopDev(dev) + if err != nil { + return fmt.Errorf("device %s has type %s but could not be determined as usable: %w", dev.KName, DeviceTypeLoop, err) + } + if !usable { + return fmt.Errorf("device %s has type %s but is not usable", dev.KName, DeviceTypeLoop) + } case DeviceTypeROM: - return false, nil + fallthrough case DeviceTypeLVM: - return false, nil - default: - return true, nil + return fmt.Errorf("device %s has type %s cannot be used", dev.KName, dev.Type) } + return nil }, } } diff --git a/pkg/filter/filter_test.go b/pkg/filter/filter_test.go index 14bafe5ad..2e05874b9 100644 --- a/pkg/filter/filter_test.go +++ b/pkg/filter/filter_test.go @@ -10,84 +10,86 @@ import ( type filterTestCase struct { label string device lsblk.BlockDevice - expected bool expectErr bool } func TestNotReadOnly(t *testing.T) { testcases := []filterTestCase{ - {label: "tc false", device: lsblk.BlockDevice{ReadOnly: false}, expected: true, expectErr: false}, - {label: "tc true", device: lsblk.BlockDevice{ReadOnly: true}, expected: false, expectErr: false}, + {label: "tc false", device: lsblk.BlockDevice{ReadOnly: false}, expectErr: false}, + {label: "tc true", device: lsblk.BlockDevice{ReadOnly: true}, expectErr: true}, } for _, tc := range testcases { - result, err := DefaultFilters(nil, nil)[notReadOnly](tc.device) - assert.Equal(t, tc.expected, result) - if tc.expectErr { - assert.Error(t, err) - } else { - assert.NoError(t, err) - } + t.Run(tc.label, func(t *testing.T) { + err := DefaultFilters(nil, nil)[notReadOnly](tc.device) + if tc.expectErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) } } func TestNotSuspended(t *testing.T) { testcases := []filterTestCase{ - {label: "tc suspended", device: lsblk.BlockDevice{State: "suspended"}, expected: false, expectErr: false}, - {label: "tc live", device: lsblk.BlockDevice{State: "live"}, expected: true, expectErr: false}, - {label: "tc running", device: lsblk.BlockDevice{State: "running"}, expected: true, expectErr: false}, + {label: "tc suspended", device: lsblk.BlockDevice{State: "suspended"}, expectErr: true}, + {label: "tc live", device: lsblk.BlockDevice{State: "live"}, expectErr: false}, + {label: "tc running", device: lsblk.BlockDevice{State: "running"}, expectErr: false}, } for _, tc := range testcases { - result, err := DefaultFilters(nil, nil)[notSuspended](tc.device) - assert.Equal(t, tc.expected, result) - if tc.expectErr { - assert.Error(t, err) - } else { - assert.NoError(t, err) - } + t.Run(tc.label, func(t *testing.T) { + err := DefaultFilters(nil, nil)[notSuspended](tc.device) + if tc.expectErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) } } func TestNoFilesystemSignature(t *testing.T) { testcases := []filterTestCase{ - {label: "tc no fs", device: lsblk.BlockDevice{FSType: ""}, expected: true, expectErr: false}, - {label: "tc xfs", device: lsblk.BlockDevice{FSType: "xfs"}, expected: false, expectErr: false}, - {label: "tc swap", device: lsblk.BlockDevice{FSType: "swap"}, expected: false, expectErr: false}, + {label: "tc no fs", device: lsblk.BlockDevice{FSType: ""}, expectErr: false}, + {label: "tc xfs", device: lsblk.BlockDevice{FSType: "xfs"}, expectErr: true}, + {label: "tc swap", device: lsblk.BlockDevice{FSType: "swap"}, expectErr: true}, } for _, tc := range testcases { - result, err := DefaultFilters(nil, nil)[noValidFilesystemSignature](tc.device) - assert.Equal(t, tc.expected, result) - if tc.expectErr { - assert.Error(t, err) - } else { - assert.NoError(t, err) - } + t.Run(tc.label, func(t *testing.T) { + err := DefaultFilters(nil, nil)[noValidFilesystemSignature](tc.device) + if tc.expectErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) } } func TestNoChildren(t *testing.T) { testcases := []filterTestCase{ - {label: "tc child", device: lsblk.BlockDevice{Name: "dev1", Children: []lsblk.BlockDevice{{Name: "child1"}}}, expected: false, expectErr: false}, - {label: "tc no child", device: lsblk.BlockDevice{Name: "dev2", Children: []lsblk.BlockDevice{}}, expected: true, expectErr: false}, + {label: "tc child", device: lsblk.BlockDevice{Name: "dev1", Children: []lsblk.BlockDevice{{Name: "child1"}}}, expectErr: true}, + {label: "tc no child", device: lsblk.BlockDevice{Name: "dev2", Children: []lsblk.BlockDevice{}}, expectErr: false}, } for _, tc := range testcases { - result, err := DefaultFilters(nil, nil)[noChildren](tc.device) - assert.Equal(t, tc.expected, result) - if tc.expectErr { - assert.Error(t, err) - } else { - assert.NoError(t, err) - } + t.Run(tc.label, func(t *testing.T) { + err := DefaultFilters(nil, nil)[noChildren](tc.device) + if tc.expectErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) } } func TestIsUsableDeviceType(t *testing.T) { testcases := []filterTestCase{ - {label: "tc ROM", device: lsblk.BlockDevice{Name: "dev1", Type: "rom"}, expected: false, expectErr: false}, - {label: "tc Disk", device: lsblk.BlockDevice{Name: "dev2", Type: "disk"}, expected: true, expectErr: false}, + {label: "tc ROM", device: lsblk.BlockDevice{Name: "dev1", Type: "rom"}, expectErr: true}, + {label: "tc Disk", device: lsblk.BlockDevice{Name: "dev2", Type: "disk"}, expectErr: false}, } for _, tc := range testcases { - result, err := DefaultFilters(nil, nil)[usableDeviceType](tc.device) - assert.Equal(t, tc.expected, result) + err := DefaultFilters(nil, nil)[usableDeviceType](tc.device) if tc.expectErr { assert.Error(t, err) } else { @@ -98,35 +100,41 @@ func TestIsUsableDeviceType(t *testing.T) { func TestNoBiosBootInPartLabel(t *testing.T) { testcases := []filterTestCase{ - {label: "tc 1", device: lsblk.BlockDevice{Name: "dev1", PartLabel: ""}, expected: true, expectErr: false}, - {label: "tc 2", device: lsblk.BlockDevice{Name: "dev2", PartLabel: "abc"}, expected: true, expectErr: false}, - {label: "tc 3", device: lsblk.BlockDevice{Name: "dev3", PartLabel: "bios"}, expected: false, expectErr: false}, - {label: "tc 4", device: lsblk.BlockDevice{Name: "dev4", PartLabel: "BIOS"}, expected: false, expectErr: false}, - {label: "tc 5", device: lsblk.BlockDevice{Name: "dev5", PartLabel: "boot"}, expected: false, expectErr: false}, - {label: "tc 6", device: lsblk.BlockDevice{Name: "dev6", PartLabel: "BOOT"}, expected: false, expectErr: false}, + {label: "tc 1", device: lsblk.BlockDevice{Name: "dev1", PartLabel: ""}, expectErr: false}, + {label: "tc 2", device: lsblk.BlockDevice{Name: "dev2", PartLabel: "abc"}, expectErr: false}, + {label: "tc 3", device: lsblk.BlockDevice{Name: "dev3", PartLabel: "bios"}, expectErr: true}, + {label: "tc 4", device: lsblk.BlockDevice{Name: "dev4", PartLabel: "BIOS"}, expectErr: true}, + {label: "tc 5", device: lsblk.BlockDevice{Name: "dev5", PartLabel: "boot"}, expectErr: true}, + {label: "tc 6", device: lsblk.BlockDevice{Name: "dev6", PartLabel: "BOOT"}, expectErr: true}, } for _, tc := range testcases { - result, err := DefaultFilters(nil, nil)[noBiosBootInPartLabel](tc.device) - assert.Equal(t, tc.expected, result) - if tc.expectErr { - assert.Error(t, err) - } else { - assert.NoError(t, err) - } + t.Run(tc.label, func(t *testing.T) { + err := DefaultFilters(nil, nil)[noBiosBootInPartLabel](tc.device) + if tc.expectErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) } } func TestNoReservedInPartLabel(t *testing.T) { testcases := []filterTestCase{ - {label: "tc 1", device: lsblk.BlockDevice{Name: "dev1", PartLabel: ""}, expected: true}, - {label: "tc 2", device: lsblk.BlockDevice{Name: "dev2", PartLabel: "abc"}, expected: true}, - {label: "tc 3", device: lsblk.BlockDevice{Name: "dev3", PartLabel: "reserved"}, expected: false}, - {label: "tc 4", device: lsblk.BlockDevice{Name: "dev4", PartLabel: "RESERVED"}, expected: false}, - {label: "tc 5", device: lsblk.BlockDevice{Name: "dev5", PartLabel: "Reserved"}, expected: false}, + {label: "tc 1", device: lsblk.BlockDevice{Name: "dev1", PartLabel: ""}, expectErr: false}, + {label: "tc 2", device: lsblk.BlockDevice{Name: "dev2", PartLabel: "abc"}, expectErr: false}, + {label: "tc 3", device: lsblk.BlockDevice{Name: "dev3", PartLabel: "reserved"}, expectErr: true}, + {label: "tc 4", device: lsblk.BlockDevice{Name: "dev4", PartLabel: "RESERVED"}, expectErr: true}, + {label: "tc 5", device: lsblk.BlockDevice{Name: "dev5", PartLabel: "Reserved"}, expectErr: true}, } for _, tc := range testcases { - result, err := DefaultFilters(nil, nil)[noReservedInPartLabel](tc.device) - assert.NoError(t, err) - assert.Equal(t, tc.expected, result) + t.Run(tc.label, func(t *testing.T) { + err := DefaultFilters(nil, nil)[noReservedInPartLabel](tc.device) + if tc.expectErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) } } diff --git a/pkg/vgmanager/devices.go b/pkg/vgmanager/devices.go index 9895a4432..19a9f2505 100644 --- a/pkg/vgmanager/devices.go +++ b/pkg/vgmanager/devices.go @@ -21,8 +21,10 @@ import ( "errors" "fmt" "path/filepath" + "sort" lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1" + "github.com/openshift/lvm-operator/pkg/filter" "github.com/openshift/lvm-operator/pkg/lsblk" "github.com/openshift/lvm-operator/pkg/lvm" "sigs.k8s.io/controller-runtime/pkg/log" @@ -71,119 +73,125 @@ func (r *VGReconciler) addDevicesToVG(ctx context.Context, vgs []lvm.VolumeGroup return nil } -// getAvailableDevicesForVG determines the available devices that can be used to create a volume group. -func (r *VGReconciler) getAvailableDevicesForVG(ctx context.Context, blockDevices []lsblk.BlockDevice, vgs []lvm.VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]lsblk.BlockDevice, error) { - // filter devices based on DeviceSelector.Paths if specified - availableDevices, err := r.filterMatchingDevices(ctx, blockDevices, vgs, volumeGroup) - if err != nil { - return nil, fmt.Errorf("failed to filter matching devices for volume group %s: %w", volumeGroup.GetName(), err) - } +type FilteredBlockDevice struct { + lsblk.BlockDevice + FilterError error +} - return r.filterAvailableDevices(ctx, availableDevices), nil +type FilteredBlockDevices struct { + RegisteredFilters []string + Available []lsblk.BlockDevice + Filtered map[string][]FilteredBlockDevice } -// filterAvailableDevices returns: +// filterDevices returns: // availableDevices: the list of blockdevices considered available -func (r *VGReconciler) filterAvailableDevices(ctx context.Context, blockDevices []lsblk.BlockDevice) []lsblk.BlockDevice { +func (r *VGReconciler) filterDevices(ctx context.Context, devices []lsblk.BlockDevice, filters filter.Filters) *FilteredBlockDevices { logger := log.FromContext(ctx) + registered := make([]string, 0) + for name := range filters { + registered = append(registered, name) + } + var availableDevices []lsblk.BlockDevice + filteredDevices := make(map[string][]FilteredBlockDevice) + // using a label so `continue DeviceLoop` can be used to skip devices DeviceLoop: - for _, blockDevice := range blockDevices { + for _, device := range devices { // check for partitions recursively - if blockDevice.HasChildren() { - childAvailableDevices := r.filterAvailableDevices(ctx, blockDevice.Children) - availableDevices = append(availableDevices, childAvailableDevices...) + if device.HasChildren() { + filteredChildDevices := r.filterDevices(ctx, device.Children, filters) + availableDevices = append(availableDevices, filteredChildDevices.Available...) + for filterName, filteredFromChild := range filteredChildDevices.Filtered { + filteredDevices[filterName] = append(filteredDevices[filterName], filteredFromChild...) + } } - - logger = logger.WithValues("Device.Name", blockDevice.Name) - for name, filter := range r.Filters(r.LVM, r.LSBLK) { + logger = logger.WithValues("device.KName", device.KName) + for name, filterFunc := range filters { logger := logger.WithValues("filter.Name", name) - valid, err := filter(blockDevice) - if err != nil { - logger.Error(err, "filter error") - continue DeviceLoop - } else if !valid { - logger.Info("does not match filter") + if err := filterFunc(device); err != nil { + logger.Error(err, "does not match filter") + filteredDevices[name] = append(filteredDevices[name], FilteredBlockDevice{ + BlockDevice: device, + FilterError: err, + }) continue DeviceLoop } } - availableDevices = append(availableDevices, blockDevice) + availableDevices = append(availableDevices, device) + } + sort.Strings(registered) + + return &FilteredBlockDevices{ + RegisteredFilters: registered, + Available: availableDevices, + Filtered: filteredDevices, } - return availableDevices } -// filterMatchingDevices filters devices based on DeviceSelector.Paths if specified. -func (r *VGReconciler) filterMatchingDevices(ctx context.Context, blockDevices []lsblk.BlockDevice, vgs []lvm.VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]lsblk.BlockDevice, error) { +// getNewDevicesToBeAdded gets all devices that should be added to the volume group +func (r *VGReconciler) getNewDevicesToBeAdded(ctx context.Context, blockDevices []lsblk.BlockDevice, vgs []lvm.VolumeGroup, volumeGroup *lvmv1alpha1.LVMVolumeGroup) ([]lsblk.BlockDevice, error) { logger := log.FromContext(ctx) - var filteredBlockDevices []lsblk.BlockDevice - devicesAlreadyInVG := false + var validBlockDevices []lsblk.BlockDevice + atLeastOneDeviceIsAlreadyInVolumeGroup := false - if volumeGroup.Spec.DeviceSelector != nil { + if volumeGroup.Spec.DeviceSelector == nil { + // return all available block devices if none is specified in the CR + return blockDevices, nil + } - if err := checkDuplicateDeviceSelectorPaths(volumeGroup.Spec.DeviceSelector); err != nil { - return nil, fmt.Errorf("unable to validate device selector paths: %v", err) + // If Paths is specified, treat it as required paths + for _, path := range volumeGroup.Spec.DeviceSelector.Paths { + blockDevice, err := getValidDevice(path, blockDevices, vgs, volumeGroup) + if err != nil { + // An error for required devices is critical + return nil, fmt.Errorf("unable to validate device %s: %v", path, err) } - // If Paths is specified, treat it as required paths - if len(volumeGroup.Spec.DeviceSelector.Paths) > 0 { - for _, path := range volumeGroup.Spec.DeviceSelector.Paths { - blockDevice, err := getValidDevice(path, blockDevices, vgs, volumeGroup) - if err != nil { - // An error for required devices is critical - return nil, fmt.Errorf("unable to validate device %s: %v", path, err) - } - - // Check if we should skip this device - if blockDevice.DevicePath == "" { - logger.Info(fmt.Sprintf("skipping required device that is already part of volume group %s: %s", volumeGroup.Name, path)) - devicesAlreadyInVG = true - continue - } - - filteredBlockDevices = append(filteredBlockDevices, blockDevice) - } + // Check if we should skip this device + if blockDevice.DevicePath == "" { + logger.Info(fmt.Sprintf("skipping required device that is already part of volume group %s: %s", volumeGroup.Name, path)) + atLeastOneDeviceIsAlreadyInVolumeGroup = true + continue } - // Check for any optional paths - if len(volumeGroup.Spec.DeviceSelector.OptionalPaths) > 0 { - for _, path := range volumeGroup.Spec.DeviceSelector.OptionalPaths { - blockDevice, err := getValidDevice(path, blockDevices, vgs, volumeGroup) - - // Check if we should skip this device - if err != nil { - logger.Info(fmt.Sprintf("skipping optional device path: %v", err)) - continue - } + validBlockDevices = append(validBlockDevices, blockDevice) + } - // Check if we should skip this device - if blockDevice.DevicePath == "" { - logger.Info(fmt.Sprintf("skipping optional device path that is already part of volume group %s: %s", volumeGroup.Name, path)) - devicesAlreadyInVG = true - continue - } + for _, path := range volumeGroup.Spec.DeviceSelector.OptionalPaths { + blockDevice, err := getValidDevice(path, blockDevices, vgs, volumeGroup) - filteredBlockDevices = append(filteredBlockDevices, blockDevice) - } + // Check if we should skip this device + if err != nil { + logger.Info(fmt.Sprintf("skipping optional device path: %v", err)) + continue + } - // At least 1 of the optional paths are required if: - // - OptionalPaths was specified AND - // - There were no required paths - // - Devices were not already part of the volume group (meaning this was run after vg creation) - // This guarantees at least 1 device could be found between optionalPaths and paths - // if len(filteredBlockDevices) == 0 && !devicesAlreadyInVG { - if len(filteredBlockDevices) == 0 && !devicesAlreadyInVG { - return nil, errors.New("at least 1 valid device is required if DeviceSelector paths or optionalPaths are specified") - } + // Check if we should skip this device + if blockDevice.DevicePath == "" { + logger.Info(fmt.Sprintf("skipping optional device path that is already part of volume group %s: %s", volumeGroup.Name, path)) + atLeastOneDeviceIsAlreadyInVolumeGroup = true + continue } - return filteredBlockDevices, nil + validBlockDevices = append(validBlockDevices, blockDevice) + } + + // Check for any optional paths + // At least 1 of the optional paths are required if: + // - OptionalPaths was specified AND + // - There were no required paths + // - Devices were not already part of the volume group (meaning this was run after vg creation) + // This guarantees at least 1 device could be found between optionalPaths and paths + // if len(FilteredBlockDevices) == 0 && !atLeastOneDeviceIsAlreadyInVolumeGroup { + if len(validBlockDevices) == 0 && !atLeastOneDeviceIsAlreadyInVolumeGroup { + return nil, errors.New("at least 1 valid device is required if DeviceSelector paths or optionalPaths are specified") } - // return all available block devices if none is specified in the CR - return blockDevices, nil + return validBlockDevices, nil } func isDeviceAlreadyPartOfVG(vgs []lvm.VolumeGroup, diskName string, volumeGroup *lvmv1alpha1.LVMVolumeGroup) bool { @@ -214,43 +222,6 @@ func hasExactDisk(blockDevices []lsblk.BlockDevice, deviceName string) (lsblk.Bl return lsblk.BlockDevice{}, false } -func checkDuplicateDeviceSelectorPaths(selector *lvmv1alpha1.DeviceSelector) error { - uniquePaths := make(map[string]bool) - duplicatePaths := make(map[string]bool) - - // Check for duplicate required paths - for _, path := range selector.Paths { - if _, exists := uniquePaths[path]; exists { - duplicatePaths[path] = true - continue - } - - uniquePaths[path] = true - } - - // Check for duplicate optional paths - for _, path := range selector.OptionalPaths { - if _, exists := uniquePaths[path]; exists { - duplicatePaths[path] = true - continue - } - - uniquePaths[path] = true - } - - // Report any duplicate paths - if len(duplicatePaths) > 0 { - keys := make([]string, 0, len(duplicatePaths)) - for k := range duplicatePaths { - keys = append(keys, k) - } - - return fmt.Errorf("duplicate device paths found: %v", keys) - } - - return nil -} - // getValidDevice will do various checks on a device path to make sure it is a valid device // // An error will be returned if the device is invalid diff --git a/pkg/vgmanager/devices_test.go b/pkg/vgmanager/devices_test.go index 2ef1acbf9..1b6fa8f99 100644 --- a/pkg/vgmanager/devices_test.go +++ b/pkg/vgmanager/devices_test.go @@ -19,8 +19,8 @@ import ( var devicePaths map[string]string -func TestAvailableDevicesForVG(t *testing.T) { - // create a folder for each disk to resolve filepath.EvalSymlinks(path) call in filterMatchingDevices. +func Test_getNewDevicesToBeAdded(t *testing.T) { + // create a folder for each disk to resolve filepath.EvalSymlinks(path) call in getNewDevicesToBeAdded. tmpDir := t.TempDir() devicePaths = make(map[string]string) devicePaths["nvme1n1p1"] = fmt.Sprintf("%s/%s", tmpDir, "nvme1n1p1") @@ -33,12 +33,10 @@ func TestAvailableDevicesForVG(t *testing.T) { } r := &VGReconciler{} - r.Filters = func(a lvm.LVM, b lsblk.LSBLK) filter.Filters { - filters := filter.DefaultFilters(a, b) - // remove noBindMounts filter as it reads `proc/1/mountinfo` file. - delete(filters, "noBindMounts") - return filters - } + + filters := filter.DefaultFilters(nil, nil) + // remove noBindMounts filter as it reads `proc/1/mountinfo` file. + delete(filters, "noBindMounts") testCases := []struct { description string @@ -571,13 +569,20 @@ func TestAvailableDevicesForVG(t *testing.T) { for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { - availableDevices, err := r.getAvailableDevicesForVG(log.IntoContext(context.Background(), testr.New(t)), tc.existingBlockDevices, tc.existingVGs, &tc.volumeGroup) + ctx := log.IntoContext(context.Background(), testr.New(t)) + availableDevices, err := r.getNewDevicesToBeAdded(ctx, tc.existingBlockDevices, tc.existingVGs, &tc.volumeGroup) + if !tc.expectError { + assert.NoError(t, err) + } else { + assert.Error(t, err) + } + devices := r.filterDevices(ctx, availableDevices, filters) if !tc.expectError { assert.NoError(t, err) } else { assert.Error(t, err) } - assert.Equal(t, tc.numOfAvailableDevices, len(availableDevices), "expected numOfAvailableDevices is not equal to actual number") + assert.Equal(t, tc.numOfAvailableDevices, len(devices.Available), "expected numOfAvailableDevices is not equal to actual number") }) } } diff --git a/pkg/vgmanager/status.go b/pkg/vgmanager/status.go index 14ee3203c..9ed60c5e6 100644 --- a/pkg/vgmanager/status.go +++ b/pkg/vgmanager/status.go @@ -29,21 +29,35 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) -func (r *VGReconciler) setVolumeGroupReadyStatus(ctx context.Context, vg *lvmv1alpha1.LVMVolumeGroup) error { +func (r *VGReconciler) setVolumeGroupProgressingStatus(ctx context.Context, vg *lvmv1alpha1.LVMVolumeGroup, devices *FilteredBlockDevices) (bool, error) { + status := &lvmv1alpha1.VGStatus{ + Name: vg.GetName(), + Status: lvmv1alpha1.VGStatusProgressing, + } + + // Set devices for the VGStatus. + if _, err := r.setDevices(status, devices); err != nil { + return false, err + } + + return r.setVolumeGroupStatus(ctx, vg, status) +} + +func (r *VGReconciler) setVolumeGroupReadyStatus(ctx context.Context, vg *lvmv1alpha1.LVMVolumeGroup, devices *FilteredBlockDevices) (bool, error) { status := &lvmv1alpha1.VGStatus{ Name: vg.GetName(), Status: lvmv1alpha1.VGStatusReady, } // Set devices for the VGStatus. - if _, err := r.setDevices(status); err != nil { - return err + if _, err := r.setDevices(status, devices); err != nil { + return false, err } return r.setVolumeGroupStatus(ctx, vg, status) } -func (r *VGReconciler) setVolumeGroupFailedStatus(ctx context.Context, vg *lvmv1alpha1.LVMVolumeGroup, err error) error { +func (r *VGReconciler) setVolumeGroupFailedStatus(ctx context.Context, vg *lvmv1alpha1.LVMVolumeGroup, devices *FilteredBlockDevices, err error) (bool, error) { status := &lvmv1alpha1.VGStatus{ Name: vg.GetName(), Status: lvmv1alpha1.VGStatusFailed, @@ -52,8 +66,8 @@ func (r *VGReconciler) setVolumeGroupFailedStatus(ctx context.Context, vg *lvmv1 // Set devices for the VGStatus. // If there is backing volume group, then set as degraded - if devicesExist, err := r.setDevices(status); err != nil { - return fmt.Errorf("could not set devices in VGStatus: %w", err) + if devicesExist, err := r.setDevices(status, devices); err != nil { + return false, fmt.Errorf("could not set devices in VGStatus: %w", err) } else if devicesExist { status.Status = lvmv1alpha1.VGStatusDegraded } @@ -61,7 +75,7 @@ func (r *VGReconciler) setVolumeGroupFailedStatus(ctx context.Context, vg *lvmv1 return r.setVolumeGroupStatus(ctx, vg, status) } -func (r *VGReconciler) setVolumeGroupStatus(ctx context.Context, vg *lvmv1alpha1.LVMVolumeGroup, status *lvmv1alpha1.VGStatus) error { +func (r *VGReconciler) setVolumeGroupStatus(ctx context.Context, vg *lvmv1alpha1.LVMVolumeGroup, status *lvmv1alpha1.VGStatus) (bool, error) { logger := log.FromContext(ctx).WithValues("VolumeGroup", client.ObjectKeyFromObject(vg)) // Get LVMVolumeGroupNodeStatus and set the relevant VGStatus @@ -87,17 +101,14 @@ func (r *VGReconciler) setVolumeGroupStatus(ctx context.Context, vg *lvmv1alpha1 return nil }) + updated := result != controllerutil.OperationResultNone if err != nil { - return fmt.Errorf("LVMVolumeGroupNodeStatus could not be updated: %w", err) + return updated, fmt.Errorf("LVMVolumeGroupNodeStatus could not be updated: %w", err) } - - if result != controllerutil.OperationResultNone { + if updated { logger.Info("LVMVolumeGroupNodeStatus modified", "operation", result, "name", nodeStatus.Name) - } else { - logger.Info("LVMVolumeGroupNodeStatus unchanged") } - - return nil + return updated, nil } func (r *VGReconciler) removeVolumeGroupStatus(ctx context.Context, vg *lvmv1alpha1.LVMVolumeGroup) error { @@ -150,7 +161,7 @@ func (r *VGReconciler) removeVolumeGroupStatus(ctx context.Context, vg *lvmv1alp return nil } -func (r *VGReconciler) setDevices(status *lvmv1alpha1.VGStatus) (bool, error) { +func (r *VGReconciler) setDevices(status *lvmv1alpha1.VGStatus, devices *FilteredBlockDevices) (bool, error) { vgs, err := r.LVM.ListVGs() if err != nil { return false, fmt.Errorf("failed to list volume groups. %v", err) @@ -169,6 +180,21 @@ func (r *VGReconciler) setDevices(status *lvmv1alpha1.VGStatus) (bool, error) { } } + status.FilterStatus = &lvmv1alpha1.FilterStatus{} + if devices != nil { + for filterName, filteredDevices := range devices.Filtered { + for _, device := range filteredDevices { + status.FilterStatus.Devices[filterName] = append( + status.FilterStatus.Devices[filterName], lvmv1alpha1.FilteredDevice{ + Name: device.KName, + FilteredReason: device.FilterError.Error(), + }) + break + } + } + status.FilterStatus.Registered = devices.RegisteredFilters + } + return devicesExist, nil } diff --git a/pkg/vgmanager/vgmanager_controller.go b/pkg/vgmanager/vgmanager_controller.go index 35fecb627..413b7b279 100644 --- a/pkg/vgmanager/vgmanager_controller.go +++ b/pkg/vgmanager/vgmanager_controller.go @@ -23,7 +23,6 @@ import ( "time" "github.com/google/go-cmp/cmp" - lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1" "github.com/openshift/lvm-operator/controllers" "github.com/openshift/lvm-operator/pkg/filter" @@ -98,7 +97,7 @@ func (r *VGReconciler) getFinalizer() string { } func (r *VGReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - logger := log.FromContext(ctx).WithValues("LVMVolumeGroup", req) + logger := log.FromContext(ctx) logger.Info("reconciling") // Check if this LVMVolumeGroup needs to be processed on this node @@ -132,8 +131,11 @@ func (r *VGReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Re return r.reconcile(ctx, volumeGroup) } -func (r *VGReconciler) reconcile(ctx context.Context, volumeGroup *lvmv1alpha1.LVMVolumeGroup) (ctrl.Result, error) { - logger := log.FromContext(ctx).WithValues("VGName", volumeGroup.GetName()) +func (r *VGReconciler) reconcile( + ctx context.Context, + volumeGroup *lvmv1alpha1.LVMVolumeGroup, +) (ctrl.Result, error) { + logger := log.FromContext(ctx) // Check if the LVMVolumeGroup resource is deleted if !volumeGroup.DeletionTimestamp.IsZero() { return ctrl.Result{}, r.processDelete(ctx, volumeGroup) @@ -148,7 +150,7 @@ func (r *VGReconciler) reconcile(ctx context.Context, volumeGroup *lvmv1alpha1.L lvmdConfig, err := r.LVMD.Load() if err != nil { err = fmt.Errorf("failed to read the lvmd config file: %w", err) - if err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, err); err != nil { + if _, err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, nil, err); err != nil { logger.Error(err, "failed to set status to failed") } return ctrl.Result{}, err @@ -169,36 +171,40 @@ func (r *VGReconciler) reconcile(ctx context.Context, volumeGroup *lvmv1alpha1.L return ctrl.Result{}, fmt.Errorf("failed to list volume groups: %w", err) } + vgExistsInLVM := false + for _, vg := range vgs { + if volumeGroup.Name == vg.Name { + if len(vg.PVs) > 0 { + vgExistsInLVM = true + } + } + } + blockDevices, err := r.LSBLK.ListBlockDevices() if err != nil { return ctrl.Result{}, fmt.Errorf("failed to list block devices: %w", err) } // Get the available block devices that can be used for this volume group - availableDevices, err := r.getAvailableDevicesForVG(ctx, blockDevices, vgs, volumeGroup) + // valid means that it can be used to create or extend the volume group + // devices that are already part of the volume group will not be returned + newDevices, err := r.getNewDevicesToBeAdded(ctx, blockDevices, vgs, volumeGroup) if err != nil { - return ctrl.Result{}, fmt.Errorf("failed to get block devices for volumegroup %s: %w", volumeGroup.GetName(), err) + return ctrl.Result{}, fmt.Errorf("failed to get matching available block devices for volumegroup %s: %w", volumeGroup.GetName(), err) } - logger.Info("listing available and delayed devices", "availableDevices", availableDevices) + devices := r.filterDevices(ctx, newDevices, r.Filters(r.LVM, r.LSBLK)) + + logger.Info("listing available and filtered devices", "available", devices.Available, "filtered", devices.Filtered) // If there are no available devices, that could mean either // - There is no available devices to attach to the volume group // - All the available devices are already attached - if len(availableDevices) == 0 { - volumeGroupExists := false - for _, vg := range vgs { - if volumeGroup.Name == vg.Name { - if len(vg.PVs) > 0 { - volumeGroupExists = true - } - } - } - - if !volumeGroupExists { + if len(devices.Available) == 0 { + if !vgExistsInLVM { err := fmt.Errorf("the volume group %s does not exist and there were no available devices to create it", volumeGroup.GetName()) r.WarningEvent(ctx, volumeGroup, EventReasonErrorNoAvailableDevicesForVG, err) - if err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, err); err != nil { + if _, err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, devices, err); err != nil { logger.Error(err, "failed to set status to failed") } return ctrl.Result{}, err @@ -208,7 +214,7 @@ func (r *VGReconciler) reconcile(ctx context.Context, volumeGroup *lvmv1alpha1.L if err := r.validateLVs(ctx, volumeGroup); err != nil { err := fmt.Errorf("error while validating logical volumes in existing volume group: %w", err) r.WarningEvent(ctx, volumeGroup, EventReasonErrorInconsistentLVs, err) - if err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, err); err != nil { + if _, err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, devices, err); err != nil { logger.Error(err, "failed to set status to failed") } return ctrl.Result{}, err @@ -216,19 +222,27 @@ func (r *VGReconciler) reconcile(ctx context.Context, volumeGroup *lvmv1alpha1.L msg := "all the available devices are attached to the volume group" logger.Info(msg) - if err := r.setVolumeGroupReadyStatus(ctx, volumeGroup); err != nil { + if updated, err := r.setVolumeGroupReadyStatus(ctx, volumeGroup, devices); err != nil { return ctrl.Result{}, fmt.Errorf("failed to set status for volume group %s to ready: %w", volumeGroup.Name, err) + } else if updated { + r.NormalEvent(ctx, volumeGroup, EventReasonVolumeGroupReady, msg) } - r.NormalEvent(ctx, volumeGroup, EventReasonVolumeGroupReady, msg) return reconcileAgain, nil + } else { + if updated, err := r.setVolumeGroupProgressingStatus(ctx, volumeGroup, devices); err != nil { + logger.Error(err, "failed to set status to progressing") + } else if updated { + logger.Info("new available devices were discovered and status was updated to progressing") + return ctrl.Result{Requeue: true}, nil + } } // Create VG/extend VG - if err = r.addDevicesToVG(ctx, vgs, volumeGroup.Name, availableDevices); err != nil { + if err = r.addDevicesToVG(ctx, vgs, volumeGroup.Name, devices.Available); err != nil { err = fmt.Errorf("failed to create/extend volume group %s: %w", volumeGroup.Name, err) r.WarningEvent(ctx, volumeGroup, EventReasonErrorVGCreateOrExtendFailed, err) - if err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, err); err != nil { + if _, err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, devices, err); err != nil { logger.Error(err, "failed to set status to failed") } return ctrl.Result{}, err @@ -238,7 +252,7 @@ func (r *VGReconciler) reconcile(ctx context.Context, volumeGroup *lvmv1alpha1.L if err = r.addThinPoolToVG(ctx, volumeGroup.Name, volumeGroup.Spec.ThinPoolConfig); err != nil { err := fmt.Errorf("failed to create thin pool %s for volume group %s: %w", volumeGroup.Spec.ThinPoolConfig.Name, volumeGroup.Name, err) r.WarningEvent(ctx, volumeGroup, EventReasonErrorThinPoolCreateOrExtendFailed, err) - if err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, err); err != nil { + if _, err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, devices, err); err != nil { logger.Error(err, "failed to set status to failed") } return ctrl.Result{}, err @@ -248,7 +262,7 @@ func (r *VGReconciler) reconcile(ctx context.Context, volumeGroup *lvmv1alpha1.L if err := r.validateLVs(ctx, volumeGroup); err != nil { err := fmt.Errorf("error while validating logical volumes in existing volume group: %w", err) r.WarningEvent(ctx, volumeGroup, EventReasonErrorInconsistentLVs, err) - if err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, err); err != nil { + if _, err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, devices, err); err != nil { logger.Error(err, "failed to set status to failed") } return ctrl.Result{}, err @@ -282,7 +296,7 @@ func (r *VGReconciler) reconcile(ctx context.Context, volumeGroup *lvmv1alpha1.L if !cmp.Equal(existingLvmdConfig, lvmdConfig) { if err := r.LVMD.Save(lvmdConfig); err != nil { err := fmt.Errorf("failed to update lvmd config file to update volume group %s: %w", volumeGroup.GetName(), err) - if err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, err); err != nil { + if _, err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, devices, err); err != nil { logger.Error(err, "failed to set status to failed") } return ctrl.Result{}, err @@ -292,11 +306,12 @@ func (r *VGReconciler) reconcile(ctx context.Context, volumeGroup *lvmv1alpha1.L r.NormalEvent(ctx, volumeGroup, EventReasonLVMDConfigUpdated, msg) } - if err := r.setVolumeGroupReadyStatus(ctx, volumeGroup); err != nil { + if updated, err := r.setVolumeGroupReadyStatus(ctx, volumeGroup, devices); err != nil { return ctrl.Result{}, fmt.Errorf("failed to set status for volume group %s to ready: %w", volumeGroup.Name, err) + } else if updated { + msg := "all the available devices are attached to the volume group" + r.NormalEvent(ctx, volumeGroup, EventReasonVolumeGroupReady, msg) } - msg := "all the available devices are attached to the volume group" - r.NormalEvent(ctx, volumeGroup, EventReasonVolumeGroupReady, msg) return reconcileAgain, nil } @@ -348,7 +363,7 @@ func (r *VGReconciler) processDelete(ctx context.Context, volumeGroup *lvmv1alph if thinPoolExists { if err := r.LVM.DeleteLV(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 { + if _, err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, nil, err); err != nil { logger.Error(err, "failed to set status to failed") } return err @@ -361,7 +376,7 @@ func (r *VGReconciler) processDelete(ctx context.Context, volumeGroup *lvmv1alph if err = r.LVM.DeleteVG(vg); err != nil { err := fmt.Errorf("failed to delete volume group %s: %w", volumeGroup.Name, err) - if err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, err); err != nil { + if _, err := r.setVolumeGroupFailedStatus(ctx, volumeGroup, nil, err); err != nil { logger.Error(err, "failed to set status to failed", "VGName", volumeGroup.GetName()) } return err diff --git a/pkg/vgmanager/vgmanager_controller_test.go b/pkg/vgmanager/vgmanager_controller_test.go index 4b40af4b0..ed71666de 100644 --- a/pkg/vgmanager/vgmanager_controller_test.go +++ b/pkg/vgmanager/vgmanager_controller_test.go @@ -46,7 +46,10 @@ func testMockedBlockDeviceOnHost(ctx context.Context) { OverprovisionRatio: 10, } - mockLVM.EXPECT().ListVGs().Return(nil, nil).Once() + // 1st Call discover blockdevices and set filter status + // 2nd Call set progressing status + // 3rd Call actually add stuff + mockLVM.EXPECT().ListVGs().Return(nil, nil).Times(4) mockLSBLK.EXPECT().ListBlockDevices().Return([]lsblk.BlockDevice{ { Name: "mock0", @@ -61,9 +64,9 @@ func testMockedBlockDeviceOnHost(ctx context.Context) { Serial: "MOCK", DevicePath: device, }, - }, nil).Once() + }, nil).Times(2) // hasBindMounts in filters needs a mock - mockLSBLK.EXPECT().HasBindMounts(mock.AnythingOfType("BlockDevice")).Return(false, "", nil).Once() + mockLSBLK.EXPECT().HasBindMounts(mock.AnythingOfType("BlockDevice")).Return(false, "", nil).Times(2) // Create VG lvmPV := lvm.PhysicalVolume{PvName: device} @@ -129,6 +132,18 @@ func testMockedBlockDeviceOnHost(ctx context.Context) { Name: vg.GetName(), Status: lvmv1alpha1.VGStatusReady, Devices: vg.Spec.DeviceSelector.Paths, + FilterStatus: &lvmv1alpha1.FilterStatus{ + Registered: []string{ + "noBindMounts", + "noBiosBootInPartLabel", + "noChildren", + "noReservedInPartLabel", + "noValidFilesystemSignature", + "notReadOnly", + "notSuspended", + "usableDeviceType", + }, + }, }), "volume group needs to be ready and contain all the devices from the selector") }).WithContext(ctx).Should(Succeed()) }