Siyao Meng created HDDS-17390:
---------------------------------
Summary: ExportJobManager registers an export task only after
handing it to the worker, so cancelJob and the worker's cleanup can miss it
Key: HDDS-17390
URL: https://issues.apache.org/jira/browse/HDDS-17390
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: MC-1-register-export-task-before-job-is-visible.patch
h3. Mechanism
{{ExportJobManager.submitJob}} adds the new job to {{jobTracker}} and
{{jobQueue}} under the {{jobQueue}} lock, releases the lock, hands the task to
the single worker with {{workerPool.submit}} and only then stores the returned
{{Future}} in {{runningTasks}}. Two methods rely on that entry: {{cancelJob}}
removes it and calls {{cancel(true)}} to stop a QUEUED or RUNNING job, and
{{executeExport}} removes it in its {{finally}} block. Between the hand-off and
the registration, each can miss the other.
* A {{cancelJob}} in that interval finds no entry, so nothing cancels or
interrupts the task. It still marks the job FAILED with "Cancelled by user",
deletes the files the job has so far, removes the job from {{jobTracker}} and
returns normally. The task then runs unchanged: {{executeExport}} sets RUNNING,
writes the CSV files and the tar archive at the job's file path and sets
COMPLETED. The job is no longer in {{jobTracker}}, so no method of the manager
refers to that archive any more. It stays in the export directory until the
startup cleanup in the {{ExportJobManager}} constructor runs at the next Recon
start.
* If the worker runs the whole task, including the {{finally}} removal, before
{{submitJob}} reaches the registration, the registration adds an entry for a
task that has already finished. Nothing removes it later: {{cancelJob}} touches
{{runningTasks}} only for QUEUED or RUNNING jobs, {{shutdown}} does not clear
the map, and job IDs are random UUIDs that are never reused.
h3. Trigger
Both are interleavings inside {{submitJob}}, between {{workerPool.submit}} and
{{runningTasks.put}}:
# {{cancelJob}} for the new job runs in that interval, before the worker has
created the job directory ({{Files.createDirectories}} at the top of
{{executeExport}}). If the worker has already created the job directory, no
archive is left: {{cancelJob}} deletes that directory and then the tar path, so
the worker either fails on the missing directory or its finished tar is deleted
(code reading, not run).
# The worker runs the whole job before the submitting thread reaches
{{runningTasks.put}}. This requires the submitting thread to be descheduled for
the length of a complete export, for example a small one on a busy host.
Both windows are short. The tests below fix the order of the steps.
h3. Impact
* A {{cancelJob}} that returns normally does not stop the export. The job
object ends COMPLETED with the error message "Cancelled by user", and its tar
archive stays in the export directory with no job referring to it until Recon
restarts. The worker spends the full export time on the cancelled job, and jobs
submitted after it wait for it.
* Every occurrence of the second interleaving leaves one {{runningTasks}} entry
(a job ID and a finished {{FutureTask}}) for the life of the process. Nothing
reads it, so no caller gets a wrong result.
h3. Reproduction
Three regression tests in the attached patch, in the existing
{{TestExportJobManager}} ({{hadoop-ozone/recon}}), fail on unmodified source.
All three use reflection to replace the manager's worker pool with one that
runs each task on the submitting thread at the moment {{submitJob}} hands it
over. This fixes the order of steps that otherwise race between threads, and no
{{ExportJobManager}} code is changed. The database layer is the suite's Mockito
mock. This issue has no separate standalone reproduction; the regression tests
are the reproduction.
* {{cancelBeforeTaskReachesWorkerStopsExport}} calls {{cancelJob}} at the
hand-off and expects the export directory to stay empty. Without the change it
fails because the archive is there.
* {{taskFinishingBeforeSubmitReturnsIsNotLeftRegistered}} lets the task finish
before {{submitJob}} continues and expects {{runningTasks}} to be empty.
Without the change it fails because the finished task is still registered.
* {{cancelBeforeJobDirectoryExistsStopsEmptyExport}} covers the related case
described under Patch: it calls {{cancelJob}} from inside the running task for
a state with no rows, then recreates the job directory as if the cancel had
come before {{executeExport}} created it, and expects the export directory to
stay empty. Without the change it fails because the archive is there.
{noformat}
cancelBeforeTaskReachesWorkerStopsExport:
to be an empty directory but it contained:
[.../export_missing_1792751293225.tar]
taskFinishingBeforeSubmitReturnsIsNotLeftRegistered:
Expecting empty but was:
{"380046c3-9230-4b3a-92b3-a7937fa67a16"=FutureTask[Completed: null]}
cancelBeforeJobDirectoryExistsStopsEmptyExport:
to be an empty directory but it contained:
[.../export_missing_1792753300334.tar]
{noformat}
h3. Patch
[^MC-1-register-export-task-before-job-is-visible.patch], against
fb2d0310a9c6ea37c72cefb5cc1d5b4db35a7ab1. It also applies to master at
9c3e5477ffb.
{{submitJob}} now creates the {{FutureTask}} itself and stores it in
{{runningTasks}} inside the locked block, just before the job is added to
{{jobTracker}}. It hands the task to the worker with {{workerPool.execute}}
after the lock is released. So a {{cancelJob}} that finds the job also finds
its task: a task cancelled before it starts never runs, and a running task is
interrupted as before. The worker's {{finally}} removal always comes after the
registration. {{submit}} already wrapped the task in a {{FutureTask}}, so the
task runs exactly as before. Job states and the queue checks are unchanged.
The patch also adds the cursor loop's interrupt check once more, right before
{{Archiver.create}}. The CSV and tar writes do not observe an interrupt
({{Files.newOutputStream}} is not interruptible on current JDKs), so before
this check nothing stopped an interrupted export of a state with no rows. If
{{cancelJob}} interrupted such a task after it had started but before it had
created its job directory, the worker created the directory and wrote the
archive after {{cancelJob}} had deleted both paths, and the archive was left.
With the check, {{cancelJob}} interrupts before it deletes anything, so the
flag is already set when the worker reaches the check. The worker then marks
the job FAILED and deletes its files. Past the check, the deleted directory and
tar path stop the export as described under Trigger. What remains is that same
early window combined with a database driver that clears the interrupt flag
during the count query without failing (code reading, not run against a real
database).
With the patch, {{TestExportJobManager}} (17 tests) passes and checkstyle on
{{hadoop-ozone/recon}} is clean.
Found by TLA+ model checking and code review of the Recon unhealthy container
export job manager under HDDS-15926, on commit
fb2d0310a9c6ea37c72cefb5cc1d5b4db35a7ab1. Checked against HDDS issues and
apache/ozone pull requests for duplicates before filing. Related but not
duplicates: HDDS-14913 (introduced {{ExportJobManager}}), HDDS-17208 (retention
of SCM container ID export jobs, a different component). The attached patch is
a proposal for review. Generated with Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
