mgr/dashboard: introduce NvmeofCLICommand's success_message_template …#66879
mgr/dashboard: introduce NvmeofCLICommand's success_message_template …#66879
Conversation
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
| def escape_address_if_ipv6(addr: str) -> str: | ||
| ret_addr = addr | ||
| if ":" in addr and not addr.strip().startswith("["): | ||
| ret_addr = f"[{addr}]" | ||
| return ret_addr | ||
|
|
||
| def build_listener_del_success_message(args: Dict[str, Any], _) -> str: | ||
| traddr = args.get('traddr') | ||
| trsvcid = args.get('trsvcid') | ||
| subsystem = args.get('nqn') | ||
| host_name = args.get('host_name') | ||
|
|
||
| escaped_traddr = escape_address_if_ipv6(traddr) if traddr is not None else '' | ||
| host_msg = "for all hosts" if host_name == "*" else f"for host {host_name}" | ||
| return ( | ||
| f"Deleting listener {escaped_traddr}:{trsvcid} from {subsystem} " | ||
| f"{host_msg}: Successful" | ||
| ) | ||
|
|
||
| def build_ns_change_visibility_success_message(args: Dict[str, Any], _) -> str: | ||
| nsid = args.get('nsid') | ||
| subsystem = args.get('nqn') | ||
| auto_visible_val = args.get('auto_visible') | ||
|
|
||
| if isinstance(auto_visible_val, str): | ||
| auto_visible = auto_visible_val.lower() == "yes" | ||
| else: | ||
| auto_visible = bool(auto_visible_val) | ||
| vis_text = "\"visible to all hosts\"" if auto_visible else "\"visible to selected hosts\"" | ||
| return ( | ||
| f"Changing visibility of namespace {nsid} in {subsystem} " | ||
| f"to {vis_text}: Successful" | ||
| ) | ||
|
|
||
| def build_ns_set_auto_resize_success_message(args: Dict[str, Any], _) -> str: | ||
| nsid = args.get('nsid') | ||
| subsystem = args.get('nqn') | ||
| auto_resize_enabled = args.get('auto_resize_enabled') | ||
|
|
||
| auto_resize_text = 'auto resize namespace"' | ||
| if not auto_resize_enabled: | ||
| auto_resize_text = "do not " + auto_resize_text | ||
| auto_resize_text = '"' + auto_resize_text | ||
| return ( | ||
| f"Setting auto resize flag for namespace {nsid} in " | ||
| f"{subsystem} to {auto_resize_text}: Successful" | ||
| ) | ||
|
|
||
| def build_ns_set_rbd_trash_image_success_message(args: Dict[str, Any], _) -> str: | ||
| nsid = args.get('nsid') | ||
| subsystem = args.get('nqn') | ||
| rbd_trash_image_on_delete = args.get('rbd_trash_image_on_delete') | ||
|
|
||
| trash_image = str_to_bool(rbd_trash_image_on_delete) | ||
| trash_text = 'trash on namespace deletion"' | ||
| if not trash_image: | ||
| trash_text = "do not " + trash_text | ||
| trash_text = '"' + trash_text | ||
| return ( | ||
| f"Setting RBD trash image flag for namespace {nsid} in " | ||
| f"{subsystem} to {trash_text}: Successful" | ||
| ) | ||
|
|
||
| def build_host_add_success_message(args: Dict[str, Any], _) -> str: | ||
| subsystem = args.get('nqn') | ||
| host_nqn_list: List[str] = args.get('host_nqn') or [] | ||
|
|
||
| messages: List[str] = [] | ||
| for one_host_nqn in host_nqn_list: | ||
| if one_host_nqn == "*": | ||
| messages.append(f"Allowing open host access to {subsystem}: Successful") | ||
| else: | ||
| messages.append(f"Adding host {one_host_nqn} to {subsystem}: Successful") | ||
| return "\n".join(messages) | ||
|
|
||
| def build_host_del_success_message(args: Dict[str, Any], _) -> str: | ||
| subsystem = args.get('nqn') | ||
| host_nqn_list: List[str] = args.get('host_nqn') or [] | ||
|
|
||
| messages: List[str] = [] | ||
| for one_host_nqn in host_nqn_list: | ||
| if one_host_nqn == "*": | ||
| messages.append(f"Disabling open host access to {subsystem}: Successful") | ||
| else: | ||
| messages.append(f"Removing host {one_host_nqn} access from {subsystem}: Successful") | ||
| return "\n".join(messages) |
There was a problem hiding this comment.
these functions looks like it should exist in the services instead of the controller files.
There was a problem hiding this comment.
with that said, i am bit concerned about the maintainability of this one though. I mean the pattern where we introduce separate functions for generating messages for each of those functions...
| success_message_template: Optional[str] = None, | ||
| success_message_fn: Optional[Callable[[Dict[str, Any]], str]] = None |
There was a problem hiding this comment.
okay, I see you are using a template and a function here. Maybe we can utilize it and make it a bit more powerful and declarative so it is more simpler?
Instead of the function, can we have like a map that maps the values to the strings? like
success_message_template='Changing visibility of namespace {nsid} in {nqn} to "{auto_visible}": Successful',
msg_map={
'auto_visible': {
True: "visible to all hosts",
False: "visible to selected hosts"
}
}
so eventually what you are gonna need is to extend the NvmeofCLICommand to normalize and process this map to get the success message (probably a generic function of what you have above might suffice I think) and eventually gets format to the template you already defined?
There was a problem hiding this comment.
I think this will be feasible for all the 1-1 mapping but might be complicated for the host related one since it involves some extra processing there. Maybe you can check if it can work with a declarative template but if not, again a generic function should be nicer here which can be nicer here.
github-project-automation
bot
moved this from New
to Review in progress
in Ceph-Dashboard
Hezko
force-pushed
the
nvme-cli-responses
branch
from
44c009f to
956e9b4
Compare
Hezko
force-pushed
the
nvme-cli-responses
branch
13 times, most recently
from
1afe5b6 to
7dcc491
Compare
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
1 similar comment
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
Hezko
force-pushed
the
nvme-cli-responses
branch
from
7dcc491 to
2d8323a
Compare
Hezko
force-pushed
the
nvme-cli-responses
branch
8 times, most recently
from
ab85838 to
ae0e341
Compare
Hezko
force-pushed
the
nvme-cli-responses
branch
from
ae0e341 to
be97d00
Compare
nizamial09
left a comment
nizamial09
left a comment
There was a problem hiding this comment.
looks good, thanks for addressing my comments.
github-project-automation
bot
moved this from Review in progress
to Reviewer approved
in Ceph-Dashboard
Hezko
force-pushed
the
nvme-cli-responses
branch
7 times, most recently
from
579338f to
a966d47
Compare
…and success_message_map parameters to allow meaningful success messages Signed-off-by: Tomer Haskalovitch <tomer.haska@ibm.com>
Hezko
force-pushed
the
nvme-cli-responses
branch
from
a966d47 to
afcc1fc
Compare
github-project-automation
bot
moved this from Reviewer approved
to Done
in Ceph-Dashboard
added the infra code needed to support custom output messages from NVMeoF CLI operations. This aligned the new CLI with the more tailored outputs of the old CLI.
that means that instead of the output of "success" on the CLI operations, we will be able to output things like:
Adding namespace 17 to nqn.2016-06.io.spdk:cnode2.mygroup1: Successfulthis is done via two new parameters of
NvmeofCLICommanddecorator:I also edited all relevant CLI commands to use these new parameters, so you can see examples of usage in the PR itself.
some examples of the new outputs:
before the change all the above commands would result with the same output of "success", and in case of an error in this mechanism or invalid template we will fall back to this output
Signed-off-by: Tomer Haskalovitch tomer.haska@ibm.com
fixes: https://tracker.ceph.com/issues/62705
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an
xbetween the brackets:[x]. Spaces and capitalization matter when checking off items this way.Checklist
Show available Jenkins commands
jenkins test classic perfJenkins Job | Jenkins Job Definitionjenkins test crimson perfJenkins Job | Jenkins Job Definitionjenkins test signedJenkins Job | Jenkins Job Definitionjenkins test make checkJenkins Job | Jenkins Job Definitionjenkins test make check arm64Jenkins Job | Jenkins Job Definitionjenkins test submodulesJenkins Job | Jenkins Job Definitionjenkins test dashboardJenkins Job | Jenkins Job Definitionjenkins test dashboard cephadmJenkins Job | Jenkins Job Definitionjenkins test apiJenkins Job | Jenkins Job Definitionjenkins test docsReadTheDocs | Github Workflow Definitionjenkins test ceph-volume allJenkins Jobs | Jenkins Jobs Definitionjenkins test windowsJenkins Job | Jenkins Job Definitionjenkins test rook e2eJenkins Job | Jenkins Job DefinitionYou must only issue one Jenkins command per-comment. Jenkins does not understand
comments with more than one command.