Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-02-22
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 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: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: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 gibi so if another port is tried to be bound to the same PF
12:29:53 ralonsoh right, and you can also retrieve the port created and stored in the DB
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
12:47:32 ralonsoh yes, this is after deleting the port binding
12:47:52 sean-k-mooney after deleteing the source one yes
12:47:58 ralonsoh so we are reading the port randomly generated mac
12:48:03 ralonsoh generated_mac = port[port_def.PORT_MAC_ADDRESS]
12:48:03 ralonsoh # neutron generates a MAC for each port created
12:48:03 ralonsoh port = self._create_unbound_port()
12:48:36 sean-k-mooney with the current code ya but we expect it to keep the host1 pf mac
12:48:42 sean-k-mooney it wont now because of the bug
12:49:12 sean-k-mooney so i woudl like the test to be writen so that it caught the bug so it cant be regressed going forward
12:50:11 ralonsoh sean-k-mooney, when the port is first assigned, the mac is changed, right?
12:50:20 ralonsoh before the live migration
12:50:34 ralonsoh the Neutron DB port register MAC is changed
12:50:35 sean-k-mooney yes
12:50:43 ralonsoh ok
12:50:55 sean-k-mooney nova does a port update before we bind the port
12:51:06 sean-k-mooney the issue is we cant do a port update while it is bound
12:51:23 ralonsoh so the case here is to add this exception: to allow to change the MAC of the port even if it is bound
12:51:24 ralonsoh but
12:51:31 ralonsoh you also need to keep the original MAC
12:51:33 ralonsoh right?
12:51:34 gibi no
12:51:44 gibi we need to be able to define MAC per binding
12:51:49 gibi for mutliple binding
12:52:10 gibi at least that is how I model this
12:52:13 ralonsoh so this is a different feature I think
12:52:15 gibi one MAC per binding
12:52:16 sean-k-mooney we dont generally care about the orginal mac since we can always just read that form the PF if we need it
12:52:21 ralonsoh yes, like multiple port binding
12:52:55 ralonsoh ok, do you mind if I spend some time this afternoon or tomorrow thinking about this?

Earlier   Later