Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(waku_archive): only allow a single instance to execute migrations #2736

Merged
merged 1 commit into from
May 31, 2024

Conversation

richard-ramos
Copy link
Member

Description

Uses an advisory lock which acts as a mutex for the postgresql migrations.
This lock will be acquired before any migration is executed. If it's not possible to acquire, the instance will fail to start with these errors:

ERR 2024-05-27 17:59:38.155-04:00 failed to acquire lock                     topics="waku archive migration" tid=479763 file=migrations.nim:77 error="another waku instance is currently executing a migration"
ERR 2024-05-27 17:59:38.155-04:00 Mounting protocols failed                  tid=479763 file=node_factory.nim:435 error="failed to setup archive driver: ArchiveDriver build failed in migration: failed to lock the db"

Once the migration process is done, the lock will be released.

This is required for scenarios in which a postgresql database is used by more than one node (like in status).

cc: @jakubgs

Issue

closes #2726

Copy link

This PR may contain changes to database schema of one of the drivers.

If you are introducing any changes to the schema, make sure the upgrade from the latest release to this change passes without any errors/issues.

Please make sure the label release-notes is added to make sure upgrade instructions properly highlight this change.

@richard-ramos richard-ramos added the release-notes Issue/PR needs to be evaluated for inclusion in release notes highlights or upgrade instructions label May 27, 2024
Copy link

github-actions bot commented May 27, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2736-rln-v1

Built from 5d20b65

Copy link

github-actions bot commented May 27, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2736-rln-v2

Built from 5d20b65

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for it! 💯

Comment on lines 860 to 870
let lockedRes = await s.getStr(
fmt"""
SELECT pg_try_advisory_lock({lockId})
"""
)
if lockedRes.isErr():
return err("error acquiring a lock: " & $lockedRes.error)

if lockedRes.value == "f":
return err("another waku instance is currently executing a migration")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it could be a little bit more concise with the valueOr approach ( notice that we also use isOkOr for Result[void, ...] )

Suggested change
let lockedRes = await s.getStr(
fmt"""
SELECT pg_try_advisory_lock({lockId})
"""
)
if lockedRes.isErr():
return err("error acquiring a lock: " & $lockedRes.error)
if lockedRes.value == "f":
return err("another waku instance is currently executing a migration")
let locked = (await s.getStr(
fmt"""
SELECT pg_try_advisory_lock({lockId})
"""
)).valueOr:
return err("error acquiring a lock: " & $error)
if locked == "f":
return err("another waku instance is currently executing a migration")

Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Thanks so much! 🤩

@richard-ramos richard-ramos merged commit 88b8e18 into master May 31, 2024
13 of 15 checks passed
@richard-ramos richard-ramos deleted the dbLock branch May 31, 2024 16:08
Copy link
Contributor

@jakubgs jakubgs left a comment

Choose a reason for hiding this comment

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

Nice, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Issue/PR needs to be evaluated for inclusion in release notes highlights or upgrade instructions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: migrations must have an status and check against it before a migration is executed
4 participants