Allow http.query to report 'changes' instead of failure

[mchugh19]

Ran into an issue where this seems like a reasonable fix, but this seems worthy of greater discussion.

We have a case where we want to take conditionally perform an action such as create a hadoop cluster. If it does not already exist, this operation runs just fine. If it does already exist, the create call fails. We therefore need to run a query to determine if the cluster already exists, and then only run the create job if it does not. The service functions with a rest interface so both the check and create calls are done over http. There seem to be a few different ways of handling this case:

1. The only existing way to setup this behavior with salt seems to be with an http.query which checks if the cluster exists. If it does not, we run the create cluster http.query with onfail. This works and allows us to run the state in both cases where the cluster does not yet exist and where it already does. This is problematic however, in that it expects a state to fail as part of the normal operation. Furthermore, this state failure causes testkitchen failures and breaks CI testing.

2. Another possibility could be to ensure that curl is installed, and use an unless option on the create_cluster operation, but this seems like a hack and requires more overhead to ensure that proxies, authentication, and the curl package are all available and configured properly.

3. It might also make sense to extend the existing http.query function to support a condition, say a version of unless that does not just run an external command, but instead allows you to use the same headers and auth as the current http.query call. This seems a bit more complex than implementing option 4 and it is unclear that this would be better.

4. Our suggested fix is to create a http.query_changes function which duplicates http.query, but instead of returning failure when a match is not found, it instead reports changes. This is technically inaccurate as the query itself did not necessarily change anything, but this is needed for a subsequent state to use the onchanges attribute to then effect the necessary update. This has a downside as reporting 2 changes on highstate runs which only had a single change, but this is likely unavoidable unless option 3 is implemented.

Previous Behavior

check_if_cluster_exists:
  http.query:
    - url: http://localhost/api/clustername
    - status: 200

create_cluster:
  http.query:
    - url: http://localhost/api
    - post: '{"some": "json"}'
    - onfail:
      - check_if_cluster_exists

In this case, if the cluster does not exist, the first check reports a failure, which causes the create_cluster http call to run. While this meets the end goal of creating a cluster, it also relies on failures for normal operations and invalidates unit testing.

New Behavior

check_if_cluster_exists:
  http.query_changes:
    - url: http://localhost/api/clustername
    - status: 200

create_cluster:
  http.query:
    - url: http://localhost/api
    - post: '{"some": "json"}'
    - onchanges:
      - check_if_cluster_exists

In this case, if the cluster does not exist, the first check reports a change, which causes the create_cluster http call to run. When dealing with rest calls, this functionality seems necessary.

[mchugh19]

As part of the discussion, the proposed solution is a bit too simplistic. In the case of the suggested query_changes, the http calls can still fail and should still be marked as a failure. So we would likely need to work up something to support version 3 as defined above, or add in status_change and match_change to raise a change from the existing http.query.

[mchugh19]

Are there any thoughts about the proper way to handle these cases? Is raising a change the proper solution? Should http.query be extended to have a unless which can run another http query?

[Ch3LL]

@mchugh19 just to clarify are you only trying to create a hadoop cluster? I'm asking if you have to absolutely have to use the http module? Wouldn't a better approach be to create a hadoop state module within salt that does all this checking behavior for you. If the cluster is created mark as passing and no changes and if it does not exist create the cluster.

Either way I do not want to completely bypass your initial thought process here as I am sure there are others who are using this http.query module to do something similar to your use case. I like the idea of approach this with similar to unless or onlyif. but I think getting some additional opinions in here would be beneficial. ping @terminalmage @rallytime or @cachedout do you guys have an ideas as to the best way to approach this problem?

[mchugh19]

This specific example is a hadoop cluster, but the question is more around the generic, what is the best way to handle conditionally running an http request. We could create a module to handle this specific request, but the function trying to be performed is pretty basic, and so it seems like using the http.query with a proper conditional would get the most widespread use.

Does adding a status_change and match_change to http.query in order to combine with the onchanges function make the most sense? Or is there a better, saltier, way of handling these scenarios?

[mchugh19]

I suppose an even more generic solution could be to extend unless to support running a salt module instead of just a command. That way the http.query could be wrapped by an unless other http.query, or performing a mysql.query could be wrapped with an unless, other mysql.query.

Does adding a new unless_module or extending the current unless make sense?

[stale[bot]]

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]

Still an open question. Happy to submit a PR if a direction is chosen

[stale[bot]]

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

[Ch3LL]

ping @saltstack/team-core any thoughts here?

[gtmanfred]

There was talk of making it so that onfail: would unmark the failed state as result=False, imho, i think that would be a better solution.

[Poil]

Hi,

Any update on this issue ? I also prefer to unmark the failed state, easier and more readable

Best regards,

[Ch3LL]

I'll go ahead and mark this as a feature request but no one is currently working on this due to other higher priority issues, so anyone else here is welcome to take a stab at it.

[stale[bot]]

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]

Still an open question. Is there a salt recommendation on how to proceed?

[stale[bot]]

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

[ari]

It would be incredibly useful if all states had options like:

return:
  fail:
     result: ignore | error | warning
     event: { 'returner' : 'my data' }
  success: ...
  changes: ...

We'd have control over sending data to monitoring systems, returners, and also changing the behaviour of requisites.

[johncollaros]

Was any progress made on this, or even a different direction taken?

[dmurphy18]

@johncollaros Given the age of this and the current reduction in the size of the Salt Core Team, I would say no movement on this.
Also given all the former Salt Team members that replied to this issue are no longer with the Salt Team, I doubt this will be implemented by the Salt Team anytime soon, just too reduced a team after the Broadcom buyout.
However, if you want to submit a PR for this feature, with tests written in pytest, would be more than happy to review it. It would need to be made against the master branch (since feature request), but I would first check if the http handling is going into a salt extension and make the changes there. See https://github.com/saltstack/great-module-migration/tree/main