Page MenuHomeSoftware Heritage

nixguix: catch and log artifact resolution failures
ClosedPublic

Authored by ardumont on Apr 2 2020, 5:38 PM.

Details

Summary

This catches any unexpected revision metadata structure which would made the
loader crash (when it tries to resolve known artifacts from a snapshot) and
continue the ingestion.

Related to T2371

Test Plan

tox

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Build is green

Patch application report for D2949 (id=10497)

Rebasing onto fbf457e92c...

Current branch diff-target is up to date.
Changes applied before test
commit c1f2fc70056935359ad1ae5c018dc5c7daebe1bf
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Thu Apr 2 17:34:01 2020 +0200

    nixguix: Drop old snapshot format
    
    Previous run on the same origin could miss the integrity field. This makes for
    unresolvable revision. This returns None directly.

commit 25a02890e09a8af66dae13bd50ab3d7bf3f9c0a6
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Thu Apr 2 17:33:19 2020 +0200

    nixguix: Some sources.json can be inconsistent in their sources
    
    Drop those without integrity key.
    
    The fix should be upstream in the sources.json though

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

First part of the diff will be fixed by D2951

Rebase on latest master (drop 1 commit)

Build is green

Patch application report for D2949 (id=10525)

Rebasing onto 5921567408...

Current branch diff-target is up to date.
Changes applied before test
commit 8fb9827d0cc89c057136a7bbe6000477f7f075c7
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Thu Apr 2 17:34:01 2020 +0200

    nixguix: Drop old snapshot format
    
    Previous run on the same origin could miss the integrity field. This makes for
    unresolvable revision. This returns None directly.

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

ardumont retitled this revision from nixguix: Fix current staging issues to nixguix: Fix "integrity" field resolution out of a snapshot.
ardumont edited the summary of this revision. (Show Details)
ardumont added a project: Package Loader.
ardumont edited the summary of this revision. (Show Details)
swh/loader/package/nixguix/loader.py
68–69

We should catch a keyError exception instead since it could come from several keys.

ardumont added inline comments.
swh/loader/package/nixguix/loader.py
68–69

yes

douardda added inline comments.
swh/loader/package/nixguix/loader.py
68–69

I agree a try/except KeyError is better here.

Also why exit the for loop? Isn't there any chance the searched item is found later in the for loop?

Working on adding one test which pass in there (load from archive loader, then load from nixguix whose listing list the same artifact...)

swh/loader/package/nixguix/loader.py
68–69

Yes, i changed it locally already. Thanks.

I can only assume i was thinking (partially wrongly), the other revisions must
be in the same bad format. It was true in staging when i first hit that issue,
that's possibly not the case in production.

Working on adding one test which pass in there (load from archive loader, then load from nixguix whose listing list the same artifact...)

I actually don't hit the issue in a test...

ardumont edited the summary of this revision. (Show Details)
  • Rebase on latest master
  • when a missing key is triggered, continue checking for other revisions of the snapshot

Build is green

Patch application report for D2949 (id=11291)

Rebasing onto e816a9a98a...

Current branch diff-target is up to date.
Changes applied before test
commit dc2c0a226149abad7691911cb7f0e3a1c3b10fa7
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Mon May 25 12:43:18 2020 +0200

    nixguix: Fix "integrity" field resolution out of a snapshot
    
    Summary:
    Due to the way we store revision metadata for package loader. A snapshot can
    reference a previous revision with metadata that are not nixguix related. Thus,
    the expected "integrity" (from the nixguix loader point of view) field is not
    found. Which makes the loader crash.
    
    This diff allows the loader to continue its work without crashing by
    considering such revision to be fetched again.
    
    pros:
    - the loader works
    
    cons:
    - redo the ingestion for a given artifact which is hit by this issue. This will
      be noops in the storage side though
    
    Related to T2371
    Related to T2411
    
    Test Plan: tox
    
    Reviewers: #reviewers, lewo
    
    Tags: #package_loader
    
    Maniphest Tasks: T2371, T2411
    
    Differential Revision: https://forge.softwareheritage.org/D2949

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

Build is green

Patch application report for D2949 (id=11292)

Rebasing onto e816a9a98a...

Current branch diff-target is up to date.
Changes applied before test
commit 3b9fda6df52544cd7795ce9ddb778f0d2e908979
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Mon May 25 12:43:18 2020 +0200

    nixguix: Fix revision "integrity" field resolution (out of a snapshot)
    
    Due to the way we store revision metadata for package loader. A snapshot can
    reference a previous revision with metadata that are not nixguix related. Thus,
    the expected "integrity" (from the nixguix loader point of view) field is not
    found. Which makes the loader crash.
    
    This commit allows the loader to continue its work without crashing by
    considering such revision to be fetched again.
    
    Related to T2371
    Related to T2411

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

Working on adding one test which pass in there (load from archive loader, then load from nixguix whose listing list the same artifact...)

I actually don't hit the issue in a test...

Well, i did through patching...

Also, something hit me during that phase...
The "evaluation" branch is a snapshot branch as well and this one does not have any metadata at all.
So in the end, wondering is this not that branch which is the culprit...

swh/loader/package/nixguix/loader.py
70

When the sources.json file is parsed, we check the integrity attribute is present. So, I think it should never be missing.
In this case, I think it would improve code clearness to move this expression out of the try block.

Rework according to review
(still working on a test)

Build is green

Patch application report for D2949 (id=11294)

Rebasing onto e816a9a98a...

Current branch diff-target is up to date.
Changes applied before test
commit eb9bf749399dfa51512981724a4f61c20255c9de
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Mon May 25 12:43:18 2020 +0200

    nixguix: Fix revision "integrity" field resolution (out of a snapshot)
    
    Due to the way we store revision metadata for package loader. A snapshot can
    reference a previous revision with metadata that are not nixguix related. Thus,
    the expected "integrity" (from the nixguix loader point of view) field is not
    found. Which makes the loader crash.
    
    This commit allows the loader to continue its work without crashing by
    considering such revision to be fetched again.
    
    Related to T2371
    Related to T2411

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

ardumont added inline comments.
swh/loader/package/nixguix/loader.py
70

ack, adapted accordingly, thx.

ardumont marked an inline comment as done.
  • nixguix: Add a test around the wrong metadata revision structure detection

Build has FAILED

Patch application report for D2949 (id=11295)

Rebasing onto e816a9a98a...

Current branch diff-target is up to date.
Changes applied before test
commit e850966ff526ee0c1cc35a124510076e4413d29a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon May 25 15:54:34 2020 +0200

    nixguix: Add a test around the wrong metadata revision structure

commit eb9bf749399dfa51512981724a4f61c20255c9de
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Mon May 25 12:43:18 2020 +0200

    nixguix: Fix revision "integrity" field resolution (out of a snapshot)
    
    Due to the way we store revision metadata for package loader. A snapshot can
    reference a previous revision with metadata that are not nixguix related. Thus,
    the expected "integrity" (from the nixguix loader point of view) field is not
    found. Which makes the loader crash.
    
    This commit allows the loader to continue its work without crashing by
    considering such revision to be fetched again.
    
    Related to T2371
    Related to T2411

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

ardumont retitled this revision from nixguix: Fix "integrity" field resolution out of a snapshot to nixguix: Fix "integrity" field resolution.May 25 2020, 6:50 PM

Rebase on latest master

I'm in a mind of landing this after all (after a night of sleep).

So this closes properly T2371 (this adds logs so we should be able to identify
the issue if that happens at all)

Related to T2371

Build has FAILED

Patch application report for D2949 (id=11299)

Rebasing onto 5ba92ac219...

Current branch diff-target is up to date.
Changes applied before test
commit 71f89a4c7d3470b0265b3f65c8ea0d171a40b409
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon May 25 15:54:34 2020 +0200

    nixguix: Add a test around the wrong metadata revision structure

commit 1944ab28489f90eed40cd8c52f0f1554b9fa704a
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Mon May 25 12:43:18 2020 +0200

    nixguix: Fix revision "integrity" field resolution (out of a snapshot)
    
    Due to the way we store revision metadata for package loader. A snapshot can
    reference a previous revision with metadata that are not nixguix related. Thus,
    the expected "integrity" (from the nixguix loader point of view) field is not
    found. Which makes the loader crash.
    
    This commit allows the loader to continue its work without crashing by
    considering such revision to be fetched again.
    
    Related to T2371
    Related to T2411

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

ardumont retitled this revision from nixguix: Fix "integrity" field resolution to nixguix: catch eventual artifact resolution failures and logs those.May 26 2020, 10:05 AM
ardumont edited the summary of this revision. (Show Details)
swh/loader/package/nixguix/tests/data/https_nix-community.github.io/nixpkgs-swh_sources.json_visit1
12 ↗(On Diff #11299)

That should not have been modified.
That's most possibly the source of the test failures.
Fixing.

ardumont edited the summary of this revision. (Show Details)
  • Fix existing source.json which should not have been modified in the first place
  • Add test scenario around the modification the diff introduces

Build is green

Patch application report for D2949 (id=11300)

Rebasing onto 5ba92ac219...

Current branch diff-target is up to date.
Changes applied before test
commit 5002c56e9bba2e2b2bf0d161bdf76f38eb7a1c36
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Mon May 25 12:43:18 2020 +0200

    nixguix: catch eventual artifact resolution failures and log those
    
    This eventually catch any malformed revision (from a metadata standpoint) which
    would made the loader crash (when it tries to resolve the known artifacts).
    
    This catches the error, logs information about it and then continue on loading.
    
    Related to T2371

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

Drop unnecessary log instructions

Build is green

Patch application report for D2949 (id=11301)

Rebasing onto 5ba92ac219...

Current branch diff-target is up to date.
Changes applied before test
commit 98b54d7ef12fa9930f43d81052624e520b992202
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Mon May 25 12:43:18 2020 +0200

    nixguix: catch eventual artifact resolution failures and log those
    
    This eventually catch any malformed revision (from a metadata standpoint) which
    would made the loader crash (when it tries to resolve the known artifacts).
    
    This catches the error, logs information about it and then continue on loading.
    
    Related to T2371

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

ardumont retitled this revision from nixguix: catch eventual artifact resolution failures and logs those to nixguix: catch and log artifact resolution failures.May 26 2020, 12:11 PM
vlorentz added a subscriber: vlorentz.
vlorentz added inline comments.
swh/loader/package/nixguix/loader.py
71–80

why not log.exception? it looks important enough to deserve it

72

I'd rather call it "unexpected" than "divergent"

(divergent evokes an infinite data structure for me)

This revision is now accepted and ready to land.May 26 2020, 1:00 PM
  • Adapt according to review (log.exception, prefer unexpected to divergent)
  • Rework commit message to align with the diff
ardumont edited the summary of this revision. (Show Details)

rework commit message again (last time!)

Build is green

Patch application report for D2949 (id=11305)

Rebasing onto 5ba92ac219...

Current branch diff-target is up to date.
Changes applied before test
commit 30caeb640ad61107ad99e407d2fa69e4d67fca5c
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Mon May 25 12:43:18 2020 +0200

    nixguix: catch and log artifact resolution failures
    
    This eventually catch any unexpected revision metadata structure which would
    made the loader crash (when it tries to resolve the known artifacts) and
    continue the ingestion.
    
    Related to T2371

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

Build is green

Patch application report for D2949 (id=11306)

Rebasing onto 5ba92ac219...

Current branch diff-target is up to date.
Changes applied before test
commit c6be4c0fd41963278717fe2b4ac119b1f99e0fdd
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Mon May 25 12:43:18 2020 +0200

    nixguix: catch and log artifact resolution failures
    
    This catches any unexpected revision metadata structure which would made the
    loader crash (when it tries to resolve known artifacts from a snapshot) and
    continue the ingestion.
    
    Related to T2371

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