nfs: admin APIs for managing nfs exports by phlogistonjohn · Pull Request #655 · ceph/go-ceph

phlogistonjohn · 2022-02-23T15:22:21Z

The initial set of APIs are mostly in place, but the tests are untested.

Fixes: #628

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?

BlaineEXE · 2022-02-23T15:34:53Z

common/admin/nfs/export.go

+const delSucc = "Successfully deleted export"
+
+// RemoveExport will remove an NFS export based on the pseudo-path of the export.
+//  PREVIEW
+//
+// Similar To:
+//  ceph nfs export rm
+func (nfsa *Admin) RemoveExport(clusterID, pseudoPath string) error {
+	m := map[string]string{
+		"prefix":      "nfs export rm",
+		"format":      "json",
+		"cluster_id":  clusterID,
+		"pseudo_path": pseudoPath,
+	}
+	return (commands.MarshalMgrCommand(nfsa.conn, m).
+		FilterBodyPrefix(delSucc).NoData().End())
+}


Is this looking for text in the response from Ceph (delSucc)? How is that different from getting a return code? Could this be broken if the NFS code changes this message?

Since this might apply to Rook as well, (and assuming I'm on the right track) how does go-ceph handle things like this where ceph changes might break integration?

Yes it is coming from the ceph mgr module for nfs. Normally on success we expect either a json object in the body to parse and return to the caller OR nothing. Unfortunately, the mgr module returns this "helpful" string in this case. If future versions of ceph change the response string (prefix) it could cause a problem. If it stops returning the "helpful" string, its fine as we "want" no body. Note that we are not checking for that string, just filtering it out if it appears.

Unfortunately, I don't think there's a general way to future proof all the things ceph could change in the future, But if things do change we'd need to release a new version that handles all those versions.

BlaineEXE · 2022-02-23T15:41:06Z

internal/commands/response_test.go

+	t.Run("filterBodyPrefix", func(t *testing.T) {
+		rtemp := Response{
+			body: []byte("No way, no how"),
+		}
+		if assert.True(t, rtemp.Ok()) {
+			r2 := rtemp.FilterBodyPrefix("No way")
+			assert.True(t, r2.Ok())
+			assert.Equal(t, []byte(""), r2.body)
+			err := r2.NoBody().End()
+			assert.NoError(t, err)
+		}
+	})


Should there be an additional test that validates the negative case where the body is not filtered? Or is that somehow implicit from the above tests?

Sure. I can add an explicit case for that.

nixpanic · 2022-03-24T10:15:31Z

Just noting that ceph/ceph-csi#2948 could use this API. We might be able to test it out with a custom build of the (work-in-progress) NFS provisioner.

The NFS-Admin API is not part of go-ceph yet, it has been proposed as an enhancement. This vendors the contents of the PR, which should be replaced once the PR has been merged. See-also: ceph/go-ceph#655 Signed-off-by: Niels de Vos <ndevos@redhat.com>

This call is used for removing "routine" responses that get sent in the body of a response rather than the status. Otherwise, it is the same as the FilterPrefix function. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Signed-off-by: John Mulligan <jmulligan@redhat.com>

This will provide APIs for managing NFS exports via the Ceph NFS manager module. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Signed-off-by: John Mulligan <jmulligan@redhat.com>

* Add CreateCephFSExport method * Add RemoveExport method * Add ListDetailedExports method * Add ExportInfo method Signed-off-by: John Mulligan <jmulligan@redhat.com>

Signed-off-by: John Mulligan <jmulligan@redhat.com>

Our test container does not support an orchestrator to pre configure the ".nfs" configuration pool for us. This is something that rook and cephadm are expected to do before exports are created, etc. So we support creating a minimal "mock" configuration in rados. This option can be toggled on and off via the GO_CEPH_TEST_MOCK_NFS env var. Turning it off can be useful if you want to run the live suite against a "real" ceph cluster. Signed-off-by: John Mulligan <jmulligan@redhat.com>

I found this useful when debugging the mock nfs configuration pool and the nfs mgr module. Signed-off-by: John Mulligan <jmulligan@redhat.com>

phlogistonjohn · 2022-03-31T18:09:02Z

@nixpanic @BlaineEXE @ansiwen - based on some chats & verbal discussions this week, I've added a special build tag ceph_ci_untested that will allow us to leave have the source code for this in our tree but not be enabled by default or tested by the CI. The ceph-csi team can then try/test it manually by setting this build tag w/o go-ceph team "making promises" about this API to the other code bases that consume go-ceph.

Once ceph pacific has the necessary patches I will remove the special build tag and enable the code as a "normal" preview API. If ceph pacific releases soon enough I can simply do that before our release in approx. 2 weeks. If I learn ceph pacific is going to release around mid-april, I'm willing to delay the release - but only by a week or so. Otherwise, if ceph pacific doesn't release until late April or later we'll probably just release with this feature gated by this build tag.

Any questions/comments/concerns?

If we're generally all good with the above, and CI passes I'll move this PR out of draft state.

phlogistonjohn · 2022-03-31T18:10:13Z

CCing @Rakshith-R as well. See my previous comment, please.

nixpanic · 2022-04-01T08:50:39Z

If we're generally all good with the above, and CI passes I'll move this PR out of draft state.

👍 Thanks, I'm happy with that! Unfortunately the new API does not work with Ceph 16.2.0, but is seems to be fine for 16.2.7 and newer. In case of failure with the API, Ceph-CSI will probably fall-back to the old ceph command (which also changed in 16.2.7).

phlogistonjohn · 2022-04-01T14:42:39Z

If we're generally all good with the above, and CI passes I'll move this PR out of draft state.

+1 Thanks, I'm happy with that! Unfortunately the new API does not work with Ceph 16.2.0, but is seems to be fine for 16.2.7 and newer. In case of failure with the API, Ceph-CSI will probably fall-back to the old ceph command (which also changed in 16.2.7).

OK, I do wish ceph wouldn't make "API" changes on the mgr within major version release... but it happens and we have to deal with it. If you needed it I'd consider adding code to work with older versions IF it was something we could add tests for. But that would almost certainly not happen until after the next release.

phlogistonjohn · 2022-04-01T14:46:02Z

@nixpanic github is being rather weird and not letting me add you to the reviewers. I'll try again later, but feel free to review anyway.

nixpanic · 2022-04-04T08:48:53Z

@nixpanic github is being rather weird and not letting me add you to the reviewers. I'll try again later, but feel free to review anyway.

It seems I am not part of the maintainers for go-ceph anymore?

The NFS-Admin API is not part of go-ceph yet, it has been proposed as an enhancement. This vendors the contents of the PR, which should be replaced once the PR has been merged. See-also: ceph/go-ceph#655 Signed-off-by: Niels de Vos <ndevos@redhat.com>

phlogistonjohn · 2022-04-04T13:59:31Z

@ansiwen please review when you have a moment, thanks!

ansiwen

In general LGTM. There are some comments, but if you prefer to merge like this, I'm fine with it. Then just remove the do-not-merge label.

common/admin/nfs/export.go

The NFS-Admin API has been added to go-ceph v0.15.0. As the API can not be tested in the go-ceph CI, it requires build-tag `ceph_ci_untested`. This additional build-tag has been added to the `Makefile` and should be removed when the API does not require the build-tag anymore. See-also: ceph/go-ceph#655 Signed-off-by: Niels de Vos <ndevos@redhat.com>

The NFS-Admin API has been added to go-ceph v0.15.0. As the API can not be tested in the go-ceph CI, it requires build-tag `ceph_ci_untested`. This additional build-tag has been added to the `Makefile` and should be removed when the API does not require the build-tag anymore. See-also: ceph/go-ceph#655 Signed-off-by: Niels de Vos <ndevos@redhat.com> (cherry picked from commit 282c33c)

phlogistonjohn force-pushed the jjm-nfs-exports branch 2 times, most recently from 817e299 to 45961b6 Compare February 23, 2022 15:29

BlaineEXE reviewed Feb 23, 2022

View reviewed changes

phlogistonjohn force-pushed the jjm-nfs-exports branch 2 times, most recently from 20c85f5 to 62ecb4b Compare February 23, 2022 20:33

phlogistonjohn force-pushed the jjm-nfs-exports branch from 62ecb4b to 3dfea74 Compare March 10, 2022 19:55

This was referenced Mar 24, 2022

nfs: add basic provisioner with create/delete procedures ceph/ceph-csi#2948

Merged

Provisioning of NFS exported volumes ceph/ceph-csi#2913

Closed

Rakshith-R mentioned this pull request Mar 31, 2022

nfs: support new and old NFS-management commands ceph/ceph-csi#2978

Merged

phlogistonjohn added 10 commits March 31, 2022 13:46

commands: add FilterBodyPrefix method to command type

740c23a

This call is used for removing "routine" responses that get sent in the body of a response rather than the status. Otherwise, it is the same as the FilterPrefix function. Signed-off-by: John Mulligan <jmulligan@redhat.com>

commands: add test cases for FilterBodyPrefix

3db9f0f

Signed-off-by: John Mulligan <jmulligan@redhat.com>

makefile: add internal/commands pkg to list of test-bins

701f56a

Signed-off-by: John Mulligan <jmulligan@redhat.com>

nfs: add a new common/admin/nfs subpackage

81e709d

This will provide APIs for managing NFS exports via the Ceph NFS manager module. Signed-off-by: John Mulligan <jmulligan@redhat.com>

makefile: add common/admin/nfs pkg to test bins

e7be6be

Signed-off-by: John Mulligan <jmulligan@redhat.com>

nfs admin: add the admin type for nfs

69fc79d

Signed-off-by: John Mulligan <jmulligan@redhat.com>

nfs admin: add APIs for basic NFS export management

652c03e

* Add CreateCephFSExport method * Add RemoveExport method * Add ListDetailedExports method * Add ExportInfo method Signed-off-by: John Mulligan <jmulligan@redhat.com>

nfs admin: add unit and functional tests for nfs export functions

984ae4d

Signed-off-by: John Mulligan <jmulligan@redhat.com>

micro-osd.sh: set a location for mgr logging

513dcff

I found this useful when debugging the mock nfs configuration pool and the nfs mgr module. Signed-off-by: John Mulligan <jmulligan@redhat.com>

phlogistonjohn force-pushed the jjm-nfs-exports branch from 3dfea74 to 513dcff Compare March 31, 2022 17:51

phlogistonjohn marked this pull request as ready for review April 1, 2022 14:43

phlogistonjohn requested a review from ansiwen April 1, 2022 14:43

phlogistonjohn changed the title ~~[WIP] nfs: admin APIs for managing nfs exports~~ nfs: admin APIs for managing nfs exports Apr 1, 2022

nixpanic approved these changes Apr 4, 2022

View reviewed changes

nixpanic mentioned this pull request Apr 4, 2022

nfs: use go-ceph API for creating/deleting exports ceph/ceph-csi#2977

Merged

ansiwen added API This PR includes a change to the public API of a go-ceph package do-not-merge labels Apr 6, 2022

ansiwen approved these changes Apr 6, 2022

View reviewed changes

common/admin/nfs/export.go Show resolved Hide resolved

common/admin/nfs/export.go Show resolved Hide resolved

phlogistonjohn removed the do-not-merge label Apr 6, 2022

mergify bot merged commit 50f803d into ceph:master Apr 6, 2022

phlogistonjohn deleted the jjm-nfs-exports branch June 9, 2022 20:26

Conversation

phlogistonjohn commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

BlaineEXE Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phlogistonjohn Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

BlaineEXE Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

phlogistonjohn Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

phlogistonjohn Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

nixpanic commented Mar 24, 2022

Uh oh!

phlogistonjohn commented Mar 31, 2022

Uh oh!

phlogistonjohn commented Mar 31, 2022

Uh oh!

nixpanic commented Apr 1, 2022

Uh oh!

phlogistonjohn commented Apr 1, 2022

Uh oh!

phlogistonjohn commented Apr 1, 2022

Uh oh!

nixpanic commented Apr 4, 2022

Uh oh!

phlogistonjohn commented Apr 4, 2022

Uh oh!

ansiwen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

phlogistonjohn commented Feb 23, 2022 •

edited

Loading

BlaineEXE Feb 23, 2022 •

edited

Loading