Skip to content

Conversation

@smengcl
Copy link
Contributor

@smengcl smengcl commented Jan 13, 2023

What changes were proposed in this pull request?

  • Implement CLI ozone sh snapshot delete.
  • Implement Ratis Tx OMSnapshotDeleteRequest.
  • Improve CLI output for UX.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6857

How was this patch tested?

  • Add UT TestOMSnapshotDeleteRequest.
  • Finish UT TestOMSnapshotDeleteResponse. (Done in HDDS-7862 instead)
  • Tested manually in Docker locally:
$ ozone fs -mkdir -p ofs://om/vol1/buck1/dir1/
$ ozone fs -put README.md ofs://om/vol1/buck1/dir1/
$ ozone sh snapshot create /vol1/buck1 snap1
$ ozone sh snapshot delete /vol1/buck1 snap1
$ ozone sh snapshot delete /vol1/buck1 snap1
FILE_NOT_FOUND Snapshot exists but no longer active
$ ozone sh snapshot list /vol1/buck1
[ {
  "volumeName" : "vol1",
  "bucketName" : "buck1",
  "name" : "snap1",
  "creationTime" : 1673583279047,
  "snapshotStatus" : "SNAPSHOT_DELETED",
  "snapshotID" : "08f640e3-95db-4596-9d5f-a47cb7272329",
  "snapshotPath" : "vol1/buck1",
  "checkpointDir" : "-08f640e3-95db-4596-9d5f-a47cb7272329"
} ]

Change-Id: I06724459fc4e472df952ee2d3255bf9fdf1e3513
…s well for consistency.

Change-Id: I6bdb64f992121ef6815eeeb0631e2ba5df142435
Change-Id: I6b1a4a69bec12ccbc2524c4cc7eb3127782902f6
@smengcl smengcl added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Jan 13, 2023
@smengcl
Copy link
Contributor Author

smengcl commented Jan 13, 2023

@GeorgeJahad @hemanthboyina @aswinshakil Please also take a look.

Some of the dead code commented out is intentional in this draft to facilitate discussion. e.g. whether to keep the BUCKET_LOCK or not in OMSnapshotDeleteRequest. They will be cleaned up later.

@neils-dev
Copy link
Contributor

With the snapshot delete cli command is it possible to indicate to the user the amount of space that will be freed with the operation? If not here, then through another method.

$ ozone sh snapshot list /vol1/buck1
[ {
  "volumeName" : "vol1",
  "bucketName" : "buck1",
  "name" : "snap1",
  "creationTime" : 1673583279047,
  "snapshotStatus" : "SNAPSHOT_DELETED",
  "snapshotID" : "08f640e3-95db-4596-9d5f-a47cb7272329",
  "snapshotPath" : "vol1/buck1",
  "checkpointDir" : "-08f640e3-95db-4596-9d5f-a47cb7272329"
} ]

In the example provided, after executing the delete, listing the snapshots (shoiwn above) for the bucket shows the "SNAPSHOT_DELETED" state. With the current example it appears that listing snapshots for the bucket always shows a snapshot in deleted state. We expect it to be removed from the listing at some point? After garbage collection?

if (!snapshotInfo.getSnapshotStatus().equals(
SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE)) {
// If the snapshot is not in active state, throw exception as well
throw new OMException("Snapshot exists but no longer active",
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps "Snapshot pending deletion".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Snapshot pending deletion" would only apply to SNAPSHOT_DELETED.

I have detailed the snapshot status a bit. Please take another look.

Copy link
Member

@aswinshakil aswinshakil 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 working on this @smengcl. I have some minor comments inline.

@smengcl
Copy link
Contributor Author

smengcl commented Jan 26, 2023

With the snapshot delete cli command is it possible to indicate to the user the amount of space that will be freed with the operation? If not here, then through another method.

Hi Neil, we do have snapshot disk usage display planned in HDDS-7744 and has been detailed in the design doc.

$ ozone sh snapshot list /vol1/buck1
[ {
  "volumeName" : "vol1",
  "bucketName" : "buck1",
  "name" : "snap1",
  "creationTime" : 1673583279047,
  "snapshotStatus" : "SNAPSHOT_DELETED",
  "snapshotID" : "08f640e3-95db-4596-9d5f-a47cb7272329",
  "snapshotPath" : "vol1/buck1",
  "checkpointDir" : "-08f640e3-95db-4596-9d5f-a47cb7272329"
} ]

In the example provided, after executing the delete, listing the snapshots (shoiwn above) for the bucket shows the "SNAPSHOT_DELETED" state. With the current example it appears that listing snapshots for the bucket always shows a snapshot in deleted state. We expect it to be removed from the listing at some point? After garbage collection?

It will be cleaned up and removed by SnapshotDeletingTask later, being implemented in HDDS-7740.

@smengcl smengcl marked this pull request as ready for review January 31, 2023 19:13
Change-Id: I233511813814f78128c2918bd5db18ce16217144
Change-Id: I4ade694b7c4443ac8ea703c7dfa372720a89b98d
Copy link
Contributor

@prashantpogde prashantpogde left a comment

Choose a reason for hiding this comment

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

LGTM once all the CI tests pass.

Change-Id: Ibf839375697f62f019b3260e3820752fa7a7bf1a
@smengcl
Copy link
Contributor Author

smengcl commented Feb 1, 2023

Thanks @prashantpogde for the +1. CI passed. I will merge this to unblock other GC work.

I've filed HDDS-7862 for the TestOMSnapshotDeleteResponse UT TODO.

Thanks @hemantk-12 @aswinshakil @neils-dev for the reviews.

@smengcl smengcl merged commit ccc814e into apache:HDDS-6517-Snapshot Feb 1, 2023
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.

5 participants