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

hammer: rbd: snap rollback: restore the link to parent #8535

Merged
1 commit merged into from May 16, 2016

Conversation

asheplyakov
Copy link

@asheplyakov asheplyakov commented Apr 11, 2016

const SnapInfo& si(it->second);
// current parent does not match the one of the snapshot being reverting to
bool should_update_parent = ictx->parent_md.spec != si.parent.spec ||
ictx->parent_md.overlap != si.parent.overlap;

Choose a reason for hiding this comment

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

If the overlap changes (but parent record still exists), the cls set_parent method will return -EEXISTS when trying to update the parent record. Therefore, you will need to remove_parent first. Also need to support the case where HEAD is associated with a parent but the rollback snapshot doesn't have a parent.

@asheplyakov
Copy link
Author

@dillaman

If the overlap changes (but parent record still exists), the cls set_parent method will return -EEXISTS when trying to update the parent record. Therefore, you will need to remove_parent first.

Done.

Also need to support the case where HEAD is associated with a parent but the rollback snapshot doesn't have a parent.

Done.

Can you move this test to test_rbd.py?

Done.

Probably enough to just verify parent spec after rollback.

I've added parent info checks, however think it's important to verify if the actual data matches the expected one.

Would also be good to add tests for various combinations of updates (no parent->parent, parent->no parent, overlap size change, etc).

I've implemented no parent -> parent, parent -> no parent tests, however I don't quite understand which operations can change the overlap size. Any hints, please?

@dillaman
Copy link

@asheplyakov The only way to change the overlap size is to shrink the image. So if you resize the image down past the original parent size, the overlap is now smaller.

@asheplyakov
Copy link
Author

@dillaman

The only way to change the overlap size is to shrink the image.

I've added a test which verifies that rollback handles such a modification. Does the patch look OK now?

CephContext *cct = ictx->cct;
int r = 0;
std::map<librados::snap_t, SnapInfo>::const_iterator it = ictx->snap_info.find(snap_id);
assert(it != ictx->snap_info.end());

Choose a reason for hiding this comment

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

Probably sufficient to just return -ENOENT in this case.

Copy link
Author

Choose a reason for hiding this comment

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

Done

So snapshot, flatten, rollback of a cloned image does not loose any data

Fixes: ceph#14512
Signed-off-by: Alexey Sheplyakov <asheplyakov@mirantis.com>
@dillaman
Copy link

lgtm

@dillaman
Copy link

👍 -- good to merge once 0.94.7 is released.

@ghost ghost merged commit b90c097 into ceph:hammer May 16, 2016
@ghost ghost assigned dillaman May 16, 2016
@smithfarm smithfarm changed the title hammer: rbd snap rollback: restore the link to parent hammer: rbd: snap rollback: restore the link to parent Aug 15, 2016
@asheplyakov asheplyakov deleted the bug-14512-hammer branch November 24, 2016 12:37
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants