Fix:cascade subjob failure - #2409
Open
BelhsanHmida wants to merge 12 commits into
Open
Conversation
Context: - Issue #2404: when a subjob in a sequential scheduling chain fails and its scheduler defines no fallback, the chain never reaches a terminal state. RQ only enqueues the dependents of a job that succeeded, so the remaining subjobs and the wrap-up job stay deferred forever. The wrap-up job's id is what POST /assets/<id>/schedules/trigger hands to the client, so that client never learns what went wrong. - This already happens for every scheduler without a fallback (the base Scheduler defaults fallback_scheduler_class to None), such as the ProcessScheduler and any custom scheduler. Change: - trigger_optional_fallback now handles any failure without a fallback job by failing the deferred jobs that depend on the failed job, recording an UpstreamSchedulingFailure that names the device that could not be scheduled. A failing fallback job cascades from the original job, whose dependents it was standing in for. - The wrap-up job of a sequential chain now tolerates a failing dependency, so it runs and fails with a message listing the devices that failed and the devices that were consequently never scheduled. - Split the fallback creation out of trigger_optional_fallback into _trigger_fallback_job, which reports whether a fallback job was created. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Context: - Issue #2404: a subjob failing under a scheduler without a fallback used to leave the rest of the chain deferred forever, so nothing proved that a client ever reaches a terminal state. Change: - test_create_sequential_jobs_without_fallback retires the storage fallback and makes the first device infeasible, then asserts that the second subjob and the wrap-up job both end up failed, that the wrap-up job's reason names the device that could not be scheduled, and that no job is left deferred. - test_asset_sequential_schedule_without_fallback_fails_terminally covers the same end-to-end: it triggers a sequential schedule over the API with a genuinely infeasible first device, polls the returned job id, and asserts a 422 whose message names that device. It also re-triggers the same request and asserts the job it gets back is not waiting on a chain that will never complete. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Context: - Issue #2404: the client-visible outcome of a failing sequential schedule changed, so both the general and the API change log need an entry. Change: - Added a bugfix entry describing the cascade, and an API change log entry stating the status codes and message a client now gets when polling a sequential schedule whose device could not be scheduled. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Context: - The background-job section walks a client through triggering and polling, including the failure path, but never says what happens when that client re-sends the request: schedule triggers are de-duplicated, so within FLEXMEASURES_JOB_CACHE_TTL the same failed job is handed back instead of a new attempt. - Issue #2404 makes this reachable in practice, since a failing sequential schedule now ends up in a terminal failed state that a client will want to retry. Change: - Added a "Retrying after a failed job" paragraph to the background job monitoring section, naming the config setting that governs the cache and showing the force-new-job-creation field that bypasses it. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
… job Context: - Marking the wrap-up job's dependency with Dependency(allow_failure=True) made RQ enqueue that job the moment the last subjob failed. When that subjob has a fallback scheduler, its fallback job is enqueued by the same failure, so both sit in the queue at once: with more than one scheduling worker, the wrap-up job can run while the fallback is still pending, see a subjob that has not (yet) produced a schedule, and report the chain as failed just before the fallback schedules the device after all. - A single worker pops the queue in order and happens to run the fallback first, which is why this did not show up in the tests. Change: - The wrap-up job depends on the last subjob plainly again, so RQ never enqueues it on failure and the fallback path behaves as it did before this branch. - A job that should run anyway is now marked in its meta data with RUNS_ON_CHAIN_FAILURE, and the cascade queues those jobs itself, after the rest of the chain has reached a terminal state. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Context: - The existing fallback test fails the first device, whose wrap-up job sits two dependencies away. Nothing covered a fallback for the last device, where the wrap-up job depends on the failing subjob directly, and where an eagerly queued wrap-up job would report the chain as failed while the fallback was still pending. Change: - Added test_create_sequential_jobs_fallback_for_last_device, asserting that the wrap-up job waits for the fallback job and finishes, rather than reporting a failure. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Context: - Naming the device that failed costs a database look-up, and the cascade now runs for every failed scheduling job, not just for an infeasible problem. A job that failed on a database error leaves the session needing a rollback, and nothing rolls it back between jobs, so that look-up raises. The failure callback would then abort before cascading, and the chain would wedge again -- precisely for the failures where it matters most. Change: - _describe_scheduled_device falls back to naming the device by its bare reference when the look-up raises a SQLAlchemyError, and logs a warning. Losing the device's name is a small price for still reporting the failure. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Context: - The first version of this test only checked the end state, which a single worker reaches correctly even when the wrap-up job is queued too early, because it pops the queue in order and runs the fallback job first. It therefore passed with the bug it was meant to catch still in place. Change: - The test now stops the worker right after the last subjob failed, and asserts that the wrap-up job is still deferred while its fallback job is queued. That is the invariant a second worker would break, and it does fail when the wrap-up job's dependency tolerates failure. - Also added a unit test for _describe_scheduled_device falling back to the bare reference when the session is unusable. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Context: - The entry was added before the PR existed, so it carried a placeholder number and also linked the issue, where entries in this changelog normally reference the PR only. Change: - Corrected the link to PR #2409, and dropped the issue reference. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Documentation build overview
39 files changed ·
|
Signed-off-by: Mohamed Belhsan Hmida <149331360+BelhsanHmida@users.noreply.github.com>
…back Context: - Merging main brought in PR #2252, which retires the storage fallback scheduler. Three things broke or went stale as a result. - test_create_sequential_jobs_without_storage_fallback, added by #2252, asserts that the deferred subjobs stay deferred, and clears them so they do not leak into the next test. It kept passing here only because its assertions are "not in finished_jobs", which a cascaded (failed) job also satisfies, so it silently asserted the opposite of what this branch establishes. - The fallback race test patched StorageFallbackScheduler, which #2252 deleted, so it failed on import. - The end-to-end API test relied on a soc-target the device could not reach, which #2252 turns into a priced breach rather than a failure, so the chain succeeded and the test no longer exercised a failure at all. Change: - Removed test_create_sequential_jobs_without_storage_fallback, which test_create_sequential_jobs_without_fallback supersedes with stronger assertions, along with the cleanup block the cascade makes unnecessary. - The fallback race test now makes the storage scheduler stand in as its own fallback, which is the situation a custom scheduler that defines one is still in. - The API test now provokes a genuine infeasibility with a soc-usage above the device's power-capacity, which stays a hard constraint, and both tests assert up front that the scheduler really has no fallback rather than patching one away. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <149331360+BelhsanHmida@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes #2404.
When a subjob in a sequential scheduling chain fails and its scheduler defines no fallback, the chain never reaches a terminal state. RQ only enqueues the dependents of a job that succeeded, so the remaining subjobs and the wrap-up job stay deferred forever. The wrap-up job's id is what
POST /assets/<id>/schedules/triggerhands to the client, so that client polls a job that will never move, and never learns what went wrong.failedstate, recording anUpstreamSchedulingFailurethat names the device which could not be scheduled.documentation/changelog.rstanddocumentation/api/change_log.rstdocumentation/api/introduction.rstScope after #2252
This branch was opened against
mainbefore #2252 (retiring the storage fallback scheduler) landed, because the bug did not depend on it:trigger_optional_fallbackonly creates a fallback job whenscheduler_class.fallback_scheduler_class is not None, and the baseSchedulerdefaults that toNone, soProcessSchedulerand every custom scheduler that did not opt in already wedged chains.Two smaller wedges are closed along the way. The old code only did anything at all for
InfeasibleProblemException, so any other failure (a bug, a database error) wedged the chain regardless of fallbacks; the cascade now runs for every failure. And a failing fallback job used to leave the original job's dependents deferred, which the fallback's ownsuccess_callbackwas the only thing that would have rescued.Now that #2252 has merged, no scheduler ships with a fallback at all, so this affects every sequential schedule whose devices are scheduled by a built-in scheduler. The merge is included in this branch, and three things were adjusted for it:
test_create_sequential_jobs_without_storage_fallback, which Feat/retire fallback scheduler #2252 added to assert the wedged behaviour (deferred subjobs stay deferred, plus a cleanup block to stop them leaking into the next test), is superseded bytest_create_sequential_jobs_without_fallbackhere and has been removed. Its assertions werenot in finished_jobs, which a cascaded (failed) job still satisfies, so it would have passed while asserting the opposite of the intended behaviour.StorageFallbackScheduler, which Feat/retire fallback scheduler #2252 deleted. It now makes the storage scheduler stand in as its own fallback, which is the situation a custom scheduler that defines one is still in.soc-usageabove the device'spower-capacity, which stays a hard constraint.Client-visible outcome
GET /api/v3_0/jobs/<uuid>on the returned job id now answers422 Unprocessable Entity:{ "status": "FAILED", "message": "Scheduling job failed with UpstreamSchedulingFailure: Sequential scheduling failed for sensor 15 (Test charging station - power): InfeasibleProblemException: infeasible. As a result, no schedule was computed for sensor 16 (Test charging station (bidirectional) - power).", "result": null }GET /sensors/<id>/schedules/<uuid>answersUNKNOWN_SCHEDULEwith the same reason. Polling a cascaded subjob directly gives that subjob's own terminal reason, naming the device that failed upstream.Notes for reviewers
Why not
Dependency(allow_failure=True)for the wrap-up job? That is the obvious RQ-native mechanism, and it is wrong here. RQ enqueues anallow_failuredependent the moment its dependency fails — but when the failing subjob has a fallback scheduler, the fallback job is enqueued by that same failure, so both sit in the queue at once. A single worker pops FIFO and happens to run the fallback first, which hides it; with more than one scheduling worker, the wrap-up job can run while the fallback is still pending, find a device without a schedule, and report the chain as failed moments before the fallback schedules that device after all. The wrap-up job therefore keeps a plain dependency (so RQ never enqueues it on failure, exactly as before this PR), and is marked in its meta data withRUNS_ON_CHAIN_FAILURE; the cascade queues such jobs itself, only after the rest of the chain has reached a terminal state.test_create_sequential_jobs_fallback_for_last_devicepins this invariant down by stopping the worker right after the failing subjob and asserting the wrap-up job is still deferred.This still matters after #2252: the fallback machinery remains for custom schedulers that define one (
FLEXMEASURES_FALLBACK_REDIRECTis documented as applying to exactly those), so the race would be reachable for them.Why does naming the device swallow
SQLAlchemyError? The cascade runs for every failure now, and naming the device costs a database look-up. A job that failed on a database error leaves the session needing a rollback, and nothing rolls it back between jobs, so that look-up raises — which would abort the failure callback before it cascades, and wedge the chain precisely for the failures where a terminal state matters most._describe_scheduled_devicetherefore falls back to the bare reference (sensor 4) and logs a warning. Losing the device's name is a small price for still reporting the failure.Out of scope
@job_cacheis left alone. Re-triggering an identical request now hands back the failed wrap-up job — terminal, with a reason — which is the same behaviour a failed single-sensor trigger already has, and satisfies the issue's requirement that a re-trigger must not hand back a wedged job. Getting a fresh attempt still needsforce-new-job-creation(now documented indocumentation/api/introduction.rst). Making the cache skip terminally-failed jobs would change behaviour for every trigger endpoint, which felt like a separate decision.Relatedly,
job_cache'srequeue=Truepath would requeue only the wrap-up job of a chain, which is wrong — but nothing in the API passes it, so that is pre-existing and untouched.Testing
test_create_sequential_jobs_without_fallback— makes the first device infeasible, asserts the second subjob and the wrap-up job both end up failed, that the reason names the device, and that nothing is left deferred. Replacestest_create_sequential_jobs_without_storage_fallback.test_asset_sequential_schedule_without_fallback_fails_terminally— the same end-to-end over the API, with a genuinely infeasible first device (no mockedcompute): triggers, polls the returned job id for a422naming the device, and re-triggers to check the job handed back is not waiting on a chain that will never complete.test_create_sequential_jobs_fallback_for_last_device— a fallback for the last device, whose wrap-up job depends on the failing subjob directly.test_describe_scheduled_device_survives_an_unusable_session— the device look-up degrades instead of raising.Sign-off