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. Martin. > Dumitru > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
