Open Stack

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: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>