Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(tenants): tenant cleanup #775

Merged

Conversation

lusergit
Copy link
Collaborator

Closes #504.
Manages resource cleanup on tenant deletion, deleting base images, ephemeral images and system models images on remote storage.

@lusergit lusergit requested a review from noaccOS December 12, 2024 16:57
@coveralls
Copy link

coveralls commented Dec 12, 2024

Pull Request Test Coverage Report for Build 83a215a0aeb5ccb5c8de527747f9eeb256e6f32a-PR-775

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 78.387%

Files with Coverage Reduction New Missed Lines %
lib/edgehog/tenants/tenant/tenant.ex 2 60.0%
Totals Coverage Status
Change from base Build e25b04d0901a73c89d3b135e2715dbe5e3bc9fc4: 0.2%
Covered Lines: 1244
Relevant Lines: 1587

💛 - Coveralls

Comment on lines 66 to 46
OTAOperation
|> Ash.Query.filter(tenant_id == ^tenant.tenant_id)
|> Ash.Query.filter(manual?)
|> Ash.read!(tenant: tenant)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep in mind that inside the Ash.Changeset.after_action logic the multitenant resources are already deleted, thanks to the cascade deletes and the tenant being deleted.
We could either:

  • verify that Ash automagically calls the destroy actions on resources instead of relying on the cascade deletes, and we add Changes to the destroy actions that try to cleanup the resources from external systems.
  • read the resources before the after_action callback, and then call directly the modules (e.g. @ephemeral_image_module.delete) to cleanup the external systems.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the first point, because of the way we set up the deletion (using ash_postgres references) ash does not automatically call the destroy actions of associated resources (see ash_postgres documentation). I think the second route is our best option, as it allows to handle the deletion directly in the resources (with a before_action) that triggers the external sotrages behavior only after the transaction if successful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the commit with this approach, the resources get deleted even on external storage (see tenants_test)

@lusergit lusergit force-pushed the chore/tenant-cleanup branch 3 times, most recently from 0f0e644 to 08747b1 Compare January 8, 2025 08:49
Closes edgehog-device-manager#504.
Manages resource cleanup on tenant deletion, deleting base images,
ephemeral images and system models images on remote storage.

Signed-off-by: Luca Zaninotto <[email protected]>
@lusergit lusergit force-pushed the chore/tenant-cleanup branch from 08747b1 to 83a215a Compare January 9, 2025 11:57
@davidebriani davidebriani merged commit 5eb5288 into edgehog-device-manager:main Jan 13, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reimplement tenant cleanup using Ash
3 participants