Do not build a Job out of a result that is not a job id - #100
Open
eschultz wants to merge 1 commit into
Open
Conversation
With new-style jobs the server does not answer a job method with its id. It returns the method's real result when the job completes, and the client learns the id from a core.get_jobs event: _process_message matches the event's message_ids against the pending call and substitutes the id into call.result, marking the call returned early. Nothing emits that event for a method that is not a job, or for a job declared transient=True, so the call instead returns the method's own result. wait() then built a Job around that result and Job.result() waited on an Event with no timeout, for an id the server never issued, so it never returned. midclt call -j pool.scrub hangs this way, as does job=True on any non-job method such as vm.start. Record on the Call whether an event bound it to a job and branch on that rather than on the shape of the result. When nothing bound it the call has already returned the method's own result, so return it; job='RETURN' raises, since there is no job object to hand back. The legacy-jobs path, where the server does answer with the id as the plain result, is unchanged.
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
1 participant
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.
Related ticket: https://ixsystems.atlassian.net/browse/NAS-143328 (the server half is
pool.scrubbeing a transient job; this half is independent of it).Problem
With new-style jobs the server does not answer a job method with its id. It returns the method's real result when the job completes. The client learns the id from a
core.get_jobsevent:_process_messagematches the event'smessage_idsagainst the pending call and substitutes the id intocall.result, marking the call returned early.Nothing emits that event for a method that is not a job, or for a job declared
@job(transient=True)—send_job_event()in middleware returns early for those. The call then stays pending until the work finishes and receives the method's own result.wait()didJob(self, c.result)unconditionally, andJob.result()waits on anEventwith no timeout, for a job id the server never issued. It never returns.Reproduces as
midclt call -j pool.scrub <id> STARThanging until its timeout while the scrub finishes in about a second, and equally asjob=Trueon any non-job method such asvm.startorvm.delete.Change
Record on the
Callwhether acore.get_jobsevent bound it to a job, and branch on that inwait()rather than on the shape of the result.When nothing bound it, the call has already returned the method's own result, so return that.
job='RETURN'raisesClientException, since there is no job object to hand back.Branching on an explicit marker rather than sniffing the result's type matters: a job id is an integer, but so are plenty of ordinary results, so a type check would still build a bogus
Jobfor those and keep hanging.The legacy-jobs path is unchanged. Against a server that does not negotiate
legacy_jobs, middleware returns the job id as the call's plain result and_new_style_jobsstaysFalse, so that branch still usesc.resultas the id.Tests
tests/test_wait_non_job.py, following the bare-client pattern already used bytest_job_disconnect.py. Covers a non-job method, a transient job, an integer result that is not a job id,job='RETURN', call unregistration, the ordinary trackable-job path, and legacy mode.Without the change the suite hangs rather than failing, which is the bug.