Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-03-22
15:47:16 mriedem /os-aggregates/{aggregate_id}
15:47:19 mriedem is the primary key int
15:47:30 mriedem dansmith: have you gotten to aggregates in your multi-cell api series yet?
15:47:36 dansmith mriedem: aggs are in api db
15:47:42 dansmith mriedem: so just ugly, but not a problem
15:47:42 mriedem oh
15:47:49 mriedem ok
15:47:54 mriedem i should have known that
15:48:41 sdague mriedem: changing the id field is probably fine, documentation will be a little odd on that, but should be fine.
15:48:48 mriedem yeah
15:48:50 mriedem ok
15:53:17 openstackgerrit Gábor Antal proposed openstack/nova master: Transform instance.volume_attach notification https://review.openstack.org/401992
15:53:37 mriedem sdague: dansmith: and we're good with changing "PUT /os-services/disable" to "PUT /os-services/{service_id}disable" ?
15:53:51 mriedem for the 4 service action APIs
15:54:01 mriedem and removing the host/binary body as identifiers
15:54:10 mriedem https://developer.openstack.org/api-ref/compute/?expanded=show-aggregate-details-detail,disable-scheduling-for-a-compute-service-detail#disable-scheduling-for-a-compute-service
15:54:47 mriedem or just tell me to shut up and stop asking
15:56:51 sdague the whole empty PUT thing seems weird to me, as it's not a thing you would GET
15:58:09 mriedem what do we change it to?
15:58:10 mriedem POST?
15:58:25 mriedem POST is what we use for the server action APIs
15:59:38 sdague yes
16:01:12 cdent POST is probably right for empty actions, but only because actions are outright weird in the first place
16:01:36 cdent we've started exploring empty PUTs in the placement API, but they do involve things which can be GET
16:01:54 cdent (and are still empty when gotten)
16:02:32 sdague cdent: that whole thing seem super weird to me as well, as it seems really odd to have resources with no content
16:03:22 cdent sdague: there's tons of those things across the placement api though: empty strings that mean stuff only by their existence
16:03:23 cdent customer resource classes and traits
16:03:35 cdent s/empty/simple/
16:04:21 mriedem sdague: cdent: so to be clear, we can remove the body for this https://developer.openstack.org/api-ref/compute/?expanded=disable-scheduling-for-a-compute-service-detail#disable-scheduling-for-a-compute-service
16:04:24 mriedem which then makes it a POST
16:04:41 mriedem but we'd still have a body for https://developer.openstack.org/api-ref/compute/?expanded=log-disabled-compute-service-information-detail#log-disabled-compute-service-information
16:04:48 mriedem so does that become POST or remain PUT?
16:05:09 mriedem also, does the response affect which method it is in any way?
16:05:12 sdague mriedem: is that changing urls as well?
16:05:16 mriedem since these all return a response body
16:05:19 mriedem sdague: yes
16:05:26 sdague mriedem: honestly, I think from the principle of least surprise
16:05:43 sdague if you are going to GET the same url as the modification, PUT is fine
16:05:54 sdague if it's a different URL, POST is better
16:05:57 mriedem all 4 PUTs here would change the URL https://developer.openstack.org/api-ref/compute/#compute-services-os-services
16:06:08 sdague mriedem: which will have GET urls still?
16:06:23 mriedem the only GET we have is to list services
16:06:29 sdague yeh, so I'd make them POST
16:06:46 sdague if the urls are changing anyway
16:07:06 mriedem let me make sure we don't have GET /os-services/{service_id}
16:07:24 mriedem yeah we don't
16:07:40 mriedem ok so change all 4 PUTs to POSTs and modify the URL to include the {service_id}
16:07:48 mriedem and drop host/binary from the POST body
16:07:54 mriedem since those are replaced by the service_id
16:08:28 mriedem i'll make those changes to the spec
16:08:35 mriedem thanks y'all
16:08:55 cdent I was under the impression we didn't want to make that breadth of change, but since we are willing, that sounds like the right way to go
16:09:23 sdague actually, I'm going to raise a bigger question
16:09:52 sdague why isn't this just a PUT/PATCH of /os-services/{service_id}
16:10:20 sdague every one of these urls is really an attribute in the response
16:10:22 mriedem with a body with the action to perform?
16:10:42 mriedem or PUT with 'status' in the req body?
16:10:44 cdent that's what I think is the long term right way, but I thought we didn't want that because people did want the attribute free way of doing stuff
16:10:48 sdague http://paste.openstack.org/show/603789/
16:11:11 sdague mriedem: yeh, just like how we can update the description in a server
16:12:42 mriedem we could do that too
16:12:47 cdent the debate surrounding that kind of thing in the past has been that PUT is supposed to be a full representation, and PATCH is supposed to be a diff, not a partial representation. <- just reporting, not presenting that as my position
16:13:21 cdent and some people think a full rep PUT to change one thing is wasteful
16:13:24 sdague which would also let you do things like set {"status": "disabled", "disabled_reason": "...."}
16:13:25 mriedem all of these actions are actualy served by the update method in the controller anyway
16:13:26 cdent because there are only so many bytes in the universe
16:13:31 mriedem with the id being the action
16:13:36 sdague so, we already do PUT with partials, and I think that's fine
16:14:02 sdague I honestly am anti PATCH, because I think it is really for something more like what swift uses it for
16:14:07 sdague not for this stuff
16:14:23 openstackgerrit Sujitha proposed openstack/nova master: Add description to policies in console_auth_tokens.py https://review.openstack.org/448663
16:14:26 mriedem https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/services.py#L212 is what handles all of those PUTs
16:14:30 mriedem the id is the action
16:14:31 sdague mriedem: well, it would be PUT /os-services/{service_id} ... stuff you want to change
16:14:36 mriedem sdague: right
16:14:47 mriedem so we'd have another microversioned update method,
16:14:48 sdague just like severs today
16:14:53 mriedem where id becomes the service uuid
16:15:00 mriedem and the body contains the things you want to change
16:15:06 mriedem like status or forced-down
16:15:47 sdague yeh
16:16:11 sdague especially because those things are reflected in the representation when you get it
16:16:24 mriedem i'd kind of like to split that into a separate spec/microversion, but doing that gets weird when we also want to change the url
16:16:44 sdague yeh, I think it should all change at once, otherwise it's just painful for users
16:16:46 mriedem so probably can't split
16:16:48 mriedem right
16:17:02 mriedem since we don't test any of this in tempest, i worry
16:17:10 mriedem but anyway
16:17:51 cdent someday
16:18:26 mriedem it's just a lot of cases for the single PUT to handle, but we'll see
16:18:37 mriedem plus there is already another microversion in this flow already, 2.11
16:18:38 mriedem for forced-down
16:18:39 openstackgerrit Sujitha proposed openstack/nova master: Add description to policies in cloudpipe.py https://review.openstack.org/448271
16:18:40 sdague we don't test this stuff in tempest because it would break the world
16:18:44 mriedem sdague: i know
16:18:49 mriedem i know why, i'm just saying
16:18:50 mriedem you know
16:18:57 sdague you can't start randomly turning off the compute service in the middle of tests
16:19:03 sdague it's all testable on the functional side

Earlier   Later