Page MenuHomeSoftware Heritage

package.loader: Check for snapshot id prior to reifying model object
ClosedPublic

Authored by ardumont on May 29 2020, 12:24 PM.

Details

Summary

This is an issue mostly encountered on staging. But technically, the error
could happen in production as snapshot_get_all_branches could return None as
well (which is currently ignored). If that happens though, Snapshot.from_dict
raises.

Most proeminent in staging, because we can mount partial dump from production.
Thus not everything exists. Typically, recently we mounted a partial dump of
origins/origin-visits only. This means origin visits will target inexisting
snapshots [1]

Thix fix ensures we can use staging nonetheless. This also improves the current
implementation path so win.

Related to T2428

[1] https://sentry.softwareheritage.org/share/issue/4eb69ea2cc314c4ab811809ff60c358f/

Test Plan

tox

Diff Detail

Repository
rDLDBASE Generic VCS/Package Loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.May 29 2020, 12:24 PM

Build is green

Patch application report for D3196 (id=11351)

Rebasing onto c6be4c0fd4...

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

    package.loader: Check for snapshot id prior to reify the model object
    
    This is an issue encountered on staging alone for now. As we can mount partial
    dump from production, not everything exists. Typically dangling snapshot from
    origin visits currently. This ensures we can use staging nonetheless.
    
    Related to T2428

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

ardumont edited the summary of this revision. (Show Details)May 29 2020, 12:26 PM
ardumont edited the summary of this revision. (Show Details)May 29 2020, 12:31 PM
ardumont edited the summary of this revision. (Show Details)May 29 2020, 12:44 PM
olasd accepted this revision.May 29 2020, 12:52 PM
olasd added a subscriber: olasd.

Thanks!

Please rename the snap variable to something meaningful :)

swh/loader/package/loader.py
127–128

snap -> snapshot_dict

This revision is now accepted and ready to land.May 29 2020, 12:52 PM
ardumont updated this revision to Diff 11352.May 29 2020, 1:21 PM
ardumont edited the summary of this revision. (Show Details)
  • Adapt according to review
  • Align git commit with diff description
ardumont edited the summary of this revision. (Show Details)May 29 2020, 1:22 PM
ardumont updated this revision to Diff 11353.May 29 2020, 1:22 PM
ardumont edited the summary of this revision. (Show Details)

Rework commit message

Build is green

Patch application report for D3196 (id=11352)

Rebasing onto faa138226d...

First, rewinding head to replay your work on top of it...
Applying: package.loader: Check for snapshot id prior to reifying model object
Changes applied before test
commit da6795a2b178bd139ec2ac012ce11b1116816758
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri May 29 12:14:14 2020 +0200

    package.loader: Check for snapshot id prior to reifying model object
    
    This is an issue mostly encountered on staging. But technically, the error
    could happen in production as snapshot_get_all_branches could return None as
    well (which is currently ignored). If that happens though, Snapshot.from_dict
    raises.
    
    Most proeminent in staging, because we can mount partial dump from production.
    Thus not everything exists. Typically, recently we mounted a partial dump of
    origins/origin-visits only. This means origin visits will target not
    inexisting [1]
    
    Thix fix ensures we can use staging nonetheless. This also improves the current
    implementation path so win.
    
    Related to T2428
    
    [1] https://sentry.softwareheritage.org/share/issue/4eb69ea2cc314c4ab811809ff60c358f/

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

ardumont retitled this revision from package.loader: Check for snapshot id prior to reify the model object to package.loader: Check for snapshot id prior to reifying model object.May 29 2020, 1:23 PM

Build is green

Patch application report for D3196 (id=11353)

Rebasing onto faa138226d...

First, rewinding head to replay your work on top of it...
Applying: package.loader: Check for snapshot id prior to reifying model object
Changes applied before test
commit 93a748ca2884cb7dd6bda2175b2281205437cffa
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri May 29 12:14:14 2020 +0200

    package.loader: Check for snapshot id prior to reifying model object
    
    This is an issue mostly encountered on staging. But technically, the error
    could happen in production as snapshot_get_all_branches could return None as
    well (which is currently ignored). If that happens though, `Snapshot.from_dict`
    raises.
    
    Most proeminent in staging, because we can mount partial dump from production.
    Thus not everything exists. Typically, recently we mounted a partial dump of
    origins/origin-visits only. This means origin visits will target inexisting
    snapshots [1]
    
    Thix fix ensures we can use staging nonetheless. This also improves the current
    implementation path so win.
    
    Related to T2428
    
    [1] https://sentry.softwareheritage.org/share/issue/4eb69ea2cc314c4ab811809ff60c358f/

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

ardumont updated this revision to Diff 11366.May 29 2020, 3:57 PM

Rebase on latest master

Build is green

Patch application report for D3196 (id=11366)

Rebasing onto dd2dc47667...

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

    package.loader: Check for snapshot id prior to reifying model object
    
    This is an issue mostly encountered on staging. But technically, the error
    could happen in production as snapshot_get_all_branches could return None as
    well (which is currently ignored). If that happens though, `Snapshot.from_dict`
    raises.
    
    Most proeminent in staging, because we can mount partial dump from production.
    Thus not everything exists. Typically, recently we mounted a partial dump of
    origins/origin-visits only. This means origin visits will target inexisting
    snapshots [1]
    
    Thix fix ensures we can use staging nonetheless. This also improves the current
    implementation path so win.
    
    Related to T2428
    
    [1] https://sentry.softwareheritage.org/share/issue/4eb69ea2cc314c4ab811809ff60c358f/

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