diff --git a/.github/workflows/build-container.yml b/.github/workflows/build-container.yml index f40d9f8f..3d7bead8 100644 --- a/.github/workflows/build-container.yml +++ b/.github/workflows/build-container.yml @@ -133,7 +133,7 @@ jobs: strategy: fail-fast: false matrix: - test: ["cli", "cli_change_lb", "cli_change_keys", "state", "multi_gateway", "server", "grpc", "omap_lock", "log_files", "nsid", "psk", "dhchap"] + test: ["cli", "cli_change_lb", "cli_change_keys", "cli_change_ns_visibility", "state", "multi_gateway", "server", "grpc", "omap_lock", "log_files", "nsid", "psk", "dhchap"] runs-on: ubuntu-latest env: HUGEPAGES: 512 # for multi gateway test, approx 256 per gateway instance diff --git a/control/cli.py b/control/cli.py index dcd9cbf3..8179713b 100644 --- a/control/cli.py +++ b/control/cli.py @@ -506,6 +506,7 @@ def gw_set_log_level(self, args): gw_actions.append({"name" : "get_log_level", "args" : [], "help" : "Get gateway's log level"}) gw_actions.append({"name" : "set_log_level", "args" : gw_set_log_level_args, "help" : "Set gateway's log level"}) gw_choices = get_actions(gw_actions) + @cli.cmd(gw_actions) def gw(self, args): """Gateway commands""" @@ -596,8 +597,6 @@ def spdk_log_level_get(self, args): def spdk_log_level_set(self, args): """Set SPDK log levels and nvmf log flags""" - rc = 0 - errmsg = "" out_func, err_func = self.get_output_functions(args) log_level = None @@ -655,6 +654,7 @@ def spdk_log_level_set(self, args): spdk_log_actions.append({"name" : "set", "args" : spdk_log_set_args, "help" : "Set SPDK log levels and nvmf log flags"}) spdk_log_actions.append({"name" : "disable", "args" : spdk_log_disable_args, "help" : "Disable SPDK nvmf log flags"}) spdk_log_choices = get_actions(spdk_log_actions) + @cli.cmd(spdk_log_actions) def spdk_log_level(self, args): """SPDK nvmf log level commands""" @@ -691,7 +691,7 @@ def subsystem_add(self, args): new_nqn = "" try: new_nqn = ret.nqn - except Exception as ex: # In case of an old gateway the returned value wouldn't have the nqn field + except Exception: # In case of an old gateway the returned value wouldn't have the nqn field pass if not new_nqn: new_nqn = args.subsystem @@ -826,7 +826,6 @@ def subsystem_list(self, args): def subsystem_change_key(self, args): """Change subsystem's inband authentication key.""" - rc = 0 out_func, err_func = self.get_output_functions(args) req = pb2.change_subsystem_key_req(subsystem_nqn=args.subsystem, dhchap_key=args.dhchap_key) @@ -884,6 +883,7 @@ def subsystem_change_key(self, args): subsystem_actions.append({"name" : "list", "args" : subsys_list_args, "help" : "List subsystems"}) subsystem_actions.append({"name" : "change_key", "args" : subsys_change_key_args, "help" : "Change subsystem key"}) subsystem_choices = get_actions(subsystem_actions) + @cli.cmd(subsystem_actions) def subsystem(self, args): """Subsystem commands""" @@ -1026,11 +1026,11 @@ def listener_list(self, args): if args.format == "text" or args.format == "plain": if listeners_info.status == 0: listeners_list = [] - for l in listeners_info.listeners: - adrfam = GatewayEnumUtils.get_key_from_value(pb2.AddressFamily, l.adrfam) + for lstnr in listeners_info.listeners: + adrfam = GatewayEnumUtils.get_key_from_value(pb2.AddressFamily, lstnr.adrfam) adrfam = self.format_adrfam(adrfam) - secure = "Yes" if l.secure else "No" - listeners_list.append([l.host_name, l.trtype, adrfam, f"{l.traddr}:{l.trsvcid}", secure]) + secure = "Yes" if lstnr.secure else "No" + listeners_list.append([lstnr.host_name, lstnr.trtype, adrfam, f"{lstnr.traddr}:{lstnr.trsvcid}", secure]) if len(listeners_list) > 0: if args.format == "text": table_format = "fancy_grid" @@ -1086,6 +1086,7 @@ def listener_list(self, args): listener_actions.append({"name" : "del", "args" : listener_del_args, "help" : "Delete a listener"}) listener_actions.append({"name" : "list", "args" : listener_list_args, "help" : "List listeners"}) listener_choices = get_actions(listener_actions) + @cli.cmd(listener_actions) def listener(self, args): """Listener commands""" @@ -1216,7 +1217,6 @@ def host_del(self, args): def host_change_key(self, args): """Change host's inband authentication keys.""" - rc = 0 out_func, err_func = self.get_output_functions(args) if args.host_nqn == "*": @@ -1327,6 +1327,7 @@ def host_list(self, args): host_actions.append({"name" : "list", "args" : host_list_args, "help" : "List subsystem's host access"}) host_actions.append({"name" : "change_key", "args" : host_change_key_args, "help" : "Change host's inband authentication keys"}) host_choices = get_actions(host_actions) + @cli.cmd(host_actions) def host(self, args): """Host commands""" @@ -1406,6 +1407,7 @@ def connection_list(self, args): connection_actions = [] connection_actions.append({"name" : "list", "args" : connection_list_args, "help" : "List active connections"}) connection_choices = get_actions(connection_actions) + @cli.cmd(connection_actions) def connection(self, args): """Connection commands""" @@ -1601,7 +1603,7 @@ def get_size_in_bytes(self, sz): try: sz = sz.strip() int_size = int(sz) - except: + except Exception: self.cli.parser.error(f"Size {sz} must be numeric") int_size *= multiply @@ -2001,6 +2003,61 @@ def ns_del_host(self, args): return rc + def ns_change_visibility(self, args): + """Change namespace visibility.""" + + out_func, err_func = self.get_output_functions(args) + if args.nsid <= 0: + self.cli.parser.error("nsid value must be positive") + + if not args.auto_visible and not args.no_auto_visible: + self.cli.parser.error("Either --auto-visible or --no-auto-visible should be specified") + + if args.auto_visible and args.no_auto_visible: + self.cli.parser.error("--auto-visible and --no-auto-visible are mutually exclusive") + + if args.auto_visible: + auto_visible = True + elif args.no_auto_visible: + auto_visible = False + else: + assert False + + try: + change_visibility_req = pb2.namespace_change_visibility_req(subsystem_nqn=args.subsystem, + nsid=args.nsid, auto_visible=auto_visible, + force=args.force) + ret = self.stub.namespace_change_visibility(change_visibility_req) + except Exception as ex: + ret = pb2.req_status(status = errno.EINVAL, error_message = f"Failure changing namespace visibility:\n{ex}") + + if auto_visible: + vis_text = "\"visible to all hosts\"" + else: + vis_text = "\"visible to selected hosts\"" + if args.format == "text" or args.format == "plain": + if ret.status == 0: + out_func(f"Changing visibility of namespace {args.nsid} in {args.subsystem} to {vis_text}: Successful") + else: + err_func(f"{ret.error_message}") + elif args.format == "json" or args.format == "yaml": + ret_str = json_format.MessageToJson( + ret, + indent=4, + including_default_value_fields=True, + preserving_proto_field_name=True) + if args.format == "json": + out_func(f"{ret_str}") + elif args.format == "yaml": + obj = json.loads(ret_str) + out_func(yaml.dump(obj)) + elif args.format == "python": + return ret + else: + assert False + + return ret.status + ns_common_args = [ argument("--subsystem", "-n", help="Subsystem NQN", required=True), ] @@ -2034,6 +2091,12 @@ def ns_del_host(self, args): argument("--nsid", help="Namespace ID", type=int, required=True), argument("--load-balancing-group", "-l", help="Load balancing group", type=int, required=True), ] + ns_change_visibility_args_list = ns_common_args + [ + argument("--nsid", help="Namespace ID", type=int, required=True), + argument("--auto-visible", help="Visible to all hosts", action='store_true', required=False), + argument("--no-auto-visible", help="Visible to selected hosts only", action='store_true', required=False), + argument("--force", help="Change visibility of namespace even if there hosts added to it or active connections on the subsystem", action='store_true', required=False), + ] ns_set_qos_args_list = ns_common_args + [ argument("--nsid", help="Namespace ID", type=int, required=True), argument("--rw-ios-per-second", help="R/W IOs per second limit, 0 means unlimited", type=int), @@ -2059,7 +2122,9 @@ def ns_del_host(self, args): ns_actions.append({"name" : "set_qos", "args" : ns_set_qos_args_list, "help" : "Set QOS limits for a namespace"}) ns_actions.append({"name" : "add_host", "args" : ns_add_host_args_list, "help" : "Add a host to a namespace"}) ns_actions.append({"name" : "del_host", "args" : ns_del_host_args_list, "help" : "Delete a host from a namespace"}) + ns_actions.append({"name" : "change_visibility", "args" : ns_change_visibility_args_list, "help" : "Change visibility for a namespace"}) ns_choices = get_actions(ns_actions) + @cli.cmd(ns_actions, ["ns"]) def namespace(self, args): """Namespace commands""" @@ -2081,6 +2146,8 @@ def namespace(self, args): return self.ns_add_host(args) elif args.action == "del_host": return self.ns_del_host(args) + elif args.action == "change_visibility": + return self.ns_change_visibility(args) if not args.action: self.cli.parser.error(f"missing action for namespace command (choose from {GatewayClient.ns_choices})") @@ -2135,5 +2202,6 @@ def main(args=None) -> int: return main_common(client, parsed_args) + if __name__ == "__main__": sys.exit(main()) diff --git a/control/grpc.py b/control/grpc.py index 7b17eca3..2783f798 100644 --- a/control/grpc.py +++ b/control/grpc.py @@ -14,9 +14,7 @@ import random import os import errno -import contextlib import threading -import time import hashlib import tempfile from pathlib import Path @@ -129,12 +127,12 @@ def add_host_nqn(self, subsys, hostnqn): self.host_nqn[subsys].add(hostnqn) def remove_host_nqn(self, subsys, hostnqn): - if not subsys in self.host_nqn: + if subsys not in self.host_nqn: return self.host_nqn[subsys].discard(hostnqn) def get_host_count(self, subsys): - if not subsys in self.host_nqn: + if subsys not in self.host_nqn: return 0 return len(self.host_nqn[subsys]) @@ -165,16 +163,16 @@ def get_subsystem_dhchap_key(self, subsys) -> str: return key class NamespaceInfo: - def __init__(self, nsid, bdev, uuid, anagrpid, no_auto_visible): + def __init__(self, nsid, bdev, uuid, anagrpid, auto_visible): self.nsid = nsid self.bdev = bdev self.uuid = uuid - self.no_auto_visible = no_auto_visible + self.auto_visible = auto_visible self.anagrpid = anagrpid self.host_list = [] def __str__(self): - return f"nsid: {self.nsid}, bdev: {self.bdev}, uuid: {self.uuid}, no_auto_visible: {self.no_auto_visible}, anagrpid: {self.anagrpid}, hosts: {self.host_list}" + return f"nsid: {self.nsid}, bdev: {self.bdev}, uuid: {self.uuid}, auto_visible: {self.auto_visible}, anagrpid: {self.anagrpid}, hosts: {self.host_list}" def empty(self) -> bool: if self.bdev or self.uuid: @@ -191,6 +189,12 @@ def remove_host(self, host_nqn): except ValueError: pass + def remove_all_hosts(self): + self.host_list = [] + + def set_visibility(self, auto_visible: bool): + self.auto_visible = auto_visible + def is_host_in_namespace(self, host_nqn): return host_nqn in self.host_list @@ -216,10 +220,10 @@ def remove_namespace(self, nqn, nsid=None): else: self.namespace_list.pop(nqn, None) - def add_namespace(self, nqn, nsid, bdev, uuid, anagrpid, no_auto_visible): + def add_namespace(self, nqn, nsid, bdev, uuid, anagrpid, auto_visible): if not bdev: bdev = GatewayService.find_unique_bdev_name(uuid) - self.namespace_list[nqn][nsid] = NamespaceInfo(nsid, bdev, uuid, anagrpid, no_auto_visible) + self.namespace_list[nqn][nsid] = NamespaceInfo(nsid, bdev, uuid, anagrpid, auto_visible) def find_namespace(self, nqn, nsid, uuid = None) -> NamespaceInfo: if nqn not in self.namespace_list: @@ -238,7 +242,7 @@ def find_namespace(self, nqn, nsid, uuid = None) -> NamespaceInfo: return NamespacesLocalList.EMPTY_NAMESPACE - def get_namespace_count(self, nqn, no_auto_visible = None, min_hosts = 0) -> int: + def get_namespace_count(self, nqn, auto_visible = None, min_hosts = 0) -> int: if nqn and nqn not in self.namespace_list: return 0 @@ -246,15 +250,15 @@ def get_namespace_count(self, nqn, no_auto_visible = None, min_hosts = 0) -> int subsystems = [nqn] else: subsystems = self.namespace_list.keys() - + ns_count = 0 for one_subsys in subsystems: for nsid in self.namespace_list[one_subsys]: ns = self.namespace_list[one_subsys][nsid] if ns.empty(): continue - if no_auto_visible is not None: - if ns.no_auto_visible == no_auto_visible and ns.host_count() >= min_hosts: + if auto_visible is not None: + if ns.auto_visible == auto_visible and ns.host_count() >= min_hosts: ns_count += 1 else: if ns.host_count() >= min_hosts: @@ -388,7 +392,7 @@ def __init__(self, config: GatewayConfig, gateway_state: GatewayStateHandler, rp self.host_info = SubsystemHostAuth() self.up_and_running = True self.rebalance = Rebalance(self) - + def get_directories_for_key_file(self, key_type : str, subsysnqn : str, create_dir : bool = False) -> []: tmp_dirs = [] dir_prefix = f"{key_type}_{subsysnqn}_" @@ -425,7 +429,6 @@ def create_host_key_file(self, key_type : str, subsysnqn : str, hostnqn : str, k if not tmp_dir_names: return None - file = None filepath = None keyfile_prefix = f"{hostnqn}_" try: @@ -556,7 +559,7 @@ def is_valid_host_nqn(nqn): return pb2.req_status(status=rc[0], error_message=rc[1]) def parse_json_exeption(self, ex): - if type(ex) != JSONRPCException: + if not isinstance(ex, JSONRPCException): return None json_error_text = "Got JSON-RPC error response" @@ -1201,9 +1204,9 @@ def check_if_image_used(self, pool_name, image_name): continue return errmsg, nqn - def create_namespace(self, subsystem_nqn, bdev_name, nsid, anagrpid, uuid, no_auto_visible, context): + def create_namespace(self, subsystem_nqn, bdev_name, nsid, anagrpid, uuid, auto_visible, context): """Adds a namespace to a subsystem.""" - + if context: assert self.omap_lock.locked(), "OMAP is unlocked when calling create_namespace()" @@ -1219,7 +1222,7 @@ def create_namespace(self, subsystem_nqn, bdev_name, nsid, anagrpid, uuid, no_au add_namespace_error_prefix = f"Failure adding namespace{nsid_msg} to {subsystem_nqn}" peer_msg = self.get_peer_message(context) - self.logger.info(f"Received request to add {bdev_name} to {subsystem_nqn} with ANA group id {anagrpid}{nsid_msg}, no_auto_visible: {no_auto_visible}, context: {context}{peer_msg}") + self.logger.info(f"Received request to add {bdev_name} to {subsystem_nqn} with ANA group id {anagrpid}{nsid_msg}, auto_visible: {auto_visible}, context: {context}{peer_msg}") if subsystem_nqn not in self.subsys_max_ns: errmsg = f"{add_namespace_error_prefix}: No such subsystem" @@ -1236,9 +1239,9 @@ def create_namespace(self, subsystem_nqn, bdev_name, nsid, anagrpid, uuid, no_au self.logger.error(errmsg) return pb2.nsid_status(status=errno.EINVAL, error_message=errmsg) - if no_auto_visible and self.subsystem_nsid_bdev_and_uuid.get_namespace_count(subsystem_nqn, - True, 0) >= self.max_namespaces_with_netmask: - errmsg = f"{add_namespace_error_prefix}: Maximal number of namespaces which are not auto visible ({self.max_namespaces_with_netmask}) has already been reached" + if not auto_visible and self.subsystem_nsid_bdev_and_uuid.get_namespace_count(subsystem_nqn, + False, 0) >= self.max_namespaces_with_netmask: + errmsg = f"{add_namespace_error_prefix}: Maximal number of namespaces which are only visible to selected hosts ({self.max_namespaces_with_netmask}) has already been reached" self.logger.error(f"{errmsg}") return pb2.req_status(status=errno.E2BIG, error_message=errmsg) @@ -1271,13 +1274,15 @@ def create_namespace(self, subsystem_nqn, bdev_name, nsid, anagrpid, uuid, no_au nsid=nsid, anagrpid=anagrpid, uuid=uuid, - no_auto_visible=no_auto_visible, + no_auto_visible=not auto_visible, ) - self.subsystem_nsid_bdev_and_uuid.add_namespace(subsystem_nqn, nsid, bdev_name, uuid, anagrpid, no_auto_visible) + self.subsystem_nsid_bdev_and_uuid.add_namespace(subsystem_nqn, nsid, bdev_name, uuid, anagrpid, auto_visible) self.logger.debug(f"subsystem_add_ns: {nsid}") self.ana_grp_ns_load[anagrpid] += 1 - if anagrpid in self.ana_grp_subs_load and subsystem_nqn in self.ana_grp_subs_load[anagrpid]: self.ana_grp_subs_load[anagrpid][subsystem_nqn] += 1 - else : self.ana_grp_subs_load[anagrpid][subsystem_nqn] = 1 + if anagrpid in self.ana_grp_subs_load and subsystem_nqn in self.ana_grp_subs_load[anagrpid]: + self.ana_grp_subs_load[anagrpid][subsystem_nqn] += 1 + else: + self.ana_grp_subs_load[anagrpid][subsystem_nqn] = 1 except Exception as ex: self.logger.exception(add_namespace_error_prefix) errmsg = f"{add_namespace_error_prefix}:\n{ex}" @@ -1308,7 +1313,6 @@ def set_ana_state_safe(self, ana_info: pb2.ana_info, context=None): """Sets ana state for this gateway.""" self.logger.info(f"Received request to set ana states {ana_info.states}, {peer_msg}") - state = self.gateway_state.local.get_state() inaccessible_ana_groups = {} awaited_cluster_contexts = set() # Iterate over nqn_ana_states in ana_info @@ -1321,7 +1325,7 @@ def set_ana_state_safe(self, ana_info: pb2.ana_info, context=None): self.ana_grp_state[gs.grp_id] = gs.state # If this is not set the subsystem was not created yet - if not nqn in self.subsys_max_ns: + if nqn not in self.subsys_max_ns: continue self.logger.debug(f"Iterate over {nqn=} {self.subsystem_listeners[nqn]=}") @@ -1398,7 +1402,6 @@ def choose_anagrpid_for_namespace(self, nsid) ->int: self.logger.info(f"Found min loaded cluster: chosen ana group {chosen_ana_group} for ns {nsid} ") return chosen_ana_group - def namespace_add_safe(self, request, context): """Adds a namespace to a subsystem.""" @@ -1434,9 +1437,9 @@ def namespace_add_safe(self, request, context): ns = self.subsystem_nsid_bdev_and_uuid.find_namespace(request.subsystem_nqn, None, request.uuid) if not ns.empty(): - errmsg = f"Failure adding namespace, UUID {request.uuid} is already in use" - self.logger.error(f"{errmsg}") - return pb2.nsid_status(status=errno.EEXIST, error_message = errmsg) + errmsg = f"Failure adding namespace, UUID {request.uuid} is already in use" + self.logger.error(f"{errmsg}") + return pb2.nsid_status(status=errno.EEXIST, error_message = errmsg) omap_lock = self.omap_lock.get_omap_lock_to_use(context) with omap_lock: @@ -1488,7 +1491,7 @@ def namespace_add_safe(self, request, context): # If we got here we asserted that ret_bdev.bdev_name == bdev_name - ret_ns = self.create_namespace(request.subsystem_nqn, bdev_name, request.nsid, anagrp, request.uuid, request.no_auto_visible, context) + ret_ns = self.create_namespace(request.subsystem_nqn, bdev_name, request.nsid, anagrp, request.uuid, not request.no_auto_visible, context) if ret_ns.status == 0 and request.nsid and ret_ns.nsid != request.nsid: errmsg = f"Returned NSID {ret_ns.nsid} differs from requested one {request.nsid}" self.logger.error(errmsg) @@ -1547,7 +1550,8 @@ def namespace_change_load_balancing_group_safe(self, request, context): self.logger.error(errmsg) return pb2.req_status(status=errno.ENODEV, error_message=errmsg) - if context: #below checks are legal only if command is initiated by local cli or is sent from the local rebalance logic. + #below checks are legal only if command is initiated by local cli or is sent from the local rebalance logic. + if context: grps_list = self.ceph_utils.get_number_created_gateways(self.gateway_pool, self.gateway_group) if request.anagrpid not in grps_list: self.logger.debug(f"ANA groups: {grps_list}") @@ -1568,7 +1572,7 @@ def namespace_change_load_balancing_group_safe(self, request, context): try: state_ns = state[ns_key] ns_entry = json.loads(state_ns) - except Exception as ex: + except Exception: errmsg = f"{change_lb_group_failure_prefix}: Can't find entry for namespace {request.nsid} in {request.subsystem_nqn}" self.logger.error(errmsg) return pb2.req_status(status=errno.ENOENT, error_message=errmsg) @@ -1605,17 +1609,18 @@ def namespace_change_load_balancing_group_safe(self, request, context): self.logger.error(change_lb_group_failure_prefix) return pb2.req_status(status=errno.EINVAL, error_message=change_lb_group_failure_prefix) # change LB success - need to update the data structures - self.ana_grp_ns_load[anagrpid] -= 1 #decrease loading of previous "old" ana group + self.ana_grp_ns_load[anagrpid] -= 1 # decrease loading of previous "old" ana group self.ana_grp_subs_load[anagrpid][request.subsystem_nqn] -= 1 self.logger.debug(f"updated load in grp {anagrpid} = {self.ana_grp_ns_load[anagrpid]} ") self.ana_grp_ns_load[request.anagrpid] += 1 if request.anagrpid in self.ana_grp_subs_load and request.subsystem_nqn in self.ana_grp_subs_load[request.anagrpid]: self.ana_grp_subs_load[request.anagrpid][request.subsystem_nqn] += 1 - else : self.ana_grp_subs_load[request.anagrpid][request.subsystem_nqn] = 1 + else: + self.ana_grp_subs_load[request.anagrpid][request.subsystem_nqn] = 1 self.logger.debug(f"updated load in grp {request.anagrpid} = {self.ana_grp_ns_load[request.anagrpid]} ") #here update find_ret.set_ana_group_id(request.anagrpid) if not find_ret.empty(): - find_ret.set_ana_group_id(request.anagrpid) + find_ret.set_ana_group_id(request.anagrpid) if context: assert ns_entry, "Namespace entry is None for non-update call" @@ -1647,6 +1652,130 @@ def namespace_change_load_balancing_group(self, request, context=None): """Changes a namespace load balancing group.""" return self.execute_grpc_function(self.namespace_change_load_balancing_group_safe, request, context) + def subsystem_has_connections(self, subsys: str) -> bool: + assert subsys, "Subsystem NQN is empty" + try: + ctrl_ret = rpc_nvmf.nvmf_subsystem_get_controllers(self.spdk_rpc_client, nqn=subsys) + except Exception: + return False + if not ctrl_ret: + return False + return True + + def namespace_change_visibility_safe(self, request, context): + """Changes a namespace visibility.""" + + peer_msg = self.get_peer_message(context) + failure_prefix = f"Failure changing visibility for namespace {request.nsid} in {request.subsystem_nqn}" + vis_txt = "\"visible to all hosts\"" if request.auto_visible else "\"visible to selected hosts\"" + self.logger.info(f"Received request to change the visibility of namespace {request.nsid} in {request.subsystem_nqn} to {vis_txt}, force: {request.force}, context: {context}{peer_msg}") + + if not request.subsystem_nqn: + errmsg = f"Failure changing visibility for namespace, missing subsystem NQN" + self.logger.error(f"{errmsg}") + return pb2.req_status(status=errno.EINVAL, error_message=errmsg) + + if not request.nsid: + errmsg = f"Failure changing visibility for namespace in {request.subsystem_nqn}: No NSID was given" + self.logger.error(errmsg) + return pb2.req_status(status=errno.ENODEV, error_message=errmsg) + + find_ret = self.subsystem_nsid_bdev_and_uuid.find_namespace(request.subsystem_nqn, request.nsid) + if find_ret.empty(): + errmsg = f"{failure_prefix}: Can't find namespace" + self.logger.error(errmsg) + return pb2.req_status(status=errno.ENODEV, error_message=errmsg) + + if find_ret.host_count() > 0 and request.auto_visible: + if request.force: + self.logger.warning(f"Asking to change visibility of namespace {request.nsid} in {request.subsystem_nqn} to be visible to all hosts while there are already hosts added to it. Will continue as the \"--force\" parameter was used but these hosts will be removed from the namespace.") + else: + errmsg = f"{failure_prefix}: Asking to change visibility of namespace to be visible to all hosts while there are already hosts added to it. Either remove these hosts or use the \"--force\" parameter" + self.logger.error(errmsg) + return pb2.req_status(status=errno.EBUSY, error_message=errmsg) + + if self.subsystem_has_connections(request.subsystem_nqn): + if request.force: + self.logger.warning(f"Asking to change visibility of namespace {request.nsid} in {request.subsystem_nqn} while there are active connections on the subsystem, will continue as the \"--force\" parameter was used.") + else: + errmsg = f"{failure_prefix}: Asking to change visibility of namespace while there are active connections on the subsystem, please disconnect them or use the \"--force\" parameter." + self.logger.error(errmsg) + return pb2.req_status(status=errno.EBUSY, error_message=errmsg) + + omap_lock = self.omap_lock.get_omap_lock_to_use(context) + with omap_lock: + ns_entry = None + if context: + # notice that the local state might not be up to date in case we're in the middle of update() but as the + # context is not None, we are not in an update(), the omap lock made sure that we got here with an updated local state + state = self.gateway_state.local.get_state() + ns_key = GatewayState.build_namespace_key(request.subsystem_nqn, request.nsid) + try: + state_ns = state[ns_key] + ns_entry = json.loads(state_ns) + if ns_entry["no_auto_visible"] == (not request.auto_visible): + self.logger.warning(f"No change to namespace {request.nsid} in {request.subsystem_nqn} visibility, nothing to do") + return pb2.req_status(status=0, error_message=os.strerror(0)) + except Exception: + errmsg = f"{failure_prefix}: Can't find entry for namespace {request.nsid} in {request.subsystem_nqn}" + self.logger.error(errmsg) + return pb2.req_status(status=errno.ENOENT, error_message=errmsg) + try: + ret = rpc_nvmf.nvmf_subsystem_set_ns_visibility( + self.spdk_rpc_client, + nqn=request.subsystem_nqn, + nsid=request.nsid, + auto_visible=request.auto_visible, + ) + self.logger.debug(f"nvmf_subsystem_set_ns_visible: {ret}") + if request.force and find_ret.host_count() > 0 and request.auto_visible: + self.logger.warning(f"Removing all hosts added to namespace {request.nsid} in {request.subsystem_nqn} as it was set to be visible to all hosts") + find_ret.remove_all_hosts() + find_ret.set_visibility(request.auto_visible) + except Exception as ex: + errmsg = f"{failure_prefix}:\n{ex}" + resp = self.parse_json_exeption(ex) + status = errno.EINVAL + if resp: + status = resp["code"] + errmsg = f"{failure_prefix}: {resp['message']}" + return pb2.req_status(status=status, error_message=errmsg) + + # Just in case SPDK failed with no exception + if not ret: + self.logger.error(failure_prefix) + return pb2.req_status(status=errno.EINVAL, error_message=failure_prefix) + + if context: + assert ns_entry, "Namespace entry is None for non-update call" + # Update gateway state + try: + add_req = pb2.namespace_add_req(rbd_pool_name=ns_entry["rbd_pool_name"], + rbd_image_name=ns_entry["rbd_image_name"], + subsystem_nqn=ns_entry["subsystem_nqn"], + nsid=ns_entry["nsid"], + block_size=ns_entry["block_size"], + uuid=ns_entry["uuid"], + anagrpid=ns_entry["anagrpid"], + create_image=ns_entry["create_image"], + size=int(ns_entry["size"]), + force=ns_entry["force"], + no_auto_visible=not request.auto_visible) + json_req = json_format.MessageToJson( + add_req, preserving_proto_field_name=True, including_default_value_fields=True) + self.gateway_state.add_namespace(request.subsystem_nqn, request.nsid, json_req) + except Exception as ex: + errmsg = f"Error persisting visibility change for namespace {request.nsid} in {request.subsystem_nqn}" + self.logger.exception(errmsg) + errmsg = f"{errmsg}:\n{ex}" + return pb2.req_status(status=errno.EINVAL, error_message=errmsg) + + return pb2.req_status(status=0, error_message=os.strerror(0)) + + def namespace_change_visibility(self, request, context=None): + """Changes a namespace visibility.""" + return self.execute_grpc_function(self.namespace_change_visibility_safe, request, context) + def remove_namespace_from_state(self, nqn, nsid, context): if not context: return pb2.req_status(status=0, error_message=os.strerror(0)) @@ -1657,17 +1786,17 @@ def remove_namespace_from_state(self, nqn, nsid, context): # Update gateway state try: self.gateway_state.remove_namespace_qos(nqn, str(nsid)) - except Exception as ex: + except Exception: pass find_ret = self.subsystem_nsid_bdev_and_uuid.find_namespace(nqn, nsid) for hst in find_ret.host_list: try: self.gateway_state.remove_namespace_host(nqn, str(nsid), hst) - except Exception as ex: + except Exception: pass try: self.gateway_state.remove_namespace_lb_group(nqn, str(nsid)) - except Exception as ex: + except Exception: pass try: self.gateway_state.remove_namespace(nqn, str(nsid)) @@ -1801,12 +1930,11 @@ def list_namespaces(self, request, context=None): find_ret = self.subsystem_nsid_bdev_and_uuid.find_namespace(request.subsystem, nsid) if find_ret.empty(): self.logger.warning(f"Can't find info of namesapce {nsid} in {request.subsystem}. Visibility status will be inaccurate") - no_auto_visible = find_ret.no_auto_visible one_ns = pb2.namespace_cli(nsid = nsid, bdev_name = bdev_name, uuid = n["uuid"], load_balancing_group = lb_group, - no_auto_visible = no_auto_visible, + auto_visible = find_ret.auto_visible, hosts = find_ret.host_list) with self.rpc_lock: ns_bdev = self.get_bdev_info(bdev_name) @@ -1941,7 +2069,7 @@ def namespace_get_io_stats(self, request, context=None): pass return pb2.namespace_io_stats_info(status=errno.EINVAL, - error_message=f"Failure getting IO stats for namespace {nsid_msg}on {request.subsystem_nqn}: Error parsing returned stats:\n{exmsg}") + error_message=f"Failure getting IO stats for namespace {request.nsid} on {request.subsystem_nqn}: Error parsing returned stats:\n{exmsg}") def get_qos_limits_string(self, request): limits_to_set = "" @@ -2002,7 +2130,7 @@ def namespace_set_qos_limits_safe(self, request, context): try: state_ns_qos = state[ns_qos_key] ns_qos_entry = json.loads(state_ns_qos) - except Exception as ex: + except Exception: self.logger.info(f"No previous QOS limits found, this is the first time the limits are set for namespace {request.nsid} on {request.subsystem_nqn}") # Merge current limits with previous ones, if exist @@ -2207,7 +2335,7 @@ def namespace_add_host_safe(self, request, context): self.logger.error(errmsg) return pb2.namespace_io_stats_info(status=errno.ENODEV, error_message=errmsg) - if not find_ret.no_auto_visible: + if find_ret.auto_visible: errmsg = f"Failure adding host {request.host_nqn} to namespace {request.nsid} on {request.subsystem_nqn}: Namespace is visible to all hosts" self.logger.error(f"{errmsg}") return pb2.req_status(status=errno.EINVAL, error_message=errmsg) @@ -2227,8 +2355,7 @@ def namespace_add_host_safe(self, request, context): host=request.host_nqn ) self.logger.debug(f"ns_visible {request.host_nqn}: {ret}") - if not find_ret.empty(): - find_ret.add_host(request.host_nqn) + find_ret.add_host(request.host_nqn) # Just in case SPDK failed with no exception if not ret: @@ -2308,7 +2435,7 @@ def namespace_delete_host_safe(self, request, context): self.logger.error(errmsg) return pb2.namespace_io_stats_info(status=errno.ENODEV, error_message=errmsg) - if not find_ret.empty() and not find_ret.no_auto_visible: + if find_ret.auto_visible: errmsg = f"Failure deleting host from namespace {request.nsid} on {request.subsystem_nqn}: Namespace is visible to all hosts" self.logger.error(f"{errmsg}") return pb2.req_status(status=errno.EINVAL, error_message=errmsg) @@ -3027,7 +3154,6 @@ def list_connections_safe(self, request, context): adrfam = "" trtype = "TCP" hostnqn = conn["hostnqn"] - connected = False found = False secure = False psk = False @@ -3513,7 +3639,7 @@ def get_subsystems_safe(self, request, context): nonce = self.cluster_nonce[self.bdev_cluster[bdev]] n["nonce"] = nonce find_ret = self.subsystem_nsid_bdev_and_uuid.find_namespace(s["nqn"], n["nsid"]) - n["no_auto_visible"] = find_ret.no_auto_visible + n["auto_visible"] = find_ret.auto_visible n["hosts"] = find_ret.host_list # Parse the JSON dictionary into the protobuf message subsystem = pb2.subsystem() @@ -3577,7 +3703,7 @@ def change_subsystem_key_safe(self, request, context): try: state_subsys = state[subsys_key] subsys_entry = json.loads(state_subsys) - except Exception as ex: + except Exception: errmsg = f"{failure_prefix}: Can't find entry for subsystem {request.subsystem_nqn}" self.logger.error(errmsg) return pb2.req_status(status=errno.ENOENT, error_message=errmsg) @@ -3612,7 +3738,7 @@ def change_subsystem_key_safe(self, request, context): dhchap_key=hosts[hnqn]) try: self.change_host_key_safe(change_req, context) - except Excpetion: + except Exception: pass diff --git a/control/proto/gateway.proto b/control/proto/gateway.proto index d2d893c5..cee82a1d 100644 --- a/control/proto/gateway.proto +++ b/control/proto/gateway.proto @@ -60,6 +60,9 @@ service Gateway { // Changes namespace's load balancing group rpc namespace_change_load_balancing_group(namespace_change_load_balancing_group_req) returns (req_status) {} + // Changes namespace's visibility + rpc namespace_change_visibility(namespace_change_visibility_req) returns (req_status) {} + // Deletes a namespace rpc namespace_delete(namespace_delete_req) returns (req_status) {} @@ -168,6 +171,13 @@ message namespace_change_load_balancing_group_req { optional bool auto_lb_logic = 5; } +message namespace_change_visibility_req { + string subsystem_nqn = 1; + uint32 nsid = 2; + bool auto_visible = 3; + optional bool force = 4; +} + message namespace_delete_req { string subsystem_nqn = 1; uint32 nsid = 2; @@ -375,7 +385,7 @@ message namespace { optional string uuid = 5; optional uint32 anagrpid = 6; optional string nonce = 7; - optional bool no_auto_visible = 8; + optional bool auto_visible = 8; repeated string hosts = 9; } @@ -493,7 +503,7 @@ message namespace_cli { uint64 rw_mbytes_per_second = 10; uint64 r_mbytes_per_second = 11; uint64 w_mbytes_per_second = 12; - bool no_auto_visible = 13; + bool auto_visible = 13; repeated string hosts = 14; } diff --git a/control/server.py b/control/server.py index f1e889a1..fdbea7ee 100644 --- a/control/server.py +++ b/control/server.py @@ -15,7 +15,6 @@ import grpc import json import threading -import contextlib import time from concurrent import futures from google.protobuf import json_format @@ -144,7 +143,7 @@ def __exit__(self, exc_type, exc_value, traceback): if self.spdk_log_file: try: - close(self.spdk_log_file) + self.spdk_log_file.close() except Exception: pass self.spdk_log_file = None @@ -155,7 +154,7 @@ def __exit__(self, exc_type, exc_value, traceback): if self.monitor_client_log_file: try: - close(self.monitor_client_log_file) + self.monitor_client_log_file.close() except Exception: pass self.monitor_client_log_file = None @@ -250,13 +249,12 @@ def serve(self): self.server.start() # Set SPDK log level - log_level_args = {} log_level = self.config.get_with_default("spdk", "log_level", None) if log_level and log_level.strip(): log_level = log_level.strip().upper() log_req = pb2.set_spdk_nvmf_logs_req(log_level=log_level, print_level=log_level) self.gateway_rpc.set_spdk_nvmf_logs(log_req) - + self._register_service_map() # This should be at the end of the function, after the server is up @@ -472,7 +470,7 @@ def _start_spdk(self, omap_state): sockname = self.config.get_with_default("spdk", "rpc_socket_name", "spdk.sock") if sockname.find("/") >= 0: self.logger.error(f"Invalid SPDK socket name \"{sockname}\". Name should not contain a \"/\".") - raise(f"Invalid SPDK socket name.") + raise RuntimeError(f"Invalid SPDK socket name.") self.spdk_rpc_socket_path = sockdir + sockname self.logger.info(f"SPDK Socket: {self.spdk_rpc_socket_path}") spdk_tgt_cmd_extra_args = self.config.get_with_default( @@ -609,7 +607,7 @@ def _stop_monitor_client(self): self.monitor_client_process = None if self.monitor_client_log_file: try: - close(self.monitor_client_log_file) + self.monitor_client_log_file.close() except Exception: pass self.monitor_client_log_file = None @@ -625,7 +623,7 @@ def _stop_spdk(self): self.spdk_process = None if self.spdk_log_file: try: - close(self.spdk_log_file) + self.spdk_log_file.close() except Exception: pass self.spdk_log_file = None @@ -671,8 +669,7 @@ def _create_transport(self, trtype): raise try: - status = rpc_nvmf.nvmf_create_transport( - self.spdk_rpc_client, **args) + rpc_nvmf.nvmf_create_transport( self.spdk_rpc_client, **args) except Exception: self.logger.exception(f"Create Transport {trtype} returned with error") raise @@ -717,7 +714,7 @@ def keep_alive(self): def _ping(self): """Confirms communication with SPDK process.""" try: - ret = spdk.rpc.spdk_get_version(self.spdk_rpc_ping_client) + spdk.rpc.spdk_get_version(self.spdk_rpc_ping_client) return True except Exception: self.logger.exception(f"spdk_get_version failed") @@ -759,7 +756,7 @@ def probe_huge_pages(self): self.logger.warning(f"The actual huge page count {hugepages_val} is smaller than the requested value of {requested_hugepages_val}") else: self.logger.warning(f"Can't read actual huge pages count value from {hugepages_file}") - except Exception as ex: + except Exception: self.logger.exception(f"Can't read actual huge pages count value from {hugepages_file}") else: self.logger.warning(f"Can't find huge pages file {hugepages_file}") @@ -810,6 +807,10 @@ def gateway_rpc_caller(self, requests, is_add_req): if is_add_req: req = json_format.Parse(val, pb2.namespace_change_load_balancing_group_req(), ignore_unknown_fields=True) self.gateway_rpc.namespace_change_load_balancing_group(req) + elif key.startswith(GatewayState.NAMESPACE_VISIBILITY_PREFIX): + if is_add_req: + req = json_format.Parse(val, pb2.namespace_change_visibility_req(), ignore_unknown_fields=True) + self.gateway_rpc.namespace_change_visibility(req) elif key.startswith(GatewayState.NAMESPACE_HOST_PREFIX): if is_add_req: req = json_format.Parse(val, pb2.namespace_add_host_req(), ignore_unknown_fields=True) diff --git a/control/state.py b/control/state.py index 90837165..7562e0b9 100644 --- a/control/state.py +++ b/control/state.py @@ -37,9 +37,10 @@ class GatewayState(ABC): NAMESPACE_QOS_PREFIX = "qos" + OMAP_KEY_DELIMITER NAMESPACE_LB_GROUP_PREFIX = "lbgroup" + OMAP_KEY_DELIMITER NAMESPACE_HOST_PREFIX = "ns-host" + OMAP_KEY_DELIMITER + NAMESPACE_VISIBILITY_PREFIX = "ns-visibility" + OMAP_KEY_DELIMITER def is_key_element_valid(s: str) -> bool: - if type(s) != str: + if not isinstance(s, str): return False if GatewayState.OMAP_KEY_DELIMITER in s: return False @@ -57,6 +58,12 @@ def build_namespace_lbgroup_key(subsystem_nqn: str, nsid) -> str: key += GatewayState.OMAP_KEY_DELIMITER + str(nsid) return key + def build_namespace_visibility_key(subsystem_nqn: str, nsid) -> str: + key = GatewayState.NAMESPACE_VISIBILITY_PREFIX + subsystem_nqn + if nsid is not None: + key += GatewayState.OMAP_KEY_DELIMITER + str(nsid) + return key + def build_namespace_qos_key(subsystem_nqn: str, nsid) -> str: key = GatewayState.NAMESPACE_QOS_PREFIX + subsystem_nqn if nsid is not None: @@ -339,11 +346,11 @@ def lock_omap(self): if i > 0: self.logger.info(f"Succeeded to lock OMAP file after {i} retries") break - except rados.ObjectExists as ex: + except rados.ObjectExists: self.logger.info(f"We already locked the OMAP file") got_lock = True break - except rados.ObjectBusy as ex: + except rados.ObjectBusy: self.logger.warning( f"The OMAP file is locked, will try again in {self.omap_file_lock_retry_sleep_interval} seconds") with ReleasedLock(self.rpc_lock): @@ -378,7 +385,7 @@ def unlock_omap(self): try: self.omap_state.ioctx.unlock(self.omap_state.omap_name, self.OMAP_FILE_LOCK_NAME, self.OMAP_FILE_LOCK_COOKIE) - except rados.ObjectNotFound as ex: + except rados.ObjectNotFound: if self.is_locked: self.logger.warning(f"No such lock, the lock duration might have passed") except Exception: @@ -526,7 +533,7 @@ def _add_key(self, key: str, val: str): # Notify other gateways within the group of change try: self.ioctx.notify(self.omap_name, timeout_ms = self.notify_timeout) - except Exception as ex: + except Exception: self.logger.warning(f"Failed to notify.") def _remove_key(self, key: str): @@ -553,7 +560,7 @@ def _remove_key(self, key: str): # Notify other gateways within the group of change try: self.ioctx.notify(self.omap_name, timeout_ms = self.notify_timeout) - except Exception as ex: + except Exception: self.logger.warning(f"Failed to notify.") def delete_state(self): @@ -762,6 +769,30 @@ def namespace_only_lb_group_id_changed(self, old_val, new_val): return (False, None) return (True, new_req.anagrpid) + def namespace_only_visibility_changed(self, old_val, new_val): + # If only the visibility field has changed we can use change_visibility request instead of re-adding the namespace + old_req = None + new_req = None + try: + old_req = json_format.Parse(old_val, pb2.namespace_add_req(), ignore_unknown_fields=True) + except json_format.ParseError: + self.logger.exception(f"Got exception parsing {old_val}") + return (False, None) + try: + new_req = json_format.Parse(new_val, pb2.namespace_add_req(), ignore_unknown_fields=True) + except json_format.ParseError: + self.logger.exeption(f"Got exception parsing {new_val}") + return (False, None) + if not old_req or not new_req: + self.logger.debug(f"Failed to parse requests, old: {old_val} -> {old_req}, new: {new_val} -> {new_req}") + return (False, None) + assert old_req != new_req, f"Something was wrong we shouldn't get identical old and new values ({old_req})" + old_req.auto_visible = new_req.auto_visible + if old_req != new_req: + # Something besides the group id is different + return (False, None) + return (True, new_req.auto_visible) + def host_only_key_changed(self, old_val, new_val): # If only the dhchap key has changed we can use change_key request instead of re-adding the host old_req = None @@ -877,7 +908,7 @@ def break_subsystem_key(self, subsys_key: str): return subsys_nqn def get_str_from_bytes(val): - val_str = val.decode() if type(val) == type(b'') else val + val_str = val.decode() if isinstance(val, bytes) else val return val_str def compare_state_values(val1, val2) -> bool: @@ -933,6 +964,7 @@ def update(self) -> bool: # Handle namespace changes in which only the load balancing group id was changed only_lb_group_changed = [] + only_visibility_changed = [] only_host_key_changed = [] only_subsystem_key_changed = [] ns_prefix = GatewayState.build_namespace_key("nqn", None) @@ -940,35 +972,32 @@ def update(self) -> bool: subsystem_prefix = GatewayState.build_subsystem_key("nqn") for key in changed.keys(): if key.startswith(ns_prefix): - try: - (should_process, new_lb_grp_id) = self.namespace_only_lb_group_id_changed(local_state_dict[key], - omap_state_dict[key]) - if should_process: - assert new_lb_grp_id, "Shouldn't get here with an empty lb group id" - self.logger.debug(f"Found {key} where only the load balancing group id has changed. The new group id is {new_lb_grp_id}") - only_lb_group_changed.append((key, new_lb_grp_id)) - except Exception as ex: - self.logger.warning("Got exception checking namespace for load balancing group id change") + (should_process, new_lb_grp_id) = self.namespace_only_lb_group_id_changed(local_state_dict[key], + omap_state_dict[key]) + if should_process: + assert new_lb_grp_id, "Shouldn't get here with an empty lb group id" + self.logger.debug(f"Found {key} where only the load balancing group id has changed. The new group id is {new_lb_grp_id}") + only_lb_group_changed.append((key, new_lb_grp_id)) + + (should_process, + new_visibility) = self.namespace_only_visibility_changed(local_state_dict[key], omap_state_dict[key]) + if should_process: + self.logger.debug(f"Found {key} where only the visibility has changed. The new visibility is {new_visibility}") + only_visibility_changed.append((key, new_visibility)) elif key.startswith(host_prefix): - try: - (should_process, - new_dhchap_key) = self.host_only_key_changed(local_state_dict[key], omap_state_dict[key]) - if should_process: - assert new_dhchap_key, "Shouldn't get here with an empty dhchap key" - self.logger.debug(f"Found {key} where only the key has changed. The new DHCHAP key is {new_dhchap_key}") - only_host_key_changed.append((key, new_dhchap_key)) - except Exception as ex: - self.logger.warning("Got exception checking host for key change") + (should_process, + new_dhchap_key) = self.host_only_key_changed(local_state_dict[key], omap_state_dict[key]) + if should_process: + assert new_dhchap_key, "Shouldn't get here with an empty dhchap key" + self.logger.debug(f"Found {key} where only the key has changed. The new DHCHAP key is {new_dhchap_key}") + only_host_key_changed.append((key, new_dhchap_key)) elif key.startswith(subsystem_prefix): - try: - (should_process, - new_dhchap_key) = self.subsystem_only_key_changed(local_state_dict[key], omap_state_dict[key]) - if should_process: - assert new_dhchap_key, "Shouldn't get here with an empty dhchap key" - self.logger.debug(f"Found {key} where only the key has changed. The new DHCHAP key is {new_dhchap_key}") - only_subsystem_key_changed.append((key, new_dhchap_key)) - except Exception as ex: - self.logger.warning("Got exception checking subsystem for key change") + (should_process, + new_dhchap_key) = self.subsystem_only_key_changed(local_state_dict[key], omap_state_dict[key]) + if should_process: + assert new_dhchap_key, "Shouldn't get here with an empty dhchap key" + self.logger.debug(f"Found {key} where only the key has changed. The new DHCHAP key is {new_dhchap_key}") + only_subsystem_key_changed.append((key, new_dhchap_key)) for ns_key, new_lb_grp in only_lb_group_changed: ns_nqn = None @@ -976,8 +1005,8 @@ def update(self) -> bool: try: changed.pop(ns_key) (ns_nqn, ns_nsid) = self.break_namespace_key(ns_key) - except Exception as ex: - self.logger.error(f"Exception removing {ns_key} from {changed}:\n{ex}") + except Exception: + self.logger.exception(f"Exception removing {ns_key} from {changed}") if ns_nqn and ns_nsid: try: lbgroup_key = GatewayState.build_namespace_lbgroup_key(ns_nqn, ns_nsid) @@ -986,8 +1015,27 @@ def update(self) -> bool: json_req = json_format.MessageToJson(req, preserving_proto_field_name=True, including_default_value_fields=True) added[lbgroup_key] = json_req - except Exception as ex: - self.logger.error(f"Exception formatting change namespace load balancing group request:\n{ex}") + except Exception: + self.logger.exception(f"Exception formatting change namespace load balancing group request") + + for ns_key, new_visibility in only_visibility_changed: + ns_nqn = None + ns_nsid = None + try: + changed.pop(ns_key) + (ns_nqn, ns_nsid) = self.break_namespace_key(ns_key) + except Exception: + self.logger.exception(f"Exception removing {ns_key} from {changed}") + if ns_nqn and ns_nsid: + try: + visibility_key = GatewayState.build_namespace_visibility_key(ns_nqn, ns_nsid) + req = pb2.namespace_change_visibility_req(subsystem_nqn=ns_nqn, nsid=ns_nsid, + auto_visible=new_visibility, force=True) + json_req = json_format.MessageToJson(req, preserving_proto_field_name=True, + including_default_value_fields=True) + added[visibility_key] = json_req + except Exception: + self.logger.exception(f"Exception formatting change namespace visibility request") for host_key, new_dhchap_key in only_host_key_changed: subsys_nqn = None @@ -995,8 +1043,8 @@ def update(self) -> bool: try: changed.pop(host_key) (subsys_nqn, host_nqn) = self.break_host_key(host_key) - except Exception as ex: - self.logger.error(f"Exception removing {host_key} from {changed}:\n{ex}") + except Exception: + self.logger.exception(f"Exception removing {host_key} from {changed}") if host_nqn == "*": self.logger.warning(f"Something went wrong, host \"*\" can't have DH-HMAC-CHAP keys, ignore") continue @@ -1016,8 +1064,8 @@ def update(self) -> bool: try: changed.pop(subsys_key) subsys_nqn = self.break_subsystem_key(subsys_key) - except Exception as ex: - self.logger.error(f"Exception removing {subsys_key} from {changed}:\n{ex}") + except Exception: + self.logger.exception(f"Exception removing {subsys_key} from {changed}") if subsys_nqn: try: subsys_key_key = GatewayState.build_subsystem_key_key(subsys_nqn) @@ -1028,12 +1076,14 @@ def update(self) -> bool: except Exception as ex: self.logger.error(f"Exception formatting change subsystem key request:\n{ex}") - if len(only_lb_group_changed) > 0 or len(only_host_key_changed) > 0 or len(only_subsystem_key_changed) > 0: + if len(only_lb_group_changed) > 0 or len(only_host_key_changed) > 0 or len(only_subsystem_key_changed) > 0 or len(only_visibility_changed) > 0: grouped_changed = self._group_by_prefix(changed, prefix_list) if len(only_subsystem_key_changed) > 0: prefix_list += [GatewayState.SUBSYSTEM_KEY_PREFIX] if len(only_lb_group_changed) > 0: prefix_list += [GatewayState.NAMESPACE_LB_GROUP_PREFIX] + if len(only_visibility_changed) > 0: + prefix_list += [GatewayState.NAMESPACE_VISIBILITY_PREFIX] if len(only_host_key_changed) > 0: prefix_list += [GatewayState.HOST_KEY_PREFIX] grouped_added = self._group_by_prefix(added, prefix_list) diff --git a/spdk b/spdk index ef5d1aeb..4ee571e3 160000 --- a/spdk +++ b/spdk @@ -1 +1 @@ -Subproject commit ef5d1aebfedc30965e5c416da00015d73ad3bf62 +Subproject commit 4ee571e3e500d19eca66e5a72f04bf11c391f615 diff --git a/tests/ha/demo_test.sh b/tests/ha/demo_test.sh index 6f9374b2..e73cf71d 100755 --- a/tests/ha/demo_test.sh +++ b/tests/ha/demo_test.sh @@ -190,6 +190,67 @@ function demo_bdevperf_unsecured() [[ `echo $conns | jq -r '.subsystem_nqn'` == "${NQN}" ]] [[ `echo $conns | jq -r '.connections[0]'` == "null" ]] + echo "ℹ️ bdevperf tcp connect ip: $NVMEOF_IP_ADDRESS port: $NVMEOF_IO_PORT nqn: $NQN, host not in namespace netmask" + devs=`make exec -s SVC=bdevperf OPTS=-T CMD="$rpc -v -s $BDEVPERF_SOCKET bdev_nvme_attach_controller -b Nvme0 -t tcp -a $NVMEOF_IP_ADDRESS -s $NVMEOF_IO_PORT -f ipv4 -n $NQN -q ${NQN}host -l -1 -o 10"` + [[ "$devs" == "Nvme0n1" ]] + + echo "ℹ️ verify connection list" + conns=$(cephnvmf_func --output stdio --format json connection list --subsystem $NQN) + [[ `echo $conns | jq -r '.status'` == "0" ]] + [[ `echo $conns | jq -r '.subsystem_nqn'` == "${NQN}" ]] + [[ `echo $conns | jq -r '.connections[0].nqn'` == "${NQN}host" ]] + [[ `echo $conns | jq -r '.connections[0].trsvcid'` == "${NVMEOF_IO_PORT}" ]] + [[ `echo $conns | jq -r '.connections[0].traddr'` == "${NVMEOF_IP_ADDRESS}" ]] + [[ `echo $conns | jq -r '.connections[0].adrfam'` == "ipv4" ]] + [[ `echo $conns | jq -r '.connections[0].trtype'` == "TCP" ]] + [[ `echo $conns | jq -r '.connections[0].connected'` == "true" ]] + [[ `echo $conns | jq -r '.connections[0].qpairs_count'` == "1" ]] + [[ `echo $conns | jq -r '.connections[0].secure'` == "false" ]] + [[ `echo $conns | jq -r '.connections[0].use_psk'` == "false" ]] + [[ `echo $conns | jq -r '.connections[0].use_dhchap'` == "false" ]] + [[ `echo $conns | jq -r '.connections[1]'` == "null" ]] + + echo "ℹ️ change namespace visibility" + set +e + cephnvmf_func namespace change_visibility --subsystem $NQN --nsid 2 --auto-visible + if [[ $? -eq 0 ]]; then + echo "Changing namespace visibility with active connections should fail" + exit 1 + fi + set -e + + cephnvmf_func namespace change_visibility --subsystem $NQN --nsid 2 --auto-visible --force + make exec SVC=bdevperf OPTS=-T CMD="$rpc -v -s $BDEVPERF_SOCKET bdev_nvme_detach_controller Nvme0" + set +e + cephnvmf_func namespace change_visibility --subsystem $NQN --nsid 5 --no-auto-visible + if [[ $? -eq 0 ]]; then + echo "Changing visibility of a non-existing namespace should fail" + exit 1 + fi + set -e + + cephnvmf_func namespace change_visibility --subsystem $NQN --nsid 2 --no-auto-visible + + echo "ℹ️ bdevperf tcp connect ip: $NVMEOF_IP_ADDRESS port: $NVMEOF_IO_PORT nqn: $NQN, after changing visibility" + devs=`make exec -s SVC=bdevperf OPTS=-T CMD="$rpc -v -s $BDEVPERF_SOCKET bdev_nvme_attach_controller -b Nvme0 -t tcp -a $NVMEOF_IP_ADDRESS -s $NVMEOF_IO_PORT -f ipv4 -n $NQN -q ${NQN}host -l -1 -o 10"` + [[ "$devs" == "Nvme0n1" ]] + make exec SVC=bdevperf OPTS=-T CMD="$rpc -v -s $BDEVPERF_SOCKET bdev_nvme_detach_controller Nvme0" + + echo "ℹ️ add host to namespace again, wrong NSID" + set +e + cephnvmf_func namespace add_host --subsystem $NQN --nsid 5 --host-nqn ${NQN}host + if [[ $? -eq 0 ]]; then + echo "Adding host to a non-existing namespace should fail" + exit 1 + fi + set -e + + echo "ℹ️ add host to namespace again" + cephnvmf_func namespace add_host --subsystem $NQN --nsid 2 --host-nqn ${NQN}host + devs=`make exec -s SVC=bdevperf OPTS=-T CMD="$rpc -v -s $BDEVPERF_SOCKET bdev_nvme_attach_controller -b Nvme0 -t tcp -a $NVMEOF_IP_ADDRESS -s $NVMEOF_IO_PORT -f ipv4 -n $NQN -q ${NQN}host -l -1 -o 10"` + [[ "$devs" == "Nvme0n1 Nvme0n2" ]] + make exec SVC=bdevperf OPTS=-T CMD="$rpc -v -s $BDEVPERF_SOCKET bdev_nvme_detach_controller Nvme0" + return $? } diff --git a/tests/test_cli.py b/tests/test_cli.py index af678020..71060df6 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -9,7 +9,6 @@ from control.proto import gateway_pb2 as pb2 from control.proto import gateway_pb2_grpc as pb2_grpc import os -import copy image = "mytestdevimage" image2 = "mytestdevimage2" @@ -24,6 +23,7 @@ image11 = "mytestdevimage11" image12 = "mytestdevimage12" image13 = "mytestdevimage13" +image14 = "mytestdevimage14" pool = "rbd" subsystem = "nqn.2016-06.io.spdk:cnode1" subsystem2 = "nqn.2016-06.io.spdk:cnode2" @@ -45,6 +45,9 @@ host5 = "nqn.2016-06.io.spdk:host5" host6 = "nqn.2016-06.io.spdk:host6" host7 = "nqn.2016-06.io.spdk:host7" +host8 = "nqn.2016-06.io.spdk:host8" +host9 = "nqn.2016-06.io.spdk:host9" +host10 = "nqn.2016-06.io.spdk:host10" nsid = "1" anagrpid = "1" anagrpid2 = "2" @@ -163,7 +166,7 @@ def test_get_gateway_info(self, caplog, gateway): assert gw_info.max_namespaces_per_subsystem == 11 assert gw_info.max_hosts_per_subsystem == 4 assert gw_info.status == 0 - assert gw_info.bool_status == True + assert gw_info.bool_status class TestCreate: def test_create_subsystem(self, caplog, gateway): @@ -489,19 +492,80 @@ def test_add_namespace_no_auto_visible(self, caplog, gateway): def test_add_host_to_namespace(self, caplog, gateway): caplog.clear() - cli(["namespace", "add_host", "--subsystem", subsystem, "--nsid", "8", "--host-nqn", "nqn.2016-06.io.spdk:host8"]) - assert f"Adding host nqn.2016-06.io.spdk:host8 to namespace 8 on {subsystem}: Successful" in caplog.text + cli(["namespace", "add_host", "--subsystem", subsystem, "--nsid", "8", "--host-nqn", host8]) + assert f"Adding host {host8} to namespace 8 on {subsystem}: Successful" in caplog.text + caplog.clear() + cli(["namespace", "add_host", "--subsystem", subsystem, "--nsid", "9", "--host-nqn", host8]) + assert f"Adding host {host8} to namespace 9 on {subsystem}: Successful" in caplog.text def test_add_too_many_hosts_to_namespace(self, caplog, gateway): caplog.clear() - cli(["namespace", "add_host", "--subsystem", subsystem, "--nsid", "8", "--host-nqn", "nqn.2016-06.io.spdk:host9"]) - assert f"Failure adding host nqn.2016-06.io.spdk:host9 to namespace 8 on {subsystem}, maximal host count for namespace (1) was already reached" in caplog.text + cli(["namespace", "add_host", "--subsystem", subsystem, "--nsid", "8", "--host-nqn", host9]) + assert f"Failure adding host {host9} to namespace 8 on {subsystem}, maximal host count for namespace (1) was already reached" in caplog.text def test_add_all_hosts_to_namespace(self, caplog, gateway): caplog.clear() cli(["namespace", "add_host", "--subsystem", subsystem, "--nsid", "8", "--host-nqn", "*"]) assert f"Failure adding host to namespace 8 on {subsystem}: Host NQN can't be \"*\"" in caplog.text + def test_change_namespace_visibility(self, caplog, gateway): + caplog.clear() + cli(["namespace", "change_visibility", "--subsystem", subsystem, "--nsid", "8", "--auto-visible"]) + assert f"Failure changing visibility for namespace 8 in {subsystem}: Asking to change visibility of namespace to be visible to all hosts while there are already hosts added to it." in caplog.text + caplog.clear() + cli(["namespace", "change_visibility", "--subsystem", subsystem, "--nsid", "8", "--auto-visible", "--force"]) + assert f'Asking to change visibility of namespace 8 in {subsystem} to be visible to all hosts while there are already hosts added to it. Will continue as the "--force" parameter was used but these hosts will be removed from the namespace.' in caplog.text + assert f'Changing visibility of namespace 8 in {subsystem} to "visible to all hosts": Successful' in caplog.text + caplog.clear() + cli(["namespace", "change_visibility", "--subsystem", subsystem, "--nsid", "8", "--auto-visible"]) + assert f'Changing visibility of namespace 8 in {subsystem} to "visible to all hosts": Successful' in caplog.text + assert f"No change to namespace 8 in {subsystem} visibility, nothing to do" in caplog.text + caplog.clear() + cli(["--format", "json", "namespace", "list", "--subsystem", subsystem, "--nsid", "8"]) + assert f'"nsid": 8' in caplog.text + assert f'"auto_visible": true' in caplog.text + assert f'"hosts": []' in caplog.text + caplog.clear() + cli(["namespace", "add_host", "--subsystem", subsystem, "--nsid", "8", "--host-nqn", host8]) + assert f"Failure adding host {host8} to namespace 8 on {subsystem}: Namespace is visible to all hosts" in caplog.text + caplog.clear() + cli(["namespace", "change_visibility", "--subsystem", subsystem, "--nsid", "8", "--no-auto-visible"]) + assert f'Changing visibility of namespace 8 in {subsystem} to "visible to selected hosts": Successful' in caplog.text + caplog.clear() + cli(["--format", "json", "namespace", "list", "--subsystem", subsystem, "--nsid", "8"]) + assert f'"nsid": 8' in caplog.text + assert '"auto_visible":' not in caplog.text or f'"auto_visible": false' in caplog.text + assert f'"hosts": []' in caplog.text + caplog.clear() + cli(["namespace", "add_host", "--subsystem", subsystem, "--nsid", "8", "--host-nqn", host8]) + assert f"Adding host {host8} to namespace 8 on {subsystem}: Successful" in caplog.text + caplog.clear() + cli(["--format", "json", "namespace", "list", "--subsystem", subsystem, "--nsid", "8"]) + assert f'"nsid": 8' in caplog.text + assert '"auto_visible":' not in caplog.text or f'"auto_visible": false' in caplog.text + assert f'"hosts": []' not in caplog.text + assert f"{host8}" in caplog.text + + def test_change_namespace_visibility_wrong_params(self, caplog, gateway): + caplog.clear() + rc = 0 + try: + cli(["namespace", "change_visibility", "--subsystem", subsystem, "--nsid", "8"]) + except SystemExit as sysex: + rc = int(str(sysex)) + pass + assert "Either --auto-visible or --no-auto-visible should be specified" in caplog.text + assert rc == 2 + caplog.clear() + rc = 0 + try: + cli(["namespace", "change_visibility", "--subsystem", subsystem, "--nsid", "8", "--auto-visible", "--no-auto-visible"]) + except SystemExit as sysex: + rc = int(str(sysex)) + pass + assert "--auto-visible and --no-auto-visible are mutually exclusive" in caplog.text + assert rc == 2 + def test_add_namespace_no_such_subsys(self, caplog, gateway): caplog.clear() cli(["namespace", "add", "--subsystem", f"{subsystem3}", "--rbd-pool", pool, "--rbd-image", image13, "--size", "16MB", "--rbd-create-image"]) @@ -543,29 +607,30 @@ def test_add_host_to_namespace_junk_subsystem(self, caplog, gateway): def test_add_host_to_wrong_namespace(self, caplog, gateway): caplog.clear() - cli(["namespace", "add_host", "--subsystem", subsystem, "--nsid", "1", "--host-nqn", "nqn.2016-06.io.spdk:host10"]) - assert f"Failure adding host nqn.2016-06.io.spdk:host10 to namespace 1 on {subsystem}: Namespace is visible to all hosts" in caplog.text + cli(["namespace", "add_host", "--subsystem", subsystem, "--nsid", "1", "--host-nqn", host10]) + assert f"Failure adding host {host10} to namespace 1 on {subsystem}: Namespace is visible to all hosts" in caplog.text def test_add_too_many_namespaces_with_hosts(self, caplog, gateway): caplog.clear() cli(["namespace", "add", "--subsystem", subsystem, "--rbd-pool", pool, "--rbd-image", image8, "--size", "16MB", "--rbd-create-image", "--no-auto-visible"]) - assert f"Failure adding namespace to {subsystem}: Maximal number of namespaces which are not auto visible (3) has already been reached" in caplog.text + assert f"Failure adding namespace to {subsystem}: Maximal number of namespaces which are only visible to selected hosts (3) has already been reached" in caplog.text caplog.clear() - cli(["namespace", "add", "--subsystem", subsystem, "--rbd-pool", pool, "--rbd-image", image8, "--size", "16MB", "--rbd-create-image"]) + cli(["namespace", "add", "--subsystem", subsystem, "--rbd-pool", pool, "--rbd-image", image14, "--size", "16MB", "--rbd-create-image"]) assert f"Adding namespace 11 to {subsystem}: Successful" in caplog.text def test_list_namespace_with_hosts(self, caplog, gateway): caplog.clear() - cli(["--format", "json", "namespace", "list", "--subsystem", subsystem, "--nsid", "8"]) - assert f'"nsid": 8' in caplog.text - assert f'"no_auto_visible": true' in caplog.text - assert f'"nqn.2016-06.io.spdk:host8"' in caplog.text + cli(["--format", "json", "namespace", "list", "--subsystem", subsystem, "--nsid", "9"]) + assert f'"nsid": 9' in caplog.text + assert '"auto_visible":' not in caplog.text or f'"auto_visible": false' in caplog.text + assert f'"{host8}"' in caplog.text + assert f'"hosts": []' not in caplog.text def test_list_namespace_with_no_hosts(self, caplog, gateway): caplog.clear() cli(["--format", "json", "namespace", "list", "--subsystem", subsystem, "--nsid", "10"]) assert f'"nsid": 10' in caplog.text - assert f'"no_auto_visible": true' in caplog.text + assert '"auto_visible":' not in caplog.text or f'"auto_visible": false' in caplog.text assert f'"hosts": []' in caplog.text def test_add_too_many_namespaces(self, caplog, gateway): @@ -1008,11 +1073,11 @@ def test_remove_namespace(self, caplog, gateway): assert bdev_found caplog.clear() del_ns_req = pb2.namespace_delete_req(subsystem_nqn=subsystem) - ret = stub.namespace_delete(del_ns_req) + stub.namespace_delete(del_ns_req) assert "Failure deleting namespace, missing NSID" in caplog.text caplog.clear() del_ns_req = pb2.namespace_delete_req(nsid=1) - ret = stub.namespace_delete(del_ns_req) + stub.namespace_delete(del_ns_req) assert "Failure deleting namespace 1, missing subsystem NQN" in caplog.text caplog.clear() cli(["namespace", "del", "--subsystem", subsystem, "--nsid", "6"]) diff --git a/tests/test_cli_change_keys.py b/tests/test_cli_change_keys.py index e053c1a5..f3392117 100644 --- a/tests/test_cli_change_keys.py +++ b/tests/test_cli_change_keys.py @@ -1,11 +1,8 @@ import pytest from control.server import GatewayServer from control.cli import main as cli -from control.cli import main_test as cli_test from control.cephutils import CephUtils -import spdk.rpc.nvmf as rpc_nvmf import grpc -from control.proto import gateway_pb2 as pb2 from control.proto import gateway_pb2_grpc as pb2_grpc import copy import time @@ -42,15 +39,12 @@ def two_gateways(config): configA.config["gateway"]["override_hostname"] = nameA configA.config["spdk"]["rpc_socket_name"] = sockA configA.config["spdk"]["tgt_cmd_extra_args"] = "-m 0x03" - portA = configA.getint("gateway", "port") + 1 - configA.config["gateway"]["port"] = str(portA) - discPortA = configA.getint("discovery", "port") + 1 - configA.config["discovery"]["port"] = str(discPortA) + portA = configA.getint("gateway", "port") configB.config["gateway"]["name"] = nameB configB.config["gateway"]["override_hostname"] = nameB configB.config["spdk"]["rpc_socket_name"] = sockB - portB = portA + 1 - discPortB = discPortA + 1 + portB = portA + 2 + discPortB = configB.getint("discovery", "port") + 1 configB.config["gateway"]["port"] = str(portB) configB.config["discovery"]["port"] = str(discPortB) configB.config["spdk"]["tgt_cmd_extra_args"] = "-m 0x0C" @@ -75,20 +69,18 @@ def two_gateways(config): def test_change_host_key(caplog, two_gateways): gatewayA, stubA, gatewayB, stubB = two_gateways - gwA = gatewayA.gateway_rpc - gwB = gatewayB.gateway_rpc caplog.clear() - cli(["--server-port", "5501", "subsystem", "add", "--subsystem", subsystem]) + cli(["subsystem", "add", "--subsystem", subsystem]) assert f"create_subsystem {subsystem}: True" in caplog.text caplog.clear() - cli(["--server-port", "5501", "host", "add", "--subsystem", subsystem, "--host-nqn", hostnqn1]) + cli(["host", "add", "--subsystem", subsystem, "--host-nqn", hostnqn1]) assert f"Adding host {hostnqn1} to {subsystem}: Successful" in caplog.text caplog.clear() - cli(["--server-port", "5501", "host", "add", "--subsystem", subsystem, "--host-nqn", hostnqn2, "--dhchap-key", key1]) + cli(["host", "add", "--subsystem", subsystem, "--host-nqn", hostnqn2, "--dhchap-key", key1]) assert f"Adding host {hostnqn2} to {subsystem}: Successful" in caplog.text assert f"Host {hostnqn2} has a DH-HMAC-CHAP key but subsystem {subsystem} has no key, a unidirectional authentication will be used" in caplog.text caplog.clear() - cli(["--server-port", "5501", "host", "change_key", "--subsystem", subsystem, "--host-nqn", hostnqn1, "--dhchap-key", key2]) + cli(["host", "change_key", "--subsystem", subsystem, "--host-nqn", hostnqn1, "--dhchap-key", key2]) assert f"Changing key for host {hostnqn1} on subsystem {subsystem}: Successful" in caplog.text assert f"Host {hostnqn1} has a DH-HMAC-CHAP key but subsystem {subsystem} has no key, a unidirectional authentication will be used" in caplog.text time.sleep(15) @@ -97,7 +89,7 @@ def test_change_host_key(caplog, two_gateways): assert f"Received request to remove host {hostnqn1} access from {subsystem}" not in caplog.text assert f"Received request to add host {hostnqn1} to {subsystem}" not in caplog.text caplog.clear() - cli(["--server-port", "5501", "host", "change_key", "--subsystem", subsystem, "--host-nqn", hostnqn2, "--dhchap-key", key3]) + cli(["host", "change_key", "--subsystem", subsystem, "--host-nqn", hostnqn2, "--dhchap-key", key3]) time.sleep(15) assert f"Received request to change inband authentication key for host {hostnqn2} on subsystem {subsystem}, dhchap: {key3}, context: