Page MenuHomeSoftware Heritage

Separate save code now status refresh routine from the listing ui
ClosedPublic

Authored by ardumont on Apr 23 2021, 10:28 AM.

Details

Summary

This introduces a new endpoint to refresh the status of the save code now requests. This
endpoint will be called periodically. That concern is out of scope of the listing ui and
source of status update desynchronization [1]

The listing ui now feels more immediate as implementation wise, it no longer writes when
actually listing rows.

This slightly improves the code by typing it a bit more. A new SaveOriginRequestInfo got
added to represent the save origin request to display view side. This matches what
other views manipulates.

[1] T3278#63827

Related to T3280

Test Plan

tox (happy tests or so i thought)

container:

  • listing ui only lists
  • cron calling the refresh endpoint to update the save requests statuses regularly [1]

[1]

$ kubectl get cronjob save-code-now-refresh-visit-status
NAME                                 SCHEDULE      SUSPEND   ACTIVE   LAST SCHEDULE   AGE
save-code-now-refresh-visit-status   */1 * * * *   False     0        60s             6m20s

webapp log outputs regularly

[webapp] 10.42.0.65 - - [23/Apr/2021:08:56:02 +0000] "GET /api/1/origin/save/refresh/ HTTP/1.1" 200 511 "-" "curl/7.76.1-DEV"

Diff Detail

Repository
rDWAPPS Web applications
Branch
separate-update-from-listing
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20959
Build 32528: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 32527: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
swh/web/common/models.py
123

I could probably revert this. My initial intent was to use it in the conversion code
below. Then as i moved towards the SaveOiriginRequestInfo typed dict, i stopped using
it.

I also wondered whether to have a to_dict method which would return directly the
SaveOriginRequestInfo dict. I don't realize if this is reasonable or not (that would
allow to avoid an intermediary function for one which sounds good to me).

@anlambert any thoughts on those matters?

Build has FAILED

Patch application report for D5583 (id=19952)

Rebasing onto a65bc357b7...

Current branch diff-target is up to date.
Changes applied before test
commit cf74c0dbe048803d7db648e74f990a6dd49568d9
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 10:21:24 2021 +0200

    scn: Separate refresh routine from the listing ui
    
    This introduces a new endpoint to refresh the status of the save code now requests. This
    endpoint will be called periodically. That concern is out of scope of the webapp.
    
    The listing ui now feels more immediate as implementation wise, it no longer writes when
    actually listing rows.
    
    This slightly improves the code by typing it a bit more. A new SaveOriginRequestInfo got
    adaded to represent the save origin request to display view side. This matches what
    other views manipulates.
    
    Related to T3280

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

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 23 2021, 10:35 AM
Harbormaster failed remote builds in B20954: Diff 19952!

Build as failed

unrelated reason most possibly, locally everything is ok (i did not update my tox, there was recent release of swh-auth so might be related?).

Build as failed

unrelated reason most possibly, locally everything is ok (i did not update my tox, there was recent release of swh-auth so might be related?).

yes, it's unrelated, the current master builld is red as well [1]

[1] https://jenkins.softwareheritage.org/job/DWAPPS/job/tests/2114/

Build as failed

unrelated reason most possibly, locally everything is ok (i did not update my tox, there was recent release of swh-auth so might be related?).

yes, it's unrelated, the current master builld is red as well [1]

[1] https://jenkins.softwareheritage.org/job/DWAPPS/job/tests/2114/

ah but yes, the fix is pending in D5579... (i saw but did not realize fully)

swh/web/common/origin_save.py
376

did not realize i renamed this, i'll revert

495

i'll add a docstring here.

ardumont edited the summary of this revision. (Show Details)
ardumont edited the test plan for this revision. (Show Details)

Adapt according to my review ;)

Just an excuse to trigger back the build with the deps on the fixing diff.

Build has FAILED

Patch application report for D5583 (id=19954)

Rebasing onto a65bc357b7...

Current branch diff-target is up to date.
Changes applied before test
commit 09ae9db0d32692d3b28ab789380794de347fe518
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 10:21:24 2021 +0200

    scn: Separate refresh routine from the listing ui
    
    This introduces a new endpoint to refresh the status of the save code now requests. This
    endpoint will be called periodically. That concern is out of scope of the webapp.
    
    The listing ui now feels more immediate as implementation wise, it no longer writes when
    actually listing rows.
    
    This slightly improves the code by typing it a bit more. A new SaveOriginRequestInfo got
    adaded to represent the save origin request to display view side. This matches what
    other views manipulates.
    
    Related to T3280

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

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 23 2021, 11:26 AM
Harbormaster failed remote builds in B20956: Diff 19954!

Build is green

Patch application report for D5583 (id=19957)

Rebasing onto 3a0ffc0879...

Current branch diff-target is up to date.
Changes applied before test
commit 1cf7b541d360582ebebbead86884a601fedbbf84
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 10:21:24 2021 +0200

    scn: Separate refresh routine from the listing ui
    
    This introduces a new endpoint to refresh the status of the save code now requests. This
    endpoint will be called periodically. That concern is out of scope of the webapp.
    
    The listing ui now feels more immediate as implementation wise, it no longer writes when
    actually listing rows.
    
    This slightly improves the code by typing it a bit more. A new SaveOriginRequestInfo got
    adaded to represent the save origin request to display view side. This matches what
    other views manipulates.
    
    Related to T3280

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

ardumont edited the summary of this revision. (Show Details)
anlambert requested changes to this revision.EditedApr 23 2021, 2:36 PM

I am okay with the idea.

In order to avoid adding a new internal endpoint, I would rather suggest to use the Jobs scheduling feature of django-extensions (packaged in debian) or simply create a django custom command.
This would still require to setup a cron job but this would be simpler (no HTTP request).

swh/web/common/models.py
123

I agree with you, the dict conversion should be handled directly in the model.

swh/web/common/origin_save.py
228–249

to be moved in the model

swh/web/common/typing.py
229–247

+1, thanks !

This revision now requires changes to proceed.Apr 23 2021, 2:36 PM

I am okay with the idea.

great.

In order to avoid adding a new internal endpoint, I would rather suggest to use the Jobs scheduling feature of django-extensions (packaged in debian)

That requires new deps so not so fan.

or simply create a django custom command.

That'd be best.

This would still require to setup a cron job but this would be simpler (no HTTP request).

Yes. Indeed.

Although, now i'm wondering what's the issue with that new refresh endpoint being opened?

That could be filtered by other means if that's an actual issue.
We could also make the endpoint requires authentication if need be (though that would require a tad more plumbing).

Advantage of it being opened is that it allows to make the cron run anywhere (with the custom command, it requires at least to run on a webapp instance and we got 2 in production now so that means conditional in puppet...).

ardumont edited the summary of this revision. (Show Details)
  • Rework model conversion according to review
  • Rebase

Remains to:

  • look into the custom django command
  • add tests on the refresh code

Build is green

Patch application report for D5583 (id=19971)

Rebasing onto 1ca5105c4e...

First, rewinding head to replay your work on top of it...
Applying: scn: Separate refresh routine from the listing ui
Changes applied before test
commit 8f75e46296325344c3714fc659b73e269deab56d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 10:21:24 2021 +0200

    scn: Separate refresh routine from the listing ui
    
    This introduces a new endpoint to refresh the status of the save code now requests. This
    endpoint will be called periodically. That concern is out of scope of the listing ui and
    source of status update desynchronization [1]
    
    The listing ui now feels more immediate as implementation wise, it no longer writes when
    actually listing rows.
    
    This slightly improves the code by typing it a bit more. A new SaveOriginRequestInfo got
    added to represent the save origin request to display view side. This matches what
    other views manipulates.
    
    [1] T3278#63827
    
    Related to T3280

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

Add tests on the refresh routine calls.

Remains to rework the public endpoint into athe django-admin custom command

Build is green

Patch application report for D5583 (id=19977)

Rebasing onto 1ca5105c4e...

First, rewinding head to replay your work on top of it...
Applying: scn: Separate refresh routine from the listing ui
Changes applied before test
commit dbc97c7a76f9605119aa6f4db1ef49b0ae62df93
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 10:21:24 2021 +0200

    scn: Separate refresh routine from the listing ui
    
    This introduces a new endpoint to refresh the status of the save code now requests. This
    endpoint will be called periodically. That concern is out of scope of the listing ui and
    source of status update desynchronization [1]
    
    The listing ui now feels more immediate as implementation wise, it no longer writes when
    actually listing rows.
    
    This slightly improves the code by typing it a bit more. A new SaveOriginRequestInfo got
    added to represent the save origin request to display view side. This matches what
    other views manipulates.
    
    [1] T3278#63827
    
    Related to T3280

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

Give a shot to use custom django admin command (drop public endpoint)

Drop some unneeded renamed variables

Build is green

Patch application report for D5583 (id=19982)

Rebasing onto 1ca5105c4e...

First, rewinding head to replay your work on top of it...
Applying: scn: Separate refresh routine from the listing ui
Applying: Prefer custom django admin command instead of a public endpoint
Changes applied before test
commit 7e87d895ec71286bd799aa1772bdca3994f0ac6f
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 17:36:52 2021 +0200

    Prefer custom django admin command instead of a public endpoint
    
    Related to T3280

commit e23432a140887885c79ba9f552dd83d6e1c32760
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 10:21:24 2021 +0200

    scn: Separate refresh routine from the listing ui
    
    This introduces a new endpoint to refresh the status of the save code now requests. This
    endpoint will be called periodically. That concern is out of scope of the listing ui and
    source of status update desynchronization [1]
    
    The listing ui now feels more immediate as implementation wise, it no longer writes when
    actually listing rows.
    
    This slightly improves the code by typing it a bit more. A new SaveOriginRequestInfo got
    added to represent the save origin request to display view side. This matches what
    other views manipulates.
    
    [1] T3278#63827
    
    Related to T3280

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

Build has FAILED

Patch application report for D5583 (id=19983)

Rebasing onto 1ca5105c4e...

First, rewinding head to replay your work on top of it...
Applying: scn: Separate refresh routine from the listing ui
Applying: Prefer custom django admin command instead of a public endpoint
Changes applied before test
commit b13ec1de0714c1fc812dadb5a0223ab384e5f38d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 17:36:52 2021 +0200

    Prefer custom django admin command instead of a public endpoint
    
    Related to T3280

commit 7b509c02d6aaf6fd70ff50178ecd4d829277b4b8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 10:21:24 2021 +0200

    scn: Separate refresh routine from the listing ui
    
    This introduces a new endpoint to refresh the status of the save code now requests. This
    endpoint will be called periodically. That concern is out of scope of the listing ui and
    source of status update desynchronization [1]
    
    The listing ui now feels more immediate as implementation wise, it no longer writes when
    actually listing rows.
    
    This slightly improves the code by typing it a bit more. A new SaveOriginRequestInfo got
    added to represent the save origin request to display view side. This matches what
    other views manipulates.
    
    [1] T3278#63827
    
    Related to T3280

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

Build is green

Patch application report for D5583 (id=19983)

Rebasing onto 1ca5105c4e...

First, rewinding head to replay your work on top of it...
Applying: scn: Separate refresh routine from the listing ui
Applying: Prefer custom django admin command instead of a public endpoint
Changes applied before test
commit ca2a109b94083f57e512b8aa49ce8c5e2cf5784d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 17:36:52 2021 +0200

    Prefer custom django admin command instead of a public endpoint
    
    Related to T3280

commit 8a4e7bb1282c2732440ac743c7598fdd52cba368
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 10:21:24 2021 +0200

    scn: Separate refresh routine from the listing ui
    
    This introduces a new endpoint to refresh the status of the save code now requests. This
    endpoint will be called periodically. That concern is out of scope of the listing ui and
    source of status update desynchronization [1]
    
    The listing ui now feels more immediate as implementation wise, it no longer writes when
    actually listing rows.
    
    This slightly improves the code by typing it a bit more. A new SaveOriginRequestInfo got
    added to represent the save origin request to display view side. This matches what
    other views manipulates.
    
    [1] T3278#63827
    
    Related to T3280

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

Move command so it's actually seen by manage.py

Build is green

Patch application report for D5583 (id=19986)

Rebasing onto 1ca5105c4e...

First, rewinding head to replay your work on top of it...
Applying: scn: Separate refresh routine from the listing ui
Applying: Prefer custom django admin command instead of a public endpoint
Changes applied before test
commit 155edb59d3fab34f81996e11197544724e17b75b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 17:36:52 2021 +0200

    Prefer custom django admin command instead of a public endpoint
    
    Related to T3280

commit b2ae57354e810948176637edf75423862bfaa7b8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 10:21:24 2021 +0200

    scn: Separate refresh routine from the listing ui
    
    This introduces a new endpoint to refresh the status of the save code now requests. This
    endpoint will be called periodically. That concern is out of scope of the listing ui and
    source of status update desynchronization [1]
    
    The listing ui now feels more immediate as implementation wise, it no longer writes when
    actually listing rows.
    
    This slightly improves the code by typing it a bit more. A new SaveOriginRequestInfo got
    added to represent the save origin request to display view side. This matches what
    other views manipulates.
    
    [1] T3278#63827
    
    Related to T3280

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

swh/web/common/management/commands/refresh_save_origin_request_status.py
21 ↗(On Diff #19986)

Found some docs on how to test this [1]
Will attend to it.

[1] https://docs.djangoproject.com/en/2.2/topics/testing/tools/#topics-testing-management-commands

nit: this should probably be in swh/web/management/ instead of swh/web/common/management/.

nit: this should probably be in swh/web/management/ instead of swh/web/common/management/.

I tried there initially but this did not work (for no obvious reason... no error is reported).
And @anlambert suggested this very place which worked so i'm good ;)

Thanks though

nit: this should probably be in swh/web/management/ instead of swh/web/common/management/.

I tried there initially but this did not work (for no obvious reason... no error is reported).

It's just plainly ignored:

18:39 ardumont swh@webapp-649c76c6fc-q7bjn:/$ ls -lah /app/swh-web/swh/web/management/commands/refresh_save_origin_request_status.py
18:39 ardumont -rw-r--r-- 1 root root 819 Apr 23 15:04 /app/swh-web/swh/web/management/commands/refresh_save_origin_request_status.py
18:39 ardumont swh@webapp-649c76c6fc-q7bjn:/$ DJANGO_SETTINGS_MODULE=swh.web.settings.development python3 swh/web/manage.py -h
18:39 ardumont python3: can't open file 'swh/web/manage.py': [Errno 2] No such file or directory

And @anlambert suggested this very place which worked so i'm good ;)

After modification:

19:07 <ardumont> swh@webapp-85ddc77bcd-ww7zb:/$ python3 /app/swh-web/swh/web/manage.py refresh_save_origin_request_status
19:07 <ardumont> /srv/softwareheritage/venv/lib/python3.7/site-packages/jose/backends/cryptography_backend.py:18: CryptographyDeprecationWarning: int_from_bytes is deprecated, use int.from_bytes instead
19:07 <ardumont>   from cryptography.utils import int_from_bytes, int_to_bytes
19:07 <ardumont> Nothing to do.
19:07 <ardumont> finally
19:09 <ardumont> Successfully updated 1 save request(s).
19:09 <ardumont> ;)
This revision is now accepted and ready to land.Apr 26 2021, 11:31 AM

18:39 ardumont swh@webapp-649c76c6fc-q7bjn:/$ DJANGO_SETTINGS_MODULE=swh.web.settings.development python3 swh/web/manage.py -h
18:39 ardumont python3: can't open file 'swh/web/manage.py': [Errno 2] No such file or directory

That's the wrong check to do.
As i don't find the actual command back, i'll triple check once i successfully ran those missing tests.

  • Add missing test on django custom command
  • Rename it slightly simpler

Build is green

Patch application report for D5583 (id=19992)

Rebasing onto 1ca5105c4e...

First, rewinding head to replay your work on top of it...
Applying: scn: Separate refresh routine from the listing ui
Applying: Prefer custom django admin command instead of a public endpoint
Changes applied before test
commit f2444549d69dcd6fdd8947f776ca02d711d9f9ca
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 17:36:52 2021 +0200

    Prefer custom django admin command instead of a public endpoint
    
    Related to T3280

commit 7bfa36c86d394656653d29e9f6dbac2874fc9617
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 10:21:24 2021 +0200

    scn: Separate refresh routine from the listing ui
    
    This introduces a new endpoint to refresh the status of the save code now requests. This
    endpoint will be called periodically. That concern is out of scope of the listing ui and
    source of status update desynchronization [1]
    
    The listing ui now feels more immediate as implementation wise, it no longer writes when
    actually listing rows.
    
    This slightly improves the code by typing it a bit more. A new SaveOriginRequestInfo got
    added to represent the save origin request to display view side. This matches what
    other views manipulates.
    
    [1] T3278#63827
    
    Related to T3280

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

ardumont retitled this revision from scn: Separate refresh routine from the listing ui to Separate save code now status refresh routine from the listing ui.Apr 26 2021, 3:18 PM
  • Rework commit message and expand scn
  • Squash commits into 1

Build is green

Patch application report for D5583 (id=19994)

Rebasing onto 1ca5105c4e...

First, rewinding head to replay your work on top of it...
Applying: Separate save code now status refresh routine from the listing ui
Changes applied before test
commit 7ae27f7bbce646a900ceb5aa3b35e39cb4e8126e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 10:21:24 2021 +0200

    Separate save code now status refresh routine from the listing ui
    
    This introduces a new endpoint to refresh the status of the save code now requests. This
    endpoint will be called periodically. That concern is out of scope of the listing ui and
    source of status update desynchronization [1]
    
    The listing ui now feels more immediate as implementation wise, it no longer writes when
    actually listing rows.
    
    This slightly improves the code by typing it a bit more. A new SaveOriginRequestInfo got
    added to represent the save origin request to display view side. This matches what
    other views manipulates.
    
    [1] T3278#63827
    
    Related to T3280

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

Build is green

Patch application report for D5583 (id=19997)

Rebasing onto 1ca5105c4e...

Current branch diff-target is up to date.
Changes applied before test
commit f92c78719a9a8a0d2acdc622efc346f7a9b07030
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Apr 23 10:21:24 2021 +0200

    Separate save code now status refresh routine from the listing ui
    
    This introduces a new endpoint to refresh the status of the save code now requests. This
    endpoint will be called periodically. That concern is out of scope of the listing ui and
    source of status update desynchronization [1]
    
    The listing ui now feels more immediate as implementation wise, it no longer writes when
    actually listing rows.
    
    This slightly improves the code by typing it a bit more. A new SaveOriginRequestInfo got
    added to represent the save origin request to display view side. This matches what
    other views manipulates.
    
    [1] T3278#63827
    
    Related to T3280

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