Skip to content

fix(podlogs): Follow mode exit bug and test improvements - #9599

Merged
leonardoce merged 2 commits into
mainfrom
dev/fix-flaky-podlogs-test
Jan 12, 2026
Merged

leonardoce merged 2 commits into
mainfrom
dev/fix-flaky-podlogs-test

Conversation

@mnencia

@mnencia mnencia commented Dec 29, 2025 •

Copy link
Copy Markdown
Member

The test "should catch extra logs if given the follow option" was flaky in CI because it tested implementation details (counting loop iterations with tight timing) rather than actual behavior.

Redesigned the test to verify what Follow=true actually does: it keeps streaming until the context is cancelled. The test now uses Eventually/Consistently patterns that handle timing variations gracefully, making it robust across different environments.

The improved test exposed a bug in the production code: the streaming function would exit when all current log streams completed, even when Follow=true was set. This caused premature exit instead of continuing to poll for new or restarted pods. Fixed by changing the exit condition to only apply when Follow=false.

@mnencia
mnencia requested a review from a team December 29, 2025 16:34
@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Dec 29, 2025
@cnpg-bot cnpg-bot added backport-requested ◀️ This pull request should be backported to all supported releases release-1.25 release-1.28 labels Dec 29, 2025
@github-actions

Copy link
Copy Markdown
Contributor

❗ By default, the pull request is configured to backport to all release branches.

  • To stop backporting this pr, remove the label: backport-requested ◀️ or add the label 'do not backport'
  • To stop backporting this pr to a certain release branch, remove the specific branch label: release-x.y

@NiccoloFei
NiccoloFei force-pushed the dev/fix-flaky-podlogs-test branch from d6cc96d to 027c158 Compare December 29, 2025 16:36
@mnencia

mnencia commented Dec 29, 2025 •

Copy link
Copy Markdown
Member Author

/ok-to-merge test only change

@mnencia mnencia added the ok to merge 👌 This PR can be merged label Dec 29, 2025
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Dec 30, 2025
@mnencia
mnencia force-pushed the dev/fix-flaky-podlogs-test branch from 027c158 to 2bda92a Compare December 30, 2025 12:17
@mnencia
mnencia force-pushed the dev/fix-flaky-podlogs-test branch from 2bda92a to 73a08a5 Compare January 7, 2026 17:55
@mnencia mnencia changed the title test(podlogs): make Follow=true test robust and behavior-focused fix(podlogs): Follow mode exit bug and test improvements Jan 10, 2026
@mnencia

mnencia commented Jan 10, 2026 •

Copy link
Copy Markdown
Member Author

One of the executions of the improved test failed in CI: https://github.com/cloudnative-pg/cloudnative-pg/actions/runs/20791171388/job/59713483662?pr=9599

The failure exposed a bug in cluster_writer.go: the streaming function would exit when streamSet.isZero() returned true, even with Follow=true enabled. This violated the Follow semantics which should keep polling for new/restarted pods.

The race occurred because log streams could complete before the main loop checked for active streams, causing premature exit. Fixed by adding !csr.Options.Follow to the exit condition, ensuring Follow mode continues looping as intended.

@mnencia
mnencia force-pushed the dev/fix-flaky-podlogs-test branch from 3203816 to a48a08c Compare January 10, 2026 22:35
@mnencia

mnencia commented Jan 10, 2026

Copy link
Copy Markdown
Member Author

/test

@github-actions

Copy link
Copy Markdown
Contributor

@mnencia
mnencia force-pushed the dev/fix-flaky-podlogs-test branch from a48a08c to 22baf8e Compare January 10, 2026 22:42
The Follow=true test was flaky in CI because it tested implementation
details (counting loop iterations with tight timing) rather than actual
behavior.

Redesigned the test to verify what Follow=true actually does: it keeps
streaming until the context is cancelled, rather than exiting after one
read. The test now checks that streaming starts, continues running
without exiting on its own, and only stops when we cancel the context.

This approach uses Eventually/Consistently patterns that handle timing
variations gracefully, making it robust across different environments.

Signed-off-by: Marco Nenciarini <[email protected]>
Fixed a bug where Follow=true would exit prematurely when all current
log streams completed, instead of continuing to poll for new or
restarted pods.

Changed the exit condition on zero active streams to only apply when
Follow=false, allowing Follow mode to keep looping as intended.

Signed-off-by: Marco Nenciarini <[email protected]>
@leonardoce
leonardoce force-pushed the dev/fix-flaky-podlogs-test branch from 22baf8e to 53265c6 Compare January 12, 2026 16:03
@leonardoce
leonardoce merged commit 76817ec into main Jan 12, 2026
34 checks passed
@leonardoce
leonardoce deleted the dev/fix-flaky-podlogs-test branch January 12, 2026 16:19
cnpg-bot pushed a commit that referenced this pull request Jan 12, 2026
The test "should catch extra logs if given the follow option" was flaky
in CI because it tested implementation details (counting loop iterations
with tight timing) rather than actual behavior.

Redesigned the test to verify what Follow=true actually does: it keeps
streaming until the context is cancelled. The test now uses
Eventually/Consistently patterns that handle timing variations
gracefully, making it robust across different environments.

The improved test exposed a bug in the production code: the streaming
function would exit when all current log streams completed, even when
Follow=true was set. This caused premature exit instead of continuing to
poll for new or restarted pods. Fixed by changing the exit condition to
only apply when Follow=false.

Signed-off-by: Marco Nenciarini <[email protected]>
(cherry picked from commit 76817ec)
cnpg-bot pushed a commit that referenced this pull request Jan 12, 2026
The test "should catch extra logs if given the follow option" was flaky
in CI because it tested implementation details (counting loop iterations
with tight timing) rather than actual behavior.

Redesigned the test to verify what Follow=true actually does: it keeps
streaming until the context is cancelled. The test now uses
Eventually/Consistently patterns that handle timing variations
gracefully, making it robust across different environments.

The improved test exposed a bug in the production code: the streaming
function would exit when all current log streams completed, even when
Follow=true was set. This caused premature exit instead of continuing to
poll for new or restarted pods. Fixed by changing the exit condition to
only apply when Follow=false.

Signed-off-by: Marco Nenciarini <[email protected]>
(cherry picked from commit 76817ec)
cnpg-bot pushed a commit that referenced this pull request Jan 12, 2026
The test "should catch extra logs if given the follow option" was flaky
in CI because it tested implementation details (counting loop iterations
with tight timing) rather than actual behavior.

Redesigned the test to verify what Follow=true actually does: it keeps
streaming until the context is cancelled. The test now uses
Eventually/Consistently patterns that handle timing variations
gracefully, making it robust across different environments.

The improved test exposed a bug in the production code: the streaming
function would exit when all current log streams completed, even when
Follow=true was set. This caused premature exit instead of continuing to
poll for new or restarted pods. Fixed by changing the exit condition to
only apply when Follow=false.

Signed-off-by: Marco Nenciarini <[email protected]>
(cherry picked from commit 76817ec)
mnencia added a commit that referenced this pull request Jan 20, 2026
The test "should catch extra logs if given the follow option" was flaky
in CI because it tested implementation details (counting loop iterations
with tight timing) rather than actual behavior.

Redesigned the test to verify what Follow=true actually does: it keeps
streaming until the context is cancelled. The test now uses
Eventually/Consistently patterns that handle timing variations
gracefully, making it robust across different environments.

The improved test exposed a bug in the production code: the streaming
function would exit when all current log streams completed, even when
Follow=true was set. This caused premature exit instead of continuing to
poll for new or restarted pods. Fixed by changing the exit condition to
only apply when Follow=false.

Signed-off-by: Marco Nenciarini <[email protected]>
(cherry picked from commit 76817ec)
mnencia added a commit that referenced this pull request Feb 5, 2026
Signed-off-by: Marco Nenciarini <[email protected]>
renovate Bot added a commit to sdwilsh/ansible-playbooks that referenced this pull request Mar 26, 2026
##### [\`v1.28.1\`](https://github.com/cloudnative-pg/cloudnative-pg/releases/tag/v1.28.1)

**Release date:** Feb 5, 2026

##### Enhancements

- Added support for Azure's `DefaultAzureCredential` authentication mechanism for backup and recovery operations. This can be enabled by setting `azureCredentials.useDefaultAzureCredentials: true` in the backup configuration, simplifying authentication in Azure environments without requiring explicit storage account keys or SAS tokens. ([#9468](cloudnative-pg/cloudnative-pg#9468)) <!-- 1.27 1.25 -->

##### Fixes

- Fixed validation of PostgreSQL extension names containing underscores (e.g., `pg_partman`, `pg_ivm`). Extension names with underscores are automatically sanitized to use hyphens for Kubernetes volume names while preserving the original name in mount paths. Webhook validation prevents naming conflicts after sanitization. Contributed by [@shusaan](https://github.com/shusaan). ([#9386](cloudnative-pg/cloudnative-pg#9386)) <!-- 1.27 -->

- Fixed a critical issue where the `TimelineID` in the cluster status was not reset to 1 after a major version upgrade. Because `pg_upgrade` initializes a new timeline, keeping the old ID (e.g., timeline 2) caused replicas to attempt to restore incompatible history files from object storage, leading to fatal "requested timeline is not a child of this server's history" errors. ([#9830](cloudnative-pg/cloudnative-pg#9830)) <!-- 1.27 -->

- Fixed an issue where stale TLS status fields in the `Pooler` were not cleared after being removed from the specification. This was particularly critical when upgrading to v1.28.0, where the `ServerTLS` field was repurposed, causing PgBouncer to use incorrect certificates and resulting in "unsupported certificate" errors that blocked all application connectivity. The operator now explicitly clears `ServerCA`, `ClientCA`, `ClientTLS`, and `ServerTLS` status fields when they are no longer configured. ([#9397](cloudnative-pg/cloudnative-pg#9397))

- Fixed a bug where replicas could enter a crash-loop by attempting to download timeline history files from future timelines. This occurred when stale files remained in the WAL archive from a previous cluster life, and replicas would incorrectly try to fetch them during recovery. ([#9650](cloudnative-pg/cloudnative-pg#9650)) <!-- 1.27 1.25 -->

- Fixed a race condition in `replica_cluster` setups during designated primary transitions, preventing transient "no primary" states in the replica cluster. ([#9601](cloudnative-pg/cloudnative-pg#9601)) <!-- 1.27 1.25 -->

- The backup controller now uses the unique instance session ID to detect instance manager restarts. This prevents the operator from incorrectly assuming a backup is still progressing if the underlying container has crashed and restarted, which previously led to orphaned backup objects. ([#9370](cloudnative-pg/cloudnative-pg#9370)) <!-- 1.27 -->

- Fixed a validation gap in Azure object store configurations where the `storageAccount` was not required when using explicit credentials (such as a storage key or SAS token). The operator now enforces that a storage account name is provided in these cases and that `connectionString` is mutually exclusive with other authentication parameters. ([#9604](cloudnative-pg/cloudnative-pg#9604)) <!-- 1.27 1.25 -->

- Optimized the deletion path so the operator begins cleaning up resources immediately when a cluster is marked for deletion. This significantly reduces the time a cluster remains in `Terminating` status while waiting for internal reconciliation loops. ([#9555](cloudnative-pg/cloudnative-pg#9555)) <!-- 1.27 1.25 -->

- Fixed an issue where replication slots were not properly dropped from replicas when the feature was disabled or the cluster was reconfigured. This ensures that unused slots do not cause WAL build-up on the primary. ([#9381](cloudnative-pg/cloudnative-pg#9381)) <!-- 1.27 1.25 -->

- Fixed an issue where `imagePullSecrets` were not added to the `ServiceAccount` created for the `Pooler`. Previously, these secrets were applied to the Deployment but not the SA, which caused image pull failures in restricted environments using certain security policies. ([#9427](cloudnative-pg/cloudnative-pg#9427)) <!-- 1.27 1.25 -->

- Added a check to verify ownership before the operator deletes a `PodMonitor`. This prevents the operator from accidentally deleting manually managed monitoring resources that happen to share a name with expected CNPG resources. Contributed by [@juliamertz](https://github.com/juliamertz). ([#9340](cloudnative-pg/cloudnative-pg#9340)) <!-- 1.27 1.25 -->

- Fixed a bug where `pg_stat_archiver` metrics would continue to report stale data on standby instances after a switchover. The exporter now skips these metrics on standbys, as PostgreSQL only provides valid archiver stats on the primary. ([#9411](cloudnative-pg/cloudnative-pg#9411)) <!-- 1.27 1.25 -->

- Clarified the interpretation of timestamp formats for recovery `targetTime`. Timestamps provided without an explicit timezone are now consistently interpreted as UTC. Contributed by [@pchovelon](https://github.com/pchovelon). ([#8937](cloudnative-pg/cloudnative-pg#8937)) <!-- 1.27 1.25 -->

- Fixed backup status updates to prevent "resource has been modified" errors during concurrent updates. ([#9551](cloudnative-pg/cloudnative-pg#9551)) <!-- 1.27 1.25 -->

- Fixed event reporting to use the correct pod name when a backup pod is not found. ([#9552](cloudnative-pg/cloudnative-pg#9552)) <!-- 1.27 1.25 -->

- Improved performance of scheduled backup operations for clusters with a very high number of historical backups. ([#9489](cloudnative-pg/cloudnative-pg#9489)) <!-- 1.27 1.25 -->

- Fixed error handling when removing finalizers on `Database` objects. ([#9431](cloudnative-pg/cloudnative-pg#9431)) <!-- 1.27 1.25 -->

- `cnpg` plugin:

  - Updated the `status` command to display "Disabled" when the `skipWalArchiving` annotation is present on a cluster. This replaces confusing "starting up" or "unknown" states when WAL archiving is intentionally bypassed. ([#9709](cloudnative-pg/cloudnative-pg#9709)) <!-- 1.27 1.25 -->

  - Fixed the `logs --follow` command to continue polling for new pods instead of exiting prematurely when all current log streams complete. ([#9599](cloudnative-pg/cloudnative-pg#9599)) <!-- 1.27 1.25 -->
sdwilsh pushed a commit to sdwilsh/ansible-playbooks that referenced this pull request Mar 26, 2026
##### [\`v1.28.1\`](https://github.com/cloudnative-pg/cloudnative-pg/releases/tag/v1.28.1)

**Release date:** Feb 5, 2026

##### Enhancements

- Added support for Azure's `DefaultAzureCredential` authentication mechanism for backup and recovery operations. This can be enabled by setting `azureCredentials.useDefaultAzureCredentials: true` in the backup configuration, simplifying authentication in Azure environments without requiring explicit storage account keys or SAS tokens. ([#9468](cloudnative-pg/cloudnative-pg#9468)) <!-- 1.27 1.25 -->

##### Fixes

- Fixed validation of PostgreSQL extension names containing underscores (e.g., `pg_partman`, `pg_ivm`). Extension names with underscores are automatically sanitized to use hyphens for Kubernetes volume names while preserving the original name in mount paths. Webhook validation prevents naming conflicts after sanitization. Contributed by [@shusaan](https://github.com/shusaan). ([#9386](cloudnative-pg/cloudnative-pg#9386)) <!-- 1.27 -->

- Fixed a critical issue where the `TimelineID` in the cluster status was not reset to 1 after a major version upgrade. Because `pg_upgrade` initializes a new timeline, keeping the old ID (e.g., timeline 2) caused replicas to attempt to restore incompatible history files from object storage, leading to fatal "requested timeline is not a child of this server's history" errors. ([#9830](cloudnative-pg/cloudnative-pg#9830)) <!-- 1.27 -->

- Fixed an issue where stale TLS status fields in the `Pooler` were not cleared after being removed from the specification. This was particularly critical when upgrading to v1.28.0, where the `ServerTLS` field was repurposed, causing PgBouncer to use incorrect certificates and resulting in "unsupported certificate" errors that blocked all application connectivity. The operator now explicitly clears `ServerCA`, `ClientCA`, `ClientTLS`, and `ServerTLS` status fields when they are no longer configured. ([#9397](cloudnative-pg/cloudnative-pg#9397))

- Fixed a bug where replicas could enter a crash-loop by attempting to download timeline history files from future timelines. This occurred when stale files remained in the WAL archive from a previous cluster life, and replicas would incorrectly try to fetch them during recovery. ([#9650](cloudnative-pg/cloudnative-pg#9650)) <!-- 1.27 1.25 -->

- Fixed a race condition in `replica_cluster` setups during designated primary transitions, preventing transient "no primary" states in the replica cluster. ([#9601](cloudnative-pg/cloudnative-pg#9601)) <!-- 1.27 1.25 -->

- The backup controller now uses the unique instance session ID to detect instance manager restarts. This prevents the operator from incorrectly assuming a backup is still progressing if the underlying container has crashed and restarted, which previously led to orphaned backup objects. ([#9370](cloudnative-pg/cloudnative-pg#9370)) <!-- 1.27 -->

- Fixed a validation gap in Azure object store configurations where the `storageAccount` was not required when using explicit credentials (such as a storage key or SAS token). The operator now enforces that a storage account name is provided in these cases and that `connectionString` is mutually exclusive with other authentication parameters. ([#9604](cloudnative-pg/cloudnative-pg#9604)) <!-- 1.27 1.25 -->

- Optimized the deletion path so the operator begins cleaning up resources immediately when a cluster is marked for deletion. This significantly reduces the time a cluster remains in `Terminating` status while waiting for internal reconciliation loops. ([#9555](cloudnative-pg/cloudnative-pg#9555)) <!-- 1.27 1.25 -->

- Fixed an issue where replication slots were not properly dropped from replicas when the feature was disabled or the cluster was reconfigured. This ensures that unused slots do not cause WAL build-up on the primary. ([#9381](cloudnative-pg/cloudnative-pg#9381)) <!-- 1.27 1.25 -->

- Fixed an issue where `imagePullSecrets` were not added to the `ServiceAccount` created for the `Pooler`. Previously, these secrets were applied to the Deployment but not the SA, which caused image pull failures in restricted environments using certain security policies. ([#9427](cloudnative-pg/cloudnative-pg#9427)) <!-- 1.27 1.25 -->

- Added a check to verify ownership before the operator deletes a `PodMonitor`. This prevents the operator from accidentally deleting manually managed monitoring resources that happen to share a name with expected CNPG resources. Contributed by [@juliamertz](https://github.com/juliamertz). ([#9340](cloudnative-pg/cloudnative-pg#9340)) <!-- 1.27 1.25 -->

- Fixed a bug where `pg_stat_archiver` metrics would continue to report stale data on standby instances after a switchover. The exporter now skips these metrics on standbys, as PostgreSQL only provides valid archiver stats on the primary. ([#9411](cloudnative-pg/cloudnative-pg#9411)) <!-- 1.27 1.25 -->

- Clarified the interpretation of timestamp formats for recovery `targetTime`. Timestamps provided without an explicit timezone are now consistently interpreted as UTC. Contributed by [@pchovelon](https://github.com/pchovelon). ([#8937](cloudnative-pg/cloudnative-pg#8937)) <!-- 1.27 1.25 -->

- Fixed backup status updates to prevent "resource has been modified" errors during concurrent updates. ([#9551](cloudnative-pg/cloudnative-pg#9551)) <!-- 1.27 1.25 -->

- Fixed event reporting to use the correct pod name when a backup pod is not found. ([#9552](cloudnative-pg/cloudnative-pg#9552)) <!-- 1.27 1.25 -->

- Improved performance of scheduled backup operations for clusters with a very high number of historical backups. ([#9489](cloudnative-pg/cloudnative-pg#9489)) <!-- 1.27 1.25 -->

- Fixed error handling when removing finalizers on `Database` objects. ([#9431](cloudnative-pg/cloudnative-pg#9431)) <!-- 1.27 1.25 -->

- `cnpg` plugin:

  - Updated the `status` command to display "Disabled" when the `skipWalArchiving` annotation is present on a cluster. This replaces confusing "starting up" or "unknown" states when WAL archiving is intentionally bypassed. ([#9709](cloudnative-pg/cloudnative-pg#9709)) <!-- 1.27 1.25 -->

  - Fixed the `logs --follow` command to continue polling for new pods instead of exiting prematurely when all current log streams complete. ([#9599](cloudnative-pg/cloudnative-pg#9599)) <!-- 1.27 1.25 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-requested ◀️ This pull request should be backported to all supported releases lgtm This PR has been approved by a maintainer no-issue ok to merge 👌 This PR can be merged release-1.28 size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants