Bug 14893 - Separate temporary storage per instance in Upload.pm
Summary: Separate temporary storage per instance in Upload.pm
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-25 06:48 UTC by Marcel de Rooy
Modified: 2019-06-27 09:24 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Note: All temporary uploaded files will be deleted during upgrade.
Version(s) released in:


Attachments
Bug 14893: Separate temporary storage per instance in Upload.pm (3.87 KB, patch)
2015-09-25 08:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14893: Separate temporary storage per instance in Upload.pm (4.15 KB, patch)
2016-01-18 20:43 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 14893: Separate temporary storage per instance in Upload.pm (4.21 KB, patch)
2016-01-22 16:42 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2015-09-25 06:48:30 UTC

    
Comment 1 Marcel de Rooy 2015-09-25 08:12:31 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2015-10-15 01:32:52 UTC
(In reply to Marcel de Rooy from comment #1)
> To make life easier when multiple instances are uploading temporary
> files to Koha, this patch adds the database name to the upload subfolder
> in your /tmp folder.

/tmp may empty after a reboot. I think I best do more testing to understand file flow.
Comment 3 Marcel de Rooy 2015-10-15 08:11:18 UTC
(In reply to M. Tompsett from comment #2)
> (In reply to Marcel de Rooy from comment #1)
> > To make life easier when multiple instances are uploading temporary
> > files to Koha, this patch adds the database name to the upload subfolder
> > in your /tmp folder.
> 
> /tmp may empty after a reboot. I think I best do more testing to understand
> file flow.

Not necessarily. Depends on your configuration.
On Debian, check /etc/default/rcS
# delete files in /tmp during boot older than x days.
# '0' means always, -1 or 'infinite' disables the feature
#TMPTIME=0

Note that when the OS empties the /tmp folder, this is not a problem. The files that we stored there were temporary. A future cronjob will remove all entries in the upload table that are not found anymore.
Of course, one should not set /tmp as the location for permanent storage :)
Comment 4 Marcel de Rooy 2016-01-18 14:15:25 UTC
Please help this lonely patch.
Comment 5 Marc Véron 2016-01-18 20:43:10 UTC Comment hidden (obsolete)
Comment 6 Magnus Enger 2016-01-18 22:27:12 UTC
As far as I can tell, this patch has been signed off by Marc Véron - changing status accordingly. 

I think it is also an important bugfix, not an enhancement: I have a server with multiple instances running off the 3.22.1 Debian packages. After the upgrade from 3.20.x, a couple of sites were complaining that they could no longer upload local cover images. After some investigation it turns out that Koha was trying to save uploaded files temporarily in /tmp/koha_upload/, but this directory was owned by another Koha instance, with permissions "drwxr-xr-x", thus denying all other sites the ability to upload files. A per-instance temporary upload directory should fix this.
Comment 7 Marcel de Rooy 2016-01-19 09:09:51 UTC
Thanks Marc and Magnus.
Comment 8 Jonathan Druart 2016-01-19 10:53:08 UTC
Marcel,
Why do you remove temporary uploads?
Comment 9 Marcel de Rooy 2016-01-21 13:57:00 UTC
(In reply to Jonathan Druart from comment #8)
> Marcel,
> Why do you remove temporary uploads?

Because of their temporary nature :)

When I wrote this, I had the hope to still include this in 3.22.
So deleting these files was not an issue.
But, since these files have been uploaded as temporary, I imagine that we can delete them as well at the time of an upgrade to the next release. A future cron job should do that too (e.g. after x days).

For a server with multiple instances it would be more complex. Instead of picking the files for each instance and moving them to different tmp subfolders, it is much easier to remove them. Note also that we could have permission problems here (even with deleting the folder).
Comment 10 Kyle M Hall 2016-01-22 16:42:47 UTC
Created attachment 47198 [details] [review]
[PASSED QA] Bug 14893: Separate temporary storage per instance in Upload.pm

To make life easier when multiple instances are uploading temporary
files to Koha, this patch adds the database name to the upload subfolder
in your /tmp folder.

Note: Although multiple instances could share the same subfolder for
temporary storage (hashvalue is based on a timestamp too), it will be
better to separate them for efficient housekeeping (removing older or
partial files with a cronjob etc.)
Since multiple instances come with separate permissions, keeping them in
separate folders will be much simpler.

Permanent storage is not affected by this patch. The location of permanent
storage is ruled by the upload_path in each config file. Sharing that
space is not recommended too.

Although it may not be strictly necessary yet to remove files from the old
temp storage folder (before the 3.22 release), the accompanying db rev
performs that housekeeping task.

Test plan:
[1] Do not yet apply this patch. Upload a temporary file (use the
    tools/upload.pl script without selecting a category).
[2] Check /tmp/koha_upload.
[3] Apply this patch. Run the db rev with web installer.
[4] Upload another temporary file.
[5] Check /tmp for folder [your_database]_upload.
[6] Check that /tmp/koha_upload is gone and the associated records too.
[7] Run the adjusted t/db../Upload.t

Followed test plan. Could not do steps before applying patch [1]-[2]
(I suppose a local permission problem).
After applying patch and updating db file appears in /tmp/[database]
as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Brendan Gallagher 2016-01-29 18:14:52 UTC
Pushed to Master - Should be in the May 2016 Release.  Thanks!
Comment 12 Julian Maurice 2016-02-01 16:35:36 UTC
Patch pushed to 3.22.x, will be in 3.22.3
Comment 13 Frédéric Demians 2016-02-20 07:02:42 UTC
Marcel, from my perspective, this patch is an enhancement, and as so shouldn't enter into 3.20 branch. Am I wrong?
Comment 14 Magnus Enger 2016-02-20 17:12:25 UTC
(In reply to Frédéric Demians from comment #13)
> Marcel, from my perspective, this patch is an enhancement, and as so
> shouldn't enter into 3.20 branch. Am I wrong?

To me it looks like a bugfix, see my comment #6.
Comment 15 Frédéric Demians 2016-02-21 16:04:13 UTC
This patch depends on a 3.22 enhancement which isn't available on 3.22 branch: see bug 14321. I can't backport and fix in the same time. Seems too risky. From my perspective, if it's a major issue on 3.20, a separate bug should be open, and a patch attached, specifically for 3.20.
Comment 16 Marcel de Rooy 2016-02-22 10:25:06 UTC
(In reply to Frédéric Demians from comment #15)
> This patch depends on a 3.22 enhancement which isn't available on 3.22
> branch: see bug 14321. I can't backport and fix in the same time. Seems too
> risky. From my perspective, if it's a major issue on 3.20, a separate bug
> should be open, and a patch attached, specifically for 3.20.

The development this depends on, was pushed to 3.22.
So this patch is not directly related to problems in 3.20.