Reconcile related drpcs when drcluster is deleted #1168
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- When filtering update events, consider also the update when the new object is marked for deletion. - When filtering drpcs consider all drpcs referencing a deleted drcluster. With this change, when a drcluster is deleted, the drpc controller updates the VRG ManifestWork on the remaining cluster. Previously this happened minutes after a drcluster was deleted. Notes: - We don't get a delete event when the drcluster is deleted, but an update event. I don't know if this is a bug in controller runtime or expected behavior. Signed-off-by: Nir Soffer <nsoffer@redhat.com>
Testing with drenv
VRG diff show that minio-on-dr1 was removedInteresting events from ramen log
Timeline
Logs and resources |
Testing multiple appsDeploy and enable dr for 3 appsDeploy 3 apps and relocate if need so all run on clsuter dr1. State before simulating disaster: Simulate disasterFailover all appsThe failover will timeout waiting for PeerReady condition - expected State after all apps failed over (Available condition met): Dump VRG before deleting the drclusterDelete drclusterDump VRG after deleting the drclusterVRGs diff: Ramen hub logs: Filtering DRPCs during update event with deleted drcluster: Updating the VRG manifest work Disabling DR for all appsState after disabling dr: Logs and resources |
|
Tested Nir's ramen image (quay.io/nirsof/ramen-operator:update-vrg-v1) today with fix described in this PR using downstream ODF 4.15 build 112, OCP 4.14.6, ACM 2.9.1. Log collected for ramen hub pod ~10-15 mins after drcluster perf2 deleted. |
| // 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ShyamsundarR
approved these changes
Jan 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With this change, when a drcluster is deleted, the drpc controller should update the VRG ManifestWork on the remaining cluster. Previously this happened minutes after a drcluster was deleted.
Testing:
Status: