Skip to content

Adds NFS support#574

Merged
UtkarshBhatthere merged 7 commits intocanonical:mainfrom
claudiubelu:add-nfs
Jul 17, 2025
Merged

Adds NFS support#574
UtkarshBhatthere merged 7 commits intocanonical:mainfrom
claudiubelu:add-nfs

Conversation

@claudiubelu
Copy link
Copy Markdown
Contributor

@claudiubelu claudiubelu commented Jun 24, 2025

Description

Adds support for enabling NFS clusters in microceph.

Adds the following commands:

  • microceph enable nfs --cluster-id <cluster-id> [--bind-address <ip-address>] [--port <port-num>] [--v4-min-version 0/1/2] [--target <server>] [--wait <bool>]
  • microceph disable nfs --cluster-id <cluster-id> [--target <server>]

Adds the services/nfs API endpoint (PUT / DELETE).

Adds the following tables:

  • service_groups: used to track microceph service clusters.
  • grouped_services: used to track clustered services running on a particular server.

Added dependencies in the snap:

  • nfs-ganesha
  • nfs-ganesha-ceph
  • nfs-ganesha-rados-grace

Type of change

  • New feature (non-breaking change which adds functionality)

How has this been tested?

Note

All functional changes should accompany corresponding tests (unit tests, functional tests, etc.).

Please describe the addition/modification of tests done to verify this change. Also list any
relevant details for your test configuration.

Contributor checklist

Please check that you have:

  • self-reviewed the code in this PR
  • added code comments, particularly in less straightforward areas
  • checked and added or updated relevant documentation
  • checked and added or updated relevant release notes
  • added tests to verify effectiveness of this change

@claudiubelu claudiubelu marked this pull request as draft June 24, 2025 07:02
Comment thread microceph/cmd/microceph/enable_nfs.go Outdated
Comment thread microceph/ceph/service_placement_nfs.go
Comment thread microceph/ceph/service_placement_nfs.go Outdated
Comment thread microceph/ceph/run.go
Comment thread microceph/ceph/run.go Outdated
Comment thread microceph/ceph/run.go Outdated
Comment thread microceph/ceph/service_placement_nfs.go
Comment thread microceph/ceph/run.go Outdated
Copy link
Copy Markdown
Contributor

@UtkarshBhatthere UtkarshBhatthere left a comment

Choose a reason for hiding this comment

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

first round of reviews: some comments and questions. Thanks @claudiubelu!

@UtkarshBhatthere UtkarshBhatthere added the enhancement New feature or request label Jun 24, 2025
@UtkarshBhatthere
Copy link
Copy Markdown
Contributor

  1. Not providing a target name does not default to the local host.
  2. The service name should default to nfs rather than nfs-ganesha (as we enable nfs not nfs-ganesha).
root@microceph1:~# microceph enable nfs --cluster-id zuzu
Error: failed placing service nfs: Failed to get cluster member for request target name "0.0.0.0:2049": CoreClusterMember not found
root@microceph1:~# microceph enable nfs --cluster-id zuzu --target microceph1
root@microceph1:~# microceph status
MicroCeph deployment summary:
- microceph1 (10.17.213.183)
  Services: mds, mgr, mon, nfs-ganesha, osd
  Disks: 1
- microceph2 (10.17.213.150)
  Services: mds, mgr, mon, osd
  Disks: 1
- microceph3 (10.17.213.215)
  Services: mds, mgr, mon, osd
  Disks: 1

@claudiubelu claudiubelu force-pushed the add-nfs branch 3 times, most recently from d71e52f to 265c88c Compare July 2, 2025 10:52
@gboutry
Copy link
Copy Markdown
Contributor

gboutry commented Jul 7, 2025

@UtkarshBhatthere Any updates? Can you review please?

@UtkarshBhatthere
Copy link
Copy Markdown
Contributor

@gboutry I will do a detailed review today

Comment thread microceph/ceph/config.go Outdated
Comment thread microceph/ceph/configwriter.go Outdated
Comment thread microceph/ceph/configwriter_test.go
Comment thread microceph/ceph/configwriter_test.go
Comment thread microceph/api/types/services.go
Comment thread microceph/ceph/service_placement_nfs.go Outdated
Comment thread microceph/ceph/service_placement_nfs.go Outdated
Comment thread microceph/ceph/service_placement_nfs.go Outdated
Comment thread microceph/ceph/service_placement_nfs.go Outdated
Comment thread microceph/ceph/service_placement_nfs.go Outdated
Comment thread microceph/ceph/nfs.go Outdated
@UtkarshBhatthere
Copy link
Copy Markdown
Contributor

Enabling a second daemon (on a multinode microceph cluster fails)

@claudiubelu

Node 1

root@secondary:~# microceph enable nfs --cluster-id manila
root@secondary:~#

Node 2

# enablement on second cluster fails
root@primary:~# microceph enable nfs --cluster-id manila
Error: failed placing service nfs: failed to add DB record for nfs: conflicting service group configurations
root@primary:~#

Database Dump

root@primary:~# microceph cluster sql "select * from grouped_services"
+----+------------------+-----------+---------------------------------------------+
| id | service_group_id | member_id |                    info                     |
+----+------------------+-----------+---------------------------------------------+
| 1  | 1                | 2         | {"bind_address":"0.0.0.0","bind_port":2049} |
+----+------------------+-----------+---------------------------------------------+
root@primary:~# microceph cluster sql "select * from service_groups"
+----+---------+----------+----------------------+
| id | service | group_id |        config        |
+----+---------+----------+----------------------+
| 1  | nfs     | manila   | {"v4_min_version":1} |
+----+---------+----------+----------------------+
root@primary:~#

@claudiubelu claudiubelu force-pushed the add-nfs branch 7 times, most recently from 1eee56c to 71b1359 Compare July 12, 2025 12:47
Adds the following packages to the microceph snap:

- nfs-ganesha
- nfs-ganesha-ceph
- nfs-ganesha-rados-grace

Adds nfs-ganesha startup script.

Signed-off-by: Claudiu Belu <cbelu@cloudbasesolutions.com>
Adds the service_groups and grouped_services database tables.

Adds the "services/nfs" API endpoint (PUT / DELETE).

The NFS service can now be enabled on a host. When enabled, microceph
will configure the nfs-ganesha service, and will create related ceph
client, rados pools (".nfs", ".nfs.metadata"), add the node to the
Shared Grace Management Database, and add the service to the
service_groups and grouped_services database as needed.

Signed-off-by: Claudiu Belu <cbelu@cloudbasesolutions.com>
Adds the following commands:

- microceph enable nfs --cluster-id <cluster-id> [--bind-address <ip-address>] [--port <port-num>] [--v4-min-version 0/1/2] [--target <server>] [--wait <bool>]
- microceph disable nfs --cluster-id <cluster-id> [--target <server>]

The commands will use the "services/nfs" API endpoint.

Signed-off-by: Claudiu Belu <cbelu@cloudbasesolutions.com>
The test will create a single node microceph cluster, enable nfs,
create a NFS FS and export, mount it, and create a file on the mount.

Signed-off-by: Claudiu Belu <cbelu@cloudbasesolutions.com>
The test will create a multinode microceph cluster, enable nfs,
create a NFS FS and export it, mount it, and create a file on the
mount.

Signed-off-by: Claudiu Belu <cbelu@cloudbasesolutions.com>
Signed-off-by: utkarsh bhatt <utkarsh.bhatt@canonical.com>
@UtkarshBhatthere
Copy link
Copy Markdown
Contributor

root@primary:~# microceph status
MicroCeph deployment summary:
- primary (10.206.113.174)
  Services: mds, mgr, mon, osd
  Disks: 4
- secondary (10.206.113.47)
  Services: mds, mgr, mon, nfs.manila
  Disks: 0
root@primary:~# microceph enable nfs
Error: please provide a cluster ID using the `--cluster-id` flag
root@primary:~# microceph enable nfs --cluster-id manila
root@primary:~# microceph status
MicroCeph deployment summary:
- primary (10.206.113.174)
  Services: mds, mgr, mon, nfs.manila, osd
  Disks: 4
- secondary (10.206.113.47)
  Services: mds, mgr, mon, nfs.manila
  Disks: 0
root@primary:~#

Copy link
Copy Markdown
Contributor

@UtkarshBhatthere UtkarshBhatthere left a comment

Choose a reason for hiding this comment

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

lgtm. thanks @claudiubelu

@UtkarshBhatthere UtkarshBhatthere marked this pull request as ready for review July 14, 2025 09:22
Copy link
Copy Markdown
Collaborator

@sabaini sabaini left a comment

Choose a reason for hiding this comment

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

Thanks @claudiubelu ! Added some commentary

Comment thread microceph/ceph/service_placement_nfs.go
Comment thread microceph/cmd/microceph/enable_nfs.go Outdated
Comment thread microceph/ceph/nfs.go
Comment thread microceph/ceph/nfs.go
@claudiubelu
Copy link
Copy Markdown
Contributor Author

Please also consider adding a functional test and documentation.

Added functional tests (single node and multinode). Will add documentation.

A general request, it would be great if we could get more logging of key operations.

That would entail logging of errors, as well as debug logs e.g. for entry and exit of public functions -- especially on the microcephd side

Added more logging, and reverts to EnableNFS.

1. The output of `microceph status` does not report nfs service.

Wasn't sure how you envisioned this to be reported exactly, but I see you've added a commit for that, and now it reports nfs.<cluster_id>. Thanks!

2. There is a mismatch in the underlying service name (`snap.microceph.nfs-ganesha`) and the microceph command name `microceph enable nfs`. In my opinion it should be called `nfs` everywhere accessible to users (CLI commands, snapcraft service name, etc) . Ans should use `nfs-ganesha` in implementation (like actual service name, and expected paths etc)

Done, now the service name is snap.microceph.nfs.

2. There is a mismatch in the underlying service name (`snap.microceph.nfs-ganesha`) and the microceph command name `microceph enable nfs`. In my opinion it should be called `nfs` everywhere accessible to users (CLI commands, snapcraft service name, etc) . Ans should use `nfs-ganesha` in implementation (like actual service name, and expected paths etc)

Enabling a second daemon (on a multinode microceph cluster fails)

Resolved, added another functional test for the multinode scenario. It passes.

@UtkarshBhatthere UtkarshBhatthere requested a review from sabaini July 14, 2025 19:24
@claudiubelu claudiubelu changed the title WIP: Adds NFS Adds NFS support Jul 15, 2025
Copy link
Copy Markdown
Collaborator

@sabaini sabaini left a comment

Choose a reason for hiding this comment

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

Thanks @claudiubelu!
We will need docs for this too but from a functionality perspective this looks good.

Signed-off-by: Claudiu Belu <cbelu@cloudbasesolutions.com>
@claudiubelu
Copy link
Copy Markdown
Contributor Author

Thanks @claudiubelu! We will need docs for this too but from a functionality perspective this looks good.

Added the documentation update here #583, so we can discuss any changes needed to it there, so we can proceed with merging this.

@UtkarshBhatthere UtkarshBhatthere merged commit 63d1eeb into canonical:main Jul 17, 2025
19 checks passed
@UtkarshBhatthere
Copy link
Copy Markdown
Contributor

Thanks for the amazing contribution @claudiubelu 🚀

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants