| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-04 | |||
| 15:18:06 | kashyap | mriedem: You mean, just to exercise the relevant code for tests? | |
| 15:18:18 | mriedem | yes | |
| 15:18:56 | mriedem | even if jamespage / coreycb were able to backport those patches from 3.10 to 3.6 in the pike UCA, we'd have to put a workaround option in nova to bypass the version check and just run the multiattach code | |
| 15:18:59 | hrw | kashyap: the plan is to migrate all our cloud setups from our Newton to clean Queens. from set of venvs to containers | |
| 15:19:08 | mriedem | i.e. 'i've patched my packages, so don't care about the versions you think are required' | |
| 15:19:37 | hrw | mriedem: can not libvirt report that multiattach feature in domcapabilities or sth? | |
| 15:20:01 | kashyap | Yeah, I saw your version check code in that patch series | |
| 15:20:10 | hrw | mriedem: so instead "if libvirt >= x.y" you can use "if libvirt.capabilities.contains('multiattach')" | |
| 15:20:31 | mriedem | hrw: maybe? https://github.com/libvirt/libvirt/commit/860a3c4bea1d24773d8a495f213d5de3ac48a462 | |
| 15:20:40 | hrw | version check suxx when features are not enabled on all archs | |
| 15:20:53 | mriedem | is that how this would work with ^ and "disk-share-rw"? | |
| 15:21:49 | hrw | kashyap: so, can I get +2 from you on patch? ;D | |
| 15:21:59 | kashyap | hrw: I can't +2 | |
| 15:22:11 | kashyap | hrw: But I'd like someone else's opinion, too, like mriedem | |
| 15:22:18 | kashyap | Or mdrabe | |
| 15:22:21 | hrw | kashyap: or w8... I was supposed to do something with tests... no idea how to do that part | |
| 15:22:22 | kashyap | Err, mdbooth I mean | |
| 15:22:50 | mriedem | kashyap: can you tell me if https://github.com/libvirt/libvirt/commit/860a3c4bea1d24773d8a495f213d5de3ac48a462 is used to expose a hypervisor capability? | |
| 15:23:00 | mdbooth | kashyap: ? | |
| 15:23:01 | kashyap | hrw: I have one more comment on the change | |
| 15:23:08 | kashyap | Almost about to hit send | |
| 15:23:10 | mdbooth | Which change? | |
| 15:23:12 | mriedem | like in https://github.com/openstack/nova/blob/master/nova/virt/libvirt/host.py#L615 | |
| 15:23:12 | kashyap | mriedem: 1 sec, let me look | |
| 15:23:13 | hrw | mdbooth: https://review.openstack.org/#/c/530965/ | |
| 15:23:15 | kashyap | mdbooth: https://review.openstack.org/#/c/530965/4 | |
| 15:23:33 | kashyap | mdbooth: It's arch-specific, but migration-related, too. I noted the concern there | |
| 15:24:18 | mdbooth | kashyap: I don't have a useful opinion on that unfortunately without doing my own doc diving. | |
| 15:24:32 | kashyap | mdbooth: Okido, I have enough context there. Disregard | |
| 15:25:16 | hrw | mdbooth: imho https://bugzilla.redhat.com/show_bug.cgi?id=1430987 is best part of info on subject | |
| 15:25:17 | openstack | bugzilla.redhat.com bug 1430987 in libvirt "No cpu model and feature in capabilities" [High,Assigned] - Assigned to abologna | |
| 15:25:50 | kashyap | mriedem: Still checking | |
| 15:25:57 | hrw | kashyap: thx | |
| 15:29:27 | kashyap | mriedem: Hmm, I just built the newest libvirt-python bindings, and don't see it any of the capabilities; let me ask one of the libvirt folks | |
| 15:30:16 | kashyap | mriedem: So no -- it isn't exposed via any capabilities currently (like `virsh (dom)capablities`) | |
| 15:30:39 | kashyap | mriedem: So Peter (who wrote that commit says): | |
| 15:31:16 | kashyap | 16:29 < pkrempa> kashyap: it is not exposed currently, since it's supposed to be transparent for the users | |
| 15:31:19 | kashyap | 16:30 < pkrempa> and if it's not transparent I'd suggest to complain to qemu | |
| 15:31:38 | mriedem | kashyap: ok so we're stuck with version checks | |
| 15:31:40 | mriedem | thanks for asking | |
| 15:31:56 | mriedem | we could, as noted, add a workaround config option to bypass the version checks if you've patched your packages | |
| 15:32:20 | kashyap | mriedem: But that'd be asking the user to be too awake, and alert and aware :P | |
| 15:33:01 | kashyap | More seriously, yeah - workaround config sounds good | |
| 15:33:09 | mriedem | it wouldn't be the end user, it'd be the deployer | |
| 15:33:12 | mriedem | but yes | |
| 15:33:17 | mriedem | depends on how much people want their multiattach | |
| 15:33:25 | kashyap | You mean in the [workarounds] section, right. Like that live snapshots thing (that now we removed, IIRC) | |
| 15:34:03 | mriedem | yes | |
| 15:34:04 | openstackgerrit | Marcin Juszkiewicz proposed openstack/nova master: libvirt: use 'host-passthrough' as default on AArch64 https://review.openstack.org/530965 | |
| 15:34:19 | mriedem | a "this isn't tested upstream so it's not supported thing, but it's a backdoor if you need it" | |
| 15:34:27 | kashyap | Heh, yeah | |
| 15:35:08 | mdbooth | mriedem: lyarwood I also commented on the multi attach patch. There must be a better place to stash multiattach than connection_info. That's essentially adding pain to something we're effectively deprecating. | |
| 15:35:12 | kashyap | hrw: So, on the above patch, thanks for addressing that. Besides that, I don't have anything else | |
| 15:35:27 | hrw | kashyap: thanks for help | |
| 15:35:37 | coreycb | mriedem: will get back to you hopefully shortly. i'm checking with our libvirt maintainer. | |
| 15:36:14 | mdbooth | Incidentally, I think a 'can multiattach' flag on the BDM object would be appropriate. | |
| 15:36:24 | ildikov | mdbooth: what are we deprecating? | |
| 15:36:53 | mdbooth | ildikov: We're trying to get rid of the requirement for Nova to know anything about connection_info | |
| 15:37:01 | kashyap | hrw: Oh, hang on -- same update needs to be done in the rel note. | |
| 15:37:07 | mdbooth | Ideally we can just fetch it from cinder whenever we need it. | |
| 15:37:12 | kashyap | hrw: As that's more public facing; sorry, should've caught it earlier | |
| 15:37:15 | mriedem | mdbooth: with 2 weeks to feature freeze, i think that's a future improvement | |
| 15:37:18 | hrw | kashyap: will do | |
| 15:37:30 | mriedem | mdbooth: because i don't want to drag this out for queens with a schema and object migration and all that | |
| 15:37:44 | mdbooth | ildikov: We definitely don't want to be stashing more stuff in a foreign opaque dict, and then relying on it. | |
| 15:37:52 | mriedem | mdbooth: hell, we could just add a "multiattach" boolean flag to driver.attach_volume | |
| 15:38:08 | mdbooth | mriedem: Yeah, I wondered about that, but I think it needs to be persistent. | |
| 15:38:14 | mriedem | why? | |
| 15:38:29 | mriedem | we get the multiattach value from the volume | |
| 15:38:38 | mdbooth | mriedem: Admittedly I didn't stare at it for a long time, but it's used in volume_driver.get_config() | |
| 15:38:38 | mriedem | i don't really want to persist cinder state in nova's db | |
| 15:38:50 | mriedem | mdbooth: yeah i think that's called from driver.attach_volume | |
| 15:38:51 | mdbooth | I'm pretty sure we can call that outside of the context of attach() | |
| 15:38:54 | mriedem | so we can pass a boolean arg down | |
| 15:39:00 | hrw | kashyap: I just copy comment to release notes basically in next patch | |
| 15:39:10 | mriedem | mdbooth: hmm, for live migrate maybe | |
| 15:39:11 | mriedem | yeah | |
| 15:39:13 | kashyap | hrw: Yep | |
| 15:39:18 | mdbooth | mriedem: However, if that's not true (I didn't check), that would be great | |
| 15:39:32 | ildikov | mdbooth: mriedem: I'm open to suggestions it's just the first idea I had two years ago... | |
| 15:40:18 | mdbooth | Anyway, just my 2c, and I totally get that pragmatism might be required here. | |
| 15:40:46 | mdbooth | But it raised a flag for me. We'll need to unwind it eventually. | |
| 15:41:20 | hrw | also commit message got rewritten | |
| 15:41:21 | openstackgerrit | Marcin Juszkiewicz proposed openstack/nova master: libvirt: use 'host-passthrough' as default on AArch64 https://review.openstack.org/530965 | |
| 15:41:26 | mriedem | yeah i think passing a boolean through attach_volume to get_config is easy, it's live migrate that i'm worried about | |
| 15:41:48 | ildikov | mdbooth: not the first time it came up, but we didn't manage to have this as a top priority problem as of yet to find a better way :( | |
| 15:42:33 | mdbooth | ildikov: At the very least we'll need to be able to find it in order to unwind it. | |
| 15:43:12 | kashyap | mriedem: Oh the previous point about capablities, libvirt upstream says, if I file a bug they could add it - as it shouldn't be too difficult | |
| 15:43:29 | kashyap | mriedem: That'd be cleaner for us (Nova), isn't it? | |
| 15:43:50 | mdbooth | ildikov: Incidentally, does service_uuid indicate a shared pool of volumes? | |
| 15:44:10 | mdbooth | ildikov: e.g. multiple volumes on the same NFS mount? | |
| 15:44:28 | ildikov | mdbooth: no, that's independent | |
| 15:44:40 | mdbooth | ildikov: Where can I read what it means? | |
| 15:44:55 | mriedem | kashyap: long-term that would be cleaner yes, | |
| 15:45:01 | mriedem | but not something that is going to help me in queens | |
| 15:45:37 | kashyap | Right. /me imagines: If you add the version check, and then the capability comes along later, no one will remember to swap that, until prompted by something | |
| 15:46:13 | mriedem | kashyap: feel free to file a bug if you want :) | |
| 15:46:17 | kashyap | ildikov: Can you point to the latest URL of the multi-attach specification, please? | |
| 15:46:27 | openstackgerrit | Matthew Booth proposed openstack/nova master: Use real block_device_info data in libvirt tests https://review.openstack.org/527916 | |
| 15:46:27 | openstackgerrit | Matthew Booth proposed openstack/nova master: Pass DriverBlockDevice to driver.attach_volume https://review.openstack.org/528363 | |