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

rbd: implement librbd.rbd_group_snap_get_info #1025

Merged
merged 3 commits into from
Oct 9, 2024

Conversation

nixpanic
Copy link
Member

@nixpanic nixpanic commented Sep 10, 2024

The new GroupSnapGetInfo function can be used to get a list of the RBD
image snapshots that were created as part of the RBD group snapshot.

This feature is implemented in 2 steps:

  1. direct librbd call (only in Ceph main branch)
  2. modification to use dlsym

I hope that makes it easier to understand, and might help others contributing such dynamically loaded functions too.

Depends-on: ceph/ceph#59883
Depends-on: ceph/ceph#59959

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

@nixpanic nixpanic force-pushed the rbd/rbd_group_snap_get_info branch 2 times, most recently from a40229f to dee055a Compare September 11, 2024 08:34
@nixpanic nixpanic marked this pull request as ready for review September 11, 2024 08:54
@phlogistonjohn phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Sep 11, 2024
anoopcs9
anoopcs9 previously approved these changes Sep 12, 2024
Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

If we could ignore the norm to have the corresponding tests for the newly introduced API in a separate source file, which I think may not hold good here due to its nature, changes look good to me.

@anoopcs9
Copy link
Collaborator

@Mergifyio rebase

@anoopcs9 anoopcs9 force-pushed the rbd/rbd_group_snap_get_info branch from dee055a to 5d6c6f6 Compare September 12, 2024 11:38
Copy link

mergify bot commented Sep 12, 2024

rebase

✅ Branch has been successfully rebased

@nixpanic
Copy link
Member Author

Thanks @anoopcs9!

It seems that @ajarr is planning to send a PR for librbd.h to uncomment the namespace_type in the struct. Until that is done, this PR should not get merged yet.

@idryomov
Copy link

send a PR for librbd.h to uncomment the namespace_type in the struct

ceph/ceph#59883

Copy link

dpulls bot commented Sep 20, 2024

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

@nixpanic nixpanic force-pushed the rbd/rbd_group_snap_get_info branch from 5d6c6f6 to 2052bb5 Compare September 20, 2024 08:47
@mergify mergify bot dismissed anoopcs9’s stale review September 20, 2024 08:48

Pull request has been modified.

rbd/group_snap_info.go Outdated Show resolved Hide resolved
@nixpanic nixpanic force-pushed the rbd/rbd_group_snap_get_info branch 2 times, most recently from e604719 to f7fa0ce Compare September 24, 2024 15:48
@nixpanic nixpanic marked this pull request as ready for review September 24, 2024 16:26
@nixpanic nixpanic marked this pull request as draft September 25, 2024 06:51
Copy link

dpulls bot commented Sep 25, 2024

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

1 similar comment
Copy link

dpulls bot commented Sep 25, 2024

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

@phlogistonjohn
Copy link
Collaborator

@Mergifyio rebase

The new GroupSnapGetInfo function can be used to get a list of the RBD
image snapshots that were created as part of the RBD group snapshot.

Signed-off-by: Niels de Vos <[email protected]>
Copy link

mergify bot commented Sep 25, 2024

rebase

✅ Branch has been successfully rebased

@phlogistonjohn phlogistonjohn force-pushed the rbd/rbd_group_snap_get_info branch from f7fa0ce to 4b0b8c8 Compare September 25, 2024 14:24
Copy link

dpulls bot commented Sep 25, 2024

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

Copy link

dpulls bot commented Sep 25, 2024

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

@nixpanic nixpanic marked this pull request as ready for review September 25, 2024 16:00
@nixpanic
Copy link
Member Author

All dependencies are merged, this should be ready for review (again) now.

@nixpanic
Copy link
Member Author

@Mergifyio refresh

Maybe that is needed to detect the merging of the dependencies?
image

Copy link

mergify bot commented Sep 25, 2024

refresh

✅ Pull request refreshed

Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

lgtm.

@nixpanic
Copy link
Member Author

@Mergifyio rebase

Copy link

mergify bot commented Sep 26, 2024

rebase

✅ Nothing to do for rebase action

@nixpanic
Copy link
Member Author

nixpanic commented Oct 3, 2024

@ansiwen could you please have a look at this?

@nixpanic nixpanic requested a review from ansiwen October 3, 2024 08:09
@nixpanic
Copy link
Member Author

nixpanic commented Oct 4, 2024

@Mergifyio rebase

Copy link

mergify bot commented Oct 4, 2024

rebase

✅ Nothing to do for rebase action

Copy link
Collaborator

@ansiwen ansiwen left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +11 to +63
// Types and constants are copied from librbd.h with added "_" as prefix. This
// prevents redefinition of the types on librbd versions that have them
// already.

typedef enum {
_RBD_GROUP_SNAP_NAMESPACE_TYPE_USER = 0
} _rbd_group_snap_namespace_type_t;

typedef struct {
char *image_name;
int64_t pool_id;
uint64_t snap_id;
} _rbd_group_image_snap_info_t;

typedef struct {
char *id;
char *name;
char *image_snap_name;
rbd_group_snap_state_t state;
_rbd_group_snap_namespace_type_t namespace_type;
size_t image_snaps_count;
_rbd_group_image_snap_info_t *image_snaps;
} _rbd_group_snap_info2_t;

// rbd_group_snap_get_info_fn matches the rbd_group_snap_get_info function signature.
typedef int(*rbd_group_snap_get_info_fn)(rados_ioctx_t group_p,
const char *group_name,
const char *snap_name,
_rbd_group_snap_info2_t *snaps);

// rbd_group_snap_get_info_dlsym take *fn as rbd_group_snap_get_info_fn and
// calls the dynamically loaded rbd_group_snap_get_info function passed as 1st
// argument.
static inline int rbd_group_snap_get_info_dlsym(void *fn,
rados_ioctx_t group_p,
const char *group_name,
const char *snap_name,
_rbd_group_snap_info2_t *snaps) {
// cast function pointer fn to rbd_group_snap_get_info and call the function
return ((rbd_group_snap_get_info_fn) fn)(group_p, group_name, snap_name, snaps);
}

// rbd_group_snap_get_info_cleanup_fn matches the rbd_group_snap_get_info_cleanup function signature.
typedef int(*rbd_group_snap_get_info_cleanup_fn)(_rbd_group_snap_info2_t *snaps);

// rbd_group_snap_get_info_cleanup_dlsym take *fn as rbd_group_snap_get_info_cleanup_fn and
// calls the dynamically loaded rbd_group_snap_get_info_cleanup function passed as 1st
// argument.
static inline int rbd_group_snap_get_info_cleanup_dlsym(void *fn,
_rbd_group_snap_info2_t *snaps) {
// cast function pointer fn to rbd_group_snap_get_info_cleanup and call the function
return ((rbd_group_snap_get_info_cleanup_fn) fn)(snaps);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wish we could avoid some of that boilerplate... But I don't have a suggestion right now, how to improve it. So I guess we just keep it like that for now, which probably means forever. 😅

@ansiwen
Copy link
Collaborator

ansiwen commented Oct 9, 2024

@Mergifyio rebase

Copy link

mergify bot commented Oct 9, 2024

rebase

✅ Nothing to do for rebase action

@anoopcs9
Copy link
Collaborator

anoopcs9 commented Oct 9, 2024

@Mergifyio refresh

Copy link

mergify bot commented Oct 9, 2024

refresh

✅ Pull request refreshed

@anoopcs9
Copy link
Collaborator

anoopcs9 commented Oct 9, 2024

@Mergifyio requeue

Copy link

mergify bot commented Oct 9, 2024

requeue

❌ This pull request head commit has not been previously disembarked from queue.

@anoopcs9
Copy link
Collaborator

anoopcs9 commented Oct 9, 2024

@Mergifyio queue

Copy link

mergify bot commented Oct 9, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 08e5b62

@mergify mergify bot merged commit 08e5b62 into ceph:master Oct 9, 2024
16 checks passed
@anoopcs9
Copy link
Collaborator

anoopcs9 commented Oct 9, 2024

Ha..just because we had the following depends in the PR description and ceph did not have it configured, merging got stalled.

Depends-on: ceph/ceph#59883
Depends-on: ceph/ceph#59959

Now I can fool Mergify by adding the above lines back.

Copy link

dpulls bot commented Oct 9, 2024

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

1 similar comment
Copy link

dpulls bot commented Oct 9, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API This PR includes a change to the public API of a go-ceph package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants