Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mgr/cephadm: adding support for nvmeof #50423

Merged
merged 4 commits into from
Jul 25, 2023
Merged

Conversation

rkachach
Copy link
Contributor

@rkachach rkachach commented Mar 7, 2023

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

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

src/cephadm/cephadm.py Outdated Show resolved Hide resolved
src/cephadm/cephadm.py Outdated Show resolved Hide resolved
src/cephadm/cephadm.py Outdated Show resolved Hide resolved
@rkachach
Copy link
Contributor Author

rkachach commented Jul 5, 2023

jenkins retest this please

@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

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.

src/cephadm/cephadm.py Outdated Show resolved Hide resolved
src/cephadm/cephadm.py Outdated Show resolved Hide resolved
src/cephadm/cephadm.py Outdated Show resolved Hide resolved
src/cephadm/cephadm.py Show resolved Hide resolved
src/python-common/ceph/deployment/service_spec.py Outdated Show resolved Hide resolved
@rkachach rkachach force-pushed the fix_issue_nmveof branch 2 times, most recently from 24bd67f to 2e9c158 Compare July 7, 2023 13:32
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.

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.

src/cephadm/cephadm.py Show resolved Hide resolved
src/cephadm/cephadm.py Outdated Show resolved Hide resolved
src/cephadm/cephadm.py Show resolved Hide resolved
src/cephadm/cephadm.py Outdated Show resolved Hide resolved
src/cephadm/cephadm.py Outdated Show resolved Hide resolved
src/pybind/mgr/cephadm/module.py Outdated Show resolved Hide resolved
src/pybind/mgr/cephadm/module.py Outdated Show resolved Hide resolved
src/pybind/mgr/cephadm/services/nvmeof.py Outdated Show resolved Hide resolved
Signed-off-by: Redouane Kachach <rkachach@redhat.com>
@rkachach
Copy link
Contributor Author

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.

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
src/cephadm/cephadm.py Show resolved Hide resolved
src/cephadm/cephadm.py Outdated Show resolved Hide resolved
src/cephadm/cephadm.py Outdated Show resolved Hide resolved
@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

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
Signed-off-by: Redouane Kachach <rkachach@redhat.com>
@rkachach
Copy link
Contributor Author

jenkins test make check

@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

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
11 of 12 checks passed
@adk3798 adk3798 mentioned this pull request Aug 15, 2023
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants