| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-06-22 | |||
| 09:09:06 | bauzas | nevermind, found it | |
| 09:09:15 | bauzas | this is just legacy | |
| 09:09:45 | bauzas | I can't see a single job running with a singleconductor | |
| 09:15:34 | stephenfin | bauzas: maybe remove it so? | |
| 09:15:41 | stephenfin | *we should | |
| 09:15:47 | bauzas | yup, think so | |
| 10:07:34 | sean-k-mooney1 | stephenfin the xml should always be ethernet | |
| 10:08:40 | sean-k-mooney1 | stephenfin: it is useing ethernet in that segment which is good | |
| 10:08:54 | sean-k-mooney1 | but it should spawn with it too | |
| 10:09:51 | sean-k-mooney1 | stephenfin: ok so we need to determin why its only taking effect on migration and not on inital spawn | |
| 10:10:12 | sean-k-mooney1 | that narrows it down at least | |
| 10:14:54 | stephenfin | sean-k-mooney: We never set the 'delegate_create' attribute of the VIF object anywhere except as part of a migration | |
| 10:15:37 | stephenfin | sean-k-mooney: https://github.com/openstack/nova/commit/a62dd42c0dbb6b2ab128e558e127d76962738446#diff-d47af556a60631b879ac4f609e24b5616aa81643d152583b1591ef90848382bbR108 | |
| 10:16:01 | stephenfin | which means it's never set as part of the os-vif object https://github.com/openstack/nova/commit/a62dd42c0dbb6b2ab128e558e127d76962738446#diff-918e740344fd54062b1d2040fc7d83a6fb349e33a484b3a442d154977a7f872bR350 | |
| 10:16:05 | sean-k-mooney | ah ok that was my orginal error then. i used to do it uncondtionally then i added the code to do the patch detection | |
| 10:17:05 | sean-k-mooney | so we want to default it to true but have the migration code check and disable if the dest is not supported | |
| 10:19:49 | stephenfin | I can look into that. It's a different issue than the one I'm fixing in that patch though, so I think that patch is still valid by itself | |
| 10:22:03 | opendevreview | Jorhson Deng proposed openstack/nova master: recheck the attachment_id after the reschedule successful https://review.opendev.org/c/openstack/nova/+/796209 | |
| 10:22:35 | sean-k-mooney | it is but without that other change its a really odd behavior | |
| 10:22:45 | sean-k-mooney | i kind of think they need to be in one patch | |
| 10:23:16 | sean-k-mooney | we dont want the interface to be differn after a live migration and have it chagne back after a hard reboot do we | |
| 10:23:26 | sean-k-mooney | we could do that but it feels odd | |
| 10:23:59 | sean-k-mooney | its also not what i had planned in that libvirt would still be invovled in port plugging | |
| 10:24:41 | sean-k-mooney | stephenfin: we can just set it here i belive https://github.com/openstack/nova/blob/master/nova/network/neutron.py#L2972 | |
| 10:26:10 | sean-k-mooney | although that technically would mean it should be in the vif model object | |
| 10:27:06 | opendevreview | Jorhson Deng proposed openstack/nova master: recheck the attachment_id after the reschedule successful https://review.opendev.org/c/openstack/nova/+/796209 | |
| 10:28:02 | sean-k-mooney | this is where we make the desision currently | |
| 10:28:25 | stephenfin | sean-k-mooney: we create the VIF just after we call that function https://github.com/openstack/nova/blob/5979c648462b03a2fe90148f20f099c964cdd298/nova/network/neutron.py#L3078 | |
| 10:28:31 | sean-k-mooney | stephenfin: we need create port to default to true | |
| 10:28:36 | stephenfin | so we can just set it there always | |
| 10:28:55 | stephenfin | there's no reason it ever needs to be false - it will be ignored for backends where it doesn't make sense | |
| 10:29:46 | sean-k-mooney | am we could but you will have to extend the vif model object if you do that which will change the versioned notifcations | |
| 10:29:51 | sean-k-mooney | and that wont be backportable | |
| 10:30:04 | sean-k-mooney | we need to do this slightly different | |
| 10:30:11 | stephenfin | that's no an o.vo | |
| 10:30:13 | stephenfin | *not | |
| 10:30:18 | sean-k-mooney | its not but its in one | |
| 10:30:23 | sean-k-mooney | the network info cache | |
| 10:30:35 | sean-k-mooney | its got a list of vifs | |
| 10:30:55 | stephenfin | but the VIF model has already been modified https://github.com/openstack/nova/commit/a62dd42c0dbb6b2ab128e558e127d76962738446#diff-dfa43d5033ab6af143d8772d584ed93255e60deb7bc58d87eb7305b19f8fa2ffR381 | |
| 10:30:59 | sean-k-mooney | i guess we normally dont change for compostion but it does change the hash | |
| 10:31:24 | sean-k-mooney | by addign a constant and properties | |
| 10:31:31 | sean-k-mooney | both of which do not change the version of the object | |
| 10:31:51 | sean-k-mooney | oh | |
| 10:31:56 | sean-k-mooney | sorry you ar just setting the property | |
| 10:32:10 | sean-k-mooney | ya so that is why that property stores its data in the profile | |
| 10:32:20 | sean-k-mooney | to avoid the ovo change and be backportable | |
| 10:32:22 | sean-k-mooney | ok | |
| 10:32:32 | sean-k-mooney | so yes we can just set the property to true uncondtionally | |
| 10:33:37 | sean-k-mooney | but we als have https://github.com/openstack/nova/blob/5979c648462b03a2fe90148f20f099c964cdd298/nova/network/model.py#L405 | |
| 10:33:41 | sean-k-mooney | ok im confused | |
| 10:34:01 | sean-k-mooney | i tought that was an ovo issue but maybe not | |
| 10:34:06 | stephenfin | those are just dics | |
| 10:34:08 | stephenfin | *dicts | |
| 10:34:17 | stephenfin | so we can store whatever we want in them | |
| 10:34:17 | sean-k-mooney | ok so we just pass delegate_create=True | |
| 10:34:44 | sean-k-mooney | stephenfin: they are but they are also https://github.com/openstack/nova/blob/master/nova/objects/instance_info_cache.py#L38 | |
| 10:35:21 | sean-k-mooney | i guess thats a network model | |
| 10:35:31 | stephenfin | all that that's doing is calling '.json' on the object https://github.com/openstack/nova/blob/5979c648462b03a2fe90148f20f099c964cdd298/nova/objects/fields.py#L1084 | |
| 10:35:37 | stephenfin | so it doesn't really matter | |
| 10:36:08 | sean-k-mooney | well its is using https://github.com/openstack/nova/blob/5979c648462b03a2fe90148f20f099c964cdd298/nova/network/model.py#L512 | |
| 10:36:19 | sean-k-mooney | ok i guess its fine | |
| 10:36:28 | sean-k-mooney | i had issue adding other filds in the past | |
| 10:36:37 | sean-k-mooney | but maybe i was chanign something else | |
| 10:36:52 | sean-k-mooney | in anycase i think your right we can just set it to true there | |
| 10:37:11 | sean-k-mooney | but we need to ensure in the migration path we set it to false wehn its not present orginally | |
| 11:13:44 | gibi | stephenfin, sean-k-mooney: for me https://review.opendev.org/c/openstack/nova/+/797142 looks fine, but sean-k-mooney has a -1 on it so I'm affraid of approving it | |
| 11:15:55 | sean-k-mooney | gibi: thats what we were discussign above stephen fixed it for migration but its still using the wrong interface type on boot | |
| 11:16:17 | gibi | OK, so there will be changes. thanks | |
| 11:16:26 | sean-k-mooney | so with that patch as is we boot with interface type=bridge then we migrate with ethernet and hard reboot back to bridge | |
| 11:16:49 | sean-k-mooney | the orignal intent was to always use ethernet | |
| 11:17:21 | sean-k-mooney | gibi: technically as is this should actully fix the migration issue we were trying to fix | |
| 11:17:35 | sean-k-mooney | but its a little odd to change the type just for the migration | |
| 11:17:54 | sean-k-mooney | im also worried that this current state breaks ovs on windows | |
| 11:18:15 | sean-k-mooney | so i would prefer to fix the boot case. | |
| 11:18:31 | opendevreview | Merged openstack/nova stable/victoria: [neutron] Get only ID and name of the SGs from Neutron https://review.opendev.org/c/openstack/nova/+/787252 | |
| 11:19:03 | opendevreview | Stephen Finucane proposed openstack/nova master: libvirt: Always delegate OVS plug to os-vif https://review.opendev.org/c/openstack/nova/+/797428 | |
| 11:20:05 | stephenfin | sean-k-mooney: gibi: ^ there's the follow-up patch. I need to figure out test coverage (currently all unit tests are passing and I'm running functional tests now, but I guess I should add a new test). We can proceed with the current patch as-is though IMO | |
| 11:20:13 | stephenfin | it definitely fixes one issue | |
| 11:20:57 | sean-k-mooney | well what about for backporting | |
| 11:21:20 | sean-k-mooney | unless you are going to squash all 3 patches you would have to undo the squashing you have already done | |
| 11:22:15 | stephenfin | Not necessarily. The main patch only ever fixed the issue for live migration but had a bug which we've squashed the fix in for. This new patch stands on its own feet | |
| 11:22:25 | sean-k-mooney | it was ment to do it always | |
| 11:22:46 | sean-k-mooney | ti did do it always before i put in the detection mechanium for upgrades | |
| 11:23:00 | stephenfin | Also, I don't think hard reboot is an issue. We'll have set the 'delegate_create' attribute of the VIF entries as part of live migration and nothing unsets that | |
| 11:23:15 | sean-k-mooney | stephenfin: well i would prefer not to backport it in its current state | |
| 11:23:17 | stephenfin | So it'll change during live migration but it shouldn't change after a hard reboot | |
| 11:23:34 | sean-k-mooney | it will revert back to bridge in a hard reboot | |
| 11:23:46 | sean-k-mooney | without the final patch | |
| 11:23:52 | stephenfin | why? | |
| 11:24:04 | sean-k-mooney | because bridge is what we use for spawn | |
| 11:24:23 | stephenfin | unless the VIF objects in network_info have delegate_create=True | |
| 11:24:32 | sean-k-mooney | which they wont | |
| 11:24:49 | stephenfin | again, why? | |
| 11:24:50 | sean-k-mooney | the migration data vifs are not the same as the ones in the info cache | |
| 11:25:15 | sean-k-mooney | the ones in the info cache get rebuilt form neutron by the perodic task | |
| 11:25:26 | sean-k-mooney | or whenever we get a network-changed event | |
| 11:25:40 | sean-k-mooney | so delegate_create=True will be lost | |
| 11:25:56 | sean-k-mooney | if its ever populated in it in the first place | |
| 11:26:32 | stephenfin | ah, TIL | |