Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-06-22
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 sean-k-mooney ok so we just pass delegate_create=True
10:34:17 stephenfin so we can store whatever we want in them
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

Earlier   Later