Skip to content

mgr/volumes/nfs: drop type param during cluster create#40411

Merged
liewegas merged 2 commits intoceph:masterfrom
mgfritch:nfs_cluster_create_notype
Apr 16, 2021
Merged

mgr/volumes/nfs: drop type param during cluster create#40411
liewegas merged 2 commits intoceph:masterfrom
mgfritch:nfs_cluster_create_notype

Conversation

@mgfritch
Copy link
Contributor

@mgfritch mgfritch commented Mar 25, 2021 •
edited
Loading

PR #37600 introduced support for both cephfs and rgw exports
to be configured using a single nfs-ganesha cluster

Fixes: https://tracker.ceph.com/issues/50369
Signed-off-by: Michael Fritch mfritch@suse.com

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 api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@mgfritch mgfritch requested review from a team and varshar16 March 25, 2021 13:39
@batrick batrick added DNM and removed needs-qa labels Mar 26, 2021
@batrick
Copy link
Member

batrick commented Mar 26, 2021

Pending discussion with @varshar16

@mgfritch mgfritch mentioned this pull request Mar 29, 2021
Merged
mgfritch added a commit to mgfritch/aquarium that referenced this pull request Mar 29, 2021
@mgfritch
gravel: ctrl/nfs: fixup todo comment
c7a6fef 
clean-up depends on outcome of:
ceph/ceph#40411

Signed-off-by: Michael Fritch <mfritch@suse.com>
mgfritch added a commit to mgfritch/aquarium that referenced this pull request Mar 29, 2021
@mgfritch
gravel: ctrl/nfs: fixup todo comment
4fb1bab 
pending: ceph/ceph#40411

Signed-off-by: Michael Fritch <mfritch@suse.com>
@mgfritch mgfritch mentioned this pull request Mar 29, 2021
Merged
AvengerMoJo pushed a commit to AvengerMoJo/aquarium that referenced this pull request Mar 30, 2021
@mgfritch
gravel: ctrl/nfs: fixup todo comment
b674204 
pending: ceph/ceph#40411

Signed-off-by: Michael Fritch <mfritch@suse.com>
@sebastian-philipp
Copy link
Contributor

sebastian-philipp commented Apr 7, 2021

after a small chat with @guits I somewhat tend to agree with this PR as it makes the deployment a bit easier and flexible. But I might miss something here.

@varshar16
Copy link
Contributor

varshar16 commented Apr 7, 2021

after a small chat with @guits I somewhat tend to agree with this PR as it makes the deployment a bit easier and flexible. But I might miss something here.

One issue is backward compatibility and other is related to RGW block in common config. https://pad.ceph.com/p/nfs

@mgfritch
Copy link
Contributor Author

mgfritch commented Apr 12, 2021

after a small chat with @guits I somewhat tend to agree with this PR as it makes the deployment a bit easier and flexible. But I might miss something here.

One issue is backward compatibility

are there existing consumers of this cli? maybe doc/readme is sufficient?

and other is related to RGW block in common config. https://pad.ceph.com/p/nfs

.. address the RGW block in another PR?

removing the required positional arg avoids the need to maintain backward compat ... and we could easily re-add it as a required named arg if we discover it's needed later ...

@liewegas liewegas added needs-qa and removed DNM labels Apr 13, 2021
@mgfritch mgfritch force-pushed the nfs_cluster_create_notype branch from dfa6c9d to 02189f7 Compare April 13, 2021 17:11
@mgfritch
Copy link
Contributor Author

mgfritch commented Apr 13, 2021

jenkins test api

@varshar16
Copy link
Contributor

varshar16 commented Apr 14, 2021

after a small chat with @guits I somewhat tend to agree with this PR as it makes the deployment a bit easier and flexible. But I might miss something here.

One issue is backward compatibility

are there existing consumers of this cli? maybe doc/readme is sufficient?

I am fine with documenting about it.

and other is related to RGW block in common config. https://pad.ceph.com/p/nfs

.. address the RGW block in another PR?

fine with this too.

varshar16
varshar16 approved these changes Apr 14, 2021
View reviewed changes
Copy link
Contributor

@varshar16 varshar16 left a comment

Choose a reason for hiding this comment

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

Minor nit, otherwise looks good.

PendingReleaseNotes
Comment on lines +14 to +18
* volumes/nfs: The ``cephfs`` cluster type has been removed from the
``nfs cluster create`` subcommand.

Copy link
Contributor

@varshar16 varshar16 Apr 14, 2021

Choose a reason for hiding this comment

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

Add a line saying both cephfs and rgw is supported by nfs cluster create subcommand.

Copy link
Contributor Author

@mgfritch mgfritch Apr 14, 2021

Choose a reason for hiding this comment

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

added verbiage similar to this to the PendingReleaseNotes.

@mgfritch mgfritch force-pushed the nfs_cluster_create_notype branch from 02189f7 to 649c8f6 Compare April 14, 2021 20:56
@batrick
Copy link
Member

batrick commented Apr 14, 2021

Is this to be backported? If so, please create a tracker ticket.

@batrick
Copy link
Member

batrick commented Apr 14, 2021

jenkins test api

mgfritch added 2 commits April 14, 2021 18:21
@mgfritch
mgr/volumes/nfs: drop type param during cluster create
e9b2c38 
PR ceph#37600 introduced support for both `cephfs` and `rgw` exports
to be configured using a single nfs-ganesha cluster

Fixes: https://tracker.ceph.com/issues/50369
Signed-off-by: Michael Fritch <mfritch@suse.com>
@mgfritch
doc: add note about removal of the cephfs nfs cluster type
81ae874 
Fixes: https://tracker.ceph.com/issues/50369
Signed-off-by: Michael Fritch <mfritch@suse.com>
@mgfritch mgfritch force-pushed the nfs_cluster_create_notype branch from 649c8f6 to 81ae874 Compare April 15, 2021 00:22
@mgfritch
Copy link
Contributor Author

mgfritch commented Apr 15, 2021

Is this to be backported? If so, please create a tracker ticket.

yeah my bad, tracker is here: https://tracker.ceph.com/issues/50369

I'd be good if we could get this into the next Pacific release.

@mgfritch
Copy link
Contributor Author

mgfritch commented Apr 15, 2021

jenkins test api

1 similar comment
@varshar16
Copy link
Contributor

varshar16 commented Apr 15, 2021

jenkins test api

@batrick
Copy link
Member

batrick commented Apr 16, 2021

https://tracker.ceph.com/projects/cephfs/wiki/Master#2021-Apr-15

@batrick
Copy link
Member

batrick commented Apr 16, 2021

jenkins test api

@batrick
Copy link
Member

batrick commented Apr 16, 2021

ready to merge from cephfs side

@liewegas liewegas merged commit 3a7c72a into ceph:master Apr 16, 2021
@mgfritch mgfritch deleted the nfs_cluster_create_notype branch April 16, 2021 15:50
@varshar16 varshar16 mentioned this pull request Apr 22, 2021
Merged
@jmolmo jmolmo mentioned this pull request Apr 23, 2021
Merged
@mgfritch mgfritch mentioned this pull request Apr 23, 2021
Merged
3 tasks
@varshar16 varshar16 mentioned this pull request May 18, 2021
Merged
mgfritch added a commit to mgfritch/aquarium that referenced this pull request May 25, 2021
@mgfritch
ceph.git: update to the latest ceph pacific
79373d5 
- Ceph Pacific v16.2.4 + latest backports

- includes upstream backport of:
  - ceph/ceph#40411
  - ceph/ceph#40555
  - ceph/ceph#40463

Signed-off-by: Michael Fritch <mfritch@suse.com>
mgfritch added a commit to mgfritch/aquarium that referenced this pull request May 25, 2021
@mgfritch
gravel: remove nfs cluster type
84714a6 
cluster `type` was removed by:
ceph/ceph#40411

Signed-off-by: Michael Fritch <mfritch@suse.com>
mgfritch added a commit to mgfritch/aquarium that referenced this pull request May 25, 2021
@mgfritch
ceph.git: update to the latest ceph pacific
af5d041 
- Ceph Pacific v16.2.4 + latest backports

- includes upstream backport of:
  - ceph/ceph#40411
  - ceph/ceph#40555
  - ceph/ceph#40463

Resolves: aquarist-labs#389
Signed-off-by: Michael Fritch <mfritch@suse.com>
mgfritch added a commit to mgfritch/aquarium that referenced this pull request May 25, 2021
@mgfritch
gravel: remove nfs cluster type
1be1675 
cluster `type` was removed by:
ceph/ceph#40411

Signed-off-by: Michael Fritch <mfritch@suse.com>
mgfritch added a commit to mgfritch/aquarium that referenced this pull request May 25, 2021
@mgfritch
ceph.git: update to the latest ceph pacific
7e9bd65 
- Ceph Pacific v16.2.4 + latest backports
- includes upstream backport of:
  - ceph/ceph#40411
  - ceph/ceph#40555
  - ceph/ceph#40463

Resolves: aquarist-labs#389
Signed-off-by: Michael Fritch <mfritch@suse.com>
mgfritch added a commit to mgfritch/aquarium that referenced this pull request May 25, 2021
@mgfritch
gravel: remove nfs cluster type
ea5d70b 
cluster `type` was removed by:
ceph/ceph#40411

Signed-off-by: Michael Fritch <mfritch@suse.com>
@mgfritch mgfritch mentioned this pull request May 25, 2021
Merged
3 tasks
@smithfarm smithfarm mentioned this pull request Nov 5, 2021
Merged
smithfarm added a commit to smithfarm/sesdev that referenced this pull request Nov 5, 2021
@smithfarm
templates/cephadm/deployment_day_2: fix NFS cluster create command
4402d58 
At some point, upstream obviously changed the syntax of the "ceph nfs cluster
create" command in octopus, thereby breaking "sesdev create pacific" and "sesdev
create ses7".

Later, this syntax change got reverted [1], but since this revert was made
possible by a big feature [2], so far it has been backported only to pacific [3]

[1] ceph/ceph#40411
[2] ceph/ceph#37600
[3] ceph/ceph#41005

Fixes: SUSE#611

Signed-off-by: Nathan Cutler <ncutler@suse.com>
smithfarm added a commit to smithfarm/sesdev that referenced this pull request Nov 5, 2021
@smithfarm
templates/cephadm/deployment_day_2: fix NFS cluster create command
923b950 
At some point, upstream obviously changed the syntax of the "ceph nfs cluster
create" command in octopus, thereby breaking "sesdev create pacific" and "sesdev
create ses7".

Later, this syntax change got reverted [1], but since this revert was made
possible by a big feature [2], so far it has been backported only to pacific [3]

[1] ceph/ceph#40411
[2] ceph/ceph#37600
[3] ceph/ceph#41005

Fixes: SUSE#611

Signed-off-by: Nathan Cutler <ncutler@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liewegas liewegas liewegas approved these changes

+1 more reviewer

@varshar16 varshar16 varshar16 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cephadm cephfs Ceph File System documentation needs-qa nfs pybind tests wip-sage-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mgfritch @batrick @sebastian-philipp @varshar16 @liewegas