Created attachment 19284 [details] patch koha-restore kohe-restore fails to create mysqluser@mysql_hostname so zebra update fails. It also attempts to drop the mysqluser which does not exist causing an error. Please find attached an updated version which corrects these problems.
Hi Robin, could you maybe take a look at this?
I don't think removing the instruction to remove the user if it exists is a good idea, as it might be there. And it it is there, then we want to remove it. As it stands, the error coming from that failing should be ignored and the script should proceed. If that's not happening, that's a bug in and of itself (though looking at it, it should be OK.) As for adding the permission for @$mysql_hostname, I wonder if that should be fixed to 'localhost' like it is in koha-create. This is to account for connecting via either a UNIX socket or a loopback TCP connection within a machine. For situations where the mysql server is on a remote host, we're probably reaching the point of complexity where koha-restore isn't suitable in it's current form. So anyway, I think it should probably be modified such that mysql_hostname is always 'localhost', and that'll solve the zebra mysql issue. Any thoughts on this, John?
I think that as the koha-remove script drops the user and if you are restoring to a new install the user should not exist, the existence of an existing user is an error that should be flagged. As for the @$mysql_hostname I originaly had is as a static 'localhost' but as the hostname is available for the koha-conf.xml file I thought the use of this value would future proof the script. Feel free to change it back to a static if you think this is more appropriate.
any chance a new patch could be added in a diff/patch format? https://en.wikipedia.org/wiki/Diff_utility
We need to decide whether clobbering (overwriting) an instance is acceptable, and stick with that. If it is, then no error should be shown, and the user and database should be removed and re-created (or just updated). If it isn't, then the script should check everything at the very beginning, and stop before making ANY changes. Another option is to give koha-restore an "--overwrite" flag to choose between these behaviours. Even then, we need to decide on a default. I'm keen to improve/rewrite the script, but will leave the decision on default behaviour to the Koha experts.
Created attachment 89529 [details] [review] Use "DROP USER IF EXISTS" and remove " || true" Anyway, if removing "ERROR 1396 (HY000) at line 1: Operation DROP USER failed for 'koha_rsc'@'%'" is all you're after, this patch fixes it. "DROP USER IF EXISTS" has been available since MySQL 5.7, which is old by now.
Formatting this as a patch might be a good 'rescue' task :)
Hi Katrin, I don't understand your last message. To whom was it directed?
The attached file is not a properly formatted patch file and can't be processed with our tools - I set the "Academy" keyword the same time as the comment. It's a keyword we use for easy bugs and tasks that are good for people starting out developing on Koha.
There is more than one attachment. The one from 2013 is not a patch. My one from 2019-05-10 is a patch created with "diff". Which one are you referring to?
HI Rudolf, yours actually :) - diff doesn't create a patch file that can be used with our tools. Have a look at git format --patch and https://wiki.koha-community.org/wiki/Commit_messages. If you compare to patch files from other bugs you will see yours is missing the header line with authorship etc. Also missing bug reference (subject line), description, test plan etc. So this needs a little more work. Please let us know if we can help - I was not sure if you were still around and just trying to encourage someone to maybe pick this up to finish. Usually in January we have a group of students tackle things, but it won't happen this year :(
... oh and status should be set to "Needs Sign-off" once the patch is fixed up and Assignee should be you, if you intent continuing work on it.
> used with our tools. Have a look at git format --patch and git format-patch
OK, let me be your student then, and try the things you suggested. (BTW, I also noticed that my patch was reversed. :facepalm: )
Created attachment 96975 [details] [review] Use `DROP USER IF EXISTS` and remove ` || true`. All right, here is my attempt at a proper patch!
Sorry, I meant to get to this earlier. I'm afraid we cannot yet use the 'IF EXISTS' syntax here as we are maintaining support for Jessie up to the end of it's LTS date (2020-06-30). I intend to drop support for that Debian version at the release of 20.05 (at which time I believe we should also drop support for it from 19.11 at 19.11.06 and 19.05 at 19.05.12). However, even once we have dropped such support, unless I am mistaken, stretch is still stuck at MySQL 5.5 (though it recommends MariaDB instead. As such, we'll need a clear deprecation of mysql 5.5 support outlined and publicised before we can make this change. I am, however, pretty keen to take such an approach and drop 5.5 support with this release and so this patch is 'on hold' for now.
For some reason comment 15 doesn't have the test plan. But it turns out the patch has a test plan. Here it is, to remove confusion and help discussing it: Subject: [PATCH] Bug 10517: Use `DROP USER IF EXISTS` and remove ` || true`. When restoring a Koha instance, and the database user does not exist, an error is shown, although it is prevented from stopping the script by ` || true`. This patch removes the ` || true` guard, and rather uses `DROP USER IF EXISTS` which does exactly what one would expect, without confusing error messages, whether the user existed or not. To test: 1) Assume your Koha instance is called `asdf`. 1) Export the Koha instance using `koha-dump`. 2) Remove the instance completely using `koha-remove`. 3) Import it again using `koha-restore`. 4) No error should be shown, and the import should work. There was NO user to drop, therefore `DROP USER IF EXISTS` did nothing. 5) A database user called `koha_asdf` should exist, and have all privileges on the database of the same name. 6) Remove the instance again using `koha-remove`. 7) Now create a database user in MySQL called `koha_instance` where instance is the name of your dumped koha instance. 8) Import your koha instance again using `koha-restore`. 9) Again, no error should be shown, and the import should work. There WAS a user to drop, therefore `DROP USER IF EXISTS` dropped the user before creating a new one. 10) Again, a database user called `koha_asdf` should exist, and have all privileges on the database of the same name.
> 2) Remove the instance completely using `koha-remove`. I'm having an issue here. Maybe it's my dev environment. root@4dfd0fcc251b:kohadevbox$ koha-remove kohadev Removing Koha instance kohadev [ ok ] Stopping Plack daemon for kohadev:. userdel: user kohadev-koha is currently used by process 1005 /usr/sbin/deluser: `/usr/sbin/userdel kohadev-koha' returned error code 8. Exiting. Using koha-disable doesn't help. There are still starman and apache processes running that are from the unix user of the instance. Could there be anything more to do before removing the instance?
(In reply to Martin Renvoize from comment #16) > I'm afraid we cannot yet use the 'IF EXISTS' syntax here as we are > maintaining support for Jessie up to the end of it's LTS date (2020-06-30). > I intend to drop support for that Debian version at the release of 20.05 (at > which time I believe we should also drop support for it from 19.11 at > 19.11.06 and 19.05 at 19.05.12). > > However, even once we have dropped such support, unless I am mistaken, > stretch is still stuck at MySQL 5.5 (though it recommends MariaDB instead. > As such, we'll need a clear deprecation of mysql 5.5 support outlined and > publicised before we can make this change. > > I am, however, pretty keen to take such an approach and drop 5.5 support > with this release and so this patch is 'on hold' for now. Is there an existing place like in the wiki that contains these info? Nothing more than you said, the current situation and future plans.
(In reply to Martin Renvoize from comment #16) > > I am, however, pretty keen to take such an approach and drop 5.5 support > with this release and so this patch is 'on hold' for now. For what it's worth, I think koha-remove uses "DROP USER IF EXISTS", so we already have mixed support for 5.5. I ended up removing "IF EXISTS" and updating koha-remove to use --force so that it could survive fatal errors.
(In reply to Victor Grousset/tuxayo from comment #18) > > 2) Remove the instance completely using `koha-remove`. > > I'm having an issue here. Maybe it's my dev environment. > > root@4dfd0fcc251b:kohadevbox$ koha-remove kohadev > Removing Koha instance kohadev > [ ok ] Stopping Plack daemon for kohadev:. > userdel: user kohadev-koha is currently used by process 1005 > /usr/sbin/deluser: `/usr/sbin/userdel kohadev-koha' returned error code 8. > Exiting. > > > > > Using koha-disable doesn't help. There are still starman and apache > processes running that are from the unix user of the instance. > > Could there be anything more to do before removing the instance? Probably your dev environment. I've never had this issue, and I'm using my own patch. Did you check which process is still using the user using `ps -u kohadev-koha`? By the way: Is the test plan supposed to be in the commit message, in a comment, or both?
(In reply to David Cook from comment #20) > (In reply to Martin Renvoize from comment #16) > > > > I am, however, pretty keen to take such an approach and drop 5.5 support > > with this release and so this patch is 'on hold' for now. > > For what it's worth, I think koha-remove uses "DROP USER IF EXISTS", so we > already have mixed support for 5.5. > > I ended up removing "IF EXISTS" and updating koha-remove to use --force so > that it could survive fatal errors. Are you talking about the `--force` option on the MySQL command line utility? That smells like technical debt. Using it is the same or worse as using `|| true`, since you will hide lots of potential errors. We only need to handle one very specific error (which is not even an error, if you consider Idempotency to be a good programming pattern for such scripts – which it is). Using a catch-all to hide all errors is almost always a Bad Idea (TM).
(In reply to Rudolf Byker from comment #21) > By the way: Is the test plan supposed to be in the commit message, in a > comment, or both? It shoudl be in the commit message - in bugzilla it will then appear automatically in a comment, if you use git bz. See here: https://wiki.koha-community.org/wiki/Commit_messages
(In reply to Rudolf Byker from comment #22) > (In reply to David Cook from comment #20) > > (In reply to Martin Renvoize from comment #16) > > > > > > I am, however, pretty keen to take such an approach and drop 5.5 support > > > with this release and so this patch is 'on hold' for now. > > > > For what it's worth, I think koha-remove uses "DROP USER IF EXISTS", so we > > already have mixed support for 5.5. > > > > I ended up removing "IF EXISTS" and updating koha-remove to use --force so > > that it could survive fatal errors. > > Are you talking about the `--force` option on the MySQL command line > utility? That smells like technical debt. Using it is the same or worse as > using `|| true`, since you will hide lots of potential errors. We only need > to handle one very specific error (which is not even an error, if you > consider Idempotency to be a good programming pattern for such scripts – > which it is). Using a catch-all to hide all errors is almost always a Bad > Idea (TM). Yes, the --force option for the MySQL cli utility. I am only suggesting it as a temporary measure. Versions of Debian we support are using MySQL server 5.5, but some Koha Debian utilities (the ones that are aiming at being idempotent) fail on those Debian versions. Either we remove the idempotency targeting newer versions of MySQL server, or we do a measure like --force (which will still warn so it doesn't hide the errors it just doesn't die on them). We really need to do one or the other. Or drop support for older versions of Debian I suppose.
OK, if you really need to support older versions, let's do it cleanly. Here is a way to check if the user exists: EXISTS=$(mysql -uroot -p -s -e "select count(*) from mysql.user where user = 'asdf'" | tail -n 1) if [[ $EXISTS -gt 0 ]] then echo "Now delete the user." fi
(In reply to Katrin Fischer from comment #23) > in bugzilla it will then appear > automatically in a comment, if you use git bz. @Rudolf that's the cause I think. Your commit seems fine but if you don't use git-bz[1] you have to manually copy the commit message in the comment created when uploading an attachment. [1] https://wiki.koha-community.org/wiki/Git_bz_configuration
(In reply to Rudolf Byker from comment #21) > Probably your dev environment. I've never had this issue, and I'm using my > own patch. Did you check which process is still using the user using `ps -u > kohadev-koha`? starman and apache
(In reply to Rudolf Byker from comment #25) > OK, if you really need to support older versions, let's do it cleanly. Here > is a way to check if the user exists: > > EXISTS=$(mysql -uroot -p -s -e "select count(*) from mysql.user where user = > 'asdf'" | tail -n 1) > if [[ $EXISTS -gt 0 ]] > then > echo "Now delete the user." > fi Sounds like a plan to me, although I think it would be better to create a new function in koha-functions.sh, since we need to drop the user in koha-restore as well as koha-remove. I think we batch too many SQL statements together for these CLI utilities. But, I'm not planning on doing that work anytime soon. So don't feel obligated to listen to that. Ultimately what I was trying to say before was that koha-remove already uses the DROP USER IF EXISTS syntax, so it seems fair to use it in koha-restore too, even if it causes problems for people using older DB versions, since that problem already exists in the master codebase.
(In reply to Rudolf Byker from comment #25) > OK, if you really need to support older versions, let's do it cleanly. By the way, Martin Renvoize is the release manager and Katrin Fischer is the QA manager, so ultimately they're the ones who decide what gets pushed. I'm just a fellow contributor. Don't feel like you need to listen to me. They're the ones to listen to. You may also find contributing easier if you're using git-bz (https://wiki.koha-community.org/wiki/Git_bz_configuration) or even the community's Docker testing images which comes bundled with git-bz (https://gitlab.com/koha-community/koha-testing-docker). I notice you submitted a patch last year as well. Good to see you back here!
> You may also find contributing easier if you're using git-bz +1 > Docker testing images which comes bundled with git-bz +1 @Rudolf come to IRC if you any question setting those up.
(In reply to Rudolf Byker from comment #21) > Probably your dev environment. I've never had this issue, and I'm using my > own patch. Can you confirm that one should be able to do the following - spin a fresh dev env (who provides the command koha-remove) on master, without any patch - koha-remove MY_INSTANCE - and not have the following error: userdel: user kohadev-koha is currently used by process 864 /usr/sbin/deluser: `/usr/sbin/userdel kohadev-koha' returned error code 8. Exiting. Here are the running process before koha-remove https://copycat.drycat.fr/?c9032e785372c590#4Kc3ir7uygVpWD67jz2huvguZbgWKy5QkYTmhYVPYu1z after https://copycat.drycat.fr/?e42e97345c1dc5c4#tgEsEof33SSxREncJRmtq6XV93QnpcBtmhynUkcYrBL
== 1 == I might had issues related to bug 25482 because my dev env was Debian 10 (koha-testing-docker ku-md10) Which could explain cause koha-plack to not work which would cause issue to stop the instance. Actually only Apache processes are still there when doing koha-remove. With koha-disable, no errors and starman and Apache processes are still there. == 2 == So I tried with Debian 9 + MySQL 5.5 (koha-testing-docker ku) And now I get this issue. > root@b92acf73a243:kohadevbox$ koha-remove kohadev > Removing Koha instance kohadev > ERROR 1064 (42000) at line 1: You have an error in your SQL syntax; > check the manual that corresponds to your MySQL server version for > the right syntax to use near 'IF EXISTS `koha_kohadev`@`%`' at line 1 == 3 == So I tried with Debian 9 + MariaDB (from it's repos) (koha-testing-docker ku-md9) Same error as with Debian 10. Processes still here. > userdel: user kohadev-koha is currently used by process 839 > /usr/sbin/deluser: `/usr/sbin/userdel kohadev-koha' returned error code 8. > Exiting.
(In reply to Martin Renvoize from comment #16) > I'm afraid we cannot yet use the 'IF EXISTS' syntax here as we are > maintaining support for Jessie up to the end of it's LTS date (2020-06-30). > I intend to drop support for that Debian version at the release of 20.05 (at > which time I believe we should also drop support for it from 19.11 at > 19.11.06 and 19.05 at 19.05.12). > > However, even once we have dropped such support, unless I am mistaken, > stretch is still stuck at MySQL 5.5 (though it recommends MariaDB instead. > As such, we'll need a clear deprecation of mysql 5.5 support outlined and > publicised before we can make this change. My second test of the above comment seems to show that MySQL 5.5 is already not supported in our case. I'm just spinning up a Koha with MySQL 5.5 and run koha-remove then it fails on a syntax error mentioning IF EXISTS. So, should we still avoid IF EXISTS?(I don't have the big picture) In this case it doesn't look like it will break anything new for anyone. "Just" breaking a second thing when using koha-remove.
Right.. time has moved on... I'm thinking we can accept this patch now as-is? Promoting to the QA queue
(In reply to Rudolf Byker from comment #15) > Created attachment 96975 [details] [review] [review] > Use `DROP USER IF EXISTS` and remove ` || true`. > > All right, here is my attempt at a proper patch! The author name is missing, it just has your username I guess: "dolf".
(In reply to Joonas Kylmälä from comment #35) > (In reply to Rudolf Byker from comment #15) > > Created attachment 96975 [details] [review] [review] [review] > > Use `DROP USER IF EXISTS` and remove ` || true`. > > > > All right, here is my attempt at a proper patch! > > The author name is missing, it just has your username I guess: "dolf". I don't understand what you mean here.
Hi, it's about the patch file, see: From 06fd450e1f9749e8fa1d6455e32f597ac0ef85b4 Mon Sep 17 00:00:00 2001 From: dolf <rudolfbyker@gmail.com> Date: Wed, 8 Jan 2020 11:20:02 +0200 Subject: [PATCH] Bug 10517: Use `DROP USER IF EXISTS` and remove ` || true`. This means that you will show up as "dolf" in some places. We do have mappings to make things show nicer/full names, but always easier to make sure it has the information you want to display. I think you might want to run: git config --global user.name "firstname lastname" I believe if you don't want to resubmit, we could also fix this as part of QA, but you should tell us what it should read like. Rudolf Byker?
> git config --global user.name "firstname lastname" I'll keep this in mind for the future. > we could also fix this as part of QA That would be great, thanks. > but you should tell us what it should read like. Rudolf Byker? Exactly :)
Created attachment 127865 [details] [review] Bug 10517: Use `DROP USER IF EXISTS` and remove ` || true`. When restoring a Koha instance, and the database user does not exist, an error is shown, although it is prevented from stopping the script by ` || true`. This patch removes the ` || true` guard, and rather uses `DROP USER IF EXISTS` which does exactly what one would expect, without confusing error messages, whether the user existed or not. To test: 1) Assume your Koha instance is called `asdf`. 1) Export the Koha instance using `koha-dump`. 2) Remove the instance completely using `koha-remove`. 3) Import it again using `koha-restore`. 4) No error should be shown, and the import should work. There was NO user to drop, therefore `DROP USER IF EXISTS` did nothing. 5) A database user called `koha_asdf` should exist, and have all privileges on the database of the same name. 6) Remove the instance again using `koha-remove`. 7) Now create a database user in MySQL called `koha_instance` where instance is the name of your dumped koha instance. 8) Import your koha instance again using `koha-restore`. 9) Again, no error should be shown, and the import should work. There WAS a user to drop, therefore `DROP USER IF EXISTS` dropped the user before creating a new one. 10) Again, a database user called `koha_asdf` should exist, and have all privileges on the database of the same name. Sponsored-by: Reformational Study Centre
Martin, are we missing your sign-off line here? I think you were the one to switch to signed-off. Fixed the author line.
(In reply to Katrin Fischer from comment #40) > Martin, are we missing your sign-off line here? I think you were the one to > switch to signed-off. > > Fixed the author line. Ping?
Yes, it should have my SO line.. oops
Created attachment 133758 [details] [review] Bug 10517: Use `DROP USER IF EXISTS` and remove ` || true`. When restoring a Koha instance, and the database user does not exist, an error is shown, although it is prevented from stopping the script by ` || true`. This patch removes the ` || true` guard, and rather uses `DROP USER IF EXISTS` which does exactly what one would expect, without confusing error messages, whether the user existed or not. To test: 1) Assume your Koha instance is called `asdf`. 1) Export the Koha instance using `koha-dump`. 2) Remove the instance completely using `koha-remove`. 3) Import it again using `koha-restore`. 4) No error should be shown, and the import should work. There was NO user to drop, therefore `DROP USER IF EXISTS` did nothing. 5) A database user called `koha_asdf` should exist, and have all privileges on the database of the same name. 6) Remove the instance again using `koha-remove`. 7) Now create a database user in MySQL called `koha_instance` where instance is the name of your dumped koha instance. 8) Import your koha instance again using `koha-restore`. 9) Again, no error should be shown, and the import should work. There WAS a user to drop, therefore `DROP USER IF EXISTS` dropped the user before creating a new one. 10) Again, a database user called `koha_asdf` should exist, and have all privileges on the database of the same name. Sponsored-by: Reformational Study Centre Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 134353 [details] [review] Bug 10517: Use `DROP USER IF EXISTS` and remove ` || true`. When restoring a Koha instance, and the database user does not exist, an error is shown, although it is prevented from stopping the script by ` || true`. This patch removes the ` || true` guard, and rather uses `DROP USER IF EXISTS` which does exactly what one would expect, without confusing error messages, whether the user existed or not. To test: 1) Assume your Koha instance is called `asdf`. 1) Export the Koha instance using `koha-dump`. 2) Remove the instance completely using `koha-remove`. 3) Import it again using `koha-restore`. 4) No error should be shown, and the import should work. There was NO user to drop, therefore `DROP USER IF EXISTS` did nothing. 5) A database user called `koha_asdf` should exist, and have all privileges on the database of the same name. 6) Remove the instance again using `koha-remove`. 7) Now create a database user in MySQL called `koha_instance` where instance is the name of your dumped koha instance. 8) Import your koha instance again using `koha-restore`. 9) Again, no error should be shown, and the import should work. There WAS a user to drop, therefore `DROP USER IF EXISTS` dropped the user before creating a new one. 10) Again, a database user called `koha_asdf` should exist, and have all privileges on the database of the same name. Sponsored-by: Reformational Study Centre Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 134354 [details] [review] Bug 10517: (QA follow-up) No need to remove second true This is just theoretical, but by removing the second true you could end the script on another error. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Pushed to 21.11.x for 21.11.06
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.
*** Bug 29009 has been marked as a duplicate of this bug. ***