| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-04 | |||
| 13:46:48 | sean-k-mooney | it will totally have all my special feature i have wanted for years in it | |
| 13:47:03 | kashyap | Yes, "nova, make me a sandwich" | |
| 13:47:16 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Use neutron contants in cmd/manage.py https://review.opendev.org/668946 | |
| 13:47:16 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Add 'resource_request' to neutronv2/constants https://review.opendev.org/668947 | |
| 13:47:18 | kashyap | https://xkcd.com/149/ | |
| 13:47:29 | sean-k-mooney | that would have been the S release | |
| 13:51:40 | openstackgerrit | Ghanshyam Mann proposed openstack/nova master: Add test coverage of existing os-services policies https://review.opendev.org/669181 | |
| 13:58:57 | stephenfin | It so peaceful here without all those pesky Americans cluttering the place up | |
| 14:00:16 | sean-k-mooney | all of his cross cell migration code of couse so he wonders where the 20 patch series went :) | |
| 14:01:19 | sean-k-mooney | and by 20 i appernetly ment 43 | |
| 14:01:32 | cdent | stephenfin: merge all the things. also thanks for abandoning that health check thing | |
| 14:01:58 | cdent | gibi, sean-k-mooney : unfinished | |
| 14:04:02 | gibi | cdent: not a bad idea :) | |
| 14:05:02 | jangutter | I thought the "U" release would be "Unnamed". | |
| 14:05:24 | gibi | sean-k-mooney, stephenfin: if you want to surprise mriedem then this could be easy too https://review.opendev.org/#/q/topic:bp/pre-filter-disabled-computes+(status:open+OR+status:merged) | |
| 14:06:05 | stephenfin | oh, I can probably do that today | |
| 14:21:06 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Remove assumption of http error if consumer not exists https://review.opendev.org/669188 | |
| 14:25:30 | openstackgerrit | John Garbutt proposed openstack/nova master: Stop using virtio1.0-block and virtio1.0-net https://review.opendev.org/669190 | |
| 14:30:04 | sean-k-mooney | johnthetubaguy: regarding ^ | |
| 14:30:08 | openstackgerrit | John Garbutt proposed openstack/nova master: Stop using virtio1.0-block and virtio1.0-net https://review.opendev.org/669190 | |
| 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 | kashyap | See my comment in the bug: https://bugs.launchpad.net/nova/+bug/1835400 | |
| 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: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 | |