On 2/14/25 12:42 PM, [email protected] wrote: > On Fri, 2025-02-14 at 12:38 +0100, Dumitru Ceara wrote: >> On 2/14/25 12:18 PM, [email protected] wrote: >> >> [...] >> >>>>>> >>>>>> I tried to address Ales' review comments here: >>>>>> https://github.com/dceara/ovn/commit/3655aa3 >>>>>> >>>>>> But I'm also going to reindent and do some minor style >>>>>> changes in >>>>>> the >>>>>> system tests. >>>>>> >>>>>> In the meantime, Ales, Martin, please let me know if the >>>>>> incremental >>>>>> changes look OK to you, I can squash them in when applying >>>>>> the >>>>>> series. >>>>> >>>>> Those changes look good to me. System test for NAT will need >>>>> slight >>>>> touchup as I mentione above, since the invalid NAT entries >>>>> won't be >>>>> advertised anymore after adding 2nd DGW port. >>>>> >>>> >>>> Hmm, I'm not sure what other things need adjustment, I did that >>>> already: >>>> >>>> https://github.com/dceara/ovn/commit/3655aa3#diff-538df8a7fc4aa894586caf2191217e5d8a50bf245ee88faf90516a1c229cf42aR16993-R16996 >>>> >>>> And the system tests pass with that change. Am I missing >>>> something? >>> >>> It's probably me that's missing the point, but if we skip invalid >>> nat >>> records, wouldn't these two stop being advertised? >>> >>> https://github.com/dceara/ovn/blob/3655aa3afce2a6b83e410830fae609788f0cd8dc/tests/system-ovn.at#L17066-L17067 >>> >>> Since those rules are now on distributed router with more than one >>> DGW >>> port and no --gateway-port? Those two were the ones that were >>> triggering the crash before we started filtering the !is_valid >>> records. >>> But you are right, tests do pass. >>> >>> Martin. >>> >> >> So, you pointed to these two routes: >> blackhole 10.42.10.10 proto 84 metric 1000 >> blackhole 10.42.10.11 proto 84 metric 1000 >> >> And my change did: >> >> # Create NAT on R2 >> check ovn-nbctl --gateway-port r2-join \ >> lr-nat-add R2 dnat_and_snat 10.42.10.10 192.168.1.10 >> check ovn-nbctl --gateway-port r2-join \ >> lr-nat-add R2 dnat_and_snat 10.42.10.11 192.168.1.11 >> >> https://github.com/dceara/ovn/blob/3655aa3afce2a6b83e410830fae609788f0cd8dc/tests/system-ovn.at#L16992C1-L16996C57 >> >> The two NATs now have gateway port configured. Without it the NAT >> entries are invalid indeed. >> > > Oh of course, you are correct. I think I'm really losing my focus after > this week, sorry. Though I wonder if it would be good to have those > invalid NAT rules in the test, to check for this cornercase. >
No worries, and thanks again for all the hard work on this feature! Good idea, I think I can easily add some invalid NAT rules to check for this corner case. Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
