I pushed the change to main, branch-22.03, and branch-21.12.
On 4/18/22 13:48, Mark Michelson wrote:
On 4/15/22 05:39, Lorenzo Bianconi wrote:
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 don't have a strong opinion either. I just wanted to make sure that
this was something that had been thought about. I think people tend to
pass in --may-exist as a way of making sure that no errors get raised
due to an existing match, even if the existing match isn't *exactly* the
same (e.g. stateful vs. stateless NAT with the same external IP). It's
probably fine not to raise an error in this case.
With that in mind,
Acked-by: Mark Michelson <mmich...@redhat.com>
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 <lorenzo.bianc...@redhat.com>
---
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.
I've acked the patch now, so there is no need to repost. Whoever merges
can make this change, since it's just one line.
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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
