On 2/14/25 2:24 PM, [email protected] wrote: > On Fri, 2025-02-14 at 13:21 +0100, Dumitru Ceara wrote: >> 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. Is there anything else that needs tending to? Or do you think > that squashing in your proposed changes is all that's left? >
Nope, we're ready to go I think. Running CI now. Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
