From 95af3bb3f1eb8fa782f1599a3c73be73feb8faf7 Mon Sep 17 00:00:00 2001 From: Gil Bregman Date: Mon, 30 Oct 2023 17:43:31 +0200 Subject: [PATCH 1/5] Handle IPv6 parameters in NVMe CLI. Fixes #245 Fixes #247 Signed-off-by: Gil Bregman --- .env | 11 +++---- .github/workflows/build-container.yml | 1 + control/cli.py | 6 ++-- control/config.py | 7 +++++ control/grpc.py | 7 +++-- control/server.py | 3 ++ docker-compose.yaml | 3 ++ mk/demo.mk | 3 ++ mk/misc.mk | 3 +- tests/test_cli.py | 42 +++++++++++++++++++++++++++ 10 files changed, 76 insertions(+), 10 deletions(-) diff --git a/.env b/.env index 5bbd3da2..64abbb2c 100644 --- a/.env +++ b/.env @@ -27,7 +27,8 @@ NVMEOF_DESCRIPTION="Service to provide block storage on top of Ceph for platform NVMEOF_URL="https://github.com/ceph/ceph-nvmeof" NVMEOF_TAGS="ceph,nvme-of,nvme-of gateway,rbd,block storage" NVMEOF_WANTS="ceph,rbd" -NVMEOF_IP_ADDRESS="192.168.13.3" +NVMEOF_IP_ADDRESS=192.168.13.3 +NVMEOF_IPV6_ADDRESS=2001:db8::3 NVMEOF_IO_PORT=4420 NVMEOF_GW_PORT=5500 NVMEOF_DISC_PORT=8009 @@ -57,10 +58,10 @@ CEPH_CLUSTER_VERSION="${CEPH_VERSION}" CEPH_VSTART_ARGS="--without-dashboard --memstore" # Demo settings -RBD_POOL="rbd" -RBD_IMAGE_NAME="demo_image" -RBD_IMAGE_SIZE="10M" -BDEV_NAME="demo_bdev" +RBD_POOL=rbd +RBD_IMAGE_NAME=demo_image +RBD_IMAGE_SIZE=10M +BDEV_NAME=demo_bdev NQN="nqn.2016-06.io.spdk:cnode1" SERIAL="SPDK00000000000001" diff --git a/.github/workflows/build-container.yml b/.github/workflows/build-container.yml index 9bebaaf9..296d2c82 100644 --- a/.github/workflows/build-container.yml +++ b/.github/workflows/build-container.yml @@ -226,6 +226,7 @@ jobs: shopt -s expand_aliases eval $(make alias) nvmeof-cli get_subsystems + nvmeof-cli-ipv6 get_subsystems - name: Run bdevperf run: | diff --git a/control/cli.py b/control/cli.py index b7e41b72..2f6b7dbd 100644 --- a/control/cli.py +++ b/control/cli.py @@ -17,7 +17,7 @@ from .proto import gateway_pb2_grpc as pb2_grpc from .proto import gateway_pb2 as pb2 - +from .config import GatewayConfig def argument(*name_or_flags, **kwargs): """Helper function to format arguments for argparse command decorator.""" @@ -124,7 +124,9 @@ def stub(self): def connect(self, host, port, client_key, client_cert, server_cert): """Connects to server and sets stub.""" - server = "{}:{}".format(host, port) + # We need to enclose IPv6 addresses in brackets before concatenating a colon and port number to it + host = GatewayConfig.escape_address_if_ipv6(host) + server = f"{host}:{port}" if client_key and client_cert: # Create credentials for mutual TLS and a secure channel diff --git a/control/config.py b/control/config.py index cfa98836..ccec4fc6 100644 --- a/control/config.py +++ b/control/config.py @@ -64,3 +64,10 @@ def dump_config_file(self, logger): self.conffile_logged = True except Exception: pass + + # We need to enclose IPv6 addresses in brackets before concatenating a colon and port number to it + def escape_address_if_ipv6(addr) -> str: + ret_addr = addr + if ":" in addr and not addr.strip().startswith("["): + ret_addr = f"[{addr}]" + return ret_addr diff --git a/control/grpc.py b/control/grpc.py index 222796d6..d2726dea 100644 --- a/control/grpc.py +++ b/control/grpc.py @@ -23,6 +23,7 @@ from google.protobuf import json_format from .proto import gateway_pb2 as pb2 from .proto import gateway_pb2_grpc as pb2_grpc +from .config import GatewayConfig MAX_ANA_GROUPS = 4 @@ -535,9 +536,10 @@ def matching_listener_exists(self, context, nqn, gw_name, trtype, traddr, trsvci def create_listener_safe(self, request, context=None): """Creates a listener for a subsystem at a given IP/Port.""" ret = True + traddr = GatewayConfig.escape_address_if_ipv6(request.traddr) self.logger.info(f"Received request to create {request.gateway_name}" f" {request.trtype} listener for {request.nqn} at" - f" {request.traddr}:{request.trsvcid}.") + f" {traddr}:{request.trsvcid}.") try: if request.gateway_name == self.gateway_name: listener_already_exist = self.matching_listener_exists( @@ -627,9 +629,10 @@ def delete_listener_safe(self, request, context=None): """Deletes a listener from a subsystem at a given IP/Port.""" ret = True + traddr = GatewayConfig.escape_address_if_ipv6(request.traddr) self.logger.info(f"Received request to delete {request.gateway_name}" f" {request.trtype} listener for {request.nqn} at" - f" {request.traddr}:{request.trsvcid}.") + f" {traddr}:{request.trsvcid}.") try: if request.gateway_name == self.gateway_name: ret = rpc_nvmf.nvmf_subsystem_remove_listener( diff --git a/control/server.py b/control/server.py index 2c3f8277..1253dba2 100644 --- a/control/server.py +++ b/control/server.py @@ -28,6 +28,7 @@ from .state import GatewayState, LocalGatewayState, OmapGatewayState, GatewayStateHandler from .grpc import GatewayService from .discovery import DiscoveryService +from .config import GatewayConfig def sigchld_handler(signum, frame): """Handle SIGCHLD, runs when a spdk process terminates.""" @@ -158,6 +159,8 @@ def _add_server_listener(self): enable_auth = self.config.getboolean("gateway", "enable_auth") gateway_addr = self.config.get("gateway", "addr") gateway_port = self.config.get("gateway", "port") + # We need to enclose IPv6 addresses in brackets before concatenating a colon and port number to it + gateway_addr = GatewayConfig.escape_address_if_ipv6(gateway_addr) if enable_auth: # Read in key and certificates for authentication server_key = self.config.get("mtls", "server_key") diff --git a/docker-compose.yaml b/docker-compose.yaml index 926eb3cc..90025098 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -78,6 +78,7 @@ services: networks: default: ipv4_address: 192.168.13.2 + ipv6_address: 2001:db8::2 nvmeof-base: build: context: . @@ -213,6 +214,8 @@ volumes: ceph-conf: networks: default: + enable_ipv6: true ipam: config: - subnet: 192.168.13.0/24 + - subnet: 2001:0DB8::/112 diff --git a/mk/demo.mk b/mk/demo.mk index f8bcbd63..c132aa3b 100644 --- a/mk/demo.mk +++ b/mk/demo.mk @@ -10,9 +10,12 @@ rbd: CMD = bash -c "rbd -p $(RBD_POOL) info $(RBD_IMAGE_NAME) || rbd -p $(RBD_PO demo: export NVMEOF_HOSTNAME != docker ps -q -f name=$(NVMEOF_CONTAINER_NAME) demo: rbd ## Expose RBD_IMAGE_NAME as NVMe-oF target $(NVMEOF_CLI) create_bdev --pool $(RBD_POOL) --image $(RBD_IMAGE_NAME) --bdev $(BDEV_NAME) + $(NVMEOF_CLI_IPV6) create_bdev --pool $(RBD_POOL) --image $(RBD_IMAGE_NAME) --bdev $(BDEV_NAME)_ipv6 $(NVMEOF_CLI) create_subsystem --subnqn $(NQN) $(NVMEOF_CLI) add_namespace --subnqn $(NQN) --bdev $(BDEV_NAME) + $(NVMEOF_CLI) add_namespace --subnqn $(NQN) --bdev $(BDEV_NAME)_ipv6 $(NVMEOF_CLI) create_listener --subnqn $(NQN) --gateway-name $(NVMEOF_HOSTNAME) --traddr $(NVMEOF_IP_ADDRESS) --trsvcid $(NVMEOF_IO_PORT) + $(NVMEOF_CLI_IPV6) create_listener --subnqn $(NQN) --gateway-name $(NVMEOF_HOSTNAME) --traddr '$(NVMEOF_IPV6_ADDRESS)' --trsvcid $(NVMEOF_IO_PORT) --adrfam IPV6 $(NVMEOF_CLI) add_host --subnqn $(NQN) --host "*" .PHONY: demo rbd diff --git a/mk/misc.mk b/mk/misc.mk index 021a8340..34766b3e 100644 --- a/mk/misc.mk +++ b/mk/misc.mk @@ -2,8 +2,9 @@ # nvmeof_cli NVMEOF_CLI = $(DOCKER_COMPOSE_ENV) $(DOCKER_COMPOSE) run --rm nvmeof-cli --server-address $(NVMEOF_IP_ADDRESS) --server-port $(NVMEOF_GW_PORT) +NVMEOF_CLI_IPV6 = $(DOCKER_COMPOSE_ENV) $(DOCKER_COMPOSE) run --rm nvmeof-cli --server-address $(NVMEOF_IPV6_ADDRESS) --server-port $(NVMEOF_GW_PORT) alias: ## Print bash alias command for the nvmeof-cli. Usage: "eval $(make alias)" - @echo alias nvmeof-cli=\"$(NVMEOF_CLI)\" + @echo alias nvmeof-cli=\"$(NVMEOF_CLI)\" \; alias nvmeof-cli-ipv6=\'$(NVMEOF_CLI_IPV6)\' .PHONY: alias diff --git a/tests/test_cli.py b/tests/test_cli.py index d883277c..4d54c926 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -12,11 +12,15 @@ serial = "SPDK00000000000001" host_list = ["nqn.2016-06.io.spdk:host1", "*"] nsid = "1" +nsid_ipv6 = "2" anagrpid = "2" trtype = "TCP" gateway_name = socket.gethostname() addr = "127.0.0.1" +addr_ipv6 = "::1" +server_addr_ipv6 = "2001:db8::3" listener_list = [["-g", gateway_name, "-a", addr, "-s", "5001"], ["-g", gateway_name, "-a", addr,"-s", "5002"]] +listener_list_ipv6 = [["-g", gateway_name, "-a", addr_ipv6, "-s", "5003"], ["-g", gateway_name, "-a", addr_ipv6, "-s", "5004"]] config = "ceph-nvmeof.conf" @pytest.fixture(scope="module") @@ -38,6 +42,10 @@ def test_get_subsystems(self, caplog, gateway): cli(["get_subsystems"]) assert "Failed to get" not in caplog.text + def test_get_subsystems_ipv6(self, caplog, gateway): + cli(["--server-address", server_addr_ipv6, "get_subsystems"]) + assert "Failed to get" not in caplog.text + class TestCreate: def test_create_bdev(self, caplog, gateway): @@ -46,6 +54,12 @@ def test_create_bdev(self, caplog, gateway): cli(["create_bdev", "-i", image, "-p", pool, "-b", bdev1]) assert "Failed to create" not in caplog.text + def test_create_bdev_ipv6(self, caplog, gateway): + cli(["--server-address", server_addr_ipv6, "create_bdev", "-i", image, "-p", pool, "-b", bdev + "_ipv6"]) + assert "Failed to create" not in caplog.text + cli(["--server-address", server_addr_ipv6, "create_bdev", "-i", image, "-p", pool, "-b", bdev1 + "_ipv6"]) + assert "Failed to create" not in caplog.text + def test_create_subsystem(self, caplog, gateway): cli(["create_subsystem", "-n", subsystem]) assert "Failed to create" not in caplog.text @@ -62,6 +76,12 @@ def test_add_namespace(self, caplog, gateway): cli(["add_namespace", "-n", subsystem, "-b", bdev1]) assert "Failed to add" not in caplog.text + def test_add_namespace_ipv6(self, caplog, gateway): + cli(["--server-address", server_addr_ipv6, "add_namespace", "-n", subsystem, "-b", bdev + "_ipv6"]) + assert "Failed to add" not in caplog.text + cli(["--server-address", server_addr_ipv6, "add_namespace", "-n", subsystem, "-b", bdev1 + "_ipv6"]) + assert "Failed to add" not in caplog.text + @pytest.mark.parametrize("host", host_list) def test_add_host(self, caplog, host): cli(["add_host", "-n", subsystem, "-t", host]) @@ -72,6 +92,11 @@ def test_create_listener(self, caplog, listener, gateway): cli(["create_listener", "-n", subsystem] + listener) assert "Failed to create" not in caplog.text + @pytest.mark.parametrize("listener_ipv6", listener_list_ipv6) + def test_create_listener_ipv6(self, caplog, listener_ipv6, gateway): + cli(["--server-address", server_addr_ipv6, "create_listener", "-n", subsystem, "--adrfam", "IPV6"] + listener_ipv6) + assert "Failed to create" not in caplog.text + class TestDelete: @pytest.mark.parametrize("host", host_list) @@ -84,15 +109,26 @@ def test_delete_listener(self, caplog, listener, gateway): cli(["delete_listener", "-n", subsystem] + listener) assert "Failed to delete" not in caplog.text + @pytest.mark.parametrize("listener_ipv6", listener_list_ipv6) + def test_delete_listener_ipv6(self, caplog, listener_ipv6, gateway): + cli(["--server-address", server_addr_ipv6, "delete_listener", "-n", subsystem, "--adrfam", "IPV6"] + listener_ipv6) + assert "Failed to delete" not in caplog.text + def test_remove_namespace(self, caplog, gateway): cli(["remove_namespace", "-n", subsystem, "-i", nsid]) assert "Failed to remove" not in caplog.text + cli(["remove_namespace", "-n", subsystem, "-i", nsid_ipv6]) + assert "Failed to remove" not in caplog.text def test_delete_bdev(self, caplog, gateway): cli(["delete_bdev", "-b", bdev, "-f"]) assert "Failed to delete" not in caplog.text cli(["delete_bdev", "-b", bdev1, "--force"]) assert "Failed to delete" not in caplog.text + cli(["delete_bdev", "-b", bdev + "_ipv6", "-f"]) + assert "Failed to delete" not in caplog.text + cli(["delete_bdev", "-b", bdev1 + "_ipv6", "--force"]) + assert "Failed to delete" not in caplog.text def test_delete_subsystem(self, caplog, gateway): cli(["delete_subsystem", "-n", subsystem]) @@ -106,6 +142,10 @@ def test_create_bdev_ana(self, caplog, gateway): cli(["create_bdev", "-i", image, "-p", pool, "-b", bdev]) assert "Failed to create" not in caplog.text + def test_create_bdev_ana_ipv6(self, caplog, gateway): + cli(["--server-address", server_addr_ipv6, "create_bdev", "-i", image, "-p", pool, "-b", bdev + "_ipv6"]) + assert "Failed to create" not in caplog.text + def test_create_subsystem_ana(self, caplog, gateway): cli(["create_subsystem", "-n", subsystem, "-a", "-t"]) @@ -137,6 +177,8 @@ def test_remove_namespace_ana(self, caplog, gateway): def test_delete_bdev_ana(self, caplog, gateway): cli(["delete_bdev", "-b", bdev, "-f"]) assert "Failed to delete" not in caplog.text + cli(["delete_bdev", "-b", bdev + "_ipv6", "-f"]) + assert "Failed to delete" not in caplog.text def test_delete_subsystem_ana(self, caplog, gateway): cli(["delete_subsystem", "-n", subsystem]) From 86ba757aec446a2861e49d32aa477b1f8283cd27 Mon Sep 17 00:00:00 2001 From: Gil Bregman Date: Tue, 31 Oct 2023 15:06:46 +0200 Subject: [PATCH 2/5] Make sure to convert enum key to proper case before fetching the enum value. Fixes #300 Signed-off-by: Gil Bregman --- control/discovery.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/control/discovery.py b/control/discovery.py index ce86bf10..ccee0932 100644 --- a/control/discovery.py +++ b/control/discovery.py @@ -731,12 +731,12 @@ def reply_get_log_page(self, conn, data, cmd_id): log_entry_counter = 0 while log_entry_counter < len(allow_listeners): log_entry = DiscoveryLogEntry() - trtype = TRANSPORT_TYPES[allow_listeners[log_entry_counter]["trtype"]] + trtype = TRANSPORT_TYPES[allow_listeners[log_entry_counter]["trtype"].upper()] if trtype is None: self.logger.error("unsupported transport type") else: log_entry.trtype = trtype - adrfam = ADRFAM_TYPES[allow_listeners[log_entry_counter]["adrfam"]] + adrfam = ADRFAM_TYPES[allow_listeners[log_entry_counter]["adrfam"].lower()] if adrfam is None: self.logger.error("unsupported adress family") else: From 76ae9ec317031345309d80596e49a11e7d64c7e0 Mon Sep 17 00:00:00 2001 From: Gil Bregman Date: Tue, 31 Oct 2023 18:21:22 +0200 Subject: [PATCH 3/5] Do not allow using the discovery NQN in the CLI commands. Fixes #299 Signed-off-by: Gil Bregman --- control/discovery.py | 2 ++ control/grpc.py | 45 ++++++++++++++++++++++++++++++++++++++++++-- control/server.py | 2 +- 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/control/discovery.py b/control/discovery.py index ccee0932..652cbef8 100644 --- a/control/discovery.py +++ b/control/discovery.py @@ -304,6 +304,8 @@ class DiscoveryService: discovery_port: Discovery controller's listening port """ + DISCOVERY_NQN = "nqn.2014-08.org.nvmexpress.discovery" + def __init__(self, config): self.version = 1 self.config = config diff --git a/control/grpc.py b/control/grpc.py index d2726dea..f4da2460 100644 --- a/control/grpc.py +++ b/control/grpc.py @@ -24,6 +24,7 @@ from .proto import gateway_pb2 as pb2 from .proto import gateway_pb2_grpc as pb2_grpc from .config import GatewayConfig +from .discovery import DiscoveryService MAX_ANA_GROUPS = 4 @@ -244,11 +245,18 @@ def delete_bdev(self, request, context=None): with self.rpc_lock: return self.delete_bdev_safe(request, context) + def is_discovery_nqn(self, nqn) -> bool: + return nqn == DiscoveryService.DISCOVERY_NQN + def create_subsystem_safe(self, request, context=None): """Creates a subsystem.""" self.logger.info( f"Received request to create subsystem {request.subsystem_nqn}, ana reporting: {request.ana_reporting} ") + + if self.is_discovery_nqn(request.subsystem_nqn): + raise Exception(f"Can't create a discovery subsystem") + min_cntlid = self.config.getint_with_default("gateway", "min_controller_id", 1) max_cntlid = self.config.getint_with_default("gateway", "max_controller_id", 65519) if not request.serial_number: @@ -296,6 +304,10 @@ def delete_subsystem_safe(self, request, context=None): self.logger.info( f"Received request to delete subsystem {request.subsystem_nqn}") + + if self.is_discovery_nqn(request.subsystem_nqn): + raise Exception(f"Can't delete a discovery subsystem") + try: ret = rpc_nvmf.nvmf_delete_subsystem( self.spdk_rpc_client, @@ -326,11 +338,16 @@ def delete_subsystem(self, request, context=None): def add_namespace_safe(self, request, context=None): """Adds a namespace to a subsystem.""" - if request.anagrpid > MAX_ANA_GROUPS: - raise Exception(f"Error group ID {request.anagrpid} is more than configured maximum {MAX_ANA_GROUPS}\n") self.logger.info(f"Received request to add {request.bdev_name} to" f" {request.subsystem_nqn}") + + if request.anagrpid > MAX_ANA_GROUPS: + raise Exception(f"Error group ID {request.anagrpid} is more than configured maximum {MAX_ANA_GROUPS}") + + if self.is_discovery_nqn(request.subsystem_nqn): + raise Exception(f"Can't add a namespace to a discovery subsystem") + try: nsid = rpc_nvmf.nvmf_subsystem_add_ns( self.spdk_rpc_client, @@ -372,6 +389,10 @@ def remove_namespace_safe(self, request, context=None): self.logger.info(f"Received request to remove nsid {request.nsid} from" f" {request.subsystem_nqn}") + + if self.is_discovery_nqn(request.subsystem_nqn): + raise Exception(f"Can't remove a namespace from a discovery subsystem") + try: ret = rpc_nvmf.nvmf_subsystem_remove_ns( self.spdk_rpc_client, @@ -415,6 +436,12 @@ def matching_host_exists(self, context, subsys_nqn, host_nqn) -> bool: def add_host_safe(self, request, context=None): """Adds a host to a subsystem.""" + if self.is_discovery_nqn(request.subsystem_nqn): + raise Exception(f"Can't allow a host to a discovery subsystem") + + if self.is_discovery_nqn(request.host_nqn): + raise Exception(f"Can't use a discovery NQN as host NQN") + try: host_already_exist = self.matching_host_exists(context, request.subsystem_nqn, request.host_nqn) if host_already_exist: @@ -480,6 +507,12 @@ def add_host(self, request, context=None): def remove_host_safe(self, request, context=None): """Removes a host from a subsystem.""" + if self.is_discovery_nqn(request.subsystem_nqn): + raise Exception(f"Can't remove a host from a discovery subsystem") + + if self.is_discovery_nqn(request.host_nqn): + raise Exception(f"Can't use a discovery NQN as host NQN") + try: if request.host_nqn == "*": # Disable allow any host access self.logger.info( @@ -540,6 +573,10 @@ def create_listener_safe(self, request, context=None): self.logger.info(f"Received request to create {request.gateway_name}" f" {request.trtype} listener for {request.nqn} at" f" {traddr}:{request.trsvcid}.") + + if self.is_discovery_nqn(request.nqn): + raise Exception(f"Can't create a listener for a discovery subsystem") + try: if request.gateway_name == self.gateway_name: listener_already_exist = self.matching_listener_exists( @@ -633,6 +670,10 @@ def delete_listener_safe(self, request, context=None): self.logger.info(f"Received request to delete {request.gateway_name}" f" {request.trtype} listener for {request.nqn} at" f" {traddr}:{request.trsvcid}.") + + if self.is_discovery_nqn(request.nqn): + raise Exception(f"Can't delete a listener from a discovery subsystem") + try: if request.gateway_name == self.gateway_name: ret = rpc_nvmf.nvmf_subsystem_remove_listener( diff --git a/control/server.py b/control/server.py index 1253dba2..3f891074 100644 --- a/control/server.py +++ b/control/server.py @@ -138,7 +138,7 @@ def _start_discovery_service(self): return try: - rpc_nvmf.nvmf_delete_subsystem(self.spdk_rpc_ping_client, "nqn.2014-08.org.nvmexpress.discovery") + rpc_nvmf.nvmf_delete_subsystem(self.spdk_rpc_ping_client, DiscoveryService.DISCOVERY_NQN) except Exception as ex: self.logger.error(f" Delete Discovery subsystem returned with error: \n {ex}") raise From 90db78fbfa31281c44990223dd156b91b7c1f70a Mon Sep 17 00:00:00 2001 From: Gil Bregman Date: Tue, 31 Oct 2023 22:48:14 +0200 Subject: [PATCH 4/5] Do not duplicate the code to construct resource keys. Fixes #275 Signed-off-by: Gil Bregman --- control/grpc.py | 7 ++++--- control/state.py | 52 ++++++++++++++++++++++++++++++++++-------------- 2 files changed, 41 insertions(+), 18 deletions(-) diff --git a/control/grpc.py b/control/grpc.py index f4da2460..a4cd29d7 100644 --- a/control/grpc.py +++ b/control/grpc.py @@ -25,6 +25,7 @@ from .proto import gateway_pb2_grpc as pb2_grpc from .config import GatewayConfig from .discovery import DiscoveryService +from .state import GatewayState MAX_ANA_GROUPS = 4 @@ -426,7 +427,7 @@ def remove_namespace(self, request, context=None): def matching_host_exists(self, context, subsys_nqn, host_nqn) -> bool: if not context: return False - host_key = "_".join([self.gateway_state.local.HOST_PREFIX + subsys_nqn, host_nqn]) + host_key = GatewayState.build_host_key(subsys_nqn, host_nqn) state = self.gateway_state.local.get_state() if state.get(host_key): return True @@ -559,7 +560,7 @@ def remove_host(self, request, context=None): def matching_listener_exists(self, context, nqn, gw_name, trtype, traddr, trsvcid) -> bool: if not context: return False - listener_key = "_".join([self.gateway_state.local.LISTENER_PREFIX + nqn, gw_name, trtype, traddr, trsvcid]) + listener_key = GatewayState.build_listener_key(nqn, gw_name, trtype, traddr, trsvcid) state = self.gateway_state.local.get_state() if state.get(listener_key): return True @@ -614,7 +615,7 @@ def create_listener_safe(self, request, context=None): state = self.gateway_state.local.get_state() req = None - subsys = state.get(self.gateway_state.local.SUBSYSTEM_PREFIX + request.nqn) + subsys = state.get(GatewayState.build_subsystem_key(request.nqn)) if subsys: self.logger.debug(f"value of sub-system: {subsys}") try: diff --git a/control/state.py b/control/state.py index 3f5acc6f..f4e575b4 100644 --- a/control/state.py +++ b/control/state.py @@ -29,6 +29,30 @@ class GatewayState(ABC): HOST_PREFIX = "host_" LISTENER_PREFIX = "listener_" + def build_bdev_key(bdev_name: str) -> str: + return GatewayState.BDEV_PREFIX + bdev_name + + def build_namespace_key(subsystem_nqn: str, nsid) -> str: + key = GatewayState.NAMESPACE_PREFIX + subsystem_nqn + if nsid is not None: + key = key + "_" + nsid + return key + + def build_subsystem_key(subsystem_nqn: str) -> str: + return GatewayState.SUBSYSTEM_PREFIX + subsystem_nqn + + def build_host_key(subsystem_nqn: str, host_nqn) -> str: + key = GatewayState.HOST_PREFIX + subsystem_nqn + if host_nqn is not None: + key = key + "_" + host_nqn + return key + + def build_partial_listener_key(subsystem_nqn: str) -> str: + return GatewayState.LISTENER_PREFIX + subsystem_nqn + + def build_listener_key(subsystem_nqn: str, gateway: str, trtype: str, traddr: str, trsvcid: str) -> str: + return GatewayState.build_partial_listener_key(subsystem_nqn) + "_" + gateway + "_" + trtype + "_" + traddr + "_" + trsvcid + @abstractmethod def get_state(self) -> Dict[str, str]: """Returns the state dictionary.""" @@ -46,64 +70,62 @@ def _remove_key(self, key: str): def add_bdev(self, bdev_name: str, val: str): """Adds a bdev to the state data store.""" - key = self.BDEV_PREFIX + bdev_name + key = GatewayState.build_bdev_key(bdev_name) self._add_key(key, val) def remove_bdev(self, bdev_name: str): """Removes a bdev from the state data store.""" - key = self.BDEV_PREFIX + bdev_name + key = GatewayState.build_bdev_key(bdev_name) self._remove_key(key) def add_namespace(self, subsystem_nqn: str, nsid: str, val: str): """Adds a namespace to the state data store.""" - key = self.NAMESPACE_PREFIX + subsystem_nqn + "_" + nsid + key = GatewayState.build_namespace_key(subsystem_nqn, nsid) self._add_key(key, val) def remove_namespace(self, subsystem_nqn: str, nsid: str): """Removes a namespace from the state data store.""" - key = self.NAMESPACE_PREFIX + subsystem_nqn + "_" + nsid + key = GatewayState.build_namespace_key(subsystem_nqn, nsid) self._remove_key(key) def add_subsystem(self, subsystem_nqn: str, val: str): """Adds a subsystem to the state data store.""" - key = self.SUBSYSTEM_PREFIX + subsystem_nqn + key = GatewayState.build_subsystem_key(subsystem_nqn) self._add_key(key, val) def remove_subsystem(self, subsystem_nqn: str): """Removes a subsystem from the state data store.""" - key = self.SUBSYSTEM_PREFIX + subsystem_nqn + key = GatewayState.build_subsystem_key(subsystem_nqn) self._remove_key(key) # Delete all keys related to subsystem state = self.get_state() for key in state.keys(): - if (key.startswith(self.NAMESPACE_PREFIX + subsystem_nqn) or - key.startswith(self.HOST_PREFIX + subsystem_nqn) or - key.startswith(self.LISTENER_PREFIX + subsystem_nqn)): + if (key.startswith(GatewayState.build_namespace_key(subsystem_nqn, None)) or + key.startswith(GatewayState.build_host_key(subsystem_nqn, None)) or + key.startswith(GatewayState.build_partial_listener_key(subsystem_nqn))): self._remove_key(key) def add_host(self, subsystem_nqn: str, host_nqn: str, val: str): """Adds a host to the state data store.""" - key = "{}{}_{}".format(self.HOST_PREFIX, subsystem_nqn, host_nqn) + key = GatewayState.build_host_key(subsystem_nqn, host_nqn) self._add_key(key, val) def remove_host(self, subsystem_nqn: str, host_nqn: str): """Removes a host from the state data store.""" - key = "{}{}_{}".format(self.HOST_PREFIX, subsystem_nqn, host_nqn) + key = GatewayState.build_host_key(subsystem_nqn, host_nqn) self._remove_key(key) def add_listener(self, subsystem_nqn: str, gateway: str, trtype: str, traddr: str, trsvcid: str, val: str): """Adds a listener to the state data store.""" - key = "{}{}_{}_{}_{}_{}".format(self.LISTENER_PREFIX, subsystem_nqn, - gateway, trtype, traddr, trsvcid) + key = GatewayState.build_listener_key(subsystem_nqn, gateway, trtype, traddr, trsvcid) self._add_key(key, val) def remove_listener(self, subsystem_nqn: str, gateway: str, trtype: str, traddr: str, trsvcid: str): """Removes a listener from the state data store.""" - key = "{}{}_{}_{}_{}_{}".format(self.LISTENER_PREFIX, subsystem_nqn, - gateway, trtype, traddr, trsvcid) + key = GatewayState.build_listener_key(subsystem_nqn, gateway, trtype, traddr, trsvcid) self._remove_key(key) @abstractmethod From 3495e2c89f660da59906169b588c3a2bfb63aa02 Mon Sep 17 00:00:00 2001 From: Gil Bregman Date: Wed, 1 Nov 2023 16:12:27 +0200 Subject: [PATCH 5/5] Block the usage of the same serial number for two subsystems. Fixes #148 Signed-off-by: Gil Bregman --- control/grpc.py | 80 ++++++++++++++++++++++++++++++++++------------- tests/test_cli.py | 6 ++++ 2 files changed, 65 insertions(+), 21 deletions(-) diff --git a/control/grpc.py b/control/grpc.py index a4cd29d7..782089dc 100644 --- a/control/grpc.py +++ b/control/grpc.py @@ -168,17 +168,17 @@ def get_bdev_namespaces(self, bdev_name) -> list: ns_list = [] local_state_dict = self.gateway_state.local.get_state() for key, val in local_state_dict.items(): - if key.startswith(self.gateway_state.local.NAMESPACE_PREFIX): - try: - req = json_format.Parse(val, pb2.add_namespace_req(), ignore_unknown_fields = True) - ns_bdev_name = req.bdev_name - if ns_bdev_name == bdev_name: - nsid = req.nsid - nqn = req.subsystem_nqn - ns_list.insert(0, {"nqn" : nqn, "nsid" : nsid}) - except Exception as ex: - self.logger.error(f"Got exception trying to get bdev {bdev_name} namespaces: {ex}") - pass + if not key.startswith(self.gateway_state.local.NAMESPACE_PREFIX): + continue + try: + ns = json.loads(val) + if ns["bdev_name"] == bdev_name: + nsid = ns["nsid"] + nqn = ns["subsystem_nqn"] + ns_list.insert(0, {"nqn" : nqn, "nsid" : nsid}) + except Exception as ex: + self.logger.error(f"Got exception trying to get bdev {bdev_name} namespaces: {ex}") + pass return ns_list @@ -249,6 +249,23 @@ def delete_bdev(self, request, context=None): def is_discovery_nqn(self, nqn) -> bool: return nqn == DiscoveryService.DISCOVERY_NQN + def serial_number_already_used(self, context, serial) -> str: + if not context: + return None + state = self.gateway_state.local.get_state() + for key, val in state.items(): + if not key.startswith(self.gateway_state.local.SUBSYSTEM_PREFIX): + continue + try: + subsys = json.loads(val) + sn = subsys["serial_number"] + if serial == sn: + return subsys["subsystem_nqn"] + except Exception: + self.logger.warning("Got exception while parsing {val}: {ex}") + continue + return None + def create_subsystem_safe(self, request, context=None): """Creates a subsystem.""" @@ -264,7 +281,24 @@ def create_subsystem_safe(self, request, context=None): random.seed() randser = random.randint(2, 99999999999999) request.serial_number = f"SPDK{randser}" + self.logger.info(f"No serial number specified, will use {request.serial_number}") + try: + subsys_using_serial = self.serial_number_already_used(context, request.serial_number) + if subsys_using_serial: + self.logger.error(f"Serial number {request.serial_number} already used by subsystem {subsys_using_serial}") + req = {"subsystem_nqn": request.subsystem_nqn, + "serial_number": request.serial_number, + "max_namespaces": request.max_namespaces, + "ana_reporting": request.ana_reporting, + "enable_ha": request.enable_ha, + "method": "nvmf_create_subsystem", "req_id": 0} + ret = {"code": -errno.EEXIST, "message": f"Serial number {request.serial_number} already used by subsystem {subsys_using_serial}"} + msg = "\n".join(["request:", "%s" % json.dumps(req, indent=2), + "Got JSON-RPC error response", + "response:", + json.dumps(ret, indent=2)]) + raise Exception(msg) ret = rpc_nvmf.nvmf_create_subsystem( self.spdk_rpc_client, nqn=request.subsystem_nqn, @@ -614,20 +648,24 @@ def create_listener_safe(self, request, context=None): return pb2.req_status() state = self.gateway_state.local.get_state() - req = None - subsys = state.get(GatewayState.build_subsystem_key(request.nqn)) - if subsys: - self.logger.debug(f"value of sub-system: {subsys}") + enable_ha = False + subsys_str = state.get(GatewayState.build_subsystem_key(request.nqn)) + if subsys_str: + self.logger.debug(f"value of sub-system: {subsys_str}") try: - req = json_format.Parse(subsys, pb2.create_subsystem_req()) - self.logger.info(f"enable_ha: {req.enable_ha}") - except Exception: - self.logger.error(f"Got exception trying to parse subsystem: {ex}") + subsys_dict = json.loads(subsys_str) + try: + enable_ha = subsys_dict["enable_ha"] + except KeyError: + enable_ha = False + self.logger.info(f"enable_ha: {enable_ha}") + except Exception as ex: + self.logger.error(f"Got exception trying to parse subsystem {request.nqn}: {ex}") pass else: - self.logger.info(f"No sub-system for {request.nqn}") + self.logger.info(f"No subsystem for {request.nqn}") - if req and req.enable_ha: + if enable_ha: for x in range (MAX_ANA_GROUPS): try: ret = rpc_nvmf.nvmf_subsystem_listener_set_ana_state( diff --git a/tests/test_cli.py b/tests/test_cli.py index 4d54c926..45169b72 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -63,10 +63,13 @@ def test_create_bdev_ipv6(self, caplog, gateway): def test_create_subsystem(self, caplog, gateway): cli(["create_subsystem", "-n", subsystem]) assert "Failed to create" not in caplog.text + assert "ana reporting: False" in caplog.text cli(["get_subsystems"]) assert serial not in caplog.text + caplog.clear() cli(["create_subsystem", "-n", subsystem2, "-s", serial]) assert "Failed to create" not in caplog.text + assert "ana reporting: False" in caplog.text cli(["get_subsystems"]) assert serial in caplog.text @@ -148,8 +151,10 @@ def test_create_bdev_ana_ipv6(self, caplog, gateway): def test_create_subsystem_ana(self, caplog, gateway): + caplog.clear() cli(["create_subsystem", "-n", subsystem, "-a", "-t"]) assert "Failed to create" not in caplog.text + assert "ana reporting: True" in caplog.text cli(["get_subsystems"]) assert serial not in caplog.text @@ -161,6 +166,7 @@ def test_add_namespace_ana(self, caplog, gateway): def test_create_listener_ana(self, caplog, listener, gateway): cli(["create_listener", "-n", subsystem] + listener) assert "Failed to create" not in caplog.text + assert "enable_ha: True" in caplog.text class TestDeleteAna: