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

Disable SPDK discovery controller on GW initialization #192

Merged
merged 1 commit into from
Aug 27, 2023

Conversation

leonidc
Copy link
Collaborator

@leonidc leonidc commented Aug 23, 2023

when Gateway is created it by default creates discovery subsystem
server.py when spdk is started, calls rpc nvmf_delete_subsystem.

Tests:

The rpc was issued before applying the new GW configuration :
[root@rhel8-leo-dev ceph-nvmeof]# nvmeof-cli get_subsystems
INFO:main:Get subsystems:
[] <= the discovery subsystem does not appear anymore

]# nvme discover -t tcp -a 192.168.13.3 -s 4420
Failed to write to /dev/nvme-fabrics: Input/output error

The PR has no impact on the current Gateway behavior. Checked also IOs from the nvme-tcp initiator.

@leonidc leonidc requested review from barakda, caroav and gbregman August 23, 2023 07:38
Copy link
Contributor

@gbregman gbregman left a comment

Choose a reason for hiding this comment

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

LGTM

@caroav caroav requested review from idryomov, oritwas and baum August 23, 2023 10:58
control/server.py Outdated Show resolved Hide resolved
@baum
Copy link
Collaborator

baum commented Aug 23, 2023

@leonidc 🖖 awesome PR! Please sign off the commit, see here, to color DCO check green 🟩.

Also multi_gateway test fails consistently with this change, any idea why?

@gbregman
Copy link
Contributor

@leonidc the multi gateway test runs "get_subsystems" and checks that it got 2 subsystem. As there is now only one subsystem you need to adjust the test.

@leonidc
Copy link
Collaborator Author

leonidc commented Aug 23, 2023

if multi gateway test checks that 2 subsystems are configured and we want to add configuration parameter in config file that in some cases server.py would not delete discovery subsystems - request of Aviv , it looks so many dependencies for such a small change.

@caroav
Copy link
Collaborator

caroav commented Aug 23, 2023

@leonidc you can check the config flag in the test, and fix accordingly.

@leonidc
Copy link
Collaborator Author

leonidc commented Aug 23, 2023

@leonidc you can check the config flag in the test, and fix accordingly.

where can I find this flag ? it does not appear in conf file

@baum
Copy link
Collaborator

baum commented Aug 27, 2023

@leonidc 🖖 awesome 👍

could you please sign your existing commit:

git commit --amend --signoff --no-edit

And re-push this change, to make DCO check happy, see here

Generally, to sign off new commit, use --signoff

ceph-nvmeof.conf Outdated Show resolved Hide resolved
control/server.py Outdated Show resolved Hide resolved
tests/test_multi_gateway.py Outdated Show resolved Hide resolved
Fix multi-gateway test

Signed-off-by: leonidc <[email protected]>
@leonidc leonidc merged commit f737d04 into ceph:devel Aug 27, 2023
@leonidc leonidc deleted the my_new branch August 30, 2023 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants