| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-04 | |||
| 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 | |
| 16:31:15 | sean-k-mooney | e.g. we should unbreak your customers and then figure out where to go from there. | |
| 16:31:24 | johnthetubaguy | sean-k-mooney: cool, that aligns with my thinking | |
| 16:32:21 | johnthetubaguy | certainly, a heap of issues in there... its tempting to just drop the libosinfo stuff honestly, I am not sure its helping us any more, if we always send "virtio" expect for very old distros | |
| 16:33:36 | sean-k-mooney | ya i considerd that too but was not sure why it was originally added e.g. what would we be regressing if we did | |
| 16:38:06 | sean-k-mooney | by the way os_distro=centos7.0 is not correct it should be os_distro=centos and os_version=7 | |
| 16:38:18 | sean-k-mooney | see https://github.com/openstack/glance/blob/master/etc/metadefs/operating-system.json | |
| 16:39:36 | sean-k-mooney | which i now realise that module does not handel correctly either | |
| 16:43:34 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/virt/osinfo.py#L123-L124 should be checking both values and technically we should be rejecting centos7.0 as an invalde disto name | |
| 16:44:52 | sean-k-mooney | but we treat it as a sting filed here https://github.com/openstack/nova/blob/master/nova/objects/image_meta.py#L456 since we dont know the full set of os we could be running | |
| 16:53:02 | openstackgerrit | Merged openstack/nova master: vif: Skip most of 'get_base_config' if not using virtio https://review.opendev.org/666629 | |
| 17:10:52 | openstackgerrit | Merged openstack/nova stable/queens: Init HostState.failed_builds https://review.opendev.org/668911 | |
| 17:24:04 | openstackgerrit | Ghanshyam Mann proposed openstack/nova master: WIP:Introduce scope_types in os-services https://review.opendev.org/645427 | |
| 18:00:20 | johnthetubaguy | sean-k-mooney: right, it only seems to work with centos7.0 right now, else it gets rejected, go figure | |
| 18:00:52 | sean-k-mooney | right but the os_disto is ment to be the os name without the version | |
| 18:00:59 | sean-k-mooney | so it was broken from the start | |
| 18:01:47 | johnthetubaguy | maybe, it spits out a warning unless you smoosh them together | |
| 18:02:12 | sean-k-mooney | libosinfo? | |
| 18:02:28 | sean-k-mooney | looking at the spec the spec was using it wrongly form the start https://specs.openstack.org/openstack/nova-specs/specs/mitaka/implemented/libvirt-hardware-policy-from-libosinfo.html | |
| 18:02:30 | johnthetubaguy | no, Nova | |
| 18:02:43 | sean-k-mooney | nova should not spit out an error | |
| 18:02:46 | johnthetubaguy | the spec wasn't updated vs the implementation | |
| 18:03:05 | sean-k-mooney | os_distro=centos is the correct form | |
| 18:03:08 | johnthetubaguy | it always, had, well only in the operators logs as a warning | |
| 18:03:14 | johnthetubaguy | saying osdistro not found | |
| 18:03:23 | openstack | bug 1825537 in OpenStack Compute (nova) stein "finish_resize failures incorrectly revert allocations" [Medium,In progress] https://launchpad.net/bugs/1825537 - Assigned to Matt Riedemann (mriedem) | |
| 18:03:23 | openstackgerrit | Merged openstack/nova stable/stein: Add functional recreate test for regression bug 1825537 https://review.opendev.org/666959 | |
| 18:03:28 | openstackgerrit | Merged openstack/nova stable/stein: Drop source node allocations if finish_resize fails https://review.opendev.org/667155 | |
| 18:03:34 | openstackgerrit | Merged openstack/nova stable/rocky: Delete resource providers for all nodes when deleting compute service https://review.opendev.org/666854 | |
| 18:03:38 | johnthetubaguy | sorry, I need ot run | |
| 18:03:39 | sean-k-mooney | yes but os_distro was not added for libosinfo | |
| 18:03:41 | openstackgerrit | Merged openstack/nova stable/rocky: Disable limit if affinity(anti)/same(different)host is requested https://review.opendev.org/659246 | |
| 18:03:49 | sean-k-mooney | johnthetubaguy: sure o/ | |
| 18:03:59 | johnthetubaguy | I thought it existed before | |
| 18:04:05 | sean-k-mooney | it did | |
| 18:04:16 | sean-k-mooney | for other drivers for other things that expect jsut the os name | |
| 18:04:25 | sean-k-mooney | not the os name + version | |
| 18:07:05 | openstackgerrit | Merged openstack/nova master: Refactor bandwidth related functional tests https://review.opendev.org/641679 | |
| #openstack-nova - 2019-07-05 | |||
| 01:01:06 | openstackgerrit | Merged openstack/nova stable/stein: Ignore hw_vif_type for direct, direct-physical vNIC types https://review.opendev.org/667354 | |