Salt exits with status 0 when it fails to render a state

[DemiMarie]

How to file a helpful issue

Qubes OS release

R4.1

Brief summary

If Salt fails to render a state (because of duplicate keys for example), the exit code of qubesctl is 0.

Steps to reproduce

Create a state that has duplicate keys in a YAML map.

Expected behavior

Non-zero exit code

Actual behavior

Zero exit code

[lkubb]

FYI: I just submitted a fix upstream for this issue, it was merged and will be in 3006.2: saltstack/salt#64515

[DemiMarie]

Thanks @lkubb! Qubes should probably backport this patch.

[qubesos-bot]

Automated announcement from builder-github

The component salt (including package salt) has been pushed to the r4.2 testing repository for the Fedora template.
To test this update, please install it with the following command:

sudo dnf update --enablerepo=qubes-vm-r4.2-current-testing

Changes included in this update

[DemiMarie]

Adding “backport pending” because R4.1 is still affected.

[marmarek]

The patch causes regression: saltstack/salt#64575. So, no backporting until it's fixed (and if not fixed soon, I'll need to revert the patch itself).

[lkubb]

@marmarek Without having much to go on, I have to assume it's caused by https://github.com/saltstack/salt/blob/c463c94b8d29ba4ebfeb45c36b7d7b6653e9711f/salt/client/ssh/__init__.py#L555-L559 and the remote returning something like {"local": {"retcode": null}} for whatever reason. Otherwise, the retcode that's passed through when calling a wfunc should always be an int.

Would you be able to patch the line and see if that fixes the stacktrace?

diff --git a/salt/client/ssh/__init__.py b/salt/client/ssh/__init__.py
index 0d9ac9509b..88449b4c6e 100644
--- a/salt/client/ssh/__init__.py
+++ b/salt/client/ssh/__init__.py
@@ -554,7 +554,7 @@ class SSH(MultiprocessingStateMixin):
                 ret["ret"] = data["local"]
                 try:
                     # Ensure a reported local retcode is kept
-                    retcode = data["local"]["retcode"]
+                    retcode = int(data["local"]["retcode"])
                 except (KeyError, TypeError):
                     pass
             else:

Either way it would be very helpful to see the final output, so if the above does not work, you could just force the retcode to be an int where the stacktrace happens.

diff --git a/salt/client/ssh/__init__.py b/salt/client/ssh/__init__.py
index 0d9ac9509b..e04d9c5234 100644
--- a/salt/client/ssh/__init__.py
+++ b/salt/client/ssh/__init__.py
@@ -807,7 +807,7 @@ class SSH(MultiprocessingStateMixin):
         final_exit = 0
         for ret, retcode in self.handle_ssh():
             host = next(iter(ret))
-            final_exit = max(final_exit, retcode)
+            final_exit = max(final_exit, int(retcode))

             self.cache_job(jid, host, ret[host], fun)
             ret = self.key_deploy(host, ret)

I have a Qubes dev setup as well, so if it's something I can reproduce there without much hassle, I'm happy to do the above myself.

[marmarek]

Unfortunately, I do not have reliable reproducer. I noticed it on one test run (given number of test runs since applying the patch, the reproduction rate is about 10%, could be worse).
The int() wont work, since int(None) is TypeError: int() argument must be a string, a bytes-like object or a real number, not 'NoneType'...

I have to assume it's caused by https://github.com/saltstack/salt/blob/c463c94b8d29ba4ebfeb45c36b7d7b6653e9711f/salt/client/ssh/__init__.py#L555-L559

Yes, I think so.

and the remote returning something like {"local": {"retcode": null}} for whatever reason. Otherwise, the retcode that's passed through when calling a wfunc should always be an int.

Unfortunately, I don't have returned data in any log, so cannot say for sure. But likely something like this.

[lkubb]

You're right, it should be or 0 of course. I will submit a PR upstream that should fix the stacktrace in those cases, but the exact cause is a mystery to me currently.

[marmarek]

I'm not sure if or 0 is the best approach - it will pretend everything is okay when you don't get exit code. Maybe better treat it as a failure instead? (until real reason for this failure is found)

[lkubb]

I agree, 0 would have been a quick fix for testing only. That's very impractical in CI ofc.

I actually reworked the salt-ssh error handling a lot since submitting this PR to fix other issues, so my picture of the current code is a bit hazy, but forcing it to 1 in those cases should be fine.

Edit: In the meantime, I'm quite certain I found the source of the None retcodes. The corresponding fix is included in the regression fix PR as well.

[qubesos-bot]

Automated announcement from builder-github

The component salt (including package salt) has been pushed to the r4.2 testing repository for the Fedora template.
To test this update, please install it with the following command:

sudo dnf update --enablerepo=qubes-vm-r4.2-current-testing

Changes included in this update

[qubesos-bot]

Automated announcement from builder-github

The component salt (including package salt) has been pushed to the r4.2 stable repository for the Fedora template.
To install this update, please use the standard update command:

sudo dnf update

Changes included in this update