-
Notifications
You must be signed in to change notification settings - Fork 998
feat: auto-downgrading revision #4678
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
base: main
Are you sure you want to change the base?
feat: auto-downgrading revision #4678
Conversation
@EnotShow is attempting to deploy a commit to the KeepHQ Team on Vercel. A member of the Team first needs to authorize it. |
hey @EnotShow - thanks for this PR! as this PR introduces some major changes to how Keep works, I thought I would add some e2e tests for it (mainly install Keep, do some migration, then downgrade). wdyt? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4678 +/- ##
==========================================
- Coverage 46.19% 46.03% -0.16%
==========================================
Files 165 165
Lines 17495 17564 +69
==========================================
+ Hits 8081 8086 +5
- Misses 9414 9478 +64 β View full report in Codecov by Sentry. π New features to boost your workflow:
|
β¦EnotShow/keep into feature/auto-downgrade-revision
Deployment failed with the following error:
|
β¦EnotShow/keep into feature/auto-downgrade-revision
Hey @shahargl! Thx for your review! I made all the required changes, so it's ready for re-review :) |
@EnotShow let tests pass and if they will I'll review |
Signed-off-by: Ihor Korolenko <[email protected]>
π¨ BugBot couldn't runPull requests from forked repositories are not yet supported (requestId: serverGenReqId_48ddba26-49ed-4dcf-a98c-b6051afed68d). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Migration Copying Fails on Missing Directories
The copy_migrations
function has several flaws: it attempts to list directory contents (local_migrations_path
and source_versions_path
) without ensuring their existence, which can lead to FileNotFoundError
if os.makedirs
fails or source_versions_path
is missing. Furthermore, its cleanup logic for local_migrations_path
only removes files and symlinks, leaving stale subdirectories that can cause conflicts during subsequent migration copying.
keep/api/core/db_on_start.py#L183-L204
keep/keep/api/core/db_on_start.py
Lines 183 to 204 in 13fe761
# Ensure destination exists | |
try: | |
os.makedirs(local_migrations_path, exist_ok=True) | |
except Exception as e: | |
logger.error(f"Failed to create local migrations folder with error: {e}") | |
# Clear previous versioned migrations to ensure only migrations relevant to the current version are present | |
for filename in os.listdir(local_migrations_path): | |
file_path = os.path.join(local_migrations_path, filename) | |
if os.path.isfile(file_path) or os.path.islink(file_path): | |
os.remove(file_path) | |
# Alembic needs the full migration history to safely perform a downgrade to earlier versions | |
# Copy new migrations | |
for item in os.listdir(source_versions_path): | |
src = os.path.join(source_versions_path, item) | |
dst = os.path.join(local_migrations_path, item) | |
if os.path.isdir(src): | |
shutil.copytree(src, dst, dirs_exist_ok=True) | |
else: | |
shutil.copy(src, dst) |
Bug: Migration Path Naming Mismatch
Environment variable MIGRATION_PATH
(singular) is used in the Docker Compose files for Postgres, but the application expects MIGRATIONS_PATH
(plural). This inconsistency prevents the configured /tmp/migrations
path from being recognized, causing the default /tmp/keep/migrations
to be used instead.
tests/e2e_tests/docker-compose-e2e-postgres-migrations.yml#L40-L41
keep/tests/e2e_tests/docker-compose-e2e-postgres-migrations.yml
Lines 40 to 41 in 13fe761
- SECRET_MANAGER_DIRECTORY=/app | |
- MIGRATION_PATH=/tmp/migrations |
tests/e2e_tests/docker-compose-e2e-postgres.yml#L41-L42
keep/tests/e2e_tests/docker-compose-e2e-postgres.yml
Lines 41 to 42 in 13fe761
- SECRET_MANAGER_DIRECTORY=/app | |
- MIGRATION_PATH=/tmp/migrations |
tests/e2e_tests/docker-compose-e2e-mysql.yml#L44-L45
keep/tests/e2e_tests/docker-compose-e2e-mysql.yml
Lines 44 to 45 in 13fe761
- SECRET_MANAGER_DIRECTORY=/app | |
- MIGRATIONS_PATH=/tmp/migrations |
Bug: Database Migration Errors and Backup Failures
The migrate_db
function has two issues:
- It incorrectly assumes any
alembic.command.upgrade
failure implies a database rollback, leading to an unwarranted downgrade attempt. Upgrade failures can stem from various causes (e.g., connection issues, permissions, migration errors), and attempting a downgrade in such cases is incorrect and risks data loss. - If the database schema is already up-to-date, the function returns early without calling
copy_migrations
. This prevents the current version's migration files from being backed up locally, making it impossible to downgrade to this specific version in the future.
keep/api/core/db_on_start.py#L268-L286
keep/keep/api/core/db_on_start.py
Lines 268 to 286 in 13fe761
# If the current revision is the same as the expected revision, we don't need to run migrations | |
if current_revision and expected_revision and current_revision == expected_revision: | |
logger.info("Database schema is up-to-date!") | |
return None | |
logger.warning(f"Database schema ({current_revision}) doesn't match application version ({expected_revision})") | |
logger.info("Running migrations...") | |
try: | |
alembic.command.upgrade(config, "head") | |
except Exception as e: | |
logger.error(f"{e} it's seems like Keep was rolled back to a previous version") | |
if not os.getenv("ALLOW_DB_DOWNGRADE", "false") == "true": | |
logger.error(f"ALLOW_DB_DOWNGRADE is not set to true, but the database schema ({current_revision}) doesn't match application version ({expected_revision})") | |
raise RuntimeError("Database downgrade is not allowed") | |
logger.info("Downgrading database schema...") | |
downgrade_db(config, expected_revision, local_migrations_path, app_migrations_path) |
Bug: Missing Step ID Causes Workflow Failure
The workflow attempts to reference steps.get_revision.outputs.revision
, but the step intended to produce this output lacks the id: get_revision
.
.github/workflows/run-migrations-e2e-tests.yml#L334-L336
keep/.github/workflows/run-migrations-e2e-tests.yml
Lines 334 to 336 in 13fe761
echo "Tests completed!" | |
env: | |
WORKFLOW_REVISION: ${{ steps.get_revision.outputs.revision }} |
.github/workflows/run-migrations-e2e-tests.yml#L478-L480
keep/.github/workflows/run-migrations-e2e-tests.yml
Lines 478 to 480 in 13fe761
poetry run pytest -v tests/e2e_tests/test_end_to_end_db_auth.py -n 4 --dist=loadfile | |
env: | |
WORKFLOW_REVISION: ${{ steps.get_revision.outputs.revision }} |
Was this report helpful? Give feedback by reacting with π or π
Closes #4665
π Description
This PR introduces automatic database downgrading support when rolling back KeepHQ to a previous version.
β Checks
βΉ Additional Information