| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-06-22 | |||
| 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 | |
| 11:26:50 | stephenfin | okay, so I'll squash the third patch in too so | |
| 11:27:31 | sean-k-mooney | ok am can we wait for grenade to run on the 3rd patch then we can merge the second one? | |
| 11:27:39 | sean-k-mooney | i just want to make sure that it is not breaking anything | |
| 11:29:38 | stephenfin | we really need to mute that SQLAlchemy warning | |
| 11:29:44 | sean-k-mooney | yep | |
| 11:29:52 | stephenfin | I've a fix proposed to oslo.db | |
| 11:29:59 | stephenfin | https://review.opendev.org/c/openstack/oslo.db/+/797426 | |
| 11:30:00 | sean-k-mooney | ah nice | |
| 11:30:22 | sean-k-mooney | i was assuming we woudl set that to false | |
| 11:30:54 | sean-k-mooney | are we ok with caching softdelete or json data | |
| 11:31:55 | sean-k-mooney | The requirements for cacheable elements is that they are hashable and also that they indicate the same SQL rendered for expressions using this type every time for a given cache value. | |
| 11:31:57 | sean-k-mooney | ok | |
| 11:32:02 | sean-k-mooney | so ya that is correct | |
| 11:32:10 | sean-k-mooney | cache_ok applies | |
| 12:39:30 | sean-k-mooney | stephenfin: the full job suite is not finished yet but the live migration job passed and is now using ethernet so thats good to see just waiting on grenade now | |
| 12:52:07 | sean-k-mooney | gibi: gmann so intersing problem, apparently we do not validate server_group policy strings | |
| 12:52:30 | sean-k-mooney | we have never intended the server group to be user extendable right? | |
| 12:52:47 | sean-k-mooney | e.g. by using out of tree filters and weighers? | |
| 12:59:22 | sean-k-mooney | actully we do have schema validataion for thsi | |
| 12:59:24 | sean-k-mooney | https://github.com/openstack/nova/blob/5979c648462b03a2fe90148f20f099c964cdd298/nova/api/openstack/compute/schemas/server_groups.py#L22-L67 | |
| 12:59:44 | sean-k-mooney | but i guess that is not workign i need to confrim what is being repoted downstream | |
| 13:10:40 | stephenfin | sean-k-mooney: gibi: gmann: Yeah, there's a request to undo the validation of what we pass to the API in OSC because its seems someone is relying on this broken behavior https://storyboard.openstack.org/#!/story/2008975 | |
| 13:13:19 | sean-k-mooney | so i thikn we need to figure out why the json schema validation is not rejectign there request and fix that and backport it in thet api | |
| 13:14:01 | sean-k-mooney | we could have a workaround config option to disable that but this is not a valid extenion point today | |
| 13:15:01 | gibi | I agree that we should validate the policy | |
| 13:15:07 | gibi | so let's fix it | |
| 13:15:10 | gibi | and backport it | |
| 13:15:29 | gibi | I'm meh on providing the workaround config | |