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

Modify api-db and keycloak-db to support MySQL8 #3816

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

tobybellwood
Copy link
Member

@tobybellwood tobybellwood commented Oct 1, 2024

In order to support MySQL8 as well as MariaDB-based databases, a couple of schema changes are needed. These are mostly to control the size of indexes, and set enum types to be compatible.

There is no current change planned, but should a Lagoon admin wish to migrate to a cloud database for Lagoon [NOTE THAT THIS IS NOT CURRENTLY POSSIBLE], these changes will have to be in place prior to migration. Additionally, MySQL strict mode is not supported (due to Lagoon's love of zero dates).

There are a couple of local-development additions to support mysql for testing etc.

@shreddedbacon
Copy link
Member

I tweaked this a bit to leave the existing docker-compose.yaml file alone and leverage make targets with a compose override for mysql specific changes to keep the overrides isolated.

This lets you now use the defaults make up or make ui-logs-development to spin up with a mariadb and use the -mysql suffixed versions of these to run with the mysql overrides in place.

Having the ability to do both allows for verifying that not only do the migrations succeed in mysql, that they hopefully also succeed in mariadb.

@shreddedbacon
Copy link
Member

shreddedbacon commented Oct 28, 2024

The only thing I haven't tried locally is starting on a MariaDB container, then attempting to change to MySQL.

I'm going to guess this will be a real pain to script for local development since our compose stack isn't really designed with in place upgrading in mind.

#3829 does offer the ability to do stable to unstable chart upgrades that could potentially test the viability of a MariaDB pod to MySQL pod upgrade in a helm pre or post upgrade task somehow. This would be super risky though. We may have to continue to support MariaDB pod only, but ensure any future migrations are valid for MySQL.

@shreddedbacon shreddedbacon force-pushed the testing/mysql8_dbs branch 2 times, most recently from 305e944 to 56f170f Compare October 29, 2024 09:37
@shreddedbacon
Copy link
Member

shreddedbacon commented Oct 29, 2024

There might be some issues with MySQL strict mode that make this difficult to support alongside MariaDB. Mostly that the timestamp defaulting to null in strict mode defaults to null but in our MariaDB images it still sets to 0. So we need to probably go with strict mode disabled. If we had a way to completely migrate from MariaDB to MySQL then we could move to properly supporting strict mode in both the migration scripts and the API. But for now, strict mode disabled with the changes in this branch appear to be valid for both MariaDB and MySQL 8.

Luckily(?) AWS appear to have strict mode disabled by default in RDS MySQL 8. Unsure about other providers. But we could document this as a requirement when we start to think about supporting external DB providers in the charts.

@tobybellwood tobybellwood modified the milestones: 2.22.0, 2.23.0 Nov 12, 2024
@tobybellwood tobybellwood marked this pull request as ready for review December 5, 2024 05:51
@shreddedbacon
Copy link
Member

One thing to be aware of with keycloak using mysql, is that it also has to connect to the lagoon api db and uses the hardcoded LAGOON_DB_VENDOR value. We'd need to make sure that if the api-db is changed to mysql, that we have a way to pass this to the keycloak pod too. Locally you've got this https://github.com/uselagoon/lagoon/pull/3816/files#diff-abd63f9e26078ca107dc1fa168d6cd47c47f8bac403e4fa46865129f0c7804c1R17 so there would need to be associated changes to the charts to know to pass this through. I know its not really part of this pullrequest though because this is just the first stage of being able to support mysql or an external provider generally

@shreddedbacon
Copy link
Member

shreddedbacon commented Dec 10, 2024

I refactored the mysql overrides to make it simpler to switch, and also able to run through the entire test suite with mysql with the use of DATABASE_VENDOR in the Makefile. This way the image built and all the configuration options required are done during make without the need for a mysql specific docker compose file, or any other make targets.

Edit: Some minor chart changes are required to handle this switch. Just verifying the required changes to be as minimal/backwards compatible

@shreddedbacon
Copy link
Member

shreddedbacon commented Dec 10, 2024

Found some things in the upstream images that are a bit annoying and breaks the mysql image from working in tests and charts.

I modified the charts with some minimal database vendor changes and added the overrides that are in the images pull request to this branch. Ideally, the images PR would be merged and we just consume those newer images when they would be released.

I also added a password entrypoint in the api-db and keycloak-db so that the charts don't need to specify a vendor in the password, the vendoring happens in the entrypoint now. The api-db and keycloak-db chart values have vendor types defined defaulting to mariadb.

This branch has had the vendor default set to be mysql which the database migration and rollback action fails with currently. We will have to revert the default vendor back to mariadb, but it might be worth fixing the rollbacks to work with mysql too.

I just want to ensure that the entire jenkins test suite runs through with images built on mysql and I'll feel much better about it.
Edit: tests passed with mysql, thats a good sign.

@tobybellwood tobybellwood modified the milestones: 2.23.0, 2.24.0 Dec 17, 2024
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