Skip to content

Conversation

@jeremystretch
Copy link
Member

Closes: #19977

  • Adds _site, _location, and _rack ForeignKeys on all device component models
    • The included database migration automatically populates these new fields for existing objects
  • Introduce the handle_device_site_change() post_save receiver to automatically update child components when a device is modified
  • Update tests
    • Manually populate these fields where necessary when bulk_create() is employed
    • Tweak GraphQLTestCase to ignore private ForeignKey fields when constructing test queries

@jeremystretch jeremystretch requested review from a team and jnovinger and removed request for a team July 30, 2025 17:55
Copy link
Member

@jnovinger jnovinger left a comment

Choose a reason for hiding this comment

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

One question about the migrations and one (much less important) question about saving denormalized values via .save(). Please and thank you.

self._rack = self.device.rack

super().save(*args, **kwargs)

Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, any reason to not use the signal handler approach for this as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really. Maybe slightly cleaner, more direct?

_site=Subquery(subquery.values('site_id')[:1]),
_location=Subquery(subquery.values('location_id')[:1]),
_rack=Subquery(subquery.values('rack_id')[:1]),
)
Copy link
Member

Choose a reason for hiding this comment

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

Any concern here about how long these updates might lock up a whole table, particularly dcim_interface? I would imagine a series of batched updates would results more, but shorter locks, meaning that other operations on the tables can happen, if needed.

I'd be curious if you had a chance to run this against a DB with 1M+ interfaces.

Having said all that, I'm far from certain this is really an issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

During testing with ~500K interfaces, the migration took only a few seconds to run, so I'm not too concerned about it. It's a valid point, but I'm not sure how we'd meaningfully split up the query to batch it. I wanted to use subqueries to avoid handling any of the bulk data in Python.

@jnovinger jnovinger self-requested a review August 1, 2025 20:39
@jnovinger jnovinger merged commit aa9ee0e into main Aug 1, 2025
10 checks passed
@jnovinger jnovinger deleted the 19977-interface-denormalization branch August 1, 2025 20:40
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2025
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.

Cache the assigned site, location, and rack of device components

3 participants