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

Chore: Attempt to reduce flakiness in integration tests #97247

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

macabu
Copy link
Contributor

@macabu macabu commented Dec 2, 2024

What is this feature?

1. In integration tests, set sync=OFF if _journal_mode=WAL is set.

Why? From the SQLite docs:

With synchronous OFF (0), SQLite continues without syncing as soon as it has handed data off to the operating system. If the application running SQLite crashes, the data will be safe, but the database might become corrupted if the operating system crashes or the computer loses power before that data has been written to the disk surface. On the other hand, commits can be orders of magnitude faster with synchronous OFF.

Even though it might corrupt the database in case of OS crashes, since these are supposed to be ephemeral databases, these characteristics are not needed. The default is FULL (2).

2. Limit max_open_conn and max_idle_conn to 2. This matches the settings used in E2E tests.

Why?

It seems that limiting the max connections (especially open) improves the locking errors.

We were previously not setting the max_open_conn at all. This might make some tests slower, but I need to compare them closely.

Why do we need this feature?

This is an attempt to reduce flakiness in integration tests that are failing with database is locked errors.

Who is this feature for?

Contributors.

Which issue(s) does this PR fix?:

N/A

@macabu macabu added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Dec 2, 2024
@macabu macabu force-pushed the macabu/int-test-sqlite-settings branch from 3a68b94 to 44435a7 Compare December 2, 2024 13:36
@macabu macabu marked this pull request as ready for review December 2, 2024 14:15
@macabu macabu requested review from a team as code owners December 2, 2024 14:15
@macabu macabu requested review from suntala and DanCech and removed request for a team December 2, 2024 14:15
@github-actions github-actions bot added this to the 11.4.x milestone Dec 2, 2024
Copy link
Collaborator

@DanCech DanCech left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@JohnnyQQQQ JohnnyQQQQ left a comment

Choose a reason for hiding this comment

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

I agree that we can use it for the tests without running in synchronous mode, but from the change, it would also affect users that currently have wal enabled. We should only disable it with OFF (0) during testing, and continue to have sync on the FULL (2) setting by default, so that a host crash doesn't corrupt their data.

Many of our users rely on stable, non-ephemeral SQLite deployments in their homelabs or any smaller setup, and we should avoid unnecessarily degrading their experience.

Copy link
Member

@JohnnyQQQQ JohnnyQQQQ left a comment

Choose a reason for hiding this comment

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

NVM, just saw it's only the testdb, good work 👍

@macabu macabu merged commit dd969f8 into main Dec 10, 2024
15 checks passed
@macabu macabu deleted the macabu/int-test-sqlite-settings branch December 10, 2024 09:55
@github-actions github-actions bot modified the milestones: 11.4.x, 11.5.x Dec 10, 2024
shubhankarunhale pushed a commit to shubhankarunhale/grafana that referenced this pull request Dec 27, 2024
* sqlstore/sqlutil: set sync=OFF for sqlite in tests if wal=true

* testinfra: set max open/idle conn to 2 to match e2e tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants