Posted	Nick	Remark
#openstack-nova - 2020-03-26
11:47:56	stephenfin	sure
11:48:15	lyarwood	luyao: ack sorry swamped with downstream things, I'll get back to that review today
11:49:27	luyao	lyarwood: thanks :)
12:07:33	openstackgerrit	Lee Yarwood proposed openstack/nova master: libvirt: Break up get_disk_mapping within blockinfo https://review.opendev.org/714962
12:09:06	hrw	kashyap: zuul gave +1 for https://review.opendev.org/#/c/709494/ - bumping to +2+w?
12:12:22	kashyap	hrw: I am a mere mortal, can't +2 :)
12:12:35	kashyap	gibi: ^ Mind having a gander at hrw's completed patch?
12:12:51	gibi	kashyap, hrw: sure I will try
12:13:14	kashyap	gibi: At its core, it is setting a sensible default CPU model for AArch64 VMs
12:13:35	kashyap	gibi: We arrived at the sensible default (CPU model 'max') after consulting the QEMU AArch64 maintainers.
12:14:15	sean-k-mooney	kashyap: you could argue that min would be a better sensible default for live migration but max is overall a better default
12:14:30	gibi	kashyap: ack. thanks for bringing in the experts on that
12:14:30	sean-k-mooney	so ya it makes sense even if its not the safest default
12:14:32	kashyap	"min"? There's no such thing as "min"
12:15:01	sean-k-mooney	isnt there. there was an alternitive to max we were considering
12:15:19	sean-k-mooney	my understanding of max is it will enable all feature that qemu can emulate on the host
12:15:29	sean-k-mooney	not just the ones the host has at the hardware level
12:16:05	kashyap	You mean, 'host'?
12:16:17	sean-k-mooney	perhaps yes
12:16:35	sean-k-mooney	i think our default should basically be the same as host model is on x86
12:16:35	kashyap	As I noted on the patch, that is still not a good option if you want it to work for _both_ TCG and KVM.
12:16:38	kashyap	To quote myself:
12:16:40	kashyap	[quote]
12:16:41	kashyap	For KVM, we essentially have to go with "give the guest what the host CPU has", which you can express either as "host" or as "max" CPU models, the benefit of the latter ("max") being that it also works with TCG.
12:16:45	kashyap	[/quote]
12:17:01	kashyap	sean-k-mooney: That's what 'max' is; except that it also does the right thing for TCG
12:17:19	kashyap	gibi: Thanks!
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? :)

Earlier   Later