Skip to content

Closes #12135: Prevent the deletion of interfaces with children #14091

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

Merged
merged 8 commits into from
Nov 1, 2023

Conversation

jeremystretch
Copy link
Member

@jeremystretch jeremystretch commented Oct 20, 2023

Fixes: #12135

  • Change on_delete behavior for parent field Interface and VMInterface from SET_NULL to RESTRICT
  • Update UI & API views to cleanly handle RestrictedError exceptions (the same way as ProtectedError)
  • Update BulkDeleteView to perform all deletions within an atomic transaction
  • Add get_bulk_update_queryset() and get_bulk_destroy_queryset() methods to API viewset mixins to allow overriding the default querysets for bulk operations
  • Update the bulk deletion querysets for device & VM interfaces to order objects by parent, to ensure children are deleted before their parents
  • Add tests for deleting parent interface individually as well as both parent and child interfaces, for both devices and VMs

@kkthxbye-code
Copy link
Contributor

Feedback from the issue thread:

[with this PR you] can delete a device if it doesn't have any virtual interfaces but not if it has any. Seems inconsitent and confusing. Why are child interfaces more important than parent interfaces when deleting an entire device?

The same applies for modules and while not sure how common; entire racks/sites as well.

@jeremystretch
Copy link
Member Author

[with this PR you] can delete a device if it doesn't have any virtual interfaces but not if it has any.

@kkthxbye-code Could you please elaborate on your reason for downvoting this PR? Is your opposition limited to the need to work around this limitation when deleting devices/VMs?

@kkthxbye-code
Copy link
Contributor

Is your opposition limited to the need to work around this limitation when deleting devices/VMs?

Mostly yes. If it can be limited to only applying when deleting the parent interface, I think it at least not majorly impacts usability.

Do note that the current implementation also breaks bulk deletion of interfaces if any of the devices selected has a parent and the parent is also included in the list.

image

image

@jeremystretch
Copy link
Member Author

jeremystretch commented Oct 20, 2023

I think we can use RESTRICT instead of PROTECT here to still allow the automatic deletion of a parent interface when the parent itself is being deleted via a cascading deletion.

@jeremystretch jeremystretch added the beta Concerns a bug/feature in a beta release label Oct 20, 2023
@jeremystretch
Copy link
Member Author

jeremystretch commented Oct 20, 2023

We can mostly work around the bulk deletion issue by ordering interfaces by parent (nulls last), to ensure all child interfaces in the list are deleted first. However, this doesn't address the (presumably) rare case where there are multiple levels of child assignments (e.g. A -> B -> C). (The workaround for the user in this case would be to manually delete the grandchildren first.)

Another tact would be to sort interfaces to be deleted in reverse order by primary key, as we'd typically expect child interfaces to always be created after their parents. However, there exists a corner case with this approach too, where a new interface is created and then an existing child interface is assigned to it.

Combining both approaches (by overriding get_queryset() on the appropriate views might be sufficient, but I'm open to other ideas.

@jeremystretch jeremystretch removed the beta Concerns a bug/feature in a beta release label Oct 23, 2023
@jeremystretch jeremystretch added this to the v3.7 milestone Oct 23, 2023
@jeremystretch jeremystretch merged commit 944008d into feature Nov 1, 2023
@jeremystretch jeremystretch deleted the 12135-protect-child-interfaces branch November 1, 2023 17:47
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants