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

Pre-generate the OpenAPI schema #6423

Closed
candlerb opened this issue May 15, 2021 · 10 comments
Closed

Pre-generate the OpenAPI schema #6423

candlerb opened this issue May 15, 2021 · 10 comments
Assignees
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application

Comments

@candlerb
Copy link
Contributor

candlerb commented May 15, 2021

NetBox version

v2.11.3

Feature type

Change to existing functionality

Proposed functionality

A feature like "collectstatic" which builds the openapi schema once in ./upgrade.sh, then serves it as a static file.

Currently it's served dynamically and is very slow, taking consistently 7-8 seconds on my system:

# time curl -sS localhost:8001/api/docs/?format=openapi | wc
      0   78785  799797

real	0m7.805s
user	0m0.016s
sys	0m0.005s

Alternatively, cache it in redis (it will take nearly a megabyte of RAM, but worth it)

Use case

ansible netbox inventory

The ansible netbox inventory fetches the whole openapi schema each time it runs. When combined with the actual data queries it does for devices, vms etc, this gives around a 10-second lead time to any playbook which is hitting the netbox API.

Whilst this inventory module has a feature to cache the results of the inventory (if you configure a suitable cache plugin), I want to get the "real-time" up-to-the-minute data from Netbox. The plugin can't be configured to cache the openapi schema only, whilst still making data queries to Netbox.

pynetbox

Any code which uses pynetbox, and calls nb.openapi(), will also benefit.

import pynetbox
nb = pynetbox.api(API_URL, token=API_TOKEN)
nb.openapi()

Database changes

None

External dependencies

None

@candlerb candlerb added the type: feature Introduction of new functionality to the application label May 15, 2021
@mraerino
Copy link
Contributor

This really got worse on 2.11 - my webserver even times out while generating the spec.

Is there a console command you can run to get the openapi json?

@jeremystretch
Copy link
Member

This really got worse on 2.11

I haven't noticed any difference from v2.10, and I can't recall any changes we've made that would have a significant impact on it.

@jeremystretch jeremystretch added the status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation label May 17, 2021
@jeremystretch jeremystretch changed the title Pre-generate the openapi schema Pre-generate the OpenAPI schema May 20, 2021
@sdktr
Copy link
Contributor

sdktr commented Jun 2, 2021

While serving a static version of the spec seems harmless since it's version bound anyway, I'm not sure that it makes sense to request the complete schema each time the plugin starts? No matter how fast netbox spits out the file, is that a roundtrip we can avoid?

@candlerb
Copy link
Contributor Author

candlerb commented Jun 2, 2021

There are two ways to bite the cherry.

  1. At the server end: pre-generate the spec and serve it statically
  2. At the client end: read the spec once and cache it

Whilst both are possible, (1) gives the maximum benefit:

  • It only has to be implemented once
  • It benefits all clients, even dumb ones
  • Clients don't have to worry about running with a stale copy of the spec when Netbox changes
  • TBH, transferring 1MB over the network is cheap these days

Ideally, Netbox would pregenerate the spec and give an ETag - then smart clients can check and don't have to refetch unless it has changed.

@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. NetBox is governed by a small group of core maintainers which means not all opened issues may receive direct feedback. Please see our contributing guide.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Sep 11, 2021
@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation pending closure Requires immediate attention to avoid being closed for inactivity labels Oct 1, 2021
@jeremystretch jeremystretch self-assigned this Oct 1, 2021
@jeremystretch
Copy link
Member

I don't think we can pre-generate the full API spec as part of the release process, given that it contains some values unique to the installation (i.e. the base API URL). We also can't generate this at the time of installation because such values may change as the configuration is modified (e.g. if BASE_PATH is changed).

What we can do is specify a cache timeout for the rendered page. This isn't perfect, but it should at least greatly reduce load times for bringing up the API docs. I'm going to try a 24-hour timeout for the rendered specs.

@candlerb
Copy link
Contributor Author

candlerb commented Oct 1, 2021

We also can't generate this at the time of installation because such values may change as the configuration is modified (e.g. if BASE_PATH is changed).

I think it would be reasonable to require the user to re-run some command like manage.py genapi if the BASE_PATH is changed (but to have it run automatically on ./update.sh, like collectstatic is)

@jeremystretch
Copy link
Member

It would be very difficult to convey that to the user and ensure that it's undertaken consistently. IMO it's not worth the additional overhead for such a minor impact.

@jeremystretch
Copy link
Member

IMO this is a reasonable optimization that avoids introducing any additional overhead. Just wanted to make some progress on this as no one else has volunteered to own this.

I'm happy to consider further improvements, though it's worth noting that we're somewhat limited by the capabilities of the drf-yasg package. Any suggestions for improvements to the actual spec rendering should be made upstream.

@candlerb
Copy link
Contributor Author

candlerb commented Oct 1, 2021

Given that the solution turns out to be just a case of adding cache_timeout=86400, that seems like a very reasonable cost/benefit tradeoff :-)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 31, 2021
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

4 participants