It appears that Koha cannot access /tmp on Stretch. We should standardize on each Koha instance having it's own TMPDIR in /var/lib/koha/<instance>/tmp to avoid this as well as avoiding possible file name collisions between instances.
If it is confirmed, it's blocker for 18.05
(In reply to Jonathan Druart from comment #1) > If it is confirmed, it's blocker for 18.05 To give a bit more detail, we had two Koha servers that were upgraded to Stretch. Both servers exhibited this behavior. The file upload would work just fine, the files would show up in /tmp as expected, but then Koha was unable to 'see' those files. In the code, if you printed a list of files in /tmp, Koha would see the tmp dir as empty! It's like to Koha, /tmp was a write-only filesystem!
Would this be caused by systemd PrivateTmp? https://www.maxoberberger.net/blog/2017/10/debian-9-private-tmp.html
(In reply to paxed from comment #3) > Would this be caused by systemd PrivateTmp? > > https://www.maxoberberger.net/blog/2017/10/debian-9-private-tmp.html That's the most probable cause! But the solution should be what Kyle said: creating a per-instance temp dir and adjusting the cleanup script to deal with it too.
Created attachment 73426 [details] [review] Bug 20428: Add the option to specify a tmp uploads dir
Created attachment 73427 [details] [review] Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files
Created attachment 73428 [details] [review] Bug 20428: Make about.pl inform about missing upload_tmp_path entry
Created attachment 73429 [details] [review] Bug 20428: Add the option to specify a tmp uploads dir This patch adds an option to the koha-conf.xml file for specifying a temporary uploaded files directory. The koha-create script is adjusted to handle it and a convenient option switch is added. If ommited, it will default to /var/lib/koha/<instance>/uploads_tmp. koha-create-dirs is patched to create the required directory with the right permissions. The docs get the new parameter documented.
Created attachment 73430 [details] [review] Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files This patch makes Koha::UploadedFile->temporary_directory try to use the new configuration entry. It will fallback to File::Spec->tmpdir otherwise.
Created attachment 73431 [details] [review] Bug 20428: Make about.pl inform about missing upload_tmp_path entry This patch makes about.pl warn about a missing upload_tmp_path entry in koha-conf.xml. It also mentions the effectively used tmp dir.
*** Bug 19898 has been marked as a duplicate of this bug. ***
Comment on attachment 73430 [details] [review] Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files Review of attachment 73430 [details] [review]: ----------------------------------------------------------------- ::: Koha/UploadedFile.pm @@ +169,5 @@ > sub temporary_directory { > my ( $class ) = @_; > + return (C4::Context->config('upload_tmp_path')) > + ? C4::Context->config('upload_tmp_path') > + : File::Spec->tmpdir; Why a trinary when a || File::Spec->tmpdir would work more nicely?
Created attachment 75159 [details] [review] Bug 20428: Add the option to specify a tmp uploads dir This patch adds an option to the koha-conf.xml file for specifying a temporary uploaded files directory. The koha-create script is adjusted to handle it and a convenient option switch is added. If ommited, it will default to /var/lib/koha/<instance>/uploads_tmp. koha-create-dirs is patched to create the required directory with the right permissions. The docs get the new parameter documented.
Created attachment 75160 [details] [review] Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files This patch makes Koha::UploadedFile->temporary_directory try to use the new configuration entry. It will fallback to File::Spec->tmpdir otherwise.
Created attachment 75161 [details] [review] Bug 20428: Make about.pl inform about missing upload_tmp_path entry This patch makes about.pl warn about a missing upload_tmp_path entry in koha-conf.xml. It also mentions the effectively used tmp dir.
Created attachment 75162 [details] [review] Bug 20428: Add the option to specify a tmp uploads dir This patch adds an option to the koha-conf.xml file for specifying a temporary uploaded files directory. The koha-create script is adjusted to handle it and a convenient option switch is added. If ommited, it will default to /var/lib/koha/<instance>/uploads_tmp. koha-create-dirs is patched to create the required directory with the right permissions. The docs get the new parameter documented. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 75163 [details] [review] Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files This patch makes Koha::UploadedFile->temporary_directory try to use the new configuration entry. It will fallback to File::Spec->tmpdir otherwise. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 75164 [details] [review] Bug 20428: Make about.pl inform about missing upload_tmp_path entry This patch makes about.pl warn about a missing upload_tmp_path entry in koha-conf.xml. It also mentions the effectively used tmp dir. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
No access to /tmp? This will probably affect much more than upload? What about CGI sessions? Several cron jobs ? We should also look for hardcoded /tmp's in the codebase.. etc. Or make sure that koha-instance has enough permissions at install/upgrade ?
Created attachment 75169 [details] [review] Bug 20428: Add the option to specify a tmp uploads dir This patch adds an option to the koha-conf.xml file for specifying a temporary uploaded files directory. The koha-create script is adjusted to handle it and a convenient option switch is added. If ommited, it will default to /var/lib/koha/<instance>/uploads_tmp. koha-create-dirs is patched to create the required directory with the right permissions. The docs get the new parameter documented. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 75170 [details] [review] Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files This patch makes Koha::UploadedFile->temporary_directory try to use the new configuration entry. It will fallback to File::Spec->tmpdir otherwise. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 75171 [details] [review] Bug 20428: Make about.pl inform about missing upload_tmp_path entry This patch makes about.pl warn about a missing upload_tmp_path entry in koha-conf.xml. It also mentions the effectively used tmp dir. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 75172 [details] [review] Bug 20428: Added missing parameter --upload-tmp-path Line 463 lists valid parameters, but it was missing upload-tmp-path: which resulted in the koha-create saying it is an invalid parameter. TEST PLAN --------- 1) apply all patches but this. 2) sudo perl ~/misc4dev/cp_debian_files.pl 3) restart_all 4) sudo koha-create --create-db --upload-tmp-path unique-path awesome-test-name -- Fails to run. 5) apply this patch 6) repeat 2-4 7) sudo vi /etc/koha/sites/awesome-test-name/koha-conf.xml -- the upload_tmp_path entry should have unique-path in it.
I'm going for lunch, but I'll have to test this on Stretch for a MARC import to be sure. Changing back to needs signoff.
This is PQA.
(In reply to Marcel de Rooy from comment #19) > No access to /tmp? This will probably affect much more than upload? > What about CGI sessions? Several cron jobs ? > We should also look for hardcoded /tmp's in the codebase.. > etc. > > Or make sure that koha-instance has enough permissions at install/upgrade ? You are right about this being a broader problem for Stretch users. Kyle filed bug 20727 to have that conversation as this one only deals with file uploads.
(In reply to Tomás Cohen Arazi from comment #26) > (In reply to Marcel de Rooy from comment #19) > > No access to /tmp? This will probably affect much more than upload? > > What about CGI sessions? Several cron jobs ? > > We should also look for hardcoded /tmp's in the codebase.. > > etc. > > > > Or make sure that koha-instance has enough permissions at install/upgrade ? > > You are right about this being a broader problem for Stretch users. Kyle > filed bug 20727 to have that conversation as this one only deals with file > uploads. I think koha-conf should contain an entry for an alternative tmp directory. Upload should then create a subfolder there. But the config var should be more general and not be upload-tmp-path..
(In reply to Marcel de Rooy from comment #27) > (In reply to Tomás Cohen Arazi from comment #26) > > (In reply to Marcel de Rooy from comment #19) > > > No access to /tmp? This will probably affect much more than upload? > > > What about CGI sessions? Several cron jobs ? > > > We should also look for hardcoded /tmp's in the codebase.. > > > etc. > > > > > > Or make sure that koha-instance has enough permissions at install/upgrade ? > > > > You are right about this being a broader problem for Stretch users. Kyle > > filed bug 20727 to have that conversation as this one only deals with file > > uploads. > > I think koha-conf should contain an entry for an alternative tmp directory. > Upload should then create a subfolder there. > But the config var should be more general and not be upload-tmp-path.. I agree. Will try to provide a sensible follow-up.
Created attachment 75275 [details] [review] Bug 20428: Make upload_tmp a more general tmp directory Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 75279 [details] [review] Bug 20428: Add the option to specify a tmp uploads dir This patch adds an option to the koha-conf.xml file for specifying a temporary uploaded files directory. The koha-create script is adjusted to handle it and a convenient option switch is added. If ommited, it will default to /var/lib/koha/<instance>/uploads_tmp. koha-create-dirs is patched to create the required directory with the right permissions. The docs get the new parameter documented. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 75280 [details] [review] Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files This patch makes Koha::UploadedFile->temporary_directory try to use the new configuration entry. It will fallback to File::Spec->tmpdir otherwise. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 75281 [details] [review] Bug 20428: Make about.pl inform about missing upload_tmp_path entry This patch makes about.pl warn about a missing upload_tmp_path entry in koha-conf.xml. It also mentions the effectively used tmp dir. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 75282 [details] [review] Bug 20428: Added missing parameter --upload-tmp-path Line 463 lists valid parameters, but it was missing upload-tmp-path: which resulted in the koha-create saying it is an invalid parameter. TEST PLAN --------- 1) apply all patches but this. 2) sudo perl ~/misc4dev/cp_debian_files.pl 3) restart_all 4) sudo koha-create --create-db --upload-tmp-path unique-path awesome-test-name -- Fails to run. 5) apply this patch 6) repeat 2-4 7) sudo vi /etc/koha/sites/awesome-test-name/koha-conf.xml -- the upload_tmp_path entry should have unique-path in it.
Created attachment 75283 [details] [review] Bug 20428: Make upload_tmp a more general tmp directory Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 75286 [details] [review] Bug 20428: Make upload_tmp a more general tmp directory Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Marcel: can you review the last changes?
(In reply to Tomás Cohen Arazi from comment #36) > Marcel: can you review the last changes? QA: Looking here now
dev-koha@dev:/usr/share/koha/devclone$ ll /var/lib/koha/dev/tmp/uploads/ drwxr-xr-x 2 dev-koha dev-koha 4096 May 14 08:56 koha_dev_upload Removing this additional level. In an enhancement we can remove the database name from the main upload folder in tmp since it is instance bound now.
Created attachment 75293 [details] [review] Bug 20428: Add the option to specify a tmp uploads dir This patch adds an option to the koha-conf.xml file for specifying a temporary uploaded files directory. The koha-create script is adjusted to handle it and a convenient option switch is added. If ommited, it will default to /var/lib/koha/<instance>/uploads_tmp. koha-create-dirs is patched to create the required directory with the right permissions. The docs get the new parameter documented. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 75294 [details] [review] Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files This patch makes Koha::UploadedFile->temporary_directory try to use the new configuration entry. It will fallback to File::Spec->tmpdir otherwise. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 75295 [details] [review] Bug 20428: Make about.pl inform about missing upload_tmp_path entry This patch makes about.pl warn about a missing upload_tmp_path entry in koha-conf.xml. It also mentions the effectively used tmp dir. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 75296 [details] [review] Bug 20428: Added missing parameter --upload-tmp-path Line 463 lists valid parameters, but it was missing upload-tmp-path: which resulted in the koha-create saying it is an invalid parameter. TEST PLAN --------- 1) apply all patches but this. 2) sudo perl ~/misc4dev/cp_debian_files.pl 3) restart_all 4) sudo koha-create --create-db --upload-tmp-path unique-path awesome-test-name -- Fails to run. 5) apply this patch 6) repeat 2-4 7) sudo vi /etc/koha/sites/awesome-test-name/koha-conf.xml -- the upload_tmp_path entry should have unique-path in it. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 75297 [details] [review] Bug 20428: Make upload_tmp a more general tmp directory Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 75298 [details] [review] Bug 20428: (QA follow-up) Remove redundant directory level Upload creates its own upload folder in the temp folder, so there is no need to add another level in temporary_directory. Removing the creation of this folder in koha-create-dirs too. Also removing the use Koha::UploadedFiles in about.pl. No longer needed. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
I would recommend release notes and a db rev warning to alert people to move existing installs in this direction.
(In reply to Marcel de Rooy from comment #45) > I would recommend release notes and a db rev warning to alert people to move > existing installs in this direction. Maybe on the about page?
(In reply to Marcel de Rooy from comment #45) > I would recommend release notes and a db rev warning to alert people to move > existing installs in this direction. I agree there should be an IMPORTANT: notice in the release notes for people in Stretch. And I think it is not the only one on this release.
(In reply to Jonathan Druart from comment #46) > (In reply to Marcel de Rooy from comment #45) > > I would recommend release notes and a db rev warning to alert people to move > > existing installs in this direction. > > Maybe on the about page? Yeah, there is an about warning already on the new tmp config var. A print warning from updatedatabase would be extra.
Created attachment 75305 [details] [review] Bug 20428: Display warning if the entry is empty And not only if the entry is missing. We should do the same for other entries. Also, use Koha::UploadedFile->temporary_directory to know the fallback value.
(In reply to Marcel de Rooy from comment #48) > (In reply to Jonathan Druart from comment #46) > > (In reply to Marcel de Rooy from comment #45) > > > I would recommend release notes and a db rev warning to alert people to move > > > existing installs in this direction. > > > > Maybe on the about page? > > Yeah, there is an about warning already on the new tmp config var. A print > warning from updatedatabase would be extra. I do not think it is necessary.
Pushed to master for 18.05, thanks to everybody involved!
PHP uses parameter "upload_tmp_dir", usually set at php.ini or webserver site profile. Doesn't Perl have some similar to not make necessary to check filesystem paths and permissions? I have a Koha 17.11 running in Debian 9 (Stretch) without problem about uploads. How does Koha 17.11 deal with this?
(In reply to Narcis Garcia from comment #52) > PHP uses parameter "upload_tmp_dir", usually set at php.ini or webserver > site profile. Doesn't Perl have some similar to not make necessary to check > filesystem paths and permissions? > > I have a Koha 17.11 running in Debian 9 (Stretch) without problem about > uploads. How does Koha 17.11 deal with this? Did you see comment 4?
Is "template_cache_dir" parameter affected by this issue? I have Koha 17.11 running on Debian 9 without this directive. Should I enable it when following performance recommendations?
(In reply to Narcis Garcia from comment #54) > Is "template_cache_dir" parameter affected by this issue? No > I have Koha 17.11 running on Debian 9 without this directive. Should I > enable it when following performance recommendations? Yes
*** Bug 19676 has been marked as a duplicate of this bug. ***
(In reply to Narcis Garcia from comment #52) > PHP uses parameter "upload_tmp_dir", usually set at php.ini or webserver > site profile. Doesn't Perl have some similar to not make necessary to check > filesystem paths and permissions? > > I have a Koha 17.11 running in Debian 9 (Stretch) without problem about > uploads. How does Koha 17.11 deal with this? fyi: i've hit this problem on deb9 with 17.11.6 + plack
(In reply to Jonathan Druart from comment #51) > Pushed to master for 18.05, thanks to everybody involved! hi Frido, would you accept this patchset for the 17.11 release?