New NVMeOf CLI

[gbregman]

Fixes #151

Originally this was PR #349

[epuertat]

Given the amount of (breaking) changes this PR brought (CLI and server API), I'd have increased the major version number (1.0.0).

[epuertat]

jq is a great tool to parse JSON data. Something like this:

Suggested change

	ha_status=$(cephnvmf --output stdio --format json subsystem list | grep "enable_ha" | grep "true" | tr -d '\n\r')
	ha_status=$(cephnvmf --output stdio --format json subsystem list | jq '.enable_ha'

[epuertat]

pkg_resources.parse_version would do the trick but Python 3.12 deprecated pkg_resouces, so we could use packaging.version.parse instead.

[epuertat]

IMHO it should be the server who states whether the client version is compatible with it, rather than the other way around.

That said, gw_ver < base_ver looks like a very gentle version support. I'd limit this to a minor version comparison, while different major versions is not supported (this PR is a very good example of a situation where a newer client cannot communicate with an older GW server.

[gbregman]

@epuertat there is also some code in grpc.py to return an error in case the CLI version is older than the gateway's one. But, we needed a way to give an error in case we use a gateway version older than 0.0.7 as the GRPC protocol is not compatible. As in such a case we can't rely on the server side, being older we need some code in the CLI side. I made sure that the "gw info" request remains compatible with the old versions so we could have that.

[epuertat]

Ok! 👍🏼

[epuertat]

Dead code? Aren't we running a linter or something?

[gbregman]

@epuertat this is used in the test_cli.py code. I needed a way to just get the python object into a python script without getting a json string and parsing it.

[epuertat]

In general we shouldn't put test code in production artifacts. Python, unlike other languages, is flexible enough not to need test code/branching inside production code.

In any case, it looks like it has to do with the output format and that "hack" of --format=python to allow by-passing Python objects for testing purposes (is there any other reason?).

I checked and the args.format == expression appears 169 times in the cli.py file:

        if args.format == "text" or args.format == "plain":
            if ret.status == 0:
                if args.nsid:
                    ns_id_str = f"{args.nsid}"
                elif args.uuid:
                    ns_id_str = f"with UUID {args.uuid}"
                else:
                    assert False
                out_func(f"Changing load balancing group of namespace {ns_id_str} in {args.subsystem} to {args.load_balancing_group}: 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

First of all, I'd follow as much as possible the Unix approach: whenever a command worked as expected, no output is shown.

That aside, rather than doing this checking inside every command, you could use a declarative approach and a Formatter-like (singleton) class/decorator, where you just return the object, and describe how you wanted it to be formatted:

  @cli.formatter(
    text=lambda x: f"Version is {x}")
  )
  def version(self, args):
    ...
    return ret

The formatter class would:

• read the --format arg directly from sys.argv or explicitly at instantiation.
• format the output by running the input callables text, json, yaml, ...
• if not provided, inspect the passed object for magic methods like __str__, __json__, __yaml__, and invoke them,
• otherwise, would implement default methods for serializing to text, json, yaml, etc.

Something like that would reduce the code here and might create a more consistent output format. For the format=python case, just mocking the Formatter in the test files would be enough.

What do you think, @gbregman ?

[gbregman]

@epuertat I followed the guidelines from the design documents. I'm sure that my Python code could be improved as I'm not that fluent in Python.

[epuertat]

First of all, I don't like too much the way the @cli.cmd decorator works, as it obfuscates the actual parameters accepted by a method, and complicates debugging.

I've checked the most popular Python cli parsing tools, and looks like the closest to this would be click:

@click.group
def ns():
  pass

@ns.command('add')
@click.option('--rbd-pool', default="rbd", help="RBD pool name")
@click.argument('subsystem', help="Subsystem NQN")
def ns_add(rbd_pool, subsystem):
  ...

@ns.command('list')
@click.option('--rbd-pool', default="rbd", help="RBD pool name")
def ns_list(rbd_pool)
  ...

[epuertat]

I'm a bit concerned by the fact that the cli file has quadrupled its size... I understand that there's a lot of code to unwrap and format the data for human-readable output, but it results in a tightly coupling with the server, and that always complicates cross version support.

As I suggested before, I'd try to reduce this file size by using some other CLI parsing tool, like click (I'm almost sure that alone might halve the length of this file) and maybe using some templating engine for textual outputs (jinja2, humanize for the unit conversion, or tabulate as already used here).