Repository navigation
fix: follow the task TTL of the taskmaster job - #118
Merged
Merged
Conversation
The taskmaster's job now owns the task's ConfigMap, so that Kubernetes
deletes it with the job (e.g. after the TTL of tesk-core#109). The
job's UID is known only after it was created, so TESK API creates the
job first and the ConfigMap right after it, with the owner reference;
the job's pod waits for the ConfigMap it mounts. If the ConfigMap
can't be created, the job is deleted (background propagation) and the
error is passed on, so that a name collision still retries.
Taskmaster jobs with a deletion timestamp count as gone: GET
/tasks/{id} answers 404 and GET /tasks leaves them out.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review findings: with the job created first, a ConfigMap that Kubernetes refuses (e.g. above 1 MiB) or whose name is taken by a leftover ConfigMap failed only after the job existed. A dry run of the ConfigMap now catches these before the job is created; a taken name retries with a new one, as before. If the job can't be deleted after its ConfigMap failed, the request fails without a retry, so that no more jobs are left behind and none runs with another task's ConfigMap of the same name. Docs: delete on configmaps (OwnerReferencesPermissionEnforcement, e.g. OpenShift), phantom QUEUED tasks, short pages, upgrade steps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Round-2 review findings: the dry run of the ConfigMap had no owner reference, so it couldn't catch OwnerReferencesPermissionEnforcement refusing the owner (e.g. on OpenShift without delete on configmaps), and the job was created for nothing. The dry run now carries an owner reference to the job with a placeholder UID, which Kubernetes doesn't store. The log of a failed cleanup names the ConfigMap's error, too. Docs move the failure handling to its own section, and note that a job whose create request timed out after Kubernetes created it stays behind. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideImplementationCreate the taskmaster job before its ConfigMap, validate the ConfigMap with a placeholder owner reference, and clean up the job if real ConfigMap creation fails.
Add Kubernetes helpers for dry-run ConfigMap validation and background job deletion, and treat jobs with a deletion timestamp as absent from task reads and lists.
DocumentationExplain the revised task creation order, failure handling, task deletion lifecycle, and behavior for jobs being deleted.
Document the additional Kubernetes delete permissions required for job and ConfigMap cleanup.
Describe upgrade implications, required RBAC changes, and the new behavior for deleting tasks.
TestsTest that task creation validates ownership before creating the job, creates an owned ConfigMap afterward, and handles collisions and cleanup failures safely.
Test dry-run validation, background job deletion, and omission of jobs marked for deletion.
Update integration-test request stubs for URL path matching and verify dry-run and persisted ConfigMap owner references.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #118 +/- ##
==========================================
+ Coverage 64.05% 65.00% +0.94%
==========================================
Files 66 66
Lines 2579 2626 +47
Branches 268 272 +4
==========================================
+ Hits 1652 1707 +55
+ Misses 778 769 -9
- Partials 149 150 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Kubernetes always returns a job's metadata, and the code around it reads the metadata without a check; the branch was the only line of the patch without coverage. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
uniqueg
marked this pull request as ready for review
November 7, 2026 21:26
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/main/java/uk/ac/ebi/tsc/tesk/tes/service/TesServiceImpl.java" line_range="82-83" />
<code_context>
+ // a placeholder UID, which Kubernetes doesn't store
+ setOwner(taskMasterConfigMap, taskMasterJob, UUID.randomUUID().toString());
+ this.kubernetesClientWrapper.checkConfigMap(taskMasterConfigMap);
V1Job createdJob = this.kubernetesClientWrapper.createJob(taskMasterJob);
+ this.createOwnedConfigMap(taskMasterConfigMap, createdJob);
return this.converter.fromK8sJobToTesCreateTaskResponse(createdJob);
} catch (KubernetesException e) {
</code_context>
<issue_to_address>
**Task runs with another task's input**
When another actor creates a ConfigMap with the generated name after the dry run but before the real ConfigMap create, and the pod starts before cleanup removes it, `createOwnedConfigMap` encounters the name conflict after `createJob` has started a pod; the pod mounts the existing ConfigMap by name and can run with another task's input before cleanup removes it.
Gate the pod from processing until the ConfigMap created for its job is verified.
Also at `src/main/java/uk/ac/ebi/tsc/tesk/tes/service/TesServiceImpl.java:130`.
</issue_to_address>
### Comment 2
<location path="src/main/java/uk/ac/ebi/tsc/tesk/tes/service/TesServiceImpl.java" line_range="82" />
<code_context>
+ // a placeholder UID, which Kubernetes doesn't store
+ setOwner(taskMasterConfigMap, taskMasterJob, UUID.randomUUID().toString());
+ this.kubernetesClientWrapper.checkConfigMap(taskMasterConfigMap);
V1Job createdJob = this.kubernetesClientWrapper.createJob(taskMasterJob);
+ this.createOwnedConfigMap(taskMasterConfigMap, createdJob);
return this.converter.fromK8sJobToTesCreateTaskResponse(createdJob);
</code_context>
<issue_to_address>
**Timed-out creation leaves a waiting job**
When the job-create request times out or otherwise errors after Kubernetes has stored the job, `createTask` exits on the exception without creating the ConfigMap, while Kubernetes may already have stored the job; its pod then waits for the missing ConfigMap and the client never receives the task ID. Reconcile the ambiguous create outcome and either finish setup or remove the created job when safe.
Reconcile an ambiguous job-create result so a stored job either receives its ConfigMap or is safely removed.
</issue_to_address>
### Comment 3
<location path="src/main/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapper.java" line_range="94" />
<code_context>
+ */
+ public void checkConfigMap(V1ConfigMap map) {
+ try {
+ this.coreApi.createNamespacedConfigMap(namespace, map, null, DRY_RUN_ALL, null);
+ } catch (ApiException e) {
+ throw KubernetesException.fromApiException(e);
</code_context>
<issue_to_address>
**Task creation fails with webhook**
When the cluster has a matching admission webhook that rejects dry-run requests, `checkConfigMap` propagates the admission error as a `KubernetesException`, so `createTask` exits before creating the job and every task-creation request fails.
Handle rejection of dry-run requests by the matching webhook without aborting task creation.
</issue_to_address>
### Comment 4
<location path="src/main/java/uk/ac/ebi/tsc/tesk/tes/service/TesServiceImpl.java" line_range="141" />
<code_context>
+ e.getMessage(),
+ deleteError);
+ deleteError.addSuppressed(e);
+ throw deleteError;
+ }
+ throw e;
</code_context>
<issue_to_address>
**Cleanup conflict creates more jobs**
When deleting the job after ConfigMap creation fails returns HTTP 409, `createTask` catches the propagated delete error as a `KubernetesException`; because `isObjectNameDuplicated()` treats every HTTP 409 as a name collision, it retries even though cleanup failed. The caller can get an error after additional jobs are left behind.
Propagate cleanup failures in a form the outer retry handler cannot mistake for a job-name collision.
</issue_to_address>Linked issues
#117 fix: follow the task TTL in tesk-api · 3 of 3 met
| # | Requirement | Status | Notes |
|---|---|---|---|
| 1 | Give the task's ConfigMap an owner reference to the taskmaster job once the job exists. | ✅ Met | The ConfigMap is created after the job with an owner reference containing the created job's UID. |
| 2 | Treat taskmaster jobs with a deletion timestamp as gone in GET /tasks/{id} and GET /tasks. | ✅ Met | Single-task reads reject jobs being deleted, and task-list results filter them out. |
| 3 | Add regression tests and update docs for the task lifecycle and upgrade notes. | ✅ Met | Tests cover ownership and deletion-timestamp behavior; the docs describe task creation, deletion, and upgrade implications. |
Sourcery assessment
Needs a human reviewer. 4 findings to address first, and when a taskmaster job is deleted or its TTL expires, Kubernetes now also deletes the task's ConfigMap, which holds its inputs and task configuration. If that deletion is wrong, reverting won't restore the ConfigMap or its contents.
Blocking findings: src/main/java/uk/ac/ebi/tsc/tesk/tes/service/TesServiceImpl.java:83, src/main/java/uk/ac/ebi/tsc/tesk/tes/service/TesServiceImpl.java:82, src/main/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapper.java:94, src/main/java/uk/ac/ebi/tsc/tesk/tes/service/TesServiceImpl.java:141
Sourcery review findings: - An admission webhook for ConfigMaps that doesn't support dry runs makes Kubernetes refuse every dry run (400, "does not support dry run"), which failed every task creation. The check is now skipped with a warning in that case. - A 409 from deleting the job after a failed ConfigMap was taken for a name collision and created the task again. The cleanup failure is now an IllegalStateException (500), with the delete's error as its cause. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.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.
Summary
TESK API now follows the optional TTL of the taskmaster job (elixir-cloud-aai/tesk-core#109, part of elixir-cloud-aai/TESK#123).
taskmasterservice account. Its Role has nopatchonconfigmaps, but it already hasdeleteonjobs, which the new order needs.dryRun=All) checks the ConfigMap with a placeholder owner reference. That way an oversized ConfigMap, a missing right to set the owner (OwnerReferencesPermissionEnforcement, e.g. on OpenShift) and a name that's already taken all fail before anything exists. A taken name retries with a new one, as before.blockOwnerDeletionisn't set, so it needs no right on the job's finalizers.deletionTimestampcounts as deleted:GET /tasks/{id}(and cancel) answer404, andGET /tasksleaves it out.docs/index.md: the creation steps, a new section "Failures when creating a task", a new section "Deleting tasks", and a note on short pages.docs/deployment.md: the RBAC table addsdeleteonjobs, plusdeleteonconfigmapsforOwnerReferencesPermissionEnforcement.docs/upgrading.md: the new rights, the behaviour change, and that ConfigMaps from before the upgrade have no owner.Closes #117
Verification
./mvnw clean verify checkstyle:check(Temurin 8): 7363 unit tests and 77 integration tests pass, 0 Checkstyle violations;spotless:applywas run.mkdocs build --strict: clean.TesServiceImplCreateTestcovers:KubernetesClientWrapperTestcovers:readTaskmasterJobandlistAllTaskmasterJobsForUserwith a deletion timestamp;deleteJob(Background);checkConfigMap(dryRun=All).NoAuthITchecks against WireMock that both the dry-run request and the real one carry the owner reference, and that the real one names the created job's UID.adversarial-reviewerandquality-reviewer, 2 rounds):deleteright),OwnerReferencesPermissionEnforcement, and the docs. Fixed with the dry run, no retry after a failed cleanup, and the docs.codecov/patch, on an unreachable null check, which I removed. Iteration 2 passed every check. Iteration 3 (the Sourcery fixes): see the checks.Declined review findings
"The chart may not grant
deleteonjobs" (quality, round 1): checked. TESK'scharts/tesk/templates/common/taskmaster-rbac.yamlondevgrantscreate,delete,get,listandpatchonjobs, andcreate,deleteandgetonconfigmaps. TESK API's deployment uses that service account.Unit tests may not pin
BackgroundanddryRun=All(quality, round 2): they do, witheq("Background")andeq("All")inKubernetesClientWrapperTest.Deleting the job by name when
createJobtimes out after Kubernetes created it (adversarial, round 2, low): only documented. The error can't tell a timeout before the job was stored from one after it, and a delete by name could hit a job of the same name created by another request.Setting
activeDeadlineSecondson the taskmaster job, so a pod that never starts fails eventually (adversarial, round 1): not done. It would also limit how long a task may run, and that is the taskmaster's and the chart's decision, not part of this issue.A ConfigMap of the same name created by another actor between the dry run and the real create (Sourcery): not changed. The name is random, and the window lasts milliseconds. An actor that can create ConfigMaps in the namespace can already change any task's input. Gating the taskmaster would need tesk-core.
A timed-out job creation (Sourcery): documented, as above.
Open questions
None.
🤖 Generated with Claude Code