Phabricator: Prevent erroneous scheduler tasks disabling
ClosedPublic
Actions

Authored by anlambert on Sep 16 2019, 8:27 PM.

Details

Reviewers

ardumont

Group Reviewers

Reviewers

Maniphest Tasks

T2000: Phabricator lister erroneously delete some loading tasks

Commits

rDLS4c8d7baf7591: phabricator/lister: Prevent erroneous scheduler tasks disabling

Summary

This diff contains the set of fixes for the issues noticed while working on D1982.

Previously, the Phabricator lister was disabling some loading tasks while it was not
supposed to. More precisely, due to an invalid index provided to a database query,
the latest created scheduler task was disabled each time a new page of results was
provided to the lister by the Phabricator API. Moreover, database queries were not
filtered according to the Phabricator instance resulting in possible disabling of
scheduler tasks from other instances.

Closes T2000

Depends on D1984

Test Plan

New tests have been added for both task disabling cases.

I have the feeling that we could encounter such issues in other indexing listers.
Maybe the new tests could be moved in base testing class to check all listers behave
correctly regarding scheduler tasks creation.

Diff Detail

Repository

rDLS Listers

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

anlambert created this revision.Sep 16 2019, 8:27 PM

Harbormaster failed remote builds in B7803: Diff 6704!Sep 16 2019, 8:28 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tox/369/
See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tox/369/console

anlambert edited the summary of this revision. (Show Details)Sep 16 2019, 8:29 PM

anlambert added a parent revision: D1984: Phabricator: Fix get_next_target_from_response return type.

Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/370/ for more details.

Harbormaster completed remote builds in B7803: Diff 6704.Sep 16 2019, 8:32 PM

Thanks, sounds good.

I'll need another pass a tad later though ;)

swh/lister/phabricator/lister.py
98–118	I forgot we had that behavior...
152	Now i'm wondering if we don't need to fix that for the other "instance" aware lister (e.g gitlab, cgit, ...)

ardumont added inline comments.Sep 17 2019, 9:44 AM

swh/lister/core/tests/test_lister.py
116	Thanks btw, i did not that one (since 3.5) [1] I would have probably tried to use "side_effect" instead. [1] https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock

ardumont added inline comments.Sep 17 2019, 9:57 AM

swh/lister/phabricator/lister.py
98–118	I wonder if we need those cleanup actions... That complexifies and probably slows down the scheduler. My take is that we could simplify the `indexing_lister` and removes that method. Let the scheduler fails enough time (because the repo no longer exists) so that it disables the tasks. Thoughts?
152	Ok, i remember something changed in the data "model" translation for phabricator. We used to have the uid which includes the forge_url so that was unique. - 'uid': self.forge_url + str(repo['id']), + 'uid': url, We no longer have that since 87cec2f5c39e86e179189c8d0bbeabafe5b64537. On second though, i'm just being noisy, that should not change anything, shouldn't it? I expect url to be unique anyways.

ardumont added inline comments.Sep 17 2019, 10:06 AM

swh/lister/core/tests/test_lister.py
116	I did not know* that one
swh/lister/phabricator/lister.py
98–118	(Not necessarily for this diff though, let's discuss it ;)

douardda added a reviewer: Reviewers.Sep 17 2019, 10:10 AM

ardumont accepted this revision.Sep 17 2019, 10:56 AM

ardumont added a subscriber: olasd.

ardumont added inline comments.

swh/lister/phabricator/lister.py
98–118	Ok, so discussing with @olasd, i thought we had that behavior for the scheduler but in the end, it's only for the oneshot tasks and not the recurring ones. So in the end, right now, we cannot do as i proposed. Cheers,