| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-02-22 | |||
| 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 | |
| 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 | port = self._create_unbound_port() | |
| 12:48:03 | ralonsoh | # neutron generates a MAC for each port created | |
| 12:48:03 | ralonsoh | generated_mac = port[port_def.PORT_MAC_ADDRESS] | |
| 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? | |
| 12:53:00 | ralonsoh | I need to finish some patches now | |
| 12:53:00 | gibi | ralonsoh: sure | |
| 12:53:03 | ralonsoh | thanks!! | |
| 12:53:08 | ralonsoh | I'll ping you | |
| 12:53:11 | gibi | thanks! | |
| 12:53:24 | gibi | I anyhow need to work on the nova side of this too | |
| 12:54:07 | gibi | sean-k-mooney: I like the idea to define the MAC when nova creates the binding instead of defining it when nova activates an inactive binding | |
| 12:54:20 | gibi | so I'd like to store the MAC in the binding profile | |
| 12:54:35 | gibi | it feels clearer | |
| 12:54:54 | gibi | as we define the rest of the infor in the profile at the same time like slot | |
| 12:55:14 | sean-k-mooney | ack | |
| 12:55:26 | sean-k-mooney | but in that case we need to permently keep the mac in the profile yes | |
| 12:55:47 | sean-k-mooney | so we shoudl be doing this for first spawn and sriov attach too | |
| 12:55:50 | sean-k-mooney | for consitency | |
| 12:55:58 | gibi | yes we do it | |