rbd: add mirroring support for nautilus #714

Sanford137 · 2022-06-24T02:33:01Z

I've implemented mirroring support that is compatible with Nautilus clusters. It's a fairly different API from the one exposed for Octopus, since the underlying librbd interface changed so much between them.

It needs to be clear in the documentation that these methods are only available when using the nautilus build tag. One way to do that is just mention it in the documentation comments for all of them (not very DRY, but simple), another is to add a new section to the API docs, similar to the existing "preview" section, which would box off those methods into their own category. What approach would you like to take?

Checklist

Added tests for features and functional changes
Public functions and types are documented
Standard formatting is applied to Go code
Is this a new API? Added a new file that begins with //go:build ceph_preview

phlogistonjohn · 2022-06-24T13:24:50Z

I've implemented mirroring support that is compatible with Nautilus clusters. It's a fairly different API from the one exposed for Octopus, since the underlying librbd interface changed so much between them.

It needs to be clear in the documentation that these methods are only available when using the nautilus build tag. One way to do that is just mention it in the documentation comments for all of them (not very DRY, but simple), another is to add a new section to the API docs, similar to the existing "preview" section, which would box off those methods into their own category. What approach would you like to take?

Because this is a "backwards" looking feature I think the doc comment approach is best. I'd rather not make changes to the tooling to support a ceph release that's already deprecated.

I appreciate that you use nautilus and want to use go-ceph, and that makes me quite pleased, but looking at the changes... Gee that's a lot of net new stuff for a release I had planned on dropping support for.
Looking at https://docs.ceph.com/en/latest/releases/#ceph-releases-index I see that Nautilus has been EOL at ceph for nearly a year already and that octopus is near or at EOL too. Now, that this isn't an outright rejection of your patches, I just want to communicate some of my concerns about taking large additions that only work for nautilus. I'm going to certainly think about it and discuss it with others on the maintenance team - I hope you understand.

In the meantime there's some CI errors related to your patches that need looking into (make check).

Sanford137 · 2022-06-24T18:48:03Z

CI should be fixed now - the one remaining failing test is a flaky test in cephfs/admin, I think.

I get your concerns about adding a big chunk of Nautilus-specific code, and that you want to discuss it with the other maintainers. If you need some time to decide, no rush!

I will say that although Nautilus is EOL, it is only 3 years old, and it remains in decently widespread use - and some organizations are still on even earlier versions! As an example, just a few days ago someone submitted a large patch to to the Mimic branch of the ceph_exporter tool.

My company (DigitalOcean) is moving towards Octopus and eventually Pacific, but it will be at least a year before all of our clusters are fully switched over; upgrading is slow when you are running many clusters, and large ones, as we are (a problem that I suspect other organizations that are heavy users of Ceph are also running into).

In the meantime, me and my team are happy to be responsible for maintaining any Nautilus-specific code (and happy to work out any logistics with you on how best to get Nautilus-related bugs and requests routed to us).

ansiwen · 2022-06-29T05:11:16Z

The documentation would not show up anyway AFAIK, because the doc generator doesn't set any tags. So I would also prefer that this doesn't go in the preview section but is completely "hidden" and disappears automatically whenever we have to delete the nautilus files, because something breaks. I don't mind adding "hidden" support for features of old versions, but I think it should not show up in docs or the api status. It must be "zero-cost" for the management of the current, supported versions. So as long as all the code stays in "nautilus-guarded" files, I'm ok. And of cause, no guarantees, it comes "as-is".

rbd/mirror_nautilus_test.go

entrypoint.sh

phlogistonjohn · 2022-06-29T13:25:37Z

Great, so it sounds like we'll take this (modulo review feedback, updates, etc) given that the majority of the maintenance costs will fall on @Sanford137 et al. At some point in the future we'll remove nautilus support but not until after the digital ocean team confirms it's no longer a must have. It would be nice to have an approximate date, emphasis on approximate, so that we can update issue #648
Anything else on the "management" front for this or is the rest just working thru the details of this PR?

mergify · 2022-06-30T08:13:49Z

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

Sanford137 · 2022-07-01T01:40:08Z

I'm happy to hear you've decided to go ahead with the patch! I think I've addressed all of your feedback, let me know if there is anything else that needs fixing up.

entrypoint.sh

ansiwen

partial review

rbd/mirror_nautilus.go

Sanford137 · 2022-07-07T14:12:40Z

Is there a way for me to re-run a test failure that I think is a flake?

Also, would you like me to close conversations once I've addressed the comment? This page is getting a bit unwieldy, but I don't want to accidentally close anything that you still want to talk about.

phlogistonjohn · 2022-07-07T14:20:00Z

Is there a way for me to re-run a test failure that I think is a flake?

I don't know if you can see the re-run jobs button on the page https://github.com/ceph/go-ceph/actions/runs/2629992092 (and the like). As an admin I'm not sure what standard contributors see.
Either way, just asking will (eventually) usually get the job done. :-D

Also, would you like me to close conversations once I've addressed the comment? This page is getting a bit unwieldy, but I don't want to accidentally close anything that you still want to talk about.

I've done it now. Usually I was something like 1 day for the original author of the sub-thread to close it and if it goes past that time I usually feel free to close them. It's not hard to reopen ("unresolve") one so I personally don't think its much of an issue.

Sanford137 · 2022-07-07T14:25:41Z

I cannot see the re-run button - will just ask for now!

rbd/mirror_nautilus.go

Sanford137 · 2022-07-11T14:14:12Z

Does this need any other changes before it's ready to merge?

phlogistonjohn

I have one small but necessary change to make. The Go code looks good to me at this point.

Makefile

phlogistonjohn

LGTM, thanks!

phlogistonjohn · 2022-07-11T14:40:46Z

@ansiwen PTAL when you have a moment. Thanks!

ansiwen

Finished review. Just some small things, then it's good to go!

rbd/mirror_nautilus.go

ansiwen · 2022-07-12T09:22:42Z

rbd/mirror_nautilus.go

+// Implements:
+//  int rbd_mirror_image_disable(rbd_image_t image, bool force)
+func (image *Image) MirrorDisable(force bool) error {
+	ret := C.rbd_mirror_image_disable(C.rbd_image_t(image.image), C.bool(force))


ansiwen · 2022-07-12T09:22:53Z

rbd/mirror_nautilus.go

+// Implements:
+//  int rbd_mirror_image_promote(rbd_image_t image, bool force)
+func (image *Image) MirrorPromote(force bool) error {
+	ret := C.rbd_mirror_image_promote(C.rbd_image_t(image.image), C.bool(force))


ansiwen · 2022-07-12T09:23:00Z

rbd/mirror_nautilus.go

+// Implements:
+//  int rbd_mirror_image_demote(rbd_image_t image)
+func (image *Image) MirrorDemote() error {
+	ret := C.rbd_mirror_image_demote(C.rbd_image_t(image.image))


ansiwen · 2022-07-12T09:23:09Z

rbd/mirror_nautilus.go

+// Implements:
+//  int rbd_mirror_image_resync(rbd_image_t image)
+func (image *Image) MirrorResync() error {
+	ret := C.rbd_mirror_image_resync(C.rbd_image_t(image.image))


Pull request has been modified.

The librbd API for mirroring-related operations changed substantially between Nautilus and Octopus. Due to this, go-ceph had previously only implemented mirroring functionality if built against Octopus client libraries. This patch implements equivalent functionality for use with Nautilus clients. Signed-off-by: Sanford Miller <[email protected]>

Sanford137 · 2022-07-12T15:14:46Z

Ready for another look!

ansiwen

LGTM, thanks!

phlogistonjohn

LGTM, thanks!

Sanford137 force-pushed the rbd-mirror-nautilus-support branch from 4760455 to 5f6a6ea Compare June 24, 2022 14:45

ansiwen requested changes Jun 29, 2022

View reviewed changes

rbd/mirror_nautilus_test.go Outdated Show resolved Hide resolved

entrypoint.sh Outdated Show resolved Hide resolved

ansiwen mentioned this pull request Jun 29, 2022

Remove nautilus #648

Closed

Sanford137 force-pushed the rbd-mirror-nautilus-support branch 2 times, most recently from 6cf32e3 to 6dc159b Compare July 1, 2022 01:13

Sanford137 force-pushed the rbd-mirror-nautilus-support branch from 6dc159b to a0311d1 Compare July 5, 2022 13:40

phlogistonjohn requested changes Jul 5, 2022

View reviewed changes

entrypoint.sh Outdated Show resolved Hide resolved

phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Jul 5, 2022

Sanford137 force-pushed the rbd-mirror-nautilus-support branch 2 times, most recently from cd18fb5 to c515cf3 Compare July 5, 2022 18:07

ansiwen reviewed Jul 6, 2022

View reviewed changes

rbd/mirror_nautilus.go Outdated Show resolved Hide resolved

rbd/mirror_nautilus.go Outdated Show resolved Hide resolved

Sanford137 force-pushed the rbd-mirror-nautilus-support branch from c515cf3 to e54102f Compare July 7, 2022 13:48

phlogistonjohn reviewed Jul 7, 2022

View reviewed changes

rbd/mirror_nautilus.go Outdated Show resolved Hide resolved

Sanford137 force-pushed the rbd-mirror-nautilus-support branch 5 times, most recently from 7913da9 to 4e3ff39 Compare July 11, 2022 13:32

phlogistonjohn requested review from ansiwen and phlogistonjohn July 11, 2022 14:20

phlogistonjohn requested changes Jul 11, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Sanford137 force-pushed the rbd-mirror-nautilus-support branch from 4e3ff39 to f528855 Compare July 11, 2022 14:32

phlogistonjohn previously approved these changes Jul 11, 2022

View reviewed changes

ansiwen requested changes Jul 12, 2022

View reviewed changes

Sanford137 force-pushed the rbd-mirror-nautilus-support branch from f528855 to ded821f Compare July 12, 2022 14:45

Sanford137 force-pushed the rbd-mirror-nautilus-support branch from ded821f to 416b46d Compare July 12, 2022 14:48

ansiwen approved these changes Jul 13, 2022

View reviewed changes

ansiwen requested a review from phlogistonjohn July 13, 2022 05:03

phlogistonjohn approved these changes Jul 13, 2022

View reviewed changes

mergify bot merged commit 15d71fe into ceph:master Jul 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rbd: add mirroring support for nautilus #714

rbd: add mirroring support for nautilus #714

Sanford137 commented Jun 24, 2022 •

edited

Loading

phlogistonjohn commented Jun 24, 2022

Sanford137 commented Jun 24, 2022

ansiwen commented Jun 29, 2022 •

edited

Loading

phlogistonjohn commented Jun 29, 2022

mergify bot commented Jun 30, 2022

Sanford137 commented Jul 1, 2022

ansiwen left a comment

Sanford137 commented Jul 7, 2022

phlogistonjohn commented Jul 7, 2022 •

edited

Loading

Sanford137 commented Jul 7, 2022

Sanford137 commented Jul 11, 2022

phlogistonjohn left a comment

phlogistonjohn left a comment

phlogistonjohn commented Jul 11, 2022

ansiwen left a comment

ansiwen Jul 12, 2022

Sanford137 Jul 12, 2022

ansiwen Jul 12, 2022

Sanford137 Jul 12, 2022

ansiwen Jul 12, 2022

Sanford137 Jul 12, 2022 •

edited

Loading

ansiwen Jul 12, 2022

Sanford137 Jul 12, 2022

Sanford137 commented Jul 12, 2022

ansiwen left a comment

phlogistonjohn left a comment

rbd: add mirroring support for nautilus #714

rbd: add mirroring support for nautilus #714

Conversation

Sanford137 commented Jun 24, 2022 • edited Loading

Checklist

phlogistonjohn commented Jun 24, 2022

Sanford137 commented Jun 24, 2022

ansiwen commented Jun 29, 2022 • edited Loading

phlogistonjohn commented Jun 29, 2022

mergify bot commented Jun 30, 2022

Sanford137 commented Jul 1, 2022

ansiwen left a comment

Choose a reason for hiding this comment

Sanford137 commented Jul 7, 2022

phlogistonjohn commented Jul 7, 2022 • edited Loading

Sanford137 commented Jul 7, 2022

Sanford137 commented Jul 11, 2022

phlogistonjohn left a comment

Choose a reason for hiding this comment

phlogistonjohn left a comment

Choose a reason for hiding this comment

phlogistonjohn commented Jul 11, 2022

ansiwen left a comment

Choose a reason for hiding this comment

ansiwen Jul 12, 2022

Choose a reason for hiding this comment

Sanford137 Jul 12, 2022

Choose a reason for hiding this comment

ansiwen Jul 12, 2022

Choose a reason for hiding this comment

Sanford137 Jul 12, 2022

Choose a reason for hiding this comment

ansiwen Jul 12, 2022

Choose a reason for hiding this comment

Sanford137 Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

ansiwen Jul 12, 2022

Choose a reason for hiding this comment

Sanford137 Jul 12, 2022

Choose a reason for hiding this comment

Sanford137 commented Jul 12, 2022

ansiwen left a comment

Choose a reason for hiding this comment

phlogistonjohn left a comment

Choose a reason for hiding this comment

Sanford137 commented Jun 24, 2022 •

edited

Loading

ansiwen commented Jun 29, 2022 •

edited

Loading

phlogistonjohn commented Jul 7, 2022 •

edited

Loading

Sanford137 Jul 12, 2022 •

edited

Loading