| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-04-16 | |||
| 15:22:10 | artom | dansmith, not sure we ever blocked a backport, but there was a case where we made a point of *not* changing the driver interface to enable the backport | |
| 15:22:19 | artom | And then in a FUP re-did the thing the "correct" way | |
| 15:22:55 | artom | https://review.opendev.org/#/c/670645/2/nova/virt/libvirt/driver.py@9045 | |
| 15:23:04 | dansmith | artom: because it was doable without I guess? either way, I bet I wasn't the one worried about it :) | |
| 15:23:05 | bauzas | dansmith: stephenfin: gibi: shouldn't we just discuss this as a 'Stuck review' during the nova meeting in 48 mins ? | |
| 15:23:17 | bauzas | and find a consensus there so we move on | |
| 15:23:19 | artom | dansmith, it may have been mriedem, don't remember | |
| 15:23:28 | dansmith | sounds like him :) | |
| 15:23:35 | stephenfin | bauzas: um, sure, why not | |
| 15:24:01 | bauzas | again, that doesn't really change my business | |
| 15:24:15 | bauzas | if we go accepting this change, that's cool | |
| 15:24:18 | artom | (And here's the "correct" fup: https://review.opendev.org/#/c/668631/5 ) | |
| 15:24:23 | bauzas | if we prefer be conservative, then meh | |
| 15:24:44 | bauzas | but I just feel we somehow need to decide on what to do with internal virt API changes when we're post FF | |
| 15:25:04 | artom | ... and I totally messed it up anyways | |
| 15:26:44 | dansmith | the actual fix is the third patch right? | |
| 15:28:17 | sean-k-mooney | yes the rest is just passing the allocations | |
| 15:28:32 | bauzas | dansmith: yup | |
| 15:30:35 | sean-k-mooney | you could actully just use the reportclient to get the allcoation in the driver to avoid the virt api change too but that is less clean. | |
| 15:30:36 | dansmith | I'll have to go learn what _allocate_mdevs() does in order to review that patch I guess | |
| 15:30:48 | dansmith | sean-k-mooney: yeah, don't do that, IMHO | |
| 15:30:59 | bauzas | sean-k-mooney: NO | |
| 15:31:14 | sean-k-mooney | bauzas: im not saying we should | |
| 15:31:29 | bauzas | the virt driver is an island :) | |
| 15:32:18 | bauzas | dansmith: _allocate_mdevs() is the process that checks the configuration to see whether it should assign some kernel mediated devices (in other words, vgpus) to the guest XML | |
| 15:33:43 | dansmith | bauzas: right, but if we're reverting, it had an mdev before right? | |
| 15:33:51 | dansmith | I just need to look at how all that works is all | |
| 15:33:56 | gibi | bauzas: if we not merge the virt driver interface change in U would you like to backport the V change to stable/U later? | |
| 15:33:56 | mriedem | what did i do? | |
| 15:34:16 | mriedem | oh the TypeError check on a backport? | |
| 15:34:18 | bauzas | gibi: yeah, if that doesn't break the stable policy, which I'm unsure | |
| 15:34:30 | bauzas | stable rules* policy | |
| 15:35:04 | alex_xu | gibi: sean-k-mooney, we are just working on ensuring the requirement on https://review.opendev.org/#/q/topic:bp/provider-config-file+(status:open+OR+status:merged | |
| 15:35:21 | sean-k-mooney | bauzas: altering the virt driver internal api i think should be a stable policy violation even if its not a public api | |
| 15:35:49 | dansmith | totes disagree :) | |
| 15:35:58 | dansmith | as you all know | |
| 15:36:38 | gibi | it is basically a decision between changing the virt driver interface in a major versior (now) or in a minor version (later in a backport) | |
| 15:36:41 | sean-k-mooney | yes although if we really dont want to provide any support for out of tree driver i would suggest we also stop sending the email when we change things | |
| 15:36:49 | sean-k-mooney | which i think you are also in faovr of | |
| 15:36:54 | dansmith | indeed :) | |
| 15:37:10 | bauzas | that's the exact reason why I want to raise it as a stuck review | |
| 15:37:13 | bauzas | go fight it ! | |
| 15:37:36 | bauzas | and honestly, I don't really care of shipping it in U if you wanna get MHO | |
| 15:37:41 | bauzas | because Red Hat... | |
| 15:37:55 | sean-k-mooney | bauzas: we are not planning to back port this downstream beyond ussuri right? | |
| 15:38:06 | bauzas | sean-k-mooney: don't say gross words | |
| 15:38:30 | bauzas | we *could* but do we want, honestly ? :) | |
| 15:38:53 | bauzas | dansmith: for the revert case, that's an interesting question | |
| 15:39:08 | sean-k-mooney | just triple checing that we will deliver this to customers in 17 which will be based on victoria | |
| 15:39:29 | gibi | alex_xu: thanks. If it turns out that there are requirements on Intel side then will there be some dev from intel to push the patches in V? | |
| 15:39:31 | bauzas | oh man, don't get me spilling my business here :) | |
| 15:39:41 | bauzas | but yeah, 17 so U or V, I doesn't matter | |
| 15:39:45 | alex_xu | gibi: yes, I think so | |
| 15:40:02 | gibi | alex_xu: cool. thanks. let me know the result of your investigation | |
| 15:40:17 | alex_xu | gibi: yea, I will let you know whatever the result coming out | |
| 15:40:46 | openstackgerrit | Lee Yarwood proposed openstack/nova master: images: Make JSON the default output format of calls to qemu-img info https://review.opendev.org/711679 | |
| 15:40:47 | lyarwood | ^ now with added unit tests, thanks again all. | |
| 15:45:18 | sean-k-mooney | bauzas: for the revert case we have not undeifed the domain on the source node when we trigger revert | |
| 15:46:02 | sean-k-mooney | so im not sure the allocation are truly needed as we should not need to regenerate the xml | |
| 15:46:39 | sean-k-mooney | that said we might do anyway depending on how we restart the vm | |
| 15:46:55 | bauzas | sean-k-mooney: we regenerate the XML https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L10259 | |
| 15:47:04 | sean-k-mooney | if we just call power on it call hard reboot which would | |
| 15:48:03 | sean-k-mooney | ya ok so that is why we need it in this case. | |
| 15:48:45 | bauzas | yuuuuuup | |
| 15:49:02 | bauzas | that does sound crazy, but we blindy recreate the guest when reverting | |
| 15:49:21 | bauzas | sean-k-mooney: see my functtest, you'll see evidence of it | |
| 15:49:27 | sean-k-mooney | im not sure if we really need to regenerate the xml in that case i cant think of why but lets not get distracted by that for now | |
| 15:49:28 | dansmith | sean-k-mooney: right that was my exact concern | |
| 15:50:04 | sean-k-mooney | oh i know why i think | |
| 15:50:08 | dansmith | where does allocate_mdevs persist? does it look at all the other mdevs currently being used? | |
| 15:50:24 | dansmith | meaning, when we undefine the original and re-define with a new one, will the old one be available again, or might we leak something? | |
| 15:50:34 | sean-k-mooney | we regenerate the xml becasue snapshots | |
| 15:50:58 | sean-k-mooney | this code https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L10249-L10254 | |
| 15:51:09 | bauzas | dansmith: mdevs aren't persisted | |
| 15:51:34 | bauzas | mdevs can be even pre-provisioned | |
| 15:51:39 | dansmith | sean-k-mooney: also maybe because you can attach a volume to the new one before revert | |
| 15:51:54 | dansmith | bauzas: how do we select one that isn't in use? that's what I'm getting at | |
| 15:51:59 | bauzas | "allocating a mdev" just means modifying the guest XML domain to use a specific sysfs object | |
| 15:52:05 | bauzas | dansmith: oh this, sec | |
| 15:52:16 | dansmith | right, I know, but we need to make sure we hand it one not in use | |
| 15:52:38 | bauzas | dansmith: https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L7206 | |
| 15:52:59 | dansmith | bauzas: so we create a new one in sysfs? | |
| 15:53:17 | sean-k-mooney | ya i guess that is ture although im realy not sure why we allow volume or interface attach in resize_verify or whatever that state is called | |
| 15:53:23 | dansmith | bauzas: how does the old one get reaped when we redefine? | |
| 15:53:27 | bauzas | dansmith: which is made by comparing all the existing mdevs and the ones that are in use by each domain | |
| 15:53:28 | bauzas | https://github.com/openstack/nova/blob/e1359567e4985e9a671359d4c0d53404a8ba64ab/nova/virt/libvirt/driver.py#L7088-L7102 | |
| 15:53:32 | sean-k-mooney | but given that we do we cant assume the current xml is correct | |
| 15:53:36 | dansmith | sean-k-mooney: we do, but it's crazy | |
| 15:54:16 | dansmith | bauzas: right so when we revert, we'll see that our original one is in use (by us) and allocate a new one right? | |
| 15:54:51 | bauzas | dansmith: during the resize, before the confirm, we have a doubled allocation of mdevs | |
| 15:55:02 | bauzas | (and a double placement allocation) | |
| 15:55:27 | bauzas | dansmith: when we revert, the target mdev allocation disappears because we undefine the domain | |
| 15:55:35 | dansmith | bauzas: but that doubling is on two different hosts | |
| 15:55:38 | sean-k-mooney | https://github.com/openstack/nova/blob/e1359567e4985e9a671359d4c0d53404a8ba64ab/nova/virt/libvirt/driver.py#L6349 | |
| 15:56:04 | sean-k-mooney | it look like the domain creation fucntion will overwrite existing domains | |
| 15:56:18 | dansmith | bauzas: I'm talking about when we revert, don't we hit the "no free mdevs, allocate a new one" in _allocate_mdevs() since our original mdev is still in use by the libvirt domain we're about to redefine? | |
| 15:56:20 | bauzas | dansmith: yeah https://review.opendev.org/#/c/712741/2/nova/tests/functional/libvirt/test_vgpu.py@224 | |
| 15:56:56 | bauzas | dansmith: I see your point and I honestly have to double check | |
| 15:57:11 | sean-k-mooney | oh you were asking about the allcotaion fo the mdevs | |
| 15:57:27 | dansmith | sean-k-mooney: yes | |