[DPE-8865] Extract error messages by marceloneppel · Pull Request #1320 · canonical/postgresql-operator

marceloneppel · 2025-11-28T19:42:43Z

Issue

Sometimes the error message from pgBackRest is sent to stdout instead of stderr (and we don't log them, as seen in #1280).

Solution

Check both stdout and stderr for errors and warnings. Debugging with the Field Engineering team before the sprint in Gothenburg, I figured out that some errors are reported as warnings (one case is when the replica cannot communicate with the primary through the pgBackRest TLS server because its IP is not in the SANs list, which was already fixed by #1162).

Fixes #1280

Checklist

I have added or updated any relevant documentation.
I have cleaned any remaining cloud resources from my accounts.

Sometimes the error message is sent to the stdout instead of the stderr, so it's important to check both Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

codecov · 2025-11-28T19:43:52Z

Codecov Report

❌ Patch coverage is 87.87879% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.60%. Comparing base (fbfd2e8) to head (a9f6fdc).
⚠️ Report is 5 commits behind head on 16/edge.

Files with missing lines	Patch %	Lines
src/backups.py	87.09%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           16/edge    #1320      +/-   ##
===========================================
+ Coverage    70.46%   70.60%   +0.13%     
===========================================
  Files           16       16              
  Lines         4249     4276      +27     
  Branches       684      689       +5     
===========================================
+ Hits          2994     3019      +25     
- Misses        1045     1047       +2     
  Partials       210      210

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

taurus-forever · 2025-12-09T13:08:42Z

LGTM, it looks like we are missing here # Test when the failure in the stanza check is due to an archive timeout. from 14 branch: https://github.com/canonical/postgresql-operator/pull/1328/files#diff-08e542b8e478d7c63342b8b5958a3f2987418c1cde80604661aba4ffd2100fceR893-R900

Concatenate multiple ERROR/WARN lines with "; " instead of " " for improved readability when debugging backup issues. Updated unit test to match new separator format. Cherry-picked from #1328 (commit 1f1b601). Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

marceloneppel · 2025-12-10T20:51:36Z

LGTM, it looks like we are missing here # Test when the failure in the stanza check is due to an archive timeout. from 14 branch: https://github.com/canonical/postgresql-operator/pull/1328/files#diff-08e542b8e478d7c63342b8b5958a3f2987418c1cde80604661aba4ffd2100fceR893-R900

Yes, we're missing. I initially created this PR to solve the issue it solves, then I ported it's code to the PG 14 branch's PR. I created #1346 to port it.

taurus-forever

Excellent! Thank you!

onkolahmet

Thank you Marcelo!

carlcsaposs-canonical

if https://pgbackrest.org/configuration.html#section-log is set, would just using stderr (or just using stdout) be sufficient?

it appears that it might be possible to configure stderr to output warning messages (--log-level-stderr warn) and that including stderr + stdout might lead to log duplication

(--log-level-console refers to stdout—ref: pgbackrest/pgbackrest#569 (comment))

by default, it appears that --log-level-console is warn and --log-level-stderr is off

Port archive timeout error handling (error code 82) from PR #1328 to allow users to fix network issues and retry with `juju resolve`. When archive operations timeout, the charm enters error state instead of blocked state, enabling recovery via juju resolve. Fixes #1346 Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

marceloneppel · 2025-12-15T12:00:37Z

if https://pgbackrest.org/configuration.html#section-log is set, would just using stderr (or just using stdout) be sufficient?

it appears that it might be possible to configure stderr to output warning messages (--log-level-stderr warn) and that including stderr + stdout might lead to log duplication

(--log-level-console refers to stdout—ref: pgbackrest/pgbackrest#569 (comment))

by default, it appears that --log-level-console is warn and --log-level-stderr is off

I think you're right, @carlcsaposs-canonical! I'll test updating those config options.

Addresses code review feedback from PR #1320 (review #3577653265) about potential log duplication when checking both stdout and stderr for errors. Changes: - Add PGBACKREST_LOG_LEVEL_STDERR constant (--log-level-stderr=warn) to ensure all pgBackRest commands output errors/warnings to stderr - Update all 9 pgBackRest command invocations to include the new flag - Simplify _extract_error_message() to only check stderr (removes stdout parameter entirely) - Update all 8 call sites and 7 unit tests to use new single-parameter API - Remove redundant test for stdout handling This approach eliminates the risk of log duplication while maintaining predictable error extraction. All errors and warnings now consistently appear in stderr, making error handling more reliable. Reference: https://pgbackrest.org/configuration.html#section-log

Addresses code review feedback from PR #1320 (review #3577653265) about potential log duplication when checking both stdout and stderr for errors. Changes: - Add PGBACKREST_LOG_LEVEL_STDERR constant (--log-level-stderr=warn) to ensure all pgBackRest commands output errors/warnings to stderr - Update all 9 pgBackRest command invocations to include the new flag - Simplify _extract_error_message() to only check stderr (removes stdout parameter entirely) - Update all 8 call sites and 7 unit tests to use new single-parameter API - Remove redundant test for stdout handling This approach eliminates the risk of log duplication while maintaining predictable error extraction. All errors and warnings now consistently appear in stderr, making error handling more reliable. Reference: https://pgbackrest.org/configuration.html#section-log Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

This reverts commit 13a9ac4. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

This reverts commit 1c3eb04. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

- Add --log-level-stderr=warn to test_initialise_stanza expected command to match implementation from commit d3a46b6 - Replace unused stdout variables with '_' in check_stanza, _is_primary_pgbackrest_service_running, and _on_restore_action - Add --log-level-console=debug to backup command for detailed output Fixes assertion error in tests/unit/test_backups.py::test_initialise_stanza Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

marceloneppel · 2025-12-15T15:01:19Z

Thanks, @carlcsaposs-canonical! I updated the PR to turn on logging on stderr and use that log instead of both.

src/constants.py

* Extract warning and error messages from pgBackRest logs Sometimes the error message is sent to the stdout instead of the stderr, so it's important to check both Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * Use semicolon separator for pgBackRest error messages Concatenate multiple ERROR/WARN lines with "; " instead of " " for improved readability when debugging backup issues. Updated unit test to match new separator format. Cherry-picked from #1328 (commit 1f1b601). Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * [DPE-9070] Handle pgBackRest archive timeout in check_stanza (#1350) Port archive timeout error handling (error code 82) from PR #1328 to allow users to fix network issues and retry with `juju resolve`. When archive operations timeout, the charm enters error state instead of blocked state, enabling recovery via juju resolve. Fixes #1346 Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * Standardize pgBackRest logging to use stderr consistently Addresses code review feedback from PR #1320 (review #3577653265) about potential log duplication when checking both stdout and stderr for errors. Changes: - Add PGBACKREST_LOG_LEVEL_STDERR constant (--log-level-stderr=warn) to ensure all pgBackRest commands output errors/warnings to stderr - Update all 9 pgBackRest command invocations to include the new flag - Simplify _extract_error_message() to only check stderr (removes stdout parameter entirely) - Update all 8 call sites and 7 unit tests to use new single-parameter API - Remove redundant test for stdout handling This approach eliminates the risk of log duplication while maintaining predictable error extraction. All errors and warnings now consistently appear in stderr, making error handling more reliable. Reference: https://pgbackrest.org/configuration.html#section-log Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * Revert "Standardize pgBackRest logging to use stderr consistently" This reverts commit 13a9ac4. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * Reapply "Standardize pgBackRest logging to use stderr consistently" This reverts commit 1c3eb04. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * Fix test_initialise_stanza and cleanup unused variables - Add --log-level-stderr=warn to test_initialise_stanza expected command to match implementation from commit d3a46b6 - Replace unused stdout variables with '_' in check_stanza, _is_primary_pgbackrest_service_running, and _on_restore_action - Add --log-level-console=debug to backup command for detailed output Fixes assertion error in tests/unit/test_backups.py::test_initialise_stanza Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> --------- Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> Signed-off-by: andreia <andreia.velasco@canonical.com>

Extract warning and error messages from pgBackRest logs

9813b49

Sometimes the error message is sent to the stdout instead of the stderr, so it's important to check both Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

github-actions bot added the Libraries: OK The charm libs used are OK and in-sync label Nov 28, 2025

marceloneppel added the bug Something isn't working as expected label Nov 28, 2025

marceloneppel mentioned this pull request Dec 2, 2025

[DPE-6762] Catch archive command timeout #1328

Merged

2 tasks

marceloneppel marked this pull request as ready for review December 4, 2025 12:47

marceloneppel requested review from onkolahmet and taurus-forever December 4, 2025 12:47

taurus-forever mentioned this pull request Dec 9, 2025

Failures with create-backup doesn't surface the error #794

Closed

taurus-forever previously approved these changes Dec 9, 2025

View reviewed changes

marceloneppel dismissed taurus-forever’s stale review via 1592518 December 10, 2025 20:18

github-actions bot added Libraries: Out of sync The charm libs used are out-of-sync and removed Libraries: OK The charm libs used are OK and in-sync labels Dec 12, 2025

marceloneppel force-pushed the dpe-8865-extract-error-messages branch from f6ff193 to 1592518 Compare December 12, 2025 13:59

Merge branch '16/edge' into dpe-8865-extract-error-messages

6928787

marceloneppel mentioned this pull request Dec 12, 2025

[DPE-9070] Handle pgBackRest archive timeout in check_stanza #1350

Merged

2 tasks

marceloneppel requested a review from taurus-forever December 12, 2025 19:41

taurus-forever requested a review from carlcsaposs-canonical December 15, 2025 10:38

taurus-forever previously approved these changes Dec 15, 2025

View reviewed changes

onkolahmet previously approved these changes Dec 15, 2025

View reviewed changes

carlcsaposs-canonical reviewed Dec 15, 2025

View reviewed changes

marceloneppel dismissed stale reviews from onkolahmet and taurus-forever via ac88cce December 15, 2025 11:31

Merge branch '16/edge' into dpe-8865-extract-error-messages

1382039

marceloneppel force-pushed the dpe-8865-extract-error-messages branch from 9db9b1e to 13a9ac4 Compare December 15, 2025 13:46

marceloneppel force-pushed the dpe-8865-extract-error-messages branch from 1c3eb04 to 58e049a Compare December 15, 2025 13:55

marceloneppel added 2 commits December 15, 2025 10:56

Revert "Standardize pgBackRest logging to use stderr consistently"

a4e61fc

This reverts commit 13a9ac4. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

Reapply "Standardize pgBackRest logging to use stderr consistently"

d3a46b6

This reverts commit 1c3eb04. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

marceloneppel force-pushed the dpe-8865-extract-error-messages branch from 58e049a to d3a46b6 Compare December 15, 2025 13:56

marceloneppel requested review from carlcsaposs-canonical, onkolahmet and taurus-forever December 15, 2025 15:01

taurus-forever approved these changes Dec 15, 2025

View reviewed changes

carlcsaposs-canonical approved these changes Dec 16, 2025

View reviewed changes

src/constants.py Show resolved Hide resolved

marceloneppel mentioned this pull request Dec 16, 2025

Move --log-level-stderr=warn to pgBackRest's configuration file #1353

Open

marceloneppel merged commit dad1b02 into 16/edge Dec 16, 2025
248 of 253 checks passed

marceloneppel deleted the dpe-8865-extract-error-messages branch December 16, 2025 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DPE-8865] Extract error messages#1320

[DPE-8865] Extract error messages#1320
marceloneppel merged 9 commits into16/edgefrom
dpe-8865-extract-error-messages

marceloneppel commented Nov 28, 2025 •

edited

Loading

Uh oh!

codecov bot commented Nov 28, 2025 •

edited

Loading

Uh oh!

taurus-forever commented Dec 9, 2025

Uh oh!

marceloneppel commented Dec 10, 2025

Uh oh!

taurus-forever left a comment

Uh oh!

onkolahmet left a comment

Uh oh!

carlcsaposs-canonical left a comment

Uh oh!

marceloneppel commented Dec 15, 2025

Uh oh!

marceloneppel commented Dec 15, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

marceloneppel commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Solution

Checklist

Uh oh!

codecov bot commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

taurus-forever commented Dec 9, 2025

Uh oh!

marceloneppel commented Dec 10, 2025

Uh oh!

taurus-forever left a comment

Choose a reason for hiding this comment

Uh oh!

onkolahmet left a comment

Choose a reason for hiding this comment

Uh oh!

carlcsaposs-canonical left a comment

Choose a reason for hiding this comment

Uh oh!

marceloneppel commented Dec 15, 2025

Uh oh!

marceloneppel commented Dec 15, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marceloneppel commented Nov 28, 2025 •

edited

Loading

codecov bot commented Nov 28, 2025 •

edited

Loading