Skip to content

mgr/volumes: Integrate cephadm with volume nfs interface#34672

Merged
batrick merged 28 commits intoceph:masterfrom
varshar16:wip-integrate-cephadm-with-volume-nfs
Jun 1, 2020
Merged

mgr/volumes: Integrate cephadm with volume nfs interface#34672
batrick merged 28 commits intoceph:masterfrom
varshar16:wip-integrate-cephadm-with-volume-nfs

Conversation

@varshar16
Copy link
Contributor

@varshar16 varshar16 commented Apr 22, 2020 •
edited
Loading

Major changes are:

  • Add placement option to cluster create interface
    $ ceph nfs cluster create <type=cephfs> <clusterid> [<placement>]

  • Add cluster delete and update interface

$ ceph nfs cluster update <clusterid> <placement>
$ ceph nfs cluster delete <clusterid>
  • Rearrangement of binding and cluster id in export create interface
  • Remove type option in export create interface
 $ ceph nfs export create cephfs <fsname> <clusterid> <binding> [--readonly] [--path=/path/in/cephfs]
  • Add export delete interface
$ ceph nfs export delete <clusterid> <binding>
  • Update Ganesha package details
  • Add watch_url in vstart
  • Add tests
  • Call orch nfs interface
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

@varshar16 varshar16 added the cephfs Ceph File System label Apr 22, 2020
sebastian-philipp
sebastian-philipp reviewed Apr 22, 2020
View reviewed changes
@varshar16 varshar16 force-pushed the wip-integrate-cephadm-with-volume-nfs branch from 1813055 to 022060f Compare April 22, 2020 17:48
@varshar16
Copy link
Contributor Author

varshar16 commented Apr 23, 2020

jenkins render docs

@ceph-jenkins
Copy link
Collaborator

ceph-jenkins commented Apr 23, 2020

Doc render available at http://docs.ceph.com/ceph-prs/34672/

sebastian-philipp
sebastian-philipp reviewed Apr 23, 2020
View reviewed changes
@sebastian-philipp
Copy link
Contributor

sebastian-philipp commented Apr 23, 2020

looks great!

@varshar16 varshar16 force-pushed the wip-integrate-cephadm-with-volume-nfs branch from 022060f to d838e23 Compare April 24, 2020 17:53
@varshar16
Copy link
Contributor Author

varshar16 commented Apr 24, 2020

jenkins render docs

@varshar16
Copy link
Contributor Author

varshar16 commented Apr 24, 2020

Please don't merge, it requires tests in teuthology.

jtlayton
jtlayton reviewed Apr 24, 2020
View reviewed changes
@jtlayton jtlayton self-assigned this Apr 24, 2020
@ajarr ajarr self-requested a review April 27, 2020 09:06
ajarr
ajarr reviewed Apr 27, 2020
View reviewed changes
Copy link
Contributor

@ajarr ajarr left a comment

Choose a reason for hiding this comment

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

can you squash commit, 2b010cd
into 3f128ad

@varshar16 varshar16 force-pushed the wip-integrate-cephadm-with-volume-nfs branch 2 times, most recently from a987983 to 9c8eace Compare April 27, 2020 11:33
@varshar16
Copy link
Contributor Author

varshar16 commented Apr 27, 2020

can you squash commit, 2b010cd
into 3f128ad

Done

mgfritch
mgfritch reviewed Apr 27, 2020
View reviewed changes
src/pybind/mgr/volumes/fs/nfs.py Outdated

def create_empty_rados_obj(self):
common_conf = 'conf-nfs'
common_conf = 'conf-nfs.{}'.format(self.cluster_id)
Copy link
Contributor

@mgfritch mgfritch Apr 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need this function?

An empty common config is created by the orchestrator:

ceph/src/pybind/mgr/cephadm/nfs.py

Line 78 in 63d690f

def create_rados_config_obj(self, clobber=False):

Copy link
Contributor Author

@varshar16 varshar16 Apr 28, 2020

Choose a reason for hiding this comment

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

Based on the discussion here, volume module creates common config.

mgfritch
mgfritch reviewed Apr 27, 2020
View reviewed changes
src/pybind/mgr/volumes/fs/nfs.py
def _update_common_conf(self, ex_id):
common_conf = 'conf-nfs'
def _update_common_conf(self, cluster_id, ex_id):
common_conf = 'conf-nfs.ganesha-{}'.format(cluster_id)
Copy link
Contributor

@mgfritch mgfritch Apr 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Can we use the ServiceDescription returned by describe_service?

ceph/src/pybind/mgr/orchestrator/_interface.py

Line 1382 in 63d690f

self.rados_config_location = rados_config_location

See the rados_config_location attribute returned by orch ls:

$ ceph orch ls nfs --format yaml
namespace: nfs-ns
placement:
  hosts:
  - hostname: host1
    name: ''
    network: ''
pool: cephfs.a.data
service_id: foo
service_name: nfs.foo
service_type: nfs
status:
  container_image_id: c10ee0889ebeef6d8b82acdb4064be9208e52d938dfae7603ca228f439c948b3
  container_image_name: docker.io/ceph/daemon-base:latest-master-devel
  created: '2020-04-27T14:42:40.890439'
  last_refresh: '2020-04-27T20:43:52.658584'
  rados_config_location: rados://cephfs.a.data/nfs-ns/conf-nfs.foo
  running: 1
  size: 1

Copy link
Contributor Author

@varshar16 varshar16 Apr 28, 2020

Choose a reason for hiding this comment

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

Again in the same doc, it says Orchestrator ServiceDescription rados_config_location should be removed

Copy link
Contributor

@sebastian-philipp sebastian-philipp Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

relates to #34592 According to the discussion I had with @epuertat , we should configure the dashboard using mon commands. using https://docs.ceph.com/ceph-prs/33886/api/mon_command_api/#dashboard-set-ganesha-clusters-rados-pool-namespace etc. cc @ceph/dashboard

Copy link
Contributor Author

@varshar16 varshar16 Apr 30, 2020

Choose a reason for hiding this comment

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

Afair we wanted dashboard to call up volume's nfs interface to set-up ganesha clusters. This interface creates a common pool and sets unique namespace for each instance. So why configure dashboard pool and namespace using mon commands?

Copy link
Contributor

@mgfritch mgfritch May 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Using mon commands will restrict to a single pool/namespace:
https://docs.ceph.com/docs/master/mgr/dashboard/#configuring-nfs-ganesha-in-the-dashboard

Whereas this PR is attempting to configure a namespace per NFS cluster ?

cc @sebastian-philipp @varshar16

Copy link
Member

@batrick batrick May 19, 2020

Choose a reason for hiding this comment

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

Yes, that's correct. Each cluster has its own set of configs. NFS clusters do not share anything except the pool.

Copy link
Contributor

@mgfritch mgfritch May 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

hrm, the documentation is unlcear, but it does appear that multiple namespaces can specified via the dashboard mon command (using a comma as the delim) --

ceph/src/pybind/mgr/dashboard/services/ganesha.py

Line 35 in 8bcfaf2

location_list = [l.strip() for l in location_list_str.split(",")]

Copy link
Contributor Author

@varshar16 varshar16 May 20, 2020

Choose a reason for hiding this comment

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

From doc, I see it can be set like this: $ ceph dashboard set-ganesha-clusters-rados-pool-namespace <cluster_id>:<pool_name>[/<namespace>](,<cluster_id>:<pool_name>[/<namespace>])*

It will need to be set every time new cluster is deployed along with old cluster values.

@varshar16 varshar16 force-pushed the wip-integrate-cephadm-with-volume-nfs branch from 9c8eace to 91ef8e9 Compare April 28, 2020 16:30
sebastian-philipp
sebastian-philipp reviewed May 4, 2020
View reviewed changes
@varshar16 varshar16 force-pushed the wip-integrate-cephadm-with-volume-nfs branch 3 times, most recently from feb620d to 02d6298 Compare May 6, 2020 10:21
varshar16 added 11 commits May 29, 2020 14:47
@varshar16
mgr/volumes: Make nfs create export interface idempotent
b3b83b0 
Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16
mgr/volumes/nfs: Delete all exports on cluster deletion
d12aed8 
Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16
mgr/volumes/nfs: Delete common config object on cluster deletion
0ee8439 
Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16
mgr/volumes: Rearrange nfs export interface
8a84744 
Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16
vstart: Instead of CACHEINODE use MDCACHE
2c50c9b 
CACHEINODE will be deprecated soon. Instead use MDCACHE.

Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16
mgr/volumes/nfs: Remove type option from export create interface
9bcd7b6 
Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16
qa/cephfs: Add tests for nfs
ab41951 
Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16
mgr/fs/nfs: Use check_mon_command() instead of mon_command()
fee3f25 
check_mon_command() checks the return code of mon command.

Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16
mgr/volumes/nfs: Fix incorrect read only access_type value
923d181 
Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16
mgr/volumes/nfs: Fix idempotency of cluster and export commands
b8ce61e 
Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16
qa/tasks/cephfs: Add tests for nfs exports
af3b925 
Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16 varshar16 force-pushed the wip-integrate-cephadm-with-volume-nfs branch from 176fbc3 to 719d0c3 Compare May 29, 2020 09:41
@varshar16
Copy link
Contributor Author

varshar16 commented May 29, 2020

I have removed the pool deletion patch for now.
On testing this locally the pool is present before creating the conf object.
Here is the mgr log and changes.

@varshar16
qa/tasks/cephfs: Don't enable cephadm in TestNFS
1477c98 
Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16
Copy link
Contributor Author

varshar16 commented May 29, 2020

Failed again, auth delete used to pass earlier.
mgr.log

2020-05-29T13:02:27.286+0000 7fab244b7700  0 [volumes DEBUG volumes.fs.nfs] Export deleted: %url "rados://nfs-ganesha/test/export-1"


2020-05-29T13:02:27.286+0000 7fab244b7700  1 -- 172.21.15.27:0/1707518105 --> [v2:172.21.15.27:3300/0,v1:172.21.15.27:6789/0] -- mon_command({"prefix": "auth del", "entity": "client.test1"} v 0) v1 -- 0x5609d41f0f00 con 0x5609d3f1a000
2020-05-29T13:02:27.286+0000 7fab4730a700  1 --2- 172.21.15.27:0/1707518105 >> [v2:172.21.15.27:3300/0,v1:172.21.15.27:6789/0] conn(0x5609d3f1a000 0x5609d3f20000 secure :-1 s=THROTTLE_DONE pgs=154 cs=0 l=1 rx=0x5609cde39350 tx=0x5609cea784b0).handle_read_frame_epilogue_main read frame epilogue bytes=32
2020-05-29T13:02:27.286+0000 7fab43b03700  1 -- 172.21.15.27:0/1707518105 <== mon.0 v2:172.21.15.27:3300/0 153 ==== mon_command_ack([{"prefix": "auth del", "entity": "client.test1"}]=-13 access denied v0) v1 ==== 95+0+0 (secure 0 0 0) 0x5609d3fc36c0 con 0x5609d3f1a000
2020-05-29T13:02:27.286+0000 7fab28cba700  1 -- 172.21.15.27:0/1707518105 --> [v2:172.21.15.27:3300/0,v1:172.21.15.27:6789/0] -- mon_get_version(what=osdmap handle=28) v1 -- 0x5609cdda2b00 con 0x5609d3f1a000
2020-05-29T13:02:27.286+0000 7fab4730a700  1 --2- 172.21.15.27:0/1707518105 >> [v2:172.21.15.27:3300/0,v1:172.21.15.27:6789/0] conn(0x5609d3f1a000 0x5609d3f20000 secure :-1 s=THROTTLE_DONE pgs=154 cs=0 l=1 rx=0x5609cde39350 tx=0x5609cea784b0).handle_read_frame_epilogue_main read frame epilogue bytes=32
2020-05-29T13:02:27.286+0000 7fab43b03700  1 -- 172.21.15.27:0/1707518105 <== mon.0 v2:172.21.15.27:3300/0 154 ==== mon_get_version_reply(handle=28 version=29) v2 ==== 24+0+0 (secure 0 0 0) 0x5609cdda2b00 con 0x5609d3f1a000
2020-05-29T13:02:27.286+0000 7fab42300700 10 MonCommandCompletion::finish()
2020-05-29T13:02:27.286+0000 7fab244b7700  0 [volumes DEBUG root] mon_command: 'auth del' -> -13 in 0.001s
2020-05-29T13:02:27.286+0000 7fab244b7700  0 [volumes WARNING volumes.fs.nfs] Failed to delete exports
2020-05-29T13:02:27.286+0000 7fab244b7700 20 mgr ~Gil Destroying new thread state 0x5609d3fe8900
2020-05-29T13:02:27.286+0000 7fab244b7700 -1 mgr.server reply reply (22) Invalid argument auth del failed: access denied retval: -13
2020-05-29T13:02:27.286+0000 7fab244b7700  1 -- [v2:172.21.15.27:6800/352952147,v1:172.21.15.27:6801/352952147] --> 172.21.15.27:0/4282170931 -- mgr_command_reply(tid 0: -22 auth del failed: access denied retval: -13) v1 -- 0x5609cdda3a20 con 0x5609d41eec00

http://qa-proxy.ceph.com/teuthology/varsha-2020-05-29_12:44:53-rados-wip-integrate-cephadm-with-volume-nfs-distro-basic-smithi/

varshar16 added 2 commits May 29, 2020 23:24
@varshar16
mgr/nfs: Instead of 'auth del' use 'auth rm'
87c517a 
`mgr` profile allows 'auth rm'. Use it instead of 'auth del' which is not
allowed.

Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16
qa/tasks/cephfs: Enable multiple exports tests
b2adff1 
Signed-off-by: Varsha Rao <varao@redhat.com>
@varshar16 varshar16 force-pushed the wip-integrate-cephadm-with-volume-nfs branch from 719d0c3 to b2adff1 Compare May 29, 2020 18:08
@varshar16
Copy link
Contributor Author

varshar16 commented May 30, 2020

Tests have passed: http://qa-proxy.ceph.com/teuthology/varsha-2020-05-30_04:09:11-rados-wip-integrate-cephadm-with-volume-nfs-distro-basic-smithi/
@sebastian-philipp @batrick

@batrick batrick merged commit 9442abd into ceph:master Jun 1, 2020
@varshar16 varshar16 mentioned this pull request Jun 9, 2020
Merged
@tchaikov
Copy link
Contributor

tchaikov commented Jun 17, 2020

@varshar16 could you take a look at https://tracker.ceph.com/issues/46046 ?

@varshar16
Copy link
Contributor Author

varshar16 commented Jun 17, 2020

@varshar16 could you take a look at https://tracker.ceph.com/issues/46046 ?

@tchaikov Yes, looking into it.

@varshar16 varshar16 deleted the wip-integrate-cephadm-with-volume-nfs branch July 16, 2020 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@batrick batrick batrick approved these changes

@ajarr ajarr ajarr approved these changes

@mgfritch mgfritch Awaiting requested review from mgfritch

@sebastian-philipp sebastian-philipp Awaiting requested review from sebastian-philipp

@jtlayton jtlayton Awaiting requested review from jtlayton

Assignees

No one assigned

Labels

cephadm cephfs Ceph File System needs-review wip-swagner-testing My Teuthology tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@varshar16 @ceph-jenkins @sebastian-philipp @batrick @tchaikov @mgfritch @ajarr @jtlayton