Raise TaskResourceNotFound instead of bare DoesNotExist in tasks - #2439
Open
wussh wants to merge 4 commits into
Open
Raise TaskResourceNotFound instead of bare DoesNotExist in tasks#2439wussh wants to merge 4 commits into
wussh wants to merge 4 commits into
Conversation
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 — non-deterministic, so a different one surfaces on each CI run (observed both ManifestSigningService and ContainerDistribution DoesNotExist warnings on unrelated PRs). Add TaskResourceNotFound(PulpException) and a get_task_resource_or_raise() helper in app/utils.py, and use it at the top-of-task object lookups in tag.py, untag.py, recursive_add.py, recursive_remove.py, synchronize.py, download_image_data.py, sign.py, and builder.py. 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. Note: pulpcore's own generic tasks (e.g. general_multi_delete, which can raise ContainerDistribution.DoesNotExist) are outside pulp_container's control and are not addressed by this change.
…nrelated to this diff)
pulpcore treats any task exception that isn't a PulpException subclass as deprecated: it logs a pulpcore.deprecation warning today and will sanitize the error message away entirely in pulpcore 3.130, losing the actual podman/buildah stderr. build_image_from_scratch and the deprecated build_image_from_containerfile both raised bare Exception on a failed podman build/push, tripping this path — surfaced as CI noise in the "deprecations" check, but a real forward-compat bug. Add ContainerBuildError(PulpException) and raise it with the decoded stderr instead; pytest.raises(PulpTaskError) assertions in test_build_images.py are unaffected since the exception still propagates through task.error the same way.
ContainerNamespace has a PROTECT foreign key to Domain. cdomain_factory's teardown cleared content-redirect guards but never ContainerNamespace objects, so any domain-scoped ContainerDistribution left one behind (auto-created by ContainerNamespaceSerializer.get_or_create). When pulpcore's own domain_factory teardown then deleted the domain, Django raised ProtectedError — sanitized into a pulpcore.deprecation warning that failed the "deprecations" CI check. test_pull_in_domain creates two distributions and cleaned up neither namespace, making it the concrete trigger. Query-and-delete namespaces by domain in cdomain_factory's teardown instead of relying on each test to remember; safe to run even for tests that already clean their own namespace via add_to_cleanup since it re-lists current state.
gerrod3
requested changes
Jul 28, 2026
gerrod3
left a comment
gerrod3
left a comment
Contributor
There was a problem hiding this comment.
Not sold on the retrieve or raise logic/exception. The other builder exception looks good.
| @@ -0,0 +1 @@ | |||
| Raised a proper ``PulpException`` subclass instead of a bare ``Exception`` when building or pushing an OCI image fails, so the error is not sanitized away by pulpcore in a future release. | |||
Contributor
There was a problem hiding this comment.
The changelogs are rendered in markdown so use single ticks.
| yield _domain_factory | ||
|
|
||
| for domain in domains: | ||
| # ContainerNamespace has a PROTECT FK to Domain, so it must be cleared before |
Contributor
There was a problem hiding this comment.
I would copy the fix I am adding here: https://github.com/pulp/pulp_container/pull/2424/changes#diff-5287a8b917dfa3076a52a1b5bdfc8ce254b5d74ecbddf6ad08179904c5e91649R30
| class ContainerBuildError(PulpException): | ||
| """Exception to signal that building or pushing an OCI image failed.""" | ||
|
|
||
| error_code = "PLP_CONTAINER_BUILD_0001" |
Contributor
There was a problem hiding this comment.
@jobselko What did we decide for the nomenclature of error codes?
Member
There was a problem hiding this comment.
3 uppercase letters/_ , followed by 4 leading-zero digits, so CON0001 for pulp container
Member
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
3 participants
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
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.