Re: [ovs-dev] [ovs-dev v5] dpif-netdev: fix dpif_netdev_flow_put
Thanks！
Ilya Maximets 于2023年8月15日 周二02:02写道：
> On 8/14/23 04:37, Peng He wrote:
> > OVS allows overlapping megaflows, as long as the actions of these
> > megaflows are equal. However, the current implementation of action
> > modification relies on flow_lookup instead of ufid, this could result
> > in looking up a wrong megaflow and make the ukeys and megaflows
> inconsistent
> >
> > Just like the test case in the patch, at first we have a rule with the
> > prefix:
> >
> > 10.1.2.0/24
> >
> > and we will get a megaflow with prefixes 10.1.2.2/24 when a packet with
> IP
> > 10.1.2.2 is received.
> >
> > Then suppose we change the rule into 10.1.0.0/16. OVS prefers to keep
> the
> > 10.1.2.2/24 megaflow and just changes its action instead of extending
> > the prefix into 10.1.2.2/16.
> >
> > then suppose we have a 10.1.0.2 packet, since it misses the megaflow,
> > this time, we will have an overlapping megaflow with the right prefix:
> > 10.1.0.2/16
> >
> > now we have two megaflows:
> > 10.1.2.2/24
> > 10.1.0.2/16
> >
> > last, suppose we have changed the ruleset again. The revalidator this
> > time still decides to change the actions of both megaflows instead of
> > deleting them.
> >
> > The dpif_netdev_flow_put will search the megaflow to modify with unmasked
> > keys, however it might lookup the wrong megaflow as the key 10.1.2.2
> matches
> > both 10.1.2.2/24 and 10.1.0.2/16!
> >
> > This patch changes the megaflow lookup code in modification path into
> > relying the ufid to find the correct megaflow instead of key lookup.
> >
> > Fixes: beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline")
> > Signed-off-by: Peng He
> > ---
> > lib/dpif-netdev.c | 45 ++---
> > tests/pmd.at | 47 +++
> > 2 files changed, 77 insertions(+), 15 deletions(-)
>
> Thanks! Applied and backported down to 2.17.
>
> Best regards, Ilya Maximets.
>
--
hepeng
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next v3 03/10] genetlink: remove userhdr from struct genl_info
Only three families use info->userhdr today and going forward
we discourage using fixed headers in new families.
So having the pointer to user header in struct genl_info
is an overkill. Compute the header pointer at runtime.
Reviewed-by: Johannes Berg
Reviewed-by: Jiri Pirko
Signed-off-by: Jakub Kicinski
---
CC: philipp.reis...@linbit.com
CC: lars.ellenb...@linbit.com
CC: christoph.boehmwal...@linbit.com
CC: ax...@kernel.dk
CC: pshe...@ovn.org
CC: jma...@redhat.com
CC: ying@windriver.com
CC: jacob.e.kel...@intel.com
CC: drbd-...@lists.linbit.com
CC: linux-bl...@vger.kernel.org
CC: d...@openvswitch.org
CC: tipc-discuss...@lists.sourceforge.net
---
drivers/block/drbd/drbd_nl.c | 9 +
include/net/genetlink.h | 7 +--
net/netlink/genetlink.c | 1 -
net/openvswitch/conntrack.c | 2 +-
net/openvswitch/datapath.c | 29 -
net/openvswitch/meter.c | 10 +-
net/tipc/netlink_compat.c| 2 +-
7 files changed, 33 insertions(+), 27 deletions(-)
diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index cddae6f4b00f..d3538bd83fb3 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -159,7 +159,7 @@ static int drbd_msg_sprintf_info(struct sk_buff *skb, const
char *fmt, ...)
static int drbd_adm_prepare(struct drbd_config_context *adm_ctx,
struct sk_buff *skb, struct genl_info *info, unsigned flags)
{
- struct drbd_genlmsghdr *d_in = info->userhdr;
+ struct drbd_genlmsghdr *d_in = genl_info_userhdr(info);
const u8 cmd = info->genlhdr->cmd;
int err;
@@ -1396,8 +1396,9 @@ static void drbd_suspend_al(struct drbd_device *device)
static bool should_set_defaults(struct genl_info *info)
{
- unsigned flags = ((struct drbd_genlmsghdr*)info->userhdr)->flags;
- return 0 != (flags & DRBD_GENL_F_SET_DEFAULTS);
+ struct drbd_genlmsghdr *dh = genl_info_userhdr(info);
+
+ return 0 != (dh->flags & DRBD_GENL_F_SET_DEFAULTS);
}
static unsigned int drbd_al_extents_max(struct drbd_backing_dev *bdev)
@@ -4276,7 +4277,7 @@ static void device_to_info(struct device_info *info,
int drbd_adm_new_minor(struct sk_buff *skb, struct genl_info *info)
{
struct drbd_config_context adm_ctx;
- struct drbd_genlmsghdr *dh = info->userhdr;
+ struct drbd_genlmsghdr *dh = genl_info_userhdr(info);
enum drbd_ret_code retcode;
retcode = drbd_adm_prepare(_ctx, skb, info, DRBD_ADM_NEED_RESOURCE);
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 0366d0925596..9dc21ec15734 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -95,7 +95,6 @@ struct genl_family {
* @snd_portid: netlink portid of sender
* @nlhdr: netlink message header
* @genlhdr: generic netlink message header
- * @userhdr: user specific header
* @attrs: netlink attributes
* @_net: network namespace
* @user_ptr: user pointers
@@ -106,7 +105,6 @@ struct genl_info {
u32 snd_portid;
const struct nlmsghdr * nlhdr;
struct genlmsghdr * genlhdr;
- void * userhdr;
struct nlattr **attrs;
possible_net_t _net;
void * user_ptr[2];
@@ -123,6 +121,11 @@ static inline void genl_info_net_set(struct genl_info
*info, struct net *net)
write_pnet(>_net, net);
}
+static inline void *genl_info_userhdr(const struct genl_info *info)
+{
+ return (u8 *)info->genlhdr + GENL_HDRLEN;
+}
+
#define GENL_SET_ERR_MSG(info, msg) NL_SET_ERR_MSG((info)->extack, msg)
#define GENL_SET_ERR_MSG_FMT(info, msg, args...) \
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 0d4285688ab9..f98f730bb245 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -943,7 +943,6 @@ static int genl_family_rcv_msg_doit(const struct
genl_family *family,
info.snd_portid = NETLINK_CB(skb).portid;
info.nlhdr = nlh;
info.genlhdr = nlmsg_data(nlh);
- info.userhdr = nlmsg_data(nlh) + GENL_HDRLEN;
info.attrs = attrbuf;
info.extack = extack;
genl_info_net_set(, net);
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 0cfa1e9482e6..0b9a785dea45 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1605,7 +1605,7 @@ static struct sk_buff *
ovs_ct_limit_cmd_reply_start(struct genl_info *info, u8 cmd,
struct ovs_header **ovs_reply_header)
{
- struct ovs_header *ovs_header = info->userhdr;
+ struct ovs_header *ovs_header = genl_info_userhdr(info);
struct sk_buff *skb;
skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index d33cb739883f..0a974eeef76e 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -590,7 +590,7 @@ static int
[ovs-dev] [PATCH net] net: openvswitch: reject negative ifindex
Recent changes in net-next (commit 759ab1edb56c ("net: store netdevs
in an xarray")) refactored the handling of pre-assigned ifindexes
and let syzbot surface a latent problem in ovs. ovs does not validate
ifindex, making it possible to create netdev ports with negative
ifindex values. It's easy to repro with YNL:
$ ./cli.py --spec netlink/specs/ovs_datapath.yaml \
--do new \
--json '{"upcall-pid": 1, "name":"my-dp"}'
$ ./cli.py --spec netlink/specs/ovs_vport.yaml \
--do new \
--json '{"upcall-pid": "0001", "name": "some-port0",
"dp-ifindex":3,"ifindex":4294901760,"type":2}'
$ ip link show
-65536: some-port0: mtu 1500 qdisc noop state DOWN mode
DEFAULT group default qlen 1000
link/ether 7a:48:21:ad:0b:fb brd ff:ff:ff:ff:ff:ff
...
Validate the inputes. Now the second command correctly returns:
$ ./cli.py --spec netlink/specs/ovs_vport.yaml \
--do new \
--json '{"upcall-pid": "0001", "name": "some-port0",
"dp-ifindex":3,"ifindex":4294901760,"type":2}'
lib.ynl.NlError: Netlink error: Numerical result out of range
nl_len = 108 (92) nl_flags = 0x300 nl_type = 2
error: -34 extack: {'msg': 'integer out of range', 'unknown':
[[type:4 len:36]
b'\x0c\x00\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x0c\x00\x03\x00\xff\xff\xff\x7f\x00\x00\x00\x00\x08\x00\x01\x00\x08\x00\x00\x00'],
'bad-attr': '.ifindex'}
Accept 0 since it used to be silently ignored.
Fixes: 54c4ef34c4b6 ("openvswitch: allow specifying ifindex of new interfaces")
Reported-by: syzbot+7456b5dcf65111553...@syzkaller.appspotmail.com
Signed-off-by: Jakub Kicinski
---
CC: pshe...@ovn.org
CC: andrey.zhadche...@virtuozzo.com
CC: brau...@kernel.org
CC: d...@openvswitch.org
---
net/openvswitch/datapath.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index a6d2a0b1aa21..3d7a91e64c88 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1829,7 +1829,7 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct
genl_info *info)
parms.port_no = OVSP_LOCAL;
parms.upcall_portids = a[OVS_DP_ATTR_UPCALL_PID];
parms.desired_ifindex = a[OVS_DP_ATTR_IFINDEX]
- ? nla_get_u32(a[OVS_DP_ATTR_IFINDEX]) : 0;
+ ? nla_get_s32(a[OVS_DP_ATTR_IFINDEX]) : 0;
/* So far only local changes have been made, now need the lock. */
ovs_lock();
@@ -2049,7 +2049,7 @@ static const struct nla_policy
datapath_policy[OVS_DP_ATTR_MAX + 1] = {
[OVS_DP_ATTR_USER_FEATURES] = { .type = NLA_U32 },
[OVS_DP_ATTR_MASKS_CACHE_SIZE] = NLA_POLICY_RANGE(NLA_U32, 0,
PCPU_MIN_UNIT_SIZE / sizeof(struct mask_cache_entry)),
- [OVS_DP_ATTR_IFINDEX] = {.type = NLA_U32 },
+ [OVS_DP_ATTR_IFINDEX] = NLA_POLICY_MIN(NLA_S32, 0),
};
static const struct genl_small_ops dp_datapath_genl_ops[] = {
@@ -2302,7 +2302,7 @@ static int ovs_vport_cmd_new(struct sk_buff *skb, struct
genl_info *info)
parms.port_no = port_no;
parms.upcall_portids = a[OVS_VPORT_ATTR_UPCALL_PID];
parms.desired_ifindex = a[OVS_VPORT_ATTR_IFINDEX]
- ? nla_get_u32(a[OVS_VPORT_ATTR_IFINDEX]) : 0;
+ ? nla_get_s32(a[OVS_VPORT_ATTR_IFINDEX]) : 0;
vport = new_vport();
err = PTR_ERR(vport);
@@ -2539,7 +2539,7 @@ static const struct nla_policy
vport_policy[OVS_VPORT_ATTR_MAX + 1] = {
[OVS_VPORT_ATTR_TYPE] = { .type = NLA_U32 },
[OVS_VPORT_ATTR_UPCALL_PID] = { .type = NLA_UNSPEC },
[OVS_VPORT_ATTR_OPTIONS] = { .type = NLA_NESTED },
- [OVS_VPORT_ATTR_IFINDEX] = { .type = NLA_U32 },
+ [OVS_VPORT_ATTR_IFINDEX] = NLA_POLICY_MIN(NLA_S32, 0),
[OVS_VPORT_ATTR_NETNSID] = { .type = NLA_S32 },
[OVS_VPORT_ATTR_UPCALL_STATS] = { .type = NLA_NESTED },
};
--
2.41.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] NEWS: Add note about L4_SYM being used by default for ECMP.
On 8/14/23 21:01, Mark Michelson wrote:
> On 8/14/23 12:25, Han Zhou wrote:
>> On Mon, Aug 14, 2023 at 6:54 AM Dumitru Ceara wrote:
>>>
>>> Fixes: 596ea7acbe68 ("ovn-controller: Detect and use L4_SYM dp-hash if
>> available.")
>>> Signed-off-by: Dumitru Ceara
>>> ---
>>> NEWS | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/NEWS b/NEWS
>>> index 8275877f99..f99334c1b8 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -10,6 +10,9 @@ Post v23.06.0
>>> - To allow optimizing ovn-controller's monitor conditions for the
>> regular
>>> VIF case, ovn-controller now unconditionally monitors all
>>> sub-ports
>>> (ports with parent_port set).
>>> + - ECMP routes use L4_SYM dp-hash by default if the datapath supports
>> it.
>>> + Existing sessions might get re-hashed to a different ECMP path when
>>> + OVN detects the algorithm support in the datapath.
>>
>> nit: probably add "during the upgrade" to avoid misunderstanding.
>>
>> Acked-by: Han Zhou
>
> I updated the sentence to be "Existing sessions might get re-hashed to a
> different ECMP path when OVN detects the algorithm support in the
> datapath during an upgrade or restart of ovn-controller."
>
> I pushed the change to main. Thanks Dumitru and Han.
>
Thanks, Han, Mark!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] binding: handle ovs ofport update
On 8/9/23 20:01, Mark Michelson wrote:
> Thanks Mohammad and Ales. I have pushed the change to main and all
> branches back to 22.03.
>
Hi Mark, Mohammad,
It seems this change broke 22.03:
https://github.com/ovn-org/ovn/actions/runs/5812479357
> On 8/3/23 04:46, Ales Musil wrote:
>> On Mon, Jul 31, 2023 at 7:18 PM Mohammad Heib wrote:
>>
>>> Currently when ovs interface ofport is updated
>>> after setting external_ids:iface_id, the ovn-controller
>>> will see this change but will not do much if it handles
>>> this change incrementally.
>>>
>>> This behavior leads to a mismatch between the ovs openflow
>>> flows in table=0 (inaccurate in_port) and the ofport number
>>> that the packet was received at which will lead to packets
>>> drop in table=0.
>>>
>>> This patch will resolve the above issue by triggering
>>> flows recompute during the I-P processing only if the
>>> affected port are associated with lport and has flows
>>> that need to be updated.
>>>
>>> Reported-at: https://issues.redhat.com/browse/FD-3063
>>> Signed-off-by: Mohammad Heib
>>> ---
>>> controller/binding.c | 15 ++
>>> tests/ovn-controller.at | 45 +
>>> 2 files changed, 60 insertions(+)
>>>
>>> diff --git a/controller/binding.c b/controller/binding.c
>>> index 9aa3fc6c4..cc4c2b0bb 100644
>>> --- a/controller/binding.c
>>> +++ b/controller/binding.c
>>> @@ -2360,6 +2360,21 @@ consider_iface_claim(const struct
>>> ovsrec_interface
>>> *iface_rec,
>>> /* Get the (updated) b_lport again for the lbinding. */
>>> b_lport = local_binding_get_primary_lport(lbinding);
>>>
>>> + /*
>>> + * Update the tracked_dp_bindings whenever an ofport
>>> + * on a specific ovs port changes.
>>> + * This update will trigger flow recomputation during
>>> + * the incremental processing run which updates the local
>>> + * flows in_port filed.
>>> + */
>>> + if (b_lport && ovsrec_interface_is_updated(iface_rec,
>>> + OVSREC_INTERFACE_COL_OFPORT)) {
>>> + tracked_datapath_lport_add(b_lport->pb,
>>> TRACKED_RESOURCE_UPDATED,
This part triggers the ct_zones_runtime_data_handler() I-P handler to be
called. But on 22.03 this aborts on TRACKED_RESOURCE_UPDATED:
https://github.com/ovn-org/ovn/blob/915c556f0c50e81b1dbc05a68d754cf4cb7d4551/controller/ovn-controller.c#L2154
Do you maybe have some time to look into this? Or should we just revert
the backport on 22.03? I don't think ofport updates are common.
Thanks,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 4/4] tests: Add missing sync calls
Hi Ales, I have a couple of notes below. On 8/14/23 05:21, Ales Musil wrote: Add various missing sync calls which caused the tests to be flaky due to sometimes missed on various checks or packets. Signed-off-by: Ales Musil --- tests/ovn-controller.at | 10 +- tests/ovn-ic.at | 7 --- tests/ovn-northd.at | 14 +- tests/ovn.at| 20 ++-- tests/system-ovn.at | 5 +++-- 5 files changed, 35 insertions(+), 21 deletions(-) diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index f2216d245..6a4bcedab 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -433,10 +433,10 @@ check ovn-nbctl --wait=hv sync check ovn-nbctl --wait=hv sync # nb_cfg should be set to 1 in the chassis_private/nb_global/sb_global table -check_column 1 chassis_private nb_cfg -check_column 1 sb_global nb_cfg -check_column 1 nb:nb_global nb_cfg -check_column 0 chassis nb_cfg +wait_row_count nb:nb_global 1 nb_cfg=1 +wait_row_count chassis_private 1 nb_cfg=1 +wait_row_count sb_global 1 nb_cfg=1 +wait_row_count chassis 1 nb_cfg=0 Why is this change necessary? We can see that there is an `ovn-nbctl --wait=hv sync` call above. Shouldn't we be able to check the values directly instead of having to wait? OVN_CLEANUP([hv]) AT_CLEANUP @@ -562,7 +562,7 @@ primary lport : [[lsp1]] ]) # Set the port type to localport -check ovn-nbctl lsp-set-type lsp1 localport +check ovn-nbctl --wait=hv lsp-set-type lsp1 localport check as hv1 ovs-vsctl set open . external_ids:ovn-cms-options=localport OVS_WAIT_UNTIL([test localport = $(ovn-sbctl get chassis . other_config:ovn-cms-options)]) diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at index 09eac860c..a654e59fe 100644 --- a/tests/ovn-ic.at +++ b/tests/ovn-ic.at @@ -543,6 +543,7 @@ done # Create directly-connected routes ovn_as az2 ovn-nbctl lrp-add lr12 lrp-lr12-ls2 aa:aa:aa:aa:bb:01 "192.168.0.1/24" ovn_as az2 ovn-nbctl lr-route-add lr12 10.10.10.0/24 192.168.0.10 +ovn_as az1 ovn-nbctl --wait=sb sync echo az1 ovn_as az1 ovn-nbctl show @@ -951,7 +952,7 @@ ovn_as az2 ovn-nbctl --route-table=rtb2 lr-route-add lr12 10.10.10.0/24 192.168. ovn_as az2 ovn-nbctl --route-table=rtb3 lr-route-add lr12 10.10.10.0/24 192.168.0.12 # Create directly-connected route in VPC2 -ovn_as az2 ovn-nbctl lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bb:01 "192.168.0.1/24" +ovn_as az2 ovn-nbctl --wait=sb lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bb:01 "192.168.0.1/24" # Test direct routes from lr12 were learned to lr11 AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 192.168 | @@ -1077,7 +1078,7 @@ ovn_as az2 ovn-nbctl --route-table=rtb2 lr-route-add lr12 2001:db8:::/64 200 ovn_as az2 ovn-nbctl --route-table=rtb3 lr-route-add lr12 2001:db8:::/64 2001:db8:200::12 # Create directly-connected route in VPC2 -ovn_as az2 ovn-nbctl lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bb:01 "2001:db8:200::1/64" +ovn_as az2 ovn-nbctl --wait=sb lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bb:01 "2001:db8:200::1/64" # Test direct routes from lr12 were learned to lr11 AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 2001:db8:200 | @@ -1146,7 +1147,7 @@ for i in 1 2; do ovn-nbctl lrp-add $lr lrp-local-subnet 00:00:00:00:00:0$i 192.168.$i.1/24 ovn-nbctl list logical-router-static-route check ovn-nbctl lr-route-add $lr 10.0.0.0/24 192.168.$i.10 -check ovn-nbctl lr-route-add $lr 0.0.0.0/0 192.168.$i.11 +check ovn-nbctl --wait=sb lr-route-add $lr 0.0.0.0/0 192.168.$i.11 done AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep dst-ip | sort], [0], [dnl diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index d5be3be75..d1ea892ec 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -1156,7 +1156,7 @@ ovn-nbctl --stateless lr-nat-add DR dnat_and_snat 172.16.1.2 50.0.0.11 ovn-nbctl --stateless lr-nat-add CR dnat_and_snat 172.16.1.2 50.0.0.11 ovn-nbctl lr-nat-update-ext-ip DR dnat_and_snat 172.16.1.2 allowed_range -ovn-nbctl lr-nat-update-ext-ip CR dnat_and_snat 172.16.1.2 allowed_range +check ovn-nbctl --wait=sb lr-nat-update-ext-ip CR dnat_and_snat 172.16.1.2 allowed_range ovn-sbctl dump-flows DR > drflows5 AT_CAPTURE_FILE([drflows2]) @@ -4815,7 +4815,7 @@ AS_BOX([Checking that routable NAT flows are installed when gateway chassis exis check ovn-nbctl lr-nat-del ro1 check ovn-nbctl lr-nat-del ro2 check ovn-nbctl --add-route lr-nat-add ro1 dnat 10.0.0.100 192.168.1.100 -check ovn-nbctl --add-route lr-nat-add ro2 dnat 20.0.0.100 192.168.2.100 +check ovn-nbctl --wait=sb --add-route lr-nat-add ro2 dnat 20.0.0.100 192.168.2.100 check_lflows 1 @@ -4846,7 +4846,7 @@ check ovn-nbctl lr-nat-del ro1 check ovn-nbctl lr-nat-del ro2 check ovn-nbctl lr-nat-add ro1 dnat_and_snat 10.0.0.100 192.168.1.2 vm1 00:00:00:00:00:01 -check ovn-nbctl lr-nat-add ro2 dnat_and_snat 20.0.0.100 192.168.2.2 vm2
Re: [ovs-dev] [PATCH ovn 3/4] tests: Make sure the port group is not hardcoded
Acked-by: Mark Michelson
On 8/14/23 05:21, Ales Musil wrote:
The port group name consists of DP key and NB PG name.
Use first PG that is avaiable to avoid flakes when
neither of the logical switches has DP key 2.
Signed-off-by: Ales Musil
---
tests/ovn.at | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/ovn.at b/tests/ovn.at
index 9bb4b7287..56ac17261 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -32487,9 +32487,9 @@ AT_CHECK([test $(ovs-ofctl dump-flows br-int table=46 |
grep conjunction | wc -l
for i in $(seq 1 10); do
# Delete and recreate the SB PG with same name and content.
-sb_pg_name=2_pg1 # dp key + NB pg name
+sb_pg_name=$(fetch_column port_group name | cut -d ' ' -f1)
sb_pg_uuid=$(fetch_column port_group _uuid name=$sb_pg_name)
-ports_=$(fetch_column port_group ports name=2_pg1)
+ports_=$(fetch_column port_group ports name=$sb_pg_name)
ports=${ports_/ /,}
AT_CHECK([ovn-sbctl destroy port_group $sb_pg_uuid -- create port_group
name=$sb_pg_name ports=$ports], [0], [ignore])
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 1/4] tests: Check proper DP and port key
Thanks Ales,
Acked-by: Mark Michelson
On 8/14/23 05:21, Ales Musil wrote:
The test was assuming that the DP key and Port keys are
always fixed. Make sure that we use proper values for the
flow check.
Signed-off-by: Ales Musil
---
tests/ovn.at | 67 +++-
1 file changed, 35 insertions(+), 32 deletions(-)
diff --git a/tests/ovn.at b/tests/ovn.at
index 94f04d011..9bb4b7287 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -21871,8 +21871,8 @@ eth_dst=ff01
ip_src=$(ip_to_hex 10 0 0 10)
ip_dst=$(ip_to_hex 172 168 0 101)
send_icmp_packet 1 1 $eth_src $eth_dst $ip_src $ip_dst c4c9
00
-AT_CHECK([as hv1 ovs-ofctl dump-flows br-int metadata=0x$lr0_dp_key | awk '/table=28,
n_packets=1, n_bytes=45/{print $7" "$8}'],[0],[dnl
-priority=80,ip,reg15=0x3,metadata=0x3,nw_src=10.0.0.10 actions=drop
+AT_CHECK_UNQUOTED([as hv1 ovs-ofctl dump-flows br-int metadata=0x$lr0_dp_key | awk
'/table=28, n_packets=1, n_bytes=45/{print $7" "$8}'],[0],[dnl
+priority=80,ip,reg15=0x$lr0_public_dp_key,metadata=0x$lr0_dp_key,nw_src=10.0.0.10
actions=drop
])
# hv1 should remove the flow for the ACL with is_chassis_redirect check for sw0-vir.
@@ -31432,6 +31432,9 @@ sw0_dpkey=$(fetch_column datapath_binding tunnel_key
external_ids:name=sw0)
sw0p1_dpkey=$(fetch_column port_binding tunnel_key logical_port=sw0-p1)
sw0p3_dpkey=$(fetch_column port_binding tunnel_key logical_port=sw0-p3)
+dp_key=$(printf "%x" $sw0_dpkey)
+port_key=$(printf "%x" $sw0p1_dpkey)
+
check_column '50:54:00:00:00:13' fdb mac
check_column $sw0_dpkey fdb dp_key
check_column $sw0p1_dpkey fdb port_key
@@ -31443,12 +31446,12 @@ as hv2 ovs-ofctl dump-flows br-int table=71 >
hv2_offlows_table71.txt
AT_CAPTURE_FILE([hv1_offlows_table71.txt])
AT_CAPTURE_FILE([hv2_offlows_table71.txt])
-AT_CHECK([cat hv1_offlows_table71.txt | grep -v NXST | cut -d ' ' -f8- |
sort], [0], [dnl
-priority=100,metadata=0x1,dl_dst=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG15[[]]
+AT_CHECK_UNQUOTED([cat hv1_offlows_table71.txt | grep -v NXST | cut -d ' '
-f8- | sort], [0], [dnl
+priority=100,metadata=0x$dp_key,dl_dst=50:54:00:00:00:13
actions=load:0x$port_key->NXM_NX_REG15[[]]
])
-AT_CHECK([cat hv2_offlows_table71.txt | grep -v NXST | cut -d ' ' -f8- | sort], [0], [dnl
-priority=100,metadata=0x1,dl_dst=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG15[[]]
+AT_CHECK_UNQUOTED([cat hv2_offlows_table71.txt | grep -v NXST | cut -d ' '
-f8- | sort], [0], [dnl
+priority=100,metadata=0x$dp_key,dl_dst=50:54:00:00:00:13
actions=load:0x$port_key->NXM_NX_REG15[[]]
])
as hv1 ovs-ofctl dump-flows br-int table=72 > hv1_offlows_table72.txt
@@ -31456,12 +31459,12 @@ as hv2 ovs-ofctl dump-flows br-int table=72 >
hv2_offlows_table72.txt
AT_CAPTURE_FILE([hv1_offlows_table72.txt])
AT_CAPTURE_FILE([hv2_offlows_table72.txt])
-AT_CHECK([cat hv1_offlows_table72.txt | grep -v NXST | cut -d ' ' -f8- |
sort], [0], [dnl
-priority=100,reg14=0x1,metadata=0x1,dl_src=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG10[[8]]
+AT_CHECK_UNQUOTED([cat hv1_offlows_table72.txt | grep -v NXST | cut -d ' '
-f8- | sort], [0], [dnl
+priority=100,reg14=0x$port_key,metadata=0x$dp_key,dl_src=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG10[[8]]
])
-AT_CHECK([cat hv2_offlows_table72.txt | grep -v NXST | cut -d ' ' -f8- | sort], [0], [dnl
-priority=100,reg14=0x1,metadata=0x1,dl_src=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG10[[8]]
+AT_CHECK_UNQUOTED([cat hv2_offlows_table72.txt | grep -v NXST | cut -d ' '
-f8- | sort], [0], [dnl
+priority=100,reg14=0x$port_key,metadata=0x$dp_key,dl_src=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG10[[8]]
])
# Create the logical port sw0-p4 and this should be claimed by
@@ -31480,12 +31483,12 @@ as hv3 ovs-ofctl dump-flows br-int table=72 >
hv3_offlows_table72.txt
AT_CAPTURE_FILE([hv3_offlows_table71.txt])
AT_CAPTURE_FILE([hv3_offlows_table72.txt])
-AT_CHECK([cat hv3_offlows_table71.txt | grep -v NXST | cut -d ' ' -f8- | sort], [0], [dnl
-priority=100,metadata=0x1,dl_dst=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG15[[]]
+AT_CHECK_UNQUOTED([cat hv3_offlows_table71.txt | grep -v NXST | cut -d ' '
-f8- | sort], [0], [dnl
+priority=100,metadata=0x$dp_key,dl_dst=50:54:00:00:00:13
actions=load:0x$port_key->NXM_NX_REG15[[]]
])
-AT_CHECK([cat hv3_offlows_table72.txt | grep -v NXST | cut -d ' ' -f8- | sort], [0], [dnl
-priority=100,reg14=0x1,metadata=0x1,dl_src=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG10[[8]]
+AT_CHECK_UNQUOTED([cat hv3_offlows_table72.txt | grep -v NXST | cut -d ' '
-f8- | sort], [0], [dnl
+priority=100,reg14=0x$port_key,metadata=0x$dp_key,dl_src=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG10[[8]]
])
# Use the src mac 50:54:00:00:00:14 instead of 50:54:00:00:00:03
@@ -31512,19 +31515,19 @@ as hv3 ovs-ofctl dump-flows br-int table=71 >
hv3_offlows_table71.txt
Re: [ovs-dev] [PATCH ovn 2/4] system-tests: Make sure that the CT entries are sorted
Acked-by: Mark Michelson On 8/14/23 05:21, Ales Musil wrote: Make sure the CT entries are sorted, otherwise the check sometimes fails depending on the order from OvS. Signed-off-by: Ales Musil --- tests/system-ovn.at | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 766a250e5..bfa323b5c 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -7833,9 +7833,9 @@ sleep 3s # Ensure conntrack entry is present and ct_label is set. AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ sed -e 's/zone=[[0-9]]*/zone=/' | \ -sed -e 's/labels=0x4d2[[0-9a-f]]*/labels=0x4d2/'], [0], [dnl -icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d2 +sed -e 's/labels=0x4d2[[0-9a-f]]*/labels=0x4d2/' | sort], [0], [dnl icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone= +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d2 ]) # Add a higher priority ACL with different label. @@ -7849,9 +7849,9 @@ sleep 3s # Ensure conntrack entry is updated with new ct_label is set. AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ sed -e 's/zone=[[0-9]]*/zone=/' | \ -sed -e 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3/'], [0], [dnl -icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d3 +sed -e 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3/' | sort], [0], [dnl icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone= +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d3 ]) OVS_APP_EXIT_AND_WAIT([ovn-controller]) @@ -7934,9 +7934,9 @@ sleep 3s # Ensure conntrack entry is present and ct_label is set. AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ sed -e 's/zone=[[0-9]]*/zone=/' | \ -sed -e 's/labels=0x4d2[[0-9a-f]]*/labels=0x4d2/'], [0], [dnl -icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d2 +sed -e 's/labels=0x4d2[[0-9a-f]]*/labels=0x4d2/' | sort], [0], [dnl icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone= +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d2 ]) # Add a higher priority ACL with different label. @@ -7950,9 +7950,9 @@ sleep 3s # Ensure conntrack entry is updated with new ct_label is set. AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ sed -e 's/zone=[[0-9]]*/zone=/' | \ -sed -e 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3/'], [0], [dnl -icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d3 +sed -e 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3/' | sort], [0], [dnl icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone= +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d3 ]) OVS_APP_EXIT_AND_WAIT([ovn-controller]) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] NEWS: Add note about L4_SYM being used by default for ECMP.
On 8/14/23 12:25, Han Zhou wrote:
On Mon, Aug 14, 2023 at 6:54 AM Dumitru Ceara wrote:
Fixes: 596ea7acbe68 ("ovn-controller: Detect and use L4_SYM dp-hash if
available.")
Signed-off-by: Dumitru Ceara
---
NEWS | 3 +++
1 file changed, 3 insertions(+)
diff --git a/NEWS b/NEWS
index 8275877f99..f99334c1b8 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,9 @@ Post v23.06.0
- To allow optimizing ovn-controller's monitor conditions for the
regular
VIF case, ovn-controller now unconditionally monitors all sub-ports
(ports with parent_port set).
+ - ECMP routes use L4_SYM dp-hash by default if the datapath supports
it.
+Existing sessions might get re-hashed to a different ECMP path when
+OVN detects the algorithm support in the datapath.
nit: probably add "during the upgrade" to avoid misunderstanding.
Acked-by: Han Zhou
I updated the sentence to be "Existing sessions might get re-hashed to a
different ECMP path when OVN detects the algorithm support in the
datapath during an upgrade or restart of ovn-controller."
I pushed the change to main. Thanks Dumitru and Han.
OVN v23.06.0 - 01 Jun 2023
--
--
2.31.1
___
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] system-traffic.at: Avoid names veth0/veth1 in SRv6 tests.
On 8/14/23 15:37, Eelco Chaudron wrote:
>
>
> On 11 Aug 2023, at 19:23, Ilya Maximets wrote:
>
>> It's fairly common to have veth0/veth1 interfaces on a system,
>> but that breaks SRv6 tests that are trying to create them.
>>
>> Adding ovs- prefix to avoid name collision.
>>
>> Fixes: 03fc1ad78521 ("userspace: Add SRv6 tunnel support.")
>> Signed-off-by: Ilya Maximets
>
> This change looks good to me!
>
> Acked-by: Eelco Chaudron
Thanks! Applied and backported to 3.2.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-dev v5] dpif-netdev: fix dpif_netdev_flow_put
On 8/14/23 04:37, Peng He wrote:
> OVS allows overlapping megaflows, as long as the actions of these
> megaflows are equal. However, the current implementation of action
> modification relies on flow_lookup instead of ufid, this could result
> in looking up a wrong megaflow and make the ukeys and megaflows inconsistent
>
> Just like the test case in the patch, at first we have a rule with the
> prefix:
>
> 10.1.2.0/24
>
> and we will get a megaflow with prefixes 10.1.2.2/24 when a packet with IP
> 10.1.2.2 is received.
>
> Then suppose we change the rule into 10.1.0.0/16. OVS prefers to keep the
> 10.1.2.2/24 megaflow and just changes its action instead of extending
> the prefix into 10.1.2.2/16.
>
> then suppose we have a 10.1.0.2 packet, since it misses the megaflow,
> this time, we will have an overlapping megaflow with the right prefix:
> 10.1.0.2/16
>
> now we have two megaflows:
> 10.1.2.2/24
> 10.1.0.2/16
>
> last, suppose we have changed the ruleset again. The revalidator this
> time still decides to change the actions of both megaflows instead of
> deleting them.
>
> The dpif_netdev_flow_put will search the megaflow to modify with unmasked
> keys, however it might lookup the wrong megaflow as the key 10.1.2.2 matches
> both 10.1.2.2/24 and 10.1.0.2/16!
>
> This patch changes the megaflow lookup code in modification path into
> relying the ufid to find the correct megaflow instead of key lookup.
>
> Fixes: beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline")
> Signed-off-by: Peng He
> ---
> lib/dpif-netdev.c | 45 ++---
> tests/pmd.at | 47 +++
> 2 files changed, 77 insertions(+), 15 deletions(-)
Thanks! Applied and backported down to 2.17.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Fix a link.
Thank you Igor. Acked-by: Mark Michelson Since this is such a small change, I went ahead and applied it to all OVN branches from main back to 22.03. On 8/12/23 09:37, Igor Zhukov wrote: The server returns 404 for the previous link. Signed-off-by: Igor Zhukov --- ovn-architecture.7.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml index a2a87ec28..96294fe2b 100644 --- a/ovn-architecture.7.xml +++ b/ovn-architecture.7.xml @@ -1806,7 +1806,7 @@ For more information on L3 gateway high availability, please refer to -http://docs.ovn.org/en/latest/topics/high-availability. +http://docs.ovn.org/en/latest/topics/high-availability.html. Restrictions of Distributed Gateway Ports ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: Allow delay of northd engine runs
On Mon, Aug 14, 2023 at 9:54 AM Ilya Maximets wrote: > > On 8/11/23 15:07, Dumitru Ceara wrote: > > On 8/10/23 18:38, Ilya Maximets wrote: > >> On 8/10/23 17:34, Dumitru Ceara wrote: > >>> On 8/10/23 17:20, Han Zhou wrote: > On Thu, Aug 10, 2023 at 6:36 AM Dumitru Ceara wrote: > > > > On 8/10/23 15:34, Han Zhou wrote: > >> On Thu, Aug 10, 2023 at 2:29 AM Dumitru Ceara wrote: > >>> > >>> On 8/10/23 08:12, Ales Musil wrote: > On Wed, Aug 9, 2023 at 5:13 PM Mark Michelson < mmich...@redhat.com> > >> wrote: > > > Hi Ales, > > > > I have some high-level comments/questions about this patch. > > > > Hi Mark, > > >>> > >>> Hi Ales, Mark, > >>> > thank you for the review. See my answers inline below. > > > > I have been privy to the conversations that led to this change. My > > understanding is that by having ovn-northd wake up immediately, it > can > > be more CPU-intensive than waiting a bit for changes to accumulate > and > > handling all of those at once instead. However, nothing in either the > > commit message or ovn-nb.xml explains what the purpose of this new > > configuration option is. I think you should add a sentence or two to > > explain why someone would want to enable this option. > > > > > Yeah that's my bad, I have v2 prepared with some explanation in the > >> commit > message > together with results from scale run. > > >>> > >>> +1 we really need to explain why this change is needed. > >>> > > > > > Next, the algorithm used here strikes me as odd. We use the previous > >> run > > time of ovn-northd to determine how long to wait before running > again. > > This delay is capped by the configured backoff time. Let's say that > > we've configured the backoff interval to be 200 ms. If ovn-northd > has a > > super quick run and only takes 10 ms, then we will only delay the > next > > run by 10 ms. IMO, this seems like it would not mitigate the original > > issue by much, since we are only allowing a maximum of 20 ms (10 ms > for > > the run of ovn-northd + 10 ms delay) of NB changes to accumulate. > > Conversely, if northd has a huge recompute and it takes 5000 ms to > > complete, then we would delay the next run by 200ms. In this case, > > delaying at all seems like it's not necessary since we potentially > have > > 5000 ms worth of NB DB updates that have not been addressed. I would > > have expected the opposite approach to be taken. If someone > configures > > 200ms as their backoff interval, I would expect us to always allow a > > *minimum* of 200ms of NB changes to accumulate before running again. > So > > for instance, if northd runs quickly and is done in 10 ms, then we > >> would > > wait 200 - 10 = 190 ms before processing changes again. If northd > takes > > a long time to recompute and takes 5000 ms, then we would not wait at > > all before processing changes again. Was the algorithm chosen based > on > > experimentation? Is it a well-known method I'm just not familiar > with? > >>> > >>> I think the main assumption (that should probably be made explicit in > >>> the commit log and/or documentation) is that on average changes happen > >>> in a uniform way. This might not always be accurate. > >>> > >>> However, if we're off with the estimate, in the worst case we'd be > >>> adding the configured max delay to the latency of processing changes. > >>> So, as long as the value is not extremely high, the impact is not that > >>> high either. > >>> > >>> Last but not least, as this value would be configured by the CMS, we > >>> assume the CMS knows what they're doing. :) > >>> > > > > I'm not sure if the algorithm is well known. > > The thing is that at scale we almost always cap at the backoff so it > has > probably > the same effect as your suggestion with the difference that we > actually > delay even > after long runs. And that is actually desired, it's true that in the > >> let's > say 500 ms > should be enough to accumulate more changes however that can lead to > >> another > 500 ms run and so on. That in the end means that northd will spin at > >> 100% > CPU > anyway which is what we want to avoid. So from another point of view > the > accumulation > of IDL changes is a secondary effect which is still desired, but not > the > main purpose. >
Re: [ovs-dev] [PATCH ovn] northd: Allow delay of northd engine runs
On 8/11/23 15:07, Dumitru Ceara wrote: > On 8/10/23 18:38, Ilya Maximets wrote: >> On 8/10/23 17:34, Dumitru Ceara wrote: >>> On 8/10/23 17:20, Han Zhou wrote: On Thu, Aug 10, 2023 at 6:36 AM Dumitru Ceara wrote: > > On 8/10/23 15:34, Han Zhou wrote: >> On Thu, Aug 10, 2023 at 2:29 AM Dumitru Ceara wrote: >>> >>> On 8/10/23 08:12, Ales Musil wrote: On Wed, Aug 9, 2023 at 5:13 PM Mark Michelson >> wrote: > Hi Ales, > > I have some high-level comments/questions about this patch. > Hi Mark, >>> >>> Hi Ales, Mark, >>> thank you for the review. See my answers inline below. > I have been privy to the conversations that led to this change. My > understanding is that by having ovn-northd wake up immediately, it can > be more CPU-intensive than waiting a bit for changes to accumulate and > handling all of those at once instead. However, nothing in either the > commit message or ovn-nb.xml explains what the purpose of this new > configuration option is. I think you should add a sentence or two to > explain why someone would want to enable this option. > > Yeah that's my bad, I have v2 prepared with some explanation in the >> commit message together with results from scale run. >>> >>> +1 we really need to explain why this change is needed. >>> > > Next, the algorithm used here strikes me as odd. We use the previous >> run > time of ovn-northd to determine how long to wait before running again. > This delay is capped by the configured backoff time. Let's say that > we've configured the backoff interval to be 200 ms. If ovn-northd has a > super quick run and only takes 10 ms, then we will only delay the next > run by 10 ms. IMO, this seems like it would not mitigate the original > issue by much, since we are only allowing a maximum of 20 ms (10 ms for > the run of ovn-northd + 10 ms delay) of NB changes to accumulate. > Conversely, if northd has a huge recompute and it takes 5000 ms to > complete, then we would delay the next run by 200ms. In this case, > delaying at all seems like it's not necessary since we potentially have > 5000 ms worth of NB DB updates that have not been addressed. I would > have expected the opposite approach to be taken. If someone configures > 200ms as their backoff interval, I would expect us to always allow a > *minimum* of 200ms of NB changes to accumulate before running again. So > for instance, if northd runs quickly and is done in 10 ms, then we >> would > wait 200 - 10 = 190 ms before processing changes again. If northd takes > a long time to recompute and takes 5000 ms, then we would not wait at > all before processing changes again. Was the algorithm chosen based on > experimentation? Is it a well-known method I'm just not familiar with? >>> >>> I think the main assumption (that should probably be made explicit in >>> the commit log and/or documentation) is that on average changes happen >>> in a uniform way. This might not always be accurate. >>> >>> However, if we're off with the estimate, in the worst case we'd be >>> adding the configured max delay to the latency of processing changes. >>> So, as long as the value is not extremely high, the impact is not that >>> high either. >>> >>> Last but not least, as this value would be configured by the CMS, we >>> assume the CMS knows what they're doing. :) >>> > I'm not sure if the algorithm is well known. The thing is that at scale we almost always cap at the backoff so it has probably the same effect as your suggestion with the difference that we actually delay even after long runs. And that is actually desired, it's true that in the >> let's say 500 ms should be enough to accumulate more changes however that can lead to >> another 500 ms run and so on. That in the end means that northd will spin at >> 100% CPU anyway which is what we want to avoid. So from another point of view the accumulation of IDL changes is a secondary effect which is still desired, but not the main purpose. Also delaying by short time if the previous run was short is fine, you >> are right that we don't accumulate enough however during short running times there is a high >> chance that the northd would get to sleep anyway (We will help it to sleep at least a
Re: [ovs-dev] [PATCH ovn] northd: Allow delay of northd engine runs
On Mon, Aug 14, 2023 at 4:46 AM Dumitru Ceara wrote: > > On 8/12/23 07:08, Han Zhou wrote: > > On Fri, Aug 11, 2023 at 6:07 AM Dumitru Ceara wrote: > >> > >> On 8/10/23 18:38, Ilya Maximets wrote: > >>> On 8/10/23 17:34, Dumitru Ceara wrote: > On 8/10/23 17:20, Han Zhou wrote: > > On Thu, Aug 10, 2023 at 6:36 AM Dumitru Ceara > > wrote: > >> > >> On 8/10/23 15:34, Han Zhou wrote: > >>> On Thu, Aug 10, 2023 at 2:29 AM Dumitru Ceara > > wrote: > > On 8/10/23 08:12, Ales Musil wrote: > > On Wed, Aug 9, 2023 at 5:13 PM Mark Michelson < mmich...@redhat.com > >> > >>> wrote: > > > >> Hi Ales, > >> > >> I have some high-level comments/questions about this patch. > >> > > > > Hi Mark, > > > > Hi Ales, Mark, > > > thank you for the review. See my answers inline below. > > > > > >> I have been privy to the conversations that led to this change. > > My > >> understanding is that by having ovn-northd wake up immediately, > > it > > can > >> be more CPU-intensive than waiting a bit for changes to > > accumulate > > and > >> handling all of those at once instead. However, nothing in > > either the > >> commit message or ovn-nb.xml explains what the purpose of this > > new > >> configuration option is. I think you should add a sentence or > > two to > >> explain why someone would want to enable this option. > >> > >> > > Yeah that's my bad, I have v2 prepared with some explanation in > > the > >>> commit > > message > > together with results from scale run. > > > > +1 we really need to explain why this change is needed. > > > > >> > >> Next, the algorithm used here strikes me as odd. We use the > > previous > >>> run > >> time of ovn-northd to determine how long to wait before running > > again. > >> This delay is capped by the configured backoff time. Let's say > > that > >> we've configured the backoff interval to be 200 ms. If ovn-northd > > has a > >> super quick run and only takes 10 ms, then we will only delay the > > next > >> run by 10 ms. IMO, this seems like it would not mitigate the > > original > >> issue by much, since we are only allowing a maximum of 20 ms (10 > > ms > > for > >> the run of ovn-northd + 10 ms delay) of NB changes to accumulate. > >> Conversely, if northd has a huge recompute and it takes 5000 ms > > to > >> complete, then we would delay the next run by 200ms. In this > > case, > >> delaying at all seems like it's not necessary since we > > potentially > > have > >> 5000 ms worth of NB DB updates that have not been addressed. I > > would > >> have expected the opposite approach to be taken. If someone > > configures > >> 200ms as their backoff interval, I would expect us to always > > allow a > >> *minimum* of 200ms of NB changes to accumulate before running > > again. > > So > >> for instance, if northd runs quickly and is done in 10 ms, then > > we > >>> would > >> wait 200 - 10 = 190 ms before processing changes again. If northd > > takes > >> a long time to recompute and takes 5000 ms, then we would not > > wait at > >> all before processing changes again. Was the algorithm chosen > > based > > on > >> experimentation? Is it a well-known method I'm just not familiar > > with? > > I think the main assumption (that should probably be made explicit > > in > the commit log and/or documentation) is that on average changes > > happen > in a uniform way. This might not always be accurate. > > However, if we're off with the estimate, in the worst case we'd be > adding the configured max delay to the latency of processing > > changes. > So, as long as the value is not extremely high, the impact is not > > that > high either. > > Last but not least, as this value would be configured by the CMS, > > we > assume the CMS knows what they're doing. :) > > >> > > > > I'm not sure if the algorithm is well known. > > > > The thing is that at scale we almost always cap at the backoff so > > it > > has > > probably > > the same effect as your suggestion with the difference that we > > actually > > delay even > > after long runs. And that is actually desired, it's true that in > > the > >>> let's > > say 500 ms > > should be enough to accumulate more changes however that can lead > > to > >>> another > > 500 ms run and so on. That in the end means that
Re: [ovs-dev] [PATCH ovn] NEWS: Add note about L4_SYM being used by default for ECMP.
On Mon, Aug 14, 2023 at 6:54 AM Dumitru Ceara wrote:
>
> Fixes: 596ea7acbe68 ("ovn-controller: Detect and use L4_SYM dp-hash if
available.")
> Signed-off-by: Dumitru Ceara
> ---
> NEWS | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index 8275877f99..f99334c1b8 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -10,6 +10,9 @@ Post v23.06.0
>- To allow optimizing ovn-controller's monitor conditions for the
regular
> VIF case, ovn-controller now unconditionally monitors all sub-ports
> (ports with parent_port set).
> + - ECMP routes use L4_SYM dp-hash by default if the datapath supports
it.
> +Existing sessions might get re-hashed to a different ECMP path when
> +OVN detects the algorithm support in the datapath.
nit: probably add "during the upgrade" to avoid misunderstanding.
Acked-by: Han Zhou
>
> OVN v23.06.0 - 01 Jun 2023
> --
> --
> 2.31.1
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovn-controller: Detect and use L4_SYM dp-hash if available.
On 8/12/23 07:19, Han Zhou wrote:
> On Fri, Aug 11, 2023 at 6:23 AM Dumitru Ceara wrote:
>>
>> On 8/1/23 12:51, Han Zhou wrote:
>>> On Tue, Jul 18, 2023 at 6:16 AM Dumitru Ceara wrote:
On 7/18/23 12:14, Han Zhou wrote:
> On Mon, Jul 17, 2023 at 9:51 PM Dumitru Ceara
> wrote:
>>
>> On 7/14/23 08:41, Ales Musil wrote:
>>> On Thu, Jul 13, 2023 at 4:39 PM Dumitru Ceara
>>> wrote:
>>>
Regular dp-hash is not a canonical L4 hash (at least with the
> netlink
datapath). If the datapath supports l4 symmetrical dp-hash use
> that
> one
instead.
Reported-at: https://github.com/ovn-org/ovn/issues/112
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2188679
Signed-off-by: Dumitru Ceara
>>>
>>> Hi Dumitru,
>>>
>>>
---
include/ovn/features.h | 2 ++
lib/actions.c | 6 ++
lib/features.c | 49
>>> +-
3 files changed, 47 insertions(+), 10 deletions(-)
diff --git a/include/ovn/features.h b/include/ovn/features.h
index de8f1f5489..3bf536127f 100644
--- a/include/ovn/features.h
+++ b/include/ovn/features.h
@@ -34,12 +34,14 @@ enum ovs_feature_support_bits {
OVS_CT_ZERO_SNAT_SUPPORT_BIT,
OVS_DP_METER_SUPPORT_BIT,
OVS_CT_TUPLE_FLUSH_BIT,
+OVS_DP_HASH_L4_SYM_BIT,
};
enum ovs_feature_value {
OVS_CT_ZERO_SNAT_SUPPORT = (1 <<
> OVS_CT_ZERO_SNAT_SUPPORT_BIT),
OVS_DP_METER_SUPPORT = (1 << OVS_DP_METER_SUPPORT_BIT),
OVS_CT_TUPLE_FLUSH_SUPPORT = (1 << OVS_CT_TUPLE_FLUSH_BIT),
+OVS_DP_HASH_L4_SYM_SUPPORT = (1 << OVS_DP_HASH_L4_SYM_BIT),
};
void ovs_feature_support_destroy(void);
diff --git a/lib/actions.c b/lib/actions.c
index 037172e606..9d52cb75a8 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -1625,6 +1625,12 @@ encode_SELECT(const struct ovnact_select
> *select,
struct ds ds = DS_EMPTY_INITIALIZER;
ds_put_format(, "type=select,selection_method=dp_hash");
+if (ovs_feature_is_supported(OVS_DP_HASH_L4_SYM_SUPPORT)) {
+/* Select dp-hash l4_symmetric by setting the upper 32bits
>>> of
+ * selection_method_param to 1: */
>>>
>>> This comment is a bit unfortunate, because it reads like you want to
>>> set
>>> all bits of the upper half
>>> to 1 e.g. 0x. Maybe change it to: "selection_method_param to
> value
>>> 1 (1 << 32)." during merge, wdyt?
>>>
>>>
>>
>> Makes sense, thanks for the suggestion! I changed it accordingly.
>>
+ds_put_cstr(, ",selection_method_param=0x1");
+}
+
struct mf_subfield sf =
> expr_resolve_field(>res_field);
for (size_t bucket_id = 0; bucket_id < select->n_dsts;
> bucket_id++) {
diff --git a/lib/features.c b/lib/features.c
index 97c9c86f00..d24e8f6c5c 100644
--- a/lib/features.c
+++ b/lib/features.c
@@ -21,6 +21,7 @@
#include "lib/dirs.h"
#include "socket-util.h"
#include "lib/vswitch-idl.h"
+#include "odp-netlink.h"
#include "openvswitch/vlog.h"
#include "openvswitch/ofpbuf.h"
#include "openvswitch/rconn.h"
@@ -33,20 +34,48 @@ VLOG_DEFINE_THIS_MODULE(features);
#define FEATURES_DEFAULT_PROBE_INTERVAL_SEC 5
+/* Parses 'cap_name' from 'ovs_capabilities' and returns whether
> the
+ * type of capability is supported or not. */
+typedef bool ovs_feature_parse_func(const struct smap
> *ovs_capabilities,
+const char *cap_name);
+
struct ovs_feature {
enum ovs_feature_value value;
const char *name;
+ovs_feature_parse_func *parse;
};
+static bool
+bool_parser(const struct smap *ovs_capabilities, const char
>>> *cap_name)
+{
+return smap_get_bool(ovs_capabilities, cap_name, false);
+}
+
+static bool
+dp_hash_l4_sym_support_parser(const struct smap *ovs_capabilities,
+ const char *cap_name OVS_UNUSED)
+{
+int max_hash_alg = smap_get_int(ovs_capabilities,
>>> "max_hash_alg",
> 0);
+
+return max_hash_alg == OVS_HASH_ALG_SYM_L4;
+}
+
static struct ovs_feature all_ovs_features[] = {
{
.value = OVS_CT_ZERO_SNAT_SUPPORT,
-.name = "ct_zero_snat"
+
[ovs-dev] [PATCH ovn] NEWS: Add note about L4_SYM being used by default for ECMP.
Fixes: 596ea7acbe68 ("ovn-controller: Detect and use L4_SYM dp-hash if
available.")
Signed-off-by: Dumitru Ceara
---
NEWS | 3 +++
1 file changed, 3 insertions(+)
diff --git a/NEWS b/NEWS
index 8275877f99..f99334c1b8 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,9 @@ Post v23.06.0
- To allow optimizing ovn-controller's monitor conditions for the regular
VIF case, ovn-controller now unconditionally monitors all sub-ports
(ports with parent_port set).
+ - ECMP routes use L4_SYM dp-hash by default if the datapath supports it.
+Existing sessions might get re-hashed to a different ECMP path when
+OVN detects the algorithm support in the datapath.
OVN v23.06.0 - 01 Jun 2023
--
--
2.31.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] system-traffic.at: Avoid names veth0/veth1 in SRv6 tests.
On 11 Aug 2023, at 19:23, Ilya Maximets wrote:
> It's fairly common to have veth0/veth1 interfaces on a system,
> but that breaks SRv6 tests that are trying to create them.
>
> Adding ovs- prefix to avoid name collision.
>
> Fixes: 03fc1ad78521 ("userspace: Add SRv6 tunnel support.")
> Signed-off-by: Ilya Maximets
This change looks good to me!
Acked-by: Eelco Chaudron
> ---
> tests/system-traffic.at | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 945037ec0..808c492a2 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -1202,11 +1202,11 @@ AT_CHECK([ovs-ofctl add-flow br0
> in_port=at_srv6,actions=mod_dl_dst:aa:55:aa:55:
>
> dnl Set up tunnel endpoints on the namespace 'at_ns0',
> dnl and overlay port on the namespace 'at_ns1'
> -ADD_VETH_NS([at_ns0], [veth0], [10.1.1.2/24], [at_ns1], [veth1],
> [10.1.1.1/24])
> +ADD_VETH_NS([at_ns0], [ovs-veth0], [10.1.1.2/24], [at_ns1], [ovs-veth1],
> [10.1.1.1/24])
> NS_CHECK_EXEC([at_ns0], [ip sr tunsrc set fc00:a::1])
> NS_CHECK_EXEC([at_ns0], [ip route add 10.100.100.0/24 encap seg6 mode encap
> segs fc00::100 dev p0])
> -NS_CHECK_EXEC([at_ns0], [ip -6 route add fc00:a::1 encap seg6local action
> End.DX4 nh4 0.0.0.0 dev veth0])
> -NS_CHECK_EXEC([at_ns1], [ip route add 10.100.100.0/24 via 10.1.1.2 dev
> veth1])
> +NS_CHECK_EXEC([at_ns0], [ip -6 route add fc00:a::1 encap seg6local action
> End.DX4 nh4 0.0.0.0 dev ovs-veth0])
> +NS_CHECK_EXEC([at_ns1], [ip route add 10.100.100.0/24 via 10.1.1.2 dev
> ovs-veth1])
>
> dnl Linux seems to take a little time to get its IPv6 stack in order. Without
> dnl waiting, we get occasional failures due to the following error:
> @@ -1263,11 +1263,11 @@ AT_CHECK([ovs-ofctl add-flow br0
> in_port=at_srv6,actions=mod_dl_dst:aa:55:aa:55:
>
> dnl Set up tunnel endpoints on the namespace 'at_ns0',
> dnl and overlay port on the namespace 'at_ns1'
> -ADD_VETH_NS([at_ns0], [veth0], [fc00:1::2/64], [at_ns1], [veth1],
> [fc00:1::1/64])
> +ADD_VETH_NS([at_ns0], [ovs-veth0], [fc00:1::2/64], [at_ns1], [ovs-veth1],
> [fc00:1::1/64])
> NS_CHECK_EXEC([at_ns0], [ip sr tunsrc set fc00:a::1])
> NS_CHECK_EXEC([at_ns0], [ip -6 route add fc00:100::0/64 encap seg6 mode
> encap segs fc00::100 dev p0])
> -NS_CHECK_EXEC([at_ns0], [ip -6 route add fc00:a::1 encap seg6local action
> End.DX6 nh6 :: dev veth0])
> -NS_CHECK_EXEC([at_ns1], [ip -6 route add fc00:100::/64 via fc00:1::2 dev
> veth1])
> +NS_CHECK_EXEC([at_ns0], [ip -6 route add fc00:a::1 encap seg6local action
> End.DX6 nh6 :: dev ovs-veth0])
> +NS_CHECK_EXEC([at_ns1], [ip -6 route add fc00:100::/64 via fc00:1::2 dev
> ovs-veth1])
>
> dnl Linux seems to take a little time to get its IPv6 stack in order. Without
> dnl waiting, we get occasional failures due to the following error:
> --
> 2.40.1
>
> ___
> 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 RFC] conntrack: Remove nat_conn introducing key directionality.
Bleep bloop. Greetings Paolo Valerio, 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: ERROR: Author hepeng needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Peng He WARNING: Line is 81 characters long (recommended limit is 79) #826 FILE: lib/conntrack.c:3330: _for_expectation->key_node[CT_DIR_FWD].key.src.addr.ipv6, Lines checked: 851, Warnings: 2, Errors: 1 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 RFC] conntrack: Remove nat_conn introducing key directionality.
From: hepeng
The patch avoids the extra allocation for nat_conn.
Currently, when doing NAT, the userspace conntrack will use an extra
conn for the two directions in a flow. However, each conn has actually
the two keys for both orig and rev directions. This patch introduces a
key_node[CT_DIRS] member in the conn which consists of a key, direction,
and a cmap_node for hash lookup so addressing the feedback received by
the original patch [0].
The patch is an alternative approach to [1].
The patch has the advantage of solving the issue in a clean way, but,
unlike [1], it has the disadvantage of requiring some changes to the
connection clean up for older branches (down to 2.17) and all the
related operations. To make an idea, [0] contains most of the changes
required.
[0]
https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/
[1] https://patchwork.ozlabs.org/project/openvswitch/list/?series=351579=*
Signed-off-by: Peng He
Co-authored-by: Paolo Valerio
Signed-off-by: Paolo Valerio
---
lib/conntrack-private.h | 19 ++-
lib/conntrack-tp.c |6 +
lib/conntrack.c | 339 +++
3 files changed, 149 insertions(+), 215 deletions(-)
diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index bb326868e..3fd5fccd3 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -49,6 +49,12 @@ struct ct_endpoint {
* hashing in ct_endpoint_hash_add(). */
BUILD_ASSERT_DECL(sizeof(struct ct_endpoint) == sizeof(union ct_addr) + 4);
+enum key_dir {
+CT_DIR_FWD = 0,
+CT_DIR_REV,
+CT_DIRS,
+};
+
/* Changes to this structure need to be reflected in conn_key_hash()
* and conn_key_cmp(). */
struct conn_key {
@@ -112,20 +118,18 @@ enum ct_timeout {
#define N_EXP_LISTS 100
-enum OVS_PACKED_ENUM ct_conn_type {
-CT_CONN_TYPE_DEFAULT,
-CT_CONN_TYPE_UN_NAT,
+struct conn_key_node {
+enum key_dir dir;
+struct conn_key key;
+struct cmap_node cm_node;
};
struct conn {
/* Immutable data. */
-struct conn_key key;
-struct conn_key rev_key;
+struct conn_key_node key_node[CT_DIRS];
struct conn_key parent_key; /* Only used for orig_tuple support. */
-struct cmap_node cm_node;
uint16_t nat_action;
char *alg;
-struct conn *nat_conn; /* The NAT 'conn' context, if there is one. */
atomic_flag reclaimed; /* False during the lifetime of the connection,
* True as soon as a thread has started freeing
* its memory. */
@@ -150,7 +154,6 @@ struct conn {
/* Immutable data. */
bool alg_related; /* True if alg data connection. */
-enum ct_conn_type conn_type;
uint32_t tp_id; /* Timeout policy ID. */
};
diff --git a/lib/conntrack-tp.c b/lib/conntrack-tp.c
index 89cb2704a..2149fdc73 100644
--- a/lib/conntrack-tp.c
+++ b/lib/conntrack-tp.c
@@ -253,7 +253,8 @@ conn_update_expiration(struct conntrack *ct, struct conn
*conn,
}
VLOG_DBG_RL(, "Update timeout %s zone=%u with policy id=%d "
"val=%u sec.",
-ct_timeout_str[tm], conn->key.zone, conn->tp_id, val);
+ct_timeout_str[tm], conn->key_node[CT_DIR_FWD].key.zone,
+conn->tp_id, val);
atomic_store_relaxed(>expiration, now + val * 1000);
}
@@ -273,7 +274,8 @@ conn_init_expiration(struct conntrack *ct, struct conn
*conn,
}
VLOG_DBG_RL(, "Init timeout %s zone=%u with policy id=%d val=%u sec.",
-ct_timeout_str[tm], conn->key.zone, conn->tp_id, val);
+ct_timeout_str[tm], conn->key_node[CT_DIR_FWD].key.zone,
+conn->tp_id, val);
conn->expiration = now + val * 1000;
}
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 5f1176d33..6f219eb9e 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -113,8 +113,7 @@ static void set_label(struct dp_packet *, struct conn *,
static void *clean_thread_main(void *f_);
static bool
-nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
- struct conn *nat_conn,
+nat_get_unique_tuple(struct conntrack *ct, struct conn *conn,
const struct nat_action_info_t *nat_info);
static uint8_t
@@ -234,61 +233,6 @@ conn_key_cmp(const struct conn_key *key1, const struct
conn_key *key2)
return 1;
}
-static void
-ct_print_conn_info(const struct conn *c, const char *log_msg,
- enum vlog_level vll, bool force, bool rl_on)
-{
-#define CT_VLOG(RL_ON, LEVEL, ...) \
-do {\
-if (RL_ON) {\
-static struct vlog_rate_limit rl_ = VLOG_RATE_LIMIT_INIT(5, 5); \
-vlog_rate_limit(_module, LEVEL, _, __VA_ARGS__);\
-} else {
Re: [ovs-dev] [PATCH ovn] northd: Allow delay of northd engine runs
On Mon, Aug 14, 2023 at 1:46 PM Dumitru Ceara wrote: > On 8/12/23 07:08, Han Zhou wrote: > > On Fri, Aug 11, 2023 at 6:07 AM Dumitru Ceara wrote: > >> > >> On 8/10/23 18:38, Ilya Maximets wrote: > >>> On 8/10/23 17:34, Dumitru Ceara wrote: > On 8/10/23 17:20, Han Zhou wrote: > > On Thu, Aug 10, 2023 at 6:36 AM Dumitru Ceara > > wrote: > >> > >> On 8/10/23 15:34, Han Zhou wrote: > >>> On Thu, Aug 10, 2023 at 2:29 AM Dumitru Ceara > > wrote: > > On 8/10/23 08:12, Ales Musil wrote: > > On Wed, Aug 9, 2023 at 5:13 PM Mark Michelson < > mmich...@redhat.com > >> > >>> wrote: > > > >> Hi Ales, > >> > >> I have some high-level comments/questions about this patch. > >> > > > > Hi Mark, > > > > Hi Ales, Mark, > > > thank you for the review. See my answers inline below. > > > > > >> I have been privy to the conversations that led to this change. > > My > >> understanding is that by having ovn-northd wake up immediately, > > it > > can > >> be more CPU-intensive than waiting a bit for changes to > > accumulate > > and > >> handling all of those at once instead. However, nothing in > > either the > >> commit message or ovn-nb.xml explains what the purpose of this > > new > >> configuration option is. I think you should add a sentence or > > two to > >> explain why someone would want to enable this option. > >> > >> > > Yeah that's my bad, I have v2 prepared with some explanation in > > the > >>> commit > > message > > together with results from scale run. > > > > +1 we really need to explain why this change is needed. > > > > >> > >> Next, the algorithm used here strikes me as odd. We use the > > previous > >>> run > >> time of ovn-northd to determine how long to wait before running > > again. > >> This delay is capped by the configured backoff time. Let's say > > that > >> we've configured the backoff interval to be 200 ms. If > ovn-northd > > has a > >> super quick run and only takes 10 ms, then we will only delay > the > > next > >> run by 10 ms. IMO, this seems like it would not mitigate the > > original > >> issue by much, since we are only allowing a maximum of 20 ms (10 > > ms > > for > >> the run of ovn-northd + 10 ms delay) of NB changes to > accumulate. > >> Conversely, if northd has a huge recompute and it takes 5000 ms > > to > >> complete, then we would delay the next run by 200ms. In this > > case, > >> delaying at all seems like it's not necessary since we > > potentially > > have > >> 5000 ms worth of NB DB updates that have not been addressed. I > > would > >> have expected the opposite approach to be taken. If someone > > configures > >> 200ms as their backoff interval, I would expect us to always > > allow a > >> *minimum* of 200ms of NB changes to accumulate before running > > again. > > So > >> for instance, if northd runs quickly and is done in 10 ms, then > > we > >>> would > >> wait 200 - 10 = 190 ms before processing changes again. If > northd > > takes > >> a long time to recompute and takes 5000 ms, then we would not > > wait at > >> all before processing changes again. Was the algorithm chosen > > based > > on > >> experimentation? Is it a well-known method I'm just not familiar > > with? > > I think the main assumption (that should probably be made explicit > > in > the commit log and/or documentation) is that on average changes > > happen > in a uniform way. This might not always be accurate. > > However, if we're off with the estimate, in the worst case we'd be > adding the configured max delay to the latency of processing > > changes. > So, as long as the value is not extremely high, the impact is not > > that > high either. > > Last but not least, as this value would be configured by the CMS, > > we > assume the CMS knows what they're doing. :) > > >> > > > > I'm not sure if the algorithm is well known. > > > > The thing is that at scale we almost always cap at the backoff so > > it > > has > > probably > > the same effect as your suggestion with the difference that we > > actually > > delay even > > after long runs. And that is actually desired, it's true that in > > the > >>> let's > > say 500 ms > > should be enough to accumulate more changes however that can lead > > to > >>> another > > 500 ms run and so on. That in the end
Re: [ovs-dev] [PATCH] MAINTAINERS: Add Aaron Conole.
On 8/14/23 12:39, Ilya Maximets wrote: > On 8/12/23 10:07, Simon Horman wrote: >> Aaron Conole was recently elected by the Open vSwitch committers. >> This formalizes his status as an Open vSwitch committer. >> >> Welcome Aaron! > > Welcome! > Congrats, Aaron, well deserved! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: Allow delay of northd engine runs
On 8/12/23 07:08, Han Zhou wrote: > On Fri, Aug 11, 2023 at 6:07 AM Dumitru Ceara wrote: >> >> On 8/10/23 18:38, Ilya Maximets wrote: >>> On 8/10/23 17:34, Dumitru Ceara wrote: On 8/10/23 17:20, Han Zhou wrote: > On Thu, Aug 10, 2023 at 6:36 AM Dumitru Ceara > wrote: >> >> On 8/10/23 15:34, Han Zhou wrote: >>> On Thu, Aug 10, 2023 at 2:29 AM Dumitru Ceara > wrote: On 8/10/23 08:12, Ales Musil wrote: > On Wed, Aug 9, 2023 at 5:13 PM Mark Michelson > >>> wrote: > >> Hi Ales, >> >> I have some high-level comments/questions about this patch. >> > > Hi Mark, > Hi Ales, Mark, > thank you for the review. See my answers inline below. > > >> I have been privy to the conversations that led to this change. > My >> understanding is that by having ovn-northd wake up immediately, > it > can >> be more CPU-intensive than waiting a bit for changes to > accumulate > and >> handling all of those at once instead. However, nothing in > either the >> commit message or ovn-nb.xml explains what the purpose of this > new >> configuration option is. I think you should add a sentence or > two to >> explain why someone would want to enable this option. >> >> > Yeah that's my bad, I have v2 prepared with some explanation in > the >>> commit > message > together with results from scale run. > +1 we really need to explain why this change is needed. > >> >> Next, the algorithm used here strikes me as odd. We use the > previous >>> run >> time of ovn-northd to determine how long to wait before running > again. >> This delay is capped by the configured backoff time. Let's say > that >> we've configured the backoff interval to be 200 ms. If ovn-northd > has a >> super quick run and only takes 10 ms, then we will only delay the > next >> run by 10 ms. IMO, this seems like it would not mitigate the > original >> issue by much, since we are only allowing a maximum of 20 ms (10 > ms > for >> the run of ovn-northd + 10 ms delay) of NB changes to accumulate. >> Conversely, if northd has a huge recompute and it takes 5000 ms > to >> complete, then we would delay the next run by 200ms. In this > case, >> delaying at all seems like it's not necessary since we > potentially > have >> 5000 ms worth of NB DB updates that have not been addressed. I > would >> have expected the opposite approach to be taken. If someone > configures >> 200ms as their backoff interval, I would expect us to always > allow a >> *minimum* of 200ms of NB changes to accumulate before running > again. > So >> for instance, if northd runs quickly and is done in 10 ms, then > we >>> would >> wait 200 - 10 = 190 ms before processing changes again. If northd > takes >> a long time to recompute and takes 5000 ms, then we would not > wait at >> all before processing changes again. Was the algorithm chosen > based > on >> experimentation? Is it a well-known method I'm just not familiar > with? I think the main assumption (that should probably be made explicit > in the commit log and/or documentation) is that on average changes > happen in a uniform way. This might not always be accurate. However, if we're off with the estimate, in the worst case we'd be adding the configured max delay to the latency of processing > changes. So, as long as the value is not extremely high, the impact is not > that high either. Last but not least, as this value would be configured by the CMS, > we assume the CMS knows what they're doing. :) >> > > I'm not sure if the algorithm is well known. > > The thing is that at scale we almost always cap at the backoff so > it > has > probably > the same effect as your suggestion with the difference that we > actually > delay even > after long runs. And that is actually desired, it's true that in > the >>> let's > say 500 ms > should be enough to accumulate more changes however that can lead > to >>> another > 500 ms run and so on. That in the end means that northd will spin > at >>> 100% > CPU > anyway which is what we want to avoid. So from another point of > view > the > accumulation > of IDL changes is a secondary effect which is still desired, but > not > the > main purpose. > > Also delaying by short time if the previous run was short is >
Re: [ovs-dev] [PATCH] MAINTAINERS: Add Aaron Conole.
On 8/12/23 10:07, Simon Horman wrote: > Aaron Conole was recently elected by the Open vSwitch committers. > This formalizes his status as an Open vSwitch committer. > > Welcome Aaron! Welcome! > > Signed-off-by: Simon Horman Applied. Thanks! Best regards, Ilya Maximets. > --- > MAINTAINERS.rst | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/MAINTAINERS.rst b/MAINTAINERS.rst > index a3e923195c3e..99a0bd405b2a 100644 > --- a/MAINTAINERS.rst > +++ b/MAINTAINERS.rst > @@ -41,6 +41,8 @@ This is the current list of active Open vSwitch committers: > > * - Name > - Email > + * - Aaron Conole > + - acon...@redhat.com > * - Alin Serdean > - aserd...@ovn.org > * - Ansis Atteka ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 4/4] tests: Add missing sync calls
Add various missing sync calls which caused the tests to be flaky due to sometimes missed on various checks or packets. Signed-off-by: Ales Musil --- tests/ovn-controller.at | 10 +- tests/ovn-ic.at | 7 --- tests/ovn-northd.at | 14 +- tests/ovn.at| 20 ++-- tests/system-ovn.at | 5 +++-- 5 files changed, 35 insertions(+), 21 deletions(-) diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index f2216d245..6a4bcedab 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -433,10 +433,10 @@ check ovn-nbctl --wait=hv sync check ovn-nbctl --wait=hv sync # nb_cfg should be set to 1 in the chassis_private/nb_global/sb_global table -check_column 1 chassis_private nb_cfg -check_column 1 sb_global nb_cfg -check_column 1 nb:nb_global nb_cfg -check_column 0 chassis nb_cfg +wait_row_count nb:nb_global 1 nb_cfg=1 +wait_row_count chassis_private 1 nb_cfg=1 +wait_row_count sb_global 1 nb_cfg=1 +wait_row_count chassis 1 nb_cfg=0 OVN_CLEANUP([hv]) AT_CLEANUP @@ -562,7 +562,7 @@ primary lport : [[lsp1]] ]) # Set the port type to localport -check ovn-nbctl lsp-set-type lsp1 localport +check ovn-nbctl --wait=hv lsp-set-type lsp1 localport check as hv1 ovs-vsctl set open . external_ids:ovn-cms-options=localport OVS_WAIT_UNTIL([test localport = $(ovn-sbctl get chassis . other_config:ovn-cms-options)]) diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at index 09eac860c..a654e59fe 100644 --- a/tests/ovn-ic.at +++ b/tests/ovn-ic.at @@ -543,6 +543,7 @@ done # Create directly-connected routes ovn_as az2 ovn-nbctl lrp-add lr12 lrp-lr12-ls2 aa:aa:aa:aa:bb:01 "192.168.0.1/24" ovn_as az2 ovn-nbctl lr-route-add lr12 10.10.10.0/24 192.168.0.10 +ovn_as az1 ovn-nbctl --wait=sb sync echo az1 ovn_as az1 ovn-nbctl show @@ -951,7 +952,7 @@ ovn_as az2 ovn-nbctl --route-table=rtb2 lr-route-add lr12 10.10.10.0/24 192.168. ovn_as az2 ovn-nbctl --route-table=rtb3 lr-route-add lr12 10.10.10.0/24 192.168.0.12 # Create directly-connected route in VPC2 -ovn_as az2 ovn-nbctl lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bb:01 "192.168.0.1/24" +ovn_as az2 ovn-nbctl --wait=sb lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bb:01 "192.168.0.1/24" # Test direct routes from lr12 were learned to lr11 AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 192.168 | @@ -1077,7 +1078,7 @@ ovn_as az2 ovn-nbctl --route-table=rtb2 lr-route-add lr12 2001:db8:::/64 200 ovn_as az2 ovn-nbctl --route-table=rtb3 lr-route-add lr12 2001:db8:::/64 2001:db8:200::12 # Create directly-connected route in VPC2 -ovn_as az2 ovn-nbctl lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bb:01 "2001:db8:200::1/64" +ovn_as az2 ovn-nbctl --wait=sb lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bb:01 "2001:db8:200::1/64" # Test direct routes from lr12 were learned to lr11 AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 2001:db8:200 | @@ -1146,7 +1147,7 @@ for i in 1 2; do ovn-nbctl lrp-add $lr lrp-local-subnet 00:00:00:00:00:0$i 192.168.$i.1/24 ovn-nbctl list logical-router-static-route check ovn-nbctl lr-route-add $lr 10.0.0.0/24 192.168.$i.10 -check ovn-nbctl lr-route-add $lr 0.0.0.0/0 192.168.$i.11 +check ovn-nbctl --wait=sb lr-route-add $lr 0.0.0.0/0 192.168.$i.11 done AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep dst-ip | sort], [0], [dnl diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index d5be3be75..d1ea892ec 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -1156,7 +1156,7 @@ ovn-nbctl --stateless lr-nat-add DR dnat_and_snat 172.16.1.2 50.0.0.11 ovn-nbctl --stateless lr-nat-add CR dnat_and_snat 172.16.1.2 50.0.0.11 ovn-nbctl lr-nat-update-ext-ip DR dnat_and_snat 172.16.1.2 allowed_range -ovn-nbctl lr-nat-update-ext-ip CR dnat_and_snat 172.16.1.2 allowed_range +check ovn-nbctl --wait=sb lr-nat-update-ext-ip CR dnat_and_snat 172.16.1.2 allowed_range ovn-sbctl dump-flows DR > drflows5 AT_CAPTURE_FILE([drflows2]) @@ -4815,7 +4815,7 @@ AS_BOX([Checking that routable NAT flows are installed when gateway chassis exis check ovn-nbctl lr-nat-del ro1 check ovn-nbctl lr-nat-del ro2 check ovn-nbctl --add-route lr-nat-add ro1 dnat 10.0.0.100 192.168.1.100 -check ovn-nbctl --add-route lr-nat-add ro2 dnat 20.0.0.100 192.168.2.100 +check ovn-nbctl --wait=sb --add-route lr-nat-add ro2 dnat 20.0.0.100 192.168.2.100 check_lflows 1 @@ -4846,7 +4846,7 @@ check ovn-nbctl lr-nat-del ro1 check ovn-nbctl lr-nat-del ro2 check ovn-nbctl lr-nat-add ro1 dnat_and_snat 10.0.0.100 192.168.1.2 vm1 00:00:00:00:00:01 -check ovn-nbctl lr-nat-add ro2 dnat_and_snat 20.0.0.100 192.168.2.2 vm2 00:00:00:00:00:02 +check ovn-nbctl --wait=sb lr-nat-add ro2 dnat_and_snat 20.0.0.100 192.168.2.2 vm2 00:00:00:00:00:02 check_lflows 0 @@ -4991,7 +4991,7 @@ check ovn-nbctl lsp-add ls2 ls2-ro2 check ovn-nbctl lsp-set-type ls2-ro2 router check ovn-nbctl lsp-set-addresses ls2-ro2 router check ovn-nbctl lsp-set-options ls2-ro2
[ovs-dev] [PATCH ovn 2/4] system-tests: Make sure that the CT entries are sorted
Make sure the CT entries are sorted, otherwise the check sometimes fails depending on the order from OvS. Signed-off-by: Ales Musil --- tests/system-ovn.at | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 766a250e5..bfa323b5c 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -7833,9 +7833,9 @@ sleep 3s # Ensure conntrack entry is present and ct_label is set. AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ sed -e 's/zone=[[0-9]]*/zone=/' | \ -sed -e 's/labels=0x4d2[[0-9a-f]]*/labels=0x4d2/'], [0], [dnl -icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d2 +sed -e 's/labels=0x4d2[[0-9a-f]]*/labels=0x4d2/' | sort], [0], [dnl icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone= +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d2 ]) # Add a higher priority ACL with different label. @@ -7849,9 +7849,9 @@ sleep 3s # Ensure conntrack entry is updated with new ct_label is set. AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ sed -e 's/zone=[[0-9]]*/zone=/' | \ -sed -e 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3/'], [0], [dnl -icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d3 +sed -e 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3/' | sort], [0], [dnl icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone= +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d3 ]) OVS_APP_EXIT_AND_WAIT([ovn-controller]) @@ -7934,9 +7934,9 @@ sleep 3s # Ensure conntrack entry is present and ct_label is set. AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ sed -e 's/zone=[[0-9]]*/zone=/' | \ -sed -e 's/labels=0x4d2[[0-9a-f]]*/labels=0x4d2/'], [0], [dnl -icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d2 +sed -e 's/labels=0x4d2[[0-9a-f]]*/labels=0x4d2/' | sort], [0], [dnl icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone= +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d2 ]) # Add a higher priority ACL with different label. @@ -7950,9 +7950,9 @@ sleep 3s # Ensure conntrack entry is updated with new ct_label is set. AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ sed -e 's/zone=[[0-9]]*/zone=/' | \ -sed -e 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3/'], [0], [dnl -icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d3 +sed -e 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3/' | sort], [0], [dnl icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone= +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=,type=0,code=0),zone=,labels=0x4d3 ]) OVS_APP_EXIT_AND_WAIT([ovn-controller]) -- 2.41.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 1/4] tests: Check proper DP and port key
The test was assuming that the DP key and Port keys are
always fixed. Make sure that we use proper values for the
flow check.
Signed-off-by: Ales Musil
---
tests/ovn.at | 67 +++-
1 file changed, 35 insertions(+), 32 deletions(-)
diff --git a/tests/ovn.at b/tests/ovn.at
index 94f04d011..9bb4b7287 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -21871,8 +21871,8 @@ eth_dst=ff01
ip_src=$(ip_to_hex 10 0 0 10)
ip_dst=$(ip_to_hex 172 168 0 101)
send_icmp_packet 1 1 $eth_src $eth_dst $ip_src $ip_dst c4c9
00
-AT_CHECK([as hv1 ovs-ofctl dump-flows br-int metadata=0x$lr0_dp_key | awk
'/table=28, n_packets=1, n_bytes=45/{print $7" "$8}'],[0],[dnl
-priority=80,ip,reg15=0x3,metadata=0x3,nw_src=10.0.0.10 actions=drop
+AT_CHECK_UNQUOTED([as hv1 ovs-ofctl dump-flows br-int metadata=0x$lr0_dp_key |
awk '/table=28, n_packets=1, n_bytes=45/{print $7" "$8}'],[0],[dnl
+priority=80,ip,reg15=0x$lr0_public_dp_key,metadata=0x$lr0_dp_key,nw_src=10.0.0.10
actions=drop
])
# hv1 should remove the flow for the ACL with is_chassis_redirect check for
sw0-vir.
@@ -31432,6 +31432,9 @@ sw0_dpkey=$(fetch_column datapath_binding tunnel_key
external_ids:name=sw0)
sw0p1_dpkey=$(fetch_column port_binding tunnel_key logical_port=sw0-p1)
sw0p3_dpkey=$(fetch_column port_binding tunnel_key logical_port=sw0-p3)
+dp_key=$(printf "%x" $sw0_dpkey)
+port_key=$(printf "%x" $sw0p1_dpkey)
+
check_column '50:54:00:00:00:13' fdb mac
check_column $sw0_dpkey fdb dp_key
check_column $sw0p1_dpkey fdb port_key
@@ -31443,12 +31446,12 @@ as hv2 ovs-ofctl dump-flows br-int table=71 >
hv2_offlows_table71.txt
AT_CAPTURE_FILE([hv1_offlows_table71.txt])
AT_CAPTURE_FILE([hv2_offlows_table71.txt])
-AT_CHECK([cat hv1_offlows_table71.txt | grep -v NXST | cut -d ' ' -f8- |
sort], [0], [dnl
-priority=100,metadata=0x1,dl_dst=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG15[[]]
+AT_CHECK_UNQUOTED([cat hv1_offlows_table71.txt | grep -v NXST | cut -d ' '
-f8- | sort], [0], [dnl
+priority=100,metadata=0x$dp_key,dl_dst=50:54:00:00:00:13
actions=load:0x$port_key->NXM_NX_REG15[[]]
])
-AT_CHECK([cat hv2_offlows_table71.txt | grep -v NXST | cut -d ' ' -f8- |
sort], [0], [dnl
-priority=100,metadata=0x1,dl_dst=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG15[[]]
+AT_CHECK_UNQUOTED([cat hv2_offlows_table71.txt | grep -v NXST | cut -d ' '
-f8- | sort], [0], [dnl
+priority=100,metadata=0x$dp_key,dl_dst=50:54:00:00:00:13
actions=load:0x$port_key->NXM_NX_REG15[[]]
])
as hv1 ovs-ofctl dump-flows br-int table=72 > hv1_offlows_table72.txt
@@ -31456,12 +31459,12 @@ as hv2 ovs-ofctl dump-flows br-int table=72 >
hv2_offlows_table72.txt
AT_CAPTURE_FILE([hv1_offlows_table72.txt])
AT_CAPTURE_FILE([hv2_offlows_table72.txt])
-AT_CHECK([cat hv1_offlows_table72.txt | grep -v NXST | cut -d ' ' -f8- |
sort], [0], [dnl
-priority=100,reg14=0x1,metadata=0x1,dl_src=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG10[[8]]
+AT_CHECK_UNQUOTED([cat hv1_offlows_table72.txt | grep -v NXST | cut -d ' '
-f8- | sort], [0], [dnl
+priority=100,reg14=0x$port_key,metadata=0x$dp_key,dl_src=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG10[[8]]
])
-AT_CHECK([cat hv2_offlows_table72.txt | grep -v NXST | cut -d ' ' -f8- |
sort], [0], [dnl
-priority=100,reg14=0x1,metadata=0x1,dl_src=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG10[[8]]
+AT_CHECK_UNQUOTED([cat hv2_offlows_table72.txt | grep -v NXST | cut -d ' '
-f8- | sort], [0], [dnl
+priority=100,reg14=0x$port_key,metadata=0x$dp_key,dl_src=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG10[[8]]
])
# Create the logical port sw0-p4 and this should be claimed by
@@ -31480,12 +31483,12 @@ as hv3 ovs-ofctl dump-flows br-int table=72 >
hv3_offlows_table72.txt
AT_CAPTURE_FILE([hv3_offlows_table71.txt])
AT_CAPTURE_FILE([hv3_offlows_table72.txt])
-AT_CHECK([cat hv3_offlows_table71.txt | grep -v NXST | cut -d ' ' -f8- |
sort], [0], [dnl
-priority=100,metadata=0x1,dl_dst=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG15[[]]
+AT_CHECK_UNQUOTED([cat hv3_offlows_table71.txt | grep -v NXST | cut -d ' '
-f8- | sort], [0], [dnl
+priority=100,metadata=0x$dp_key,dl_dst=50:54:00:00:00:13
actions=load:0x$port_key->NXM_NX_REG15[[]]
])
-AT_CHECK([cat hv3_offlows_table72.txt | grep -v NXST | cut -d ' ' -f8- |
sort], [0], [dnl
-priority=100,reg14=0x1,metadata=0x1,dl_src=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG10[[8]]
+AT_CHECK_UNQUOTED([cat hv3_offlows_table72.txt | grep -v NXST | cut -d ' '
-f8- | sort], [0], [dnl
+priority=100,reg14=0x$port_key,metadata=0x$dp_key,dl_src=50:54:00:00:00:13
actions=load:0x1->NXM_NX_REG10[[8]]
])
# Use the src mac 50:54:00:00:00:14 instead of 50:54:00:00:00:03
@@ -31512,19 +31515,19 @@ as hv3 ovs-ofctl dump-flows br-int table=71 >
hv3_offlows_table71.txt
AT_CAPTURE_FILE([hv1_offlows_table71.txt])
AT_CAPTURE_FILE([hv2_offlows_table71.txt])
AT_CAPTURE_FILE([hv3_offlows_table71.txt])
-AT_CHECK([cat
[ovs-dev] [PATCH ovn 3/4] tests: Make sure the port group is not hardcoded
The port group name consists of DP key and NB PG name.
Use first PG that is avaiable to avoid flakes when
neither of the logical switches has DP key 2.
Signed-off-by: Ales Musil
---
tests/ovn.at | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/ovn.at b/tests/ovn.at
index 9bb4b7287..56ac17261 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -32487,9 +32487,9 @@ AT_CHECK([test $(ovs-ofctl dump-flows br-int table=46 |
grep conjunction | wc -l
for i in $(seq 1 10); do
# Delete and recreate the SB PG with same name and content.
-sb_pg_name=2_pg1 # dp key + NB pg name
+sb_pg_name=$(fetch_column port_group name | cut -d ' ' -f1)
sb_pg_uuid=$(fetch_column port_group _uuid name=$sb_pg_name)
-ports_=$(fetch_column port_group ports name=2_pg1)
+ports_=$(fetch_column port_group ports name=$sb_pg_name)
ports=${ports_/ /,}
AT_CHECK([ovn-sbctl destroy port_group $sb_pg_uuid -- create port_group
name=$sb_pg_name ports=$ports], [0], [ignore])
--
2.41.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] northd: Fix incorrect warning logs when handling port binding changes.
From: Numan Siddique
When changes to port bindings corresponding to router ports are
handled by northd engine node, incorrect warning logs (like below)
are logged.
northd|WARN|A port-binding for lrp0 is created but the LSP is not found.
Fix these warnings.
Since we are preserving the "lr_ports" data in the northd engine
across engine runs, it is important to store the port binding
address in 'op->sb' after the transaction is committed. And this
patch does that.
Fixes: 3b120ccf7f7c ("northd: Incremental processing of SB port_binding in
"northd" node.")
CC: Han Zhou
Signed-off-by: Numan Siddique
---
controller/binding.c | 75
controller/binding.h | 20 +
lib/ovn-util.c | 101 +++
lib/ovn-util.h | 21 +
northd/en-northd.c | 2 +-
northd/northd.c | 19 ++--
northd/northd.h | 3 +-
7 files changed, 141 insertions(+), 100 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index 3ac0c35df3..a521f2828e 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -756,7 +756,6 @@ static void binding_lport_clear_port_sec(struct
binding_lport *);
static bool binding_lport_update_port_sec(
struct binding_lport *, const struct sbrec_port_binding *);
-static char *get_lport_type_str(enum en_lport_type lport_type);
static bool ovs_iface_matches_lport_iface_id_ver(
const struct ovsrec_interface *,
const struct sbrec_port_binding *);
@@ -1055,80 +1054,6 @@ binding_dump_local_bindings(struct local_binding_data
*lbinding_data,
free(nodes);
}
-static bool
-is_lport_vif(const struct sbrec_port_binding *pb)
-{
-return !pb->type[0];
-}
-
-enum en_lport_type
-get_lport_type(const struct sbrec_port_binding *pb)
-{
-if (is_lport_vif(pb)) {
-if (pb->parent_port && pb->parent_port[0]) {
-return LP_CONTAINER;
-}
-return LP_VIF;
-} else if (!strcmp(pb->type, "patch")) {
-return LP_PATCH;
-} else if (!strcmp(pb->type, "chassisredirect")) {
-return LP_CHASSISREDIRECT;
-} else if (!strcmp(pb->type, "l3gateway")) {
-return LP_L3GATEWAY;
-} else if (!strcmp(pb->type, "localnet")) {
-return LP_LOCALNET;
-} else if (!strcmp(pb->type, "localport")) {
-return LP_LOCALPORT;
-} else if (!strcmp(pb->type, "l2gateway")) {
-return LP_L2GATEWAY;
-} else if (!strcmp(pb->type, "virtual")) {
-return LP_VIRTUAL;
-} else if (!strcmp(pb->type, "external")) {
-return LP_EXTERNAL;
-} else if (!strcmp(pb->type, "remote")) {
-return LP_REMOTE;
-} else if (!strcmp(pb->type, "vtep")) {
-return LP_VTEP;
-}
-
-return LP_UNKNOWN;
-}
-
-static char *
-get_lport_type_str(enum en_lport_type lport_type)
-{
-switch (lport_type) {
-case LP_VIF:
-return "VIF";
-case LP_CONTAINER:
-return "CONTAINER";
-case LP_VIRTUAL:
-return "VIRTUAL";
-case LP_PATCH:
-return "PATCH";
-case LP_CHASSISREDIRECT:
-return "CHASSISREDIRECT";
-case LP_L3GATEWAY:
-return "L3GATEWAY";
-case LP_LOCALNET:
-return "PATCH";
-case LP_LOCALPORT:
-return "LOCALPORT";
-case LP_L2GATEWAY:
-return "L2GATEWAY";
-case LP_EXTERNAL:
-return "EXTERNAL";
-case LP_REMOTE:
-return "REMOTE";
-case LP_VTEP:
-return "VTEP";
-case LP_UNKNOWN:
-return "UNKNOWN";
-}
-
-OVS_NOT_REACHED();
-}
-
void
set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb,
const struct sbrec_chassis *chassis_rec,
diff --git a/controller/binding.h b/controller/binding.h
index 235e5860d0..24bc840791 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -22,6 +22,7 @@
#include "openvswitch/hmap.h"
#include "openvswitch/uuid.h"
#include "openvswitch/list.h"
+# include "lib/ovn-util.h"
#include "sset.h"
#include "lport.h"
@@ -217,25 +218,6 @@ void port_binding_set_down(const struct sbrec_chassis
*chassis_rec,
const char *iface_id,
const struct uuid *pb_uuid);
-/* Corresponds to each Port_Binding.type. */
-enum en_lport_type {
-LP_UNKNOWN,
-LP_VIF,
-LP_CONTAINER,
-LP_PATCH,
-LP_L3GATEWAY,
-LP_LOCALNET,
-LP_LOCALPORT,
-LP_L2GATEWAY,
-LP_VTEP,
-LP_CHASSISREDIRECT,
-LP_VIRTUAL,
-LP_EXTERNAL,
-LP_REMOTE
-};
-
-enum en_lport_type get_lport_type(const struct sbrec_port_binding *);
-
/* This structure represents a logical port (or port binding)
* which is associated with 'struct local_binding'.
*
diff --git a/lib/ovn-util.c b/lib/ovn-util.c
index 080ad4c0ce..3a237b7fe3 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -1168,3 +1168,104 @@ encode_fqdn_string(const char *fqdn, size_t *len)
return encoded;
}
+
+static bool
+is_lport_vif(const struct
Re: [ovs-dev] [net-next v5 0/7] openvswitch: add drop reasons
Hello: This series was applied to netdev/net-next.git (main) by David S. Miller : On Fri, 11 Aug 2023 16:12:47 +0200 you wrote: > There is currently a gap in drop visibility in the openvswitch module. > This series tries to improve this by adding a new drop reason subsystem > for OVS. > > Apart from adding a new drop reasson subsystem and some common drop > reasons, this series takes Eric's preliminary work [1] on adding an > explicit drop action and integrates it into the same subsystem. > > [...] Here is the summary with links: - [net-next,v5,1/7] net: openvswitch: add last-action drop reason https://git.kernel.org/netdev/net-next/c/9d802da40b7c - [net-next,v5,2/7] net: openvswitch: add action error drop reason https://git.kernel.org/netdev/net-next/c/ec7bfb5e5a05 - [net-next,v5,3/7] net: openvswitch: add explicit drop action https://git.kernel.org/netdev/net-next/c/e7bc7db9ba46 - [net-next,v5,4/7] net: openvswitch: add meter drop reason https://git.kernel.org/netdev/net-next/c/f329d1bc1a45 - [net-next,v5,5/7] net: openvswitch: add misc error drop reasons https://git.kernel.org/netdev/net-next/c/43d95b30cf57 - [net-next,v5,6/7] selftests: openvswitch: add drop reason testcase https://git.kernel.org/netdev/net-next/c/aab1272f5dac - [net-next,v5,7/7] selftests: openvswitch: add explicit drop testcase https://git.kernel.org/netdev/net-next/c/4242029164d6 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
