cephfs: add subvolume metadata APIs #691

pkalever · 2022-05-18T17:28:17Z

Description

Exports below APIs:
SetMetadata,
GetMetadata(),
RemoveMetadata(),
ForceRemoveMetadata() and
ListMetadata()

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

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? Is this new API marked PREVIEW?

pkalever · 2022-05-18T17:34:43Z

@phlogistonjohn FYI, I had tested these changes with cephfs master/devel code quay.io/ceph/daemon-base:latest-master. Yesterday I checked quay.io/ceph/ceph:v17.2 Quincy build, the cephfs subvolume metadata changes are missing there.

phlogistonjohn · 2022-05-18T18:07:29Z

@phlogistonjohn FYI, I had tested these changes with cephfs master/devel code quay.io/ceph/daemon-base:latest-master. Yesterday I checked quay.io/ceph/ceph:v17.2 Quincy build, the cephfs subvolume metadata changes are missing there.

Looks really cool so far. The fact that it doesn't work with any released version of ceph yet is a bit annoying though. We need to add the build tags that will restrict what versions the code will work with. Last release I had a similar challenge with nfs mgr module changes - so I created a build tag 'ceph_ci_untested' that allows us to manually toggle a feature that we want in the codebase but can not be tested yet.
Take a look at common/admin/nfs/admin.go and the build tags on that file. Similarly, we'll also want the ceph_preview build tag and the PREVIEW comment on the new APIs.

If you have any questions/thoughts about this process feel fee to ask!

pkalever · 2022-05-19T06:10:29Z

@nmshelke What version of ceph will have ceph/ceph#45603 ? Do we have plans to backport it to Quincy ?

cc: @vshankar @kotreshhr

Madhu-1 · 2022-05-19T06:13:04Z

@pkalever https://tracker.ceph.com/issues/54472 has the backport details. backport is merged to Quincy.

pkalever · 2022-05-19T06:16:27Z

Thanks @Madhu-1, as per https://tracker.ceph.com/issues/54472 this feature will be part of Ceph - v18.0.0, and will be backported to quincy and pacific.

pkalever · 2022-05-19T06:41:57Z

Looks really cool so far. The fact that it doesn't work with any released version of ceph yet is a bit annoying though. We need to add the build tags that will restrict what versions the code will work with. Last release I had a similar challenge with nfs mgr module changes - so I created a build tag 'ceph_ci_untested' that allows us to manually toggle a feature that we want in the codebase but can not be tested yet. Take a look at common/admin/nfs/admin.go and the build tags on that file. Similarly, we'll also want the ceph_preview build tag and the PREVIEW comment on the new APIs.

If you have any questions/thoughts about this process feel fee to ask!

Thanks @phlogistonjohn, added the build tags on the files and also PREVIEW comment on the APIs, PTAL.

nmshelke · 2022-05-19T06:54:33Z

@nmshelke What version of ceph will have ceph/ceph#45603 ?
cc: @vshankar @kotreshhr

As per tracker information (https://tracker.ceph.com/issues/54472 ),
ceph/ceph#45603 will be in Ceph - v18.0.0

Do we have plans to backport it to Quincy ?

We have backport pull requests for quincy and pacific. Please follow following pull request and tracker for more details:
For quincy: ceph/ceph#45994 , tracker: https://tracker.ceph.com/issues/55376
For pacific: ceph/ceph#45961 , tracker: https://tracker.ceph.com/issues/55375

This is needed for now, ceph/go-ceph#691 This PR at go-ceph add subvolume metadata APIs. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

pkalever · 2022-06-09T03:47:22Z

@Mergifyio rebase

mergify · 2022-06-09T03:48:03Z

rebase

✅ Branch has been successfully rebased

pkalever · 2022-06-09T06:18:40Z

@phlogistonjohn @ansiwen could you please help review.

phlogistonjohn

I was going to approve this but then I noticed the new functions are all being added to the stable section of api-status.json. That's not right as they're certainly not stable, as they're not even preview yet. The API itself looks good tho.

ansiwen · 2022-06-09T14:31:17Z

I was going to approve this but then I noticed the new functions are all being added to the stable section of api-status.json. That's not right as they're certainly not stable, as they're not even preview yet. The API itself looks good tho.

Huh, is that a bug of my recent change?

ansiwen · 2022-06-09T14:34:02Z

docs/api-status.json

@@ -494,6 +494,26 @@
      {
        "name": "FSAdmin.VolumeStatus",
        "comment": "VolumeStatus returns a VolumeStatus object for the given volume name.\n\nSimilar To:\n ceph fs status cephfs <name>\n"
+      },


Did you manually add this, or is this coming from make api-update? In the latter case we have a bug there.

make api-update didn't work for me :-(
Had manually edited this with an online json tool.

That's unfortunate, but now the CI is failing again because we don't have any API information for the functions.
Please describe how api-update failed for you.
As @ansiwen mentioned there could be a bug in the tool. If we can't fix it quickly we can also edit the JSON manually but the new APIs need to go into a preview section not the stable section.

Sorry, I rushed my reply. I see you did add the functions but they're missing the descriptive metadata needed for that section. Please take a look a the other sections and how they're adding added_in_version and expected_stable_version. Please feel free to use v0.16.0 for added_in_version.
For expected_stable_version let's try "n/a" for now. If the CI still barfs because it's not a version number (I don't think it cares) just pick a version number far enough in the future that we're not making any promises of quick stabilization (we can always change that, its just a hint) like v0.20.0.

Actually, I can not reproduce the CI failure locally. So I'm a bit unclear on what's actually going on. Please hold off working on this until I report back. Sorry for the noise.

OK, after one SNAFU where I was trying the changes from your other PR, I think the proper and simplest fix is to simply drop the 2nd patch in your pr ('docs: update subvolume metadata API details'). Because the code is now also guarded by the ceph_ci_untested tag there's no need to do the API stuff until it is in proper preview status, eg. when it's only guarded by the ceph_preview build tag.

I've actually tried this locally and it worked so I have high confidence in this suggestion, and it should be pretty easy to implement on your side. :-)
Just remember when we come from untested status to preview status, then we'll need to do the api status tracking json changes. Hopefully then we can work out any issues you have with the make api-update command and so it'll mostly be automatic at that time.

I looked into this, and it seems implements evaluates the tags itself. Since ceph_ci_untested is not set it ignores these files. So everything works as designed. You should simply not touch this file in this PR and call make api-update later after you removed the untested tag in a later PR.

Sure @phlogistonjohn and @ansiwen, dropped the 2nd patch for now. Thanks!

cephfs/admin/metadata.go

ansiwen

In general LGTM, but there are minor tweaks necessary.

Exports below APIs: SetMetadata(), GetMetadata(), RemoveMetadata(), ForceRemoveMetadata() and ListMetadata() Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

pkalever · 2022-06-10T05:47:31Z

@phlogistonjohn @ansiwen addressed the review comments now, PTAL. Thanks!

phlogistonjohn · 2022-06-13T14:43:47Z

We can't seem to get past the darn test flake today. Because we know that a) this feature is disabled by default b) its not causing any test failures, I'm going to merge w/o having all the CI pass.

This is needed for now, ceph/go-ceph#691 This PR at go-ceph add subvolume metadata APIs. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

pkalever force-pushed the metadata branch from 2e74e88 to 0878b3a Compare May 19, 2022 06:32

pkalever pushed a commit to pkalever/ceph-csi that referenced this pull request May 24, 2022

build: use a custom go-ceph branch

5b9da72

This is needed for now, ceph/go-ceph#691 This PR at go-ceph add subvolume metadata APIs. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

pkalever mentioned this pull request May 24, 2022

cephfs: set PV/PVC and clusterName details on the subvolume as metadata ceph/ceph-csi#3133

Merged

pkalever pushed a commit to pkalever/ceph-csi that referenced this pull request May 26, 2022

build: use a custom go-ceph branch

7a4a983

This is needed for now, ceph/go-ceph#691 This PR at go-ceph add subvolume metadata APIs. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

pkalever marked this pull request as draft May 30, 2022 12:39

pkalever marked this pull request as ready for review May 30, 2022 12:39

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

pkalever pushed a commit to pkalever/ceph-csi that referenced this pull request Jun 8, 2022

build: use a custom go-ceph branch

8764947

This is needed for now, ceph/go-ceph#691 This PR at go-ceph add subvolume metadata APIs. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

pkalever pushed a commit to pkalever/ceph-csi that referenced this pull request Jun 8, 2022

build: use a custom go-ceph branch

4aea3a2

This is needed for now, ceph/go-ceph#691 This PR at go-ceph add subvolume metadata APIs. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

pkalever pushed a commit to pkalever/ceph-csi that referenced this pull request Jun 8, 2022

build: use a custom go-ceph branch

5520c31

This is needed for now, ceph/go-ceph#691 This PR at go-ceph add subvolume metadata APIs. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

ktdreyer force-pushed the metadata branch from 0878b3a to 2491654 Compare June 9, 2022 03:48

phlogistonjohn requested changes Jun 9, 2022

View reviewed changes

phlogistonjohn mentioned this pull request Jun 9, 2022

cephfs: add subvolume snapshot metadata APIs #698

Merged

4 tasks

ansiwen reviewed Jun 9, 2022

View reviewed changes

cephfs/admin/metadata.go Outdated Show resolved Hide resolved

ansiwen reviewed Jun 9, 2022

View reviewed changes

cephfs/admin/metadata.go Show resolved Hide resolved

ansiwen requested changes Jun 9, 2022

View reviewed changes

cephfs: add subvolume metadata APIs

562a91e

Exports below APIs: SetMetadata(), GetMetadata(), RemoveMetadata(), ForceRemoveMetadata() and ListMetadata() Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

pkalever force-pushed the metadata branch from 2491654 to 78cb291 Compare June 10, 2022 05:45

pkalever requested review from ansiwen and phlogistonjohn June 10, 2022 05:46

pkalever force-pushed the metadata branch from 78cb291 to 562a91e Compare June 13, 2022 06:34

phlogistonjohn approved these changes Jun 13, 2022

View reviewed changes

ansiwen approved these changes Jun 13, 2022

View reviewed changes

phlogistonjohn merged commit 7d22490 into ceph:master Jun 13, 2022

pkalever pushed a commit to pkalever/ceph-csi that referenced this pull request Jun 14, 2022

build: use a custom go-ceph branch

f2bd80d

This is needed for now, ceph/go-ceph#691 This PR at go-ceph add subvolume metadata APIs. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

pkalever pushed a commit to pkalever/ceph-csi that referenced this pull request Jun 14, 2022

build: use a custom go-ceph branch

2c8a10d

This is needed for now, ceph/go-ceph#691 This PR at go-ceph add subvolume metadata APIs. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

phlogistonjohn mentioned this pull request Jun 27, 2022

CI: Test go-ceph with unreleased versions of Ceph #703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cephfs: add subvolume metadata APIs #691

cephfs: add subvolume metadata APIs #691

pkalever commented May 18, 2022 •

edited

pkalever commented May 18, 2022

phlogistonjohn commented May 18, 2022

pkalever commented May 19, 2022

Madhu-1 commented May 19, 2022

pkalever commented May 19, 2022

pkalever commented May 19, 2022

nmshelke commented May 19, 2022 •

edited

pkalever commented Jun 9, 2022

mergify bot commented Jun 9, 2022

pkalever commented Jun 9, 2022

phlogistonjohn left a comment

ansiwen commented Jun 9, 2022

ansiwen Jun 9, 2022

pkalever Jun 10, 2022

phlogistonjohn Jun 10, 2022

phlogistonjohn Jun 10, 2022

phlogistonjohn Jun 10, 2022

phlogistonjohn Jun 10, 2022

ansiwen Jun 11, 2022

pkalever Jun 13, 2022

ansiwen left a comment

pkalever commented Jun 10, 2022

phlogistonjohn commented Jun 13, 2022

cephfs: add subvolume metadata APIs #691

cephfs: add subvolume metadata APIs #691

Conversation

pkalever commented May 18, 2022 • edited

Description

Checklist

pkalever commented May 18, 2022

phlogistonjohn commented May 18, 2022

pkalever commented May 19, 2022

Madhu-1 commented May 19, 2022

pkalever commented May 19, 2022

pkalever commented May 19, 2022

nmshelke commented May 19, 2022 • edited

pkalever commented Jun 9, 2022

mergify bot commented Jun 9, 2022

✅ Branch has been successfully rebased

pkalever commented Jun 9, 2022

phlogistonjohn left a comment

Choose a reason for hiding this comment

ansiwen commented Jun 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansiwen left a comment

Choose a reason for hiding this comment

pkalever commented Jun 10, 2022

phlogistonjohn commented Jun 13, 2022

pkalever commented May 18, 2022 •

edited

nmshelke commented May 19, 2022 •

edited