| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-03-26 | |||
| 12:24:08 | kashyap | No worries; just wanted to say that the loose end (discussion) is tied :) | |
| 12:24:20 | gibi | kashyap: thanks, it helps :) | |
| 12:24:32 | kashyap | later == April 01 (yes, really; not a joke ;)) | |
| 12:25:50 | gibi | kashyap: have a nice time off | |
| 12:40:02 | hrw | thanks | |
| 13:22:02 | openstackgerrit | Merged openstack/nova master: libvirt: Use domain capabilities to get supported device models https://review.opendev.org/666915 | |
| 13:26:49 | openstackgerrit | Kevin Zhao proposed openstack/nova master: fix scsi disk unit number of the attaching volume when cdrom bus is scsi https://review.opendev.org/712607 | |
| 13:41:16 | dansmith | gibi: yeah, I really just don't like PATCH in general. We don't have it anywhere else in here, and I'm missing the thing that makes it so hard to use PUT that it's worth using PATCH here | |
| 13:41:51 | dansmith | gibi: I wasn't in the discussion where that was decided, which is fine, so until I'm convinced I'm -1 on it, but that doesn't mean you have to stop just for me if you (all) really think it's the only way | |
| 13:41:54 | gibi | dansmith: PUT is not use to update the fields of an existing volume connection but to replace the existing volume connection | |
| 13:42:35 | dansmith | gibi: you don't mean PUT in general, I assume | |
| 13:42:46 | gibi | dansmith: nope. I mean https://docs.openstack.org/api-ref/compute/#update-a-volume-attachment | |
| 13:42:55 | gibi | this is an unfortunate use of PUT ^^ | |
| 13:43:44 | gibi | as it replaces an object not updating the field of an object | |
| 13:43:49 | dansmith | right, so if I put the attachment with the same fields but with delete_on_terminate set differently, then it's easy to determine that it's not new yeah? | |
| 13:45:01 | gibi | but then the PUT with the same volume_id needs to be end user facing, while when the volume_id is new it needs to be admin only | |
| 13:45:34 | dansmith | right, so admin can update volume_id, but users can only update delete_on_termination | |
| 13:45:39 | dansmith | seems pretty straightforward to me | |
| 13:46:55 | openstackgerrit | Kevin Zhao proposed openstack/nova master: fix scsi disk unit number of the attaching volume when cdrom bus is scsi https://review.opendev.org/712607 | |
| 13:48:35 | gibi | dansmith: your argument feels convinving to me but also in the past the discussion in the spec and on the nova meeting convinved me to use the PATCH approach so I now go to the confused state | |
| 13:49:21 | johnthetubaguy | FWIW, consistency with other Nova APIs seems like a big win, i.e. use PUT | |
| 13:49:39 | dansmith | gibi: okay, it seems like we still need to be able to do a policy check on the body of the thing, even if it''s a PATCH, so I can't imagine why we can't do that for PUT as well | |
| 13:49:46 | dansmith | johnthetubaguy: ++ for sure | |
| 13:56:24 | gibi | I'm sad that this discussion happens so late. From reading back I see that another concern was to mix two slightly unrelated API swap, and update into the same PUT method | |
| 13:57:09 | dansmith | from the api client's perspective it's the same operation isn't it? | |
| 13:58:17 | dansmith | gibi: I apologize for bringing it up I guess, but ... this is what wide review is for, IMHO, and this is a big step out of existing conventions for nova so it seems worth having wide review on | |
| 13:59:12 | gibi | dansmith: I also apologize to saying this but I think such wide review should happen in the spec. this is why we have spec review for | |
| 13:59:31 | gibi | from me the two operation is different from the client perspective for mutliple reasons | |
| 13:59:40 | dansmith | like, the internal plumbing of nova turns one of them into a special rpc call to the virt driver and the other into a db update, but that shouldn't affect the external design | |
| 13:59:45 | dansmith | well, I disagree | |
| 13:59:55 | dansmith | (about them being different) | |
| 13:59:57 | gibi | i) the client is different for the two operations | |
| 14:00:16 | gibi | for the swap volume the client is cinder | |
| 14:00:32 | gibi | for the update delete_on_terminate flag the client is the owner of the instance | |
| 14:00:49 | dansmith | in one specific set of cases it is, | |
| 14:00:53 | gibi | ii) the swap operation effects the running instance while the update operation does not | |
| 14:01:08 | dansmith | but a client library used by both wouldn't need to distinguish.. they're both "update the volume attachment" operations | |
| 14:01:10 | sean-k-mooney | the real issue is swap volume should have been a server action keeping put free for updates. | |
| 14:01:49 | dansmith | sean-k-mooney: I can see that, but I'm not sure it *had* to be.. this is very RESTful as it is, wouldn't you say? | |
| 14:02:10 | dansmith | we expose a resource, it's changing the resource.. what happens behind the scenes is internal logic | |
| 14:02:33 | sean-k-mooney | it is not a voilation or the rest design to do it as we do today | |
| 14:02:59 | dansmith | but server actions are pretty much all not RESTful :) | |
| 14:03:08 | sean-k-mooney | so yes from that perspecive swap volume via update if fine | |
| 14:03:32 | sean-k-mooney | dansmith: yes by definition its acutlly a rest anti pattened to have rpc like api actions | |
| 14:03:56 | dansmith | right | |
| 14:04:26 | dansmith | so point is, I just don't think swap-via-update is super terrible in a fundamental way (swap in general may be) | |
| 14:04:35 | sean-k-mooney | i was ok with the PUT in the spec by the way. gmann was not so i proposed patch as a compormise | |
| 14:04:52 | dansmith | and I don't think that we need should justify using a different method externally because the internal plumbing is currently setup in one specific way | |
| 14:05:00 | sean-k-mooney | since the api method was free and it allowed used to expresss policy cleanly | |
| 14:05:01 | dansmith | that is the point of the api.. to abstract those things away | |
| 14:05:35 | dansmith | right, but where does it end? use HEAD for the next one? :) | |
| 14:06:02 | sean-k-mooney | no but patch is for a paritl update which is what we are doing | |
| 14:06:26 | sean-k-mooney | we are setting one atribute and not the rest | |
| 14:06:51 | sean-k-mooney | but as i said it was a compromise not the ideal solution | |
| 14:06:53 | dansmith | sure, but we're using a different http method because we just want to abuse the WSGI plumbing to call different code for this operation vs. the existing put | |
| 14:07:04 | dansmith | so when the next thing comes along, we could use HEAD and avoid having to abstract that one too :) | |
| 14:07:50 | sean-k-mooney | well head would only be apporpriate if the new action match the semantics of head but i get your point and i think you get mine | |
| 14:08:09 | sean-k-mooney | dansmith: in your view you would prefer that we keep with PUT correct | |
| 14:08:14 | dansmith | yes | |
| 14:08:27 | dansmith | and yes, HEAD is a silly example for the sake of being silly | |
| 14:09:04 | sean-k-mooney | i have not looked at gmann specific objection in a while but i belive it was related to the semaintic of the payload | |
| 14:09:16 | sean-k-mooney | do you have a proposal to adress his concern? | |
| 14:09:26 | dansmith | I don't know what his concern is | |
| 14:10:08 | sean-k-mooney | i think there was a question about including the volume id in the payload vs the url and how to correctly apply differnet policies to each sub filed | |
| 14:10:28 | sean-k-mooney | i belive we can already do a polciy check on the payload so that is not really a concern | |
| 14:10:37 | openstackgerrit | Sundar Nadathur proposed openstack/nova master: Block unsupported instance operations with accelerators. https://review.opendev.org/674726 | |
| 14:10:38 | openstackgerrit | Sundar Nadathur proposed openstack/nova master: Add cyborg tempest job. https://review.opendev.org/670999 | |
| 14:10:41 | dansmith | I don't see that as a problem yeah | |
| 14:10:50 | sean-k-mooney | but there was a question about if the volume shoudl be set in teh payload or just in the url | |
| 14:11:29 | sean-k-mooney | currently i think the volume id in the url is the current volume and the one in the payload is the one to swap too | |
| 14:12:17 | dansmith | well, that makes it even easier to determine what is going on right? | |
| 14:12:35 | dansmith | if they differ, then check admin policy, if they're the same, user policy and only look for user-changeable options in the body | |
| 14:12:35 | sean-k-mooney | i think we would have to require that its not set in the payload | |
| 14:12:55 | sean-k-mooney | well ya we could require they are the same | |
| 14:13:11 | dansmith | further, PATCH has to be atomic and provides no way to guarantee that the representation hasn't changed since I fetched it on the client side | |
| 14:13:33 | dansmith | so if someone has updated the definition between me fetching it and sending a single field to change, I might not notice | |
| 14:13:41 | dansmith | i.e. there's no generation or other indicator | |
| 14:13:43 | sean-k-mooney | honestly i could be miss remebering what gmann objection was | |
| 14:14:00 | sean-k-mooney | dansmith: yes that is also true | |
| 14:14:05 | dansmith | probably not likely on a volume attachment currently, but certainly could happen in the future | |
| 14:14:20 | dansmith | i.e. some sort of swap-snapshot-swap thing or something | |
| 14:14:32 | dansmith | (this is a major objection of mine to patch in general for most things) | |
| 14:14:46 | sean-k-mooney | i think the most likely issue would be with doing a volume detach via cinder in parralle | |
| 14:15:29 | sean-k-mooney | that said we have the same issue with PUT no? unless we have a genertion field in the payload | |
| 14:15:52 | dansmith | no, with put you have all the original fields there, you can check to see that nothing has changed | |
| 14:15:53 | sean-k-mooney | since put is ment to do a full update replacing all fields at least semantically | |
| 14:16:11 | dansmith | I'm not saying we *have* to for this specific case, but it's at least *possible* | |
| 14:16:18 | dansmith | also, looking at the api-ref here, | |
| 14:16:22 | sean-k-mooney | yes that is true | |
| 14:16:58 | dansmith | there's a big red warning about the whole api being obscure, because people would expect to be able to PUT that.. it would be a lot clearer to revise that and say "changing the volumeId requires special privileges and support" instead of just walling off all of PUT | |
| 14:17:03 | sean-k-mooney | anyway i was pretty nutral on this orginally so if there is a stong feeling (sounds like there is) that it should be PUT then im ok with that too | |
| 14:17:12 | dansmith | because people would say "What? why can't I change this flag without special virt support?" | |
| 14:17:42 | nightmare_unreal | how can I run nova functional tests ? | |
| 14:17:53 | sean-k-mooney | nightmare_unreal: tox -e functional-py36 | |
| 14:18:13 | nightmare_unreal | and for specific file ? -- -n file-name ? | |
| 14:18:34 | sean-k-mooney | you can do -- folowed by any arges supported by stestr | |
| 14:18:51 | sean-k-mooney | normally i just pass a regex for the class name or test name | |
| 14:19:06 | nightmare_unreal | cool :) Thanks | |
| 14:19:21 | sean-k-mooney | e.g. tox -e functional-py36 -- "compute|libvirt" | |
| 14:19:48 | nightmare_unreal | in quotes can i specify exact class name ? | |