| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-22 | |||
| 15:46:14 | dansmith | mriedem: I'm fine eitther way | |
| 15:46:20 | mriedem | ok | |
| 15:46:21 | dansmith | tend to thing re-using id is easier | |
| 15:46:22 | openstackgerrit | Gábor Antal proposed openstack/nova master: Ensure instance is in active state after notification test https://review.openstack.org/448655 | |
| 15:46:23 | dansmith | *think | |
| 15:46:30 | mriedem | dansmith: that's what i was originally planning too | |
| 15:47:11 | mriedem | shit, now that i mention aggregates https://developer.openstack.org/api-ref/compute/?expanded=show-aggregate-details-detail#host-aggregates-os-aggregates | |
| 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 | |