| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-02-22 | |||
| 09:09:13 | opendevreview | Elod Illes proposed openstack/nova stable/queens: Fix sphinx doc job https://review.opendev.org/c/openstack/nova/+/830327 | |
| 11:05:05 | opendevreview | Fabian Wiesel proposed openstack/nova master: Transport context to all threads https://review.opendev.org/c/openstack/nova/+/827467 | |
| 11:42:13 | opendevreview | Balazs Gibizer proposed openstack/nova master: Fix eventlet.tpool import https://review.opendev.org/c/openstack/nova/+/830383 | |
| 11:50:40 | chateaulav | gibi: with a multi patch series, like i have. when i go to update, do i rebase back to the driver patch and then make the appropriate changes, or do I have to submit the whole series for review each time even though the objects patch has the first +2? | |
| 11:51:22 | chateaulav | i hope that makes sense, just want to make sure i fully understand and am not creating to much work for you guys | |
| 12:00:05 | sean-k-mooney | chateaulav: the object patch is the base patch so you dont have to rebase that to work on the following patch | |
| 12:00:17 | chateaulav | ok | |
| 12:00:47 | sean-k-mooney | you have 2 ways to do it you can write a patch at the end to adress the issues and then do an interactive rebase to squash merge it into the patch its fixign | |
| 12:01:08 | sean-k-mooney | our you can start with a interactive rebase and mark the commits you want to alter for editing | |
| 12:01:25 | sean-k-mooney | in both cases you tell git to keep the base object patch exactly as is | |
| 12:01:45 | chateaulav | ok, good, thats what im doing then. just wanted to make sure | |
| 12:02:01 | sean-k-mooney | then when you finish the rebase it will have only modified the following commits and the git review at the end will not update the first review | |
| 12:02:22 | sean-k-mooney | you can also pass -R to git review to ensure it does not do any automatic rebases for you | |
| 12:05:38 | sean-k-mooney | gibi: did you see ralonsoh comments on https://review.opendev.org/c/openstack/neutron/+/829247 | |
| 12:06:01 | sean-k-mooney | gibi: while form a nova perspeictive it looks like the port is fully updated apprently its not fully updated in the db | |
| 12:12:37 | opendevreview | yuval proposed openstack/nova master: Lightbits LightOS driver https://review.opendev.org/c/openstack/nova/+/821606 | |
| 12:16:25 | gibi | sean-k-mooney: now read ralonsoh's comment | |
| 12:17:26 | gibi | sean-k-mooney: I'm not a neutron expert so it can very well be that the change is not persisted | |
| 12:17:33 | sean-k-mooney | the fact that what ralonsoh commented is possibel today speaks to a larger problem in neutron IMO but i guess for now you just need to ensure the db version is also updated | |
| 12:17:52 | ralonsoh | sean-k-mooney, what other problem? | |
| 12:17:52 | sean-k-mooney | gibi: i think its possible perseited but only in memory | |
| 12:18:11 | sean-k-mooney | ralonsoh: teh fact that you can partly update a port but later api calls will show it as updated | |
| 12:18:15 | sean-k-mooney | even though the db is not | |
| 12:18:52 | sean-k-mooney | it suggest there is a problem with caching or how neutron is retriving data | |
| 12:19:14 | gibi | ralonsoh: if you happen to have a pointer where shoudl the mac_address be updated in neutron for this to work then please let me know that will help me updating the patch properly | |
| 12:19:16 | ralonsoh | sean-k-mooney, no no, this is not the case | |
| 12:19:40 | ralonsoh | gibi, yes, let me work on this today (or tomorrow morning) | |
| 12:19:44 | ralonsoh | I'll focus on this | |
| 12:20:05 | sean-k-mooney | ralonsoh: the unit test are doing a port show to get the port after it was updated, and you said you could see the chage in port show/list | |
| 12:20:06 | gibi | ralonsoh: thanks | |
| 12:20:21 | sean-k-mooney | eventhough you also said the db record still had the old mac | |
| 12:20:37 | sean-k-mooney | so there is clearly a disconenct in my and your understanding | |
| 12:20:41 | ralonsoh | sean-k-mooney, yes, this is because you populate the port dictionary (the one that is going to be in the JSON) with the port register info | |
| 12:20:51 | ralonsoh | and other related db registers | |
| 12:21:32 | ralonsoh | sean-k-mooney, the port dictionary comes from how neutron was implemented 10 years ago | |
| 12:21:35 | ralonsoh | without OVOs | |
| 12:21:42 | sean-k-mooney | right so you are not just readign the port info form the db | |
| 12:21:53 | ralonsoh | the OVO is a DB view, linking several DB registers | |
| 12:22:06 | ralonsoh | this method (and most of the _dict_* ones) | |
| 12:22:24 | ralonsoh | are only methods to populate in one dict several DB registers data | |
| 12:23:20 | sean-k-mooney | but its clearly possibel for the mac to have 2 different values in two places so db consitecy is not enforced | |
| 12:23:20 | gibi | OK I think I see. Every time a port is returned it will hit the code I added so that code will apply the MAC from the binding _on the port dict being returned_ | |
| 12:23:45 | gibi | but I never apply the MAC from the binding on a port that is being saved to the db | |
| 12:23:56 | sean-k-mooney | gibi: ya you coudl validate that by claring the mac form the binding profile | |
| 12:23:57 | ralonsoh | sean-k-mooney, no, it is not. But this is why we should review what we populate in the resource dictionaries | |
| 12:24:00 | sean-k-mooney | and asserting it does not change | |
| 12:24:58 | sean-k-mooney | ralonsoh: well our actully remove them and just use the ovos | |
| 12:25:25 | sean-k-mooney | we still ahve some dicts in nova too like the bdms | |
| 12:25:38 | sean-k-mooney | but we try to not use dicst where we can avoid it | |
| 12:25:45 | ralonsoh | sean-k-mooney, those _dict_* methods are also used by the extensions | |
| 12:25:54 | ralonsoh | to populate new data in the resource dictionaries | |
| 12:26:23 | ralonsoh | in any case, changing this now is complex and an unnecessary refactor | |
| 12:27:16 | sean-k-mooney | we can disagree on unnesesary but it would prevent the possibleity fo havign the db and runtime view getting out of sync in this way | |
| 12:27:28 | sean-k-mooney | well or at least reduce it | |
| 12:28:08 | sean-k-mooney | so right here at the end https://review.opendev.org/c/openstack/neutron/+/829247/2/neutron/tests/unit/plugins/ml2/test_port_binding.py#776 | |
| 12:28:19 | sean-k-mooney | if we just add a call to clear the mac in the binding profile | |
| 12:28:41 | sean-k-mooney | and then assert the mac has the new value it should fail | |
| 12:29:00 | sean-k-mooney | since it will nolonger triger gibis change and we will get the mac from the db right | |
| 12:29:19 | gibi | hm that acutally has an interesting side effect | |
| 12:29:36 | gibi | if a port was bound to a PF then unbound from it | |
| 12:29:43 | gibi | then that port keeps the MAC of the PF | |
| 12:29:53 | ralonsoh | right, and you can also retrieve the port created and stored in the DB | |
| 12:29:53 | gibi | so if another port is tried to be bound to the same PF | |
| 12:30:11 | gibi | that would cause MAC conflict in neutron | |
| 12:30:26 | sean-k-mooney | gibi: because the reset of the mac only resets the mac in the db | |
| 12:30:56 | sean-k-mooney | ya unless the bidnign profile is cleared it woudl be a problem | |
| 12:31:04 | gibi | I'm not sure how the reset works today, but if we follow ralonsoh's suggestion and persist the MAC in the DB then when the binding is removed we need to clear the DB too | |
| 12:31:29 | sean-k-mooney | isnt there already code for that | |
| 12:31:32 | sean-k-mooney | for the db part | |
| 12:31:41 | sean-k-mooney | that code wont clear the mac in the profile however | |
| 12:31:50 | gibi | the profile is cleard by nova | |
| 12:31:54 | gibi | that is OPK | |
| 12:31:55 | gibi | OK | |
| 12:32:19 | gibi | as I said I'm not sure about the reset logic | |
| 12:32:23 | sean-k-mooney | ok then the old code for reseting the mac shoudl fix the db version | |
| 12:33:35 | sean-k-mooney | https://github.com/openstack/neutron/commit/e603d19939c98b94081bb6a3af8bcb943f7bd2ce | |
| 12:34:07 | gibi | ahha OK | |
| 12:34:11 | gibi | then that is OK | |
| 12:34:54 | sean-k-mooney | ya the detach case is covered | |
| 12:40:00 | ralonsoh | sean-k-mooney, https://github.com/openstack/neutron/commit/e603d19939c98b94081bb6a3af8bcb943f7bd2ce | |
| 12:40:08 | ralonsoh | this is when the port MAC can be changed | |
| 12:40:43 | ralonsoh | of course, in this call you can bind the port and change the MAC | |
| 12:40:54 | ralonsoh | the only consideration is the other port with the same MAC | |
| 12:41:21 | ralonsoh | because you first bind the new port and then unbind the older one, right? | |
| 12:43:10 | sean-k-mooney | no | |
| 12:43:15 | sean-k-mooney | there is only 1 port | |
| 12:43:29 | sean-k-mooney | we either have multiple prot bidnings | |
| 12:43:36 | ralonsoh | right, right | |
| 12:43:40 | sean-k-mooney | or we do a update of the single port binding | |
| 12:43:47 | sean-k-mooney | https://review.opendev.org/c/openstack/neutron/+/829247/2/neutron/tests/unit/plugins/ml2/test_port_binding.py#775 does that make sense | |
| 12:43:50 | ralonsoh | my bad, there is only one DB port, two L1 ports | |
| 12:43:58 | sean-k-mooney | yes | |
| 12:45:01 | ralonsoh | no because of the implementation of "_update_port_dict_binding" | |
| 12:45:16 | sean-k-mooney | i was expecting that call to be wrong | |
| 12:45:22 | ralonsoh | in L672 we are calling the API | |
| 12:45:39 | ralonsoh | this API will retrieve the DB port and create this dictionary, this is what is returned | |
| 12:46:11 | ralonsoh | why? | |
| 12:46:34 | sean-k-mooney | right but what i was trying to capture was the logic of unset bidnding:profile['port_mac'] | |
| 12:47:03 | sean-k-mooney | to ensure that mac is not coming form the profile but the db | |
| 12:47:23 | ralonsoh | ahhhh sorry | |