8000 Optimize updateAndSetOwner to Reduce Multiple Updates and Prevent Conflicts by OdedViner · Pull Request #1874 · RamenDR/ramen · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Optimize updateAndSetOwner to Reduce Multiple Updates and Prevent Conflicts #1874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Optimize updateAndSetOwner to Reduce Multiple Updates and Prevent Conflicts #1874

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 98 additions & 54 deletions internal/controller/drplacementcontrol_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,41 +564,37 @@ func GetDRClusters(ctx context.Context, client client.Client, drPolicy *rmn.DRPo
return drClusters, nil
}

// updateObjectMetadata updates drpc labels, annotations and finalizer, and also updates placementObj finalizer
func (r DRPlacementControlReconciler) updateObjectMetadata(ctx context.Context,
drpc *rmn.DRPlacementControl, placementObj client.Object, log logr.Logger,
) error {
var update bool
// updateDRPCMetadata updates DRPC labels, annotations and finalizer only
func (r DRPlacementControlReconciler) updateDRPCMetadata(
drpc *rmn.DRPlacementControl, placementObj client.Object,
) (modified bool, err error) {
modified = false

if rmnutil.AddLabel(drpc, rmnutil.OCMBackupLabelKey, rmnutil.OCMBackupLabelValue) {
modified = true
}

update = rmnutil.AddLabel(drpc, rmnutil.OCMBackupLabelKey, rmnutil.OCMBackupLabelValue)
update = rmnutil.AddFinalizer(drpc, DRPCFinalizer) || update
if rmnutil.AddFinalizer(drpc, DRPCFinalizer) {
modified = true
}

vrgNamespace, err := selectVRGNamespace(r.Client, r.Log, drpc, placementObj)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OdedViner , maybe we can selectVRGNamespace and only then add finalizer and label?

if err != nil {
return err
return modified, err
8000 }

update = rmnutil.AddAnnotation(drpc, DRPCAppNamespace, vrgNamespace) || update

if update {
if err := r.Update(ctx, drpc); err != nil {
log.Error(err, "Failed to add annotations, labels, or finalizer to drpc")

return fmt.Errorf("%w", err)
}
if rmnutil.AddAnnotation(drpc, DRPCAppNamespace, vrgNamespace) {
modified = true
}

// add finalizer to User PlacementRule/Placement
finalizerAdded := rmnutil.AddFinalizer(placementObj, DRPCFinalizer)
if finalizerAdded {
if err := r.Update(ctx, placementObj); err != nil {
log.Error(err, "Failed to add finalizer to user placement rule")

return fmt.Errorf("%w", err)
}
}
return modified, nil
}

return nil
// updatePlacementMetadata updates placement object finalizer only
func (r DRPlacementControlReconciler) updatePlacementMetadata(
placementObj client.Object,
) bool {
return rmnutil.AddFinalizer(placementObj, DRPCFinalizer)
}

func (r *DRPlacementControlReconciler) processDeletion(ctx context.Context,
Expand Down Expand Up @@ -854,15 +850,74 @@ func (r *DRPlacementControlReconciler) updateAndSetOwner(
usrPlacement client.Object,
log logr.Logger,
) (bool, error) {
if err := r.annotateObject(ctx, drpc, usrPlacement, log); err != nil {
var (
err error
ownerReferenceDRPCModified bool
placementModified bool
placementAnnotateModified bool
placementMetadataModified bool
)

// Annotate the placement object
if placementAnnotateModified, err = r.annotateObject(drpc, usrPlacement, log); err != nil {
return false, err
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Update object metadata
if _, err = r.updateDRPCMetadata(drpc, usrPlacement); err != nil {
return false, err
}

placementMetadataModified = r.updatePlacementMetadata(usrPlacement)

// Merge both modifications
placementModified = placementAnnotateModified || placementMetadataModified
Copy link
Member
@ELENAGER ELENAGER Mar 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
		}
	}


// Set DRPC Owner
ownerReferenceDRPCModified, err = r.setDRPCOwner(drpc, usrPlacement, log)
if err != nil {
return false, err
}

if err := r.updateObjectMetadata(ctx, drpc, usrPlacement, log); err != nil {
if placementModified {
if err = r.updatePlacement(ctx, usrPlacement, log); err != nil {
return false, err
}
}

if err = r.updateDRPC(ctx, drpc, log); err != nil {
return false, err
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to check if the drpc needs an update?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DRPC CR updates every time without any condition.

ramen/internal/controller/drplacementcontrol_controller.go

Line 1042 in c975f8c

err = r.Update(ctx, drpc)


return r.setDRPCOwner(ctx, drpc, usrPlacement, log)
return ownerReferenceDRPCModified, nil
}

func (r *DRPlacementControlReconciler) updatePlacement(
ctx context.Context,
usrPlacement client.Object,
log logr.Logger,
) error {
if err := r.Update(ctx, usrPlacement); err != nil {
return fmt.Errorf("failed to update placement %s/%s: %w", usrPlacement.GetNamespace(), usrPlacement.GetName(), err)
}

log.Info("Updated placement", "placement", usrPlacement.GetName())

return nil
}

func (r *DRPlacementControlReconciler) updateDRPC(
ctx context.Context,
drpc *rmn.DRPlacementControl,
log logr.Logger,
) error {
if err := r.Update(ctx, drpc); err != nil {
return fmt.Errorf("failed to update DRPC %s: %w", drpc.GetName(), err)
}

log.Info("Successfully updated DRPC resource", "drpc", drpc.GetName())

return nil
}

func getPlacementOrPlacementRule(
Expand Down Expand Up @@ -984,11 +1039,13 @@ func getPlacement(ctx context.Context, k8sclient client.Client,
return usrPlmnt, nil
}

func (r *DRPlacementControlReconciler) annotateObject(ctx context.Context,
drpc *rmn.DRPlacementControl, obj client.Object, log logr.Logger,
) error {
func (r *DRPlacementControlReconciler) annotateObject(
drpc *rmn.DRPlacementControl,
obj client.Object,
log logr.Logger,
) (bool, error) {
if rmnutil.ResourceIsDeleted(obj) {
return nil
return false, nil
}

if obj.GetAnnotations() == nil {
Expand All @@ -1002,51 +1059,38 @@ func (r *DRPlacementControlReconciler) annotateObject(ctx context.Context,
obj.GetAnnotations()[DRPCNameAnnotation] = drpc.Name
obj.GetAnnotations()[DRPCNamespaceAnnotation] = drpc.Namespace

err := r.Update(ctx, obj)
if err != nil {
log.Error(err, "Failed to update Object annotation", "objName", obj.GetName())

return fmt.Errorf("failed to update Object %s annotation '%s/%s' (%w)",
obj.GetName(), DRPCNameAnnotation, drpc.Name, err)
}

return nil
return true, nil
}

if ownerName != drpc.Name || ownerNamespace != drpc.Namespace {
log.Info("Object not owned by this DRPC", "objName", obj.GetName())

return fmt.Errorf("object %s not owned by this DRPC '%s/%s'",
return false, fmt.Errorf("object %s not owned by this DRPC '%s/%s'",
obj.GetName(), drpc.Name, drpc.Namespace)
}

return nil
return false, nil
}

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
const needUpdate = true

for _, ownerReference := range drpc.GetOwnerReferences() {
if ownerReference.Name == owner.GetName() {
return !updated, nil // ownerreference already set
return !needUpdate, nil // ownerreference already set
}
}

err := ctrl.SetControllerReference(owner, drpc, r.Client.Scheme())
if err != nil {
return !updated, fmt.Errorf("failed to set DRPC owner %w", err)
}

err = r.Update(ctx, drpc)
if err != nil {
return !updated, fmt.Errorf("failed to update drpc %s (%w)", drpc.GetName(), err)
return !needUpdate, fmt.Errorf("failed to set DRPC owner %w", err)
}

log.Info(fmt.Sprintf("Object %s owns DRPC %s", owner.GetName(), drpc.GetName()))

return updated, nil
return needUpdate, nil
}

func (r *DRPlacementControlReconciler) getOrClonePlacementRule(ctx context.Context,
Expand Down
0