Skip to content

RHINENG-27056: add feature flag to terminate all db sessions during migration#2237

Open
TenSt wants to merge 3 commits into
RedHatInsights:masterfrom
TenSt:stepan/RHINENG-27056-add-ff-to-terminate-all-db-sessions-during-migration
Open

RHINENG-27056: add feature flag to terminate all db sessions during migration#2237
TenSt wants to merge 3 commits into
RedHatInsights:masterfrom
TenSt:stepan/RHINENG-27056-add-ff-to-terminate-all-db-sessions-during-migration

Conversation

@TenSt

@TenSt TenSt commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Context:

ALTER USER … NOLOGIN blocks new logins but leaves existing connections open, which can block DDL. This flag is opt-in for major migrations only; normal deploys are unchanged (default false).

Built on top of the waitForSessionClosed fix (PR). This PR will be rebased afterwards to avoid conflicts.

This PR:

  • Adds terminate_db_sessions POD_CONFIG flag (default off) to kill app DB sessions before migration DDL
  • Terminates listener/evaluator/manager/vmaas_sync backends via pg_terminate_backend after NOLOGIN, then waits until no app sessions remain
  • Extracts prepareForMigration() from startMigration() (block → optional terminate → wait) so the pre-migration flow is testable without running MigrateUp
  • Adds listActiveAppSessions / terminateAppSessions helpers and unit tests for session termination and full pre-migration flow
  • Documents terminate_db_sessions=true on DATABASE_ADMIN_CONFIG in deploy/clowdapp.yaml
  • Adds documentation in all related files

Summary by Sourcery

Add an optional configuration flag to terminate application database sessions before running migrations and document the migration/session-handling flow.

New Features:

  • Introduce the terminate_db_sessions POD configuration flag to force-close app database sessions prior to migration DDL when enabled.

Enhancements:

  • Refactor pre-migration logic into a reusable prepareForMigration step that blocks users, optionally terminates sessions, and waits for sessions to drain.
  • Improve robustness of session polling by handling query errors explicitly and excluding the admin connection from active-session checks.

Documentation:

  • Document the migration and session-handling process, including the terminate_db_sessions flag, in the database and architecture docs and agent guide.
  • Update deployment configuration docs to describe how to enable terminate_db_sessions on the db-migration job.

Tests:

  • Add integration-style tests covering active-session detection, session termination, and the full pre-migration flow using the new configuration flag.

@TenSt TenSt requested a review from a team as a code owner June 18, 2026 19:39
@sourcery-ai

sourcery-ai Bot commented Jun 18, 2026

Copy link
Copy Markdown

Reviewer's Guide

Adds an opt-in terminate_db_sessions flag to database-admin to forcibly close app DB sessions before migrations, refactors pre-migration logic into a reusable prepareForMigration() flow, introduces helpers and tests for listing/terminating active sessions, and documents the new behavior and configuration across database and architecture docs and ClowdApp config.

File-Level Changes

Change Details Files
Add helper functions to detect, list, and terminate active app database sessions and integrate them into the pre-migration flow.
  • Introduce findActiveAppSession to safely query for a single active app session using pg_stat_activity with proper error handling.
  • Add appSession struct and listActiveAppSessions to fetch PIDs, usernames, and queries for current app sessions, excluding the current backend.
  • Implement terminateAppSessions to call pg_terminate_backend on listed sessions and log each termination.
  • Update waitForSessionClosed to use findActiveAppSession and robust error logging/retry instead of inline SQL logic.
database_admin/update.go
Refactor migration startup into a testable pre-migration preparation step with optional session termination controlled by a new config flag.
  • Add terminateDBSessions boolean configured via utils.PodConfig (terminate_db_sessions) to control whether lingering app sessions are killed before DDL.
  • Extract prepareForMigration from startMigration to encapsulate blocking users, optional termination, and waiting for sessions to close.
  • Modify startMigration to call prepareForMigration before running MigrateUp, preserving the existing migration sequence with added optional termination.
database_admin/update.go
database_admin/config.go
Add comprehensive tests covering session discovery, termination behavior, and the full pre-migration flow.
  • Create update_test.go with helpers to open app DB connections against the configured database.
  • Add tests for findActiveAppSession handling of invalid DB connections, no sessions, and existing sessions.
  • Add tests verifying listActiveAppSessions returns expected app sessions with valid PIDs.
  • Add tests for terminateAppSessions to ensure app sessions are killed and the client connection becomes unusable.
  • Add TestStartMigrationBeforeMigrateUp to verify prepareForMigration blocks users, terminates sessions when configured, and leaves roles in NOLOGIN before MigrateUp.
database_admin/update_test.go
Document migration/session-handling behavior and the terminate_db_sessions flag, and wire it into ClowdApp configuration and architecture docs.
  • Extend docs/md/database.md with a Migrations section describing schema migration flow, pre-migration session handling, and detailed guidance on when/how to use terminate_db_sessions.
  • Update AGENTS.md to reference migration docs and summarize guidance for recommending terminate_db_sessions in support scenarios.
  • Expand docs/md/architecture.md database-admin section to include the new pre-migration behavior and link back to the migration docs.
  • Annotate deploy/clowdapp.yaml DATABASE_ADMIN_CONFIG parameter with the terminate_db_sessions usage comment.
docs/md/database.md
AGENTS.md
docs/md/architecture.md
deploy/clowdapp.yaml

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@TenSt TenSt changed the title Stepan/rhineng 27056 add ff to terminate all db sessions during migration RHINENG-27056: add feature flag to terminate all db sessions during migration Jun 18, 2026

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="database_admin/update.go" line_range="21" />
<code_context>
+const activeAppSessionsQuery = `SELECT usename || ' ' || substring(query for 50)`
</code_context>
<issue_to_address>
**suggestion:** Consider aligning the pg_stat_activity filters between the "find" and "list" queries.

`activeAppSessionsQuery` and `activeAppSessionPIDsQuery` use slightly different filters (e.g., only the latter excludes `pg_backend_pid()`). Please either share a common base condition or make their semantics intentionally consistent, so the “active” sessions you wait on are the same ones you terminate. Adding an explicit `pg_backend_pid()` exclusion to both would also guard against future config changes (e.g., if `lockUsers` ever includes the current user).
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread database_admin/update.go

const activeAppSessionsQuery = `SELECT usename || ' ' || substring(query for 50)
FROM pg_stat_activity
WHERE usename = ANY($1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Consider aligning the pg_stat_activity filters between the "find" and "list" queries.

activeAppSessionsQuery and activeAppSessionPIDsQuery use slightly different filters (e.g., only the latter excludes pg_backend_pid()). Please either share a common base condition or make their semantics intentionally consistent, so the “active” sessions you wait on are the same ones you terminate. Adding an explicit pg_backend_pid() exclusion to both would also guard against future config changes (e.g., if lockUsers ever includes the current user).

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.06%. Comparing base (704b877) to head (df30fad).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2237   +/-   ##
=======================================
  Coverage   59.06%   59.06%           
=======================================
  Files         138      138           
  Lines        8848     8848           
=======================================
  Hits         5226     5226           
  Misses       3076     3076           
  Partials      546      546           
Flag Coverage Δ
unittests 59.06% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 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.

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.

2 participants