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

Add untracked Subvolume and Snapshot metadata APIs in api-status #811

Merged
merged 2 commits into from
Jan 23, 2023

Conversation

anoopcs9
Copy link
Collaborator

@anoopcs9 anoopcs9 commented Jan 11, 2023

Following APIs are being added as preview APIs after ceph_pre_* tags are removed with an exception to Pacific where backports are not yet merged.

  • FSAdmin.GetMetadata
  • FSAdmin.SetMetadata
  • FSAdmin.RemoveMetadata
  • FSAdmin.ForceRemoveMetadata
  • FSAdmin.ListMetadata
  • FSAdmin.GetSnapshotMetadata
  • FSAdmin.SetSnapshotMetadata
  • FSAdmin.RemoveSnapshotMetadata
  • FSAdmin.ForceRemoveSnapshotMetadata
  • FSAdmin.ListSnapshotMetadata

fixes #752

@phlogistonjohn
Copy link
Collaborator

I'm a bit confused. First, why are you removing the ceph_preview tag but not the ceph_pre_quincy tag? Should not the process be: remove pre_* tags and then remove preview tags later?
I also don't think the made stable in version numbers are correct - how can the APIs be stable in a past release?

@anoopcs9
Copy link
Collaborator Author

I'm a bit confused. First, why are you removing the ceph_preview tag but not the ceph_pre_quincy tag? Should not the process be: remove pre_* tags and then remove preview tags later?

Unlike ceph_preview, ceph_pre* tags are just an indication to run CI tests on specific branches(as and when patches get backports). So I thought we should correct/update json(by removing ceph_preview) prior to removing ceph_pre* tags from sources.

I also don't think the made stable in version numbers are correct - how can the APIs be stable in a past release?

These were supposed to be updated in json and markdown format when they were added(in v0.16.0) but we failed to do so. v0.18.0 would have been the right version at which those APIs become stable. But I'm OK with updating it to v0.20.0 since we always say expected_stable_version which can change subject to reasons.

@phlogistonjohn
Copy link
Collaborator

I'm a bit confused. First, why are you removing the ceph_preview tag but not the ceph_pre_quincy tag? Should not the process be: remove pre_* tags and then remove preview tags later?

Unlike ceph_preview, ceph_pre* tags are just an indication to run CI tests on specific branches(as and when patches get backports). So I thought we should correct/update json(by removing ceph_preview) prior to removing ceph_pre* tags from sources.

I may be mistaken, but I don't think that's quite right. Any unfulfilled tag (unless in an || in the new syntax) is going to skip that file from being built. AFAIU this is why there was no previous instance of these APIs as preview in the json. The pre tag is causing them to be filtered out of the API discovery of "implements" tool.

If we remove the pre tags first. Then "implements" should suddenly find a new API (in the preview) state. Then we have a preview api that works on supported (& released) vrersions of ceph. After that we can then stablize that API through the (new) standard process.

I also don't think the made stable in version numbers are correct - how can the APIs be stable in a past release?

These were supposed to be updated in json and markdown format when they were added(in v0.16.0) but we failed to do so. v0.18.0 would have been the right version at which those APIs become stable. But I'm OK with updating it to v0.20.0 since we always say expected_stable_version which can change subject to reasons.

I think if we follow the process I outlined above everything should "just work" the api is added as preview in the current version. etc.

@phlogistonjohn
Copy link
Collaborator

cc: @ansiwen for 2nd opinion on the tags

@ansiwen
Copy link
Collaborator

ansiwen commented Jan 13, 2023

IMHO these tags are almost orthogonal. You guys might get a bit carried away with processes and "rules", so let's take a step back and ask two simple questions:

  • Is the exported functionality considered mature, that is, we think it's very unlikely that we would like to change it again? Then let's remove the ceph_preview tag.
  • Is the used pre-release feature available in a release version now? Then let's replace the ceph_pre_* tag with negative tags for the releases that didn't receive backports yet.

Of course APIs that are based on pre-release features are more likely to be considered immature for a longer time, but I wouldn't make an automatism out of that. On the other hand, it also doesn't hurt to keep features longer in preview state. So, if we think these APIs are stable, I don't really care if we remove ceph_preview first.

However, I have no idea how the tooling can handle these cases, so that might be a valid practical argument.

With an exemption to Pacific, necessary backports are already available
with recent Quincy releases.

Signed-off-by: Anoop C S <anoopcs@cryptolab.net>
Following APIs are being added as preview APIs:

FSAdmin.GetMetadata
FSAdmin.SetMetadata
FSAdmin.RemoveMetadata
FSAdmin.ForceRemoveMetadata
FSAdmin.ListMetadata
FSAdmin.GetSnapshotMetadata
FSAdmin.SetSnapshotMetadata
FSAdmin.RemoveSnapshotMetadata
FSAdmin.ForceRemoveSnapshotMetadata
FSAdmin.ListSnapshotMetadata

Signed-off-by: Anoop C S <anoopcs@cryptolab.net>
@anoopcs9 anoopcs9 changed the title Add untracked Snapshot and Subvolume metadata APIs in JSON format Add untracked Subvolume and Snapshot metadata APIs in JSON format Jan 20, 2023
@anoopcs9 anoopcs9 changed the title Add untracked Subvolume and Snapshot metadata APIs in JSON format Add untracked Subvolume and Snapshot metadata APIs in api-status Jan 20, 2023
@dpulls
Copy link

dpulls bot commented Jan 20, 2023

🎉 All dependencies have been resolved !

@anoopcs9
Copy link
Collaborator Author

@phlogistonjohn @ansiwen PR updated considering all responses received so far.

@anoopcs9 anoopcs9 requested a review from ansiwen January 20, 2023 08:29
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, as you addressed all my concerns from earlier.

Just to be on the up-and-up I'll not set the needed github label on this PR until monday, in case there's any additional discussion warranted.

@phlogistonjohn phlogistonjohn added the no-API This PR does not include any changes to the public API of a go-ceph package label Jan 23, 2023
@mergify mergify bot merged commit 8960979 into ceph:master Jan 23, 2023
@anoopcs9 anoopcs9 deleted the add-untracked-apis branch April 10, 2023 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-API This PR does not include any changes to the public API of a go-ceph package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove ceph_pre_quincy tags from cephfs/admin metadata and snap metadata apis
3 participants