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

6347 Cache the number of each component type assigned to devices/VMs #12632

Merged
merged 48 commits into from
Jul 25, 2023

Conversation

arthanson
Copy link
Collaborator

@arthanson arthanson commented May 16, 2023

Fixes: #6347

Adds counter fields to cache related model counts. Does this by adding a CachedCounterField type that hooks up post-save and post-delete signals on the related model, these hooked up by calling connect_counter in apps.py

also has change tracking, there is a TrackingModelMixin which I added to the base classes - by default it does nothing but if you set change_tracking_fields on the model to the fields to track it will do change tracking on them. The connect_counter method automatically does the correct fields (generally device_id) and then checks this on post_save signal to see if it has been moved and updates the counts on the old and new parent models appropriately.

The change tracking is probably useful elsewhere - you can check model.tracker.changed to see any changed fields. Note: these fields must be listed in change_tracking_fields.

@arthanson arthanson changed the title DRAFT: 6347 cache counts DRAFT: 6347 Cache the number of each component type assigned to devices/VMs May 16, 2023
@kkthxbye-code
Copy link
Contributor

I know this is a draft, so apologies if this is premature. Looking at the code it doesn't look like it currently take into account that InventoryItems can be moved between devices.

It might also be an idea to add a management command to re-calculate counts, if they ever get out of sync.

@arthanson arthanson changed the title DRAFT: 6347 Cache the number of each component type assigned to devices/VMs 6347 Cache the number of each component type assigned to devices/VMs May 18, 2023
@arthanson arthanson marked this pull request as ready for review May 18, 2023 16:25
@arthanson
Copy link
Collaborator Author

@kkthxbye-code could definitely use a more thorough review on this one, any feedback welcome.

@jeremystretch jeremystretch added the beta Concerns a bug/feature in a beta release label Jun 20, 2023
netbox/utilities/counter.py Outdated Show resolved Hide resolved
netbox/utilities/mixins.py Outdated Show resolved Hide resolved
netbox/utilities/fields.py Show resolved Hide resolved
Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

Excellent work! Just working on simplifying and cleaning things up a bit at this point. Also, could you add some simple tests for the counters? A single relationship (e.g. device interfaces) should be sufficient.

netbox/utilities/counter.py Outdated Show resolved Hide resolved
netbox/utilities/mixins.py Outdated Show resolved Hide resolved
netbox/utilities/mixins.py Outdated Show resolved Hide resolved
netbox/utilities/counter.py Outdated Show resolved Hide resolved
netbox/utilities/mixins.py Outdated Show resolved Hide resolved
netbox/utilities/mixins.py Outdated Show resolved Hide resolved
netbox/utilities/mixins.py Outdated Show resolved Hide resolved
@arthanson
Copy link
Collaborator Author

@jeremystretch I added auto-discovery and cleaned up how you register / connect the fields, now you just supply it as params to the field:

    _interface_count = CounterCacheField(to_model='virtualization.VMInterface', to_field='virtual_machine')

and in apps.py ready function call:

        connect_counters(self)

I think this solves your issues with auto-discovery and the app registry? Please let me know if any additional changes.

It would be nice if there was a global app init, then could just run through all models and wouldn't have to remember to put connect_counters in each application apps.py.

Should I document using this in the models doc? Sort of an internal thing so not sure if this should go into the docs, but does effect people if they want to use this and add a field that should be cached.

@jeremystretch jeremystretch merged commit 149a496 into feature Jul 25, 2023
8 checks passed
@jeremystretch jeremystretch deleted the 6347-cache-counts branch July 25, 2023 13:39
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
beta Concerns a bug/feature in a beta release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants