Need to normalize salt-api responses

[mchugh19]

Salt-api is very useful, but also in need of response normalization. This issue is partially tracked through a few different issues, and it would be helpful to keep progress in a single place. See: #20301 #18510 jenkinsci/saltstack-plugin#18 jenkinsci/saltstack-plugin#36

Here are some short examples demonstrating the problems:

Most state responses include a result key which contains the appropriate success status. So far so good

[{
  "outputter": "highstate",
  "data": {"pws-3.dev.mpl.ru":   {
    "file_|-/some/file":     {
      "comment": "No directory to create /some/file in",
      "name": "/other/file",
      "start_time": "16:21:10.068787",
      "result": false,
      "duration": "0.947 ms",
      "__run_num__": 6,
      "changes": {}
    }
  }}
}]

Other times the API does not include the necessary info at all:
CLI

# salt 'web1' cmd.run 'false 1'
web1:
ERROR: Minions returned with non-zero exit code

API

{
    "return": [
        {
            "web1": ""
        }
    ]
}

This same issue occurs with the service execution module. Here is an example of a service failing to restart:

{
    "info": [
        {
            "Arguments": [
                "klsdklsd"
            ],
            "Function": "service.restart",
            "Minions": [
                "web1"
            ],
            "Result": {
                "web1": {
                    "return": false
                }
            },
            "StartTime": "2016, May 27 14:08:28.970138",
            "Target": "web1",
            "Target-type": "glob",
            "User": "jenkins",
            "jid": "20160527140828970138"
        }
    ],
    "return": [
        {
            "web1": false
        }
    ]
}

And some errors are in different structures

[{
  "outputter": "highstate",
  "data": {"minion-name": ["Rendering SLS 'base:hostname' failed: mapping values are not allowed here; line 40..."]}
}]

Or

[{"infra01": "ERROR executing 'pillar.get': The following keyword arguments are not valid: output=raw"}]

The problem isn't just errors either. Commands like the manage.present runner respond with successful, but again completely different output, making parsing difficult.

[["minion1","minion2","salt"]]

Overall, the responses generated by salt-api are somewhat all over the board making for constant edge cases increasing complexity.

[Ch3LL]

@mchugh19 i think this is a sound idea to keep track of these issues here instead for salt-api :)

ping @whiteinge just as a fyi

[whiteinge]

Definitely agree. This has been a long-standing annoyance. Well summarized.

The exit codes addition to Salt-core provide a good opportunity for us to surface this information in salt-api. Prior to that initiative the behavior is very CLI-centric, such as returning errors as strings. I an uncertain what the current state of support and documentation there is for these internal exit codes but as mentioned in one of the above-linked issues, it will be important to collaborate with the core team to get these into Salt's Python API and then salt-api.

This issue should be considered blocked by #18510.

• Step 1 is to finish and document the changes in salt exit codes #18510.

• Step 2 is to modify Salt's Python API to output the extra information.

  This will need to be done carefully to preserve backward-compatibility. There is no top-level namespace we can add to in the existing output. E.g., LocalClient.cmd_sync() returns the execution module output verbatim.

  Perhaps a kwarg to the various *Client.cmd* methods to opt-in to outputting a different return structure. E.g., LocalClient.cmd_sync(include_full=True) could output {"return": <verbatim module return>, "exit_codes": <new data>}.

  Perhaps changing the entire return structure is too much for a simple kwarg and we need a new client type such as local_sync_full. E.g., LocalClient.cmd_sync_full().

  Suggestions welcome.

• Step 3 is to surface that extra data from Salt's Python API to salt-api.

  This is easily done in a backward-compat way by adding a new "exit_codes": [...] key to the top-level structure, adjacent to "return" so that consumers can correlate exit codes with returns. This will retain backward compatibility with the existing return structure since it is additive.

  Depending on how Step 2 is accomplished we might also want to add a new client type and a new convenience endpoint.

[danlsgiga]

+1

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[mchugh19]

Calm it down stalebot, this is still a major issue and not resolved.

[stale]

Thank you for updating this issue. It is no longer marked as stale.

[whiteinge]

The three checkboxes I mentioned above around exposing exit codes have been addressed (see #41356 for more info). That won't help the structural issues around Local/Runner/WheelClient differences or differences across all the modules in common formats or how errors are raised, however.