Skip to content
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

Reconcile related drpcs when drcluster is deleted #1168

Merged
merged 1 commit into from
Jan 12, 2024
Merged

Reconcile related drpcs when drcluster is deleted #1168

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
86 changes: 83 additions & 3 deletions controllers/drplacementcontrol_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@ func DRClusterPredicateFunc() predicate.Funcs {
// DRClusterUpdateOfInterest checks if the new DRCluster resource as compared to the older version
// requires any attention, it checks for the following updates:
// - If any maintenance mode is reported as activated
// - If drcluster was marked for deletion
//
// TODO: Needs some logs for easier troubleshooting
func DRClusterUpdateOfInterest(oldDRCluster, newDRCluster *rmn.DRCluster) bool {
Expand All @@ -303,8 +304,8 @@ func DRClusterUpdateOfInterest(oldDRCluster, newDRCluster *rmn.DRCluster) bool {
}
}

// Exhausted all failover activation checks, this update is NOT of interest
return false
// Exhausted all failover activation checks, the only interesting update is deleting a drcluster.
return drClusterIsDeleted(newDRCluster)
Copy link
Member

Choose a reason for hiding this comment

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

Unsure if a delete of a resource is causing an Update event to trigger as well. I would have just returned true from line 269 above, which is where the predicate function called when the watched resource is deleted.

Copy link
Member

@BenamarMk BenamarMk Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with Shyam. You are better off returning true on line 269 and allowing all drpcs to reconcile. Deleting a drcluster should NOT be a common use case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I started this change by returning true in line 269, but unfortunately this does not work.

Deleting creates an update event, when the new object has a deletionTimeStamp. When the object is actually removed from the system, we get a delete event.

In our case this event is not relevant. It will happen when the drpolicy is deleted the the ramen removes the finalizers from the the drcluster.

Copy link
Member

@BenamarMk BenamarMk Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see, so adding the deletionTimeStamp ends up firing for Updated. Got it.
So how about if in line 307, you return true every time the objectNew (newDRCluster) has a non-zero deletionTimeStamp?
I think avoiding adding the new code (DRPCsUsingDRCluster and DRPCsUsingDRPolicy) is desirable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not try the change without modifying FilterDRCluster - but if we don't modify it we will trigger a reconcile only in the DRPCs which are failing over to the deleted cluster, which is always no DRPC.

We an simplify by returning all DPPCS but if we have to add code it makes sense to add the right code which is only few more lines.

}

// checkFailoverActivation checks if provided provisioner and storage instance is activated as per the
Expand Down Expand Up @@ -347,7 +348,16 @@ func getFailoverActivatedCondition(mMode rmn.ClusterMaintenanceMode) *metav1.Con
func (r *DRPlacementControlReconciler) FilterDRCluster(drcluster *rmn.DRCluster) []ctrl.Request {
log := ctrl.Log.WithName("DRPCFilter").WithName("DRCluster").WithValues("cluster", drcluster)

drpcCollections, err := DRPCsFailingOverToCluster(r.Client, log, drcluster.GetName())
var drpcCollections []DRPCAndPolicy

var err error

if drClusterIsDeleted(drcluster) {
drpcCollections, err = DRPCsUsingDRCluster(r.Client, log, drcluster)
} else {
drpcCollections, err = DRPCsFailingOverToCluster(r.Client, log, drcluster.GetName())
}

if err != nil {
log.Info("Failed to process filter")

Expand All @@ -373,6 +383,76 @@ type DRPCAndPolicy struct {
drPolicy *rmn.DRPolicy
}

// DRPCsUsingDRCluster finds DRPC resources using the DRcluster.
func DRPCsUsingDRCluster(k8sclient client.Client, log logr.Logger, drcluster *rmn.DRCluster) ([]DRPCAndPolicy, error) {
drpolicies := &rmn.DRPolicyList{}
if err := k8sclient.List(context.TODO(), drpolicies); err != nil {
log.Error(err, "Failed to list DRPolicies", "drcluster", drcluster.GetName())

return nil, err
}

found := []DRPCAndPolicy{}

for i := range drpolicies.Items {
drpolicy := &drpolicies.Items[i]

for _, clusterName := range drpolicy.Spec.DRClusters {
if clusterName != drcluster.GetName() {
continue
}

log.Info("Found DRPolicy referencing DRCluster", "drpolicy", drpolicy.GetName())

drpcs, err := DRPCsUsingDRPolicy(k8sclient, log, drpolicy)
if err != nil {
return nil, err
}

for _, drpc := range drpcs {
found = append(found, DRPCAndPolicy{drpc: drpc, drPolicy: drpolicy})
}

break
}
}

return found, nil
}

// DRPCsUsingDRPolicy finds DRPC resources that reference the DRPolicy.
func DRPCsUsingDRPolicy(
k8sclient client.Client,
log logr.Logger,
drpolicy *rmn.DRPolicy,
) ([]*rmn.DRPlacementControl, error) {
drpcs := &rmn.DRPlacementControlList{}
if err := k8sclient.List(context.TODO(), drpcs); err != nil {
log.Error(err, "Failed to list DRPCs", "drpolicy", drpolicy.GetName())

return nil, err
}

found := []*rmn.DRPlacementControl{}

for i := range drpcs.Items {
drpc := &drpcs.Items[i]

if drpc.Spec.DRPolicyRef.Name != drpolicy.GetName() {
continue
}

log.Info("Found DRPC referencing drpolicy",
"name", drpc.GetName(),
"namespace", drpc.GetNamespace(),
"drpolicy", drpolicy.GetName())

found = append(found, drpc)
}

return found, nil
}

// DRPCsFailingOverToCluster lists DRPC resources that are failing over to the passed in drcluster
//
//nolint:gocognit
Expand Down