OSSM-2188 Reconcile SMMR even when SMCP is missing #1063
Conversation
This ensures that the ServiceMeshMember resources created by the controller are deleted when the SMCP is deleted.
| for _, ns := range requiredNamespaces.List() { | ||
| member, err := r.ensureMemberExists(ctx, ns, mesh.Name, meshNamespace) | ||
| if err != nil { | ||
| return reconcile.Result{}, err |
There was a problem hiding this comment.
| return reconcile.Result{}, err | |
| return common.RequeueWithError(err) |
There was a problem hiding this comment.
I don't see any real benefit of doing this. I see how you could argue that seeing RequeueWithError() immediately tells you that the object is getting re-queued, but on the other hand, when you see this function call, you can't know what it does until you look at its implementation.
I feel invoking a function here just slows down anyone reading this code (until they know by heart what the function does), whereas a simple return reconcile.Result{}, err perfectly shows what it does (even though it doesn't say that the object gets re-queued, but that's something every engineer working on k8s controllers should know). Also, who guarantees that the fact that you're returning an error will result in the object being re-queued? That may not be true in some cases. In this case, the function name would be misleading.
There was a problem hiding this comment.
I don't see any real benefit of doing this.
Ok, I expected that you will like the idea, because I remember your positive feedback when I added this function. Look at this comment.
whereas a simple
return reconcile.Result{}, errperfectly shows what it does (even though it doesn't say that the object gets re-queued, but that's something every engineer working on k8s controllers should know)
It's a subjective point of view and I don't agree with you. I review PRs regularly and every time I see reconciliation I have to stay focus to understand how we reconcile objects. And it's not only my opinion - look at point 7 here.
There was a problem hiding this comment.
I don't dislike the idea. I just think there are pros and cons (as always) that I outlined in my previous comment. And I agree with point 7, but also think it doesn't address the part about having to go look at the function (and possibly being misleading if the error is handled in one of the calling functions and thus preventing the re-queue).
The thing with these types of helper functions is that nothing forces you to use it, so you'll eventually end up with X% of the code using the function and the remaining code that doesn't. Then the question of consistency comes up.
I don't really have a strong opinion on using one approach or the other. I guess I have to give your opinion a bit more weight, since I've been writing controllers a long time and it's all the same to me. But if it helps people who work on controller code less often, I agree that we should be using the function.
However, if we decide that we should always use RequeueWithError and similar, we should probably add a linter that enforces this decision.
| member, err := r.ensureMemberExists(ctx, ns, mesh.Name, meshNamespace) | ||
| if err != nil { | ||
| return reconcile.Result{}, err | ||
| } else if member == nil { |
There was a problem hiding this comment.
The previous branch of this if-else statement returns from function when it's true, so else if is not necessary in this case. What do you think about this?
| } else if member == nil { | |
| } | |
| if member == nil { |
There was a problem hiding this comment.
You're correct, but the reason I did it like this is to clearly separate the exceptional and the normal outcome of the ensureMemberExists function.
The exceptional outcomes of the function are an actual error or a nil member, both of which require special handling. By using if ... else if the handling of these outcomes is visually tied together, whereas a separate if statement wouldn't be (as much).
In reality, the ensureMemberExists function should return a NamespaceNotExist type of error instead of returning a nil member.
| @@ -411,6 +416,25 @@ func (r *MemberRollReconciler) reconcileObject(ctx context.Context, roll *maistr | |||
| return reconcile.Result{}, nil | |||
There was a problem hiding this comment.
Since you are changing this function, what do you think about refactoring this line?
| return reconcile.Result{}, nil | |
| return common.Reconciled() |
This ensures that any ServiceMeshMembers that were created by the controller while the SMCP existed get removed.