Example code for return structures of modules

Description of Issue/Question

I created https://github.com/SUSE/DeepSea/compare/next...wip-returns-and-logging for discussion. I have not run this code through pylint nor used a formatter. I have not added help functions. The goal is to show what behavior I would like to see.

I included both a runner and a module. Since we already have a module named keyring, I opted for keyring2 for the moment to not cause confusion with the original module. I imitated Salt's behavior for state modules, but did add the return code since it's more useful than just returning that something failed.

Calling the module directly gives the following behavior for the admin keyring

# salt-call keyring2.admin
local:
    creating /srv/salt/ceph/bootstrap/ceph.client.admin.keyring

# salt-call keyring2.adminrc
local:
    ----------
    changes:
        ----------
        out:
            creating /srv/salt/ceph/bootstrap/ceph.client.admin.keyring
    comment:
    name:
        keyring2.adminrc
    rc:
        0
    result:
        True

Another for the bootstrap keyring

# salt-call keyring2.bootstrap
local:
    creating /srv/salt/ceph/bootstrap/keyring

# salt-call keyring2.bootstraprc
local:
    ----------
    changes:
        ----------
        out:
            creating /srv/salt/ceph/bootstrap/keyring
    comment:
    name:
        keyring2.bootstraprc
    rc:
        0
    result:
        True

And an example of a failed command

# salt-call keyring2.bad
[ERROR   ] rc: 125 -- Command "xyz" not found.
 See `podman --help`.

local:
    podman exit code: 125
    Command "xyz" not found.
     See `podman --help`.

# salt-call keyring2.badrc
[ERROR   ] rc: 125 -- Command "xyz" not found.
 See `podman --help`.

local:
    ----------
    changes:
        ----------
    comment:
        Command "xyz" not found.
         See `podman --help`.
    name:
        keyring2.badrc
    rc:
        125
    result:
        False

I am debating about the log.error. Considering commenting it out until the python3-systemd is available. The systemd logging is commented out for the moment, but does work on systems with the python module.

The runner serves as the higher level wrapper and at least, gives the appearance of subcommands.

# salt-run keyring.create admin
creating /srv/salt/ceph/bootstrap/ceph.client.admin.keyring

# salt-run keyring.create bootstrap
creating /srv/salt/ceph/bootstrap/keyring

# salt-run keyring.create bad
Failed: Command "xyz" not found.
 See `podman --help`.

Without a subcommand, both the admin and bootstrap steps are run

# salt-run keyring.create
creating /srv/salt/ceph/bootstrap/ceph.client.admin.keyring

creating /srv/salt/ceph/bootstrap/keyring

And if another runner wishes to call this runner then the functions create_adminrc, create_bootstraprc and create_badrc are available.

My main concern is making sure that we have access to the various levels directly and this allows the admin to investigate without too much handholding if we stay consistent.

I'm not too concerned about specific verbs, but I went with create over deploy in this case. Deploy tends to imply distribution which is not what is happening with this example.

I found that I did not submit my final keyring2.py. (It was still in my VM.) It's there now with a force push.

To experiment with yapf3, I created two additional commits and left them separate for others to see and comment as well. The difference between the two is the number of columns.

https://github.com/SUSE/DeepSea/commit/0e4e06776ddceada6ab450705f8c2590536bbbda https://github.com/SUSE/DeepSea/commit/6b45ce1f87283856c764320089ce21cb6f5ebab9

The one thing I would like to note is that it's possible to disable formatting for some sections. In the particular examples that I did here, I think this is the right tactic when displaying podman command lines. Compressing all the options and arguments into as few lines as possible does not look good.

All that said, yapf3 fixed my indenting mistake and recreated some structures which is something I would have preferred. I think I would be more in favor of adopting yapf3 and dropping pylint. If yapf3 and pylint ever disagree, then it's just more work for the submitter to appease a tool.

Here you can find my approach to the problem: https://github.com/SUSE/DeepSea/commit/aec0ce00a6c2e16bd3a470c8b69c0bce3a5f5f91

taken from branch https://github.com/SUSE/DeepSea/compare/next...return_struct_next?expand=1

I went with the existing podman modules and extended it with a class that acts as a DataContainer typ thing. See here:

https://github.com/SUSE/DeepSea/commit/aec0ce00a6c2e16bd3a470c8b69c0bce3a5f5f91#diff-12fe82452ced2beebb7266e548e7ec19R23-R100

That basically implements a glorified return structure that also allows dynamic assignment of certain fields based on the return we get. For shell invocation I decided to go with subprocess.run (the why is explained in the comments of the code)

This allows us to set timeouts, retrieve the returncode, stdout, stderr and raise Exceptions based on the behavior. See here https://github.com/SUSE/DeepSea/commit/aec0ce00a6c2e16bd3a470c8b69c0bce3a5f5f91#diff-12fe82452ced2beebb7266e548e7ec19R172-R192

A module return output looks like this for me now:


local:
    ----------
    command:
        /usr/bin/podman run --rm --net=host -e CONTAINER_IMAGE=registry.suse.de/devel/storage/6.0/images/ses/6/ceph/ceph -e NODE_NAME=admin -v /srv/salt/ceph/bootstrap:/srv/salt/ceph/bootstrap --entrypoint /usr/bin/ceph-authtool registry.suse.de/devel/storage/6.0/images/ses/6/ceph/ceph --create-keyring /srv/salt/ceph/bootstrap/keyring --gen-key -n mon. --cap mon allow *
    comment:
        The function create_initial_keyring of module salt.loaded.ext.module.podman returned with code 0
    err:
    func_name:
        create_initial_keyring
    human_result:
        success
    module_name:
        salt.loaded.ext.module.podman
    out:
        creating /srv/salt/ceph/bootstrap/keyring
    rc:
        0
    result:
        True

One layer up the stack we have do_x (https://github.com/SUSE/DeepSea/commit/aec0ce00a6c2e16bd3a470c8b69c0bce3a5f5f91#diff-83b09f45ee8aa92de255d294c79a56ebR523)

That calls the LocalClient with a module.function.

The return is check with a newly implemented evaluate_module_return function https://github.com/SUSE/DeepSea/commit/aec0ce00a6c2e16bd3a470c8b69c0bce3a5f5f91#diff-83b09f45ee8aa92de255d294c79a56ebR495

( This is necessary as the return from LocalClient contains a nested datastructure, basically the return from multiple minions)

do_x returns a tuple which represents the overall/summarized return of all the affected minions and the new returnstructure.

Ultimately the do_x function is called from a runner. https://github.com/SUSE/DeepSea/commit/aec0ce00a6c2e16bd3a470c8b69c0bce3a5f5f91#diff-8fd6811c61959f53acd72040a1674dd0R6

The runner inteprets the return based on it's context, which I haven't found an elegant solution so far.

Looking forward to comments/suggestions/improvements!

I extended the return structure a bit and added the bit for CalledProcessError:

~The new commit can be found here https://github.com/SUSE/DeepSea/commit/c5eb793e794ec06286b2b1586d0bb6e006d5c36e~

updated: https://github.com/SUSE/DeepSea/commit/1f4e1c0ab4b86851b4b0b40e724c975fa157d335

local:
    ----------
    command:
        /usr/bin/podman run --rm --net=host -e CONTAINER_IMAGE=registry.suse.de/devel/storage/6.0/images/ses/6/ceph/ceph -e NODE_NAME=admin -v /srv/salt/ceph/bootstrap:/srv/salt/ceph/bootstrap --entrypoint /usr/bin/ceph-authtool registry.s
use.de/devel/storage/6.0/images/ses/6/ceph/ceph --doesnt exist
    comment:
        The function create_initial_keyring_failure of module podman returned with code 1
    err:
        /usr/bin/ceph-authtool: unexpected 'exist'
    func_name:
        create_initial_keyring_failure
    guide:
        Try running: salt '<target_minion>' podman.create_initial_keyring_failure
    human_result:
        failure
    module_name:
        podman                                                                                                                                                                                                                                
    out:                                                                                                                                                                                                                                      
        No stdout captured
    output:
        usage: ceph-authtool keyringfile [OPTIONS]...
        where the options are:
          -l, --list                    will list all keys and capabilities present in
                                        the keyring
          -p, --print-key               will print an encoded key for the specified
                                        entityname. This is suitable for the
                                        'mount -o secret=..' argument
          -C, --create-keyring          will create a new keyring, overwriting any
                                        existing keyringfile
          -g, --gen-key                 will generate a new secret key for the
                                        specified entityname
          --gen-print-key               will generate a new secret key without set it
                                        to the keyringfile, prints the secret to stdout
          --import-keyring FILE         will import the content of a given keyring
                                        into the keyringfile
          -n NAME, --name NAME          specify entityname to operate on
          -a BASE64, --add-key BASE64   will add an encoded key to the keyring
          --cap SUBSYSTEM CAPABILITY    will set the capability for given subsystem
          --caps CAPSFILE               will set all of capabilities associated with a
                                        given key, for all subsystems
          --mode MODE                   will set the desired file mode to the keyring
                                        e.g: '0644', defaults to '0600'
    rc:
        1
    result:
        False
    timout:
        0

for the positive output it looks like this:

local:   
    ----------
    command:
        /usr/bin/podman run --rm --net=host -e CONTAINER_IMAGE=registry.suse.de/devel/storage/6.0/images/ses/6/ceph/ceph -e NODE_NAME=admin -v /srv/salt/ceph/bootstrap:/srv/salt/ceph/bootstrap --entrypoint /usr/bin/ceph-authtool registry.s
use.de/devel/storage/6.0/images/ses/6/ceph/ceph --create-keyring /srv/salt/ceph/bootstrap/keyring --gen-key -n mon. --cap mon allow *
    comment:
        The function create_initial_keyring of module podman returned with code 0
    err:
        No stderr captured
    func_name:
        create_initial_keyring
    guide:
        No guidance needed
    human_result:
        success
    module_name:
        podman
    out:
        creating /srv/salt/ceph/bootstrap/keyring
    output:
        No output captured
    rc:
        0
    result:
        True
    timout:
        0

The func_name looks good. I am wondering if module_name and func_name should just be one key. So, the value would be podman.create_initial_keyring. (At some point, I would like to revisit the names themselves.)

With respect to CalledProcessError changing the capturing of output depending on whether a command succeeded or failed, I'm not a fan. I also believe we can do without the canned responses. The exception handling isn't buying anything that we don't already have. I would be in favor of dropping the guide, human_result and output. (When I first saw output with out and err, I was confused about what would show up where.) Any guide content can be in comment; otherwise, the comment doesn't really have too much use.

I think the command, out, err, rc, some form of module/func and result are good. I'm uncertain about the timeout.

update the commit with some more improvements: https://github.com/SUSE/DeepSea/commit/1f4e1c0ab4b86851b4b0b40e724c975fa157d335

I also factored multiple functions into non-salt managed files for re-usability.

Another implementation using Python imports within Salt https://github.com/SUSE/DeepSea/commit/f26dff8344c4c52b1d00208e914390625932494f. The Podman class in this example only builds the command. The cmd.run_all remains in the module.

SUSE / DeepSea

Example code for return structures of modules #1745

Description of Issue/Question