close
Skip to content

Route pgjsonb database errors through Salt's logger instead of stderr#69049

Open
co-cy wants to merge 1 commit into
saltstack:3006.xfrom
co-cy:pgjsonb-stderr-to-log
Open

Route pgjsonb database errors through Salt's logger instead of stderr#69049
co-cy wants to merge 1 commit into
saltstack:3006.xfrom
co-cy:pgjsonb-stderr-to-log

Conversation

@co-cy
Copy link
Copy Markdown

@co-cy co-cy commented May 5, 2026

What does this PR do?

salt/returners/pgjsonb.py has eight error-handling sites in _get_serv, _purge_jobs and _archive_jobs that write psycopg2.DatabaseError messages directly to sys.stderr and re-raise. On a daemonized master sys.stderr ends up in systemd's journal at best (or /dev/null at worst) — never in the configured log_file / syslog destination. Operators that scrape Salt's log file have no idea those database errors happened.

This PR replaces every sys.stderr.write with log.exception(...) carrying a description of the operation that failed, so the message and a full traceback reach Salt's configured log destination. The dead error = err.args assignment is removed, the now-unused import sys is dropped, and raise err is normalised to bare raise to preserve the original traceback. Behaviour (rollback, re-raise) is unchanged — only the visibility of the error changes.

The asymmetric except Exception block on _archive_jobs jids-insert is updated to use log.exception for consistency, but it is left in place for the same scope reason — sorting that asymmetry out belongs in a separate PR.

What issues does this PR fix or reference?

Fixes #69048

Previous Behavior

except psycopg2.DatabaseError as err:
    error = err.args
    sys.stderr.write(str(error))
    cursor.execute("ROLLBACK")
    raise err

The error message went to stderr (journal / /dev/null) and never to the configured Salt log. The exception still propagated, but with a synthetic traceback (because of raise err instead of bare raise).

New Behavior

except psycopg2.DatabaseError:
    log.exception("pgjsonb: failed to purge salt_returns")
    cursor.execute("ROLLBACK")
    raise

The error message and the full traceback are routed through log.exception to Salt's configured logger. The original traceback is preserved.

Merge requirements satisfied?

  • Docs (no user-facing change; not required)
  • Changelog — changelog/69048.fixed.md
  • Tests written/updated — see tests/pytests/unit/returners/test_pgjsonb.py:
    • test__purge_jobs_logs_via_salt_logger_and_reraises_on_db_error
    • test__archive_jobs_logs_via_salt_logger_and_reraises_on_db_error
    • test__get_serv_logs_via_salt_logger_and_reraises_on_yield_error

Commits signed with GPG?

No.

`salt.returners.pgjsonb` had eight error-handling sites in `_get_serv`,
`_purge_jobs` and `_archive_jobs` that wrote `psycopg2.DatabaseError`
messages directly to `sys.stderr` and re-raised:

    except psycopg2.DatabaseError as err:
        error = err.args
        sys.stderr.write(str(error))
        cursor.execute("ROLLBACK")
        raise err

On a daemonized master `sys.stderr` is captured by systemd's journal
(or worse, /dev/null), bypassing the configured `log_file` /
`log_level_logfile` and syslog. Operators that scrape Salt's log file
never see these errors, even though the rest of the file already uses
`log.error` (e.g. `clean_old_jobs`).

The local `error = err.args` assignment is dead -- only `str(err.args)`
is written, then the variable is unused. Also drop `import sys`,
which becomes unused after the migration.

Replace each call with `log.exception(...)` carrying a description of
the operation that failed, so the traceback is preserved in the log.
Behaviour (rollback, re-raise) is unchanged. The asymmetric
`except Exception` block on `_archive_jobs` jids-insert is updated to
the same pattern but kept in place (its scope is PR-7, not this PR).

Add three behavioural tests with `caplog`: one each for `_get_serv`,
`_purge_jobs` and `_archive_jobs` confirming the error reaches Salt's
logger, the transaction is rolled back, and the exception propagates.

Refs: saltstack#69048
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant