| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-03-26 | |||
| 12:17:31 | kashyap | sean-k-mooney: Give your ACK on the change, too, please | |
| 12:17:57 | sean-k-mooney | max will not do the right thing for kvm however right. it will expose more then the host can support in hardware right | |
| 12:18:22 | sean-k-mooney | kashyap: well im not conviced max is the right thing in the kvm case i would prefer to have a different default for both honestly | |
| 12:18:33 | sean-k-mooney | max i think makes sense in the qemu/tcg case | |
| 12:18:57 | sean-k-mooney | unless host and max are always identical in the kvm case i woudl prefer it to be host when virt_type=kvm | |
| 12:19:19 | kashyap | sean-k-mooney: Did you read the above quote? | |
| 12:19:26 | sean-k-mooney | yes | |
| 12:20:04 | sean-k-mooney | is host always identical to max with kvm | |
| 12:20:25 | kashyap | No; 'max' will give you the moving-target "all the stuff we can currently emulate". | |
| 12:20:29 | sean-k-mooney | or if your qemu can emulate feature not present on the host hardware will max result in it doing so even with kvm | |
| 12:20:32 | kashyap | Let's not get worried about theoretical concerns. | |
| 12:21:29 | sean-k-mooney | kashyap: its not a terirectical concern i disagreed with your live migration change and this in my view is one step closer to use not supporting live migration with aarch64 at all | |
| 12:22:28 | sean-k-mooney | useing max will have an upgrade impact as we will not be able to live migrate safely form a newer host to an older one | |
| 12:22:35 | sean-k-mooney | use host with kvm will allow that | |
| 12:22:43 | kashyap | sean-k-mooney: Hang on; I just asked Peter from QEMU again. | |
| 12:22:56 | kashyap | So, yes: 'max' is same as 'host' on KVM. Always | |
| 12:23:12 | sean-k-mooney | ok if that is the case im fine with it :) | |
| 12:23:15 | kashyap | Okido | |
| 12:23:23 | kashyap | gibi: Please go ahead with the patch. --^ | |
| 12:23:25 | sean-k-mooney | ill go +1 it now | |
| 12:23:30 | kashyap | Thx! | |
| 12:23:53 | gibi | thanks, I will look in a minute | |
| 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 | |