DeepSea
DeepSea copied to clipboard
SUSE
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.