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