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.

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

HDDS-10524. [Snapshot] Invalidate the cache entry from snapshotInfoTable cache in OMSnapshotPurgeRequest #6443

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

hemantk-12
Copy link
Contributor

@hemantk-12 hemantk-12 commented Mar 26, 2024

What changes were proposed in this pull request?

Currently when snapshot is purged, it is not removed from the snapshotInfoTable cache. Because of which it is possible that a purged snapshot is added by SnapshotSetProperty API.

KeyDeletingService and SnapshotDeletingService are two background service which run in parallel.
1). KeyDeletingService updates deep clean flag and exclusive size for a snapshot.
2). SnapshotDeletingService purges a deleted snapshot.

KeyDeletionService will try to mark a snapshot deep clean here: code
Since it is in a loop, it is possible when KeyDeletingService is going over all the snapshot. Meanwhile user deletes the snapshot, SnapshotDeletingService kicks in and purge the snapshot code here.
Now when OMSnapshotSetPropertyRequest gets request to update deep clean for the same snapshot (purged by SnapshotDeletingService), it gets the value from cache because we don’t purge snapshot from cache on the purge path and wait for double buffer flush.
Now there are two transaction in double buffer one for purge and other for updating deep clean. Some time snapshot can be present in the cache and sometime it doesn’t. When it has snapshot in cache, it causes snapshot chain corruption because both of the transaction will succeed.

In this change, we are invalidating the cache entry in OMSnapshotPurgeRequest so that other APIs don't see the snapshot.

What is the link to the Apache JIRA

HDDS-10524

How was this patch tested?

  1. Wrote a test to mimic the same behavior as in described in the details. This test was run against existing code and with fix. It failed all the time in the existing code and passed in fixed code.

  2. Also updated existing unit test to validate before and after snapshot purge transaction is committed.

@hemantk-12 hemantk-12 changed the title HDDS-10524. [Snapshot] Snapshot chain fix HDDS-10524. [Snapshot] Invalidate the cache entry from snapshotInfoTable cache in OMSnapshotPurgeRequest Mar 26, 2024
Copy link
Contributor

@swamirishi swamirishi left a comment

Choose a reason for hiding this comment

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

Thanks for figuring out the issue and raising a patch for the same @hemantk-12

@adoroszlai adoroszlai added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Mar 26, 2024
Copy link
Contributor

@swamirishi swamirishi left a comment

Choose a reason for hiding this comment

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

LGTM

@hemantk-12 hemantk-12 merged commit 7559e1f into apache:master Mar 27, 2024
36 checks passed
@hemantk-12
Copy link
Contributor Author

Thanks @swamirishi for the review.

myskov pushed a commit to myskov/ozone that referenced this pull request Apr 4, 2024
…ble cache in OMSnapshotPurgeRequest (apache#6443)

(cherry picked from commit 7559e1f)
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request May 29, 2024
…ble cache in OMSnapshotPurgeRequest (apache#6443)

(cherry picked from commit 7559e1f)
swamirishi pushed a commit to swamirishi/ozone that referenced this pull request Jun 10, 2024
…apshotInfoTable cache in OMSnapshotPurgeRequest (apache#6443)

Change-Id: I96831f1f18f092950a2fbf257571d7a587e13f8a
@hemantk-12 hemantk-12 deleted the HDDS-10524 branch October 28, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
snapshot https://issues.apache.org/jira/browse/HDDS-6517
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants