Skip to content

WIP: mgr/volumes/nfs: minor enhancements#35414

Closed
ajarr wants to merge 7 commits intoceph:masterfrom
ajarr:mgr-ganesha-misc
Closed

WIP: mgr/volumes/nfs: minor enhancements#35414
ajarr wants to merge 7 commits intoceph:masterfrom
ajarr:mgr-ganesha-misc

Conversation

@ajarr
Copy link
Contributor

@ajarr ajarr commented Jun 5, 2020

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard backend
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@ajarr ajarr changed the title mgr/volumes/nfs: minor enhancements WIP: mgr/volumes/nfs: minor enhancements Jun 5, 2020
@ajarr
Copy link
Contributor Author

ajarr commented Jun 5, 2020

Still needs tests.

@ajarr ajarr marked this pull request as draft June 5, 2020 13:35
@ajarr
Copy link
Contributor Author

ajarr commented Jun 5, 2020

@batrick @mgfritch

Couple of questions,

  • Any reason to allow only alphanumeric characters in ganesha cluster IDs? Should we also allow additional chars such as period, underscore and hyphen. Patrick, I know that you didn't want slashes in a cluster ID

  • Listing exports

@ajarr ajarr requested review from batrick, mgfritch and varshar16 June 5, 2020 14:16
@mgfritch
Copy link
Contributor

mgfritch commented Jun 5, 2020

  • Any reason to allow only alphanumeric characters in ganesha cluster IDs? Should we also allow additional chars such as period, underscore and hyphen. Patrick, I know that you didn't want slashes in a cluster ID

any of those chars should be aok for the orchestrator (period, underscore, hyphen etc.), but avoiding the forward slash makes sense for rados ...

  • And do we want to output the export contents in the native ganesha export syntax or in the python dictionary format (as done in the dashboard code)?

I'd vote for the python format so the cli can be expanded to support things like --format yaml or --format json etc.

@batrick batrick added cephfs Ceph File System needs-review labels Jun 5, 2020
@batrick
Copy link
Member

batrick commented Jun 5, 2020

@batrick @mgfritch

Couple of questions,

* Any reason to allow only alphanumeric characters in ganesha cluster IDs? Should we also allow additional chars such as period, underscore and hyphen. Patrick, I know that you didn't want slashes in a cluster ID

[._-] should be okay too.

* **Listing exports**
  
  * In this version of the PR, I only output the pseudopaths/bindings in a NFS cluster. In the dashboard code, list_exports outputs the entire content of all the exports in a cluster. Should we do that instead? https://github.com/ceph/ceph/blob/master/src/pybind/mgr/dashboard/controllers/nfsganesha.py#L118

I think adding an option to get the full export contents for a listing would be a good balance. (Think ls -l.)

  * And do we want to output the export contents in the native ganesha export syntax or in the python dictionary format (as done in the dashboard code)?

Python dictionary format is fine. If we have some existing translation code we could even add an option to dump the Ganesha config instead.

varshar16
varshar16 suggested changes Jun 8, 2020
View reviewed changes
src/pybind/mgr/volumes/fs/nfs.py Outdated
Comment on lines +375 to +377
for export in self.exports[cluster_id]:
if export.pseudo == pseudo_path:
export_obj = "export-{}".format(export.export_id)
Copy link
Contributor

@varshar16 varshar16 Jun 8, 2020

Choose a reason for hiding this comment

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

Use _fetch_export().

Suggested change
for export in self.exports[cluster_id]:
if export.pseudo == pseudo_path:
export_obj = "export-{}".format(export.export_id)
self.rados_namespace = cluster_id
export = self._fetch_export(pseudo_path)
export_obj = "export-{}".format(export.export_id)

@ajarr ajarr force-pushed the mgr-ganesha-misc branch from 58c548b to 3f08a16 Compare June 14, 2020 15:07
@ajarr
Copy link
Contributor Author

ajarr commented Jun 14, 2020

This PR was based on top of #35417
Once that PR includes the three commits in this PR that fix trackers 45744 and 45741. I will abandon this PR.

ajarr added 3 commits June 14, 2020 21:36
@ajarr
mgr/volume/nfs: allow only [A-Za-z0-9-_.] characters
6c9161a 
... for ganesha cluster names.

Fixes: https://tracker.ceph.com/issues/45744
Signed-off-by: Ramana Raja <rraja@redhat.com>
@ajarr
mgr/volumes/nfs: list exports within a ganesha cluster
f8711d2 
List the pseudo paths of the exports within a ganesha cluster
with `nfs export ls <cluster ID>` command.

List all the details of the exports within a ganesha cluster
with `nfs export ls <cluster ID> --detailed` command.

Partially-fixes: https://tracker.ceph.com/issues/45741
Signed-off-by: Ramana Raja <rraja@redhat.com>
@ajarr
mgr/volumes/nfs: get export in a nfs cluster
8967a3c 
Fixes: https://tracker.ceph.com/issues/45741
Signed-off-by: Ramana Raja <rraja@redhat.com>
@ajarr ajarr force-pushed the mgr-ganesha-misc branch from 3f08a16 to 8967a3c Compare June 14, 2020 16:39
@ajarr ajarr mentioned this pull request Jun 14, 2020
Merged
@ajarr ajarr closed this Jun 16, 2020
@ajarr
Copy link
Contributor Author

ajarr commented Jun 16, 2020

This PR is subsumed by #35417

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgfritch mgfritch Awaiting requested review from mgfritch

@batrick batrick Awaiting requested review from batrick

1 more reviewer

@varshar16 varshar16 varshar16 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cephfs Ceph File System needs-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ajarr @mgfritch @batrick @varshar16