> Hi Lorenzo, > > I have a question about the intent of this patch. In both the cited BZ, as > well as the altered test, the procedure is to create a stateless NAT, and > then try to create an identical stateless NAT with --may-exist. > > However, what about the case when the existing NAT is not stateless, but we > try to add a stateless NAT with --may-exist? > > With the old code, this would result in an error message. With this change, > the error message is not output, and the new stateless NAT is not added. > > Is this intentional? If there is a mismatch between statelessness in the old > and new NAT, should there be an error message output even if --may-exist is > present?
Hi Mark, honestly I do not have a strong opinion on it. I think what really matters is to not add the entry, but I guess it is fine to not rise an error if we provide --may-exist option. Thoughts? > > I have one additional note down below. > > On 4/11/22 13:18, Lorenzo Bianconi wrote: > > Do not provide an error log if there is an already created stateless nat > > entry > > with the same external_ip and if the --may-exist option is provided in > > the ovn-nbctl command. > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2066551 > > Signed-off-by: Lorenzo Bianconi <[email protected]> > > --- > > tests/ovn-nbctl.at | 2 ++ > > utilities/ovn-nbctl.c | 9 ++++++--- > > 2 files changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > > index f9b9defd0..dd1626a96 100644 > > --- a/tests/ovn-nbctl.at > > +++ b/tests/ovn-nbctl.at > > @@ -597,6 +597,8 @@ AT_CHECK([ovn-nbctl --stateless lr-nat-add lr0 > > dnat_and_snat 40.0.0.3 192.168.1. > > [ovn-nbctl: 40.0.0.3, 192.168.1.7: External ip cannot be shared across > > stateless and stateful NATs > > ]) > > +AT_CHECK([ovn-nbctl --stateless --may-exist lr-nat-add lr0 dnat_and_snat > > 40.0.0.3 192.168.1.7]) > > + > > dnl Deletes the NATs > > AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat_and_snat 30.0.0.3]) > > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > > index 4ed4d27c6..81913f559 100644 > > --- a/utilities/ovn-nbctl.c > > +++ b/utilities/ovn-nbctl.c > > @@ -4825,9 +4825,12 @@ nbctl_lr_nat_add(struct ctl_context *ctx) > > struct smap nat_options = SMAP_INITIALIZER(&nat_options); > > I noticed that the above smap is declared but not used. If you're messing > around in this area, you can just remove this. ack, I will fix if I need to repost. Regards, Lorenzo > > > if (!strcmp(smap_get(&nat->options, "stateless"), > > "true") || stateless) { > > - ctl_error(ctx, "%s, %s: External ip cannot be shared " > > - "across stateless and stateful NATs", > > - new_external_ip, new_logical_ip); > > + if (!may_exist) { > > + ctl_error(ctx, "%s, %s: External ip cannot be > > shared " > > + "across stateless and stateful NATs", > > + new_external_ip, new_logical_ip); > > + } > > + should_return = true; > > } > > } > > } > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
