| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-22 | |||
| 15:38:28 | nkorabli | I am sorry, for interrupting, though. But I just was wondering if that (what I've posted above) is expected or not? | |
| 15:38:45 | openstackgerrit | Hironori Shiina proposed openstack/nova master: Ironic: Support boot from Cinder volume https://review.openstack.org/215385 | |
| 15:39:26 | mriedem | nkorabli: check your policy, can non-admins users unshelve in your deployment? | |
| 15:39:49 | cfriesen | mriedem: even if they couldn't, presumably it should just return an error message rather than put the instance into error state | |
| 15:40:03 | mriedem | cfriesen: true | |
| 15:40:07 | mriedem | policy fail would be a 403 | |
| 15:40:13 | openstackgerrit | Sujitha proposed openstack/nova master: Add description to policies in baremetal_nodes.py https://review.openstack.org/448650 | |
| 15:40:33 | mriedem | default policy for shelve is admin or owner | |
| 15:40:39 | mriedem | so the demo user that created the instance should be able to unshelve it | |
| 15:40:53 | mriedem | the error is probably due to the instance actually failing to unshelve, i.e. spawn on a host | |
| 15:41:05 | mriedem | which is not a policy issue, just an issue | |
| 15:41:25 | nkorabli | oh, alright... | |
| 15:41:25 | mriedem | nkorabli: so check the conductor, scheduler and nova-compute logs for failures | |
| 15:41:27 | mriedem | for the unshelve | |
| 15:41:40 | mriedem | it should be pretty obvious if the instance is in ERROR state | |
| 15:42:07 | nkorabli | Got it! Let me check that.. | |
| 15:42:18 | nkorabli | Thanks a lot! | |
| 15:44:20 | mriedem | alex_xu: did you want me to specifically call out that the search-related GET methods for os-hypervisors will not be changed in https://review.openstack.org/#/c/447149/1/specs/pike/approved/service-hyper-uuid-in-api.rst@117 ? | |
| 15:45:40 | mriedem | sdague: johnthetubaguy: dansmith: thoughts on https://review.openstack.org/#/c/447149/1/specs/pike/approved/service-hyper-uuid-in-api.rst@128 and if we change the value of the id key to be a uuid value, or add a uuid key to the response, or completely drop id and just return a uuid key? | |
| 15:45:57 | mriedem | server.id response is a uuid, but aggregates response has both id (int) and uuid | |
| 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 | mriedem | oh | |
| 15:47:42 | dansmith | mriedem: so just ugly, but not a problem | |
| 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 | |