Hi Chris, thanks for taking this on. Comments inline.
On 08/18/2017 01:23 PM, Chris Dent wrote:
(I put [api] in the subject tags because this might be of interest
to a wider audience that cares about APIs.)
Long ago and far away I made this bug:
https://bugs.launchpad.net/nova/+bug/1632852
"placement api responses should not be cacehable"
Today I've pushed up a WIP that demonstrates a way to address this:
https://review.openstack.org/#/c/495380/
Before I get too far along with it, I'd like us to decide whether we
think it is worth doing and consider some of the questions it will
raise.
I think it is worth doing not just because it would be correct but
because without it, we cannot be assured that proxies or user agents
will not cache resource providers and other entities, and that would
lead to bizarre results.
Yes, +1.
At the same time, any entity you put on the web, according to the
RFCs[1], should have a last-modified header if it "can be reasonably
and consistently determined".
So my change above adds 'last-modified' and 'cache-control:
no-cache' to GET of /resource_providers and
/resource_providers/{uuid} and proposes we do it for everything
else.
Should we?
No. :) Not everything. In particular, I think both the GET
/resource_classes and GET /traits URIs are very cacheable and we
shouldn't disallow proxies from caching that content if they want to.
If we do, some things to think about:
* The related OVO will need the updated_at and created_at
fields exposed. This is pretty easy to do with the
NovaTimestampObject mixin. This doesn't need to require a object
version bump because we don't do RPC with them.
Technically, only the updated_at field would need to be exposed via the
OVO objects. But OK, sure. I'd even advocate a patch to OVO that would
bring in the NovaTimestampObject mixin. Just call it Timestamped or
something...
* Adding a response header violates interop guidelines, so this
would mean a microversion bump that impacts all GET requests. In
systems that currently use placement (the report client in nova,
mostly) no attention is being paid to either of the headers being
added, so there would be no need for motion on that side.
Ack.
* The current implementation of getting the last modified time for a
collection of resources is intentionally naive and decoupled from
other stuff. For very large result sets[3] this might be annoying,
but since we are already doing plenty of traversing of long lists,
it may not be a big deal. If it is we can incorporate getting the
last modified time in the loop that serializes objects to JSON
output.
I'm not sure what you're referring to above as "intentionally naive and
decoupled from other stuff"? Adding the updated_at field of the
underlying DB tables would be trivial -- maybe 10-15 lines total for
DB/object layer and REST API as well. Am I missing something?
Best,
-jay
I think we should. Generally speaking I think it is good form to
fulfil the expectations of HTTP. It helps make sure the HTTP APIs
work with the unknown client. Working with the unknown client is one
of the best reasons to have an HTTP API.k
[1] https://tools.ietf.org/html/rfc7232#section-2.2
[2] An argument could be made that this change is fixing a protocol
level bug, but I reckon that argument wouldn't fly with most people.
__________________________________________________________________________
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: [email protected]?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
__________________________________________________________________________
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: [email protected]?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
