[FEATURE REQUEST] Define state file pre-requisites in metadata (instead of code)

[dkfsalt]

Is your feature request related to a problem? Please describe.
The moment you start working in a heterogenous platform environment, you have to start writing states that support different operating systems. If you work in YAML/JINJA2, you end up adding a bunch of really ugly code to check do your pre-requisites, and then a few more if-statements and even some branching to make it even more unreadable.

Describe the solution you'd like
Add reserved words in the YAML doc string that allow users to define pre-requisites for the state

Describe alternatives you've considered
onlyif and unless both support modules (since 3000)

Additional context
Consider these two state files...

---
version: 1.0.0
salt_prerequisites:
  - grains.get:
      os_family: 
        - Debian
        - RedHat
      @fail: clamav not installable on this platform  <-- returns "fail no changes"
.. or...
      @succeed: clamav not installable on this platform  <-- returns "succeed no changes"
---
{% import_yaml "/clamav/conf.map"as conf %}
{% set pkgs = salt['grains.filter_by'](conf) %}
{% if pkgs is defined and pkgs is not none %}
install clamav:
  pkg.installed:
    - pkgs:
    {% for pkg in pkgs %}
        - {{pkg}}
    {% endfor %}
{% endif %}

--- 
version: 1.0.0
---
{% if grains['kernel'] != 'Linux' %}
not supported:
  test.show_notification:
    - text: clamav cannot be installed on this os

{% else %}
  {% import_yaml "/clamav/conf.map"as conf %}
  {% set pkgs = salt['grains.filter_by'](conf) %}
  {% if pkgs is defined and pkgs is not none %}
install clamav:
  pkg.installed:
    - pkgs:
    {% for pkg in pkgs %}
        - {{pkg}}
    {% endfor %}
  {% endif %}
{% endif %}

The first one is infinitely more readable (well, in so far as anything with Jinja2 in it is readable).

It is also safe to say that almost no state file will operate on all platforms. So right now...

• you either have to accept that a state file will execute on an invalid platform and possibly make dangerous changes,
• you will get a stack trace back (not friendly to users in Enterprises where there are L1/L2 operational people running our states)

Note: I haven't quite figured out what the right grammar should be for this but I think the fundamental requirements should be:

1. Platform support should be defined in the state's metdata, not in the code - this makes states more maintainable
2. We should be able to define whether a failure of any pre-req is "failure" or "success", with a friendly message to return back to the user.
3. The pre-req check should allow us to execute modules as checks

[whytewolf]

This could be an extension of #52776 which was a thought about using yaml multi-document support as a metadata separator. in fact, the pass/fail information from this could drive sls_doc info from that feature request allowing Clean separation of logic in pass/fail conditions.

[dkfsalt]

Agreed. I guess that the fundamental goal behind both of these is to make state files more human readable because, right now, if you want to do anything more than just the most basic state file, you end up with a perl-like mess of symbols.

If we can identify some common use cases that the state system can manage for users whilst making states more readable, I see that as a win.

I'm trying, with my first state, to be as flexible as possible by defining functions, but perhaps that's too complex. Perhaps we can just use a target string?

---
salt_prerequisites:
    G@os_family:  Debian, RedHat
    G@os: 
      - CentOS 7
      - CentOS 8
      - etc
    P@customer: stuff
    G@role: webserver, oracle

Something like this doesn't have the flexibility of using salt modules for pre-testing, but maybe that kind of thing should be left to the code block.

One thing that I also struggle with is that there is no "return/break" function for a state file....

States are effectively batch scripts with a bunch of {% if then else %} statement when you have logical branches.

If we treated each state file as a function and had a "return None" kind of thing; ie

{% return if condition %}   <-- or whatever kind of syntax we'd like

That would also greatly simplify state files because you don't have to handle the if...then...else blocks for branching.

[dfidler]

Something that that I've not specifically called out in the above is that the behavior that I'm proposing is to use the logical "AND" operation for the rules.

But looking at this now, with fresh eyes, I wonder, will there ever be a situation where we'd want to use other expression operators (like parentheses for grouping, or logical AND/OR/NOT).

Compound matchers already include logical semantics but they aren't very readable and my initial goal was to make states more readable and I fear that compound matchers don't achieve that.

Consider the following:

YAML-like syntax, AND-only semantics:

---
@prereq:
    G@os_family:  Debian, RedHat  <-- converts to list
    G@osfinger:   <-- is a list
      - CentOS 7
      - CentOS 8
      - etc
    G@customer: squirrel
    G@role: webserver, oracle <-- converts to list

... or ...
@prereq:
    G@os_family:  Debian, RedHat  <-- converts to list 
    P@osfinger:  CentOS [7|8]
    G@customer: squirrel
    G@role: webserver, oracle <-- converts to list

In the above, I'm assuming that each expression is "AND'd" and each list is "OR'd"

So the equivalent Compound Matcher would be

---
@prereq: P@os_famlily:(Debian|RedHat) and P@osfinger:CentOS [7|8] and G@customer:squirrel and P@role:(webserver|oracle)

I think it's safe to say that the whitespace in the first two is far preferable to the Compound Matcher. I recognize that creating a parser that converts these prereq rules into a compound matcher is probably the easiest solution to this and that creates code complexity (though I will always choose to implement code complexity over UX complexity).

I'm wracking my brain for an example where we might want to use logical OR, or NOT for something like this and I can honestly say that I've never had a situation like that. Even the example in the compound matchers page is contrived (why would you ever write a state file that only applies to a single device - simply use jinja variable replacement for that if that's the case)

And if you do have a situation where you required an "or" in the repreqs, would it be possible to work around this limitation by setting some grains on the systems through highstate? Maybe that would be too clunky.

@whytewolf - what do you think?

[dfidler]

Hm... there are some other considerations here; this message is about

Consider the following state files:

# /srv/salt/centos78.sls
---
@prereq:  P@osfinger: CentOS [7|8]
---
centos78:
  test.show_notification:
    - text:  I work on CentOS 7 and 8


# /srv/salt/centos8only.sls
---
@prereq:  G@osfinger: CentOS 8
---
centos8only:
  test.show_notification:
    - text:  I work on CentOS 8 only

How do you handle this with highstate?

On a CentOS 7 server, it would look like this:

local:
----------
          ID: centos78
    Function: test.show_notification
      Result: True
     Comment: I work on CentOS 7 and 8
     ...
     Changes:
----------
          ID: centos8only
    Function: test.show_notification
      Result: True
     Comment: I work on CentOS 8 only
     ...
     Changes:

Summary for local
------------
Succeeded: 2
Failed:    0
------------
Total states run:     2
Total run time:   2.875 ms

But what should be the behavior on a CentOS 8 server? In the first message I offer a "fail" and "succeed" keyword, the idea was to control the value of "Result" (because if you're just doing platform checking, if it's an invalid platform, it's not necessarily a failure - you might want to report on that as being successful.

local:
----------
          ID: centos78
    Function: test.show_notification
      Result: False
     Comment: Not Applicable:  <<succeed/failure comment>>
     Started: 10:50:45.496284
    Duration: 1.467 ms
     Changes:
----------
          ID: centos8only
    Function: test.show_notification
      Result: True
     Comment: I work on CentOS 8 only
     Started: 10:50:45.497905
    Duration: 1.408 ms
     Changes:

Summary for local
------------
Succeeded: 1
Failed:    1
------------
Total states run:     2
Total run time:   2.875 ms

Actually, what I would love to see is this:

Summary for local
------------
Succeeded: 1
Failed:    0
Not Applicable: 1
------------
Total states run:     2
Total run time:   2.875 ms

"Result" is a boolean value and changing that isn't going to work. But is there a way to report "Not Applicable" for state files that doesn't involve upending the whole state result system?

@whytewolf - you see where I'm going with this, don't you? :)

[dfidler]

And last one (well, for now)... what about state files that "include:" others (like formulas)? And what if there are multiple low states in a single state file?

I think that each low state would need to inherit the list of prereq rules from the state file that they are directly contained within.

Consider the following state files:

/srv/salt/nginx:
  init.sls             <-- pkg.installed & includes .running with a watch_in requisite G@os:CentOS
  running.sls      <-- G@os:CentOS
  uninstall.sls     <-- removes package and configs G@os:CentOS

What happens if I write the following:

# @prereq:  G@os:RedHat
include: 
  - .nginx

ensure nginx configured:
  file.managed:
    - name: /etc/nginx/nginx.conf
    - source: salt://nginx/files/conf.jinja
    - template: jinja
    - require:
      -   ensure nginx installed
    - require_in:
      -  ensure nginx running
    - watch_in:
       - ensure nginx running

If we run this on a RedHat server, what would the results be? My guess is something like this:

local:
----------
          ID: ensure nginx installed
    Function: pkg.installed
      Result: True/False?
     Comment: Not Applicable
     ...
     Changes:
----------
          ID: ensure nginx configured
    Function: file.managed
      Result: False
     Comment: Some requisites were Not Applicable
     ...
     Changes:
----------
          ID: ensure nginx running
    Function: service.running
      Result: False
     Comment: Not Applicable  <-- or would this say "not run because a prerequisite failed"?
     ...
     Changes:

Summary for local
------------
Succeeded: 0
Failed:    1
Not Applicable: 2
------------
Total states run:     2
Total run time:   2.875 ms

Do you think the above would be easy enough for state developers to debug?