Allow watching states to opt-in to mod_watch when normal run had changes

[mitar]

What does this PR do?

Adds an opt-in mechanism so a watching state whose normal run produced changes can still have its mod_watch invoked, when those changes do not subsume the work that mod_watch would do. The normal run's return may set force_mod_watch: True to request this. The changes and comment from mod_watch are then merged into the normal run's result so neither set is lost, and the combined result is the AND of the two.

The default behavior is unchanged: if the normal run reports changes and does not opt in, mod_watch is still skipped, so non-idempotent watchers (such as cmd.run, whose mod_watch simply re-invokes the main action) are not executed twice per state apply.

The status == "change" branch in State.call_chunk (salt/state.py)
now reads:

ret = self.call(low, chunks, running)
force_mod_watch = ret.pop("force_mod_watch", False)
if not ret.get("skip_watch", False) and (
    not ret["changes"] or force_mod_watch
):
    # ... call mod_watch; replace the result if the normal run had no
    # changes, otherwise merge mod_watch's changes/comment and AND the
    # result.

The force_mod_watch key is popped from the return so it does not leak into the final serialized state result.

What issues does this PR fix or reference?

Fixes #68958.

Previous Behavior

If the watching state's normal run produced any changes, Salt skipped calling mod_watch, even when a watched state reported changes. The watched-state trigger was silently dropped. A common way this surfaces is with docker_container.running: when a docker_network.present it depends on gets recreated in the same highstate, the normal run of docker_container.running performs a network reconnect (reporting changes), which then prevented mod_watch from firing to force-replace the container for a watched config-file change. The state still appeared as Changed in the output, so the missing restart was invisible.

There is not always a workaround like splitting service.enabled and service.running into separate states, as reported in #68958.

New Behavior

A watching state may return force_mod_watch: True from its normal run to signal that its changes do not subsume what mod_watch would do. When that flag is set (and skip_watch is not set), mod_watch is invoked in
addition to the normal run, and the changes and comment from mod_watch are merged into the normal run's result. The combined result is True only if both succeeded.

The previous default is preserved: when the normal run had changes and does not opt in, mod_watch is skipped. When the normal run had no changes and skip_watch is not set, mod_watch is called and its return replaces the normal run's, exactly as before.

Setting force_mod_watch is only meaningful for state modules whose mod_watch performs work orthogonal to the normal run. For example, docker_container.running's mod_watch forces a container rebuild that is not performed by a network-only reconcile. For non-idempotent watchers like cmd.run, the module should not opt in, and the default skip behavior continues to protect against double execution.

Merge requirements satisfied?

• Docs
• Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
• Tests written/updated (one test for the default skip behavior, one for the force_mod_watch opt-in)

Commits signed with GPG?

No

[bdrx312]

This change breaks backwards compatibility so should probably have some kind of option to trigger the behavior and not be enabled by default until a future release. I think this falls in line similar to a deprecation https://docs.saltproject.io/en/latest/topics/development/deprecations.html

[mitar]

Do you have some suggestion what and where would be this flag? A top-level config flag?

[whytewolf]

oh hell no. This is NOT the change that should happen.

the whole idea of mod_watch is that it runs the changes of a module once. this will give a way that it can do it twice. this is break a whole lot of things through out all of salt.

this kind of change has extreme changes throughout ALL of salt. you don't wag the dog to wag the tail.

[mitar]

This tries to fix the issue that just because some unrelated and uncontrollable change happens, watch does not trigger. It is not that salt states are badly defined.

Documentation clearly states:

If the watching state changes key contains values, then mod_watch will not be called. If you're using watch or watch_in then it's a good idea to have a state that only enforces one attribute - such as splitting out service.running into its own state and have service.enabled in another.

But what if "this good idea" is not possible? What is then the solution? The issue is that any more complex state (like container_running) can have some hidden changes you cannot really split.

BTW, this change does not break any tests really. So I am not sure about "this is break a whole lot of things through out all of salt".

[whytewolf]

BTW, this change does not break any tests really. So I am not sure about "this is break a whole lot of things through out all of salt".

This is the problem with the youth today. just because tests do not break does not mean it is the correct answer. changing the behavior of something that is used by the WAY more then your one use case. in a fundamental way.

your own test proves why you shouldn't always trust that not breaking tests is the correct answer. you test one very limited way it is working. however i can do things with your change you are not accounting for. such as cmd.run. which is not indomitable. currently because it is always going to have a changes line it is not going to trigger the mod_watch. some people do not know this. so try using watch with cmd.run. and it sort of works. the mod_watch will never trigger for them. but the command will still always run once. however with your change that once becomes twice. and with cmd.run that can be a huge change in things going on. including damaging systems.

[mitar]

Yes, I think we agree that it should be opt-in probably, even just for backwards compatibility sake.

I also understand that this is maybe too lenient as you describe. Maybe there is some additional way to keep track what has run and prevent double running. But not running at all, just because something else has changed, I think this has more side effects than maybe it was initially planned. Is my understanding correct that this was done primarily to prevent double running? Is there really no other way to prevent double running?

[mitar]

@bdrx312 @whytewolf I think I addressed both of your concerns. Now there is an opt-in mechanism for state to signal that mod watch should regardless because changes have not subsumed the mod watch action. In my local Salt version I for example modified docker_container state to set force_mod_watch if any changes made did not result in container restarting/recreating: mitar@cdf16d2

I think this addresses my issue and also allows for other states to implement this feature as suitable.

[mitar]

In fact, this change also fixes the "surprising" behavior documented called out in documentation about service.running. It can now get the same treatment and workarounds are not necessary anymore, without negative side effect for cmd.run: mitar@0a73f38

Because surprising behavior leads to bugs, I think this fix to service.running state should also be included. I can add it to this PR if others agree, and change documentation accordingly.