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

Add CLI that enables NVMe ANA report per volumes and utilizes SPDK ANA-groupIds, ANA states for namespaces and listeners #264

Merged
merged 2 commits into from
Oct 19, 2023

Conversation

leonidc
Copy link
Collaborator

@leonidc leonidc commented Oct 15, 2023

Based on ceph-nvmeof issue 122

As per explanation in the issue:
-added optional flag "ana_report" to create-subsystem cli
-added optional flag "enable_ha" to create-subsystem cli
-added optional "ana_grpid" to add-namespace cli
-limited just 4 ANA groups per subsystem
-when added listener and "enable_ha" is configured - set ana_states to all supported ANA groups as "inaccessible"

pushed as 2 commits, they would be squashed

@leonidc leonidc changed the title Pr122 Add CLI that enables NVMe ANA report per volumes and utilizes SPDK ANA-groupIds, ANA states for namespaces and listeners Oct 16, 2023
@leonidc leonidc force-pushed the pr122 branch 2 times, most recently from 5500152 to d8a4ef2 Compare October 17, 2023 14:42
control/cli.py Outdated Show resolved Hide resolved
control/cli.py Outdated Show resolved Hide resolved
control/grpc.py Outdated Show resolved Hide resolved
control/grpc.py Outdated Show resolved Hide resolved
mk/demo.mk Outdated Show resolved Hide resolved
…ubsystem, added test

Add and process enable_ha

Signed-off-by: leonidc <[email protected]>
@leonidc leonidc merged commit 2efef1b into ceph:devel Oct 19, 2023
12 checks passed
@gbregman gbregman linked an issue Oct 19, 2023 that may be closed by this pull request
@gbregman
Copy link
Contributor

@leonidc and @caroav , I missed it in the review. But I think it's wrong to use the OMAP state for the ANA stuff in create_listener. The OMAP might not be up to date yet. We should use the local state. Especially when call the SPDK right after it, not the OMAP. So we should not mix the two states. Also, in case we don't find a proper subsystem we don't set "req" but we still use it. This can't happen when using the local state as we should have this subsystem or else the previous call to SPDK to create the listener would have failed but I think it's safer and make the code easier to read to have a guard. So I suggest the following change:

diff --git a/control/grpc.py b/control/grpc.py
index 0f452d6..d1ed4e5 100644
--- a/control/grpc.py
+++ b/control/grpc.py
@@ -533,15 +533,16 @@ class GatewayService(pb2_grpc.GatewayServicer):
                 context.set_details(f"{ex}")
             return pb2.req_status()

-        state = self.gateway_state.omap.get_state()
+        req = None
+        state = self.gateway_state.local.get_state()
         for key,val  in state.items():
-              if (key.startswith(self.gateway_state.omap.SUBSYSTEM_PREFIX + request.nqn)):
+              if (key.startswith(self.gateway_state.local.SUBSYSTEM_PREFIX + request.nqn)):
                      self.logger.debug(f"values of key: {key}  val: {val} \n")
                      req = json_format.Parse(val, pb2.create_subsystem_req())
                      self.logger.info(f" enable_ha :{req.enable_ha} \n")
                      break

-        if req.enable_ha:
+        if req and req.enable_ha:
               for x in range (MAX_ANA_GROUPS):
                    try:
                       ret = rpc_nvmf.nvmf_subsystem_listener_set_ana_state(

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.

read discovery params with defaults
2 participants