OCPVE-616: fix: add conditions to error on missing lv in case VG is present but thin-pool is not #386
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jakobmoellerdev: This pull request references OCPVE-616 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci
bot
added
the
size/M
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #386 +/- ##
===========================================
+ Coverage 16.59% 57.42% +40.82%
===========================================
Files 24 26 +2
Lines 2061 2290 +229
===========================================
+ Hits 342 1315 +973
+ Misses 1693 884 -809
- Partials 26 91 +65
|
|
/test lvm-operator-e2e-aws |
| @@ -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, " + | |||
There was a problem hiding this comment.
Please use errors.New() and "`" symbol to avoid concatenation
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
-
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
-
Errorf scans the string for formatting chars like
%sand etc. When we know that there is no formatting, why do extra work?
This isErrorfImplementation
And this is forerrors.New()implementation
Most of the places which are using Errorf actually have formatting symbols
|
|
||
| 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, " + |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jakobmoellerdev, suleymanakbas91 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@jakobmoellerdev: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
This adds more failure conditions where the LV could be corrupt and the LVMCluster will now go into degraded state.
Specifically three conditions are created:
lvsreport of LVM is emptylvsreport of LVM is not empty but has no part for any LVlvsreport of LVM is not empty but only contains entries for LVs that are not the thin-pool we created.For all three cases, previously the LVMCluster would remain in "Ready", now it will be "Degraded" instead