[ovs-dev] dp_hash algorithm works incorretly when tcp retransmit
We meet a problem that same tcp session selects different ovs group bucket when in tcp retransmition, and we can easily reproduce this phenomenon. After some code research, we found that when tcp retransmit, it may call function sk_rethink_txhash(), and this function makes skb->hash changed, hence different ovs group bucket selected. anyone has good suggestions to fix this problem? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] vswitchd: update tc hwol docs regarding vswitchd restarts
Bleep bloop. Greetings Marcelo Ricardo Leitner, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 80 characters long (recommended limit is 79) #32 FILE: vswitchd/vswitch.xml:231: The default value is false. Disabling this value requires Lines checked: 50, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3] netdev-offload-tc: Reject install rules for tc flower unsupported ct_state flags
From: wenxu
TC flower doesn't support some ct state flags such as
INVALID/SNAT/DNAT/REPLY. So it is better to reject this rule.
Signed-off-by: wenxu
---
lib/netdev-offload-tc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 586d99d..7cdd849 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1646,6 +1646,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
*match,
flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_NEW;
}
flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_NEW;
+mask->ct_state &= ~OVS_CS_F_NEW;
}
if (mask->ct_state & OVS_CS_F_ESTABLISHED) {
@@ -1653,6 +1654,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
*match,
flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED;
}
flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED;
+mask->ct_state &= ~OVS_CS_F_ESTABLISHED;
}
if (mask->ct_state & OVS_CS_F_TRACKED) {
@@ -1660,14 +1662,13 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
*match,
flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
}
flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
+mask->ct_state &= ~OVS_CS_F_TRACKED;
}
if (flower.key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) {
flower.key.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
flower.mask.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
}
-
-mask->ct_state = 0;
}
if (mask->ct_zone) {
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3] vswitchd: update tc hwol docs regarding vswitchd restarts
hw-offload is can actually be enabled without a restart. Then, once enabled, a restart is needed in order to make the new value effective. tc-policy, just like hw-offload, is protected by ovsthread_once_start() in netdev_set_flow_api_enabled() and is only parsed when hw-offload is enabled. So lets document that if hw-offload was already enabled changing it requires a restart in order for it to have effect. v2: include changes regarding the new understanding on hw-offload v3: fixed warnings Signed-off-by: Marcelo Ricardo Leitner --- vswitchd/vswitch.xml | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 5b2a174ab813139d3f3253be47758d4bfb1beac3..83131b8f81502c34230e7e0b69fbb15a19721486 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -228,8 +228,8 @@ Set this value to true to enable netdev flow offload. - The default value is false. Changing this value requires - restarting the daemon + The default value is false. Disabling this value requires + restarting the daemon. Currently Open vSwitch supports hardware offloading on @@ -267,7 +267,9 @@ is enabled. - The default value is none. + The default value is none. Changing this value requires + restarting the daemon if + was already enabled. -- 2.29.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] rhel: Add option to enable AF_XDP on rpm package
On Tue, Feb 2, 2021 at 9:32 AM Ilya Maximets wrote: > > On 1/21/21 7:51 PM, Yi-Hung Wei wrote: > > This patch adds an RPMBUILD_OPT so that user can enable > > AF_XDP support in the rpm package by: > > > > $ make rpm-fedora RPMBUILD_OPT="--with afxdp" > > Thanks! > This change looks good to me in general. > > Could you, please, also update Documentation/intro/install/fedora.rst > with this new option? > > Best regards, Ilya Maximets. Thanks for Yifeng and Ilya for review. Please find the v2 with the documentation update here: https://patchwork.ozlabs.org/project/openvswitch/patch/1612398452-114205-1-git-send-email-yihung@gmail.com/ Thanks, -Yi-Hung ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] rhel: Add option to enable AF_XDP on rpm package
This patch adds an RPMBUILD_OPT so that user can enable
AF_XDP support in the rpm package by:
$ make rpm-fedora RPMBUILD_OPT="--with afxdp"
Signed-off-by: Yi-Hung Wei
---
Documentation/intro/install/fedora.rst | 7 +++
rhel/openvswitch-fedora.spec.in| 8
2 files changed, 15 insertions(+)
diff --git a/Documentation/intro/install/fedora.rst
b/Documentation/intro/install/fedora.rst
index 4a2f3507cbbb..06a0bd3d59e1 100644
--- a/Documentation/intro/install/fedora.rst
+++ b/Documentation/intro/install/fedora.rst
@@ -117,6 +117,13 @@ can be added:
$ make rpm-fedora RPMBUILD_OPT="--with dpdk --without check"
+To enable AF_XDP support in the openvswitch package, the ``--with afxdp``
+option can be added:
+
+::
+
+$ make rpm-fedora RPMBUILD_OPT="--with afxdp --without check"
+
You can also have the above commands automatically run the Open vSwitch unit
tests. This can take several minutes.
diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
index 2c0c4fa186a3..e03b26b6af34 100644
--- a/rhel/openvswitch-fedora.spec.in
+++ b/rhel/openvswitch-fedora.spec.in
@@ -28,6 +28,8 @@
%bcond_without libcapng
# To enable DPDK support, specify '--with dpdk' when building
%bcond_with dpdk
+# To enable AF_XDP support, specify '--with afxdp' when building
+%bcond_with afxdp
# If there is a need to automatically enable the package after installation,
# specify the "--with autoenable"
@@ -73,6 +75,9 @@ BuildRequires: libpcap-devel numactl-devel
BuildRequires: dpdk-devel >= 17.05.1
Provides: %{name}-dpdk = %{version}-%{release}
%endif
+%if %{with afxdp}
+BuildRequires: libbpf-devel numactl-devel
+%endif
BuildRequires: unbound unbound-devel
Requires: openssl hostname iproute module-init-tools unbound
@@ -164,6 +169,9 @@ This package provides IPsec tunneling support for OVS
tunnels.
%if %{with dpdk}
--with-dpdk=$(dirname %{_datadir}/dpdk/*/.config) \
%endif
+%if %{with afxdp}
+--enable-afxdp \
+%endif
--enable-ssl \
--disable-static \
--enable-shared \
--
2.7.4
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] netdev-afxdp: Add start qid support.
Mellanox card has different XSK design. It requires users to create
dedicated queues for XSK. Unlike Intel's NIC which loads XDP program
to all queues, Mellanox only loads XDP program to a subset of its queue.
When OVS uses AF_XDP with mlx5, it doesn't replace the existing RX and TX
queues in the channel with XSK RX and XSK TX queues, but it creates an
additional pair of queues for XSK in that channel. To distinguish
regular and XSK queues, mlx5 uses a different range of qids.
That means, if the card has 24 queues, queues 0..11 correspond to
regular queues, and queues 12..23 are XSK queues.
In this case, we should attach the netdev-afxdp with 'start-qid=12'.
I tested using Mellanox Connect-X 6Dx, by setting 'start-qid=1', and:
$ ethtool -L enp2s0f0np0 combined 1
# queue 0 is for non-XDP traffic, queue 1 is for XSK
$ ethtool -N enp2s0f0np0 flow-type udp4 action 1
note: we need additionally add flow-redirect rule to queue 1
Tested-at: https://github.com/williamtu/ovs-travis/actions/runs/535141041
Signed-off-by: William Tu
---
Documentation/intro/install/afxdp.rst | 2 ++
lib/netdev-afxdp.c| 23 ++-
lib/netdev-linux-private.h| 1 +
vswitchd/vswitch.xml | 8
4 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/Documentation/intro/install/afxdp.rst
b/Documentation/intro/install/afxdp.rst
index f2643e0d41a1..eac298c52575 100644
--- a/Documentation/intro/install/afxdp.rst
+++ b/Documentation/intro/install/afxdp.rst
@@ -204,6 +204,8 @@ more details):
* ``use-need-wakeup``: default ``true`` if libbpf supports it,
otherwise ``false``.
+* ``start-qid``: default ``0``.
+
For example, to use 1 PMD (on core 4) on 1 queue (queue 0) device,
configure these options: ``pmd-cpu-mask``, ``pmd-rxq-affinity``, and
``n_rxq``::
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index 482400d8d135..36f6a323b1bc 100644
--- a/lib/netdev-afxdp.c
+++ b/lib/netdev-afxdp.c
@@ -458,12 +458,13 @@ xsk_configure_queue(struct netdev_linux *dev, int
ifindex, int queue_id,
VLOG_DBG("%s: configuring queue: %d, mode: %s, use-need-wakeup: %s.",
netdev_get_name(>up), queue_id, xdp_modes[mode].name,
dev->use_need_wakeup ? "true" : "false");
-xsk_info = xsk_configure(ifindex, queue_id, mode, dev->use_need_wakeup,
- report_socket_failures);
+xsk_info = xsk_configure(ifindex, dev->startqid + queue_id, mode,
+ dev->use_need_wakeup, report_socket_failures);
if (!xsk_info) {
VLOG(report_socket_failures ? VLL_ERR : VLL_DBG,
- "%s: Failed to create AF_XDP socket on queue %d in %s mode.",
- netdev_get_name(>up), queue_id, xdp_modes[mode].name);
+ "%s: Failed to create AF_XDP socket on queue %d+%d in %s mode.",
+ netdev_get_name(>up), dev->startqid, queue_id,
+ xdp_modes[mode].name);
dev->xsks[queue_id] = NULL;
return -1;
}
@@ -604,6 +605,7 @@ netdev_afxdp_set_config(struct netdev *netdev, const struct
smap *args,
enum afxdp_mode xdp_mode;
bool need_wakeup;
int new_n_rxq;
+int new_startqid;
ovs_mutex_lock(>mutex);
new_n_rxq = MAX(smap_get_int(args, "n_rxq", NR_QUEUE), 1);
@@ -637,12 +639,18 @@ netdev_afxdp_set_config(struct netdev *netdev, const
struct smap *args,
}
#endif
+/* TODO: need to check
+ * new_startqid + new_n_rxq > total dev's queues. */
+new_startqid = smap_get_int(args, "start-qid", 0);
+
if (dev->requested_n_rxq != new_n_rxq
|| dev->requested_xdp_mode != xdp_mode
-|| dev->requested_need_wakeup != need_wakeup) {
+|| dev->requested_need_wakeup != need_wakeup
+|| dev->requested_startqid != new_startqid) {
dev->requested_n_rxq = new_n_rxq;
dev->requested_xdp_mode = xdp_mode;
dev->requested_need_wakeup = need_wakeup;
+dev->requested_startqid = new_startqid;
netdev_request_reconfigure(netdev);
}
ovs_mutex_unlock(>mutex);
@@ -661,6 +669,7 @@ netdev_afxdp_get_config(const struct netdev *netdev, struct
smap *args)
xdp_modes[dev->xdp_mode_in_use].name);
smap_add_format(args, "use-need-wakeup", "%s",
dev->use_need_wakeup ? "true" : "false");
+smap_add_format(args, "start-qid", "%d", dev->startqid);
ovs_mutex_unlock(>mutex);
return 0;
}
@@ -696,6 +705,7 @@ netdev_afxdp_reconfigure(struct netdev *netdev)
if (netdev->n_rxq == dev->requested_n_rxq
&& dev->xdp_mode == dev->requested_xdp_mode
&& dev->use_need_wakeup == dev->requested_need_wakeup
+&& dev->startqid == dev->requested_startqid
&& dev->xsks) {
goto out;
}
@@ -713,6 +723,7 @@ netdev_afxdp_reconfigure(struct netdev *netdev)
VLOG_ERR("setrlimit(RLIMIT_MEMLOCK) failed: %s", ovs_strerror(errno));
}
dev->use_need_wakeup =
Re: [ovs-dev] [PATCH v2] vswitchd: update tc hwol docs regarding vswitchd restarts
Bleep bloop. Greetings Marcelo Ricardo Leitner, I am a robot and I have tried
out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 80 characters long (recommended limit is 79)
#31 FILE: vswitchd/vswitch.xml:231:
The default value is false. Disabling this value requires
WARNING: Line has non-spaces leading whitespace
#42 FILE: vswitchd/vswitch.xml:271:
restarting the daemon if
WARNING: Line has non-spaces leading whitespace
#43 FILE: vswitchd/vswitch.xml:272:
was already enabled.
Lines checked: 49, Warnings: 3, Errors: 0
build:
mv tests/ovsdb-cluster-testsuite.tmp tests/ovsdb-cluster-testsuite
\
{ sed -n -e '/%AUTHORS%/q' -e p < ./debian/copyright.in; \
sed '34,/^$/d' ./AUTHORS.rst | \
sed -n -e '/^$/q' -e 's/^/ /p'; \
sed -e '34,/%AUTHORS%/d' ./debian/copyright.in; \
} > debian/copyright
(printf '\043 Generated automatically -- do not modify!-*-
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.15.90,g') <
./rhel/openvswitch-dkms.spec.in > openvswitch-dkms.spec.tmp || exit 1; if cmp
-s openvswitch-dkms.spec.tmp rhel/openvswitch-dkms.spec; then touch
rhel/openvswitch-dkms.spec; rm openvswitch-dkms.spec.tmp; else mv
openvswitch-dkms.spec.tmp rhel/openvswitch-dkms.spec; fi
(printf '\043 Generated automatically -- do not modify!-*-
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.15.90,g') <
./rhel/kmod-openvswitch-rhel6.spec.in > kmod-openvswitch-rhel6.spec.tmp || exit
1; if cmp -s kmod-openvswitch-rhel6.spec.tmp rhel/kmod-openvswitch-rhel6.spec;
then touch rhel/kmod-openvswitch-rhel6.spec; rm
kmod-openvswitch-rhel6.spec.tmp; else mv kmod-openvswitch-rhel6.spec.tmp
rhel/kmod-openvswitch-rhel6.spec; fi
(printf '\043 Generated automatically -- do not modify!-*-
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.15.90,g') <
./rhel/openvswitch-kmod-fedora.spec.in > openvswitch-kmod-fedora.spec.tmp ||
exit 1; if cmp -s openvswitch-kmod-fedora.spec.tmp
rhel/openvswitch-kmod-fedora.spec; then touch
rhel/openvswitch-kmod-fedora.spec; rm openvswitch-kmod-fedora.spec.tmp; else mv
openvswitch-kmod-fedora.spec.tmp rhel/openvswitch-kmod-fedora.spec; fi
(printf '\043 Generated automatically -- do not modify!-*-
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.15.90,g') <
./rhel/openvswitch.spec.in > openvswitch.spec.tmp || exit 1; if cmp -s
openvswitch.spec.tmp rhel/openvswitch.spec; then touch rhel/openvswitch.spec;
rm openvswitch.spec.tmp; else mv openvswitch.spec.tmp rhel/openvswitch.spec; fi
(printf '\043 Generated automatically -- do not modify!-*-
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.15.90,g') <
./rhel/openvswitch-fedora.spec.in > openvswitch-fedora.spec.tmp || exit 1; if
cmp -s openvswitch-fedora.spec.tmp rhel/openvswitch-fedora.spec; then touch
rhel/openvswitch-fedora.spec; rm openvswitch-fedora.spec.tmp; else mv
openvswitch-fedora.spec.tmp rhel/openvswitch-fedora.spec; fi
(printf '\043 Generated automatically -- do not modify!-*-
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.15.90,g') \
< ./xenserver/openvswitch-xen.spec.in > openvswitch-xen.spec.tmp ||
exit 1; \
if cmp -s openvswitch-xen.spec.tmp xenserver/openvswitch-xen.spec; then touch
xenserver/openvswitch-xen.spec; rm openvswitch-xen.spec.tmp; else mv
openvswitch-xen.spec.tmp xenserver/openvswitch-xen.spec; fi
make[3]: Entering directory
`/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace/datapath'
make[3]: Leaving directory
`/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace/datapath'
vswitchd/vswitch.xml
See above for files that use tabs for indentation.
Please use spaces instead.
make[2]: *** [check-tabs] Error 1
make[2]: Leaving directory
`/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory
`/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
make: *** [all] Error 2
Please check this out. If you feel there has been an error, please email
acon...@redhat.com
Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dist-docs: Include manpages generated from rST.
On Fri, Jan 29, 2021 at 03:03:05PM +0100, Ilya Maximets wrote:
> Some manpages are generated from rST, but these are not included
> in 'dist-docs' make target.
>
> Fixes: fd0837a76f4c ("doc: Convert ovs-vlan-test to rST")
> Signed-off-by: Ilya Maximets
Works for me.
Acked-by: Ben Pfaff
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v3 2/3] binding: Set Port_Binding.up only if supported.
The supported upgrade procedure is to always upgrade ovn-controllers
first and OVN DBs and ovn-northd later. This leads however to the
situation when ovn-controller might try to set the Port_Binding.up field
while the Southbound DB isn't yet aware of this field which would
trigger transaction failures and control plane/data plane outages.
To avoid such situations ovn-controller only sets the Port_Binding.up
field if it was explicitly set to 'false'. This ensures that the SB
database was already upgraded.
On the ovn-northd side, as soon as ovn-northd is upgraded it will update
all existing Port_Bindings and explicitly set 'Port_Binding.up' to
false, implicitly notifying ovn-controller that it is safe to write to
the field.
Reported-by: Numan Siddique
Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows
are installed.")
Signed-off-by: Dumitru Ceara
---
controller-vtep/binding.c |6 --
controller/binding.c | 33 ++-
northd/ovn-northd.c |8
tests/ovn.at | 48 -
4 files changed, 82 insertions(+), 13 deletions(-)
diff --git a/controller-vtep/binding.c b/controller-vtep/binding.c
index d28a598..01d5a16 100644
--- a/controller-vtep/binding.c
+++ b/controller-vtep/binding.c
@@ -110,9 +110,11 @@ update_pb_chassis(const struct sbrec_port_binding
*port_binding_rec,
chassis_rec->name);
}
-bool up = true;
sbrec_port_binding_set_chassis(port_binding_rec, chassis_rec);
-sbrec_port_binding_set_up(port_binding_rec, , 1);
+if (port_binding_rec->n_up) {
+bool up = true;
+sbrec_port_binding_set_up(port_binding_rec, , 1);
+}
}
}
diff --git a/controller/binding.c b/controller/binding.c
index 353debe..efaa109 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -870,6 +870,11 @@ get_lport_type(const struct sbrec_port_binding *pb)
* container and virtual ports).
* Otherwise request a notification to be sent when the OVS flows
* corresponding to 'pb' have been installed.
+ *
+ * Note:
+ * Updates (directly or through a notification) the 'pb->up' field only if
+ * it's explicitly set to 'false'.
+ * This is to ensure compatibility with older versions of ovn-northd.
*/
static void
claimed_lport_set_up(const struct sbrec_port_binding *pb,
@@ -885,7 +890,7 @@ claimed_lport_set_up(const struct sbrec_port_binding *pb,
return;
}
-if (pb->chassis != chassis_rec) {
+if (pb->chassis != chassis_rec || (pb->n_up && !pb->up[0])) {
binding_iface_bound_add(pb->logical_port);
}
}
@@ -973,7 +978,10 @@ release_lport(const struct sbrec_port_binding *pb, bool
sb_readonly,
sbrec_port_binding_set_virtual_parent(pb, NULL);
}
-sbrec_port_binding_set_up(pb, NULL, 0);
+if (pb->n_up) {
+bool up = false;
+sbrec_port_binding_set_up(pb, , 1);
+}
update_lport_tracking(pb, tracked_datapaths);
binding_iface_released_add(pb->logical_port);
VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);
@@ -2503,7 +2511,10 @@ binding_seqno_run(struct shash *local_bindings)
ovsrec_interface_update_external_ids_delkey(
lb->iface, OVN_INSTALLED_EXT_ID);
}
-sbrec_port_binding_set_up(lb->pb, NULL, 0);
+if (lb->pb->n_up) {
+bool up = false;
+sbrec_port_binding_set_up(lb->pb, , 1);
+}
simap_put(_iface_seqno_map, lb->name, new_seqno);
}
sset_delete(_iface_bound_set, SSET_NODE_FROM_NAME(iface_id));
@@ -2536,7 +2547,6 @@ binding_seqno_install(struct shash *local_bindings)
SIMAP_FOR_EACH_SAFE (node, node_next, _iface_seqno_map) {
struct shash_node *lb_node = shash_find(local_bindings, node->name);
-bool up = true;
if (!lb_node) {
goto del_seqno;
@@ -2554,12 +2564,15 @@ binding_seqno_install(struct shash *local_bindings)
ovsrec_interface_update_external_ids_setkey(lb->iface,
OVN_INSTALLED_EXT_ID,
"true");
-sbrec_port_binding_set_up(lb->pb, , 1);
-
-struct shash_node *child_node;
-SHASH_FOR_EACH (child_node, >children) {
-struct local_binding *lb_child = child_node->data;
-sbrec_port_binding_set_up(lb_child->pb, , 1);
+if (lb->pb->n_up) {
+bool up = true;
+
+sbrec_port_binding_set_up(lb->pb, , 1);
+struct shash_node *child_node;
+SHASH_FOR_EACH (child_node, >children) {
+struct local_binding *lb_child = child_node->data;
+sbrec_port_binding_set_up(lb_child->pb, , 1);
+}
}
del_seqno:
diff --git a/northd/ovn-northd.c
[ovs-dev] [PATCH ovn v3 3/3] northd: Allow backwards compatibility for Logical_Switch_Port.up.
In general, ovn-northd expects ovn-controller to set Port_Binding.up
before it declares the logical switch port as being up.
Even though the recommended upgrade procedure for OVN states that
ovn-controllers should be upgraded before ovn-northd, there are cases
when CMSs don't follow this guideline.
This would cause all existing and bound Logical_Switch_Ports to be
declared "down" until ovn-controllers are upgraded.
To avoid this situation, ovn-controllers now explicitly set
Chassis.other_config:port-up-notif in their own chassis record. Based
on this value, ovn-northd can determine if it needs to use the old type
of logic or the new one (Port_Binding.up) when setting LSP.up.
Note:
In case of downgrading ovn-controller before ovn-northd, if
ovn-controller is forcefully stopped it will not clear its chassis
record from the SB. Older versions will not have the capability to
clear the other_config:port-up-notif value so LSPs will be declared
"down" until ovn-northd is downgraded as well. As this
upgrade/downgrade procedure is not the recommended one, we don't try
to deal with this scenario.
Suggested-by: Numan Siddique
Signed-off-by: Dumitru Ceara
---
controller/chassis.c|7 +++
include/ovn/automake.mk |1 +
include/ovn/features.h | 22 ++
northd/ovn-northd.c | 13 -
ovn-sb.xml |5 +
tests/ovn-controller.at | 17 +
tests/ovn-northd.at | 22 ++
7 files changed, 86 insertions(+), 1 deletion(-)
create mode 100644 include/ovn/features.h
diff --git a/controller/chassis.c b/controller/chassis.c
index b4d4b0e..0937e33 100644
--- a/controller/chassis.c
+++ b/controller/chassis.c
@@ -28,6 +28,7 @@
#include "lib/ovn-sb-idl.h"
#include "ovn-controller.h"
#include "lib/util.h"
+#include "ovn/features.h"
VLOG_DEFINE_THIS_MODULE(chassis);
@@ -293,6 +294,7 @@ chassis_build_other_config(struct smap *config, const char
*bridge_mappings,
smap_replace(config, "iface-types", iface_types);
smap_replace(config, "ovn-chassis-mac-mappings", chassis_macs);
smap_replace(config, "is-interconn", is_interconn ? "true" : "false");
+smap_replace(config, OVN_FEATURE_PORT_UP_NOTIF, "true");
}
/*
@@ -363,6 +365,11 @@ chassis_other_config_changed(const char *bridge_mappings,
return true;
}
+if (!smap_get_bool(_rec->other_config, OVN_FEATURE_PORT_UP_NOTIF,
+ false)) {
+return true;
+}
+
return false;
}
diff --git a/include/ovn/automake.mk b/include/ovn/automake.mk
index 54b0e2c..582241a 100644
--- a/include/ovn/automake.mk
+++ b/include/ovn/automake.mk
@@ -2,5 +2,6 @@ ovnincludedir = $(includedir)/ovn
ovninclude_HEADERS = \
include/ovn/actions.h \
include/ovn/expr.h \
+ include/ovn/features.h \
include/ovn/lex.h \
include/ovn/logical-fields.h
diff --git a/include/ovn/features.h b/include/ovn/features.h
new file mode 100644
index 000..10ee46f
--- /dev/null
+++ b/include/ovn/features.h
@@ -0,0 +1,22 @@
+/* Copyright (c) 2021, Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef OVN_FEATURES_H
+#define OVN_FEATURES_H 1
+
+/* ovn-controller supported feature names. */
+#define OVN_FEATURE_PORT_UP_NOTIF "port-up-notif"
+
+#endif
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 00c8cb3..12a24a6 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -40,6 +40,7 @@
#include "lib/lb.h"
#include "memory.h"
#include "ovn/actions.h"
+#include "ovn/features.h"
#include "ovn/logical-fields.h"
#include "packets.h"
#include "openvswitch/poll-loop.h"
@@ -12838,7 +12839,17 @@ handle_port_binding_changes(struct northd_context
*ctx, struct hmap *ports,
continue;
}
-bool up = ((sb->up && (*sb->up)) || lsp_is_router(op->nbsp));
+bool up = false;
+
+if (lsp_is_router(op->nbsp)) {
+up = true;
+} else if (sb->chassis) {
+up = smap_get_bool(>chassis->other_config,
+ OVN_FEATURE_PORT_UP_NOTIF, false)
+ ? sb->n_up && sb->up[0]
+ : true;
+}
+
if (!op->nbsp->up || *op->nbsp->up != up) {
nbrec_logical_switch_port_set_up(op->nbsp, , 1);
}
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 53fdc15..2f251bd 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -322,6 +322,11 @@
[ovs-dev] [PATCH ovn v3 1/3] binding: Correctly set Port_Binding.up for container/virtual ports.
For port bindings that are children of other port bindings (container
and virtual ports) set Port_Binding.up directly, when claimed, if their
parent bindings are already up.
For non-VIF port bindings maintain compatibility with older versions
and set Port_Binding.up as soon as they are claimed.
Reported-by: Ben Pfaff
Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows
are installed.")
Signed-off-by: Dumitru Ceara
---
controller-vtep/binding.c |3 +++
controller/binding.c | 47 +++--
tests/ovn.at | 24 ++-
3 files changed, 67 insertions(+), 7 deletions(-)
diff --git a/controller-vtep/binding.c b/controller-vtep/binding.c
index 8337715..d28a598 100644
--- a/controller-vtep/binding.c
+++ b/controller-vtep/binding.c
@@ -109,7 +109,10 @@ update_pb_chassis(const struct sbrec_port_binding
*port_binding_rec,
port_binding_rec->chassis->name,
chassis_rec->name);
}
+
+bool up = true;
sbrec_port_binding_set_chassis(port_binding_rec, chassis_rec);
+sbrec_port_binding_set_up(port_binding_rec, , 1);
}
}
diff --git a/controller/binding.c b/controller/binding.c
index d44f635..353debe 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -864,21 +864,52 @@ get_lport_type(const struct sbrec_port_binding *pb)
return LP_UNKNOWN;
}
+/* For newly claimed ports, if 'notify_up' is 'false':
+ * - set the 'pb.up' field to true if 'pb' has no 'parent_pb'.
+ * - set the 'pb.up' field to true if 'parent_pb.up' is 'true' (e.g., for
+ * container and virtual ports).
+ * Otherwise request a notification to be sent when the OVS flows
+ * corresponding to 'pb' have been installed.
+ */
+static void
+claimed_lport_set_up(const struct sbrec_port_binding *pb,
+ const struct sbrec_port_binding *parent_pb,
+ const struct sbrec_chassis *chassis_rec,
+ bool notify_up)
+{
+if (!notify_up) {
+bool up = true;
+if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) {
+sbrec_port_binding_set_up(pb, , 1);
+}
+return;
+}
+
+if (pb->chassis != chassis_rec) {
+binding_iface_bound_add(pb->logical_port);
+}
+}
+
/* Returns false if lport is not claimed due to 'sb_readonly'.
* Returns true otherwise.
*/
static bool
claim_lport(const struct sbrec_port_binding *pb,
+const struct sbrec_port_binding *parent_pb,
const struct sbrec_chassis *chassis_rec,
const struct ovsrec_interface *iface_rec,
-bool sb_readonly, struct hmap *tracked_datapaths)
+bool sb_readonly, bool notify_up,
+struct hmap *tracked_datapaths)
{
+if (!sb_readonly) {
+claimed_lport_set_up(pb, parent_pb, chassis_rec, notify_up);
+}
+
if (pb->chassis != chassis_rec) {
if (sb_readonly) {
return false;
}
-binding_iface_bound_add(pb->logical_port);
if (pb->chassis) {
VLOG_INFO("Changing chassis for lport %s from %s to %s.",
pb->logical_port, pb->chassis->name,
@@ -1041,8 +1072,12 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
if (lbinding_set) {
if (can_bind) {
/* We can claim the lport. */
-if (!claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface,
- !b_ctx_in->ovnsb_idl_txn,
+const struct sbrec_port_binding *parent_pb =
+lbinding->parent ? lbinding->parent->pb : NULL;
+
+if (!claim_lport(pb, parent_pb, b_ctx_in->chassis_rec,
+ lbinding->iface, !b_ctx_in->ovnsb_idl_txn,
+ !lbinding->parent,
b_ctx_out->tracked_dp_bindings)){
return false;
}
@@ -1246,8 +1281,8 @@ consider_nonvif_lport_(const struct sbrec_port_binding
*pb,
b_ctx_out->tracked_dp_bindings);
update_local_lport_ids(pb, b_ctx_out);
-return claim_lport(pb, b_ctx_in->chassis_rec, NULL,
- !b_ctx_in->ovnsb_idl_txn,
+return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL,
+ !b_ctx_in->ovnsb_idl_txn, false,
b_ctx_out->tracked_dp_bindings);
} else if (pb->chassis == b_ctx_in->chassis_rec) {
return release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
diff --git a/tests/ovn.at b/tests/ovn.at
index e814c18..db86183 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -8923,9 +8923,9 @@ bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int
external_ids:ct-zone-bar2)
AT_CHECK([test -z $bar2_zoneid])
# Add back bar2
-wait_for_ports_up
ovn-nbctl lsp-add bar bar2 vm2 1 \
-- lsp-set-addresses bar2 "f0:00:00:01:02:08 192.168.2.3"
+wait_for_ports_up
[ovs-dev] [PATCH v3 ovn 0/3] ovn: Fix port-up notification for child ports and handle upgrades.
First two patches of the series fix issues related to: - setting Port_Binding.up for container/virtual ports/ - correctly dealing with upgrades when ovn-controller is upgraded first (recommended way). The third patch of the series tries to handle out of order upgrades (ovn-northd before ovn-controller) in order to not avoid declaring logical switch ports down during the upgrade. Changes in V3: - rebased. Changes in V2: - turned this into a series that: - addresses Ben's bug report - adds support for out-of-order upgrades for this specific feature as suggested by Numan. Dumitru Ceara (3): binding: Correctly set Port_Binding.up for container/virtual ports. binding: Set Port_Binding.up only if supported. northd: Allow backwards compatibility for Logical_Switch_Port.up. controller-vtep/binding.c |5 +++ controller/binding.c | 78 - controller/chassis.c |7 include/ovn/automake.mk |1 + include/ovn/features.h| 22 + northd/ovn-northd.c | 21 ovn-sb.xml|5 +++ tests/ovn-controller.at | 17 ++ tests/ovn-northd.at | 22 + tests/ovn.at | 72 -- 10 files changed, 232 insertions(+), 18 deletions(-) create mode 100644 include/ovn/features.h ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] vswitchd: update tc hwol docs regarding vswitchd restarts
hw-offload is can actually be enabled without a restart. Then, once enabled, a restart is needed in order to make the new value effective. tc-policy, just like hw-offload, is protected by ovsthread_once_start() in netdev_set_flow_api_enabled() and is only parsed when hw-offload is enabled. So lets document that if hw-offload was already enabled changing it requires a restart in order for it to have effect. v2: include changes regarding the new understanding on hw-offload Signed-off-by: Marcelo Ricardo Leitner --- vswitchd/vswitch.xml | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 5b2a174ab813139d3f3253be47758d4bfb1beac3..de7aba00ae91357a561e60c1e367e702713ac108 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -228,8 +228,8 @@ Set this value to true to enable netdev flow offload. - The default value is false. Changing this value requires - restarting the daemon + The default value is false. Disabling this value requires + restarting the daemon. Currently Open vSwitch supports hardware offloading on @@ -267,7 +267,9 @@ is enabled. - The default value is none. + The default value is none. Changing this value requires + restarting the daemon if + was already enabled. -- 2.29.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 1/3] binding: Correctly set Port_Binding.up for container/virtual ports.
Bleep bloop. Greetings Dumitru Ceara, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: Failed to merge in the changes. hint: Use 'git am --show-current-patch' to see the failed patch Patch failed at 0001 binding: Correctly set Port_Binding.up for container/virtual ports. When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [RFC PATCH] userspace: Enable tunnel with TSO.
Currently when setting 'userspace-tso-enable=true', tunnel test cases
fail due to incorrect checksum, at inner header and outer header.
The patch recalculates the checksum before packet is outputting to
a port (tunnel and tap), makes sure the receiver sees correct checksum.
Consider the following cases:
1) veth -> ovs -> veth, and 2) tap -> ovs -> tap
No need to recalc csum because vnet hdr carries the offload
information.
3) decap: vxlan tunnel -> br-underlay -> br-overlay
The inner packet is sent to br-overlay (which is a tap).
Need to fix the inner header's csum.
4) encap: br-overlay -> br-underlay -> vxlan tunnel
Fix the inner csum before pushing the outer header.
I added iperf and pass vxlan and geneve tests:
$ make check-system-tso TESTSUITEFLAGS="-k vxlan"
$ make check-system-tso TESTSUITEFLAGS="-k geneve"
While TCP works over tunnel, the TCP sender sending huge
packet size will fail. I have to segment the inner TCP
packet before pushing the outer tunnel header.
Signed-off-by: William Tu
---
lib/netdev-linux.c | 2 +-
lib/netdev-native-tnl.c | 11 ++-
lib/netdev.c| 18 ++
lib/packets.c | 34 ++
lib/packets.h | 1 +
tests/system-tap.at | 3 +++
tests/system-traffic.at | 9 +
7 files changed, 64 insertions(+), 14 deletions(-)
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 6be23dbeed57..bb365b3b0da3 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -1446,7 +1446,6 @@ netdev_linux_batch_rxq_recv_tap(struct netdev_rxq_linux
*rx, int mtu,
netdev_get_name(netdev_));
continue;
}
-
dp_packet_batch_add(batch, pkt);
}
@@ -1604,6 +1603,7 @@ netdev_linux_tap_batch_send(struct netdev *netdev_, bool
tso, int mtu,
int error;
if (tso) {
+packet_csum_tcpudp(packet);
netdev_linux_prepend_vnet_hdr(packet, mtu);
}
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index b89dfdd52a86..003c78a151f8 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -43,6 +43,7 @@
#include "seq.h"
#include "unaligned.h"
#include "unixctl.h"
+#include "userspace-tso.h"
#include "openvswitch/vlog.h"
VLOG_DEFINE_THIS_MODULE(native_tnl);
@@ -153,6 +154,12 @@ netdev_tnl_push_ip_header(struct dp_packet *packet,
struct ip_header *ip;
struct ovs_16aligned_ip6_hdr *ip6;
+if (userspace_tso_enabled()) {
+/* Calculate inner header's checksum before pushing outer header.
+ * (Assume the device does not support tnl checksum) */
+packet_csum_tcpudp(packet);
+}
+
eth = dp_packet_push_uninit(packet, size);
*ip_tot_size = dp_packet_size(packet) - sizeof (struct eth_header);
@@ -189,7 +196,9 @@ udp_extract_tnl_md(struct dp_packet *packet, struct
flow_tnl *tnl,
return NULL;
}
-if (udp->udp_csum) {
+/* 'udp->udp_csum' will be the pseudo header csum when when userspace
+ * TSO is enabled. Skip the validation. */
+if (udp->udp_csum && !userspace_tso_enabled()) {
if (OVS_UNLIKELY(!dp_packet_l4_checksum_valid(packet))) {
uint32_t csum;
if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) {
diff --git a/lib/netdev.c b/lib/netdev.c
index 91e91955c09b..bdfc45e9 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -960,18 +960,12 @@ netdev_push_header(const struct netdev *netdev,
size_t i, size = dp_packet_batch_size(batch);
DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, packet, batch) {
-if (OVS_UNLIKELY(dp_packet_hwol_is_tso(packet)
- || dp_packet_hwol_l4_mask(packet))) {
-COVERAGE_INC(netdev_push_header_drops);
-dp_packet_delete(packet);
-VLOG_WARN_RL(, "%s: Tunneling packets with HW offload flags is "
- "not supported: packet dropped",
- netdev_get_name(netdev));
-} else {
-netdev->netdev_class->push_header(netdev, packet, data);
-pkt_metadata_init(>md, data->out_port);
-dp_packet_batch_refill(batch, packet, i);
-}
+/* Tunneling packet with HW offload flags is not supported. */
+*dp_packet_ol_flags_ptr(packet) = 0;
+
+netdev->netdev_class->push_header(netdev, packet, data);
+pkt_metadata_init(>md, data->out_port);
+dp_packet_batch_refill(batch, packet, i);
}
return 0;
diff --git a/lib/packets.c b/lib/packets.c
index 4a7643c5dd3a..b0bb283acdfa 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -1887,3 +1887,37 @@ IP_ECN_set_ce(struct dp_packet *pkt, bool is_ipv6)
}
}
}
+
+void
+packet_csum_tcpudp(struct dp_packet *p)
+{
+struct eth_header *eth;
+struct ip_header *ip;
+struct tcp_header *tcp;
+struct udp_header *udp;
+uint32_t pseudo_hdr_csum;
+uint8_t l4proto;
+size_t l4_size;
+
+
Re: [ovs-dev] [PATCH ovn] Support configuring Load Balancer hairpin source IP.
On 2/3/21 3:26 PM, Numan Siddique wrote: On Fri, Jan 15, 2021 at 11:56 PM Dumitru Ceara wrote: In case traffic that gets load balanced is DNAT-ed to a backend IP that happens to be the source of the traffic then OVN performs an additional SNAT to ensure that return traffic is directed through OVN. Until now the load balancer VIP was chosen as SNAT IP. However, in specific scenarios, the CMS may prefer a different IP, e.g., a single cluster-wide IP. This commit adds support, through the newly added Load_Balancer.option 'hairpin_snat_ip', to allow the CMS to explicitly chose a SNAT IP. Due to the fact that now traffic that was hairpinned might need to be SNAT-ed to different IPs for different load balancers that share the same VIP address value we need to also explicitly match on L4 protocol and ports in the 'OFTABLE_CT_SNAT_FOR_VIP' table. Signed-off-by: Dumitru Ceara Thanks Dumitru for this patch. The patch LGTM. I applied to the master branch. Numan Thanks! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] ovn-controller doesn't bring re-added container ports up?
On 2/2/21 8:17 PM, Dumitru Ceara wrote:
On 2/2/21 7:18 PM, Numan Siddique wrote:
On Tue, Feb 2, 2021 at 11:26 PM Numan Siddique wrote:
On Tue, Feb 2, 2021 at 11:11 PM Ben Pfaff wrote:
The test "ovn -- nested containers" adds, removes, and then re-adds a
container port bar2. It appears to me that ovn-controller never brings
the re-added port up. I don't know why.
To see the problem, apply the following patch then run that test
(currently test 88):
Hi Ben,
Thanks for reporting this. I was able to reproduce locally. Let me debug
further and get back to you.
This regression is seen after the commit [1].
Oops, sorry about that.
Dumitru submitted a patch today [2] to fix an issue introduced by [1]
and looks like his patch
doesn't address this particular issue.
@Dumitru - Can you also please address the issue reported here in your
patch.
Sure I'll send a v2 after I fix the container port issue too.
Hi Ben, Numan,
I sent a v2:
http://patchwork.ozlabs.org/project/ovn/list/?series=227836
The first patch of the set fixes the problem Ben reported.
The other two patches fix the functionality during upgrade.
Regards,
Dumitru
Thanks,
Dumitru
[1] - 4d3cb42b07 ("binding: Set Logical_Switch_Port.up when all OVS
flows are installed.")
[2] -
https://patchwork.ozlabs.org/project/ovn/patch/1612259114-30311-1-git-send-email-dce...@redhat.com/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 3/3] northd: Allow backwards compatibility for Logical_Switch_Port.up.
In general, ovn-northd expects ovn-controller to set Port_Binding.up
before it declares the logical switch port as being up.
Even though the recommended upgrade procedure for OVN states that
ovn-controllers should be upgraded before ovn-northd, there are cases
when CMSs don't follow this guideline.
This would cause all existing and bound Logical_Switch_Ports to be
declared "down" until ovn-controllers are upgraded.
To avoid this situation, ovn-controllers now explicitly set
Chassis.other_config:port-up-notif in their own chassis record. Based
on this value, ovn-northd can determine if it needs to use the old type
of logic or the new one (Port_Binding.up) when setting LSP.up.
Note:
In case of downgrading ovn-controller before ovn-northd, if
ovn-controller is forcefully stopped it will not clear its chassis
record from the SB. Older versions will not have the capability to
clear the other_config:port-up-notif value so LSPs will be declared
"down" until ovn-northd is downgraded as well. As this
upgrade/downgrade procedure is not the recommended one, we don't try
to deal with this scenario.
Suggested-by: Numan Siddique
Signed-off-by: Dumitru Ceara
---
controller/chassis.c|7 +++
include/ovn/automake.mk |1 +
include/ovn/features.h | 22 ++
northd/ovn-northd.c | 13 -
ovn-sb.xml |5 +
tests/ovn-controller.at | 17 +
tests/ovn-northd.at | 22 ++
7 files changed, 86 insertions(+), 1 deletion(-)
create mode 100644 include/ovn/features.h
diff --git a/controller/chassis.c b/controller/chassis.c
index b4d4b0e..0937e33 100644
--- a/controller/chassis.c
+++ b/controller/chassis.c
@@ -28,6 +28,7 @@
#include "lib/ovn-sb-idl.h"
#include "ovn-controller.h"
#include "lib/util.h"
+#include "ovn/features.h"
VLOG_DEFINE_THIS_MODULE(chassis);
@@ -293,6 +294,7 @@ chassis_build_other_config(struct smap *config, const char
*bridge_mappings,
smap_replace(config, "iface-types", iface_types);
smap_replace(config, "ovn-chassis-mac-mappings", chassis_macs);
smap_replace(config, "is-interconn", is_interconn ? "true" : "false");
+smap_replace(config, OVN_FEATURE_PORT_UP_NOTIF, "true");
}
/*
@@ -363,6 +365,11 @@ chassis_other_config_changed(const char *bridge_mappings,
return true;
}
+if (!smap_get_bool(_rec->other_config, OVN_FEATURE_PORT_UP_NOTIF,
+ false)) {
+return true;
+}
+
return false;
}
diff --git a/include/ovn/automake.mk b/include/ovn/automake.mk
index 54b0e2c..582241a 100644
--- a/include/ovn/automake.mk
+++ b/include/ovn/automake.mk
@@ -2,5 +2,6 @@ ovnincludedir = $(includedir)/ovn
ovninclude_HEADERS = \
include/ovn/actions.h \
include/ovn/expr.h \
+ include/ovn/features.h \
include/ovn/lex.h \
include/ovn/logical-fields.h
diff --git a/include/ovn/features.h b/include/ovn/features.h
new file mode 100644
index 000..10ee46f
--- /dev/null
+++ b/include/ovn/features.h
@@ -0,0 +1,22 @@
+/* Copyright (c) 2021, Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef OVN_FEATURES_H
+#define OVN_FEATURES_H 1
+
+/* ovn-controller supported feature names. */
+#define OVN_FEATURE_PORT_UP_NOTIF "port-up-notif"
+
+#endif
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index ad1ade7..4047b93 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -40,6 +40,7 @@
#include "lib/lb.h"
#include "memory.h"
#include "ovn/actions.h"
+#include "ovn/features.h"
#include "ovn/logical-fields.h"
#include "packets.h"
#include "openvswitch/poll-loop.h"
@@ -12842,7 +12843,17 @@ handle_port_binding_changes(struct northd_context
*ctx, struct hmap *ports,
continue;
}
-bool up = ((sb->up && (*sb->up)) || lsp_is_router(op->nbsp));
+bool up = false;
+
+if (lsp_is_router(op->nbsp)) {
+up = true;
+} else if (sb->chassis) {
+up = smap_get_bool(>chassis->other_config,
+ OVN_FEATURE_PORT_UP_NOTIF, false)
+ ? sb->n_up && sb->up[0]
+ : true;
+}
+
if (!op->nbsp->up || *op->nbsp->up != up) {
nbrec_logical_switch_port_set_up(op->nbsp, , 1);
}
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 4c82d51..980a096 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -322,6 +322,11 @@
[ovs-dev] [PATCH ovn v2 2/3] binding: Set Port_Binding.up only if supported.
The supported upgrade procedure is to always upgrade ovn-controllers
first and OVN DBs and ovn-northd later. This leads however to the
situation when ovn-controller might try to set the Port_Binding.up field
while the Southbound DB isn't yet aware of this field which would
trigger transaction failures and control plane/data plane outages.
To avoid such situations ovn-controller only sets the Port_Binding.up
field if it was explicitly set to 'false'. This ensures that the SB
database was already upgraded.
On the ovn-northd side, as soon as ovn-northd is upgraded it will update
all existing Port_Bindings and explicitly set 'Port_Binding.up' to
false, implicitly notifying ovn-controller that it is safe to write to
the field.
Reported-by: Numan Siddique
Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows
are installed.")
Signed-off-by: Dumitru Ceara
---
controller-vtep/binding.c |6 --
controller/binding.c | 33 ++-
northd/ovn-northd.c |8
tests/ovn.at | 48 -
4 files changed, 82 insertions(+), 13 deletions(-)
diff --git a/controller-vtep/binding.c b/controller-vtep/binding.c
index d28a598..01d5a16 100644
--- a/controller-vtep/binding.c
+++ b/controller-vtep/binding.c
@@ -110,9 +110,11 @@ update_pb_chassis(const struct sbrec_port_binding
*port_binding_rec,
chassis_rec->name);
}
-bool up = true;
sbrec_port_binding_set_chassis(port_binding_rec, chassis_rec);
-sbrec_port_binding_set_up(port_binding_rec, , 1);
+if (port_binding_rec->n_up) {
+bool up = true;
+sbrec_port_binding_set_up(port_binding_rec, , 1);
+}
}
}
diff --git a/controller/binding.c b/controller/binding.c
index 353debe..efaa109 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -870,6 +870,11 @@ get_lport_type(const struct sbrec_port_binding *pb)
* container and virtual ports).
* Otherwise request a notification to be sent when the OVS flows
* corresponding to 'pb' have been installed.
+ *
+ * Note:
+ * Updates (directly or through a notification) the 'pb->up' field only if
+ * it's explicitly set to 'false'.
+ * This is to ensure compatibility with older versions of ovn-northd.
*/
static void
claimed_lport_set_up(const struct sbrec_port_binding *pb,
@@ -885,7 +890,7 @@ claimed_lport_set_up(const struct sbrec_port_binding *pb,
return;
}
-if (pb->chassis != chassis_rec) {
+if (pb->chassis != chassis_rec || (pb->n_up && !pb->up[0])) {
binding_iface_bound_add(pb->logical_port);
}
}
@@ -973,7 +978,10 @@ release_lport(const struct sbrec_port_binding *pb, bool
sb_readonly,
sbrec_port_binding_set_virtual_parent(pb, NULL);
}
-sbrec_port_binding_set_up(pb, NULL, 0);
+if (pb->n_up) {
+bool up = false;
+sbrec_port_binding_set_up(pb, , 1);
+}
update_lport_tracking(pb, tracked_datapaths);
binding_iface_released_add(pb->logical_port);
VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);
@@ -2503,7 +2511,10 @@ binding_seqno_run(struct shash *local_bindings)
ovsrec_interface_update_external_ids_delkey(
lb->iface, OVN_INSTALLED_EXT_ID);
}
-sbrec_port_binding_set_up(lb->pb, NULL, 0);
+if (lb->pb->n_up) {
+bool up = false;
+sbrec_port_binding_set_up(lb->pb, , 1);
+}
simap_put(_iface_seqno_map, lb->name, new_seqno);
}
sset_delete(_iface_bound_set, SSET_NODE_FROM_NAME(iface_id));
@@ -2536,7 +2547,6 @@ binding_seqno_install(struct shash *local_bindings)
SIMAP_FOR_EACH_SAFE (node, node_next, _iface_seqno_map) {
struct shash_node *lb_node = shash_find(local_bindings, node->name);
-bool up = true;
if (!lb_node) {
goto del_seqno;
@@ -2554,12 +2564,15 @@ binding_seqno_install(struct shash *local_bindings)
ovsrec_interface_update_external_ids_setkey(lb->iface,
OVN_INSTALLED_EXT_ID,
"true");
-sbrec_port_binding_set_up(lb->pb, , 1);
-
-struct shash_node *child_node;
-SHASH_FOR_EACH (child_node, >children) {
-struct local_binding *lb_child = child_node->data;
-sbrec_port_binding_set_up(lb_child->pb, , 1);
+if (lb->pb->n_up) {
+bool up = true;
+
+sbrec_port_binding_set_up(lb->pb, , 1);
+struct shash_node *child_node;
+SHASH_FOR_EACH (child_node, >children) {
+struct local_binding *lb_child = child_node->data;
+sbrec_port_binding_set_up(lb_child->pb, , 1);
+}
}
del_seqno:
diff --git a/northd/ovn-northd.c
[ovs-dev] [PATCH ovn v2 1/3] binding: Correctly set Port_Binding.up for container/virtual ports.
For port bindings that are children of other port bindings (container
and virtual ports) set Port_Binding.up directly, when claimed, if their
parent bindings are already up.
For non-VIF port bindings maintain compatibility with older versions
and set Port_Binding.up as soon as they are claimed.
Reported-by: Ben Pfaff
Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows
are installed.")
Signed-off-by: Dumitru Ceara
---
controller-vtep/binding.c |3 +++
controller/binding.c | 47 +++--
tests/ovn.at | 25 +++-
3 files changed, 68 insertions(+), 7 deletions(-)
diff --git a/controller-vtep/binding.c b/controller-vtep/binding.c
index 8337715..d28a598 100644
--- a/controller-vtep/binding.c
+++ b/controller-vtep/binding.c
@@ -109,7 +109,10 @@ update_pb_chassis(const struct sbrec_port_binding
*port_binding_rec,
port_binding_rec->chassis->name,
chassis_rec->name);
}
+
+bool up = true;
sbrec_port_binding_set_chassis(port_binding_rec, chassis_rec);
+sbrec_port_binding_set_up(port_binding_rec, , 1);
}
}
diff --git a/controller/binding.c b/controller/binding.c
index d44f635..353debe 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -864,21 +864,52 @@ get_lport_type(const struct sbrec_port_binding *pb)
return LP_UNKNOWN;
}
+/* For newly claimed ports, if 'notify_up' is 'false':
+ * - set the 'pb.up' field to true if 'pb' has no 'parent_pb'.
+ * - set the 'pb.up' field to true if 'parent_pb.up' is 'true' (e.g., for
+ * container and virtual ports).
+ * Otherwise request a notification to be sent when the OVS flows
+ * corresponding to 'pb' have been installed.
+ */
+static void
+claimed_lport_set_up(const struct sbrec_port_binding *pb,
+ const struct sbrec_port_binding *parent_pb,
+ const struct sbrec_chassis *chassis_rec,
+ bool notify_up)
+{
+if (!notify_up) {
+bool up = true;
+if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) {
+sbrec_port_binding_set_up(pb, , 1);
+}
+return;
+}
+
+if (pb->chassis != chassis_rec) {
+binding_iface_bound_add(pb->logical_port);
+}
+}
+
/* Returns false if lport is not claimed due to 'sb_readonly'.
* Returns true otherwise.
*/
static bool
claim_lport(const struct sbrec_port_binding *pb,
+const struct sbrec_port_binding *parent_pb,
const struct sbrec_chassis *chassis_rec,
const struct ovsrec_interface *iface_rec,
-bool sb_readonly, struct hmap *tracked_datapaths)
+bool sb_readonly, bool notify_up,
+struct hmap *tracked_datapaths)
{
+if (!sb_readonly) {
+claimed_lport_set_up(pb, parent_pb, chassis_rec, notify_up);
+}
+
if (pb->chassis != chassis_rec) {
if (sb_readonly) {
return false;
}
-binding_iface_bound_add(pb->logical_port);
if (pb->chassis) {
VLOG_INFO("Changing chassis for lport %s from %s to %s.",
pb->logical_port, pb->chassis->name,
@@ -1041,8 +1072,12 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
if (lbinding_set) {
if (can_bind) {
/* We can claim the lport. */
-if (!claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface,
- !b_ctx_in->ovnsb_idl_txn,
+const struct sbrec_port_binding *parent_pb =
+lbinding->parent ? lbinding->parent->pb : NULL;
+
+if (!claim_lport(pb, parent_pb, b_ctx_in->chassis_rec,
+ lbinding->iface, !b_ctx_in->ovnsb_idl_txn,
+ !lbinding->parent,
b_ctx_out->tracked_dp_bindings)){
return false;
}
@@ -1246,8 +1281,8 @@ consider_nonvif_lport_(const struct sbrec_port_binding
*pb,
b_ctx_out->tracked_dp_bindings);
update_local_lport_ids(pb, b_ctx_out);
-return claim_lport(pb, b_ctx_in->chassis_rec, NULL,
- !b_ctx_in->ovnsb_idl_txn,
+return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL,
+ !b_ctx_in->ovnsb_idl_txn, false,
b_ctx_out->tracked_dp_bindings);
} else if (pb->chassis == b_ctx_in->chassis_rec) {
return release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
diff --git a/tests/ovn.at b/tests/ovn.at
index e011263..c5baa94 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -8988,9 +8988,9 @@ bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int
external_ids:ct-zone-bar2)
AT_CHECK([test -z $bar2_zoneid])
# Add back bar2
-wait_for_ports_up
ovn-nbctl lsp-add bar bar2 vm2 1 \
-- lsp-set-addresses bar2 "f0:00:00:01:02:08 192.168.2.3"
+wait_for_ports_up
[ovs-dev] [PATCH v2 ovn 0/3] ovn: Fix port-up notification for child ports and handle upgrades.
First two patches of the series fix issues related to: - setting Port_Binding.up for container/virtual ports/ - correctly dealing with upgrades when ovn-controller is upgraded first (recommended way). The third patch of the series tries to handle out of order upgrades (ovn-northd before ovn-controller) in order to not avoid declaring logical switch ports down during the upgrade. Changes in V2: - turned this into a series that: - addresses Ben's bug report - adds support for out-of-order upgrades for this specific feature as suggested by Numan. Dumitru Ceara (3): binding: Correctly set Port_Binding.up for container/virtual ports. binding: Set Port_Binding.up only if supported. northd: Allow backwards compatibility for Logical_Switch_Port.up. controller-vtep/binding.c |5 +++ controller/binding.c | 78 - controller/chassis.c |7 include/ovn/automake.mk |1 + include/ovn/features.h| 22 + northd/ovn-northd.c | 21 ovn-sb.xml|5 +++ tests/ovn-controller.at | 17 ++ tests/ovn-northd.at | 22 + tests/ovn.at | 73 +- 10 files changed, 233 insertions(+), 18 deletions(-) create mode 100644 include/ovn/features.h ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] ovn-nbctl: do not allow duplicated ECMP routes
In the current ovn-nbctl lr-route-add implementation is possible to add
multiple duplicated routes adding --ecmp or --ecmp-symmetric-reply
option, e.g:
$ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 10.244.0.7/32 172.18.0.4
$ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 10.244.0.7/32 172.18.0.4
$ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 10.244.0.7/32 172.18.0.4
https://bugzilla.redhat.com/show_bug.cgi?id=1916842
Fix the issue checking nexthop for ECMP routes.
Signed-off-by: Lorenzo Bianconi
---
tests/ovn-nbctl.at| 19 +-
utilities/ovn-nbctl.c | 81 +++
2 files changed, 63 insertions(+), 37 deletions(-)
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index e4a2a9695..6d91aa4c5 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -1539,34 +1539,34 @@ AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
dnl Add ecmp routes
AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.0/24 11.0.0.1])
AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.2])
-AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.2])
AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.3])
-AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.3 lp0])
+AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.4 lp0])
AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
IPv4 Routes
10.0.0.0/24 11.0.0.1 dst-ip ecmp
10.0.0.0/24 11.0.0.2 dst-ip ecmp
- 10.0.0.0/24 11.0.0.2 dst-ip ecmp
10.0.0.0/24 11.0.0.3 dst-ip ecmp
- 10.0.0.0/24 11.0.0.3 dst-ip lp0 ecmp
+ 10.0.0.0/24 11.0.0.4 dst-ip lp0 ecmp
+])
+AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.2], [1], [],
+ [ovn-nbctl: duplicate nexthop for the same ECMP route
])
dnl Delete ecmp routes
AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.0.0/24 11.0.0.1])
AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
IPv4 Routes
- 10.0.0.0/24 11.0.0.2 dst-ip ecmp
10.0.0.0/24 11.0.0.2 dst-ip ecmp
10.0.0.0/24 11.0.0.3 dst-ip ecmp
- 10.0.0.0/24 11.0.0.3 dst-ip lp0 ecmp
+ 10.0.0.0/24 11.0.0.4 dst-ip lp0 ecmp
])
AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.0.0/24 11.0.0.2])
AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
IPv4 Routes
10.0.0.0/24 11.0.0.3 dst-ip ecmp
- 10.0.0.0/24 11.0.0.3 dst-ip lp0 ecmp
+ 10.0.0.0/24 11.0.0.4 dst-ip lp0 ecmp
])
-AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.0.0/24 11.0.0.3 lp0])
+AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.0.0/24 11.0.0.4 lp0])
AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
IPv4 Routes
10.0.0.0/24 11.0.0.3 dst-ip
@@ -1611,6 +1611,9 @@ AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0
2001:0db8:1::/64 2001:0db8:0:f103::3
AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 2001:0db8:1::/64
2001:0db8:0:f103::4])
AT_CHECK([ovn-nbctl lr-route-add lr0 2002:0db8:1::/64 2001:0db8:0:f103::5])
AT_CHECK([ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 2003:0db8:1::/64
2001:0db8:0:f103::6])
+AT_CHECK([ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 2003:0db8:1::/64
2001:0db8:0:f103::6], [1], [],
+ [ovn-nbctl: duplicate nexthop for the same ECMP route
+])
AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
IPv4 Routes
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index 3f28ba11a..07f2b008d 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -3917,6 +3917,48 @@ nbctl_lr_policy_list(struct ctl_context *ctx)
}
free(policies);
}
+
+static struct nbrec_logical_router_static_route *
+nbctl_lr_get_route_prefix(const struct nbrec_logical_router *lr,
+ char *prefix, char *next_hop, bool is_src_route,
+ bool ecmp)
+{
+for (int i = 0; i < lr->n_static_routes; i++) {
+struct nbrec_logical_router_static_route *route = lr->static_routes[i];
+
+/* Compare route policy. */
+char *nb_policy = route->policy;
+bool nb_is_src_route = false;
+if (nb_policy && !strcmp(nb_policy, "src-ip")) {
+nb_is_src_route = true;
+}
+if (is_src_route != nb_is_src_route) {
+continue;
+}
+
+/* Compare route prefix. */
+char *rt_prefix = normalize_prefix_str(route->ip_prefix);
+if (!rt_prefix) {
+/* Ignore existing prefix we couldn't parse. */
+continue;
+}
+
+if (strcmp(rt_prefix, prefix)) {
+free(rt_prefix);
+continue;
+}
+
+if (ecmp && strcmp(next_hop, route->nexthop)) {
+free(rt_prefix);
+continue;
+}
Re: [ovs-dev] [PATCH] rhel: Update for DPDK 20.11
On Wed, Feb 3, 2021 at 1:25 AM Ilya Maximets wrote:
>
> On 1/25/21 2:34 PM, Tonghao Zhang wrote:
> > On Fri, Jan 22, 2021 at 10:50 PM Timothy Redaelli
> > wrote:
> >>
> >> With DPDK 20.11, meson and pkgconfig are used instead of the old
> >> Makefile-based system and so --with-dpdk option is changed to only
> >> accept shared or static instead of the directory.
> >>
> >> This commit uses --with-dpdk=shared since Fedora and RHEL ship shared
> >> libraries of DPDK.
> >>
> >> Fixes: 252e1e576443 ("dpdk: Update to use DPDK v20.11.")
> >> Signed-off-by: Timothy Redaelli
>
> Thanks!
> I updated the subject line to better reflect the change and
> applied to master and branch-2.15.
>
> >> ---
> >> rhel/openvswitch-fedora.spec.in | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/rhel/openvswitch-fedora.spec.in
> >> b/rhel/openvswitch-fedora.spec.in
> >> index 2c0c4fa18..21929e6cf 100644
> >> --- a/rhel/openvswitch-fedora.spec.in
> >> +++ b/rhel/openvswitch-fedora.spec.in
> >> @@ -162,7 +162,7 @@ This package provides IPsec tunneling support for OVS
> >> tunnels.
> >> --disable-libcapng \
> >> %endif
> >> %if %{with dpdk}
> >> ---with-dpdk=$(dirname %{_datadir}/dpdk/*/.config) \
> >> +--with-dpdk=shared \
> > one question, should we change the version of dpdk required:
> > BuildRequires: dpdk-devel >= 17.05.1 --> 20.11
>
> Yes, we need that. Thanks for pointing out.
>
> Could one of you, please, prepare a patch with this change?
patch is sent, please review, thanks!
http://patchwork.ozlabs.org/project/openvswitch/patch/20210203151049.67212-1-xiangxia.m@gmail.com/
> Best regards, Ilya Maximets.
--
Best regards, Tonghao
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] rhel: Update build required dpdk.
From: Tonghao Zhang
Now OvS supports building with dpdk 20.11, and uses
the '--with-dpdk=shared' option in fedora.spec. Then
change version of dpdk 17.05.1 to 20.11.
Note that dpdk-devel-20.11.x is not released in fedora
distro, but use '20.11' is fine.
Fixes: 252e1e576443 ("dpdk: Update to use DPDK v20.11.")
Signed-off-by: Tonghao Zhang
---
rhel/openvswitch-fedora.spec.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
index 21929e6cf403..9b1a121f235c 100644
--- a/rhel/openvswitch-fedora.spec.in
+++ b/rhel/openvswitch-fedora.spec.in
@@ -70,7 +70,7 @@ BuildRequires: libcap-ng libcap-ng-devel
%endif
%if %{with dpdk}
BuildRequires: libpcap-devel numactl-devel
-BuildRequires: dpdk-devel >= 17.05.1
+BuildRequires: dpdk-devel >= 20.11
Provides: %{name}-dpdk = %{version}-%{release}
%endif
BuildRequires: unbound unbound-devel
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Support configuring Load Balancer hairpin source IP.
On Fri, Jan 15, 2021 at 11:56 PM Dumitru Ceara wrote:
>
> In case traffic that gets load balanced is DNAT-ed to a backend IP that
> happens to be the source of the traffic then OVN performs an additional
> SNAT to ensure that return traffic is directed through OVN.
>
> Until now the load balancer VIP was chosen as SNAT IP. However, in
> specific scenarios, the CMS may prefer a different IP, e.g., a single
> cluster-wide IP. This commit adds support, through the newly added
> Load_Balancer.option 'hairpin_snat_ip', to allow the CMS to explicitly
> chose a SNAT IP.
>
> Due to the fact that now traffic that was hairpinned might need to be
> SNAT-ed to different IPs for different load balancers that share the
> same VIP address value we need to also explicitly match on L4 protocol
> and ports in the 'OFTABLE_CT_SNAT_FOR_VIP' table.
>
> Signed-off-by: Dumitru Ceara
Thanks Dumitru for this patch. The patch LGTM.
I applied to the master branch.
Numan
> ---
> NEWS| 3 +
> controller/lflow.c | 53 +--
> lib/lb.c| 26
> lib/lb.h| 11
> lib/ovn-util.c | 21 ++
> lib/ovn-util.h | 1 +
> northd/ovn-northd.c | 9 +--
> ovn-nb.xml | 8 +++
> ovn-sb.ovsschema| 9 ++-
> ovn-sb.xml | 8 +++
> tests/ovn-northd.at | 6 ++
> tests/ovn.at| 182
> ++--
> 12 files changed, 261 insertions(+), 76 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index e89c5f4..57a9ba9 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -10,6 +10,9 @@ Post-v20.12.0
> "ovn-installed". This external-id is set by ovn-controller only after
> all
> openflow operations corresponding to the OVS interface being added have
> been processed.
> + - Add a new option to Load_Balancer.options, "hairpin_snat_ip", to allow
> +users to explicitly select which source IP should be used for load
> +balancer hairpin traffic.
>
> OVN v20.12.0 - 18 Dec 2020
> --
> diff --git a/controller/lflow.c b/controller/lflow.c
> index c02585b..5c53b0d 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -1191,26 +1191,30 @@ add_lb_vip_hairpin_flows(struct ovn_controller_lb *lb,
> struct match hairpin_reply_match = MATCH_CATCHALL_INITIALIZER;
>
> if (IN6_IS_ADDR_V4MAPPED(_vip->vip)) {
> -ovs_be32 ip4 = in6_addr_get_mapped_ipv4(_backend->ip);
> +ovs_be32 bip4 = in6_addr_get_mapped_ipv4(_backend->ip);
> +ovs_be32 vip4 = lb->hairpin_snat_ips.n_ipv4_addrs
> +? lb->hairpin_snat_ips.ipv4_addrs[0].addr
> +: in6_addr_get_mapped_ipv4(_vip->vip);
>
> match_set_dl_type(_match, htons(ETH_TYPE_IP));
> -match_set_nw_src(_match, ip4);
> -match_set_nw_dst(_match, ip4);
> -
> -match_set_dl_type(_reply_match,
> - htons(ETH_TYPE_IP));
> -match_set_nw_src(_reply_match, ip4);
> -match_set_nw_dst(_reply_match,
> - in6_addr_get_mapped_ipv4(_vip->vip));
> +match_set_nw_src(_match, bip4);
> +match_set_nw_dst(_match, bip4);
> +
> +match_set_dl_type(_reply_match, htons(ETH_TYPE_IP));
> +match_set_nw_src(_reply_match, bip4);
> +match_set_nw_dst(_reply_match, vip4);
> } else {
> +struct in6_addr *bip6 = _backend->ip;
> +struct in6_addr *vip6 = lb->hairpin_snat_ips.n_ipv6_addrs
> +? >hairpin_snat_ips.ipv6_addrs[0].addr
> +: _vip->vip;
> match_set_dl_type(_match, htons(ETH_TYPE_IPV6));
> -match_set_ipv6_src(_match, _backend->ip);
> -match_set_ipv6_dst(_match, _backend->ip);
> +match_set_ipv6_src(_match, bip6);
> +match_set_ipv6_dst(_match, bip6);
>
> -match_set_dl_type(_reply_match,
> - htons(ETH_TYPE_IPV6));
> -match_set_ipv6_src(_reply_match, _backend->ip);
> -match_set_ipv6_dst(_reply_match, _vip->vip);
> +match_set_dl_type(_reply_match, htons(ETH_TYPE_IPV6));
> +match_set_ipv6_src(_reply_match, bip6);
> +match_set_ipv6_dst(_reply_match, vip6);
> }
>
> if (lb_backend->port) {
> @@ -1256,6 +1260,7 @@ add_lb_vip_hairpin_flows(struct ovn_controller_lb *lb,
> static void
> add_lb_ct_snat_vip_flows(struct ovn_controller_lb *lb,
> struct ovn_lb_vip *lb_vip,
> + uint8_t lb_proto,
> struct ovn_desired_flow_table *flow_table)
> {
> uint64_t stub[1024 / 8];
> @@ -1279,10 +1284,16 @@ add_lb_ct_snat_vip_flows(struct ovn_controller_lb *lb,
>
> if (IN6_IS_ADDR_V4MAPPED(_vip->vip)) {
> nat->range_af = AF_INET;
> -nat->range.addr.ipv4.min = in6_addr_get_mapped_ipv4(_vip->vip);
> +nat->range.addr.ipv4.min =
> +lb->hairpin_snat_ips.n_ipv4_addrs
> +
Re: [ovs-dev] [PATCH 0/2] Add offload support for ct_state rpl and inv flags
On 2/3/21 1:39 PM, Ilya Maximets wrote: > On 2/3/21 12:58 PM, Marcelo Leitner wrote: >> On Wed, Feb 03, 2021 at 10:54:19AM +0200, Paul Blakey wrote: >>> >>> >>> On Tue, 2 Feb 2021, Marcelo Leitner wrote: >>> On Tue, Feb 02, 2021 at 04:59:44PM +0100, Ilya Maximets wrote: > On 2/2/21 4:52 PM, wenxu wrote: >> >> Hi, >> >> just ingore my patch. Now kernel can support match invalid >> ct_state in th tc flower. > > I don't think that we can ignore it. At least we need this > fix on stable branches. And also there are other conntrack > flags that are simply ignored, not only inv and rpl. These > are snat, dnat, rel, probably some other. So, we need this > change in some form anyway. That would be great, indeed. The issue is also present in tc layer, btw, but maybe Paul has his on the charts already. The effect here could be that if you use a newer ovs that supports inv ct_state, for example, up to wenxu's patch the kernel will simply ignore it. Best regards, Marcelo >>> >>> >>> Hi, >>> >>> Regarding the inv flag, yes if you offload a +inv rule, and not have >>> an updated kernel then tc rule won't match it and it will fall back to >>> software. Drivers should see this flag and reject offload, same with rpl. >> >> To be more precise, it will fallback to vswitchd upcall, because the >> tc rule will be accepted and a ovs kernel can't be added then. >> >> flower is using a u16 without a validation of known bits. But if >> unknown bits are used, flow dissector won't populate them, and such >> packet will "slip through" (it should have matched, but not). >> >> https://elixir.bootlin.com/linux/latest/source/net/sched/cls_flower.c#L689 >> https://elixir.bootlin.com/linux/latest/source/net/sched/cls_flower.c#L1398 >> >>> >>> We can probe for support by adding a ct_state rule with +inv,+rpl and see >>> the echo back. Add that for V2? >> >> With the above, I don't think that this (or probing in general in this >> case) is feasible. My understanding is that flower will just echo back >> the unknown bits, due to the above. > > IIUC, TC doesn't understand some flags and never reports any errors to > the userspace about that. In this case we should not try to offload > these flags to TC or we will have broken logic in datapath flows. > > I think, TC must return EINVAL or EOPNOTSUPP or something else in case > of unknown flags, so OVS will be able to put the correct flow to > openvswitch kernel module instead and avoid incorrect packet matches. > > Since current TC code doesn't do that, this needs to be implemented in > TC first. After that we can implement probing on the userspace side, > e.g. try to put certainly non-existent flags and check if TC supports > checking for invalid flags. If it supports that, we can after that check > for all real flags that might be not supported by current kernel. > > Until then, I think, we can not reliably support inv and rpl flags on > the OVS side. After a small discussion with Marcelo, summarizing the steps that are needed in order to support offloading of inv/rpl/etc. in OVS: 1. Finish work on "unsupport" patch (wenxu). Apply it and backport to all stable branches of OVS. 2. Fix TC in kernel, so it will reject unknown bits in ct_state, e.g. by returning EINVAL. This will allow OVS to fall back to install flow into the kernel module instead of TC. 3. Add probing mechanism to detect if kernel fixed or buggy. e.g. try to send UINT16_MAX as ct_state and check that kernel returns EINVAL. 4. Implement inv/rpl/etc. support for kernels that passes probing. OVS will not support these flags on buggy kernels even if underlying kernel supports them, because there is no way to tell if it actually supports them or just ignores. That is how I see that. Suggestions on how to do that differently are welcome. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/2] rhel: Add systemd-units for ovn-ic services
Thanks!
Regards,
Vladislav Odintsov
On 03.02.2021, 15:34, "dev on behalf of Numan Siddique"
wrote:
On Wed, Feb 3, 2021 at 2:15 PM wrote:
>
> From: Vladislav Odintsov
>
> New ovn-ic.service and ovn-ic-db.service systemd units were added
> to manage ovn-ic and ovsdb-servers for OVN IC respectively.
> ovn-ic-db.service manages both OVN_IC_Northbound and OVN_IC_Southbound
> OVSDB server processes.
>
> Submitted-at: https://github.com/ovn-org/ovn/pull/65
> Signed-off-by: Vladislav Odintsov
Thanks for the PR. I applied these patches to master.
Numan
> ---
> rhel/automake.mk | 2 +
> rhel/ovn-fedora.spec.in | 63 ++-
> rhel/usr_lib_systemd_system_ovn-ic-db.service | 32 ++
> rhel/usr_lib_systemd_system_ovn-ic.service| 31 +
> 4 files changed, 126 insertions(+), 2 deletions(-)
> create mode 100644 rhel/usr_lib_systemd_system_ovn-ic-db.service
> create mode 100644 rhel/usr_lib_systemd_system_ovn-ic.service
>
> diff --git a/rhel/automake.mk b/rhel/automake.mk
> index 661975ea9..3e71f5d80 100644
> --- a/rhel/automake.mk
> +++ b/rhel/automake.mk
> @@ -13,6 +13,8 @@ EXTRA_DIST += \
> rhel/ovn-fedora.spec.in \
> rhel/usr_lib_systemd_system_ovn-controller.service \
> rhel/usr_lib_systemd_system_ovn-controller-vtep.service \
> + rhel/usr_lib_systemd_system_ovn-ic.service \
> + rhel/usr_lib_systemd_system_ovn-ic-db.service \
> rhel/usr_lib_systemd_system_ovn-northd.service \
> rhel/usr_lib_firewalld_services_ovn-central-firewall-service.xml \
> rhel/usr_lib_firewalld_services_ovn-host-firewall-service.xml \
> diff --git a/rhel/ovn-fedora.spec.in b/rhel/ovn-fedora.spec.in
> index 6b11ef3e8..6716dd0d2 100644
> --- a/rhel/ovn-fedora.spec.in
> +++ b/rhel/ovn-fedora.spec.in
> @@ -161,7 +161,7 @@ install -p -D -m 0644 \
> rhel/usr_share_ovn_scripts_systemd_sysconfig.template \
> $RPM_BUILD_ROOT/%{_sysconfdir}/sysconfig/ovn
>
> -for service in ovn-controller ovn-controller-vtep ovn-northd; do
> +for service in ovn-controller ovn-controller-vtep ovn-northd ovn-ic
ovn-ic-db; do
> install -p -D -m 0644 \
> rhel/usr_lib_systemd_system_${service}.service \
> $RPM_BUILD_ROOT%{_unitdir}/${service}.service
> @@ -256,7 +256,7 @@ if [ $1 -eq 1 ] ; then
> if [[ "$?" = "0" && "$ovn_status" = "0" ]]; then
> # ovn-controller-vtep service is running which means old
> # openvswitch-ovn-vtep is installed and it will be cleaned up. So
> -# start ovn-controller-vtep service when posttrans host is
called.
> +# start ovn-controller-vtep service when posttrans vtep is
called.
> touch %{_localstatedir}/lib/rpm-state/ovn-controller-vtep
> fi
> fi
> @@ -272,6 +272,26 @@ fi
> fi
> %endif
>
> +%if 0%{?systemd_preun:1}
> +%systemd_preun ovn-ic.service
> +%else
> +if [ $1 -eq 0 ] ; then
> +# Package removal, not upgrade
> +/bin/systemctl --no-reload disable ovn-ic.service >/dev/null
2>&1 || :
> +/bin/systemctl stop ovn-ic.service >/dev/null 2>&1 || :
> +fi
> +%endif
> +
> +%if 0%{?systemd_preun:1}
> +%systemd_preun ovn-ic-db.service
> +%else
> +if [ $1 -eq 0 ] ; then
> +# Package removal, not upgrade
> +/bin/systemctl --no-reload disable ovn-ic-db.service >/dev/null
2>&1 || :
> +/bin/systemctl stop ovn-ic-db.service >/dev/null 2>&1 || :
> +fi
> +%endif
> +
> %preun host
> %if 0%{?systemd_preun:1}
> %systemd_preun ovn-controller.service
> @@ -312,6 +332,24 @@ fi
> fi
> %endif
>
> +%if 0%{?systemd_post:1}
> +%systemd_post ovn-ic.service
> +%else
> +# Package install, not upgrade
> +if [ $1 -eq 1 ]; then
> +/bin/systemctl daemon-reload >dev/null || :
> +fi
> +%endif
> +
> +%if 0%{?systemd_post:1}
> +%systemd_post ovn-ic-db.service
> +%else
> +# Package install, not upgrade
> +if [ $1 -eq 1 ]; then
> +/bin/systemctl daemon-reload >dev/null || :
> +fi
> +%endif
> +
> %post host
> %if 0%{?systemd_post:1}
> %systemd_post ovn-controller.service
> @@ -345,6 +383,22 @@ fi
> fi
> %endif
>
> +%if 0%{?systemd_postun_with_restart:1}
> +%systemd_postun_with_restart ovn-ic.service
> +%else
> +/bin/systemctl daemon-reload >/dev/null 2>&1 || :
> +if [ "$1" -ge "1" ] ; then
> +# Package upgrade, not uninstall
> +/bin/systemctl
Re: [ovs-dev] [PATCH ovn] docs: Update information about OVN Patchwork Instance
On Thu, Jan 28, 2021 at 10:21 PM Dumitru Ceara wrote: > > On 1/27/21 3:23 PM, Frode Nordahl wrote: > > As per [0] the OVN project now has a separate Patchwork Instance, > > let's reflect that fact in the documentation. > > > > 0: https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374317.html > > Signed-off-by: Frode Nordahl > > --- > > Looks good to me, thanks! > > Acked-by: Dumitru Ceara Thanks for the patch and reviews. I applied this patch to master. Numan > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 0/2] Add offload support for ct_state rpl and inv flags
On 2/3/21 12:58 PM, Marcelo Leitner wrote:
> On Wed, Feb 03, 2021 at 10:54:19AM +0200, Paul Blakey wrote:
>>
>>
>> On Tue, 2 Feb 2021, Marcelo Leitner wrote:
>>
>>> On Tue, Feb 02, 2021 at 04:59:44PM +0100, Ilya Maximets wrote:
On 2/2/21 4:52 PM, wenxu wrote:
>
> Hi,
>
> just ingore my patch. Now kernel can support match invalid
> ct_state in th tc flower.
I don't think that we can ignore it. At least we need this
fix on stable branches. And also there are other conntrack
flags that are simply ignored, not only inv and rpl. These
are snat, dnat, rel, probably some other. So, we need this
change in some form anyway.
>>>
>>> That would be great, indeed.
>>>
>>> The issue is also present in tc layer, btw, but maybe Paul has his on
>>> the charts already. The effect here could be that if you use a newer
>>> ovs that supports inv ct_state, for example, up to wenxu's patch the
>>> kernel will simply ignore it.
>>>
>>> Best regards,
>>> Marcelo
>>
>>
>> Hi,
>>
>> Regarding the inv flag, yes if you offload a +inv rule, and not have
>> an updated kernel then tc rule won't match it and it will fall back to
>> software. Drivers should see this flag and reject offload, same with rpl.
>
> To be more precise, it will fallback to vswitchd upcall, because the
> tc rule will be accepted and a ovs kernel can't be added then.
>
> flower is using a u16 without a validation of known bits. But if
> unknown bits are used, flow dissector won't populate them, and such
> packet will "slip through" (it should have matched, but not).
>
> https://elixir.bootlin.com/linux/latest/source/net/sched/cls_flower.c#L689
> https://elixir.bootlin.com/linux/latest/source/net/sched/cls_flower.c#L1398
>
>>
>> We can probe for support by adding a ct_state rule with +inv,+rpl and see
>> the echo back. Add that for V2?
>
> With the above, I don't think that this (or probing in general in this
> case) is feasible. My understanding is that flower will just echo back
> the unknown bits, due to the above.
IIUC, TC doesn't understand some flags and never reports any errors to
the userspace about that. In this case we should not try to offload
these flags to TC or we will have broken logic in datapath flows.
I think, TC must return EINVAL or EOPNOTSUPP or something else in case
of unknown flags, so OVS will be able to put the correct flow to
openvswitch kernel module instead and avoid incorrect packet matches.
Since current TC code doesn't do that, this needs to be implemented in
TC first. After that we can implement probing on the userspace side,
e.g. try to put certainly non-existent flags and check if TC supports
checking for invalid flags. If it supports that, we can after that check
for all real flags that might be not supported by current kernel.
Until then, I think, we can not reliably support inv and rpl flags on
the OVS side.
>
>>
>> As for missing other flags, I suggest to add a check that all flags were
>> parsed, the same as we do with
>> match's mask, set translated flags mask to 0, and then check if we
>> missed some new flag by checking mask the ct_state mask was zeroed.
>>
>> Like this:
>>
>>
>> >From 48a4cb1b537d3837df8c40c9c265ae95e9729255 Mon Sep 17 00:00:00 2001
>> From: Paul Blakey
>> Date: Wed, 3 Feb 2021 10:27:21 +0200
>> Subject: [PATCH] netdev-offload-tc: Check for unparsed ct_state flags
>>
>> Don't offload flows where we didn't fully parse ct_state flags.
>>
>> Signed-off-by: Paul Blakey
>> ---
>> lib/netdev-offload-tc.c | 6 +-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>> index 08a4735..4362c6e 100644
>> --- a/lib/netdev-offload-tc.c
>> +++ b/lib/netdev-offload-tc.c
>> @@ -1705,7 +1705,11 @@ netdev_tc_flow_put(struct netdev *netdev, struct
>> match *match,
>> flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_INVALID;
>> }
>>
>> -mask->ct_state = 0;
>> +mask->ct_state &= ~(OVS_CS_F_INVALID |
>> +OVS_CS_F_REPLY_DIR |
>> +OVS_CS_F_TRACKED |
>> +OVS_CS_F_ESTABLISHED |
>> +OVS_CS_F_NEW);
>> }
>>
>> if (mask->ct_zone) {
>> --
>>
>> Does that sound good to you?
>
> Yes.
>
>> If so, as standalone patch, or in this patchset (will rebase the above as
>> first patch in this series)?
>
> To ease the maintainers work, the optimal combination is a standalone
> patch, to be applied before this patchset (without rpl and inv), is
> welcomed (as it can be easily backported to stable branches). And then
> a v2 here to extend the validation.
We definitely need the fix first and new features after that. But I'm
not sure if we can accept this feature right now if TC ignores unknown
flags and we have no reliable way to tell if it actually supports them
or not.
Taking into account what I said above and also my reply to
Re: [ovs-dev] [PATCH ovn 2/2] rhel: Add systemd-units for ovn-ic services
On Wed, Feb 3, 2021 at 2:15 PM wrote:
>
> From: Vladislav Odintsov
>
> New ovn-ic.service and ovn-ic-db.service systemd units were added
> to manage ovn-ic and ovsdb-servers for OVN IC respectively.
> ovn-ic-db.service manages both OVN_IC_Northbound and OVN_IC_Southbound
> OVSDB server processes.
>
> Submitted-at: https://github.com/ovn-org/ovn/pull/65
> Signed-off-by: Vladislav Odintsov
Thanks for the PR. I applied these patches to master.
Numan
> ---
> rhel/automake.mk | 2 +
> rhel/ovn-fedora.spec.in | 63 ++-
> rhel/usr_lib_systemd_system_ovn-ic-db.service | 32 ++
> rhel/usr_lib_systemd_system_ovn-ic.service| 31 +
> 4 files changed, 126 insertions(+), 2 deletions(-)
> create mode 100644 rhel/usr_lib_systemd_system_ovn-ic-db.service
> create mode 100644 rhel/usr_lib_systemd_system_ovn-ic.service
>
> diff --git a/rhel/automake.mk b/rhel/automake.mk
> index 661975ea9..3e71f5d80 100644
> --- a/rhel/automake.mk
> +++ b/rhel/automake.mk
> @@ -13,6 +13,8 @@ EXTRA_DIST += \
> rhel/ovn-fedora.spec.in \
> rhel/usr_lib_systemd_system_ovn-controller.service \
> rhel/usr_lib_systemd_system_ovn-controller-vtep.service \
> + rhel/usr_lib_systemd_system_ovn-ic.service \
> + rhel/usr_lib_systemd_system_ovn-ic-db.service \
> rhel/usr_lib_systemd_system_ovn-northd.service \
> rhel/usr_lib_firewalld_services_ovn-central-firewall-service.xml \
> rhel/usr_lib_firewalld_services_ovn-host-firewall-service.xml \
> diff --git a/rhel/ovn-fedora.spec.in b/rhel/ovn-fedora.spec.in
> index 6b11ef3e8..6716dd0d2 100644
> --- a/rhel/ovn-fedora.spec.in
> +++ b/rhel/ovn-fedora.spec.in
> @@ -161,7 +161,7 @@ install -p -D -m 0644 \
> rhel/usr_share_ovn_scripts_systemd_sysconfig.template \
> $RPM_BUILD_ROOT/%{_sysconfdir}/sysconfig/ovn
>
> -for service in ovn-controller ovn-controller-vtep ovn-northd; do
> +for service in ovn-controller ovn-controller-vtep ovn-northd ovn-ic
> ovn-ic-db; do
> install -p -D -m 0644 \
> rhel/usr_lib_systemd_system_${service}.service \
> $RPM_BUILD_ROOT%{_unitdir}/${service}.service
> @@ -256,7 +256,7 @@ if [ $1 -eq 1 ] ; then
> if [[ "$?" = "0" && "$ovn_status" = "0" ]]; then
> # ovn-controller-vtep service is running which means old
> # openvswitch-ovn-vtep is installed and it will be cleaned up. So
> -# start ovn-controller-vtep service when posttrans host is called.
> +# start ovn-controller-vtep service when posttrans vtep is called.
> touch %{_localstatedir}/lib/rpm-state/ovn-controller-vtep
> fi
> fi
> @@ -272,6 +272,26 @@ fi
> fi
> %endif
>
> +%if 0%{?systemd_preun:1}
> +%systemd_preun ovn-ic.service
> +%else
> +if [ $1 -eq 0 ] ; then
> +# Package removal, not upgrade
> +/bin/systemctl --no-reload disable ovn-ic.service >/dev/null 2>&1 ||
> :
> +/bin/systemctl stop ovn-ic.service >/dev/null 2>&1 || :
> +fi
> +%endif
> +
> +%if 0%{?systemd_preun:1}
> +%systemd_preun ovn-ic-db.service
> +%else
> +if [ $1 -eq 0 ] ; then
> +# Package removal, not upgrade
> +/bin/systemctl --no-reload disable ovn-ic-db.service >/dev/null 2>&1
> || :
> +/bin/systemctl stop ovn-ic-db.service >/dev/null 2>&1 || :
> +fi
> +%endif
> +
> %preun host
> %if 0%{?systemd_preun:1}
> %systemd_preun ovn-controller.service
> @@ -312,6 +332,24 @@ fi
> fi
> %endif
>
> +%if 0%{?systemd_post:1}
> +%systemd_post ovn-ic.service
> +%else
> +# Package install, not upgrade
> +if [ $1 -eq 1 ]; then
> +/bin/systemctl daemon-reload >dev/null || :
> +fi
> +%endif
> +
> +%if 0%{?systemd_post:1}
> +%systemd_post ovn-ic-db.service
> +%else
> +# Package install, not upgrade
> +if [ $1 -eq 1 ]; then
> +/bin/systemctl daemon-reload >dev/null || :
> +fi
> +%endif
> +
> %post host
> %if 0%{?systemd_post:1}
> %systemd_post ovn-controller.service
> @@ -345,6 +383,22 @@ fi
> fi
> %endif
>
> +%if 0%{?systemd_postun_with_restart:1}
> +%systemd_postun_with_restart ovn-ic.service
> +%else
> +/bin/systemctl daemon-reload >/dev/null 2>&1 || :
> +if [ "$1" -ge "1" ] ; then
> +# Package upgrade, not uninstall
> +/bin/systemctl try-restart ovn-ic.service >/dev/null 2>&1 || :
> +fi
> +%endif
> +
> +%if 0%{?systemd_postun:1}
> +%systemd_postun ovn-ic-db.service
> +%else
> +/bin/systemctl daemon-reload >/dev/null 2>&1 || :
> +%endif
> +
> %postun host
> %if 0%{?systemd_postun_with_restart:1}
> %systemd_postun_with_restart ovn-controller.service
> @@ -439,6 +493,8 @@ fi
> %config %{_datadir}/ovn/ovn-sb.ovsschema
> %config %{_datadir}/ovn/ovn-ic-nb.ovsschema
> %config %{_datadir}/ovn/ovn-ic-sb.ovsschema
> +%{_unitdir}/ovn-ic.service
> +%{_unitdir}/ovn-ic-db.service
>
Re: [ovs-dev] [PATCH ovn] binding: Set Port_Binding.up only if supported.
On Wed, Feb 3, 2021 at 5:43 PM Dumitru Ceara wrote:
>
> On 2/3/21 12:06 PM, Numan Siddique wrote:
> > On Tue, Feb 2, 2021 at 3:15 PM Dumitru Ceara wrote:
> >>
> >> The supported upgrade procedure is to always upgrade ovn-controllers
> >> first and OVN DBs and ovn-northd later. This leads however to the
> >> situation when ovn-controller might try to set the Port_Binding.up field
> >> while the Southbound DB isn't yet aware of this field which would
> >> trigger transaction failures and control plane/data plane outages.
> >>
> >> To avoid such situations ovn-controller only sets the Port_Binding.up
> >> field if it was explicitly set to 'false'. This ensures that the SB
> >> database was already upgraded.
> >>
> >> On the ovn-northd side, as soon as ovn-northd is upgraded it will update
> >> all existing Port_Bindings and explicitly set 'Port_Binding.up' to
> >> false, implicitly notifying ovn-controller that it is safe to write to
> >> the field.
> >>
> >> Reported-by: Numan Siddique
> >> Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS
> >> flows are installed.")
> >> Signed-off-by: Dumitru Ceara
> >
> >
> > Thanks Dumitru for addressing this issue.
>
> Thanks for the review!
>
> >
> > I see a problem with the approach taken here when the central nodes
> > are upgraded first.
> >
> > I do understand that OVN recommends updating/upgrading ovn-controllers
> > first. But from
> > what I have seen, Openstack tripleo (and possibly Openshift too)
> > update the central nodes
> > first.
> >
> > Suppose if ovn-northd and DBs are upgraded first, then after this
> > patch, ovn-northd sets
> > the logical_switch_port.up to 'down' for all the logical ports until
> > the ovn-controllers are
> > upgraded. This could cause some control plane issues. Like Openstack
> > neutron may notify
> > Nova service of vif-unplugged events. We could see this issue even if
> > ovn-controllers are configured
> > with - "ovn-match-northd-version=true".
> >
> > Another approach to solve this problem would be - ovn-controller will
> > not set the port_binding.up
> > to true if the internal version in SB DB is lesser than the version in
> > which this new column - port_binding.up
> > was added.
> >
> > What do you think ?
> >
>
> An alternative is to add a "features" column to SB.Chassis. Newer
> ovn-controllers could register there that they support a specific
> feature, e.g., "port-up-notification".
>
> In northd we could check the feature support for chassis that claimed a
> port binding and based on that take into account Port_Binding.up when
> setting LSP.up.
>
> Does this sound ok?
This sounds fine to me.
Thanks
Numan
>
> Thanks,
> Dumitru
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] netdev-offload-tc: Reject install rules for tc flower unsupported ct_state flags
On 2/3/21 1:09 PM, Marcelo Ricardo Leitner wrote:
> On Wed, Feb 03, 2021 at 01:30:00PM +0800, we...@ucloud.cn wrote:
> ...
>> @@ -1641,6 +1644,10 @@ netdev_tc_flow_put(struct netdev *netdev, struct
>> match *match,
>> }
>>
>> if (mask->ct_state) {
>> +if (mask->ct_state & TC_UNSUPP_OVS_CS_FLAGS) {
>> +return EOPNOTSUPP;
>> +}
>> +
>
> Hi Wenxu,
>
> Paul's proposal on the other thread is more aligned with how the rest
> of the code is doing such kind of validation.
Well, it's more aligned, but still not good enough.
Keeping lists of supported things is error prone in both cases.
I think, we need to just disable flags at the point where these flags
were consumed, e.g.
---
if (mask->ct_state & OVS_CS_F_ESTABLISHED) {
if (key->ct_state & OVS_CS_F_ESTABLISHED) {
flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED;
}
flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED;
+mask->ct_state &= ~OVS_CS_F_ESTABLISHED;
}
---
And we should not touch 'mask->ct_state' anywhere else, so test_key_and_mask()
will catch all the unknown bits.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] binding: Set Port_Binding.up only if supported.
On 2/3/21 12:06 PM, Numan Siddique wrote:
On Tue, Feb 2, 2021 at 3:15 PM Dumitru Ceara wrote:
The supported upgrade procedure is to always upgrade ovn-controllers
first and OVN DBs and ovn-northd later. This leads however to the
situation when ovn-controller might try to set the Port_Binding.up field
while the Southbound DB isn't yet aware of this field which would
trigger transaction failures and control plane/data plane outages.
To avoid such situations ovn-controller only sets the Port_Binding.up
field if it was explicitly set to 'false'. This ensures that the SB
database was already upgraded.
On the ovn-northd side, as soon as ovn-northd is upgraded it will update
all existing Port_Bindings and explicitly set 'Port_Binding.up' to
false, implicitly notifying ovn-controller that it is safe to write to
the field.
Reported-by: Numan Siddique
Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows are
installed.")
Signed-off-by: Dumitru Ceara
Thanks Dumitru for addressing this issue.
Thanks for the review!
I see a problem with the approach taken here when the central nodes
are upgraded first.
I do understand that OVN recommends updating/upgrading ovn-controllers
first. But from
what I have seen, Openstack tripleo (and possibly Openshift too)
update the central nodes
first.
Suppose if ovn-northd and DBs are upgraded first, then after this
patch, ovn-northd sets
the logical_switch_port.up to 'down' for all the logical ports until
the ovn-controllers are
upgraded. This could cause some control plane issues. Like Openstack
neutron may notify
Nova service of vif-unplugged events. We could see this issue even if
ovn-controllers are configured
with - "ovn-match-northd-version=true".
Another approach to solve this problem would be - ovn-controller will
not set the port_binding.up
to true if the internal version in SB DB is lesser than the version in
which this new column - port_binding.up
was added.
What do you think ?
An alternative is to add a "features" column to SB.Chassis. Newer
ovn-controllers could register there that they support a specific
feature, e.g., "port-up-notification".
In northd we could check the feature support for chassis that claimed a
port binding and based on that take into account Port_Binding.up when
setting LSP.up.
Does this sound ok?
Thanks,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] netdev-offload-tc: Reject install rules for tc flower unsupported ct_state flags
On Wed, Feb 03, 2021 at 01:30:00PM +0800, we...@ucloud.cn wrote:
...
> @@ -1641,6 +1644,10 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> }
>
> if (mask->ct_state) {
> +if (mask->ct_state & TC_UNSUPP_OVS_CS_FLAGS) {
> +return EOPNOTSUPP;
> +}
> +
Hi Wenxu,
Paul's proposal on the other thread is more aligned with how the rest
of the code is doing such kind of validation.
Thanks,
Marcelo
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 0/2] Add offload support for ct_state rpl and inv flags
On Wed, Feb 03, 2021 at 10:54:19AM +0200, Paul Blakey wrote:
>
>
> On Tue, 2 Feb 2021, Marcelo Leitner wrote:
>
> > On Tue, Feb 02, 2021 at 04:59:44PM +0100, Ilya Maximets wrote:
> > > On 2/2/21 4:52 PM, wenxu wrote:
> > > >
> > > > Hi,
> > > >
> > > > just ingore my patch. Now kernel can support match invalid
> > > > ct_state in th tc flower.
> > >
> > > I don't think that we can ignore it. At least we need this
> > > fix on stable branches. And also there are other conntrack
> > > flags that are simply ignored, not only inv and rpl. These
> > > are snat, dnat, rel, probably some other. So, we need this
> > > change in some form anyway.
> >
> > That would be great, indeed.
> >
> > The issue is also present in tc layer, btw, but maybe Paul has his on
> > the charts already. The effect here could be that if you use a newer
> > ovs that supports inv ct_state, for example, up to wenxu's patch the
> > kernel will simply ignore it.
> >
> > Best regards,
> > Marcelo
>
>
> Hi,
>
> Regarding the inv flag, yes if you offload a +inv rule, and not have
> an updated kernel then tc rule won't match it and it will fall back to
> software. Drivers should see this flag and reject offload, same with rpl.
To be more precise, it will fallback to vswitchd upcall, because the
tc rule will be accepted and a ovs kernel can't be added then.
flower is using a u16 without a validation of known bits. But if
unknown bits are used, flow dissector won't populate them, and such
packet will "slip through" (it should have matched, but not).
https://elixir.bootlin.com/linux/latest/source/net/sched/cls_flower.c#L689
https://elixir.bootlin.com/linux/latest/source/net/sched/cls_flower.c#L1398
>
> We can probe for support by adding a ct_state rule with +inv,+rpl and see
> the echo back. Add that for V2?
With the above, I don't think that this (or probing in general in this
case) is feasible. My understanding is that flower will just echo back
the unknown bits, due to the above.
>
> As for missing other flags, I suggest to add a check that all flags were
> parsed, the same as we do with
> match's mask, set translated flags mask to 0, and then check if we
> missed some new flag by checking mask the ct_state mask was zeroed.
>
> Like this:
>
>
> >From 48a4cb1b537d3837df8c40c9c265ae95e9729255 Mon Sep 17 00:00:00 2001
> From: Paul Blakey
> Date: Wed, 3 Feb 2021 10:27:21 +0200
> Subject: [PATCH] netdev-offload-tc: Check for unparsed ct_state flags
>
> Don't offload flows where we didn't fully parse ct_state flags.
>
> Signed-off-by: Paul Blakey
> ---
> lib/netdev-offload-tc.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 08a4735..4362c6e 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1705,7 +1705,11 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_INVALID;
> }
>
> -mask->ct_state = 0;
> +mask->ct_state &= ~(OVS_CS_F_INVALID |
> +OVS_CS_F_REPLY_DIR |
> +OVS_CS_F_TRACKED |
> +OVS_CS_F_ESTABLISHED |
> +OVS_CS_F_NEW);
> }
>
> if (mask->ct_zone) {
> --
>
> Does that sound good to you?
Yes.
> If so, as standalone patch, or in this patchset (will rebase the above as
> first patch in this series)?
To ease the maintainers work, the optimal combination is a standalone
patch, to be applied before this patchset (without rpl and inv), is
welcomed (as it can be easily backported to stable branches). And then
a v2 here to extend the validation.
Thanks,
Marcelo
>
> Thanks,
> Paul.
>
> >
> > >
> > > Best regards, Ilya Maximets.
> > >
> > > >
> > > > BR
> > > > wenxu
> > > >
> > > >
> > > > From: Ilya Maximets
> > > > Date: 2021-02-02 23:33:41
> > > > To: Paul Blakey ,d...@openvswitch.org
> > > > Cc: Oz Shlomo ,i.maxim...@ovn.org,Marcelo Leitner
> > > > ,wenxu
> > > > Subject: Re: [ovs-dev] [PATCH 0/2] Add offload support for ct_state rpl
> > > > and inv flags>On 2/2/21 1:47 PM, Paul Blakey wrote:
> > > >>> Add offload support for ct_state rpl and inv flags.
> > > >>
> > > >>Hi.
> > > >>
> > > >>Since you're working on this, could you, please, review the following
> > > >>patch:
> > > >>
> > > >> http://patchwork.ozlabs.org/project/openvswitch/patch/1610344573-15780-1-git-send-email-we...@ucloud.cn/
> > > >>
> > > >>It seems like current OVS just ignores all the flags that can not
> > > >>be passed to TC and this doesn't look right.
> > > >>
> > > >>>
> > > >>> For example:
> > > >>> ovs-ofctl del-flows br-ovs
> > > >>> ovs-ofctl add-flow br-ovs arp,actions=normal
> > > >>> ovs-ofctl add-flow br-ovs "table=0, ip,ct_state=-trk
> > > >>> actions=ct(table=1,zone=5)"
> > > >>> ovs-ofctl add-flow br-ovs "table=1, ip,ct_state=+trk+new
> > > >>>
Re: [ovs-dev] [PATCH ovn] binding: Set Port_Binding.up only if supported.
On Tue, Feb 2, 2021 at 3:15 PM Dumitru Ceara wrote:
>
> The supported upgrade procedure is to always upgrade ovn-controllers
> first and OVN DBs and ovn-northd later. This leads however to the
> situation when ovn-controller might try to set the Port_Binding.up field
> while the Southbound DB isn't yet aware of this field which would
> trigger transaction failures and control plane/data plane outages.
>
> To avoid such situations ovn-controller only sets the Port_Binding.up
> field if it was explicitly set to 'false'. This ensures that the SB
> database was already upgraded.
>
> On the ovn-northd side, as soon as ovn-northd is upgraded it will update
> all existing Port_Bindings and explicitly set 'Port_Binding.up' to
> false, implicitly notifying ovn-controller that it is safe to write to
> the field.
>
> Reported-by: Numan Siddique
> Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows
> are installed.")
> Signed-off-by: Dumitru Ceara
Thanks Dumitru for addressing this issue.
I see a problem with the approach taken here when the central nodes
are upgraded first.
I do understand that OVN recommends updating/upgrading ovn-controllers
first. But from
what I have seen, Openstack tripleo (and possibly Openshift too)
update the central nodes
first.
Suppose if ovn-northd and DBs are upgraded first, then after this
patch, ovn-northd sets
the logical_switch_port.up to 'down' for all the logical ports until
the ovn-controllers are
upgraded. This could cause some control plane issues. Like Openstack
neutron may notify
Nova service of vif-unplugged events. We could see this issue even if
ovn-controllers are configured
with - "ovn-match-northd-version=true".
Another approach to solve this problem would be - ovn-controller will
not set the port_binding.up
to true if the internal version in SB DB is lesser than the version in
which this new column - port_binding.up
was added.
What do you think ?
Thanks
Numan
> ---
> controller/binding.c | 32 +++-
> northd/ovn-northd.c | 8
> tests/ovn.at | 48 +++-
> 3 files changed, 78 insertions(+), 10 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index d44f635..913c9d6 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -895,6 +895,12 @@ claim_lport(const struct sbrec_port_binding *pb,
> if (tracked_datapaths) {
> update_lport_tracking(pb, tracked_datapaths);
> }
> +} else if (!sb_readonly && pb->up && !(*pb->up)) {
> +/* For already claimed port bindings, potentially created by older
> + * versions of ovn-northd, make sure the 'pb->up' field gets updated
> + * only if it's explicitly set to 'false'.
> + */
> +binding_iface_bound_add(pb->logical_port);
> }
>
> /* Check if the port encap binding, if any, has changed */
> @@ -942,7 +948,10 @@ release_lport(const struct sbrec_port_binding *pb, bool
> sb_readonly,
> sbrec_port_binding_set_virtual_parent(pb, NULL);
> }
>
> -sbrec_port_binding_set_up(pb, NULL, 0);
> +if (pb->up) {
> +bool up = false;
> +sbrec_port_binding_set_up(pb, , 1);
> +}
> update_lport_tracking(pb, tracked_datapaths);
> binding_iface_released_add(pb->logical_port);
> VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);
> @@ -2468,7 +2477,10 @@ binding_seqno_run(struct shash *local_bindings)
> ovsrec_interface_update_external_ids_delkey(
> lb->iface, OVN_INSTALLED_EXT_ID);
> }
> -sbrec_port_binding_set_up(lb->pb, NULL, 0);
> +if (lb->pb->up) {
> +bool up = false;
> +sbrec_port_binding_set_up(lb->pb, , 1);
> +}
> simap_put(_iface_seqno_map, lb->name, new_seqno);
> }
> sset_delete(_iface_bound_set, SSET_NODE_FROM_NAME(iface_id));
> @@ -2501,7 +2513,6 @@ binding_seqno_install(struct shash *local_bindings)
>
> SIMAP_FOR_EACH_SAFE (node, node_next, _iface_seqno_map) {
> struct shash_node *lb_node = shash_find(local_bindings, node->name);
> -bool up = true;
>
> if (!lb_node) {
> goto del_seqno;
> @@ -2519,12 +2530,15 @@ binding_seqno_install(struct shash *local_bindings)
> ovsrec_interface_update_external_ids_setkey(lb->iface,
> OVN_INSTALLED_EXT_ID,
> "true");
> -sbrec_port_binding_set_up(lb->pb, , 1);
> -
> -struct shash_node *child_node;
> -SHASH_FOR_EACH (child_node, >children) {
> -struct local_binding *lb_child = child_node->data;
> -sbrec_port_binding_set_up(lb_child->pb, , 1);
> +if (lb->pb->up) {
> +bool up = true;
> +
> +
Re: [ovs-dev] [PATCH ovn 2/2] rhel: Add systemd-units for ovn-ic services
Bleep bloop. Greetings Numan Siddique, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 108 characters long (recommended limit is 79) #165 FILE: rhel/usr_lib_systemd_system_ovn-ic-db.service:10: # Environment="OVN_IC_DB_OPTS=--db-ic-nb-create-insecure-remote=yes --db-ic-sb-create-insecure-remote=yes" WARNING: Line is 92 characters long (recommended limit is 79) #167 FILE: rhel/usr_lib_systemd_system_ovn-ic-db.service:12: # Alternatively, you may specify environment variables in the file /etc/sysconfig/ovn-ic-db: WARNING: Line is 96 characters long (recommended limit is 79) #169 FILE: rhel/usr_lib_systemd_system_ovn-ic-db.service:14: # OVN_IC_DB_OPTS="--db-ic-nb-create-insecure-remote=yes --db-ic-sb-create-insecure-remote=yes" WARNING: Line is 143 characters long (recommended limit is 79) #203 FILE: rhel/usr_lib_systemd_system_ovn-ic.service:10: # Environment="OVN_IC_OPTS=--db-ic-nb-sock=/usr/local/var/run/ovn/ovn_ic_nb_db.sock --db-ic-sb-sock=/usr/local/var/run/ovn/ovn_ic_sb_db.sock" WARNING: Line is 89 characters long (recommended limit is 79) #205 FILE: rhel/usr_lib_systemd_system_ovn-ic.service:12: # Alternatively, you may specify environment variables in the file /etc/sysconfig/ovn-ic: WARNING: Line is 131 characters long (recommended limit is 79) #207 FILE: rhel/usr_lib_systemd_system_ovn-ic.service:14: # OVN_IC_OPTS="--db-ic-nb-sock=/usr/local/var/run/ovn/ovn_ic_nb_db.sock --db-ic-sb-sock=/usr/local/var/run/ovn/ovn_ic_sb_db.sock" Lines checked: 227, Warnings: 6, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 ovn] ovn-nbctl: add --bfd option to lr-route-add
On Sat, Jan 30, 2021 at 4:15 AM Lorenzo Bianconi wrote: > > Introduce the --bfd option to lr-route-add command. > If the BFD session UUID is provided, it will be used for the OVN route > otherwise the next-hop will be used to perform a lookup in the OVN BFD > table. > If the lookup fails and outport is specified, a new entry in the BFD table > will be created using the nexthop as dst_ip and outport as logical_port. > > Signed-off-by: Lorenzo Bianconi Thanks Lorenzo for v3. I applied this patch to master. Numan > --- > Changes since v2: > - move BFD entry allocation after possible error condition > - drop unnecessary xstrdup > > Changes since v1: > - introduce the capability to create the BFD entry if route outport has been > specified > --- > tests/ovn-northd.at | 17 ++ > tests/system-ovn.at | 5 ++-- > utilities/ovn-nbctl.8.xml | 11 + > utilities/ovn-nbctl.c | 49 ++- > 4 files changed, 73 insertions(+), 9 deletions(-) > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 8597ca1b9..c00225e99 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -2342,7 +2342,7 @@ AT_KEYWORDS([northd-bfd]) > ovn_start > > check ovn-nbctl --wait=sb lr-add r0 > -for i in $(seq 1 4); do > +for i in $(seq 1 5); do > check ovn-nbctl --wait=sb lrp-add r0 r0-sw$i 00:00:00:00:00:0$i > 192.168.$i.1/24 > check ovn-nbctl --wait=sb ls-add sw$i > check ovn-nbctl --wait=sb lsp-add sw$i sw$i-r0 > @@ -2387,17 +2387,24 @@ check_column 1000 bfd min_tx logical_port=r0-sw1 > check_column 1000 bfd min_rx logical_port=r0-sw1 > check_column 100 bfd detect_mult logical_port=r0-sw1 > > -check ovn-nbctl lr-route-add r0 100.0.0.0/8 192.168.10.2 > -route_uuid=$(fetch_column nb:logical_router_static_route _uuid > ip_prefix="100.0.0.0/8") > -check ovn-nbctl set logical_router_static_route $route_uuid bfd=$uuid > +check ovn-nbctl --bfd=$uuid lr-route-add r0 100.0.0.0/8 192.168.10.2 > check_column down bfd status logical_port=r0-sw1 > AT_CHECK([ovn-nbctl lr-route-list r0 | grep 192.168.10.2 | grep -q bfd],[0]) > > +check ovn-nbctl --bfd lr-route-add r0 200.0.0.0/8 192.168.20.2 > +check_column down bfd status logical_port=r0-sw2 > +AT_CHECK([ovn-nbctl lr-route-list r0 | grep 192.168.20.2 | grep -q bfd],[0]) > + > +check ovn-nbctl --bfd lr-route-add r0 240.0.0.0/8 192.168.50.2 r0-sw5 > +check_column down bfd status logical_port=r0-sw5 > +AT_CHECK([ovn-nbctl lr-route-list r0 | grep 192.168.50.2 | grep -q bfd],[0]) > + > +route_uuid=$(fetch_column nb:logical_router_static_route _uuid > ip_prefix="100.0.0.0/8") > check ovn-nbctl clear logical_router_static_route $route_uuid bfd > check_column admin_down bfd status logical_port=r0-sw1 > > ovn-nbctl destroy bfd $uuid > -check_row_count bfd 2 > +check_row_count bfd 3 > > AT_CLEANUP > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 4580c05e7..05ccd861a 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -5606,10 +5606,9 @@ NS_CHECK_EXEC([server], [bfdd-control allow > 172.16.1.1], [0], [dnl > Allowing connections from 172.16.1.1 > ]) > > -uuid=$(ovn-nbctl create bfd logical_port=rp-public dst_ip=172.16.1.50 > min_tx=250 min_rx=250 detect_mult=10) > -check ovn-nbctl lr-route-add R1 100.0.0.0/8 172.16.1.50 > +check ovn-nbctl --bfd lr-route-add R1 100.0.0.0/8 172.16.1.50 rp-public > +uuid=$(fetch_column nb:bfd _uuid logical_port="rp-public") > route_uuid=$(fetch_column nb:logical_router_static_route _uuid > ip_prefix="100.0.0.0/8") > -check ovn-nbctl set logical_router_static_route $route_uuid bfd=$uuid > check ovn-nbctl --wait=hv sync > > wait_column "up" nb:bfd status logical_port=rp-public > diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml > index 6ed8bcb75..2cab592ce 100644 > --- a/utilities/ovn-nbctl.8.xml > +++ b/utilities/ovn-nbctl.8.xml > @@ -659,6 +659,7 @@ > >[--may-exist] > [--policy=POLICY] > [--ecmp] [--ecmp-symmetric-reply] > +[--bfd[=UUID]] > lr-route-add router > prefix nexthop [port] > > @@ -695,6 +696,16 @@ >it is not necessary to set both. > > > + > + --bfd option is used to link a BFD session to the > + OVN route. If the BFD session UUID is provided, it will be used > + for the OVN route otherwise the next-hop will be used to perform > + a lookup in the OVN BFD table. > + If the lookup fails and port is specified, a new entry > + in the BFD table will be created using the nexthop as > + dst_ip and port as logical_port. > + > + > >It is an error if a route with prefix and >POLICY already exists, unless --may-exist, > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 1b7be1ef9..3f28ba11a 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -3960,6 +3960,29 @@
Re: [ovs-dev] [PATCH 0/2] Add offload support for ct_state rpl and inv flags
On Tue, 2 Feb 2021, Marcelo Leitner wrote:
> On Tue, Feb 02, 2021 at 04:59:44PM +0100, Ilya Maximets wrote:
> > On 2/2/21 4:52 PM, wenxu wrote:
> > >
> > > Hi,
> > >
> > > just ingore my patch. Now kernel can support match invalid
> > > ct_state in th tc flower.
> >
> > I don't think that we can ignore it. At least we need this
> > fix on stable branches. And also there are other conntrack
> > flags that are simply ignored, not only inv and rpl. These
> > are snat, dnat, rel, probably some other. So, we need this
> > change in some form anyway.
>
> That would be great, indeed.
>
> The issue is also present in tc layer, btw, but maybe Paul has his on
> the charts already. The effect here could be that if you use a newer
> ovs that supports inv ct_state, for example, up to wenxu's patch the
> kernel will simply ignore it.
>
> Best regards,
> Marcelo
Hi,
Regarding the inv flag, yes if you offload a +inv rule, and not have
an updated kernel then tc rule won't match it and it will fall back to
software. Drivers should see this flag and reject offload, same with rpl.
We can probe for support by adding a ct_state rule with +inv,+rpl and see
the echo back. Add that for V2?
As for missing other flags, I suggest to add a check that all flags were
parsed, the same as we do with
match's mask, set translated flags mask to 0, and then check if we
missed some new flag by checking mask the ct_state mask was zeroed.
Like this:
>From 48a4cb1b537d3837df8c40c9c265ae95e9729255 Mon Sep 17 00:00:00 2001
From: Paul Blakey
Date: Wed, 3 Feb 2021 10:27:21 +0200
Subject: [PATCH] netdev-offload-tc: Check for unparsed ct_state flags
Don't offload flows where we didn't fully parse ct_state flags.
Signed-off-by: Paul Blakey
---
lib/netdev-offload-tc.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 08a4735..4362c6e 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1705,7 +1705,11 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
*match,
flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_INVALID;
}
-mask->ct_state = 0;
+mask->ct_state &= ~(OVS_CS_F_INVALID |
+OVS_CS_F_REPLY_DIR |
+OVS_CS_F_TRACKED |
+OVS_CS_F_ESTABLISHED |
+OVS_CS_F_NEW);
}
if (mask->ct_zone) {
--
Does that sound good to you?
If so, as standalone patch, or in this patchset (will rebase the above as
first patch in this series)?
Thanks,
Paul.
>
> >
> > Best regards, Ilya Maximets.
> >
> > >
> > > BR
> > > wenxu
> > >
> > >
> > > From: Ilya Maximets
> > > Date: 2021-02-02 23:33:41
> > > To: Paul Blakey ,d...@openvswitch.org
> > > Cc: Oz Shlomo ,i.maxim...@ovn.org,Marcelo Leitner
> > > ,wenxu
> > > Subject: Re: [ovs-dev] [PATCH 0/2] Add offload support for ct_state rpl
> > > and inv flags>On 2/2/21 1:47 PM, Paul Blakey wrote:
> > >>> Add offload support for ct_state rpl and inv flags.
> > >>
> > >>Hi.
> > >>
> > >>Since you're working on this, could you, please, review the following
> > >>patch:
> > >>
> > >> http://patchwork.ozlabs.org/project/openvswitch/patch/1610344573-15780-1-git-send-email-we...@ucloud.cn/
> > >>
> > >>It seems like current OVS just ignores all the flags that can not
> > >>be passed to TC and this doesn't look right.
> > >>
> > >>>
> > >>> For example:
> > >>> ovs-ofctl del-flows br-ovs
> > >>> ovs-ofctl add-flow br-ovs arp,actions=normal
> > >>> ovs-ofctl add-flow br-ovs "table=0, ip,ct_state=-trk
> > >>> actions=ct(table=1,zone=5)"
> > >>> ovs-ofctl add-flow br-ovs "table=1, ip,ct_state=+trk+new
> > >>> actions=ct(zone=5, commit),normal"
> > >>> ovs-ofctl add-flow br-ovs "table=1, ip,ct_zone=5,ct_state=+trk+est+rpl
> > >>> actions=normal"
> > >>> ovs-ofctl add-flow br-ovs "table=1, ip,ct_zone=5,ct_state=+trk+est-rpl
> > >>> actions=normal"
> > >>>
> > >>> Paul Blakey (2):
> > >>> compat: Add TCA_FLOWER_KEY_CT_FLAGS_REPLY/INVALID definitions
> > >>> netdev-offload-tc: Add support for ct_state rpl and inv flags
> > >>>
> > >>> acinclude.m4| 6 +++---
> > >>> include/linux/pkt_cls.h | 4 +++-
> > >>> lib/netdev-offload-tc.c | 28
> > >>> 3 files changed, 34 insertions(+), 4 deletions(-)
> > >>>
> > >>
> > >
> > >
> >
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 2/2] rhel: Add systemd-units for ovn-ic services
From: Vladislav Odintsov
New ovn-ic.service and ovn-ic-db.service systemd units were added
to manage ovn-ic and ovsdb-servers for OVN IC respectively.
ovn-ic-db.service manages both OVN_IC_Northbound and OVN_IC_Southbound
OVSDB server processes.
Submitted-at: https://github.com/ovn-org/ovn/pull/65
Signed-off-by: Vladislav Odintsov
---
rhel/automake.mk | 2 +
rhel/ovn-fedora.spec.in | 63 ++-
rhel/usr_lib_systemd_system_ovn-ic-db.service | 32 ++
rhel/usr_lib_systemd_system_ovn-ic.service| 31 +
4 files changed, 126 insertions(+), 2 deletions(-)
create mode 100644 rhel/usr_lib_systemd_system_ovn-ic-db.service
create mode 100644 rhel/usr_lib_systemd_system_ovn-ic.service
diff --git a/rhel/automake.mk b/rhel/automake.mk
index 661975ea9..3e71f5d80 100644
--- a/rhel/automake.mk
+++ b/rhel/automake.mk
@@ -13,6 +13,8 @@ EXTRA_DIST += \
rhel/ovn-fedora.spec.in \
rhel/usr_lib_systemd_system_ovn-controller.service \
rhel/usr_lib_systemd_system_ovn-controller-vtep.service \
+ rhel/usr_lib_systemd_system_ovn-ic.service \
+ rhel/usr_lib_systemd_system_ovn-ic-db.service \
rhel/usr_lib_systemd_system_ovn-northd.service \
rhel/usr_lib_firewalld_services_ovn-central-firewall-service.xml \
rhel/usr_lib_firewalld_services_ovn-host-firewall-service.xml \
diff --git a/rhel/ovn-fedora.spec.in b/rhel/ovn-fedora.spec.in
index 6b11ef3e8..6716dd0d2 100644
--- a/rhel/ovn-fedora.spec.in
+++ b/rhel/ovn-fedora.spec.in
@@ -161,7 +161,7 @@ install -p -D -m 0644 \
rhel/usr_share_ovn_scripts_systemd_sysconfig.template \
$RPM_BUILD_ROOT/%{_sysconfdir}/sysconfig/ovn
-for service in ovn-controller ovn-controller-vtep ovn-northd; do
+for service in ovn-controller ovn-controller-vtep ovn-northd ovn-ic ovn-ic-db;
do
install -p -D -m 0644 \
rhel/usr_lib_systemd_system_${service}.service \
$RPM_BUILD_ROOT%{_unitdir}/${service}.service
@@ -256,7 +256,7 @@ if [ $1 -eq 1 ] ; then
if [[ "$?" = "0" && "$ovn_status" = "0" ]]; then
# ovn-controller-vtep service is running which means old
# openvswitch-ovn-vtep is installed and it will be cleaned up. So
-# start ovn-controller-vtep service when posttrans host is called.
+# start ovn-controller-vtep service when posttrans vtep is called.
touch %{_localstatedir}/lib/rpm-state/ovn-controller-vtep
fi
fi
@@ -272,6 +272,26 @@ fi
fi
%endif
+%if 0%{?systemd_preun:1}
+%systemd_preun ovn-ic.service
+%else
+if [ $1 -eq 0 ] ; then
+# Package removal, not upgrade
+/bin/systemctl --no-reload disable ovn-ic.service >/dev/null 2>&1 || :
+/bin/systemctl stop ovn-ic.service >/dev/null 2>&1 || :
+fi
+%endif
+
+%if 0%{?systemd_preun:1}
+%systemd_preun ovn-ic-db.service
+%else
+if [ $1 -eq 0 ] ; then
+# Package removal, not upgrade
+/bin/systemctl --no-reload disable ovn-ic-db.service >/dev/null 2>&1
|| :
+/bin/systemctl stop ovn-ic-db.service >/dev/null 2>&1 || :
+fi
+%endif
+
%preun host
%if 0%{?systemd_preun:1}
%systemd_preun ovn-controller.service
@@ -312,6 +332,24 @@ fi
fi
%endif
+%if 0%{?systemd_post:1}
+%systemd_post ovn-ic.service
+%else
+# Package install, not upgrade
+if [ $1 -eq 1 ]; then
+/bin/systemctl daemon-reload >dev/null || :
+fi
+%endif
+
+%if 0%{?systemd_post:1}
+%systemd_post ovn-ic-db.service
+%else
+# Package install, not upgrade
+if [ $1 -eq 1 ]; then
+/bin/systemctl daemon-reload >dev/null || :
+fi
+%endif
+
%post host
%if 0%{?systemd_post:1}
%systemd_post ovn-controller.service
@@ -345,6 +383,22 @@ fi
fi
%endif
+%if 0%{?systemd_postun_with_restart:1}
+%systemd_postun_with_restart ovn-ic.service
+%else
+/bin/systemctl daemon-reload >/dev/null 2>&1 || :
+if [ "$1" -ge "1" ] ; then
+# Package upgrade, not uninstall
+/bin/systemctl try-restart ovn-ic.service >/dev/null 2>&1 || :
+fi
+%endif
+
+%if 0%{?systemd_postun:1}
+%systemd_postun ovn-ic-db.service
+%else
+/bin/systemctl daemon-reload >/dev/null 2>&1 || :
+%endif
+
%postun host
%if 0%{?systemd_postun_with_restart:1}
%systemd_postun_with_restart ovn-controller.service
@@ -439,6 +493,8 @@ fi
%config %{_datadir}/ovn/ovn-sb.ovsschema
%config %{_datadir}/ovn/ovn-ic-nb.ovsschema
%config %{_datadir}/ovn/ovn-ic-sb.ovsschema
+%{_unitdir}/ovn-ic.service
+%{_unitdir}/ovn-ic-db.service
%{_unitdir}/ovn-northd.service
%{_prefix}/lib/firewalld/services/ovn-central-firewall-service.xml
@@ -454,5 +510,8 @@ fi
%{_unitdir}/ovn-controller-vtep.service
%changelog
+* Mon Feb 1 2021 Vladislav Odintsov
+- Added ovn-ic, ovn-ic-db systemd-units.
+
* Thu Dec 20 2018 Numan Siddique
- OVS/OVN split.
diff --git a/rhel/usr_lib_systemd_system_ovn-ic-db.service
[ovs-dev] [PATCH ovn 1/2] Revert "rhel: Add systemd-unit for ovn-ic and move IC to sub-rpm ovn-ic."
From: Vladislav Odintsov
This reverts commit 8abe98f386ff2b4a1f2f7b61188a377a5d020429.
Submitted-at: https://github.com/ovn-org/ovn/pull/65
Signed-off-by: Vladislav Odintsov
---
rhel/automake.mk | 1 -
rhel/ovn-fedora.spec.in| 58 +++---
rhel/usr_lib_systemd_system_ovn-ic.service | 32
3 files changed, 6 insertions(+), 85 deletions(-)
delete mode 100644 rhel/usr_lib_systemd_system_ovn-ic.service
diff --git a/rhel/automake.mk b/rhel/automake.mk
index 0e8795feb..661975ea9 100644
--- a/rhel/automake.mk
+++ b/rhel/automake.mk
@@ -13,7 +13,6 @@ EXTRA_DIST += \
rhel/ovn-fedora.spec.in \
rhel/usr_lib_systemd_system_ovn-controller.service \
rhel/usr_lib_systemd_system_ovn-controller-vtep.service \
- rhel/usr_lib_systemd_system_ovn-ic.service \
rhel/usr_lib_systemd_system_ovn-northd.service \
rhel/usr_lib_firewalld_services_ovn-central-firewall-service.xml \
rhel/usr_lib_firewalld_services_ovn-host-firewall-service.xml \
diff --git a/rhel/ovn-fedora.spec.in b/rhel/ovn-fedora.spec.in
index 6938deee6..6b11ef3e8 100644
--- a/rhel/ovn-fedora.spec.in
+++ b/rhel/ovn-fedora.spec.in
@@ -84,14 +84,6 @@ Provides: openvswitch-ovn-central =
%{?epoch:%{epoch}:}%{version}-%{release}
%description central
OVN DB servers and ovn-northd running on a central node.
-%package ic
-Summary: Open Virtual Network interconnection support
-License: ASL 2.0
-Requires: ovn
-
-%description ic
-OVN IC DB servers and ovn-ic.
-
%package host
Summary: Open Virtual Network support
License: ASL 2.0
@@ -169,7 +161,7 @@ install -p -D -m 0644 \
rhel/usr_share_ovn_scripts_systemd_sysconfig.template \
$RPM_BUILD_ROOT/%{_sysconfdir}/sysconfig/ovn
-for service in ovn-controller ovn-controller-vtep ovn-northd ovn-ic; do
+for service in ovn-controller ovn-controller-vtep ovn-northd; do
install -p -D -m 0644 \
rhel/usr_lib_systemd_system_${service}.service \
$RPM_BUILD_ROOT%{_unitdir}/${service}.service
@@ -264,7 +256,7 @@ if [ $1 -eq 1 ] ; then
if [[ "$?" = "0" && "$ovn_status" = "0" ]]; then
# ovn-controller-vtep service is running which means old
# openvswitch-ovn-vtep is installed and it will be cleaned up. So
-# start ovn-controller-vtep service when posttrans vtep is called.
+# start ovn-controller-vtep service when posttrans host is called.
touch %{_localstatedir}/lib/rpm-state/ovn-controller-vtep
fi
fi
@@ -280,17 +272,6 @@ fi
fi
%endif
-%preun ic
-%if 0%{?systemd_preun:1}
-%systemd_preun ovn-ic.service
-%else
-if [ $1 -eq 0 ] ; then
-# Package removal, not upgrade
-/bin/systemctl --no-reload disable ovn-ic.service >/dev/null 2>&1 || :
-/bin/systemctl stop ovn-ic.service >/dev/null 2>&1 || :
-fi
-%endif
-
%preun host
%if 0%{?systemd_preun:1}
%systemd_preun ovn-controller.service
@@ -331,16 +312,6 @@ fi
fi
%endif
-%post ic
-%if 0%{?systemd_post:1}
-%systemd_post ovn-ic.service
-%else
-# Package install, not upgrade
-if [ $1 -eq 1 ]; then
-/bin/systemctl daemon-reload >dev/null || :
-fi
-%endif
-
%post host
%if 0%{?systemd_post:1}
%systemd_post ovn-controller.service
@@ -374,17 +345,6 @@ fi
fi
%endif
-%postun ic
-%if 0%{?systemd_postun_with_restart:1}
-%systemd_postun_with_restart ovn-ic.service
-%else
-/bin/systemctl daemon-reload >/dev/null 2>&1 || :
-if [ "$1" -ge "1" ] ; then
-# Package upgrade, not uninstall
-/bin/systemctl try-restart ovn-ic.service >/dev/null 2>&1 || :
-fi
-%endif
-
%postun host
%if 0%{?systemd_postun_with_restart:1}
%systemd_postun_with_restart ovn-controller.service
@@ -461,6 +421,7 @@ fi
%{_mandir}/man5/ovn-sb.5*
%{_mandir}/man8/ovn-ic-nbctl.8*
%{_mandir}/man8/ovn-ic-sbctl.8*
+%{_mandir}/man8/ovn-ic.8*
%{_mandir}/man5/ovn-ic-nb.5*
%{_mandir}/man5/ovn-ic-sb.5*
%{_prefix}/lib/ocf/resource.d/ovn/ovndb-servers
@@ -472,18 +433,14 @@ fi
%files central
%{_bindir}/ovn-northd
+%{_bindir}/ovn-ic
%{_mandir}/man8/ovn-northd.8*
%config %{_datadir}/ovn/ovn-nb.ovsschema
%config %{_datadir}/ovn/ovn-sb.ovsschema
-%{_unitdir}/ovn-northd.service
-%{_prefix}/lib/firewalld/services/ovn-central-firewall-service.xml
-
-%files ic
-%{_bindir}/ovn-ic
-%{_mandir}/man8/ovn-ic.8*
%config %{_datadir}/ovn/ovn-ic-nb.ovsschema
%config %{_datadir}/ovn/ovn-ic-sb.ovsschema
-%{_unitdir}/ovn-ic.service
+%{_unitdir}/ovn-northd.service
+%{_prefix}/lib/firewalld/services/ovn-central-firewall-service.xml
%files host
%{_bindir}/ovn-controller
@@ -497,8 +454,5 @@ fi
%{_unitdir}/ovn-controller-vtep.service
%changelog
-* Wed Dec 9 2020 Vladislav Odintsov
-- Added ovn-ic systemd-unit and subpackage.
-
* Thu Dec 20 2018 Numan Siddique
- OVS/OVN split.
diff --git a/rhel/usr_lib_systemd_system_ovn-ic.service
