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

Snapshot checksum APIs #35

Open
wants to merge 8 commits into
base: longhorn-v24.05
Choose a base branch
from

Conversation

DamiaSan
Copy link

Which issue(s) this PR fixes:

Issue #8666

What this PR does / why we need it:

Special notes for your reviewer:

Additional documentation or context

Added a new function which generate CRC from ISO standard in
reflected format.

Longhorn 8666

Signed-off-by: Damiano Cipriani <[email protected]>
A new flag has been added to blob metadata to enable the addition
of new xattrs to a read only blob.

Longhorn 8666

Signed-off-by: Damiano Cipriani <[email protected]>
Added a new function to create a snapshot with options.

Longhorn 8666

Signed-off-by: Damiano Cipriani <[email protected]>
Added a new function to compute sanpshot's data checksum and to
store it as an xattr.

Longhorn 8666

Signed-off-by: Damiano Cipriani <[email protected]>
Added a new function to create a snapshot with provided options,
like a list of xattrs to be set in the snapshot and an option
to enable the addition of new xattrs to the snapshot after its
creation.

Longhorn 8666

Signed-off-by: Damiano Cipriani <[email protected]>
Added an option in snapshot create to enable the addition of new
xattrs to a snapshot after its creation. The status of the option
is shown in dump_info_json callback.

Longhorn 8666

Signed-off-by: Damiano Cipriani <[email protected]>
Added functions to register and get the checksum of snapshot's data.

Longhorn 8666

Signed-off-by: Damiano Cipriani <[email protected]>
Added the RPC interfaces to register and to get snapshot's data
checksum. Also a lvol test have been added to check the behavior
of the new RPCs.

Longhorn 8666

Signed-off-by: Damiano Cipriani <[email protected]>
@derekbit
Copy link
Member

Need to update go-spdk-helper as well.

Copy link

@shuo-wu shuo-wu left a comment

Choose a reason for hiding this comment

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

I have one question:
After deleting a parent snapshot, IIUC, its clusters should be squashed into the child snapshot. Then, do we need to manually re-calculate the child snapshot checksum?

Also, is this API a sync call? If YES, I am worried that calculating a huge snapshot will block the caller for a long time.

@DamiaSan
Copy link
Author

DamiaSan commented Nov 13, 2024

I have one question: After deleting a parent snapshot, IIUC, its clusters should be squashed into the child snapshot. Then, do we need to manually re-calculate the child snapshot checksum?

Yes, but if the child snapshot has already a checksum stored, actually we would get an error. What we could do is to add a force option to overwrite the old checksum.

Also, is this API a sync call? If YES, I am worried that calculating a huge snapshot will block the caller for a long time.

Yes, it is a sync call. We could make it async, and to test the end of the operation we can use the RPC to retrieve the checksum: until the calculation is not ended, we would get an error of xattr not present.

@shuo-wu
Copy link

shuo-wu commented Nov 13, 2024

but if the child snapshot has already a checksum stored, actually we would get an error.

Which code guarantees this kind of validity of the snapshot checksum?

We could make it async

Do you mean that the caller can open a separate thread to call this API then retrieve if the checksum presents or not?

@DamiaSan
Copy link
Author

Which code guarantees this kind of validity of the snapshot checksum?

Actually no code guarantee the validity of snapshot's checksum. What I mean is that if a snapshot has already the snapshot checksum xattr and we try to calculate again the checksum we will get an error.

Do you mean that the caller can open a separate thread to call this API then retrieve if the checksum presents or not?

Exactly

@shuo-wu
Copy link

shuo-wu commented Nov 14, 2024

What I mean is that if a snapshot has already the snapshot checksum xattr and we try to calculate again the checksum we will get an error.

Then we need a way to handle this case. Otherwise, this feature may be meaningless, as we cannot prevent users from deleting snapshots hence we never know the snapshot checksum recorded in xattrs is still valid or not.

WDYT? @derekbit @innobead

@DamiaSan
Copy link
Author

What we could do is to add a force option to overwrite the old checksum.

A possible solution.

@shuo-wu
Copy link

shuo-wu commented Nov 19, 2024

What we could do is to add a force option to overwrite the old checksum.

A possible solution.

Will you implement it in this PR?

@DamiaSan
Copy link
Author

What we could do is to add a force option to overwrite the old checksum.

A possible solution.

Will you implement it in this PR?

Yes, it will the first thing to do after the HackWeek.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants