Page MenuHomeSoftware Heritage
Differential D8922

send-to-celery: Adapt to schedule from lister name & instance_name
ClosedPublic
Actions

Authored by ardumont on Dec 6 2022, 12:25 PM.

Details

Reviewers
olasd
Group Reviewers
Reviewers
Commits
rDSCHa7769639df76: send-to-celery: Adapt to schedule from lister name & instance_name
Summary

This allows to bypass the lister id retrieval step using directly the name and instance
name of the lister to discover the uuid.

This also drops the --lister-uuid flag which is somewhat difficult to use.

Related to https://gitlab.softwareheritage.org/infra/sysadm-environment/-/issues/4674

Test Plan

Add new test ongoing

Diff Detail

Repository
rDSCH Scheduling utilities
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.Dec 6 2022, 12:25 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptDec 6 2022, 12:25 PM
swh-public-ci added a comment.Dec 6 2022, 12:30 PM

Build is green

Patch application report for D8922 (id=32137)

Rebasing onto ff75e742ee...

Current branch diff-target is up to date.
Changes applied before test
commit e6b13bdf102262f91a94296f269967e55383c67f
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Dec 6 12:24:33 2022 +0100

    scheduler: Open `swh scheduler lister` command to retrieve lister id
    
    Example use case:
swhscheduler@scheduler0:~/addforgenow$ swh scheduler \
  --url http://scheduler0.internal.staging.swh.network:5008/ \
  lister \
  --name gitea --instance-name git.afpy.org
d07d1c90-5016-4ab6-91ac-3300f8eb4fc6
```

Related to https://gitlab.softwareheritage.org/infra/sysadm-environment/-/issues/4674
See https://jenkins.softwareheritage.org/job/DSCH/job/tests-on-diff/570/ for more details.
Harbormaster completed remote builds in B33090: Diff 32137.Dec 6 2022, 12:30 PM
ardumont requested review of this revision.Dec 6 2022, 12:30 PM
olasd added a subscriber: olasd.Dec 6 2022, 12:37 PM

As I mentioned on IRC, I think we should do that "join" directly in swh scheduler origin send-to-celery (and have it error out if the lister name/instance provided don't match an existing lister); using the uuid was an "easy hack" to extend the API of grab_next_visits, but using lister name/instance in the CLI interface makes more sense.

And we should keep the argument names consistent with other endpoints that do the same filtering in that command subgroup as well.

ardumont planned changes to this revision.Dec 6 2022, 2:15 PM

To adapt according to suggestion (already on it).

ardumont updated this revision to Diff 32145.Dec 6 2022, 2:49 PM

Adapt according to suggestion (still trying to determine why the build fails)

ardumont retitled this revision from scheduler: Open `swh scheduler lister` command to retrieve lister id to scheduler: Adapt send-to-celery to allow scheduling from lister name.Dec 6 2022, 2:50 PM
ardumont edited the summary of this revision. (Show Details)
ardumont edited the test plan for this revision. (Show Details)
ardumont retitled this revision from scheduler: Adapt send-to-celery to allow scheduling from lister name to Adapt send-to-celery cli to allow scheduling from simpler lister info.Dec 6 2022, 2:56 PM
ardumont edited the summary of this revision. (Show Details)
swh-public-ci added a comment.Dec 6 2022, 3:01 PM

Build was aborted

Patch application report for D8922 (id=32145)

Rebasing onto ff75e742ee...

Current branch diff-target is up to date.
Changes applied before test
commit d38f916ac5a02004276658c0aa51bc24146ff6f0
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Dec 6 12:24:33 2022 +0100

    scheduler: Adapt send-to-celery to allow scheduling from lister name
    
    This is somewhat simpler to scheduler than to lookup for the lister id.
    
    Related to https://gitlab.softwareheritage.org/infra/sysadm-environment/-/issues/4674

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

Harbormaster failed remote builds in B33098: Diff 32145!Dec 6 2022, 3:01 PM
ardumont updated this revision to Diff 32146.Dec 6 2022, 3:45 PM
ardumont edited the summary of this revision. (Show Details)

Amend commit message and fix tests

swh-public-ci added a comment.Dec 6 2022, 3:49 PM

Build is green

Patch application report for D8922 (id=32146)

Rebasing onto ff75e742ee...

Current branch diff-target is up to date.
Changes applied before test
commit 774d8d34af3a06cc7fc77036698b210dca49b9c6
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Dec 6 12:24:33 2022 +0100

    Adapt send-to-celery cli to allow scheduling from simpler lister info
    
    This is simpler to schedule origins from a given forge when we are allowed to provide
    both the lister name and its instance name. Currently, we are required to lookup the
    lister uuid first (through psql) and then trigger the cli appropriately.
    
    Related to https://gitlab.softwareheritage.org/infra/sysadm-environment/-/issues/4674

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

Harbormaster completed remote builds in B33099: Diff 32146.Dec 6 2022, 3:49 PM
ardumont added inline comments.Dec 6 2022, 4:01 PM
swh/scheduler/backend.py
528 ↗(On Diff #32146)

I expected this to be covered but the lister i used (in the test) has no instance name ¯\_(ツ)_/¯.

swh/scheduler/cli/origin.py
213

roh, i forgot to drop this...
as i implemented it in the backend in the end.

olasd added a comment.Dec 6 2022, 4:05 PM

So, instead of adding more stuff to the grab_next_visits signature, I would have suggested just calling lister_get in swh/scheduler/cli/origin.py to get the lister uuid, and replacing the --lister-uuid CLI argument with --lister-name and --lister-instance-name (so, only changing the CLI function).

Does it seem like we're going to use these arguments in another caller of grab_next_visits?

ardumont added inline comments.Dec 6 2022, 4:08 PM
swh/scheduler/cli/origin.py
213

So, instead of adding more stuff to the grab_next_visits signature, I would have
suggested just calling lister_get in swh/scheduler/cli/origin.py to get the lister
uuid, and replacing the --lister-uuid CLI argument with --lister-name and
--lister-instance-name (so, only changing the CLI function).

lol, like this "forgotten dropped implem" ^ then (my first implem' and then somehow i
went too deep probalby due to tests...).

ardumont added a comment.Dec 6 2022, 4:09 PM

Does it seem like we're going to use these arguments in another caller of grab_next_visits?

That's a good question. I think not.

olasd added a comment.Dec 6 2022, 4:10 PM
In D8922#231994, @ardumont wrote:

Does it seem like we're going to use these arguments in another caller of grab_next_visits?

That's a good question. I think not.

Maybe we would, actually. For instance, it might make sense to use these options to give different scheduling weights for github/gitlab/... origins in the recurrent visit scheduler.

olasd added a comment.Dec 6 2022, 4:13 PM

Either way, the uuid argument in the cli endpoint should go away!

ardumont updated this revision to Diff 32147.Dec 6 2022, 4:20 PM

Drop --lister-uuid flag to the benefit of using --lister-name and --lister-instance-name

ardumont retitled this revision from Adapt send-to-celery cli to allow scheduling from simpler lister info to send-to-celery: Adapt to schedule from lister name & instance_name.Dec 6 2022, 4:20 PM
ardumont edited the summary of this revision. (Show Details)
swh-public-ci added a comment.Dec 6 2022, 4:24 PM

Build has FAILED

Patch application report for D8922 (id=32147)

Rebasing onto ff75e742ee...

Current branch diff-target is up to date.
Changes applied before test
commit a61b0e1e54bb7b3ce6ddeccb986e0270b860b277
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Dec 6 12:24:33 2022 +0100

    send-to-celery: Adapt to schedule from lister name & instance_name
    
    This allows to bypass the lister id retrieval step using directly the name and instance
    name of the lister to discover the uuid.
    
    This also drops the --lister-uuid flag which is somewhat difficult to use.
    
    Related to https://gitlab.softwareheritage.org/infra/sysadm-environment/-/issues/4674

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

Harbormaster failed remote builds in B33100: Diff 32147!Dec 6 2022, 4:24 PM
ardumont updated this revision to Diff 32149.Dec 6 2022, 4:54 PM

Fix tests

swh-public-ci added a comment.Dec 6 2022, 4:58 PM

Build is green

Patch application report for D8922 (id=32149)

Rebasing onto ff75e742ee...

Current branch diff-target is up to date.
Changes applied before test
commit a7769639df76bd2de93911cf10fb9e50d7d15b64
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Dec 6 12:24:33 2022 +0100

    send-to-celery: Adapt to schedule from lister name & instance_name
    
    This allows to bypass the lister id retrieval step using directly the name and instance
    name of the lister to discover the uuid.
    
    This also drops the --lister-uuid flag which is somewhat difficult to use.
    
    Related to https://gitlab.softwareheritage.org/infra/sysadm-environment/-/issues/4674

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

Harbormaster completed remote builds in B33102: Diff 32149.Dec 6 2022, 4:58 PM
ardumont added a child revision: D8928: grab_next_visits: Open lister name and instance name filtering.Dec 6 2022, 5:04 PM
ardumont added a comment.Dec 6 2022, 5:04 PM

Does it seem like we're going to use these arguments in another caller of grab_next_visits?

That's a good question. I think not.

Maybe we would, actually. For instance, it might make sense to use these options to give different scheduling weights for github/gitlab/... origins in the recurrent visit scheduler.

I've opened another diff for it [1]

[1] D8928

olasd accepted this revision.Dec 6 2022, 5:53 PM

Great, thanks!

This revision is now accepted and ready to land.Dec 6 2022, 5:53 PM
Closed by commit rDSCHa7769639df76: send-to-celery: Adapt to schedule from lister name & instance_name (authored by ardumont). · Explain WhyDec 6 2022, 6:12 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDSCHa7769639df76: send-to-celery: Adapt to schedule from lister name & instance_name.

Revision Contents
Changeset List

PathSize
swh/
scheduler/
cli/
19 lines
tests/
2 lines
9 lines
4 lines

Diff 32151

View Options

swh/scheduler/cli/origin.py

Loading...
View Options

swh/scheduler/tests/common.py

Loading...
View Options

swh/scheduler/tests/test_cli_origin.py

Loading...
View Options

swh/scheduler/tests/test_utils.py

Loading...
About · Developer information · Legal