| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-27 | |||
| 14:44:58 | mriedem | gibi: i know you told me to leave you alone, but i'm a soft -1 on the bottom 2 changes in the series now, should be easy things to address and then i can +2 those today | |
| 14:45:08 | sean-k-mooney | for q35 we expect nova to generate a pcie toplogy in the xml | |
| 14:45:43 | kashyap | sean-k-mooney: Hang on ... | |
| 14:46:17 | kashyap | sean-k-mooney: This is not even valid guest XML: | |
| 14:46:19 | kashyap | <type machine="x86_64=q35">hvm</type> | |
| 14:46:30 | sean-k-mooney | hehe ya i just noticed that too | |
| 14:47:01 | kashyap | sean-k-mooney: Also, it's a bug in libvirt itself! | |
| 14:47:07 | kashyap | sean-k-mooney: It should reject that as an invalid XML | |
| 14:47:17 | sean-k-mooney | it did | |
| 14:47:20 | kashyap | I'll file an upstream bug report, and take it w/ the libvirt folks | |
| 14:47:34 | openstackgerrit | Yongli He proposed openstack/nova master: Adds the server group info into show server detail API. https://review.openstack.org/621474 | |
| 14:48:03 | sean-k-mooney | the error startswith "Error defining a guest with XML: ..." | |
| 14:48:14 | sean-k-mooney | maybe that came form nova | |
| 14:48:26 | kashyap | I guess so; libvirt uses the terms "domains" | |
| 14:48:34 | kashyap | sean-k-mooney: Regardless, libvirt's parser should reject invalid XML | |
| 14:48:36 | sean-k-mooney | ah right | |
| 14:48:49 | kashyap | It is a valid bug in libvirt itself; I just double-confirmed w/ a libvirt upstream dev. | |
| 14:48:58 | sean-k-mooney | yep let me quickly set the image prop to q35 | |
| 14:50:01 | kashyap | sean-k-mooney: So, this is valid guest XML: <type arch='x86_64' machine='pc-i440fx-rhel7.2.0'>hvm</type> | |
| 14:50:42 | sean-k-mooney | kashyap: openstack image set --property hw_machine_type=q35 cirros-0.3.6-x86_64-disk works | |
| 14:51:04 | kashyap | sean-k-mooney: Excellent. So the problem is the bug reporter directly copied my bad syntax! | |
| 14:51:12 | kashyap | Talk about copy/paste on the internet. | |
| 14:51:24 | sean-k-mooney | http://paste.openstack.org/show/746437/ | |
| 14:51:49 | sean-k-mooney | kashyap: well the syntax you used was correct for the nova.conf | |
| 14:52:21 | sean-k-mooney | but an image presuably only has on architeure so i guess droping the arch= part makes sense | |
| 14:52:47 | kashyap | Yeah | |
| 14:53:11 | kashyap | sean-k-mooney: Thanks for the debugging; two birds, one stone. ;-) | |
| 14:53:23 | yonglihe | Surya's comments addressed. thanks. https://review.openstack.org/#/c/621474/27 | |
| 14:53:37 | sean-k-mooney | you could argue there is also a nova bug here in that nova did not validate the image metadata property before passing it to libvirt | |
| 14:54:22 | kashyap | sean-k-mooney: That, too. | |
| 14:54:51 | yonglihe | jaypipes: topology patch rebase to microversion 2.72 https://review.openstack.org/#/c/621476/ | |
| 14:59:05 | mriedem | gibi: -1 on https://review.openstack.org/#/c/625310/ as well | |
| 15:00:58 | kashyap | sean-k-mooney: Can you tell the libvirt/QEMU version you tested with, please? | |
| 15:01:04 | mriedem | stephenfin: you should probably take a look at this before me https://review.openstack.org/#/c/625311/ | |
| 15:03:31 | sean-k-mooney | ubuntu@os-vif-1:/opt/stack/nova$ qemu-system-x86_64 --version | |
| 15:03:32 | sean-k-mooney | QEMU emulator version 2.11.1(Debian 1:2.11+dfsg-1ubuntu7.9) | |
| 15:03:34 | sean-k-mooney | Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers | |
| 15:03:35 | cfriesen | mriedem: not sure what you mean by this comment: https://review.openstack.org/#/c/620706/25/nova/compute/api.py@612 | |
| 15:03:36 | sean-k-mooney | ubuntu@os-vif-1:/opt/stack/nova$ libvirtd --version | |
| 15:03:38 | sean-k-mooney | libvirtd (libvirt) 4.0.0 | |
| 15:04:52 | mriedem | cfriesen: reference to https://review.openstack.org/#/c/620706/25/nova/compute/api.py@3569 | |
| 15:06:36 | mriedem | cfriesen: left another comment | |
| 15:06:44 | mriedem | i'd care less about that if it wasn't copy/paste | |
| 15:06:49 | mriedem | DRY that up | |
| 15:06:50 | mriedem | please | |
| 15:14:55 | openstackgerrit | Rodolfo Alonso Hernandez proposed openstack/os-vif master: Add "master" parameter to ip.set() API function https://review.openstack.org/639702 | |
| 15:15:50 | sean-k-mooney | jaypipes: stephenfin by the way i missed ^ in the brctl removal so we need to respin os-vif... | |
| 15:15:52 | jaypipes | yonglihe: yes, I have that in my queue. I have major concerns about the response format in that patch. It is mixing the concept of a guest's virtual CPU topology with that of the NUMA information, and the two things don't really have anything to do with each other -- and unfortunately the way that InstanceNUMATopology and InstanceNUMACell objects were designed, that bad coupling is now going to leak out through the public REST API, which is a | |
| 15:15:52 | jaypipes | problem I raised during the spec review I believe. | |
| 15:24:29 | cfriesen | mriedem: regarding https://review.openstack.org/#/c/620706/25/nova/compute/api.py@3569 if I do that then wouldn't I need to change the existing code in _validate_flavor_image() where it's doing stuff like image.get('properties', {})? | |
| 15:29:27 | stephenfin | mriedem, gibi: Reviewed https://review.openstack.org/#/c/625311/ Two main issues plus a lot of doc nits (which can be ignored for now) | |
| 15:31:05 | mriedem | cfriesen: yeah i guess you're right | |
| 15:31:40 | cfriesen | making the other changes now | |
| 15:32:40 | openstackgerrit | Surya Seetharaman proposed openstack/nova master: Fix the api sample docs for microversion 2.68 https://review.openstack.org/639707 | |
| 15:32:59 | dcdawg | I'm trying to run a test "tox -e py27" but get an error File "nova/__init__.py", line 33, in <module> | |
| 15:33:02 | dcdawg | import oslo_service # noqa | |
| 15:33:12 | dcdawg | ImportError: No module named oslo_service | |
| 15:33:33 | dcdawg | Even though I installed the "test-requirements.txt" stuff | |
| 15:33:40 | stephenfin | dcdawg: 'tox -e py27 --recreate' | |
| 15:34:27 | stephenfin | dcdawg: If that doesn't work, it usually means you've introduced a syntax error in your code. 'git stash' your changes and try again to confirm (without '--recreate' this time) | |
| 15:36:43 | mriedem | gdi this root volume detach/attach just gets more complicated | |
| 15:36:53 | mriedem | artom: so i realize there is a bug in the device tag stuff | |
| 15:37:10 | artom | mriedem, your new API stuff? | |
| 15:37:24 | mriedem | no. you know how you can try to create a server with device tags and we'll fail late and abort the build in the compute service if the compute driver doesn't support device tags? | |
| 15:37:45 | artom | mriedem, yeah | |
| 15:37:48 | mriedem | if you shelve that server and then unshelve it, you could wind up on a host that doesn't support tags either and it won't fail | |
| 15:38:12 | artom | mriedem, that's fun | |
| 15:38:36 | mriedem | which was the argument i think you made when adding tags to the attach volume api - you didn't allow it for shelved offloaded servers because we wouldn't know if the compute we land on supports them, | |
| 15:38:41 | mriedem | but that's already broken | |
| 15:39:00 | artom | mriedem, it's been a while for me with that code, where exactly do we fail? And is that code path not used when we unshelve? Because that's basically a new instance build, no? | |
| 15:39:12 | mriedem | https://github.com/openstack/nova/blob/6efa3861a5a829ba5883ff191e2552b063028bb0/nova/compute/manager.py#L2114 | |
| 15:39:16 | mriedem | ^ is only called during server create | |
| 15:39:19 | mriedem | not unshelve | |
| 15:39:30 | artom | mriedem, yeah, I remember saying "can't do tagged attached with shelved because we have no idea if the eventual compute will support it" | |
| 15:39:32 | mriedem | unshelve is a new spawn of the guest | |
| 15:39:47 | mriedem | sure but we don't know if the eventual compute for server create will support it either | |
| 15:40:01 | mriedem | anyway, see my reply to Kevin_Zheng's email in the ML | |
| 15:40:11 | jangutter | stephenfin: re: https://review.openstack.org/#/c/625311/ I spoke in PM to gibi about this, I think the next review in the series shows why it's needed: the tag is propagated to the scheduler. | |
| 15:40:45 | artom | mriedem, dammit, yeah, _unshelve_instance() straight up driver.spawn()s | |
| 15:41:10 | mriedem | for that matter, | |
| 15:41:21 | mriedem | we could be migrating an instance with device tags to another host that doesn't support it | |
| 15:41:29 | mriedem | the breakdown is that we just have no filtering for device tags in the scheduler | |
| 15:41:43 | artom | mriedem, true. We couldn't, back then | |
| 15:41:53 | artom | Now in New Placement Future, we could add it | |
| 15:41:53 | mriedem | we could, we just didn't | |
| 15:42:12 | mriedem | i think at the time we just said "aggregates?" | |
| 15:42:17 | mriedem | and conveniently forgot about it | |
| 15:42:22 | artom | mriedem, no, we just didn't consider it, I think | |
| 15:42:27 | artom | Well, except for the boot case | |
| 15:42:28 | mriedem | i remember asking about a scheduler filter | |
| 15:42:53 | artom | Well, we already have ComputeCapabilitiesFilter... | |
| 15:42:55 | mriedem | because the server create aborts | |
| 15:42:57 | mriedem | which is pretty severe | |
| 15:43:01 | artom | Should get shoved in there, somewhere, no? | |
| 15:43:11 | mriedem | that would have been the option | |
| 15:43:35 | mriedem | anyway the better answer now is likely based on https://review.openstack.org/#/c/538498/ | |
| 15:44:01 | mriedem | with a placement request filter | |
| 15:44:34 | artom | mriedem, I see - New Placement Future indeed ;) | |
| 15:45:22 | mriedem | having said all this, anyone running mixed hypervisors in the same region is probably asking for trouble | |
| 15:45:36 | mriedem | and anyone running only a hypervisor driver that doesn't support tags should be disabling it in the api via policy | |