API for quiesceing io on a subvolume.

[manishym]

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.

fixes #955

Depends on ceph/ceph#54485

[manishym]

API update does not work since the PR in Ceph is in progress.
I am not sure about the //go:build Ceph_preview, but I have added it to the top of the subvolume.go file.

[anoopcs9]

API update does not work since the PR in Ceph is in progress. I am not sure about the //go:build Ceph_preview, but I have added it to the top of the subvolume.go file.

You can either mark the dependency in the PR description or convert it to draft until its ready. We may also do both for better clarity.

[dpulls]

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

[manishym]

API update does not work since the PR in Ceph is in progress. I am not sure about the //go:build Ceph_preview, but I have added it to the top of the subvolume.go file.

You can either mark the dependency in the PR description or convert it to draft until its ready. We may also do both for better clarity.

Thanks Anoop, I have added dependency in the PR description, can it be reviewed even if it is a draft PR?

[anoopcs9]

API update does not work since the PR in Ceph is in progress. I am not sure about the //go:build Ceph_preview, but I have added it to the top of the subvolume.go file.

You can either mark the dependency in the PR description or convert it to draft until its ready. We may also do both for better clarity.

Thanks Anoop, I have added dependency in the PR description, can it be reviewed even if it is a draft PR?

Draft PR indicate a work in progress code and generally doesn't call for reviews unless explicitly asked for. Similarly in this case you may have to make changes unless and until the underlying ceph support is finalized and merged. But from the looks of things it(ceph support) is almost complete. Thus I'll leave it up to you to decide.

[anoopcs9]

@manishym You could very well look into fixing the failure for CI check. Try running make check locally to identify and make changes accordingly.

[phlogistonjohn]

Hi @manishym I hope you don't mind but as a project admin I can convert your PR to draft, so I did it.

The check errors seem similar to some errors I'm seeing on another project and I wonder if they were possibly triggered by the recent golang release. It might be good to run the check without your change and see if they occur on the head of master branch. If so, it could be good practice to try and submit a PR that just fixes those :-)

[manishym]

I have fixed linting errors in my code. There are more issues because of revive 1.3.7, which I will fix in another PR.

[manishym]

Converting to draft, since api changed in Ceph.

[phlogistonjohn]

Sorry for the delay in direct feedback, I've been very busy lately.

I'd like you to start off by putting all the new APIs in a separate file. Go build tags work entirely on a file basis and go generally encourages more files over big files anyway. Call the new file quiesce.go or something like that and new tests will be put in quiesce_test.go. Then on both files we will need a set of build tags that exclude them from building on anything but main.

Does that make sense? Feel free to ask q's for clarification if needed. :-)

[manishym]

@Mergifyio rebase

[mergify]

rebase

❌ Unable to rebase: user manishym is unknown.

Please make sure manishym has logged in Mergify dashboard.

[manishym]

@phlogistonjohn The ceph PR is merged. Do we need to wait till it is back ported to squid?

[anoopcs9]

@phlogistonjohn The ceph PR is merged. Do we need to wait till it is back ported to squid?

We don't have to.

However we could remove fs_quiesce_reef_test.go altogether and have the following build tag with an issue created to properly manage squid/ceph_pre_squid tags when backport gets into squid branch and subsequently into a release?

//go:build !(nautilus || octopus || pacific || quincy || reef || squid) && ceph_preview

If backport gets into squid branch add ceph_pre_squid and remove squid
//go:build !(nautilus || octopus || pacific || quincy || reef) && ceph_preview && ceph_pre_squid

and remove ceph_pre_squid once squid gets a release.

@phlogistonjohn What do you think?

[phlogistonjohn]

I thought @nixpanic wanted the "negative" test case. Do I mis{remember,understand}?

[phlogistonjohn]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[manishym]

I thought @nixpanic wanted the "negative" test case. Do I mis{remember,understand}?

It is true, but it will not work because !squid test fails on main, since main has API, I can't remove ceph_preview from the api implementation, since it is preview. So can't actually run the negative test until API becomes stable.

[manishym]

@anoopcs9 @phlogistonjohn Can you please merge this. I have disabled test on squid, it runs only on main.

[anoopcs9]

@anoopcs9 @phlogistonjohn Can you please merge this. I have disabled test on squid, it runs only on main.

//go:build ceph_preview && squid && !ceph_pre_squid

It's a bit complicated. Going forward we have to run the tests on pre-squid, squid(as and when backports are merged accordingly) and always on main. With the above tags used once the go-ceph API become stable after 2 releases we end up running the test only on squid. Thus we may need something like I suggested via #958 (comment) where the test will be run on any version >= squid including main.

Also is that the case that @nixpanic is in agreement here to have only one test file as @phlogistonjohn also wanted to have a clarity on this?

[nixpanic]

My preference is to have a expected-to-fail test for < squid versions. As Ceph will not backport the feature to those versions, users can be given an expected error that indicates the API is not available.

We'll use this API in Ceph-CSI, and users tend to mix Ceph-CSI versions with almost any Ceph version. An error returned by this new API in case of older Ceph version could then be handled/reported differently than other errors.

[manishym]

@anoopcs9 Before feature is merged in squid, the tag should be
//go:build !(nautilus || octopus || pacific || quincy || reef || squid || ceph_pre_squid ) && ceph_preview right? Otherwise, it will fail currently on pre_squid right?
I need to create 2 issues, 1 for removing pre_squild from above list once PR is merged in squid and one for adding squid once the squid is released right?

[manishym]

My preference is to have a expected-to-fail test for < squid versions. As Ceph will not backport the feature to those versions, users can be given an expected error that indicates the API is not available.

We'll use this API in Ceph-CSI, and users tend to mix Ceph-CSI versions with almost any Ceph version. An error returned by this new API in case of older Ceph version could then be handled/reported differently than other errors.

Since this feature itself is only available on ceph_preview, how does the build tag work for pre squid? In other words, what should be the build tag on cephfs/admin/fs_quiesce.go, as of now it is ceph_preview, since it is a new api. If I put ceph_preview || !squid, it would mean the api is no longer preview right? I need help understanding how to add a api which is both preview and works on all releases?

[nixpanic]

The build-tag is important for the client application that calls the API. A Ceph client of any version can connect to a Ceph cluster running a different version. Because this is a Ceph Mgr extension, the Ceph client version is not really important here, it is only sending JSON, no cgo with linking to particular shared libraries that may or may not have certain symbols.

I think the implementation in go-ceph does not need to depend on the Ceph version (but it could, just technically it is not needed). To me it is more important that the new API returns known errors when a Ceph version does not implement the quiesce functionality. That means only the tests depend on the Ceph version, so the two variations of _test.go have different build tags.

If others prefer to have the new API excluded in pre-squid versions, I don't really mind.

[manishym]

The build-tag is important for the client application that calls the API. A Ceph client of any version can connect to a Ceph cluster running a different version. Because this is a Ceph Mgr extension, the Ceph client version is not really important here, it is only sending JSON, no cgo with linking to particular shared libraries that may or may not have certain symbols.

I think the implementation in go-ceph does not need to depend on the Ceph version (but it could, just technically it is not needed). To me it is more important that the new API returns known errors when a Ceph version does not implement the quiesce functionality. That means only the tests depend on the Ceph version, so the two variations of _test.go have different build tags.

If others prefer to have the new API excluded in pre-squid versions, I don't really mind.

Every new API should be in ceph_preview right?

[nixpanic]

Thanks! Looks good to me.

[manishym]

@anoopcs9 @phlogistonjohn Can you please merge this. I have disabled test on squid, it runs only on main.

//go:build ceph_preview && squid && !ceph_pre_squid

It's a bit complicated. Going forward we have to run the tests on pre-squid, squid(as and when backports are merged accordingly) and always on main. With the above tags used once the go-ceph API become stable after 2 releases we end up running the test only on squid. Thus we may need something like I suggested via #958 (comment) where the test will be run on any version >= squid including main.

Also is that the case that @nixpanic is in agreement here to have only one test file as @phlogistonjohn also wanted to have a clarity on this?

@anoopcs9 I have addressed all the comments. Can you please let me know if there is something else needed to merge this?

[phlogistonjohn]

LGTM, thanks!