-
Notifications
You must be signed in to change notification settings - Fork 257
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
rbd: adds support for librbd rbd_resize2 #934
Conversation
9fe8c70
to
4e6b796
Compare
I won't insist on it, but I'd like to suggest having the new function and test in new files. rbd.go is already fairly long. Having a dedicated file (and test!) focused on one or a few related things aids readability IMO. |
Thanks for the recommendation @phlogistonjohn ! Implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@khayyamsaleem Thanks for the contribution. Please only add the new functions to a separate file. This is necessary, because the new API will be in preview mode first, which requires the build constraint //go:build ceph_preview
at the top of the file (and also the according test file). Please see here for more information: https://github.com/ceph/go-ceph/blob/master/docs/development.md#documentation-conventions
5373d70
to
15357e3
Compare
@phlogistonjohn @ansiwen should be ready for another round of review; thanks so much for your attention on this PR! |
Looking better, I've restarted the CI. There are two lingering issues that I think we should deal with: first, there are still changes to rbd.go, related to the changing of the comment block, that are now unnecessary. This may have been done by an IDE or the like but I'd prefer they remain unchanged by your PR. It's just "noise" when looking through the history of the project. Second, and hopefully less work, is that it would be nice to squash your two commits together. We try to keep a long term history free of intermediate changes. Let us (me) know if there are any questions on how to go about doing either one, and I'll try to help. |
15357e3
to
8080545
Compare
… controlling whether or not the allocation should be allowed to shrink, and a progress-tracking callback. This contribution extends go-ceph to be able to call rbd_resize2. Closes ceph#933 Signed-off-by: Khayyam Saleem <[email protected]>
8080545
to
f4969df
Compare
Apologies for the slow turnaround here; this had fallen off my radar. Have rebased and squashed the commits and dropped the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, thanks so much.
@ansiwen looks like we'll need your additional review to push this through. Thanks again for helping get this PR in order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
librbd exposes rbd_resize2, which allows clients to pass in a boolean controlling
whether or not the allocation should be allowed to shrink, and a progress-tracking
callback. This contribution extends go-ceph to be able to call rbd_resize2.
Closes #933
Signed-off-by: Khayyam Saleem [email protected]
Checklist
//go:build ceph_preview
make api-update
to record new APIsNew or infrequent contributors may want to review the go-ceph Developer's Guide including the section on how we track API Status and the API Stability Plan.
The go-ceph project uses mergify. View the mergify command guide for information on how to interact with mergify. Add a comment with
@Mergifyio
rebase
to rebase your PR when github indicates that the PR is out of date with the base branch.