Raise TaskResourceNotFound instead of bare DoesNotExist in tasks - #2439
Raise TaskResourceNotFound instead of bare DoesNotExist in tasks#2439wussh wants to merge 2 commits into
Conversation
gerrod3
left a comment
There was a problem hiding this comment.
Not sold on the retrieve or raise logic/exception. The other builder exception looks good.
- error_code: use the CON00XX naming convention (CON0001, CON0002) jobselko and gerrod3 agreed on, instead of the ad-hoc PLP_CONTAINER_* strings. - CHANGES fragments: use single backticks, since they render as markdown. - cdomain_factory: adopt gerrod3's version of the namespace cleanup from pulp#2424 (suppress errors per-namespace/guard instead of letting one failure abort the rest, and wait on the delete task via monitor_task).
|
Addressed:
@jobselko the duplicated @gerrod3 re: "not sold on the retrieve or raise logic/exception" — could you elaborate on what you'd prefer instead? Happy to rework |
pulpcore's task executor treats any task exception that isn't a PulpException subclass as deprecated: it logs a pulpcore.deprecation warning today and will sanitize the message away entirely in pulpcore 3.130. Every task that looks up its arguments' referenced objects by pk (repository, remote, manifest, signing service, artifact, ...) hit this whenever that object was deleted between dispatch and execution. Add TaskResourceNotFound(PulpException) and wrap each top-of-task object lookup in an explicit try/except, mirroring how the digest-fetch build errors are handled rather than introducing a generic helper (per review feedback: a generic get_task_resource_or_raise wrapper was rejected in favor of explicit per-call-site handling). Leaves the intentional try/except DoesNotExist in builder.py's get_or_create_blob alone, since that's expected control flow, not an unhandled lookup failure.
2a028a6 to
517637e
Compare
|
@gerrod3 @jobselko rebased this branch directly onto `main` and re-did the fix as a single commit (517637e), which resolves both open items:
Force-pushed the rebased branch just now — CI is green apart from the known flaky azure job and an infra-only podman config error on lowerbounds, both unrelated to this diff. |
Summary
PulpExceptionsubclass as deprecated: it logs apulpcore.deprecationwarning today and will sanitize the message away entirely in pulpcore 3.130.DoesNotExistwhen that object was deleted between dispatch and execution. This is non-deterministic — a different one surfaces on each CI run depending on test/teardown timing (observed bothManifestSigningServiceandContainerDistributionDoesNotExistwarnings on unrelated PRs' CI, e.g. Fix tag-based manifest pulls 404ing while digest-based pulls succeed #2438).TaskResourceNotFound(PulpException)and aget_task_resource_or_raise()helper (app/utils.py), used at the top-of-task object lookups intag.py,untag.py,recursive_add.py,recursive_remove.py,synchronize.py,download_image_data.py,sign.py, andbuilder.py.try/except DoesNotExistinbuilder.py'sget_or_create_blobalone — that's expected control flow (create-on-miss), not an unhandled lookup failure.deprecations-noise fixes so this PR is self-contained (same root cause class — non-PulpExceptionraised in a task — found while chasing Domain-scoped container registry: tag-based manifest fetch 404s, digest-based works (pulpcore 3.113.0 / pulp_container 2.28.0) #2417's CI):builder.py's podman build/push failure now raisesContainerBuildError(PulpException)instead of bareException, andtest_domains.py'scdomain_factoryfixture now cleans upContainerNamespace(aPROTECTFK toDomain) before teardown deletes the domain, instead of leaving it to raiseProtectedError.Not addressed (out of scope for this PR): pulpcore's own generic tasks (e.g.
general_multi_delete, which can raiseContainerDistribution.DoesNotExist) are outside pulp_container's control, so thedeprecationsCI job may still show occasional pulpcore-origin warnings even after this merges. This is a noise-reduction / forward-compat cleanup for pulp_container's own task code, not a guarantee of a fully greendeprecationsjob.Test plan
ruff check/ruff format --checkclean on all touched files.Model.objects.get(pk=...)task-entry lookups were missed inapp/tasks/.deprecationsjob re-checked, remaining warnings confirmed unrelated to this PR's changes.