Page MenuHomeSoftware Heritage
Differential D2949

nixguix: catch and log artifact resolution failures
ClosedPublic
Actions

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

Details

Reviewers
lewo
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T2411: Deploy nixguix loader
T2371: nixguix: fails to use previous visit snapshot
Commits
rDLDBASEc6be4c0fd419: nixguix: catch and log artifact resolution failures
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
swh-public-ci added a comment.Apr 2 2020, 5:39 PM

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.

Harbormaster completed remote builds in B11589: Diff 10497.Apr 2 2020, 5:39 PM
ardumont edited the summary of this revision. (Show Details)Apr 2 2020, 5:46 PM
ardumont added a comment.Apr 3 2020, 11:50 AM

First part of the diff will be fixed by D2951

ardumont updated this revision to Diff 10525.Apr 3 2020, 3:53 PM

Rebase on latest master (drop 1 commit)

ardumont edited the summary of this revision. (Show Details)Apr 3 2020, 3:54 PM
swh-public-ci added a comment.Apr 3 2020, 3:55 PM

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.

Harbormaster completed remote builds in B11616: Diff 10525.Apr 3 2020, 3:55 PM
ardumont edited the summary of this revision. (Show Details)Apr 3 2020, 3:55 PM
ardumont abandoned this revision.Apr 6 2020, 3:05 PM
ardumont mentioned this in T2411: Deploy nixguix loader.May 25 2020, 11:34 AM
ardumont reclaimed this revision.May 25 2020, 11:43 AM
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 added a task: T2371: nixguix: fails to use previous visit snapshot.
ardumont edited the summary of this revision. (Show Details)
ardumont added a task: T2411: Deploy nixguix loader.
lewo added inline comments.May 25 2020, 12:08 PM
swh/loader/package/nixguix/loader.py
68–69

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

ardumont edited the summary of this revision. (Show Details)May 25 2020, 12:15 PM
ardumont added inline comments.
swh/loader/package/nixguix/loader.py
68–69

yes

douardda added a subscriber: douardda.May 25 2020, 2:30 PM
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?

ardumont added a comment.May 25 2020, 2:31 PM

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

ardumont added inline comments.May 25 2020, 2:33 PM
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.

ardumont added a comment.May 25 2020, 3:46 PM

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 updated this revision to Diff 11291.May 25 2020, 4:00 PM
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
swh-public-ci added a comment.May 25 2020, 4:02 PM

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.

Harbormaster completed remote builds in B12482: Diff 11291.May 25 2020, 4:02 PM
ardumont updated this revision to Diff 11292.May 25 2020, 4:11 PM

Rework commit message

swh-public-ci added a comment.May 25 2020, 4:13 PM

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.

Harbormaster completed remote builds in B12483: Diff 11292.May 25 2020, 4:13 PM
ardumont added a comment.May 25 2020, 5:33 PM

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...

lewo added inline comments.May 25 2020, 5:33 PM
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.

ardumont updated this revision to Diff 11294.May 25 2020, 5:48 PM

Rework according to review
(still working on a test)

swh-public-ci added a comment.May 25 2020, 5:49 PM

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.

Harbormaster completed remote builds in B12485: Diff 11294.May 25 2020, 5:49 PM
ardumont marked an inline comment as done.May 25 2020, 6:26 PM
ardumont added inline comments.
swh/loader/package/nixguix/loader.py
70

ack, adapted accordingly, thx.

ardumont updated this revision to Diff 11295.May 25 2020, 6:36 PM
ardumont marked an inline comment as done.
  • nixguix: Add a test around the wrong metadata revision structure detection
swh-public-ci added a comment.May 25 2020, 6:38 PM

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

Harbormaster failed remote builds in B12486: Diff 11295!May 25 2020, 6:38 PM
ardumont mentioned this in D3178: nixguix: Override known_artifacts to filter out evaluation branch.May 25 2020, 6:44 PM
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
ardumont abandoned this revision.May 25 2020, 6:52 PM

Replaced by D3178

ardumont mentioned this in T2371: nixguix: fails to use previous visit snapshot.May 26 2020, 9:48 AM
ardumont updated this revision to Diff 11299.May 26 2020, 10:02 AM

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

swh-public-ci added a comment.May 26 2020, 10:04 AM

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

Harbormaster failed remote builds in B12491: Diff 11299!May 26 2020, 10:04 AM
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)
ardumont added inline comments.May 26 2020, 10:14 AM
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 planned changes to this revision.May 26 2020, 10:15 AM
ardumont updated this revision to Diff 11300.EditedMay 26 2020, 11:55 AM
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
swh-public-ci added a comment.May 26 2020, 11:57 AM

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.

Harbormaster completed remote builds in B12492: Diff 11300.May 26 2020, 11:57 AM
ardumont updated this revision to Diff 11301.May 26 2020, 11:57 AM

Drop unnecessary log instructions

swh-public-ci added a comment.May 26 2020, 11:59 AM

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.

Harbormaster completed remote builds in B12493: Diff 11301.May 26 2020, 11:59 AM
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 accepted this revision.May 26 2020, 1:00 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
ardumont updated this revision to Diff 11305.May 26 2020, 2:03 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)May 26 2020, 2:04 PM
ardumont updated this revision to Diff 11306.May 26 2020, 2:04 PM
ardumont edited the summary of this revision. (Show Details)

rework commit message again (last time!)

swh-public-ci added a comment.May 26 2020, 2:05 PM

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.

Harbormaster completed remote builds in B12497: Diff 11305.May 26 2020, 2:05 PM
swh-public-ci added a comment.May 26 2020, 2:07 PM

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.

Harbormaster completed remote builds in B12498: Diff 11306.May 26 2020, 2:07 PM
Closed by commit rDLDBASEc6be4c0fd419: nixguix: catch and log artifact resolution failures (authored by ardumont). · Explain WhyMay 26 2020, 2:14 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLDBASEc6be4c0fd419: nixguix: catch and log artifact resolution failures.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
loader/
package/
nixguix/
18 lines95%
Loading...

Diff 11294

View Options

swh/loader/package/nixguix/loader.py

Loading...
About · Developer information · Legal