Page MenuHomeSoftware Heritage
Differential D3197

swh.deposit.models: Upload deposit archives to dedicated folder
ClosedPublic
Actions

Authored by ardumont on May 29 2020, 2:31 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T2393: Two deposits of files the same name and a "compound" extension fails the checker
Commits
rDDEPc586ff174027: swh.deposit.models: Upload deposit archives to dedicated folder
Summary

This allows to avoid the clash names when depositing multiple archives with the
same name.

This serves the same purpose as file disambiguation needed to solve the issue.
In a, most possibly, simpler manner than changing altogether the current file
storage implementation.

The folder name is a timestamp to the microsecond. So it might not be completely
bulletproof though.

Related to T2393
Related to D3192

Test Plan

tox

Diff Detail

Repository
rDDEP Push deposit
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12568
Build 19092: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 19091: arc lint + arc unit

Event Timeline

ardumont created this revision.May 29 2020, 2:31 PM
ardumont mentioned this in D3192: Disambiguate uploaded files with a prefix instead of a suffix..May 29 2020, 2:32 PM
swh-public-ci added a comment.May 29 2020, 2:34 PM

Build is green

Patch application report for D3197 (id=11354)

Rebasing onto 4b4cebc477...

Current branch diff-target is up to date.
Changes applied before test
commit 366ac7aa2fe6cc909b6176008d5dd3dd2d90311d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri May 29 14:27:07 2020 +0200

    swh.deposit.models: Upload deposit archives to dedicated folder
    
    This allows to avoid the clash names when depositing multiple archives with the
    same name.
    
    This serves the same purpose as file disambiguation needed to solve the issue.
    In a most possibly simpler manner than changing altogether the current file
    storage.
    
    The folder name is a timestamp to the second. So it might not be completely
    bulletproof though.
    
    Related to T2393
    Related to D3192

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/38/ for more details.

Harbormaster completed remote builds in B12552: Diff 11354.May 29 2020, 2:34 PM
vlorentz accepted this revision.May 29 2020, 3:42 PM

No test?

This revision is now accepted and ready to land.May 29 2020, 3:42 PM
vlorentz requested changes to this revision.May 29 2020, 3:44 PM

I think you should change check_archive to detect Django's name changes (see D3192)

This revision now requires changes to proceed.May 29 2020, 3:44 PM
ardumont added a comment.EditedMay 29 2020, 3:47 PM

No test?

It's a proposal for now so no test indeed (+ other tests pass here already).

Also, i figure there would need for some migration.

vlorentz added a comment.May 29 2020, 3:48 PM

The proposal is good imo. Maybe add milliseconds just to be sure though, in case the client is scripting multiple uploads at the same time

ardumont added a comment.EditedMay 29 2020, 3:52 PM

The proposal is good imo. Maybe add milliseconds just to be sure though, in
case the client is scripting multiple uploads at the same time

Unfortunately, I did not find the millisecond stanza in the strftime
documentation [1] the django docs [2] refers to.

Thus the "not bulletproof" solution part in the description.

[1] https://docs.python.org/3/library/time.html#time.strftime

[2] https://docs.djangoproject.com/en/2.2/ref/models/fields/#django.db.models.FileField.upload_to

vlorentz added a comment.May 29 2020, 3:56 PM

Or you could just use a random value every time.

Oh also, since we'll be creating a lot of directories with this, we need something to remove them. (Or switch to prefixing a random string to the file name)

ardumont added a comment.May 29 2020, 4:07 PM

Another documentation mentions %f for micro-seconds... which seems to work:

In [1]: from datetime import datetime

In [2]: from time import strftime

In [3]: datetime.now().strftime("%Y%m%d-%H%M%S.%f")
Out[3]: '20200529-160744.270565'
ardumont added a comment.EditedMay 29 2020, 4:12 PM

Oh also, since we'll be creating a lot of directories with this, we need
something to remove them. (Or switch to prefixing a random string to the file
name)

I'm not sure I follow.

We do not clean those up, we store them. A reference to the archive path is
kept in the model (db: DepositRequest).

Thus, why i'm mentioning that your initial proposal could be the right
direction in the long run [2]

[2] https://forge.softwareheritage.org/D3192#78011

ardumont added a comment.May 29 2020, 4:26 PM

Also, i figure there would need for some migration.

We do not clean those up, they stay and a reference is kept to them in the
deposit db (as far as i understood django on that part).

Hey, this means no migration (I thought of migrating from the old values to the
new but no need). The old archive path will target the old files so we should
be good.

ardumont updated this revision to Diff 11370.EditedMay 29 2020, 4:30 PM
  • Use f-string, it's clearer
  • Add ".%f" to go to the microseconds

TODO:

  • Need to check where to add assertions around the change (it's already covered by other tests since it's blue in the diff)
swh-public-ci added a comment.May 29 2020, 4:33 PM

Build is green

Patch application report for D3197 (id=11370)

Rebasing onto 4b4cebc477...

Current branch diff-target is up to date.
Changes applied before test
commit 9835b8ba367b9c8c2fe5f6949e652b51cdec8a51
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri May 29 14:27:07 2020 +0200

    swh.deposit.models: Upload deposit archives to dedicated folder
    
    This allows to avoid the clash names when depositing multiple archives with the
    same name.
    
    This serves the same purpose as file disambiguation needed to solve the issue.
    In a most possibly simpler manner than changing altogether the current file
    storage.
    
    The folder name is a timestamp to the second. So it might not be completely
    bulletproof though.
    
    Related to T2393
    Related to D3192

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/39/ for more details.

Harbormaster completed remote builds in B12568: Diff 11370.May 29 2020, 4:33 PM
ardumont edited the summary of this revision. (Show Details)Jun 1 2020, 11:05 AM
ardumont added inline comments.Jun 1 2020, 11:25 AM
swh/deposit/models.py
176

lol, pdb-ing this, that does not work...
The strftime thing described in the docs must work if we don't use a callable.

Nonetheless, we can do this ourselves...

reception_date = instance.deposit.reception_date
folder = reception_date.strftime('%Y%m%d-%H%M%S.%f')
return f"client_{instance.deposit.client.id}/{folder}/{filename}"

which I found clearer in the end.

ardumont added inline comments.Jun 1 2020, 11:38 AM
swh/deposit/models.py
176

Now we are talking ;)

'client_1/20200601-092624.421886/archive1.zip'

Note: Also I was uneasy about the %f which was not in the official doc, so now i know it's working \m/

ardumont updated this revision to Diff 11374.EditedJun 1 2020, 11:42 AM
  • Add tests
  • Improve implementation

Need to check where to add assertions around the change (it's already covered
by other tests since it's blue in the diff)

done

swh-public-ci added a comment.Jun 1 2020, 11:45 AM

Build is green

Patch application report for D3197 (id=11374)

Rebasing onto 4b4cebc477...

Current branch diff-target is up to date.
Changes applied before test
commit c586ff17402790e5de44aa59d69092fb35cd28b7
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri May 29 14:27:07 2020 +0200

    swh.deposit.models: Upload deposit archives to dedicated folder
    
    This allows to avoid the clash names when depositing multiple archives with the
    same name.
    
    The folder name is a timestamp to the microsecond so there should no longer be
    any clash (famous last work and all that).
    
    Related to T2393
    Related to D3192

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/40/ for more details.

Harbormaster completed remote builds in B12572: Diff 11374.Jun 1 2020, 11:45 AM
ardumont mentioned this in T2393: Two deposits of files the same name and a "compound" extension fails the checker.Jun 1 2020, 11:51 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 1 2020, 12:25 PM
Closed by commit rDDEPc586ff174027: swh.deposit.models: Upload deposit archives to dedicated folder (authored by ardumont). · Explain Why
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDDEPc586ff174027: swh.deposit.models: Upload deposit archives to dedicated folder.
vlorentz added a comment.Jun 2 2020, 9:33 AM
In D3197#78047, @ardumont wrote:

We do not clean those up, we store them. A reference to the archive path is
kept in the model (db: DepositRequest).

We do both. Django uploads files to /tmp, which we copy to a permanent location and clean.

ardumont added a comment.Jun 2 2020, 9:35 AM

We do both. Django uploads files to /tmp, which we copy to a permanent location and clean.

Ack. So it's missing a clean up routine somewhere (around the temporary location)?

ardumont added a comment.Jun 2 2020, 12:43 PM

We do both. Django uploads files to /tmp, which we copy to a permanent location and clean.

Ack. So it's missing a clean up routine somewhere (around the temporary location)?

Everything is fine. Checked the deposit server, no dangling files in /tmp/swh-deposit/archive.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
deposit/
14 lines81%
Loading...

Diff 11370

View Options

swh/deposit/models.py

Loading...
About · Developer information · Legal