Page MenuHomeSoftware Heritage
Differential D8587

{Content|Directory}Loader: Adapt support for checksums
ClosedPublic
Actions

Authored by ardumont on Sep 30 2022, 3:16 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T3781: Replace the Nixguix loader with a lister
Commits
rDLDBASE39c33a66c27c: {Content|Directory}Loader: Adapt support for checksums
Summary

This moves away the loaders from the 'integrity' fields which is now dealt with lister
side. Instead the lister is send a dictionary of hex hashes.

This also improves the loader to check those checksums when retrieving the
artifact (content or tarball). Thanks to a bump in the swh.model version, this is now
able to deal with sha512 checksums checks as well.

This echoes with the ongoing work for the package loaders [1].

Related to T3781
Depends on D8584

[1] D8595

Diff Detail

Repository
rDLDBASE Generic VCS/Package Loader
Branch
refactor-nixguix-loader-directory
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 31985
Build 50068: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 50067: arc lint + arc unit

Event Timeline

ardumont created this revision.Sep 30 2022, 3:16 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptSep 30 2022, 3:16 PM
swh-public-ci added a comment.Sep 30 2022, 3:20 PM

Build is green

Patch application report for D8587 (id=31000)

Could not rebase; Attempt merge onto f774aba59e...

Updating f774aba..1672fed
Fast-forward
 swh/loader/core/loader.py                          | 212 +++++++++++++++++----
 .../https_example.org/archives_dummy-hello.tar.gz  | Bin 0 -> 221 bytes
 swh/loader/core/tests/test_loader.py               | 130 ++++++++++++-
 3 files changed, 297 insertions(+), 45 deletions(-)
 create mode 100644 swh/loader/core/tests/data/https_example.org/archives_dummy-hello.tar.gz
Changes applied before test
commit 1672fed607576399dcd1aec67a452606a6427fe6
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 30 15:10:49 2022 +0200

    ContentLoader: Fix integrity check
    
    Related to T3781

commit 497f74f3225e4ccf11adce0d6a2bb50b2a471fab
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 30 11:54:13 2022 +0200

    Add Directory Loader to allow tarball ingestion as Directory
    
    In some marginal listing cases (Nix or Guix for now), we can receive raw tarball to
    ingest. This commit adds a loader to ingest those. The output of the ingestion is a
    snapshot with 1 branch, one HEAD branch targetting the ingested directory (contained
    within the tarball).
    
    This expects to receive a mandatory 'integrity' field. It is used to check the tarball
    received out of the origin.
    
    This can also optionally receive a list of mirror urls in case the main origin url is no
    longer available. Those mirror urls are solely used as fallback to retrieve the tarball.
    
    Related to T3781

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

Harbormaster completed remote builds in B31971: Diff 31000.Sep 30 2022, 3:20 PM
ardumont requested review of this revision.Sep 30 2022, 3:20 PM
vlorentz added a subscriber: vlorentz.Sep 30 2022, 3:55 PM
vlorentz added inline comments.
swh/loader/core/loader.py
718

why is self.expected_checksum hexadecimal bytes?

ardumont added inline comments.Sep 30 2022, 4:00 PM
swh/loader/core/loader.py
718

good question...

That's coming from the instruction line 677 which was the only way i saw that matched types:

self.expected_checksum: bytes = base64.decodebytes(checksum_value_b64.encode())
ardumont added inline comments.Sep 30 2022, 5:01 PM
swh/loader/core/loader.py
718

I must have made a mistake during my hash computation switch when i changed to a simpler content file (than the original asdf one).
Currently fixing it.

ardumont retitled this revision from ContentLoader: Fix integrity check to ContentLoader: Improve integrity check support.Sep 30 2022, 5:04 PM
ardumont edited the summary of this revision. (Show Details)
ardumont updated this revision to Diff 31015.Sep 30 2022, 5:05 PM
  • Build content out of MultiHash
  • Add support for sha512 hash
  • Fix input test checksum
ardumont marked an inline comment as done.Sep 30 2022, 5:05 PM
swh-public-ci added a comment.Sep 30 2022, 5:08 PM

Build is green

Patch application report for D8587 (id=31015)

Could not rebase; Attempt merge onto f774aba59e...

Updating f774aba..3083a3b
Fast-forward
 requirements-swh.txt                               |   2 +-
 swh/loader/core/loader.py                          | 221 +++++++++++++++++----
 .../https_example.org/archives_dummy-hello.tar.gz  | Bin 0 -> 221 bytes
 swh/loader/core/tests/test_loader.py               | 182 +++++++++++++++--
 4 files changed, 350 insertions(+), 55 deletions(-)
 create mode 100644 swh/loader/core/tests/data/https_example.org/archives_dummy-hello.tar.gz
Changes applied before test
commit 3083a3b23fa118158a3e1bf54087492086603e63
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 30 15:10:49 2022 +0200

    ContentLoader: Improve integrity check support
    
    This can deal with sha512 with the new swh.model version.
    
    Related to T3781

commit 497f74f3225e4ccf11adce0d6a2bb50b2a471fab
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 30 11:54:13 2022 +0200

    Add Directory Loader to allow tarball ingestion as Directory
    
    In some marginal listing cases (Nix or Guix for now), we can receive raw tarball to
    ingest. This commit adds a loader to ingest those. The output of the ingestion is a
    snapshot with 1 branch, one HEAD branch targetting the ingested directory (contained
    within the tarball).
    
    This expects to receive a mandatory 'integrity' field. It is used to check the tarball
    received out of the origin.
    
    This can also optionally receive a list of mirror urls in case the main origin url is no
    longer available. Those mirror urls are solely used as fallback to retrieve the tarball.
    
    Related to T3781

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

Harbormaster completed remote builds in B31985: Diff 31015.Sep 30 2022, 5:08 PM
ardumont mentioned this in T3781: Replace the Nixguix loader with a lister.Sep 30 2022, 5:16 PM
ardumont updated this revision to Diff 31027.Oct 2 2022, 11:35 AM

Adapt both Content and Directory loaders to deal with checksums dict instead of
integrity.

ardumont retitled this revision from ContentLoader: Improve integrity check support to {Content|Directory}Loader: Adapt support for checksums.Oct 2 2022, 11:36 AM
ardumont edited the summary of this revision. (Show Details)
ardumont mentioned this in D8584: Add Directory Loader to allow tarball ingestion as Directory.
swh-public-ci added a comment.Oct 2 2022, 11:38 AM

Build has FAILED

Patch application report for D8587 (id=31027)

Could not rebase; Attempt merge onto f774aba59e...

Updating f774aba..da7cc37
Fast-forward
 requirements-swh.txt                               |   2 +-
 swh/loader/core/loader.py                          | 234 +++++++++++++++++----
 .../https_example.org/archives_dummy-hello.tar.gz  | Bin 0 -> 221 bytes
 swh/loader/core/tests/test_loader.py               | 194 +++++++++++++++--
 4 files changed, 372 insertions(+), 58 deletions(-)
 create mode 100644 swh/loader/core/tests/data/https_example.org/archives_dummy-hello.tar.gz
Changes applied before test
commit da7cc372fe5a9cdf139de7cf51a2c6cb6dc8b8ed
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 30 15:10:49 2022 +0200

    {Content|Directory}Loader: Adapt support for checksums
    
    This moves away the loaders from the 'integrity' fields which is now dealt with lister
    side. Instead the lister is send a dictionary of hex hashes.
    
    This also improves the loader to check those checksums when retrieving the
    artifact (content or tarball). Thanks to a bump in the swh.model version, this is now
    able to deal with sha512 checksums checks as well.
    
    Related to T3781

commit 497f74f3225e4ccf11adce0d6a2bb50b2a471fab
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 30 11:54:13 2022 +0200

    Add Directory Loader to allow tarball ingestion as Directory
    
    In some marginal listing cases (Nix or Guix for now), we can receive raw tarball to
    ingest. This commit adds a loader to ingest those. The output of the ingestion is a
    snapshot with 1 branch, one HEAD branch targetting the ingested directory (contained
    within the tarball).
    
    This expects to receive a mandatory 'integrity' field. It is used to check the tarball
    received out of the origin.
    
    This can also optionally receive a list of mirror urls in case the main origin url is no
    longer available. Those mirror urls are solely used as fallback to retrieve the tarball.
    
    Related to T3781

Link to build: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/937/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/937/console

Harbormaster failed remote builds in B31997: Diff 31027!Oct 2 2022, 11:38 AM
ardumont added a comment.EditedOct 2 2022, 11:40 AM

Build has FAILED

Build error unrelated to this diff [1] locally ok with my old tox generation [2]
Maybe a new upstream version bump on importlib-metadata?

[1]

...
importlib-metadata==5.0.0
...
11:37:49    File "/var/lib/jenkins/workspace/DLDBASE/tests-on-diff/.tox/py3/lib/python3.7/site-packages/kombu/utils/compat.py", line 82, in entrypoints
11:37:49      for ep in importlib_metadata.entry_points().get(namespace, [])
11:37:49  AttributeError: 'EntryPoints' object has no attribute 'get'

[2]

$ tox
...
importlib-metadata==4.12.0
...
================================================================ 272 passed, 122 warnings in 62.62s (0:01:02) ================================================================
swh-public-ci added a comment.Oct 2 2022, 11:47 AM

Build has FAILED

Patch application report for D8587 (id=31027)

Could not rebase; Attempt merge onto f774aba59e...

Updating f774aba..da7cc37
Fast-forward
 requirements-swh.txt                               |   2 +-
 swh/loader/core/loader.py                          | 234 +++++++++++++++++----
 .../https_example.org/archives_dummy-hello.tar.gz  | Bin 0 -> 221 bytes
 swh/loader/core/tests/test_loader.py               | 194 +++++++++++++++--
 4 files changed, 372 insertions(+), 58 deletions(-)
 create mode 100644 swh/loader/core/tests/data/https_example.org/archives_dummy-hello.tar.gz
Changes applied before test
commit da7cc372fe5a9cdf139de7cf51a2c6cb6dc8b8ed
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 30 15:10:49 2022 +0200

    {Content|Directory}Loader: Adapt support for checksums
    
    This moves away the loaders from the 'integrity' fields which is now dealt with lister
    side. Instead the lister is send a dictionary of hex hashes.
    
    This also improves the loader to check those checksums when retrieving the
    artifact (content or tarball). Thanks to a bump in the swh.model version, this is now
    able to deal with sha512 checksums checks as well.
    
    Related to T3781

commit 497f74f3225e4ccf11adce0d6a2bb50b2a471fab
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 30 11:54:13 2022 +0200

    Add Directory Loader to allow tarball ingestion as Directory
    
    In some marginal listing cases (Nix or Guix for now), we can receive raw tarball to
    ingest. This commit adds a loader to ingest those. The output of the ingestion is a
    snapshot with 1 branch, one HEAD branch targetting the ingested directory (contained
    within the tarball).
    
    This expects to receive a mandatory 'integrity' field. It is used to check the tarball
    received out of the origin.
    
    This can also optionally receive a list of mirror urls in case the main origin url is no
    longer available. Those mirror urls are solely used as fallback to retrieve the tarball.
    
    Related to T3781

Link to build: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/938/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/938/console

ardumont edited the summary of this revision. (Show Details)Oct 2 2022, 11:51 AM
ardumont added a comment.Oct 2 2022, 7:37 PM

Maybe a new upstream version bump on importlib-metadata?

[1]

...
importlib-metadata==5.0.0

Definitely something about that release.
I reproduced it in the docker dev environment.
Adding a workaround in requirements.txt (swh.{lister|loader}: importlib_metadata!=5.0.0).
Those containers are then able to start ^.

anlambert added a subscriber: anlambert.Oct 3 2022, 12:02 PM
anlambert added inline comments.
swh/loader/core/loader.py
734–735

I think you could reuse the download function (whose documentation is incorrect, need to fix this) instead of using get_url_body. This way you do not have to reimplement checking of hash values.

734–735

set(checksum.keys()) is more readable imho

ardumont added inline comments.Oct 3 2022, 1:56 PM
swh/loader/core/loader.py
734–735

I tried that but iirc that failed (i don't recall why).
As we are downloading raw file here (and not tarball), i thought this was related.

Will give it another go (as i really don't remember what the issue was ¯\_(ツ)_/¯).

734–735

sure (idk why i did it that way, must have been more convoluted at some point).

vlorentz accepted this revision.Oct 3 2022, 2:08 PM
This revision is now accepted and ready to land.Oct 3 2022, 2:08 PM
anlambert added inline comments.Oct 3 2022, 2:32 PM
swh/loader/core/loader.py
734–735

For the record, this seems to work:

diff --git a/swh/loader/core/loader.py b/swh/loader/core/loader.py
index b55a85f..9af55cf 100644
--- a/swh/loader/core/loader.py
+++ b/swh/loader/core/loader.py
@@ -716,7 +716,6 @@ class ContentLoader(NodeLoader):
 
     def fetch_data(self) -> bool:
         """Retrieve the content file as a Content Object"""
-        data: Optional[bytes] = None
         for url in self.mirror_urls:
             url_ = urlparse(url)
             self.log.debug(
@@ -727,36 +726,19 @@ class ContentLoader(NodeLoader):
                 url_.path,
             )
             try:
-                data = get_url_body(url)
+                with tempfile.TemporaryDirectory() as tmpdir:
+                    file_path, _ = download(url, dest=tmpdir, hashes=self.checksums)
+                    with open(file_path, "rb") as file:
+                        self.content = Content.from_data(file.read())
                 # otherwise continue
-            except NotFound:
-                self.log.debug("Not found %s, continue on next mirror url if any", url)
-                continue
-
-            checksum_algos = {c for c in self.checksums.keys()}
-            content_d = MultiHash.from_data(
-                data, hash_names=DEFAULT_ALGORITHMS | checksum_algos
-            ).digest()
-
-            # We now need to determine if we found a matching content (matching the
-            # checksums provided). As an implementation detail, we must drop the
-            # checksums that are unsupported by the Content Model (e.g. sha512)
-            found: bool = False
-            for checksum_algo in checksum_algos:
-                if checksum_algo not in DEFAULT_ALGORITHMS:
-                    # We must drop this unssuported checksum algo from the Content model
-                    actual_checksum = content_d.pop(checksum_algo)
-                else:
-                    actual_checksum = content_d[checksum_algo]
-
-                # and check whether we found a correct content matching the checksum
-                found = actual_checksum == hash_to_bytes(self.checksums[checksum_algo])
-
-            if found:
-                # We have a match, we have our content to ingest
-                content_d["data"] = data
-                content_d["length"] = len(data)
-                self.content = Content.from_dict(content_d)
+            except HTTPError as http_error:
+                if http_error.response.status_code == 404:
+                    self.log.debug(
+                        "Not found %s, continue on next mirror url if any", url
+                    )
+                    continue
+                raise
+            else:
                 # we are done, no more data to fetch
                 return False
ardumont added inline comments.Oct 3 2022, 2:34 PM
swh/loader/core/loader.py
734–735

Awesome, i was rebasing my stuff and getting back to it.
You gain me some time, thx.

ardumont updated this revision to Diff 31058.Oct 3 2022, 2:46 PM
ardumont marked an inline comment as done.

Adapt according to latest review

ardumont added a child revision: D8601: {Content|Directory}Loader: Register tasks.Oct 3 2022, 2:47 PM
swh-public-ci added a comment.Oct 3 2022, 2:50 PM

Build is green

Patch application report for D8587 (id=31058)

Rebasing onto dbf7f3dca0...

Current branch diff-target is up to date.
Changes applied before test
commit 39c33a66c27c030c7ae3cfba4a393c2ced468fbc
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 30 15:10:49 2022 +0200

    {Content|Directory}Loader: Adapt support for checksums
    
    This adapts the content/directory loader implementations to use directly a checksums
    dict which is now sent by the listers.
    
    This improves the loader to check those checksums when retrieving the artifact (content
    or tarball). Thanks to a bump in the swh.model version, this is now able to deal with
    sha512 checksums checks as well.
    
    This also aligns with the current package loaders which now are also checking the
    integrity of the tarballs they ingest.
    
    Related to T3781

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

Harbormaster completed remote builds in B32028: Diff 31058.Oct 3 2022, 2:50 PM
Closed by commit rDLDBASE39c33a66c27c: {Content|Directory}Loader: Adapt support for checksums (authored by ardumont). · Explain WhyOct 3 2022, 2:51 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLDBASE39c33a66c27c: {Content|Directory}Loader: Adapt support for checksums.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
2 lines--
swh/
loader/
core/
34 lines90%
Loading...
tests/
56 lines98%
Loading...

Diff 31015

View Options

requirements-swh.txt

Loading...
View Options

swh/loader/core/loader.py

Loading...
View Options

swh/loader/core/tests/test_loader.py

Loading...
About · Developer information · Legal