From fd05cf842ff6e9dc6ff89f2822bc28ff1588e11f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20M=C3=B6ller?= Date: Fri, 2 Feb 2024 13:30:23 +0100 Subject: [PATCH 1/2] fix: more sophisticated tagging migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jakob Möller --- internal/controllers/vgmanager/controller.go | 2 +- .../controllers/vgmanager/controller_test.go | 26 +-- internal/controllers/vgmanager/lvm/lvm.go | 45 +++++- .../controllers/vgmanager/lvm/lvm_test.go | 14 +- .../vgmanager/lvm/mocks/mock_lvm.go | 29 ++-- internal/controllers/vgmanager/status.go | 2 +- internal/tagging/tagging.go | 102 ++++++++---- internal/tagging/tagging_test.go | 148 ++++++++++++++---- 8 files changed, 273 insertions(+), 95 deletions(-) diff --git a/internal/controllers/vgmanager/controller.go b/internal/controllers/vgmanager/controller.go index db1c78570..274b597ce 100644 --- a/internal/controllers/vgmanager/controller.go +++ b/internal/controllers/vgmanager/controller.go @@ -187,7 +187,7 @@ func (r *Reconciler) reconcile( devices := r.filterDevices(ctx, newDevices, r.Filters(volumeGroup, r.LVM, r.LSBLK)) - vgs, err := r.LVM.ListVGs() + vgs, err := r.LVM.ListVGs(true) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to list volume groups: %w", err) } diff --git a/internal/controllers/vgmanager/controller_test.go b/internal/controllers/vgmanager/controller_test.go index 76bcfe6a3..755d343dd 100644 --- a/internal/controllers/vgmanager/controller_test.go +++ b/internal/controllers/vgmanager/controller_test.go @@ -245,7 +245,7 @@ func testMockedBlockDeviceOnHost(ctx context.Context) { } By("triggering the second reconciliation after the initial setup", func() { - instances.LVM.EXPECT().ListVGs().Return(nil, nil).Twice() + instances.LVM.EXPECT().ListVGs(true).Return(nil, nil).Twice() instances.LSBLK.EXPECT().ListBlockDevices().Return([]lsblk.BlockDevice{blockDevice}, nil).Once() instances.LSBLK.EXPECT().HasBindMounts(blockDevice).Return(false, "", nil).Once() _, err := instances.Reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(vg)}) @@ -262,7 +262,7 @@ func testMockedBlockDeviceOnHost(ctx context.Context) { }) // Requeue effects - instances.LVM.EXPECT().ListVGs().Return(nil, nil).Twice() + instances.LVM.EXPECT().ListVGs(true).Return(nil, nil).Twice() instances.LSBLK.EXPECT().ListBlockDevices().Return([]lsblk.BlockDevice{blockDevice}, nil).Once() instances.LSBLK.EXPECT().HasBindMounts(blockDevice).Return(false, "", nil).Once() @@ -303,7 +303,7 @@ func testMockedBlockDeviceOnHost(ctx context.Context) { instances.LVM.EXPECT().ListLVs(vg.GetName()).Return(&lvm.LVReport{Report: []lvm.LVReportItem{{ Lv: []lvm.LogicalVolume{thinPool}, }}}, nil).Once() - instances.LVM.EXPECT().ListVGs().Return([]lvm.VolumeGroup{createdVG}, nil).Twice() + instances.LVM.EXPECT().ListVGs(true).Return([]lvm.VolumeGroup{createdVG}, nil).Twice() instances.LVM.EXPECT().ActivateLV(thinPool.Name, vg.GetName()).Return(nil).Once() }) @@ -370,7 +370,7 @@ func testMockedBlockDeviceOnHost(ctx context.Context) { }) By("mocking the now created vg and thin pool", func() { - instances.LVM.EXPECT().ListVGs().Return([]lvm.VolumeGroup{createdVG}, nil).Once() + instances.LVM.EXPECT().ListVGs(true).Return([]lvm.VolumeGroup{createdVG}, nil).Once() instances.LVM.EXPECT().ListLVs(vg.GetName()).Return(&lvm.LVReport{Report: []lvm.LVReportItem{{ Lv: []lvm.LogicalVolume{thinPool}, }}}, nil).Once() @@ -561,13 +561,13 @@ func testLVMD(ctx context.Context) { r.LVM = mockLVM mockLVMD.EXPECT().Load().Once().Return(nil, fmt.Errorf("mock load failure")) - mockLVM.EXPECT().ListVGs().Once().Return(nil, fmt.Errorf("mock list failure")) + mockLVM.EXPECT().ListVGs(true).Once().Return(nil, fmt.Errorf("mock list failure")) err := r.applyLVMDConfig(ctx, vg, devices) Expect(err).To(HaveOccurred(), "should error if lvmd config cannot be loaded and/or status cannot be set") mockLVMD.EXPECT().Load().Once().Return(&lvmd.Config{}, nil) mockLVMD.EXPECT().Save(mock.Anything).Once().Return(fmt.Errorf("mock save failure")) - mockLVM.EXPECT().ListVGs().Once().Return(nil, fmt.Errorf("mock list failure")) + mockLVM.EXPECT().ListVGs(true).Once().Return(nil, fmt.Errorf("mock list failure")) err = r.applyLVMDConfig(ctx, vg, devices) Expect(err).To(HaveOccurred(), "should error if lvmd config cannot be saved and/or status cannot be set") } @@ -768,7 +768,7 @@ func testReconcileFailure(ctx context.Context) { {Name: "/dev/xxx", KName: "/dev/xxx", FSType: "ext4"}, } instances.LSBLK.EXPECT().ListBlockDevices().Once().Return(blockDevices, nil) - instances.LVM.EXPECT().ListVGs().Once().Return(nil, nil) + instances.LVM.EXPECT().ListVGs(true).Once().Return(nil, nil) _, err := instances.Reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(vg)}) nodeStatus := &lvmv1alpha1.LVMVolumeGroupNodeStatus{} Expect(instances.client.Get(ctx, client.ObjectKey{ @@ -795,7 +795,7 @@ func testReconcileFailure(ctx context.Context) { instances.LSBLK.EXPECT().ListBlockDevices().Once().Return([]lsblk.BlockDevice{ {Name: "/dev/sda", KName: "/dev/sda", FSType: "xfs", PartLabel: "reserved"}, }, nil) - instances.LVM.EXPECT().ListVGs().Twice().Return(nil, nil) + instances.LVM.EXPECT().ListVGs(true).Twice().Return(nil, nil) instances.LSBLK.EXPECT().HasBindMounts(mock.Anything).Once().Return(false, "", nil) _, err := instances.Reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(vg)}) Expect(err).To(HaveOccurred()) @@ -813,8 +813,8 @@ func testReconcileFailure(ctx context.Context) { instances.LSBLK.EXPECT().ListBlockDevices().Once().Return([]lsblk.BlockDevice{ {Name: "/dev/sda", KName: "/dev/sda", FSType: "xfs", PartLabel: "reserved"}, }, nil) - instances.LVM.EXPECT().ListVGs().Once().Return(nil, nil) - instances.LVM.EXPECT().ListVGs().Once().Return(nil, fmt.Errorf("mocked error")) + instances.LVM.EXPECT().ListVGs(true).Once().Return(nil, nil) + instances.LVM.EXPECT().ListVGs(true).Once().Return(nil, fmt.Errorf("mocked error")) instances.LSBLK.EXPECT().HasBindMounts(mock.Anything).Once().Return(false, "", nil) _, err := instances.Reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(vg)}) Expect(err).To(HaveOccurred()) @@ -834,7 +834,7 @@ func testReconcileFailure(ctx context.Context) { vgs := []lvm.VolumeGroup{ {Name: "vg1", VgSize: "1g"}, } - instances.LVM.EXPECT().ListVGs().Twice().Return(vgs, nil) + instances.LVM.EXPECT().ListVGs(true).Twice().Return(vgs, nil) instances.LSBLK.EXPECT().HasBindMounts(mock.Anything).Once().Return(false, "", nil) instances.LVM.EXPECT().ListLVs("vg1").Once().Return(nil, fmt.Errorf("mocked error")) _, err := instances.Reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(vg)}) @@ -854,8 +854,8 @@ func testReconcileFailure(ctx context.Context) { vgs := []lvm.VolumeGroup{ {Name: "vg1", VgSize: "1g"}, } - instances.LVM.EXPECT().ListVGs().Once().Return(vgs, nil) - instances.LVM.EXPECT().ListVGs().Once().Return(nil, fmt.Errorf("mocked error")) + instances.LVM.EXPECT().ListVGs(true).Once().Return(vgs, nil) + instances.LVM.EXPECT().ListVGs(true).Once().Return(nil, fmt.Errorf("mocked error")) instances.LSBLK.EXPECT().HasBindMounts(mock.Anything).Once().Return(false, "", nil) instances.LVM.EXPECT().ListLVs("vg1").Once().Return(nil, fmt.Errorf("mocked error")) _, err := instances.Reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(vg)}) diff --git a/internal/controllers/vgmanager/lvm/lvm.go b/internal/controllers/vgmanager/lvm/lvm.go index 83beb6dd0..ab00b9a2f 100644 --- a/internal/controllers/vgmanager/lvm/lvm.go +++ b/internal/controllers/vgmanager/lvm/lvm.go @@ -58,6 +58,7 @@ type VGReport struct { Vg []struct { Name string `json:"vg_name"` VgSize string `json:"vg_size"` + Tags string `json:"vg_tags"` } `json:"vg"` } `json:"report"` } @@ -95,7 +96,7 @@ type LVM interface { GetVG(name string) (VolumeGroup, error) ListPVs(vgName string) ([]PhysicalVolume, error) - ListVGs() ([]VolumeGroup, error) + ListVGs(taggedByLVMS bool) ([]VolumeGroup, error) ListLVsByName(vgName string) ([]string, error) ListLVs(vgName string) (*LVReport, error) @@ -128,6 +129,9 @@ type VolumeGroup struct { // PVs is the list of physical volumes associated with the volume group PVs []PhysicalVolume `json:"pvs"` + + // Tags is the list of tags associated with the volume group + Tags []string `json:"vg_tags"` } // PhysicalVolume represents a physical volume of linux lvm. @@ -336,17 +340,29 @@ func (hlvm *HostLVM) ListPVs(vgName string) ([]PhysicalVolume, error) { } // ListVolumeGroups lists all volume groups and the physical volumes associated with them. -func (hlvm *HostLVM) ListVGs() ([]VolumeGroup, error) { +func (hlvm *HostLVM) ListVGs(tagged bool) ([]VolumeGroup, error) { res := new(VGReport) - if err := hlvm.execute(res, "vgs", lvmsTag, "--reportformat", "json"); err != nil { + args := []string{ + "vgs", "-o", "vg_name,vg_size,vg_tags", "--units", "g", "--reportformat", "json", + } + if tagged { + args = append(args, lvmsTag) + } + + if err := hlvm.execute(res, args...); err != nil { return nil, fmt.Errorf("failed to list volume groups. %v", err) } var vgList []VolumeGroup for _, report := range res.Report { for _, vg := range report.Vg { - vgList = append(vgList, VolumeGroup{Name: vg.Name, PVs: []PhysicalVolume{}}) + vgList = append(vgList, VolumeGroup{ + Name: vg.Name, + VgSize: vg.VgSize, + PVs: []PhysicalVolume{}, + Tags: strings.Split(vg.Tags, ","), + }) } } @@ -359,6 +375,10 @@ func (hlvm *HostLVM) ListVGs() ([]VolumeGroup, error) { vgList[i].PVs = pvs } + if !tagged { + return untaggedVGs(vgList), nil + } + return vgList, nil } @@ -514,3 +534,20 @@ func (hlvm *HostLVM) execute(v interface{}, args ...string) error { return nil } + +func untaggedVGs(vgs []VolumeGroup) []VolumeGroup { + var untaggedVGs []VolumeGroup + for _, vg := range vgs { + tagPresent := false + for _, tag := range vg.Tags { + if tag == lvmsTag { + tagPresent = true + break + } + } + if !tagPresent { + untaggedVGs = append(untaggedVGs, vg) + } + } + return untaggedVGs +} diff --git a/internal/controllers/vgmanager/lvm/lvm_test.go b/internal/controllers/vgmanager/lvm/lvm_test.go index aecc7d622..d836f0f2a 100644 --- a/internal/controllers/vgmanager/lvm/lvm_test.go +++ b/internal/controllers/vgmanager/lvm/lvm_test.go @@ -260,7 +260,7 @@ func TestHostLVM_ListVGs(t *testing.T) { return "", fmt.Errorf("invalid args %q", args[0]) }, } - vgs, err := NewHostLVM(executor).ListVGs() + vgs, err := NewHostLVM(executor).ListVGs(true) if tt.wantErr { assert.Error(t, err) } else { @@ -603,3 +603,15 @@ func TestHostLVM_execute(t *testing.T) { }) } } + +func Test_untaggedVGs(t *testing.T) { + vgs := []VolumeGroup{ + {Name: "vg1", Tags: []string{"tag1"}}, + {Name: "vg2", Tags: []string{lvmsTag}}, + } + + vgs = untaggedVGs(vgs) + + assert.Len(t, vgs, 1) + assert.Equal(t, "vg1", vgs[0].Name) +} diff --git a/internal/controllers/vgmanager/lvm/mocks/mock_lvm.go b/internal/controllers/vgmanager/lvm/mocks/mock_lvm.go index 09f08cc50..dcbb2da2e 100644 --- a/internal/controllers/vgmanager/lvm/mocks/mock_lvm.go +++ b/internal/controllers/vgmanager/lvm/mocks/mock_lvm.go @@ -640,25 +640,25 @@ func (_c *MockLVM_ListPVs_Call) RunAndReturn(run func(string) ([]lvm.PhysicalVol return _c } -// ListVGs provides a mock function with given fields: -func (_m *MockLVM) ListVGs() ([]lvm.VolumeGroup, error) { - ret := _m.Called() +// ListVGs provides a mock function with given fields: taggedByLVMS +func (_m *MockLVM) ListVGs(taggedByLVMS bool) ([]lvm.VolumeGroup, error) { + ret := _m.Called(taggedByLVMS) var r0 []lvm.VolumeGroup var r1 error - if rf, ok := ret.Get(0).(func() ([]lvm.VolumeGroup, error)); ok { - return rf() + if rf, ok := ret.Get(0).(func(bool) ([]lvm.VolumeGroup, error)); ok { + return rf(taggedByLVMS) } - if rf, ok := ret.Get(0).(func() []lvm.VolumeGroup); ok { - r0 = rf() + if rf, ok := ret.Get(0).(func(bool) []lvm.VolumeGroup); ok { + r0 = rf(taggedByLVMS) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]lvm.VolumeGroup) } } - if rf, ok := ret.Get(1).(func() error); ok { - r1 = rf() + if rf, ok := ret.Get(1).(func(bool) error); ok { + r1 = rf(taggedByLVMS) } else { r1 = ret.Error(1) } @@ -672,13 +672,14 @@ type MockLVM_ListVGs_Call struct { } // ListVGs is a helper method to define mock.On call -func (_e *MockLVM_Expecter) ListVGs() *MockLVM_ListVGs_Call { - return &MockLVM_ListVGs_Call{Call: _e.mock.On("ListVGs")} +// - taggedByLVMS bool +func (_e *MockLVM_Expecter) ListVGs(taggedByLVMS interface{}) *MockLVM_ListVGs_Call { + return &MockLVM_ListVGs_Call{Call: _e.mock.On("ListVGs", taggedByLVMS)} } -func (_c *MockLVM_ListVGs_Call) Run(run func()) *MockLVM_ListVGs_Call { +func (_c *MockLVM_ListVGs_Call) Run(run func(taggedByLVMS bool)) *MockLVM_ListVGs_Call { _c.Call.Run(func(args mock.Arguments) { - run() + run(args[0].(bool)) }) return _c } @@ -688,7 +689,7 @@ func (_c *MockLVM_ListVGs_Call) Return(_a0 []lvm.VolumeGroup, _a1 error) *MockLV return _c } -func (_c *MockLVM_ListVGs_Call) RunAndReturn(run func() ([]lvm.VolumeGroup, error)) *MockLVM_ListVGs_Call { +func (_c *MockLVM_ListVGs_Call) RunAndReturn(run func(bool) ([]lvm.VolumeGroup, error)) *MockLVM_ListVGs_Call { _c.Call.Return(run) return _c } diff --git a/internal/controllers/vgmanager/status.go b/internal/controllers/vgmanager/status.go index 77aa1496a..48199c787 100644 --- a/internal/controllers/vgmanager/status.go +++ b/internal/controllers/vgmanager/status.go @@ -164,7 +164,7 @@ func (r *Reconciler) removeVolumeGroupStatus(ctx context.Context, vg *lvmv1alpha } func (r *Reconciler) setDevices(status *lvmv1alpha1.VGStatus, devices *FilteredBlockDevices) (bool, error) { - vgs, err := r.LVM.ListVGs() + vgs, err := r.LVM.ListVGs(true) if err != nil { return false, fmt.Errorf("failed to list volume groups. %v", err) } diff --git a/internal/tagging/tagging.go b/internal/tagging/tagging.go index 5f8cd880f..b903f7b39 100644 --- a/internal/tagging/tagging.go +++ b/internal/tagging/tagging.go @@ -2,6 +2,7 @@ package tagging import ( "context" + "errors" "fmt" lvmv1 "github.com/openshift/lvm-operator/api/v1alpha1" @@ -17,49 +18,88 @@ import ( func AddTagToVGs(ctx context.Context, c client.Client, lvm lvm.LVM, nodeName string, namespace string) error { logger := log.FromContext(ctx) - vgs, err := lvm.ListVGs() + // now we get all untagged vgs on the node + vgs, err := lvm.ListVGs(false) if err != nil { - return fmt.Errorf("failed to list volume groups: %w", err) + return fmt.Errorf("failed to list volume groups on node "+ + "to determine if tag migration is necessary: %w", err) + } else if len(vgs) == 0 { + logger.Info("no untagged volume groups found on the node, skipping migration") + return nil } + logger.Info("found untagged volume groups on the node", "vgs", vgs) - lvmVolumeGroupList := &lvmv1.LVMVolumeGroupList{} - err = c.List(ctx, lvmVolumeGroupList, &client.ListOptions{Namespace: namespace}) + potentialVGsInNode, err := vgCandidatesForTagMigration(ctx, c, nodeName, namespace) if err != nil { - return fmt.Errorf("failed to list LVMVolumeGroups: %w", err) + return fmt.Errorf("failed to get potential volume groups for node during tag migration: %w", err) + } else if len(potentialVGsInNode) == 0 { + logger.Info("no potential volume groups found for the node, skipping migration") + return nil } + logger.Info("found existing LVMVolumeGroups candidates for the node", "vgs", potentialVGsInNode) - // If there is a matching LVMVolumeGroup CR, tag the existing volume group - for _, vg := range vgs { - tagged := false - for _, lvmVolumeGroup := range lvmVolumeGroupList.Items { - if vg.Name != lvmVolumeGroup.Name { - continue - } - if lvmVolumeGroup.Spec.NodeSelector != nil { - node := &corev1.Node{} - err = c.Get(ctx, types.NamespacedName{Name: nodeName}, node) - if err != nil { - return fmt.Errorf("failed to get node %s: %w", nodeName, err) - } + var tagged []string + var taggingErr error - matches, err := corev1helper.MatchNodeSelectorTerms(node, lvmVolumeGroup.Spec.NodeSelector) - if err != nil { - return fmt.Errorf("failed to match nodeSelector to node labels: %w", err) - } - if !matches { - continue - } + for _, potentialVG := range potentialVGsInNode { + // If there is an existing Volume Group managed by LVMS (through LVMVolumeGroup) that is also present on the node + // but is not tagged, we should tag it now. + for _, vg := range vgs { + if vg.Name != potentialVG { + continue } - + logger.Info("tagging volume group managed by LVMS", "vg", vg.Name) if err := lvm.AddTagToVG(vg.Name); err != nil { - return err + taggingErr = errors.Join(taggingErr, fmt.Errorf("failed to tag volume group %s: %w", vg.Name, err)) + continue } - tagged = true - } - if !tagged { - logger.Info("skipping tagging volume group %s as there is no corresponding LVMVolumeGroup CR", vg.Name) + tagged = append(tagged, vg.Name) } } + if len(tagged) > 0 { + logger.Info("tagged volume groups", "count", len(tagged), "vgs", tagged) + } else { + logger.Info("no volume groups were tagged") + } + + if taggingErr != nil { + return fmt.Errorf("failed to tag at least one volume group: %w", taggingErr) + } + return nil } + +func vgCandidatesForTagMigration(ctx context.Context, c client.Client, nodeName, namespace string) ([]string, error) { + node := &corev1.Node{} + if err := c.Get(ctx, types.NamespacedName{Name: nodeName}, node); err != nil { + return nil, fmt.Errorf("failed to get node %s: %w", nodeName, err) + } + + lvmVolumeGroupList := &lvmv1.LVMVolumeGroupList{} + if err := c.List(ctx, lvmVolumeGroupList, &client.ListOptions{Namespace: namespace}); err != nil { + return nil, fmt.Errorf("failed to list LVMVolumeGroups from cluster: %w", err) + } + + // to tag only the volume groups that are not tagged yet and managed by the operator + // we need to find the volume groups that should be on the node already (based on the LVMVolumeGroup CRs) + // and then reapply the tag to them if they are not tagged yet. + // however, even though the CR exists it might not be on the node yet, so its only a candidate for tagging + var potentialVGsInNode []string + for _, lvmVolumeGroup := range lvmVolumeGroupList.Items { + // If the LVMVolumeGroup CR does not have a nodeSelector, the vg is a potential match by default + if lvmVolumeGroup.Spec.NodeSelector == nil { + potentialVGsInNode = append(potentialVGsInNode, lvmVolumeGroup.GetName()) + continue + } + // If the LVMVolumeGroup CR has a nodeSelector, the vg is a potential match if the node labels match the nodeSelector + matches, err := corev1helper.MatchNodeSelectorTerms(node, lvmVolumeGroup.Spec.NodeSelector) + if err != nil { + return nil, fmt.Errorf("failed to match nodeSelector to node labels: %w", err) + } + if matches { + potentialVGsInNode = append(potentialVGsInNode, lvmVolumeGroup.GetName()) + } + } + return potentialVGsInNode, nil +} diff --git a/internal/tagging/tagging_test.go b/internal/tagging/tagging_test.go index 2cd356291..479a01839 100644 --- a/internal/tagging/tagging_test.go +++ b/internal/tagging/tagging_test.go @@ -23,12 +23,38 @@ func TestAddTagToVGs(t *testing.T) { hostnameLabelKey := "kubernetes.io/hostname" vgName := "vgtest1" + node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName, Labels: map[string]string{ + hostnameLabelKey: nodeName, + }}} + testCases := []struct { - name string - clientObjects []client.Object - volumeGroups []lvm.VolumeGroup - addTagCount int + name string + clientObjects []client.Object + untaggedVolumeGroups []lvm.VolumeGroup + listVGsErr error + addTagCountSuccessful int + addTagCountError int }{ + { + name: "there is a matching CR in the same namespace, but no volume group to tag", + clientObjects: []client.Object{ + &v1alpha1.LVMVolumeGroup{ObjectMeta: metav1.ObjectMeta{ + Name: vgName, + Namespace: namespace, + }}, + }, + untaggedVolumeGroups: []lvm.VolumeGroup{}, + }, + { + name: "there is a matching CR in the same namespace, but vg fetching failed", + clientObjects: []client.Object{ + &v1alpha1.LVMVolumeGroup{ObjectMeta: metav1.ObjectMeta{ + Name: vgName, + Namespace: namespace, + }}, + }, + listVGsErr: assert.AnError, + }, { name: "there is a matching CR in the same namespace, add a tag", clientObjects: []client.Object{ @@ -36,13 +62,14 @@ func TestAddTagToVGs(t *testing.T) { Name: vgName, Namespace: namespace, }}, + node, }, - volumeGroups: []lvm.VolumeGroup{ + untaggedVolumeGroups: []lvm.VolumeGroup{ { Name: vgName, }, }, - addTagCount: 1, + addTagCountSuccessful: 1, }, { name: "there are two matching CRs in the same namespace, add tags", @@ -55,8 +82,9 @@ func TestAddTagToVGs(t *testing.T) { Name: "vgtest2", Namespace: namespace, }}, + node, }, - volumeGroups: []lvm.VolumeGroup{ + untaggedVolumeGroups: []lvm.VolumeGroup{ { Name: vgName, }, @@ -64,17 +92,63 @@ func TestAddTagToVGs(t *testing.T) { Name: "vgtest2", }, }, - addTagCount: 2, + addTagCountSuccessful: 2, + }, { + name: "there are two matching CRs in the same namespace, add tags but fail on both of them", + clientObjects: []client.Object{ + &v1alpha1.LVMVolumeGroup{ObjectMeta: metav1.ObjectMeta{ + Name: vgName, + Namespace: namespace, + }}, + &v1alpha1.LVMVolumeGroup{ObjectMeta: metav1.ObjectMeta{ + Name: "vgtest2", + Namespace: namespace, + }}, + node, + }, + untaggedVolumeGroups: []lvm.VolumeGroup{ + { + Name: vgName, + }, + { + Name: "vgtest2", + }, + }, + addTagCountError: 2, + }, + { + name: "there are two matching CRs in the same namespace, add tags but fail on one of them", + clientObjects: []client.Object{ + &v1alpha1.LVMVolumeGroup{ObjectMeta: metav1.ObjectMeta{ + Name: vgName, + Namespace: namespace, + }}, + &v1alpha1.LVMVolumeGroup{ObjectMeta: metav1.ObjectMeta{ + Name: "vgtest2", + Namespace: namespace, + }}, + node, + }, + untaggedVolumeGroups: []lvm.VolumeGroup{ + { + Name: vgName, + }, + { + Name: "vgtest2", + }, + }, + addTagCountSuccessful: 1, + addTagCountError: 1, }, { - name: "there is no matching CR, do not add a tag", - clientObjects: []client.Object{}, - volumeGroups: []lvm.VolumeGroup{ + name: "there is no matching LVMVolumeGroup CR, do not add a tag", + clientObjects: []client.Object{node}, + untaggedVolumeGroups: []lvm.VolumeGroup{ { Name: vgName, }, }, - addTagCount: 0, + addTagCountSuccessful: 0, }, { name: "there is a matching CR in a different namespace, do not add a tag", @@ -83,13 +157,14 @@ func TestAddTagToVGs(t *testing.T) { Name: vgName, Namespace: "test-namespace-2", }}, + node, }, - volumeGroups: []lvm.VolumeGroup{ + untaggedVolumeGroups: []lvm.VolumeGroup{ { Name: vgName, }, }, - addTagCount: 0, + addTagCountSuccessful: 0, }, { name: "there is a matching CR with a matching node selector, add a tag", @@ -115,16 +190,14 @@ func TestAddTagToVGs(t *testing.T) { }, }, }, - &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName, Labels: map[string]string{ - hostnameLabelKey: nodeName, - }}}, + node, }, - volumeGroups: []lvm.VolumeGroup{ + untaggedVolumeGroups: []lvm.VolumeGroup{ { Name: vgName, }, }, - addTagCount: 1, + addTagCountSuccessful: 1, }, { name: "there is a matching CR with a non-matching node selector, do not add a tag", @@ -150,21 +223,17 @@ func TestAddTagToVGs(t *testing.T) { }, }, }, - &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName, Labels: map[string]string{ - hostnameLabelKey: nodeName, - }}}, + node, }, - volumeGroups: []lvm.VolumeGroup{ + untaggedVolumeGroups: []lvm.VolumeGroup{ { Name: vgName, }, }, - addTagCount: 0, + addTagCountSuccessful: 0, }, } - mockLVM := lvmmocks.NewMockLVM(t) - scheme, err := v1alpha1.SchemeBuilder.Build() assert.NoError(t, err, "creating scheme") err = corev1.AddToScheme(scheme) @@ -172,13 +241,32 @@ func TestAddTagToVGs(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + mockLVM := lvmmocks.NewMockLVM(t) + defer mockLVM.AssertExpectations(t) c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.clientObjects...).Build() - if tc.addTagCount > 0 { - mockLVM.EXPECT().AddTagToVG(mock.Anything).Return(nil).Times(tc.addTagCount) + if tc.addTagCountSuccessful > 0 { + mockLVM.EXPECT().AddTagToVG(mock.Anything).Return(nil).Times(tc.addTagCountSuccessful) } - mockLVM.EXPECT().ListVGs().Return(tc.volumeGroups, nil).Once() + if tc.addTagCountError > 0 { + mockLVM.EXPECT().AddTagToVG(mock.Anything).Return(assert.AnError).Times(tc.addTagCountError) + } + + if tc.listVGsErr != nil { + mockLVM.EXPECT().ListVGs(false).Return(nil, tc.listVGsErr).Times(1) + } else { + mockLVM.EXPECT().ListVGs(false).Return(tc.untaggedVolumeGroups, nil).Times(1) + } + err := tagging.AddTagToVGs(context.Background(), c, mockLVM, nodeName, namespace) - assert.NoError(t, err) + + if tc.addTagCountError > 0 { + assert.ErrorIs(t, err, assert.AnError) + } else if tc.listVGsErr != nil { + assert.ErrorIs(t, err, tc.listVGsErr) + } else { + assert.NoError(t, err) + } + }) } } From 197fc7e26cfb6812154228a42a0bf94a63370936 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20M=C3=B6ller?= Date: Fri, 2 Feb 2024 18:21:49 +0100 Subject: [PATCH 2/2] chore: correct catalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jakob Möller --- catalog/lvms-operator/operator.yaml | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/catalog/lvms-operator/operator.yaml b/catalog/lvms-operator/operator.yaml index 5255bb6aa..6b574a062 100644 --- a/catalog/lvms-operator/operator.yaml +++ b/catalog/lvms-operator/operator.yaml @@ -86,13 +86,25 @@ properties: } } operatorframework.io/suggested-namespace: openshift-storage + operatorframework.io/suggested-namespace-template: |- + { + "apiVersion": "v1", + "kind": "Namespace", + "metadata": { + "name": "openshift-storage", + "annotations": { + "workload.openshift.io/allowed": "management" + } + } + } operators.openshift.io/infrastructure-features: '["csi", "disconnected"]' - operators.operatorframework.io/builder: operator-sdk-v1.25.3 + operators.openshift.io/valid-subscription: '["OpenShift Container Platform", + "OpenShift Platform Plus"]' + operators.operatorframework.io/builder: operator-sdk-v1.33.0 operators.operatorframework.io/internal-objects: '["logicalvolumes.topolvm.io", "lvmvolumegroups.lvm.topolvm.io", "lvmvolumegroupnodestatuses.lvm.topolvm.io"]' operators.operatorframework.io/project_layout: go.kubebuilder.io/v3 repository: https://github.com/openshift/lvm-operator - target.workload.openshift.io/management: '{"effect": "PreferredDuringScheduling"}' apiServiceDefinitions: {} crdDescriptions: owned: