Hey Mark, Thanks for reviewing the patch. Regarding `ovn-nbctl lr-nat-del`, I have updated the command to have the following structure: `lr-nat-del ROUTER [TYPE [IP [GATEWAY_PORT]]]`. Most of the earlier checks are not enforced unless `GATEWAY_PORT` is also passed.
With the new patch, a NAT rule is no longer uniquely identified by `TYPE` and `IP`. The user also needs to pass `GATEWAY_PORT` to uniquely identify a NAT rule. So, I have updated the behavior of `lr-nat-del` to not raise an error when `IP` is passed but no NAT rules match, similar to its behavior when `TYPE` is passed and no NAT rules match. Also, with the new patch, if `IP` is passed without passing `GATEWAY_PORT`, all NAT rules (possibly None) with the `TYPE` and `IP` values are deleted irrespective of their `GATEWAY_PORT` value. Should we retain the previous behavior of `lr-nat-del` when LR has a single DGP - raise an error if no NAT rules match when `IP` is passed? On 14 Mar 2022, at 20:55, Mark Michelson <mmich...@redhat.com<mailto:mmich...@redhat.com>> wrote: Hi Abhiram, I had a look through the code and I'm happy with how it looks. I also did a quick check through the testsuite and it all seems good. All that being said, I nearly acked this, but I have one question down below in the test code. It may indicate some issue in `ovn-nbctl lr-nat-del` so I'd like to get that cleared up first. -AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat_and_snat 30.0.0.3], [1], [], -[ovn-nbctl: no matching NAT with the type (dnat_and_snat) and external_ip (30.0.0.3) -]) -AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat 30.0.0.2], [1], [], -[ovn-nbctl: no matching NAT with the type (dnat) and external_ip (30.0.0.2) -]) -AT_CHECK([ovn-nbctl lr-nat-del lr0 snat 192.168.10.0/24], [1], [], -[ovn-nbctl: no matching NAT with the type (snat) and logical_ip (192.168.10.0/24) -]) -AT_CHECK([ovn-nbctl --if-exists lr-nat-del lr0 snat 192.168.10.0/24]) +AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat_and_snat 30.0.0.3]) I don't understand the above change. First, I don't understand why many of the lr-nat-del checks were removed. Second, I don't understand why the lr-nat-del of 30.0.0.3 doesn't return an error. There is no NAT with that address, so shouldn't that return an error? AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat_and_snat 30.0.0.1]) AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat_and_snat fd01::1]) _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
