Optimize updateAndSetOwner to Reduce Multiple Updates and Prevent Conflicts #1874
Conversation
OdedViner
force-pushed
the
fix_updateandsetowner
branch
5 times, most recently
from
6aae30e to
6a23c9d
Compare
There was a problem hiding this comment.
You are doing too many changes at once - moving entire functions and modifying them. It is not possible to review such changes safely.
Try to do minimal changes:
- Modify existing tests or add new test to reproduce the failure. This is good first commit proving that the test is correct.
- Remove the actual update call from existing functions and add the update after we call all functions.
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" |
There was a problem hiding this comment.
We don't use these kind of helpers in ramen tests. Check how existing tests are implemented and use the same code.
If you want to add new test infra it should be done separately from this pr.
| fakeClient := fake.NewClientBuilder(). | ||
| WithScheme(scheme). | ||
| WithObjects(drpc, placementObj). | ||
| Build() |
There was a problem hiding this comment.
We use a fake client to test updateAndSetOwner in isolation, ensuring it correctly updates DRPC and placement objects without needing a real cluster. This allows us to verify annotations, finalizers, and owner references while making debugging easier. I followed a similar approach used in Rook for clarity and consistency.
https://github.com/rook/rook/blob/9a5bf16247ce1b13e2475256ce20dc2f8bfce54c/pkg/operator/ceph/file/controller_test.go#L209
Of course, I will remove it from this PR, but my question is whether we can add unit tests to the controllers package. Many private functions (not starting with a capital letter) cannot be tested from the controller_test package.
Additionally, having unit tests in controllers could help team members navigate the code more easily. It helped me a lot when I started working on the OCS Operator and Rook projects.
@nirs @BenamarMk
What do you think?
| drpcModified, placementModified, err := r.prepareObjectModifications(drpc, usrPlacement, log) | ||
| if err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
I would keep the old code - having small and focus functions that do one change instead of big and complex prepare function. Just remove the actual update from the existing functions, and finally run the actual update at the end.
We already have code doing this in other places, for example when deleting drpc we modify all resources and update only at the end.
| } | ||
|
|
||
| // applyUpdates applies updates to both DRPC and placement objects if they were modified | ||
| func (r *DRPlacementControlReconciler) applyUpdates( |
There was a problem hiding this comment.
Adding updatePlacement() and updateDRPC() will be more useful and simpler. For example this can be used in other places that need to update only one of them.
| placementModified = placementModified || modPlacement | ||
|
|
||
| // Modify DRPC to set the owner reference | ||
| mod, err = r.setDRPCOwner(drpc, usrPlacement, log) |
| placementModified = placementModified || mod | ||
|
|
||
| // Modify DRPC and placement via metadata updates | ||
| modDRPC, modPlacement, err := r.updateObjectMetadata(drpc, usrPlacement) |
There was a problem hiding this comment.
This should really be 2 functions, one to update the placement and another for the drpc.
| var drpcModified, placementModified bool | ||
|
|
||
| // Annotate the placement object | ||
| mod, err := r.annotateObject(drpc, usrPlacement, log) |
OdedViner
force-pushed
the
fix_updateandsetowner
branch
13 times, most recently
from
71d2202 to
66c030e
Compare
| update = rmnutil.AddLabel(drpc, rmnutil.OCMBackupLabelKey, rmnutil.OCMBackupLabelValue) | ||
| update = rmnutil.AddFinalizer(drpc, DRPCFinalizer) || update | ||
| added := rmnutil.AddFinalizer(drpc, DRPCFinalizer) | ||
| if added { |
There was a problem hiding this comment.
The "added" variable does not strictly need to be declared. You can inline the function call directly into the if statement
| return false, err | ||
| } | ||
|
|
||
| return r.setDRPCOwner(ctx, drpc, usrPlacement, log) | ||
| // Merge both modifications | ||
| placementModified = placementAnnotateModified || placementMetadataModified |
There was a problem hiding this comment.
placementModified can be computed inline, I mean something like this:
if placementAnnotateModified || placementMetadataModified {
if err := r.updatePlacement(ctx, usrPlacement, log); err != nil {
return false, err
}
}
| ) (drpcModified bool, placementModified bool, err error) { | ||
| // Explicit initialization | ||
| drpcModified = false | ||
| placementModified = false |
There was a problem hiding this comment.
Not really needed. A better way would be to avoid named return values which lead to abuse.
|
|
||
| vrgNamespace, err := selectVRGNamespace(r.Client, r.Log, drpc, placementObj) | ||
| if err != nil { | ||
| return err | ||
| return drpcModified, placementModified, err |
There was a problem hiding this comment.
When this fails we return with an error, but the drpc and placementObj may have changes that may fool the code using them after this point.
Maybe we need to call selectVRGNamespace() before we modify the objects?
| placementModified = placementAnnotateModified || placementMetadataModified | ||
|
|
||
| // Set DRPC Owner | ||
| updated, err = r.setDRPCOwner(drpc, usrPlacement, log) |
There was a problem hiding this comment.
What is "updated"? it is not clear as the other xxxModified variables.
|
|
||
| if err = r.updateDRPC(ctx, drpc, log); err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
We don't need to check if the drpc needs an update?
There was a problem hiding this comment.
The DRPC CR updates every time without any condition.
| log logr.Logger, | ||
| ) error { | ||
| if err := r.Update(ctx, owner); err != nil { | ||
| log.Error(err, "Failed to update Placement object") |
There was a problem hiding this comment.
Don't log an error here, we return an error. This will log the same error multiple times.
|
|
||
| func (r *DRPlacementControlReconciler) updatePlacement( | ||
| ctx context.Context, | ||
| owner client.Object, |
There was a problem hiding this comment.
owner is not clear. Based on the code this is the a placement or placementRule object. Check how we call this object in other code around. We should use the same name.
| log logr.Logger, | ||
| ) error { | ||
| if err := r.Update(ctx, drpc); err != nil { | ||
| log.Error(err, "Failed to update DRPC") |
There was a problem hiding this comment.
Don't log an error if you return the error.
| } | ||
|
|
||
| func (r *DRPlacementControlReconciler) setDRPCOwner( | ||
| ctx context.Context, drpc *rmn.DRPlacementControl, owner client.Object, log logr.Logger, | ||
| drpc *rmn.DRPlacementControl, owner client.Object, log logr.Logger, | ||
| ) (bool, error) { | ||
| const updated = true |
There was a problem hiding this comment.
updated is confusing now. Better rename to "needUpdate" or "dirty".
| var update bool | ||
| func (r DRPlacementControlReconciler) updateObjectMetadata( | ||
| drpc *rmn.DRPlacementControl, placementObj client.Object, | ||
| ) (drpcModified bool, placementModified bool, err error) { |
There was a problem hiding this comment.
Why we change both objects in this function? It will be better split this to 2 functions , each modifying one object. This avoids the error prone function return value (bool, bool, error). It is very easy to call the function in the wrong way:
drpcModified, placementModified, err := r.updateObjectMetadata()
placementModified, drpcModified, err := r.updateObjectMetadata()
Which call it the correct one? The type system does not help with such code.
| return false, err | ||
| } | ||
|
|
||
| return updated, nil |
There was a problem hiding this comment.
This may return false when the drpc and/or placement were modified.
There was a problem hiding this comment.
correct, based on the original function
ramen/internal/controller/drplacementcontrol_controller.go
Lines 865 to 866 in c975f8c
OdedViner
force-pushed
the
fix_updateandsetowner
branch
from
66c030e to
c904262
Compare
|
|
||
| if rmnutil.AddFinalizer(drpc, DRPCFinalizer) { | ||
| modified = true | ||
| } | ||
|
|
||
| vrgNamespace, err := selectVRGNamespace(r.Client, r.Log, drpc, placementObj) |
There was a problem hiding this comment.
@OdedViner , maybe we can selectVRGNamespace and only then add finalizer and label?
…flicts Signed-off-by: Oded Viner <[email protected]>
OdedViner
force-pushed
the
fix_updateandsetowner
branch
from
c904262 to
e1a8345
Compare
This PR ensures that the updateAndSetOwner function updates the Placement and DRPC resources only once per reconciliation loop. Previously, multiple Update() calls caused resource version conflicts and delayed execution.
Changes Introduced:
These optimizations reduce redundant updates, improving performance and stability.
Fixes: #1066