| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-04-16 | |||
| 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 | bauzas | https://bugs.launchpad.net/nova/+bug/1778563 | |
| 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: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 | stephenfin | I'd have approved this already were it not for that | |
| 15:19:43 | bauzas | that's the issue i see ^ | |
| 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 | |
| 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 | |