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

Address redundancy re database setup #2729 #2733

Conversation

phillxnet
Copy link
Member

@phillxnet phillxnet commented Nov 6, 2023

We currently use both an initial (years old) pg_dump, & Django's native migration system to setup our initial database structure. This patch proposes that we drop the initial setup via sql.in files and normalise on using only the Django migration files. The fix purpose here is to accommodate upstream squash events such as one observed in Django-oath-toolkit, which introduced a migration conflict with our existing initial/static db setup via the old storageadmin.sql.in dump file.

Includes

  • Revised database router to ensure the existing model/db separation regarding the smart_manager app and its use of only the dedicated smart_manager database. Along with migration awareness, so that model metadata 'app_label' informs each migration appropriately: wise to the currently specified detabase during django-admin migrate runs.
  • Remove old/legacy and now redundant smartdb.sql.in & storageadmin.sql.in files, and south_migrations archive directories for both databases.
  • Move db setup/migration code in initrock script to fstrings.
  • DB migration mechanisms not associated with the initial DB setup are not functionally modified, as this is outside the scope intended for this patch.

Fixes #2729
Concerns #2723
Prerequisite of "Update django-oauth-toolkit #2710" #2727

We currently use both an initial (years old) pg_dump, &
Djangoes native migration system to setup our initial
database structure. This patch proposes that we drop the
initial setup via sql.in files and normalise on using only
the Django migration files. The fix purpose here is to
accommodate upstream squash events such as one observed
in Django-oath-toolkit, which introduced a migration
conflict with our existing initial/static db setup via
the old storageadmin.sql.in dump file.

# Includes
- Revised database router to ensure the existing model/db
separation regarding the smart_manager app and its use of
only the dedicated smart_manager database. Along with
migration awareness, so that model metadata 'app_label'
informs each migration appropriately: wise to the currently
specified detabase during `django-admin migrate` runs.
- Remove old/legacy and now redundant smartdb.sql.in &
storageadmin.sql.in files, and south_migrations archive
directories for both databases.
- Move db setup/migration code in initrock script to fstrings.
- DB migration mechanisms not associated with the initial DB
setup are not functionally modified, as this is outside the
scope intended for this patch.
@phillxnet
Copy link
Member Author

phillxnet commented Nov 6, 2023

N.B. This PR is a squash of #2732. Copying testing info from there:

Testing

Initial DB setup (no .initrock file) looks to be as intended; under the Django migrations only setup arrangement.
And our existing (no functional change) post db existence (.initrock file exists) migration complex (needs focused issue) appears to still function without error on this new migrations only instantiated DB. Note here that we currently still run through the existing-db-in-place migrations even when we have just instantiated a new database. Again - the focus of this pr is to address the initial db setup only; and once this PR and the related #2727 PR (likely post re-base) have been addressed we can do a final tidy of the whole process wise to working only with Django migration files.

These changes relate to ongoing work to update our Django-oath-toolkit, where our prior pg_dump setup method had reached it's limits and represented a blocker to #2727: hence the related nature of this PR.

On a Leap 15.5 base amd64 arch we have:

  • A successful RPM build which then installs successfully on the build host with no prior database in place: a clean install.
  • The same host with 5.0.5-0 rpm installed, the last testing rpm released in this phase, successfully updated to the rpm build against this PR.

On a Leap 15.4 base amd64 arch we have:

  • Host likewise built and installed a-fresh an RPM build against this PR.
  • A fresh install of a 4.6.1-0 stable RPM to the 5.0.5-2732 RPM built against his PR worked as intended.

@phillxnet
Copy link
Member Author

phillxnet commented Nov 6, 2023

@FroggyFlox & @Hooverdan96 Given we are getting quite the pile-up of back-log here, re my Draft PR that this issue/pr is a spin of from, and that we have partially addressed @FroggyFlox 's linked issue re initial service setup (The "Concerns #2723" one), and that this PR is also a spin-off from that issue as well: I'll go ahead and merger this one. As otherwise we end up cornering ourselves with ongoing updates/enhancements interdependencies. And this one at least looks to be a straight swap out pg_dump to Django migrate method. And then only on the initial db setup.

I'll move to a follow-up PR where we don't bother applying the exact same migrations, that we have just used to setup an initial database. But I first wanted to isolate and address just the setup of a fresh db. And to help clear the way for continued development on:

@Hooverdan96
Copy link
Member

looks like a good approach to get this off the critical path and allow you to continue with the other items you've mentioned. Excellent work in reducing the number of files we're carrying forward and moving towards a more native update/install approach that leverages the django framework more. Thanks for that!

@Hooverdan96
Copy link
Member

Hooverdan96 commented Nov 6, 2023

@phillxnet, quick question, since I noticed you did some f-string formatting in the logging messages.

Since I wanted to understand the f-string functionality better, in reading about it I came across this where the recommendation seems to be to not use f-strings for logging ...

https://realpython.com/python-f-strings/#lazy-evaluation-in-logging

not sure, whether this would apply here or not.

@phillxnet
Copy link
Member Author

@Hooverdan96 Thanks re f-strings reference. I think in our case this is not significant. We only log here once per rockstor-pre systemd service start (per boot). Overall I would prefer the use of fstrings for readability, and the performance bottleneck in our case is likely a of lesser concern. But still - good to know, especially when we log in high iteration loops. But mostly we don't do that. Curious thought.

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