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

Disallow deletion of non-orphaned entities #8616

Closed
Rugvip opened this issue Dec 23, 2021 · 6 comments
Closed

Disallow deletion of non-orphaned entities #8616

Rugvip opened this issue Dec 23, 2021 · 6 comments
Labels
area:catalog Related to the Catalog Project Area enhancement New feature or request help wanted Help/Contributions wanted from community members needs discussion Bring up for discussion during next sync stale

Comments

@Rugvip
Copy link
Member

Rugvip commented Dec 23, 2021

Feature Suggestion

The catalog currently allows deletion of non-orphaned entities, an orphaned entity being one that is not referenced by any other entity and is usually safe to delete. Deleting a non-orphaned entity is really only valid in the case where an entity somehow gets stuck in some way, so it's basically a way to work around bugs in the catalog. The trouble with allowing deletions of non-orphaned entities is that due to caching in the catalog processing, they will remain deleted until something invalidates the cache, which can be tricky to figure out how to do and is not a self-healing problem.

We're thinking that allowing deletion of non-orphaned entities is currently worse than not allowing it, as it's causing quite a lot of confusion. It's arguably more correct to not allow deletion, and if there are any cases where entities get stuck, we can focus efforts towards resolving those bugs instead.

Possible Implementation

We're happy to have people from the community chip in on this one, so let us know if you want to take a stab at it and we can provide additional pointers as needed

The main change will be to update removeEntityByUid to reject the deletion if an entity isn't orphaned. We won't want to rely on the orphan annotation at that point though, as that's not the source of truth. We'll instead want to check the incoming references, just like the Stitcher does when it populates the orphan annotation.

With that in place we'll likely hit some issues though, as we now start rejecting deletions that aren't safe. This is particularly likely to break the location unregistration, which assumes that it is safe to optimistically delete all entities referenced by the location. Most likely the best way to solve this is to move the eager deletion to the backend instead, but to be honest it might already be the case that this happens already and just needs a bit of verification 😁

Context

Plenty of reports through Discord of deleted entities not reappearing

Requests such as #8419

Automated cleanup is another way to go about deletions, but we'll likely want a bit of both #7860

@Rugvip Rugvip added enhancement New feature or request area:catalog Related to the Catalog Project Area help wanted Help/Contributions wanted from community members labels Dec 23, 2021
@goenning
Copy link
Contributor

goenning commented Dec 23, 2021

Ah, so that's why! This happened so many times before while testing things out and never really understood why that happens. How does one invalidates the cache? Just so I know what to do next time this happens.

@Rugvip
Copy link
Member Author

Rugvip commented Dec 27, 2021

@goenning anything that fails this check in the processing engine for the parent location. The deferredEntities entities is that typically the json object of each of the documents in catalog-info.yaml, so changing the contents of the yaml file should invalidate the cache. If you want to poke at the DB then clearing the result hash of the parent location would be a way to go about it.

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added stale and removed stale labels Jul 14, 2022
@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 13, 2022
@benjdlambert benjdlambert added needs discussion Bring up for discussion during next sync and removed stale labels Sep 19, 2022
@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Nov 18, 2022
@Rugvip Rugvip reopened this Dec 8, 2022
@github-actions github-actions bot removed the stale label Dec 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Feb 6, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:catalog Related to the Catalog Project Area enhancement New feature or request help wanted Help/Contributions wanted from community members needs discussion Bring up for discussion during next sync stale
Projects
None yet
Development

No branches or pull requests

3 participants