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.

Sign up for GitHub

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

Jump to bottom

[WIP]Adding support mirror group APIs #1010

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[WIP]Adding support mirror group APIs #1010

wants to merge 1 commit into from
+642 −0

Conversation

sp98
Copy link

@sp98 sp98 commented Jul 9, 2024 •
edited by nixpanic
Loading

Add support for the Mirror Group APIs that are part of an open Ceph PR
Following APIs will be supported:

  • Mirror group enable
  • Mirror group disable
  • Mirror group promote
  • Mirror group demote
  • Mirror group resync
  • Mirror group Info
  • Mirror Group Global Status

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
  • Ran make api-update to record new APIs

New 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.

Depends-on: ceph/ceph#53793

@sp98 sp98 marked this pull request as ready for review July 9, 2024 11:37
@sp98 sp98 changed the title Adding support mirror group APIs [WIP]Adding support mirror group APIs Jul 9, 2024
sp98
sp98 commented Jul 9, 2024
View reviewed changes
rbd/group.go Outdated
@@ -110,7 +110,9 @@ func GroupImageAdd(groupIoctx *rados.IOContext, groupName string,
cephIoctx(groupIoctx),
cGroupName,
cephIoctx(imageIoctx),
cImageName)
cImageName,
C.uint32_t(0),
Copy link
Author

Choose a reason for hiding this comment

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

This change is part of the in-progress ceph PR that has mirror group APIs

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm. I don't think that's going to be an acceptable kind of change to an existing public API. It would retroactively break working code. That PR is enormous but I guess I'll have to look through it and raise that concern over there. Normally, I'd expect a new api function to be added (say "foo_bar2" or whatever).

The fact that this is still touching an existing stable API is a warning sign.

sp98
sp98 commented Jul 9, 2024
View reviewed changes
rbd/group.go Outdated
@@ -135,7 +137,8 @@ func GroupImageRemove(groupIoctx *rados.IOContext, groupName string,
cephIoctx(groupIoctx),
cGroupName,
cephIoctx(imageIoctx),
cImageName)
cImageName,
C.uint32_t(0))
Copy link
Author

Choose a reason for hiding this comment

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

This change is part of the in-progress ceph PR that has mirror group APIs

sp98
sp98 commented Jul 9, 2024
View reviewed changes
rbd/group.go Outdated
@@ -160,7 +163,8 @@ func GroupImageRemoveByID(groupIoctx *rados.IOContext, groupName string,
cephIoctx(groupIoctx),
cGroupName,
cephIoctx(imageIoctx),
cid)
cid,
C.uint32_t(0))
Copy link
Author

Choose a reason for hiding this comment

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

This change is part of the in-progress ceph PR that has mirror group APIs

@phlogistonjohn
Copy link
Collaborator

Why is this a new PR? What is going to happen with #1009?

@sp98 sp98 mentioned this pull request Jul 9, 2024
Closed
5 tasks
@sp98
Copy link
Author

sp98 commented Jul 9, 2024

Why is this a new PR? What is going to happen with #1009?

I closed that one. It was for rbd_mirror_group_get_global_status but it was not implemented in librbd.cc (due to recent name change of the API) so couldn't refer to that API in the custom ceph builds. So I have to work on that again maybe part of this PR (most likely) or a new one.

@sp98 sp98 force-pushed the mirror-group-apis branch 2 times, most recently from bc8303b to 0e5f798 Compare July 11, 2024 08:47
@sp98 sp98 changed the title [WIP]Adding support mirror group APIs Adding support mirror group APIs Jul 11, 2024
@sp98 sp98 force-pushed the mirror-group-apis branch 3 times, most recently from 0a69751 to 590037e Compare July 11, 2024 15:22
BlaineEXE
BlaineEXE reviewed Jul 11, 2024
View reviewed changes
rbd/mirror_group.go Outdated Show resolved Hide resolved
@phlogistonjohn phlogistonjohn added do-not-merge API This PR includes a change to the public API of a go-ceph package labels Jul 11, 2024
@phlogistonjohn
Copy link
Collaborator

@sp98 as a heads up I'm not going to be monitoring the content of the PR until the upstream changes settle down and are merged. Please feel free to use this PR as a playground for testing and ask specific questions as needed. I recommend moving it to draft.

@nixpanic nixpanic mentioned this pull request Jul 16, 2024
Merged
5 tasks
Madhu-1
Madhu-1 reviewed Jul 30, 2024
View reviewed changes
rbd/mirror_group.go

// MirrorGroupStatusState is used to indicate the state of a mirrored group
// within the site status info.
type MirrorGroupStatusState int64
Copy link

Choose a reason for hiding this comment

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

add String() Interface for this one as we have MirrorImageStatusState as well.

Madhu-1
Madhu-1 reviewed Jul 30, 2024
View reviewed changes
rbd/mirror_group.go
// GlobalMirrorGroupStatus contains information pertaining to the global
// status of a mirrored group. It contains general information as well
// as per-site information stored in the SiteStatuses slice.
type GlobalMirrorGroupStatus struct {
Copy link

Choose a reason for hiding this comment

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

add LocalStatus method like we have one for a mirror for this one.

Madhu-1
Madhu-1 reviewed Jul 30, 2024
View reviewed changes
rbd/mirror_group.go
// MirrorGroupStatusStateError is equivalent to MIRROR_GROUP_STATUS_STATE_ERROR
MirrorGroupStatusStateError = MirrorGroupStatusState(C.MIRROR_GROUP_STATUS_STATE_ERROR)
// MirrorGroupStatusStateStartingReplay is equivalent to MIRROR_GROUP_STATUS_STATE_STARTING_REPLAY
MirrorGroupStatusStateStartingReplay = MirrorGroupStatusState(C.MIRROR_GROUP_STATUS_STATE_STARTING_REPLAY)
Copy link

Choose a reason for hiding this comment

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

we are missing a const for syncing here like MirrorImageStatusStateSyncing

Copy link
Author

@sp98 sp98 Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

added the comment in ceph PR here - https://github.com/ceph/ceph/pull/53793/files#r1705148133

@Rakshith-R Rakshith-R mentioned this pull request Aug 6, 2024
Draft
@sp98 sp98 changed the title Adding support mirror group APIs [WIP]Adding support mirror group APIs Aug 7, 2024
@sp98 sp98 force-pushed the mirror-group-apis branch 4 times, most recently from 0d4d52f to 8990cc8 Compare August 7, 2024 05:31
@sp98 sp98 force-pushed the mirror-group-apis branch 2 times, most recently from 7edd0f8 to fda57e9 Compare August 7, 2024 05:54
@dpulls dpulls
Copy link

dpulls bot commented Aug 15, 2024

⚠️ Dpulls not installed on repository ceph/ceph. Checkout our quickstart for how to install.

@github-actions GitHub Actions
Copy link

This Pull Request has been automatically marked as stale because it has not had recent activity. It will be closed in 21 days if no further activity occurs. Remember, a closed PR can always be reopened. Thank you for your contribution.

@github-actions github-actions bot added the Stale label Oct 15, 2024
@anoopcs9 anoopcs9 marked this pull request as draft October 15, 2024 07:03
@github-actions github-actions bot removed the Stale label Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phlogistonjohn phlogistonjohn phlogistonjohn left review comments

@Madhu-1 Madhu-1 Madhu-1 left review comments

@BlaineEXE BlaineEXE BlaineEXE left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
API This PR includes a change to the public API of a go-ceph package do-not-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sp98 @phlogistonjohn @BlaineEXE @Madhu-1