Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OCPVE-616: fix: add conditions to error on missing lv in case VG is present but thin-pool is not #386

Merged
merged 1 commit into from
Aug 21, 2023
+80 −0
Merged

OCPVE-616: fix: add conditions to error on missing lv in case VG is present but thin-pool is not #386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions pkg/vgmanager/vgmanager_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,12 +382,22 @@ func (r *VGReconciler) validateLVs(volumeGroup *lvmv1alpha1.LVMVolumeGroup) erro
if err != nil {
return fmt.Errorf("could not get logical volumes found inside volume group, volume group content is degraded or corrupt: %w", err)
}
if len(resp.Report) < 1 {
return fmt.Errorf("LV report was empty, meaning that the thin-pool LV is no longer found, " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use errors.New() and "`" symbol to avoid concatenation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why its bad to have concatenation? We never used "`" in the entire Repo and only have 3 cases of errors.New()? Could you explain why thats better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits inherently don't block merging.

That said, I'm likely to blame for setting a precedent on this type of suggestion. I'm generally quite against concatentation as there's rarely a good case to use it. Happy to explain further but that's probably best done outside of a PR 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakobmoellerdev

  1. Why do we need to concatenate strings when they are not dynamic? Both of them are well known and written by us, so this is a useless job for compiler/runtime

  2. Errorf scans the string for formatting chars like %s and etc. When we know that there is no formatting, why do extra work?
    This is Errorf Implementation
    And this is for errors.New() implementation

Most of the places which are using Errorf actually have formatting symbols

"but the volume group might still exist")
}

for _, report := range resp.Report {
if len(report.Lv) < 1 {
return fmt.Errorf("no LV was found in the report, meaning that the thin-pool LV is no longer found, " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

"but the volume group might still exist")
}
thinPoolExists := false
for _, lv := range report.Lv {
if lv.Name != volumeGroup.Spec.ThinPoolConfig.Name {
continue
}
thinPoolExists = true
lvAttr, err := ParsedLvAttr(lv.LvAttr)
if err != nil {
return fmt.Errorf("could not parse lv_attr from logical volume %s: %w", lv.Name, err)
Expand All @@ -414,6 +424,9 @@ func (r *VGReconciler) validateLVs(volumeGroup *lvmv1alpha1.LVMVolumeGroup) erro

r.Log.Info("confirmed created logical volume has correct attributes", "lv_attr", lvAttr.String())
}
if !thinPoolExists {
return fmt.Errorf("the thin-pool LV is no longer present, but the volume group might still exist")
}
}
return nil
}
Expand Down
67 changes: 67 additions & 0 deletions pkg/vgmanager/vgmanager_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,34 @@ import (
"k8s.io/utils/strings/slices"
)

var mockLvsOutputNoReportContent = `
{
"report": []
}
`

var mockLvsOutputNoLVsInReport = `
{
"report": [
{
"lv": []
}
]
}
`

var mockLvsOutputWrongLVsInReport = `
{
"report": [
{
"lv": [
{"lv_name":"thin-pool-BLUB", "vg_name":"vg1", "lv_attr":"twi-a-tz--", "lv_size":"26.96g", "pool_lv":"", "origin":"", "data_percent":"0.00", "metadata_percent":"10.52", "move_pv":"", "mirror_log":"", "copy_percent":"", "convert_lv":""}
]
}
]
}
`

var mockLvsOutputThinPoolValid = `
{
"report": [
Expand Down Expand Up @@ -147,6 +175,45 @@ func TestVGReconciler_validateLVs(t *testing.T) {
}},
wantErr: assert.Error,
},
{
name: "Invalid LV due to empty report",
fields: fields{
executor: mockExecutorForLVSOutput(mockLvsOutputNoReportContent),
},
args: args{volumeGroup: &lvmv1alpha1.LVMVolumeGroup{
ObjectMeta: metav1.ObjectMeta{Name: "vg1", Namespace: "default"},
Spec: lvmv1alpha1.LVMVolumeGroupSpec{ThinPoolConfig: &lvmv1alpha1.ThinPoolConfig{
Name: "thin-pool-1",
}},
}},
wantErr: assert.Error,
},
{
name: "Invalid LV due to no LVs in report",
fields: fields{
executor: mockExecutorForLVSOutput(mockLvsOutputNoLVsInReport),
},
args: args{volumeGroup: &lvmv1alpha1.LVMVolumeGroup{
ObjectMeta: metav1.ObjectMeta{Name: "vg1", Namespace: "default"},
Spec: lvmv1alpha1.LVMVolumeGroupSpec{ThinPoolConfig: &lvmv1alpha1.ThinPoolConfig{
Name: "thin-pool-1",
}},
}},
wantErr: assert.Error,
},
{
name: "Invalid LV due to wrong LVs in report",
fields: fields{
executor: mockExecutorForLVSOutput(mockLvsOutputWrongLVsInReport),
},
args: args{volumeGroup: &lvmv1alpha1.LVMVolumeGroup{
ObjectMeta: metav1.ObjectMeta{Name: "vg1", Namespace: "default"},
Spec: lvmv1alpha1.LVMVolumeGroupSpec{ThinPoolConfig: &lvmv1alpha1.ThinPoolConfig{
Name: "thin-pool-1",
}},
}},
wantErr: assert.Error,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down