-
-
Notifications
You must be signed in to change notification settings - Fork 291
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
Recent update broke sqlite volume mapping ? #1813
Comments
The usage of sqlite for anything other that quickly testing Shlink, is discouraged. Please don't use SQLite in production, or try to mount it via docker volumes, as it has known issues and cannot be done reliably. See these links for more context: |
The setup worked perfectly fine for over 2 years, If you don't want users to use sqlite why not remove it in the first place. Switching to mysql I get the exact same issue, so seems to be more of a generic problem.
|
Have you checked if it does actually work? Shlink retries those commands until the database server is available, and it looks it worked on the third try. I will take a look though, as the output can be a bit alarming for no reason.
It has many use cases, like giving Shlink a quick try, or different automations. Just not production. |
Sorry I cut the log, since it just repeats over and over with restart=unless-stopped, tries to autorecover. I tried multiple version now and 3.5.4 works just fine, 3.6.0 stops working for me. I switched now to mysql with 3.5.4 |
Can you open a new issue reporting this? It should work with MySQL just fine. Run it with |
Yesterday I debugged my own instance, where I also use MySQL and I probably didn't notice because the downtime is not too high. Just a couple of seconds. I tried to run it with I suspect this problem has always been there, but Shlink 3.6.0 has changed how the initial commands are run, and now prints some errors that were explicitly muted before. This should be possible to verify by running older versions with This also probably relates with the fact that Shlink should actively wait for the database to be available (with a reasonable timeout), before starting to "fail", but that is tricky. I created an issue a couple of years ago around this, but I never got to do it. So the course of action here is:
|
Just another comment because this also broke our URL shortener out of nowhere. I do not see a valid reason why shlink would not work fine with a local sqlite database even if it isn't "recommended". The database works totally fine if not mounted as a single fine but instead by mounting the whole data folder. That is, it used to work fine, until the new version of the container starts running the init script everytime. The issue you get is the following:
So it basically tries to create tables even though they already exist. This is bad. Even if you don't want to support sqlite for more than quick testing this essentially means that you can't "restart" test databases even if it tries to actively create tables every time. I'd like to add that breakage like this is bad for minor version releases. Even if it isn't the "intended" deployment scenario the fact that it works out of the box like this when using the docker container makes it kind of the default deployment scenario, in which case upgrades shouldn't just break it. For now we've rolled back to 3.5.4. There also doesn't seem to be a standard migration path from sqlite to any other database, not that large of a problem in our case but annoying nevertheless. |
@Blackclaws can you try if v3.6.0 is also affected by this? I suspect this regression might be caused by #1413, which was introduced in v3.6.1 |
Just tried it and you are correct. 3.6.0 works fine while 3.6.1 is broken. |
My guess is that this might be the culprit: shlink/module/CLI/src/Command/Db/CreateDatabaseCommand.php Lines 81 to 86 in 6558c37
Wherein as far as I understand it for sqlite no tables are returned as existing. |
An easy way to test if its fixed:
This should work fine. It currently doesn't. |
I have just released Shlink 3.6.3, which fixes the regression introduced in v3.6.1 while trying to fix another bug. The docker images will be available as soon as this is done https://github.com/shlinkio/shlink/actions/runs/5269669996 That said, a couple of comments for you @Blackclaws First of all, I would encourage you to be less aggressive and more respectful when addressing an open source maintainer who's work you are taking advantage of FOR FREE.
As said, this was an unintentional regression, so please, be more empathetic.
There is: https://shlink.io/documentation/advanced/import-short-urls/#shlink-instance The link above is the only result when searching for "database migration" in the docs.
The issues I shared some comments above explain what kind of problems you can face when doing this, and when those problems arise and you loose production data, you will blame me. I'm protecting your future you (and me) from yourself.
You are using a combination that is not officially supported for production. The fact that something is not supported doesn't mean it does not work, it means it is not actively tested, and if it stops working, there's no warranty it will be fixed. I probably did a poor job documenting and stating that SQLite is not officially supported for production environments. Even less combined with docker volumes. From now on you know you are using an unsupported scenario. Proceed at your own risk. |
How Shlink is set up
Summary
After upgrading shlink it now fails to start with a sqlite volume mapping.
I have watchtower updating automatically every 7 days, so the image from last week should still work.
I tried to recreate the container with a fresh database but I get the same error.
logs:
How to reproduce
The text was updated successfully, but these errors were encountered: