-
Notifications
You must be signed in to change notification settings - Fork 256
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
Post: dropping MySQL #1404
Post: dropping MySQL #1404
Conversation
reads well, 👍, is there some timing expectation or can we merge as is right away? is the banner for 1.23 ready? |
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.
Fixed some grammar issues, other comments inline. Also, where is the banner for 1.23 added?
|
||
It was not easy decision, we tried hard in the past to support both databases. If you search for [PRs containing MySQL](https://github.com/theforeman/foreman/pulls?page=3&q=is%3Apr+mysql) you'll find counless workarounds, fixes and endless discussions about how to solve problems in a way that doesn't break for PostgreSQL users. There is one common theme everywhere: most developers do not use MySQL both for development and for production testing. This stems from the fact that Red Hat, a company which pays most of the users to do day-to-day development of Foreman, only have a product that is based on PostgreSQL. So we often try to reproduce things on PostgreSQL or blindly provide patches and let affected users to hotfix their instances. Also the biggest and most popular plugin called Katello does not support MySQL, so many community users are not using it at all. Although we haven't asked about it explicitly in the Foreman Community Survey yet, we believe that the majority of our users use PostgreSQL already. | ||
|
||
We don't stop here, the next target is dropping SQLite3 forcing all developers to use PostgreSQL. It's the only reasonable way we can do in order to leverage all powerful features from the database system of our choice. However, again it's not easy. In the build process, we have a part that generates what's called APIPie bindings, a stack which provides API documentation and bindings metadata. And this needs to boot up Rails, and Rails needs a database to run on and we all do this in a build chroot which does not provide environment for starting up daemon like PostgreSQL. We need to figure out how to workaround this problem, until then we still support SQLite3 for development but developers be ready to say goodbye to SQLite3 any new version. |
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.
I don't think this paragraph is relevant for our users. This is a more developer oriented issue.
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.
However is it a reason not to communicate this? My thinking it - I don't want to leave users just with simple "it will be gone", I want to be as much as transparent about our plans, goals and long term vision.
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.
Perhaps you can link to a Discourse RFC that talks about the future?
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.
Dropped it.
|
||
Along with the 1.23 release, we will provide official migration documentation, and mention it in our Release Notes as well. If you want to give the migration a test shot today, there's an existing [migration blog post](https://theforeman.org/2017/10/migrating-foreman-from-mysql-to-postgresql.html) written by Timo. We don't expect the official document be very much different. If you do such testing, please come back to us with time estimation so we can tell our users in advance what to expect. | ||
|
||
It was not easy decision, we tried hard in the past to support both databases. If you search for [PRs containing MySQL](https://github.com/theforeman/foreman/pulls?page=3&q=is%3Apr+mysql) you'll find counless workarounds, fixes and endless discussions about how to solve problems in a way that doesn't break for PostgreSQL users. There is one common theme everywhere: most developers do not use MySQL both for development and for production testing. This stems from the fact that Red Hat, a company which pays most of the users to do day-to-day development of Foreman, only have a product that is based on PostgreSQL. So we often try to reproduce things on PostgreSQL or blindly provide patches and let affected users to hotfix their instances. Also the biggest and most popular plugin called Katello does not support MySQL, so many community users are not using it at all. Although we haven't asked about it explicitly in the Foreman Community Survey yet, we believe that the majority of our users use PostgreSQL already. |
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.
Red Hat isn't the reason developers don't use MySQL, some RH developers actually do, and in any case this isn't the reason for moving away from PG. I would drop that sentence.
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.
Sure removing.
|
||
This is not an extensive list you might think. Supporting two databases must not be that hard. Truth is, it's a challenge. | ||
|
||
Those two items on the list are both relational database systems with SQL standard, the problem is that very often behavior is different. Historically, some features were implemented differently, however it's pretty clear that PostgreSQL is now much closer to the RDBM "ideal" for both developers and operations, should I say. There are dozens of things that are just not right in MySQL and we need to identify those problems, find a way to fix them and then carry extra bit of code in our codebase for life. This takes a lot of our time we could invest in features, bugfixing or performance improvements. |
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.
Each DB has it's own merits. We aren't moving to PG because it's "better" but rather because supporting just one has its benefits, and we already have plugins and other services that require it. PG also has weird problems we need to identify and fix, but having less databases to support will reduce the amount of such workarounds and will stop forcing us to always write db-agnostic code.
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.
https://about.gitlab.com/2019/06/27/removing-mysql-support/ is a similar blog about the exact same topic. One thing stood out to me:
As a side note – we don't use MySQL ourselves, and not doing so meant we weren't encountering issues BEFORE our users.
Both commercial vendors (Red Hat and ATIX) both only deploy on PostgreSQL. That means they care about proper support. MySQL always has been best effort by the community. The Foreman project wants to deliver high quality software and that's more than just 'it compiles, ship it'.
That's at least my take on this.
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.
I am rewriting the paragraph and dropping all my biased words about MySQL :-)
Added new commit with the change, please make sure to squash on merge. |
b0141a1
to
89f5365
Compare
Ping, I actually rebased once again and created new chapter 3.7 in the documentation which is based on Timos article but I added past experience with MySQL migrations we are seeing on Discourse. This is now ready. |
8b3b588
to
f416977
Compare
Squashed, rebased, added the missing file, renamed the filename to more appropriate date "1st of August". |
f416977
to
f8105e2
Compare
Forgot to add your last minute change in the "It was not an easy decition" paragraph, now it should be ready for re-review. Sorry I was unable to revive changes from reflog, I screwed git hard today locally :-( |
f8105e2
to
28d15a4
Compare
Updated the text: 1.24 is the last version with MySQL. Added information about possible major version bump 2.0 (1.25) - it's not decided yet but we can communicate this early. |
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 inline comments. @timogoebel - since the migration procedure is based on your blog post, could you please also give it a look?
_posts/2017-10-31-migrating-foreman-from-mysql-to-postgresql.md
Outdated
Show resolved
Hide resolved
**Do not wait with the migration for Foreman 1.24.** | ||
|
||
Along with the 1.23 release, we will provide official [migration | ||
documentation](/manuals/nightly/index.html#3.7MigratingtoPostgreSQL), and |
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.
documentation](/manuals/nightly/index.html#3.7MigratingtoPostgreSQL), and | |
documentation](/manuals/1.23/index.html#3.7MigratingtoPostgreSQL), and |
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.
Why not to point to the most recent version when a random user finds the post after some time? Updated instructions might be quite helpful I am pretty sure we will run into issues.
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.
because we will not have that section in the manual once we drop mysql
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.
Why would we ever remove section named "Migration from MySQL to PostgreSQL" in the future? People are far behind with upgrading, we should keep it.
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.
because we don't want to carry around stuff in the manual that isn't used anymore, if people in the future do the upgrade they will still find the instructions in the 1.23/1.24 manual
follow up thought - iiuc we explicitly skip the foreman tasks table, will the migration work for users with tasks? or will they lose all tasks information? cc @adamruzicka |
From the top of my head, if we skip all
|
How should we migrate the tasks data then? I think historic data might be fine and there shouldn't be any running tasks ideally during the upgrade, but what about the others?
This is the only one we don't need to worry about at all since katello already requires postgres |
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.
thanks for doing this, TBH IMHO we should just write a script that handles the migration (or as part of foreman maintain), it would be a lot easier to ask users to try it out vs giving them a doc.
``` | ||
|
||
The foreman user that has access to your Postgresql database and is used for | ||
the conversion temporarily needs super user privileges. |
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.
how would one create the account in the first place? traditionally this has been done by the installer.
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.
You don't need super privileges, what you really need is grant to create tables which we handle in the installer. For the migration using super user is probably fine.
overwritten. | ||
|
||
You can configure the new database as Foreman's new development database by | ||
adding a new connection in `/etc/foreman/database.yml`. |
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.
tbh I would patch the script to read the definitions from elsewhere, we can provide a simple yaml file with source/destination instead of messing up with rails settings.
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.
Well, I am not keen doing that, I can test that once or twice on my environment and I don't want this to be rather for the worse.
This should do the trick theforeman/foreman#6962 |
Maybe it's just me but if I am migrating a production instance, I want to execute every one last of commands manually to see what I am doing (and taking notes). If others think this is a good idea I can write such thing however. |
my thought process that we need to make it easy and reliable, those people who care (like you) would want to see the procedure, how it works, and handle it step by step, others, would just want to get it running as easily as possible. I think there is a lot of things that can go wrong in this process, having a script that is tested and improved over time, would make it more reliable than docs... my worry is that if its a hard procedure, people will simply not migrate... btw you can still do a script that goes step by step and ask for confirmation of each step (or skip)... |
Can I do that in foreman maintain @mbacovsky ? |
Forget about that @mbacovsky we can't use foreman-maintain as it is not on Debian. |
Added bunch of commits, squash during merge. There are still few open questions tho. |
migration of dynflow tables. | ||
|
||
```sh | ||
foreman-rake dynflow:migrate |
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.
Isn't this step done as part of the migration script now? Thinking of which, should we also run the regular migration task as part of it? (my main though is this is a very long process, the fewer manual steps the user has to do the better)
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.
I don't think so. More than that, it's not yet merged: theforeman/foreman#6962
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.
That will be merged once @adamruzicka gets back to the comments.
My thought is that these tasks should be idempotent, so we can call them as part of the prod2dev flow to make sure the target db is in the right state. if we do that, we can make these instruction simpler for the users - so less chance of missing a step causing the next step to fail.
adjusted manually. | ||
|
||
```sh | ||
forman-rake sequence:reset RAILS_ENV=development |
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.
Let's call this as part of the convert script as well and not bother the user with another manual step
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.
I disagree, there is no such thing as the convert script. We do have db:convert:prod2dev
which is a generic production to development convert script - your target database don't need to be PostgreSQL at all. I want to stick with least surprise, thus an explicit step is better. It's also better when calling for help - it's more obvious at which point something failed or what to do.
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.
Whenever prod2dev is called and the target db is postgres we would need to call this task, which is a noop anyways when the db isn't posgres.
Users shouldn't need to care about database internals - they want to migrate from one db to the other. If this fails we'll be able to tell from the error what fails, and if they care about what the internals of the task does they can read it.
**Do not wait with the migration for Foreman 1.24.** | ||
|
||
Along with the 1.23 release, we will provide official [migration | ||
documentation](/manuals/nightly/index.html#3.7MigratingtoPostgreSQL), and |
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.
because we don't want to carry around stuff in the manual that isn't used anymore, if people in the future do the upgrade they will still find the instructions in the 1.23/1.24 manual
Pushed bunch of changes as new commits. |
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.
Almost there - I think the remaining issue is we disagree regarding having the convert script call other tasks to simplify the process. I would like users to have a simple path to migrate, the more complicated it is the more chance some of them will miss steps or just stick to 1.24 forever.
The only other minor comment is that there is some inconsistent capitalization of PosgreSQL - some places use Postgresql
instead.
migration of dynflow tables. | ||
|
||
```sh | ||
foreman-rake dynflow:migrate |
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.
That will be merged once @adamruzicka gets back to the comments.
My thought is that these tasks should be idempotent, so we can call them as part of the prod2dev flow to make sure the target db is in the right state. if we do that, we can make these instruction simpler for the users - so less chance of missing a step causing the next step to fail.
adjusted manually. | ||
|
||
```sh | ||
forman-rake sequence:reset RAILS_ENV=development |
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.
Whenever prod2dev is called and the target db is postgres we would need to call this task, which is a noop anyways when the db isn't posgres.
Users shouldn't need to care about database internals - they want to migrate from one db to the other. If this fails we'll be able to tell from the error what fails, and if they care about what the internals of the task does they can read it.
Pushing the PostgreSQL capitalization. I'd be fine with simplified workflow as long as we keep the current |
That works for me. lets make a |
Let me do this in theforeman/foreman#6974 then. It's related. |
Integrated the new wrapper task. |
need to run all of Foreman's database migrations. | ||
|
||
```sh | ||
foreman-rake db:migrate RAILS_ENV=development |
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.
can we run this also inside the mysql2pgsql
task?
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.
This can be dangerous - the db migration must be performed in the development Rails env. Now, the mysql2pgsql wrapper task today contains only conversion task which is Rails environment independent. However I am going to add another task which resets postgresql sequence and that already requires RAILS_ENV to be set correctly (production). If we merge the db:migrate
here, there is a conflict in Rails environments.
It could be solved indeed but that can be confusing when adding more and more tasks into the wrapper one.
Any other blockers @tbrisker other than the PRs to be merged? |
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.
This looks good now, but I prefer to finish merging the related PRs first to make sure we don't have a mismatch between the instructions here and what actually needs to be done.
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.
Core is in, two small changes to this and also please copy the changes from nightly to the 1.23 manual as well.
ec4db53
to
fc26042
Compare
All done, ping me if something's wrong! Cheers. |
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.
Thanks @lzap !
Do not comment on grammar or typos, please checkout and push changes into my branch. If there are many comments I will ask you to do so :-)
Thanks.