[monitoring] Check pv minimum count #201
Conversation
Signed-off-by: Denis Shipkov <[email protected]>
Signed-off-by: Denis Shipkov <[email protected]>
Signed-off-by: Denis Shipkov <[email protected]>
Signed-off-by: Denis Shipkov <[email protected]>
| ReconcileTieBreaker(ctx, log, lc, rdMap, rgMap) | ||
| pvsList, err := GetListPV(ctx, cl) | ||
| if err != nil { | ||
| log.Error(err, "[NewLinstorResourcesWatcher] unable to get Persistent Volumes") |
There was a problem hiding this comment.
log.Error returns normally and execution proceeds. It leads to a bunch of bugs, since any collection may be nil, because of a single transmission issue. Let's not reproduce it this time, but rather fix it in all places above.
| log.Info("[ReconcilePVReplicas] starts work") | ||
|
|
||
| for _, pv := range pvs { | ||
| if pv.Spec.CSI != nil && pv.Spec.CSI.Driver == PVCSIDriver { |
There was a problem hiding this comment.
Inverse this condition to reduce nesting level of the code below.
| replicasErrLevel := checkPVMinReplicasCount(ctx, log, lc, rg, res[pv.Name]) | ||
|
|
||
| if pv.Labels == nil { | ||
| pv.Labels = make(map[string]string) |
There was a problem hiding this comment.
I'll choose this line as an example. Review point is about problem between ReconcileParams and ReconcilePVReplicas.
pv variable holds a local value, which was copied from the original slice pvs. It's not a pointer, and you're not accessing the original array element by index, so any modifications to it will affect only this local copy. I'll use term value semantics to refer to this effect.
Value semantics is fine for ReconcilePVReplicas on it's own, and it was fine for ReconcileParams, but after you've extended the scope of the state (moved pvs to the outer scope), it became a problem, since any change to labels won't affect the pvs collection. It will lead to second method ReconcilePVReplicas to overwrite the labels set by the ReconcileParams.
This should be fixed by using reference semantics.
Description
Why do we need it, and what problem does it solve?
What is the expected result?
Checklist