controller: fix reclaimspace based on ns annotation #396
Conversation
| @@ -134,13 +134,22 @@ func (r *PersistentVolumeClaimReconciler) Reconcile(ctx context.Context, req ctr | |||
| logger.Error(err, "Failed to get Namespace", "Namespace", pvc.Namespace) | |||
| return ctrl.Result{}, err | |||
| } | |||
| schedule, scheduleFound = getScheduleFromAnnotation(&logger, ns.Annotations) | |||
| // If the schedule is not found, check whether driver supports the | |||
| nsSchedule, nsScheduleFound := getScheduleFromAnnotation(&logger, ns.Annotations) | |||
There was a problem hiding this comment.
Do we need new variables? Can we assign this to exist schedule,schduleFound?
There was a problem hiding this comment.
Do we need new variables? Can we assign this to exist schedule,schduleFound?
This current form simplifies the code and makes it easier to understand,
Otherwise it was very difficult to follow the flow.
please read the comments inside the statements.
There was a problem hiding this comment.
hmm, this change does not improve the function for me. I have read the comments and change a few times now (that was really needed to understand it), so I think it is not clear enough. If you want to improve the behaviour, split things out in helper functions instead.
There was a problem hiding this comment.
We are evaluating a lot of conditions in these statements,
I don't believe adding helper function which take in a lot of arguments and also return a lot of arguments will make the code flow any cleaner.
There was a problem hiding this comment.
Moved the statements into a helper function with lots of io args.
Hopefully it is simpler to follow now 🤞
There was a problem hiding this comment.
Thanks! I am not a big fan of many return values or arguments to a function either, but this is definitely easier to understand than what there was before.
One thing that could make it even cleaner, is returning an error if requeuing shoud not be needed. This, and maybe the schedule-not-found could be pre-defined errors. You can then check with errors.Is(...) what early return from the reconcile should be done. Can you consider doing that, and see if it makes it even cleaner?
There was a problem hiding this comment.
made the suggested changes, ptal.
There was a problem hiding this comment.
Both the args are now Err types and will be detected using errors.Is().
Rakshith-R
force-pushed
the
ns-annotate-fix
branch
from
4a59fae
to
7482c6e
Compare
|
@Mergifyio rebase |
✅ Branch has been successfully rebased |
Rakshith-R
force-pushed
the
ns-annotate-fix
branch
2 times, most recently
from
d51a9dc
to
f344e4d
Compare
Rakshith-R
force-pushed
the
ns-annotate-fix
branch
from
f344e4d
to
b4ace49
Compare
Logic used for determining reclaimspace annotation based on ns annotation and driver support had a bug which caused all the PVCs regardless of driversupport being annotated. This commit makes sure only pvc with driver which support reclaimspace is annotated/ requeued. Signed-off-by: Rakshith R <rar@redhat.com>
Rakshith-R
force-pushed
the
ns-annotate-fix
branch
from
b4ace49
to
f0451b2
Compare
Logic used for determining reclaimspace annotation based on ns annotation and driver support had a bug which caused all the PVCs regardless of driversupport being annotated.
This commit makes sure only pvc with driver which
support reclaimspace is annotated/ requeued.