| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-04-16 | |||
| 13:12:44 | bauzas | heh, s/barf/bark of course | |
| 13:12:57 | bauzas | (pardon my French (c) ) | |
| 13:21:09 | kashyap | lyarwood: Zuul (grenade-py3) failure on this: https://review.opendev.org/#/c/702021/ | |
| 13:21:23 | kashyap | If you've already seen it, disregard me. | |
| 13:24:58 | lyarwood | kashyap: yeah unrelated | |
| 13:25:07 | lyarwood | thanks for looking | |
| 13:26:57 | kashyap | bauzas: FWIW, this is straightforward to just merge this: https://review.opendev.org/#/c/702021/ (libvirt: Remove VIR_DOMAIN [...]) | |
| 13:28:43 | kashyap | (And the 'qemu-img' one, too.) | |
| 14:47:49 | gibi | lyarwood, bauzas, stephenfin: Am I missing something here https://review.opendev.org/#/c/711679/5/nova/virt/images.py@42 ? | |
| 14:49:31 | stephenfin | Oh, it looks like it | |
| 14:50:02 | lyarwood | gibi: nope it's unused | |
| 14:50:25 | sean-k-mooney | well its used here https://review.opendev.org/#/c/711679/5/nova/virt/images.py@48 | |
| 14:50:37 | sean-k-mooney | but i dont know if its everset to anything other then json | |
| 14:50:54 | bauzas | gibi: lyarwood: wait, it can be | |
| 14:51:01 | bauzas | sec, finding the github link | |
| 14:51:49 | bauzas | gibi: lyarwood: stephenfin: https://github.com/openstack/nova/blob/e1359567e4985e9a671359d4c0d53404a8ba64ab/nova/virt/libvirt/utils.py#L224 | |
| 14:52:14 | stephenfin | bauzas: that's format, not output_format | |
| 14:53:35 | sean-k-mooney | looking at the reference on github the only place output_formate is ever set today | |
| 14:53:47 | sean-k-mooney | is https://github.com/openstack/nova/blob/e1359567e4985e9a671359d4c0d53404a8ba64ab/nova/tests/unit/virt/libvirt/test_utils.py#L167 | |
| 14:53:52 | sean-k-mooney | where it is currently set to json | |
| 14:54:04 | gibi | sean-k-mooney: at https://review.opendev.org/#/c/711679/5/nova/virt/images.py@48 we can hardcode the output_format to 'json' | |
| 14:54:13 | sean-k-mooney | gibi: yes | |
| 14:54:23 | sean-k-mooney | and remove the parmanter form our method | |
| 14:54:25 | stephenfin | burn it with fire | |
| 14:54:31 | lyarwood | there's actually a place in the driver as well | |
| 14:54:34 | lyarwood | but we can drop it from there | |
| 14:54:36 | lyarwood | 1 sec | |
| 14:55:58 | sean-k-mooney | so useing codesearch http://codesearch.openstack.org/?q=qemu_img_info&i=nope&files=&repos=openstack/nova | |
| 14:57:30 | sean-k-mooney | its either not set or set to json our side of the funtion definitions | |
| 14:58:11 | sean-k-mooney | since you change the defualt to json in the patch any fucntion that did not set it already works with json | |
| 14:58:23 | sean-k-mooney | so ya i think we can just hardcode it and drop it | |
| 14:58:45 | sean-k-mooney | or at least push a patch to do that and see what breaks | |
| 14:59:19 | stephenfin | bauzas: comments left on https://review.opendev.org/#/c/712741/ | |
| 14:59:31 | stephenfin | Looks like they're mostly the same as sean-k-mooney's (de-duping stuff) | |
| 14:59:36 | bauzas | stephenfin: ack, looking | |
| 15:00:30 | stephenfin | dansmith, gibi, melwitt: Thoughts on merging https://review.opendev.org/#/c/589085/ ? | |
| 15:01:32 | stephenfin | given that it touches the driver API. Is it still okay to land after feature freeze? | |
| 15:06:16 | sean-k-mooney | ya i had that question too but the notifcation was also already sent to the mailing list of the change about a month ago | |
| 15:07:50 | gibi | we need to figure out how risky is this change. what can we break? | |
| 15:08:20 | dansmith | what's the point of merging that, if not just to enable a feature? | |
| 15:08:48 | dansmith | IMHO, unless it gets us something by landing it in U to set up for fewer migration problems in V, it's really not worth doing at this point | |
| 15:09:24 | dansmith | meaning, if this got us a cycle earlier on data migration or something, but I don't see that unless I'm missing something | |
| 15:13:32 | bauzas | gibi: dansmith: sean-k-mooney: stephenfin: I wrote a ML thread 1 month ago about this and honestly it's an internal API | |
| 15:13:47 | sean-k-mooney | this is required for resize its not related to the multi gpu types so it has no impact on upgrde or data migrations | |
| 15:13:53 | bauzas | and there is no object impact or RPC call involved in any matter | |
| 15:14:03 | dansmith | sean-k-mooney: right | |
| 15:14:40 | dansmith | sean-k-mooney: that's my point.. if we're not going to merge the feature it enables, and it doesn't set us up for something early, then merging it now vs. as soon as V opens doesn't matter, except for risk and change yeah? | |
| 15:15:36 | bauzas | there is no feature behind it | |
| 15:15:52 | bauzas | we already support instances resizes | |
| 15:15:53 | dansmith | bauzas: oh is it a bug fix? | |
| 15:15:58 | bauzas | yup | |
| 15:16:14 | bauzas | I wouldn't beg for a FFE | |
| 15:16:16 | bauzas | if so | |
| 15:16:18 | openstack | Launchpad bug 1778563 in OpenStack Compute (nova) "Resize/Cold-migrate doesn't recreate vGPUs" [High,In progress] - Assigned to Sylvain Bauza (sylvain-bauza) | |
| 15:16:18 | bauzas | https://bugs.launchpad.net/nova/+bug/1778563 | |
| 15:16:37 | bauzas | we just blindly resize the instance with forgetting the vgpus | |
| 15:16:52 | dansmith | ah, sorry I figured this was for new stuff | |
| 15:16:53 | bauzas | so, 'whoops, sorry' basically | |
| 15:17:15 | bauzas | adding a new call to _allocate_mdevs() will just return if no VGPUs involvedf | |
| 15:17:20 | bauzas | that's basically a noop | |
| 15:17:27 | bauzas | (for instances with no VGPUs in there) | |
| 15:17:31 | dansmith | is this new in U or latent since T? | |
| 15:17:38 | bauzas | latent since Queens :) | |
| 15:18:00 | bauzas | https://review.opendev.org/#/c/720250/1/doc/source/admin/virtual-gpu.rst | |
| 15:18:07 | dansmith | ah, then probably not worth rushing into U then | |
| 15:18:21 | dansmith | I haven't reviewed the stack so I'm not sure how much risk, granted | |
| 15:18:34 | dansmith | but if no object, rpc, or db changes then it could be backported even right? | |
| 15:19:27 | stephenfin | I don't think so. It has virt driver API changes, which means a backport would break out-of-tree drivers | |
| 15:19:43 | bauzas | that's the issue i see ^ | |
| 15:19:43 | stephenfin | I'd have approved this already were it not for that | |
| 15:20:00 | bauzas | I warned the out-of-tree driver maintainers that we were changing the resize signature | |
| 15:20:13 | stephenfin | this hunk, basically https://review.opendev.org/#/c/589085/8/nova/virt/driver.py | |
| 15:20:30 | dansmith | I'm not aware of ever blocking a backport on those grounds | |
| 15:20:39 | dansmith | not sure why that would matter at all, when it doesn't for master | |
| 15:20:44 | stephenfin | o rly? | |
| 15:20:47 | bauzas | I dunno, honestly | |
| 15:21:03 | bauzas | either way, if folks prefer to punt it to V, fair enough | |
| 15:21:12 | stephenfin | I figured people had notice on master and would expect to have to do some work with a new major version | |
| 15:21:15 | stephenfin | not so a new minor one | |
| 15:21:19 | bauzas | I just thought it wouldn't harm (c) :p | |
| 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 | |