Follow-up from "Fix cross modification table for GroupDestroyWorker"
The following discussion from !80152 (merged) should be addressed:
-
@ayufan started a discussion: (+9 comments)
The idea is that the objects that needs to be non-trivial deleted should be ensured that they are deleted ahead of time. The cases where this is necessary are objects having attached external resources (like object storage). Application should understand which objects are non-trivial and should ensure, propagate up that resource is properly cleaned-up when parent is being removed (like project/namespace or just build). Application should ensure that such objects are .destroyed
and not .deleted
. Deleted means remove without regards to the attached resources.
We need to figure models that have external attachments, propagate tree to ensure that proper dependent:
(destroy or restrict with error, or allow to have if fast destroy is used) up to the top-item (project/namespace) and likely do it as part of testing suite.
So, by following this:
- since Job Artifacts belongs to build or project
- the build when
.destroy
should have job artifacts restrict_with_error since we should ensure that data is deleted before - the same goes for project on
job_artifacts
- now, given that job artifacts is present on projects, we don't need to require builds to fail since this is direct relation, but if this would be indirect we should
Likely this should be truly automated, since at least in above cases I see that we are very likely leaking data, ex. secure files.
Today, I do see that we likely do not properly ensure that attached resources are deleted in all cases at least for:
ci_job_artifacts
ci_build_trace_chunks
ci_secure_files
- etc.
I think we should not remove dependent: :destroy
for non-trivial objects that needs a cascade destroy. Rather such objects should be removed explicitly ahead of time, and dependent: :restrict_with_error
should be used to ensure that we forbid .destroy
if something is not cleaned-up.