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

Optionally include configuration context when retrieving by name. #2350

Closed
ebusto opened this issue Aug 10, 2018 · 8 comments
Closed

Optionally include configuration context when retrieving by name. #2350

ebusto opened this issue Aug 10, 2018 · 8 comments
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application
Milestone

Comments

@ebusto
Copy link

ebusto commented Aug 10, 2018

Environment

  • Python version: 3.5.4
  • NetBox version: 2.4.3

Proposed Functionality

When using the API to retrieve a device or virtual machine by name, NetBox should support an optional query parameter to indicate that the configuration context will be included. This eliminates the double GET required when only the name is known, but not the ID.

Use Case

The Salt external pillar, as well as any other configuration management system that only has the hostname as a starting point.

Database Changes

None.

External Dependencies

None.

@jeremystretch
Copy link
Member

So, a little background on this. Currently, config context is rendered only when retrieving a single device (or VM). This is because retrieving all of the applicable ConfigContext objects and rendering a context for the specific device is a fairly expensive operation, and doing so for an entire list of devices would add a huge amount of overhead.

The REST API is structured such that retrieving any particular object must be done using its integer primary key (e.g. GET /api/dcim/devices/<pk>/). This is called the "detail" view in DRF vernacular. Retrieving a device by name (e.g. GET /api/dcim/devices/?name=mydevice) actually employs a filter and invokes the "list" view. That is, multiple devices will be returned if more than one matches the specified name string. Because the list view is used here, config context isn't rendered for the device(s).

So, I see two options for implementing this:

  1. Expand the detail view to allow retrieving a single device by name.

  2. Hack the list view to swap out the serializer if only one object was returned. I'm not sure how feasible this is, and it will likely cause other issues (with the API documentation, for instance).

@jeremystretch jeremystretch added the status: under review Further discussion is needed to determine this issue's scope and/or implementation label Aug 13, 2018
@ebusto
Copy link
Author

ebusto commented Aug 13, 2018

I'm not sure if it is easier, but a third option may be to expose a route which only returns the configuration context for the given device or VM. This would eliminate the database operations that occur when retrieving the entire host record.

@sdktr
Copy link
Contributor

sdktr commented Aug 13, 2018

Maybe adding a seperate route (or an argument to the existing routes) could make a 'rendered-config-context' available on other objects as well.

@cimnine
Copy link
Contributor

cimnine commented Aug 13, 2018

Hack the list view to swap out the serializer if only one object was returned. I'm not sure how feasible this is, and it will likely cause other issues (with the API documentation, for instance).

IMO this introduces unpredictable behavior. Simplest case against is that if anyone adds a second device with (partially) the same name (e.g. devicename -> devicename-staging), then there's a sudden change in behavior.

@lampwins
Copy link
Contributor

lampwins commented Aug 24, 2018

I think option one is the way to go, but what about also including a query parameter on the list view (e.g. ?include_config_context=1). The reason I throw that out there is that I (or rather my automation infrastructure) frequently builds query filters which result in only a handful of devices being returned (say 1-10). Yes, rendering multiple contexts is "costly" but having the option and simply knowing that the response may take a bit longer, is still powerful.

This could also be extended to the detail view in the form of ?include_config_context=0 for times when you don't need the context rendered, which is actually more frequent for me.

@cimnine
Copy link
Contributor

cimnine commented Aug 27, 2018

Yes, rendering multiple contexts is "costly" but having the option and simply knowing that the response may take a bit longer, is still powerful.

The client could simply limit the number of results he would like to receive to a low number, e.g. 5, for these specific requests. Thereby the client is in full control of the 'additional cost'.

@jeremystretch
Copy link
Member

The cleanest way to approach this might be to check for an exclude parameter specifying a comma-separated list of fields to exclude from the serializer output. For example:

GET /api/dcim/devices/?exclude=config_context

We should be able to hotwire the view to strip these fields before the serializer is rendered. This won't have any effect on the actual queryset used for the view, but it should help in cases like this, where config_context is generated by a serializer method called for each object.

The caveat with the approach (if proven feasible) would be that config_context is included by default: Users may experience a surprising drop in performance until they discover and adopt the new query parameter.

@lampwins lampwins added status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application API change and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Jan 11, 2019
@lampwins lampwins added this to the v2.6 milestone Jan 20, 2019
@lampwins
Copy link
Contributor

Jeremy's solution has been implemented in the develop-2.6 branch.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application
Projects
None yet
Development

No branches or pull requests

5 participants