| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-06-22 | |||
| 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 | |
| 13:15:39 | gibi | if we do that the the default should be still to validate the policy | |
| 13:15:39 | sean-k-mooney | the real question is why is the existing code not validating it when we have the json scmea files to do that and unit test that apprently are testing the validation | |
| 13:16:08 | sean-k-mooney | gibi: yep agreed if we have a config option it shoudl default to validating | |
| 13:16:25 | gibi | good question. unfortunately I have swamped with other things right now | |