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: include details for parent image in the trash #906

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

nixpanic
Copy link
Member

When a parent image has been removed, it will linger in the trash until all siblings are gone. The image is not accessible through it's name anymore, only through its ID.

The ImageSpec that is returned by Image.GetParent() now contains the Trash boolean and the ImageID to identify if the image is in the trash, and use OpenImageById() to access the removed parent image.

Related-to: ceph/ceph-csi#4013

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

@phlogistonjohn phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Jul 31, 2023
@phlogistonjohn
Copy link
Collaborator

Marked as an API change for review purposes. Change itself extends an existing type with new fields.

@phlogistonjohn
Copy link
Collaborator

Change generally looks good to me and I'm willing to approve as-is, but first I want to ask. Would you mind completing the object and putting all fields present in rbd_linked_image_spec_t into the Go equivalent?

As of nautilus the fields are:

typedef struct {
  int64_t pool_id;
  char *pool_name;
  char *pool_namespace;
  char *image_id;
  char *image_name;
  bool trash;
} rbd_linked_image_spec_t;

After your change we still lack pool_id and pool_namespace. I think it would be nice to proactively complete the set so that the next person who needs one of those doesn't need to file a PR :-)

What do you think?

@phlogistonjohn
Copy link
Collaborator

Also, we can ignore failures in the CI against ceph main. This is due to a general infra issues on the ceph side related to nfs-ganesha builds.

Copy link
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

Thanks !
It looks good to me,
added a small suggestion to check for ImageID too.

rbd/rbd_nautilus_test.go Show resolved Hide resolved
Copy link
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

Lgtm !

phlogistonjohn
phlogistonjohn previously approved these changes Aug 1, 2023
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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.

Should we add an additional check to ensure that parent is no longer accessible by name but via ID(using OpenImageById()) as mentioned in the commit message?

rbd/rbd_nautilus_test.go Outdated Show resolved Hide resolved
rbd/rbd_nautilus_test.go Outdated Show resolved Hide resolved
When a parent image has been removed, it will linger in the trash until
all siblings are gone. The image is not accessible through it's name
anymore, only through its ID.

The ImageSpec that is returned by Image.GetParent() now contains the
Trash boolean and the ImageID to identify if the image is in the trash,
and use OpenImageById() to access the removed parent image.

Related-to: ceph/ceph-csi#4013
Signed-off-by: Niels de Vos <ndevos@ibm.com>
@nixpanic nixpanic requested a review from anoopcs9 August 2, 2023 07:55
@mergify mergify bot dismissed phlogistonjohn’s stale review August 2, 2023 07:55

Pull request has been modified.

@nixpanic
Copy link
Member Author

nixpanic commented Aug 2, 2023

Should we add an additional check to ensure that parent is no longer accessible by name but via ID(using OpenImageById()) as mentioned in the commit message?

That was taken from the discussion in ceph/ceph-csi#4013. I do not think we need to validate this particular RBD behavior. Possibly it could be added to a test that verifies the Trash() function.

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.

Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot merged commit 1257d81 into ceph:master Aug 2, 2023
11 of 12 checks passed
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.

None yet

4 participants