| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-03-26 | |||
| 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 | sean-k-mooney | i think we would have to require that its not set in the payload | |
| 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: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 ? | |
| 14:19:55 | nightmare_unreal | like in your example | |
| 14:20:04 | sean-k-mooney | yes | |
| 14:20:11 | nightmare_unreal | awesome, thanks | |
| 14:20:21 | sean-k-mooney | the quote are jsut so the shell does not interperate the | as a pipe | |
| 14:20:54 | sean-k-mooney | so in this case is an or in the regex so that will run all gets with compute or libvirt in the fully qulified function name | |
| 14:21:29 | nightmare_unreal | understood | |
| 14:21:43 | gibi | dansmith, sean-k-mooney: from technical perspective I'm OK to have PUT or PATCH as well. I was convinced about both at some point in the past. From process perspective I feel bad that we might change direction of the API design 2 weeks before FF. | |
| 14:22:26 | gibi | I will raise the discussion again on the team meeting as I hope gmann will be there | |
| 14:22:31 | dansmith | gibi: I would hate to change our API so significantly on a process technicality | |
| 14:22:54 | dansmith | I'd much rather help brinzhang_ get it right, and/or approve a little extra time | |
| 14:23:09 | dansmith | the API stability is serious business to me and much more important to get it right than not | |
| 14:24:05 | gibi | dansmith: OK that is a good point. Let's double check with gmann but I'm OK to give extra time after FF if brinzhang_ needs time to change | |
| 14:24:16 | dansmith | meaning, it's not easily changeable later to allow something to land before a deadline | |