Closed jschmid1 closed 4 years ago
Honestly, I would stick with the original decorator from ceph-volume https://github.com/ceph/ceph/blob/master/src/ceph-volume/ceph_volume/decorators.py. For our use case with Salt, I can see removing the exit=True
logic, since sys.exit(1)
is not graceful in modules and runners.
With respect to the modes, I would consider those results. We originally started with human=True
and machine=False
in several examples, but using out=yaml
simplifies the interaction with modules and runners. This also gives control to the caller. We do have modules specifically for returning data unmodified in a Salt state, for example. Passing additional parameters to decorators to handle these cases complicates the decorator interface. Let this decorator handle exceptions and nothing more.
I think we still have three distinct outputs that can either be defined with out=<output_type>
or output_type=<bool>
Personally I don't mind either solution. While the former solution will reduce the number of kwargs to one, the latter is more explicite.
Honestly, I would stick with the original decorator from ceph-volume https://github.com/ceph/ceph/blob/master/src/ceph-volume/ceph_volume/decorators.py. For our use case with Salt, I can see removing the
exit=True
logic, sincesys.exit(1)
is not graceful in modules and runners.
While I agree that the logic for when to return what doesn't necessarily belong in the decorator, we can't just move it out (edit: easily) . If you have a better place to put it, feel free to suggest it.
This will not only handle the Exception case but also return the correct values based on the context (orchestrator, runner)
This allows us to have consistent behavior independent of whether we are calling a runner or a module underneath.
We have three modes:
human
(default)We're returning the aggregated results in a human readable form (success/failure)
orchestrator
We're returning a datastructure of our choice (current this returns a list of returnvalues, which is not terribly insightful). We can adapt this returnvalue anytime when we have real requirements from the orchestrator.
runner
That's for internal uses only. This will re-raise the Exception in form of a str when an isssue is encountered. If it succeeds it will currently just print an aggregated list of returnvalues.
click below to see all possible cases
click me
``` admin:/srv/salt/_modules/ext_lib/c-d # salt-run test.good keyring.mon on admin: success keyring.mon on mon3: success keyring.mon on mon1: success keyring.mon on mon2: success success admin:/srv/salt/_modules/ext_lib/c-d # salt-run test.good called_by_orch=True keyring.mon on admin: success keyring.mon on mon1: success keyring.mon on mon3: success keyring.mon on mon2: success - True - True admin:/srv/salt/_modules/ext_lib/c-d # salt-run test.bad keyring.mon_failure on mon2: failure No such file or directory: 'ceph-daemon' keyring.mon_failure on mon1: failure No such file or directory: 'ceph-daemon' keyring.mon_failure on mon3: failure No such file or directory: 'ceph-daemon' Caught an Exception while runningSigned-off-by: Joshua Schmid jschmid@suse.de