Skip to content

mgr/cephadm: adding support for nvmeof#50423

Merged
adk3798 merged 4 commits intoceph:mainfrom
rkachach:fix_issue_nmveof
Jul 25, 2023
Merged

mgr/cephadm: adding support for nvmeof#50423
adk3798 merged 4 commits intoceph:mainfrom
rkachach:fix_issue_nmveof

Conversation

@rkachach
Copy link
Contributor

@rkachach rkachach commented Mar 7, 2023 •
edited
Loading

To test the changes on this branch:

  1. Use the image from registry.hub.docker.com/rkachach/ceph-nvmeof:v1.0 to bootstrap your cluster
  2. One the cluster is up, healthy and running create a pool named rbd by ceph osd pool create rbd
  3. from a cephadm shell, use the following spec to create the nvmeof service as following (saved in nvmeof.yaml i.e):
service_type: nvmeof
service_id: nvmeof.test
placement:
  hosts:
    - ceph-node-0
spec:
  pool: rbd
  name: myname
  group: mygroup
  1. ceph orch apply -i nvmeof.yaml

To test the daemon you we can use the quay.io/ceph/nvmeof-cli:0.0.1, i.e:

podman run --rm -it "quay.io/ceph/nvmeof-cli:0.0.1" --server-address 192.168.100.100 --server-port 5500 get_subsystems

Output:

INFO:__main__:Get subsystems:
[
    {
        "nqn": "nqn.2014-08.org.nvmexpress.discovery",
        "subtype": "Discovery",
        "listen_addresses": [],
        "allow_any_host": true,
        "hosts": []
    }
]

https://tracker.ceph.com/issues/61929

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
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 cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@rkachach rkachach force-pushed the fix_issue_nmveof branch 2 times, most recently from 9b0d1fa to 4b8c3af Compare March 13, 2023 08:56
@ktdreyer ktdreyer added the needs-quincy-backport backport required for quincy label Mar 16, 2023
@github-actions
Copy link

github-actions bot commented Mar 30, 2023

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@epuertat epuertat mentioned this pull request May 21, 2023
Closed
@rkachach rkachach force-pushed the fix_issue_nmveof branch from 99f96d1 to 285ddbd Compare June 1, 2023 08:51
@rkachach rkachach force-pushed the fix_issue_nmveof branch 2 times, most recently from eaf477e to 2f7756a Compare June 5, 2023 15:20
@rkachach rkachach force-pushed the fix_issue_nmveof branch 2 times, most recently from 2784483 to 6c218f4 Compare June 12, 2023 15:58
@rkachach rkachach added the DNM label Jun 21, 2023
baum
baum reviewed Jun 28, 2023
View reviewed changes
baum
baum reviewed Jun 30, 2023
View reviewed changes
baum
baum reviewed Jun 30, 2023
View reviewed changes
@rkachach
Copy link
Contributor Author

rkachach commented Jul 5, 2023

jenkins retest this please

@rkachach rkachach force-pushed the fix_issue_nmveof branch from 17936e4 to 1a4b212 Compare July 5, 2023 13:52
@rkachach
Copy link
Contributor Author

rkachach commented Jul 5, 2023

jenkins test make check

@rkachach
Copy link
Contributor Author

rkachach commented Jul 5, 2023

jenkins test dashboard cephadm

@rkachach
Copy link
Contributor Author

rkachach commented Jul 6, 2023

jenkins test make check

@rkachach
Copy link
Contributor Author

rkachach commented Jul 6, 2023

jenkins test api

@rkachach
Copy link
Contributor Author

rkachach commented Jul 6, 2023

jenkins test make check

epuertat
epuertat reviewed Jul 7, 2023
View reviewed changes
Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

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

Good progress. I think it's almost there! Just a few comments.

@rkachach rkachach force-pushed the fix_issue_nmveof branch 2 times, most recently from 24bd67f to 2e9c158 Compare July 7, 2023 13:32
phlogistonjohn
phlogistonjohn reviewed Jul 10, 2023
View reviewed changes
Copy link
Contributor

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

I just skimmed thru this PR and nothing major jumps out at me. I would recommend squashing your patches together since the 3 later patches just fix things in your new code. But it's not a major requirement.

Much of the patch is just plumbing the new types through. The interesting bits are the extra parameters to make the container function and while I don't love seeing extra caps added, if they're needed they're needed.

adk3798
adk3798 reviewed Jul 10, 2023
View reviewed changes
Copy link
Contributor

@adk3798 adk3798 left a comment

Choose a reason for hiding this comment

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

Looks like it's mostly there. Still unsure about how SSL is working with this (I see there are options to pass in certs in the spec, just not sure how they're used) and it would be nice to clean up the keyring stuff, but the overall structure seems pretty good.

@rkachach
Adding more ports needed by nvmeof daemon
1dcbf61 
Signed-off-by: Redouane Kachach <rkachach@redhat.com>
@rkachach
Copy link
Contributor Author

rkachach commented Jul 11, 2023

I just skimmed thru this PR and nothing major jumps out at me. I would recommend squashing your patches together since the 3 later patches just fix things in your new code. But it's not a major requirement.

Much of the patch is just plumbing the new types through. The interesting bits are the extra parameters to make the container function and while I don't love seeing extra caps added, if they're needed they're needed.

Thank you for reviewing the code @phlogistonjohn, I'll be squashing the changes are in fact most of the commits are small changes to fix UT and other changes from the reviews. The caps are needed by the nvmeof daemon to function so at this moment we have to add them.

@rkachach
Addressing reviewers comments
5a043fe 
Signed-off-by: Redouane Kachach <rkachach@redhat.com>
@rkachach rkachach changed the title [WIP] mgr/cephadm: adding support for nvmeof mgr/cephadm: adding support for nvmeof Jul 11, 2023
@rkachach rkachach marked this pull request as ready for review July 11, 2023 08:03
@rkachach rkachach requested a review from a team as a code owner July 11, 2023 08:03
@rkachach rkachach requested review from adk3798 and epuertat July 11, 2023 08:03
epuertat
epuertat reviewed Jul 11, 2023
View reviewed changes
@rkachach rkachach requested a review from epuertat July 11, 2023 12:39
@rkachach rkachach mentioned this pull request Jul 11, 2023
Closed
@adk3798
Copy link
Contributor

adk3798 commented Jul 12, 2023

https://pulpito.ceph.com/adking-2023-07-12_03:23:58-orch:cephadm-wip-adk-cephadm-nvmeof-distro-default-smithi/

Obviously, we don't have a test for deploying nvme-of itself yet, but did a run to make sure this wasn't breaking anything. Only failure was a known issue (https://tracker.ceph.com/issues/59704) so the run was clean.

@rkachach
Copy link
Contributor Author

rkachach commented Jul 13, 2023

https://pulpito.ceph.com/adking-2023-07-12_03:23:58-orch:cephadm-wip-adk-cephadm-nvmeof-distro-default-smithi/

Obviously, we don't have a test for deploying nvme-of itself yet, but did a run to make sure this wasn't breaking anything. Only failure was a known issue (https://tracker.ceph.com/issues/59704) so the run was clean.

Awesome, thank you very much @adk3798 for taking care of this.

@rkachach rkachach force-pushed the fix_issue_nmveof branch 2 times, most recently from ba96589 to f1eac99 Compare July 14, 2023 09:46
@rkachach
Adding support for extra args by using the field tgt_cmd_extra_args
050c0f9 
Signed-off-by: Redouane Kachach <rkachach@redhat.com>
@rkachach
Copy link
Contributor Author

rkachach commented Jul 14, 2023

jenkins test make check

@rkachach rkachach removed the DNM label Jul 17, 2023
@adk3798
Copy link
Contributor

adk3798 commented Jul 24, 2023

https://pulpito.ceph.com/adking-2023-07-19_13:28:53-orch:cephadm-wip-adk4-testing-2023-07-18-1038-distro-default-smithi/

3 Failures, 5 Dead jobs

Overall, nothing to block merging

@adk3798
Copy link
Contributor

adk3798 commented Jul 24, 2023

jenkins retest this please

adk3798
adk3798 approved these changes Jul 25, 2023
View reviewed changes
Copy link
Contributor

@adk3798 adk3798 left a comment

Choose a reason for hiding this comment

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

This still needs follow up work, but as @rkachach is going to be out for some time, going to merge this to main and fix the keyring issue and add proper testing in a follow up

@adk3798 adk3798 merged commit 19b6eaf into ceph:main Jul 25, 2023
@adk3798 adk3798 mentioned this pull request Aug 15, 2023
Closed
14 tasks
@adk3798 adk3798 mentioned this pull request Aug 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@baum baum baum left review comments

@idryomov idryomov idryomov left review comments

@phlogistonjohn phlogistonjohn phlogistonjohn left review comments

@adk3798 adk3798 adk3798 approved these changes

@epuertat epuertat Awaiting requested review from epuertat

Assignees

No one assigned

Labels

cephadm needs-quincy-backport backport required for quincy orchestrator pybind wip-adk4-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@rkachach @adk3798 @baum @idryomov @phlogistonjohn @epuertat @ktdreyer