| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-04 | |||
| 14:30:20 | sean-k-mooney | we will still be ablt ot use virtio 1.0 device if qemu supports it right | |
| 14:30:27 | sean-k-mooney | we just wont be forcing them | |
| 14:30:44 | johnthetubaguy | I should probably change the short description | |
| 14:31:09 | sean-k-mooney | well im reading https://libvirt.org/formatdomain.html#elementsVirtioTransitional now and then going back to read your patch | |
| 14:32:03 | openstackgerrit | John Garbutt proposed openstack/nova master: Stop sending bad values from libosinfo to libvirt https://review.opendev.org/669190 | |
| 14:32:29 | johnthetubaguy | that is a better tag line | |
| 14:32:36 | sean-k-mooney | yep | |
| 14:32:42 | sean-k-mooney | and the patch makes sense too | |
| 14:32:49 | johnthetubaguy | even better :) | |
| 14:33:06 | sean-k-mooney | our min version is not 5.2 so we shoudl definetly be normalising | |
| 14:33:38 | johnthetubaguy | yeah, I mean either way, we can't sent virtio1.0-net its just not a thing libvirt accepts | |
| 14:33:51 | sean-k-mooney | it will be a while but we should add a todo to remove whne we have the required min version | |
| 14:34:12 | sean-k-mooney | for the driver typ correct | |
| 14:34:20 | sean-k-mooney | well the model | |
| 14:35:10 | sean-k-mooney | actully we ming not need teh todo in that case | |
| 14:36:12 | johnthetubaguy | we could do something more fancy, when we have new libvirt, I guess, but this seems more... backportable | |
| 14:36:38 | sean-k-mooney | ya | |
| 14:37:13 | openstackgerrit | Ghanshyam Mann proposed openstack/nova-specs master: Fix followup comments of policy-defaults-refresh spec https://review.opendev.org/669196 | |
| 14:39:12 | sean-k-mooney | that module in general feels weird. | |
| 14:39:30 | sean-k-mooney | i was not expecting it to be doing things based on image metadata | |
| 14:40:13 | sean-k-mooney | it feel like the HardwareProperties class shoulnd not be there | |
| 14:41:09 | sean-k-mooney | really those properties shoudl jsut be on the image metadata object | |
| 14:41:50 | sean-k-mooney | thats unrealed to your cahnge but just an observation. | |
| 14:43:36 | johnthetubaguy | agreed... its odd | |
| 14:44:36 | sean-k-mooney | is this file used by anything other then the libvirt driver | |
| 14:46:46 | johnthetubaguy | not that I can see | |
| 14:47:00 | johnthetubaguy | I suspect it was placed aspirationally | |
| 14:47:45 | sean-k-mooney | im wondering if we should do this in https://github.com/openstack/nova/blob/master/nova/virt/libvirt/blockinfo.py instead | |
| 14:49:13 | johnthetubaguy | I think there are more issues than that, looking at this more closely | |
| 14:49:52 | johnthetubaguy | right now we fail to boot instances with the "correct" os_distro, that is probably the main bit | |
| 14:50:13 | johnthetubaguy | looking at a follow up patch... this isn't right | |
| 14:50:22 | sean-k-mooney | i think this is only ever used here https://github.com/openstack/nova/blob/master/nova/virt/libvirt/blockinfo.py#L237 | |
| 14:50:35 | sean-k-mooney | outside of test that is | |
| 14:51:06 | kashyap | johnthetubaguy: Nice, the "Stop sending bad values"... | |
| 14:52:04 | sean-k-mooney | kashyap: this actully look like a libosinfo bug to me | |
| 14:52:27 | kashyap | sean-k-mooney: This morning we analyzed, there is definitely a Nova bug here. | |
| 14:52:39 | kashyap | (If there's a libosinfo bug, that could be extra) | |
| 14:53:05 | sean-k-mooney | there proably is too but i am not sure that libosinfo should also be retruning virtion1.0-net | |
| 14:53:08 | openstack | Launchpad bug 1835400 in OpenStack Compute (nova) "Issues booting with os_distro=centos7.0" [Medium,In progress] - Assigned to John Garbutt (johngarbutt) | |
| 14:53:08 | kashyap | See my comment in the bug: https://bugs.launchpad.net/nova/+bug/1835400 | |
| 14:53:50 | kashyap | Not probably, we pretty sure know there is a Nova bug. Nova's validation is incomplete. And the is_vif_model_valid_for_virt() does not have the full list of hardware models | |
| 14:53:51 | sean-k-mooney | im not sure it should be encoding the virtio version in the model name | |
| 14:54:13 | kashyap | We're sure; I checked with the libosinfo + libvirt upstream this morning | |
| 14:54:34 | kashyap | johnthetubaguy: and I had an hour-long chat here (might want to read the scroll, if you could stomach reading it). | |
| 14:54:40 | johnthetubaguy | this is another data point: https://github.com/virt-manager/virt-manager/commit/617b92710f50015c5df5f9db15d25de18867957d | |
| 14:55:00 | kashyap | sean-k-mooney: 'virtio' is the _right_ choice. Beacuse see the documentation johnthetubaguy linked to in the commit message. | |
| 14:55:01 | sean-k-mooney | what im saying is i consider it an api breaking in libsoinfo | |
| 14:55:15 | kashyap | What precisely is an API breakage? | |
| 14:55:35 | kashyap | The 'virtio1.0-net' not being availablE? | |
| 14:55:50 | sean-k-mooney | addtion of new values to the enum in a way that is backward incompatible. | |
| 14:56:10 | kashyap | Quoting my second point from the bug: | |
| 14:56:18 | kashyap | "(2) The "model names returned by libosinfo are *NOT* required to be valid libvirt network model names". I.e. there is no such "virtio-1.0-net" model name in libvirt, if we see here: https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/conf/domain_conf.c#l515" | |
| 14:57:10 | sean-k-mooney | im not saying that we should not normalise them but i am qustioning the value of using libosinfo at all if they are not required to be valid | |
| 14:57:55 | johnthetubaguy | so they had virtio-block before and now also have virtio1.0-block for the new non-legacy device | |
| 14:58:20 | johnthetubaguy | all the other users whitelist and go digging for the info they want, it seems | |
| 14:58:35 | johnthetubaguy | we just blindly trusted and thing that was never planned to be correct | |
| 14:58:57 | sean-k-mooney | ya | |
| 14:59:19 | sean-k-mooney | if it was a python lib i would say we should be capping it in upper constratits | |
| 14:59:48 | johnthetubaguy | we are using the interface wrong here, I dislike libosinfo's interface (but honestly, this is minor in my general dislike of the interface)... but we don't have anything better, we did try feedback this dislike when we first added support. | |
| 14:59:53 | sean-k-mooney | but anyway i guess we need to normalise it but im not sure what value this code adds | |
| 15:00:25 | johnthetubaguy | I think this was probably broken when we added it, just clearly no one seems to use it | |
| 15:00:42 | johnthetubaguy | appart from me, when I tried to squash a bunch of warning logs in production... | |
| 15:00:49 | johnthetubaguy | (facepalm) | |
| 15:01:54 | johnthetubaguy | magnum actually requires an osdistro value that isn't supported by libosinfo... but that is just as well else the instance wouldn't boot | |
| 15:02:27 | johnthetubaguy | (they want fedora atomic only, for their fedora atomic driver) | |
| 15:02:49 | sean-k-mooney | so i think this is fundementally broken as it also ignores what disk buses are supporte by the hypervior | |
| 15:04:03 | sean-k-mooney | i am adding some related logic for one of my spec | |
| 15:04:06 | sean-k-mooney | https://review.opendev.org/#/c/666915/2/nova/virt/libvirt/driver.py@9659 | |
| 15:04:40 | sean-k-mooney | there i am using the new domain capablities api to get the disk models supoted by the hypervior | |
| 15:05:11 | sean-k-mooney | your current patch just checks there in the supported list but does not take account of the vrit type | |
| 15:05:49 | sean-k-mooney | so if the virt type was lxc you could end up retruning virtio which is wrong | |
| 15:06:32 | sean-k-mooney | osinfo also should not take preference overe hw_disk_bus | |
| 15:06:49 | sean-k-mooney | if the user specfically set it we shoudl use that | |
| 15:06:55 | sean-k-mooney | so there are several bugs with this | |
| 15:07:24 | sean-k-mooney | actully never mind that last point | |
| 15:07:32 | sean-k-mooney | it handel that internaly | |
| 15:07:43 | sean-k-mooney | that is is why we need to pass the image metadata | |
| 15:08:11 | sean-k-mooney | to the hardware properties class but we dont pass the virt type | |
| 15:10:17 | sean-k-mooney | i dont know maybe im wrong but it feels odd teh way its currently layed out | |
| 15:10:39 | sean-k-mooney | i have not gone back and read the commit that added it maybe it explained why the current form is used | |
| 15:10:50 | sean-k-mooney | * order | |
| 15:20:10 | johnthetubaguy | sean-k-mooney: its feels stupid the way it currently is, particular in reference to this logic: https://github.com/openstack/nova/blob/61558f274842b149044a14bbe7537b9f278035fd/nova/virt/libvirt/blockinfo.py#L219 and https://github.com/openstack/nova/blob/ff0f33cf7e44c5d01f3fb999295eff4508c24a14/nova/virt/libvirt/vif.py#L137 | |
| 15:20:28 | johnthetubaguy | I know stephenfin just did a bunch of improvements in there, but yeah, its a bit stupid in its current form | |
| 15:24:02 | stephenfin | Yeah, it's on my list. tbh, 'get_base_config' just needs to disappear | |
| 15:24:29 | stephenfin | It doesn't really provide a consistent base for anything at this point | |
| 15:26:34 | johnthetubaguy | so I liked the libosinfo plan, specify os_distro and you get a best default for your chosen hypervisor... but yeah, it doesn't do that today. | |
| 15:31:16 | kashyap | johnthetubaguy or anyone: If you want a change of scenery, some easy patches with "negative code": https://review.opendev.org/#/q/topic:Bump_min_libvirt_and_QEMU_for_Stein+(status:open) | |
| 15:31:54 | johnthetubaguy | yeah, I keep meaning to look, but I have to fix some customer stuff at the moment | |
| 15:33:19 | kashyap | johnthetubaguy: Sure, this can wait | |
| 15:33:30 | kashyap | Go for the other one; more important :-) | |
| 15:41:50 | stephenfin | kashyap: Looks like we've got some duplication between that series above and my one ending here https://review.opendev.org/#/c/666814/ | |
| 15:42:00 | kashyap | stephenfin: Whoops | |
| 15:42:40 | stephenfin | kashyap: Specifically on this one https://review.opendev.org/#/c/669171/ Think you could do some rebasing one way or the other (or just drop that patch) | |
| 15:43:03 | stephenfin | Also, have questions on the cmt patch. Rest is +" | |
| 15:43:05 | stephenfin | *+2 | |
| 15:43:24 | kashyap | stephenfin: Darn the 'vif' thing is indeed a duplicate. Sorry; I'll abandon that | |
| 15:45:33 | kashyap | Abandoned; will check the question on the CMT one. | |
| 15:45:41 | kashyap | Thanks for review. | |
| 15:45:58 | kashyap | s/for/"for the"/ | |
| 16:30:55 | sean-k-mooney | johnthetubaguy: regarding the osinfo issue we coudl merge you small fix as a stepping stone but i think we should file a seperate bug to adress the larger issue later | |