Skip to content

ROX-33509: change psql lock to take lockID argument#19943

Open
johannes94 wants to merge 3 commits intomasterfrom
jmalsam/generic-psql-lock-logic
Open

ROX-33509: change psql lock to take lockID argument#19943
johannes94 wants to merge 3 commits intomasterfrom
jmalsam/generic-psql-lock-logic

Conversation

@johannes94
Copy link
Copy Markdown
Contributor

@johannes94 johannes94 commented Apr 10, 2026

Description

I want to reuse the logic for the PSQL advisory lock for the upcoming background migration implementation, but with a different lock ID. Because of that I:

  • Moved it to a dedicated pkg pkg/dblock
  • Changed the func to take the lockID as an input argument rather than a constant
  • Changed the migrator lock.go to use that shared PKG with it's constant ID

User-facing documentation

Testing and quality

  • the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • added unit tests
  • added e2e tests
  • added regression tests
  • added compatibility tests
  • modified existing tests

How I validated my change

Automated tests.

@johannes94 johannes94 requested a review from a team as a code owner April 10, 2026 11:59
@johannes94 johannes94 changed the title ROX-33509: change psql lock logic to take lock ID as a argument ROX-33509: change psql lock to take lockID argument Apr 10, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 10, 2026

🚀 Build Images Ready

Images are ready for commit 9c9a412. To use with deploy scripts:

export MAIN_IMAGE_TAG=4.11.x-620-g9c9a412eb1

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 10, 2026

Codecov Report

❌ Patch coverage is 69.69697% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.56%. Comparing base (1901021) to head (9c9a412).
⚠️ Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
pkg/dblock/dblock.go 69.69% 6 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #19943      +/-   ##
==========================================
- Coverage   49.56%   49.56%   -0.01%     
==========================================
  Files        2764     2764              
  Lines      208346   208351       +5     
==========================================
- Hits       103276   103269       -7     
- Misses      97419    97430      +11     
- Partials     7651     7652       +1     
Flag Coverage Δ
go-unit-tests 49.56% <69.69%> (-0.01%) ⬇️

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

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.


func makeRelease(conn *postgres.Conn, lockID int64) func() {
released := false
return func() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is probably a theoretical issue, but should we wrap this closure with a sync.Once for extra safety? Example:

var once sync.Once
return func() {
  once.Do(func() {
    ...
  })
}

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Apr 10, 2026

@johannes94: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/ocp-4-21-qa-e2e-tests 9c9a412 link false /test ocp-4-21-qa-e2e-tests

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants