Re: [ovs-dev] [PATCH ovn 4/4] rbac: Only allow relevant chassis to update BFD.
Bleep bloop. Greetings Mark Michelson, 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 Mark Michelson needs to sign off. Lines checked: 112, Warnings: 0, 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
Re: [ovs-dev] [PATCH ovn 3/4] rbac: Restrict IGMP_Group updates to relevant chassis.
Bleep bloop. Greetings Mark Michelson, 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 Mark Michelson needs to sign off. WARNING: Line is 88 characters long (recommended limit is 79) #152 FILE: controller/pinctrl.c:5446: pinctrl.igmp_group_has_chassis_name); Lines checked: 212, Warnings: 1, 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
Re: [ovs-dev] [PATCH ovn 2/4] rbac: Only allow relevant chassis to update service monitors.
Bleep bloop. Greetings Mark Michelson, 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 Mark Michelson needs to sign off. Lines checked: 131, Warnings: 0, 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
Re: [ovs-dev] [PATCH ovn 1/4] rbac: MAC_Bindings can only be updated by the inserting chassis.
Bleep bloop. Greetings Mark Michelson, 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 Mark Michelson needs to sign off. WARNING: Line is 80 characters long (recommended limit is 79) #225 FILE: ovn-sb.ovsschema:289: "refTable": "Datapath_Binding"}}}, Lines checked: 247, Warnings: 1, 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
Re: [ovs-dev] [PATCH ovn 1/4] rbac: MAC_Bindings can only be updated by the inserting chassis.
Whoops, I forgot to add
Reported-at: https://issues.redhat.com/browse/FDP-155
to this series. I can add this in v2. I'll wait to post a v2 until I get
some feedback on this series.
On 1/19/24 16:33, Mark Michelson wrote:
With this change, a chassis may only update MAC Binding records that it
has created. We achieve this by adding a "chassis_name" column to the
MAC_Binding table, and having the chassis insert its name into this
column when creating a new MAC_Binding. The "chassis_name" is now part
of the rbac_auth structure for the MAC_Binding table.
---
controller/pinctrl.c | 51
northd/ovn-northd.c | 2 +-
ovn-sb.ovsschema | 7 +++---
ovn-sb.xml | 3 +++
4 files changed, 45 insertions(+), 18 deletions(-)
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 4992eab08..a00cdceea 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -180,6 +180,7 @@ struct pinctrl {
bool mac_binding_can_timestamp;
bool fdb_can_timestamp;
bool dns_supports_ovn_owned;
+bool mac_binding_has_chassis_name;
};
static struct pinctrl pinctrl;
@@ -204,7 +205,8 @@ static void run_put_mac_bindings(
struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
struct ovsdb_idl_index *sbrec_port_binding_by_key,
-struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
+struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
+const struct sbrec_chassis *chassis)
OVS_REQUIRES(pinctrl_mutex);
static void wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn);
static void send_mac_binding_buffered_pkts(struct rconn *swconn)
@@ -3591,6 +3593,13 @@ pinctrl_update(const struct ovsdb_idl *idl, const char
*br_int_name)
notify_pinctrl_handler();
}
+bool mac_binding_has_chassis_name =
+sbrec_server_has_mac_binding_table_col_chassis_name(idl);
+if (mac_binding_has_chassis_name != pinctrl.mac_binding_has_chassis_name) {
+pinctrl.mac_binding_has_chassis_name = mac_binding_has_chassis_name;
+notify_pinctrl_handler();
+}
+
ovs_mutex_unlock(_mutex);
}
@@ -3621,7 +3630,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
ovs_mutex_lock(_mutex);
run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
sbrec_port_binding_by_key,
- sbrec_mac_binding_by_lport_ip);
+ sbrec_mac_binding_by_lport_ip,
+ chassis);
run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
sbrec_port_binding_by_key, chassis);
send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath,
@@ -4285,7 +4295,8 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn *ovnsb_idl_txn,
const char *logical_port,
const struct sbrec_datapath_binding *dp,
struct eth_addr ea, const char *ip,
- bool update_only)
+ bool update_only,
+ const struct sbrec_chassis *chassis)
{
/* Convert ethernet argument to string form for database. */
char mac_string[ETH_ADDR_STRLEN + 1];
@@ -4302,6 +4313,9 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn *ovnsb_idl_txn,
sbrec_mac_binding_set_logical_port(b, logical_port);
sbrec_mac_binding_set_ip(b, ip);
sbrec_mac_binding_set_datapath(b, dp);
+if (pinctrl.mac_binding_has_chassis_name) {
+sbrec_mac_binding_set_chassis_name(b, chassis->name);
+}
}
if (strcmp(b->mac, mac_string)) {
@@ -4323,7 +4337,8 @@ send_garp_locally(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
const struct hmap *local_datapaths,
const struct sbrec_port_binding *in_pb,
- struct eth_addr ea, ovs_be32 ip)
+ struct eth_addr ea, ovs_be32 ip,
+ const struct sbrec_chassis *chassis)
{
if (!ovnsb_idl_txn) {
return;
@@ -4351,7 +4366,7 @@ send_garp_locally(struct ovsdb_idl_txn *ovnsb_idl_txn,
ip_format_masked(ip, OVS_BE32_MAX, _s);
mac_binding_add_to_sb(ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip,
remote->logical_port, remote->datapath,
- ea, ds_cstr(_s), update_only);
+ ea, ds_cstr(_s), update_only, chassis);
ds_destroy(_s);
}
}
@@ -4361,7 +4376,8 @@ run_put_mac_binding(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
struct ovsdb_idl_index *sbrec_port_binding_by_key,
struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
-const struct
[ovs-dev] [PATCH ovn 4/4] rbac: Only allow relevant chassis to update BFD.
This adds a new "chassis_name" column to the BFD table. ovn-northd sets
this to the logical port's chassis name when creating the BFD record.
RBAC has been updated so that chassis may only update their own records.
---
northd/northd.c | 9 -
northd/ovn-northd.c | 2 +-
ovn-sb.ovsschema| 5 +++--
ovn-sb.xml | 4
4 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/northd/northd.c b/northd/northd.c
index 9821fcef5..793fc13f5 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -10808,6 +10808,7 @@ build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
nbrec_bfd_set_status(nb_bt, "admin_down");
}
+struct ovn_port *op = ovn_port_find(lr_ports, nb_bt->logical_port);
bfd_e = bfd_port_lookup(_only, nb_bt->logical_port, nb_bt->dst_ip);
if (!bfd_e) {
int udp_src = bfd_get_unused_port(bfd_src_ports);
@@ -10821,6 +10822,9 @@ build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
sbrec_bfd_set_disc(sb_bt, 1 + random_uint32());
sbrec_bfd_set_src_port(sb_bt, udp_src);
sbrec_bfd_set_status(sb_bt, nb_bt->status);
+if (op && op->sb && op->sb->chassis) {
+sbrec_bfd_set_chassis_name(sb_bt, op->sb->chassis->name);
+}
int min_tx = nb_bt->n_min_tx ? nb_bt->min_tx[0] : BFD_DEF_MINTX;
sbrec_bfd_set_min_tx(sb_bt, min_tx);
@@ -10839,6 +10843,10 @@ build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
}
}
build_bfd_update_sb_conf(nb_bt, bfd_e->sb_bt);
+if (op && op->sb && op->sb->chassis &&
+strcmp(op->sb->chassis->name, sb_bt->chassis_name)) {
+sbrec_bfd_set_chassis_name(sb_bt, op->sb->chassis->name);
+}
hmap_remove(_only, _e->hmap_node);
bfd_e->ref = false;
@@ -10847,7 +10855,6 @@ build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
hmap_insert(bfd_connections, _e->hmap_node, hash);
}
-struct ovn_port *op = ovn_port_find(lr_ports, nb_bt->logical_port);
if (op) {
op->has_bfd = true;
}
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 8f70d5241..c11744b3f 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -122,7 +122,7 @@ static const char *rbac_igmp_group_auth[] =
static const char *rbac_igmp_group_update[] =
{"address", "chassis", "datapath", "ports"};
static const char *rbac_bfd_auth[] =
-{""};
+{"chassis_name"};
static const char *rbac_bfd_update[] =
{"status"};
diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
index 0e601f4e3..26c9ae75f 100644
--- a/ovn-sb.ovsschema
+++ b/ovn-sb.ovsschema
@@ -1,7 +1,7 @@
{
"name": "OVN_Southbound",
"version": "20.33.0",
-"cksum": "3042447672 31328",
+"cksum": "4078434013 31380",
"tables": {
"SB_Global": {
"columns": {
@@ -579,7 +579,8 @@
"min": 0, "max": "unlimited"}},
"options": {
"type": {"key": "string", "value": "string",
- "min": 0, "max": "unlimited"}}},
+ "min": 0, "max": "unlimited"}},
+"chassis_name": {"type": "string"}},
"indexes": [["logical_port", "dst_ip", "src_port", "disc"]],
"isRoot": true},
"FDB": {
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 833e53114..629c78095 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -4992,6 +4992,10 @@ tcp.flags = RST;
receiving system in Asynchronous mode.
+
+The name of the chassis where the logical port is bound.
+
+
Reserved for future use.
--
2.40.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 3/4] rbac: Restrict IGMP_Group updates to relevant chassis.
RBAC did not restrict which chassis could update IGMP_Groups. With this
change, we add a new "chassis_name" column to IGMP_Group.
This may seem odd since there is already a "chassis" column in
IGMP_Group. But RBAC specifically works by string matching based on the
certificate common name. Therefore, we need to have a chassis_name
string column instead of a chassis UUID column.
---
controller/ip-mcast.c | 20 ++--
controller/ip-mcast.h | 6 --
controller/pinctrl.c | 16 +---
northd/ovn-northd.c | 2 +-
ovn-sb.ovsschema | 7 ---
ovn-sb.xml| 5 +
6 files changed, 41 insertions(+), 15 deletions(-)
diff --git a/controller/ip-mcast.c b/controller/ip-mcast.c
index a870fb29e..6150cece0 100644
--- a/controller/ip-mcast.c
+++ b/controller/ip-mcast.c
@@ -38,7 +38,8 @@ static struct sbrec_igmp_group *
igmp_group_create_(struct ovsdb_idl_txn *idl_txn,
const char *addr_str,
const struct sbrec_datapath_binding *datapath,
- const struct sbrec_chassis *chassis);
+ const struct sbrec_chassis *chassis,
+ bool igmp_group_has_chassis_name);
struct ovsdb_idl_index *
igmp_group_index_create(struct ovsdb_idl *idl)
@@ -86,7 +87,8 @@ struct sbrec_igmp_group *
igmp_group_create(struct ovsdb_idl_txn *idl_txn,
const struct in6_addr *address,
const struct sbrec_datapath_binding *datapath,
- const struct sbrec_chassis *chassis)
+ const struct sbrec_chassis *chassis,
+ bool igmp_group_has_chassis_name)
{
char addr_str[INET6_ADDRSTRLEN];
@@ -94,16 +96,18 @@ igmp_group_create(struct ovsdb_idl_txn *idl_txn,
return NULL;
}
-return igmp_group_create_(idl_txn, addr_str, datapath, chassis);
+return igmp_group_create_(idl_txn, addr_str, datapath, chassis,
+ igmp_group_has_chassis_name);
}
struct sbrec_igmp_group *
igmp_mrouter_create(struct ovsdb_idl_txn *idl_txn,
const struct sbrec_datapath_binding *datapath,
-const struct sbrec_chassis *chassis)
+const struct sbrec_chassis *chassis,
+bool igmp_group_has_chassis_name)
{
return igmp_group_create_(idl_txn, OVN_IGMP_GROUP_MROUTERS, datapath,
- chassis);
+ chassis, igmp_group_has_chassis_name);
}
void
@@ -249,13 +253,17 @@ static struct sbrec_igmp_group *
igmp_group_create_(struct ovsdb_idl_txn *idl_txn,
const char *addr_str,
const struct sbrec_datapath_binding *datapath,
- const struct sbrec_chassis *chassis)
+ const struct sbrec_chassis *chassis,
+ bool igmp_group_has_chassis_name)
{
struct sbrec_igmp_group *g = sbrec_igmp_group_insert(idl_txn);
sbrec_igmp_group_set_address(g, addr_str);
sbrec_igmp_group_set_datapath(g, datapath);
sbrec_igmp_group_set_chassis(g, chassis);
+if (igmp_group_has_chassis_name) {
+sbrec_igmp_group_set_chassis_name(g, chassis->name);
+}
return g;
}
diff --git a/controller/ip-mcast.h b/controller/ip-mcast.h
index 326f39db1..a2d531097 100644
--- a/controller/ip-mcast.h
+++ b/controller/ip-mcast.h
@@ -39,11 +39,13 @@ struct sbrec_igmp_group *igmp_group_create(
struct ovsdb_idl_txn *idl_txn,
const struct in6_addr *address,
const struct sbrec_datapath_binding *datapath,
-const struct sbrec_chassis *chassis);
+const struct sbrec_chassis *chassis,
+bool igmp_group_has_chassis_name);
struct sbrec_igmp_group *igmp_mrouter_create(
struct ovsdb_idl_txn *idl_txn,
const struct sbrec_datapath_binding *datapath,
-const struct sbrec_chassis *chassis);
+const struct sbrec_chassis *chassis,
+bool igmp_group_has_chassis_name);
void igmp_group_update_ports(const struct sbrec_igmp_group *g,
struct ovsdb_idl_index *datapaths,
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index a00cdceea..1e3df02af 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -181,6 +181,7 @@ struct pinctrl {
bool fdb_can_timestamp;
bool dns_supports_ovn_owned;
bool mac_binding_has_chassis_name;
+bool igmp_group_has_chassis_name;
};
static struct pinctrl pinctrl;
@@ -3600,6 +3601,13 @@ pinctrl_update(const struct ovsdb_idl *idl, const char
*br_int_name)
notify_pinctrl_handler();
}
+bool igmp_group_has_chassis_name =
+sbrec_server_has_igmp_group_table_col_chassis_name(idl);
+if (igmp_group_has_chassis_name != pinctrl.igmp_group_has_chassis_name) {
+pinctrl.igmp_group_has_chassis_name = igmp_group_has_chassis_name;
+notify_pinctrl_handler();
+}
+
ovs_mutex_unlock(_mutex);
}
@@ -5417,8 +5425,9 @@ ip_mcast_sync(struct ovsdb_idl_txn
[ovs-dev] [PATCH ovn 1/4] rbac: MAC_Bindings can only be updated by the inserting chassis.
With this change, a chassis may only update MAC Binding records that it
has created. We achieve this by adding a "chassis_name" column to the
MAC_Binding table, and having the chassis insert its name into this
column when creating a new MAC_Binding. The "chassis_name" is now part
of the rbac_auth structure for the MAC_Binding table.
---
controller/pinctrl.c | 51
northd/ovn-northd.c | 2 +-
ovn-sb.ovsschema | 7 +++---
ovn-sb.xml | 3 +++
4 files changed, 45 insertions(+), 18 deletions(-)
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 4992eab08..a00cdceea 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -180,6 +180,7 @@ struct pinctrl {
bool mac_binding_can_timestamp;
bool fdb_can_timestamp;
bool dns_supports_ovn_owned;
+bool mac_binding_has_chassis_name;
};
static struct pinctrl pinctrl;
@@ -204,7 +205,8 @@ static void run_put_mac_bindings(
struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
struct ovsdb_idl_index *sbrec_port_binding_by_key,
-struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
+struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
+const struct sbrec_chassis *chassis)
OVS_REQUIRES(pinctrl_mutex);
static void wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn);
static void send_mac_binding_buffered_pkts(struct rconn *swconn)
@@ -3591,6 +3593,13 @@ pinctrl_update(const struct ovsdb_idl *idl, const char
*br_int_name)
notify_pinctrl_handler();
}
+bool mac_binding_has_chassis_name =
+sbrec_server_has_mac_binding_table_col_chassis_name(idl);
+if (mac_binding_has_chassis_name != pinctrl.mac_binding_has_chassis_name) {
+pinctrl.mac_binding_has_chassis_name = mac_binding_has_chassis_name;
+notify_pinctrl_handler();
+}
+
ovs_mutex_unlock(_mutex);
}
@@ -3621,7 +3630,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
ovs_mutex_lock(_mutex);
run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
sbrec_port_binding_by_key,
- sbrec_mac_binding_by_lport_ip);
+ sbrec_mac_binding_by_lport_ip,
+ chassis);
run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
sbrec_port_binding_by_key, chassis);
send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath,
@@ -4285,7 +4295,8 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn *ovnsb_idl_txn,
const char *logical_port,
const struct sbrec_datapath_binding *dp,
struct eth_addr ea, const char *ip,
- bool update_only)
+ bool update_only,
+ const struct sbrec_chassis *chassis)
{
/* Convert ethernet argument to string form for database. */
char mac_string[ETH_ADDR_STRLEN + 1];
@@ -4302,6 +4313,9 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn *ovnsb_idl_txn,
sbrec_mac_binding_set_logical_port(b, logical_port);
sbrec_mac_binding_set_ip(b, ip);
sbrec_mac_binding_set_datapath(b, dp);
+if (pinctrl.mac_binding_has_chassis_name) {
+sbrec_mac_binding_set_chassis_name(b, chassis->name);
+}
}
if (strcmp(b->mac, mac_string)) {
@@ -4323,7 +4337,8 @@ send_garp_locally(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
const struct hmap *local_datapaths,
const struct sbrec_port_binding *in_pb,
- struct eth_addr ea, ovs_be32 ip)
+ struct eth_addr ea, ovs_be32 ip,
+ const struct sbrec_chassis *chassis)
{
if (!ovnsb_idl_txn) {
return;
@@ -4351,7 +4366,7 @@ send_garp_locally(struct ovsdb_idl_txn *ovnsb_idl_txn,
ip_format_masked(ip, OVS_BE32_MAX, _s);
mac_binding_add_to_sb(ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip,
remote->logical_port, remote->datapath,
- ea, ds_cstr(_s), update_only);
+ ea, ds_cstr(_s), update_only, chassis);
ds_destroy(_s);
}
}
@@ -4361,7 +4376,8 @@ run_put_mac_binding(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
struct ovsdb_idl_index *sbrec_port_binding_by_key,
struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
-const struct mac_binding *mb)
+const struct mac_binding *mb,
+const struct sbrec_chassis *chassis)
{
/* Convert logical datapath and logical port key into lport. */
const struct sbrec_port_binding *pb = lport_lookup_by_key(
@@ -4384,7 +4400,7 @@
[ovs-dev] [PATCH ovn 2/4] rbac: Only allow relevant chassis to update service monitors.
Service monitors already had the restriction that chassis could not
insert or delete records. However, there was nothing restricting chassis
from updating records for service monitors that are relevant to other
chassis.
This change adds a new "chassis_name" column to the Service_Monitor
table. ovn-northd will set this column to the chassis on which the
relevant logical port is bound. This way, only that particular chassis
can update the status of the service monitor.
---
northd/northd.c | 19 +--
northd/ovn-northd.c | 2 +-
ovn-sb.ovsschema| 5 +++--
ovn-sb.xml | 4
4 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/northd/northd.c b/northd/northd.c
index 952f8200d..9821fcef5 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3841,13 +3841,19 @@ static struct service_monitor_info *
create_or_get_service_mon(struct ovsdb_idl_txn *ovnsb_txn,
struct hmap *monitor_map,
const char *ip, const char *logical_port,
- uint16_t service_port, const char *protocol)
+ uint16_t service_port, const char *protocol,
+ const char *chassis_name)
{
struct service_monitor_info *mon_info =
get_service_mon(monitor_map, ip, logical_port, service_port,
protocol);
if (mon_info) {
+if (chassis_name && strcmp(mon_info->sbrec_mon->chassis_name,
+ chassis_name)) {
+sbrec_service_monitor_set_chassis_name(mon_info->sbrec_mon,
+ chassis_name);
+}
return mon_info;
}
@@ -3862,6 +3868,9 @@ create_or_get_service_mon(struct ovsdb_idl_txn *ovnsb_txn,
sbrec_service_monitor_set_port(sbrec_mon, service_port);
sbrec_service_monitor_set_logical_port(sbrec_mon, logical_port);
sbrec_service_monitor_set_protocol(sbrec_mon, protocol);
+if (chassis_name) {
+sbrec_service_monitor_set_chassis_name(sbrec_mon, chassis_name);
+}
mon_info = xzalloc(sizeof *mon_info);
mon_info->sbrec_mon = sbrec_mon;
hmap_insert(monitor_map, _info->hmap_node, hash);
@@ -3904,12 +3913,18 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn,
protocol = "tcp";
}
+const char *chassis_name = NULL;
+if (op->sb && op->sb->chassis) {
+chassis_name = op->sb->chassis->name;
+}
+
struct service_monitor_info *mon_info =
create_or_get_service_mon(ovnsb_txn, monitor_map,
backend->ip_str,
backend_nb->logical_port,
backend->port,
- protocol);
+ protocol,
+ chassis_name);
ovs_assert(mon_info);
sbrec_service_monitor_set_options(
mon_info->sbrec_mon, _vip_nb->lb_health_check->options);
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index f51dbecb4..ef580b561 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -114,7 +114,7 @@ static const char *rbac_mac_binding_update[] =
{"logical_port", "ip", "mac", "datapath", "timestamp"};
static const char *rbac_svc_monitor_auth[] =
-{""};
+{"chassis_name"};
static const char *rbac_svc_monitor_auth_update[] =
{"status"};
static const char *rbac_igmp_group_auth[] =
diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
index 9cf91c8f7..563d1a215 100644
--- a/ovn-sb.ovsschema
+++ b/ovn-sb.ovsschema
@@ -1,7 +1,7 @@
{
"name": "OVN_Southbound",
-"version": "20.31.0",
-"cksum": "3395536250 31224",
+"version": "20.32.0",
+"cksum": "482767101 31276",
"tables": {
"SB_Global": {
"columns": {
@@ -510,6 +510,7 @@
"logical_port": {"type": "string"},
"src_mac": {"type": "string"},
"src_ip": {"type": "string"},
+"chassis_name": {"type": "string"},
"status": {
"type": {"key": {"type": "string",
"enum": ["set", ["online", "offline", "error"]]},
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 411074083..046913201 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -4818,6 +4818,10 @@ tcp.flags = RST;
Source IPv4 address to use in the service monitor packet.
+
+The name of the chassis where the logical port is bound.
+
+
The interval, in seconds, between service monitor checks.
--
2.40.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] compiler: Fix errors in Clang 17 ubsan checks.
The github ci failed with entries like this in the logs:
+tcp,orig=(src=10.1.1.4,dst=140.82.113.22,sport=,dport=),reply=(src=140.82.113.22,dst=10.1.1.4,sport=,dport=),protoinfo=(state=)
+tcp,orig=(src=10.1.1.4,dst=168.63.129.16,sport=,dport=),reply=(src=168.63.129.16,dst=10.1.1.4,sport=,dport=),protoinfo=(state=)
+tcp,orig=(src=10.1.1.4,dst=20.22.166.15,sport=,dport=),reply=(src=20.22.166.15,dst=10.1.1.4,sport=,dport=),protoinfo=(state=)
Those are github IP's, don't know how github traffic got into the tests.
-M
On Fri, Jan 19, 2024 at 11:13 AM Mike Pattrick wrote:
>
> This patch attempts to fix a large number of ubsan error messages that
> take the following form:
>
> lib/netlink-notifier.c:237:13: runtime error: call to function
> route_table_change through pointer to incorrect function type
> 'void (*)(const void *, void *)'
>
> In Clang 17 the undefined behaviour sanatizer check for function
> pointers was enabled by default, whereas it was previously disabled
> while compiling C code. These warnings are a false positive in the case
> of OVS, as our macros already check to make sure the function parameter
> is the correct size.
>
> So that check is disabled in the single function that is causing all of
> the errors.
>
> Signed-off-by: Mike Pattrick
> ---
> include/openvswitch/compiler.h | 11 +++
> lib/ovs-rcu.c | 1 +
> 2 files changed, 12 insertions(+)
>
> diff --git a/include/openvswitch/compiler.h b/include/openvswitch/compiler.h
> index 878c5c6a7..59a835f15 100644
> --- a/include/openvswitch/compiler.h
> +++ b/include/openvswitch/compiler.h
> @@ -69,6 +69,17 @@
> #define OVS_UNLIKELY(CONDITION) (!!(CONDITION))
> #endif
>
> +/* Clang 17's implementation of ubsan enables checking that function pointers
> + * match the type of the called function. This currently breaks ovs-rcu,
> which
> + * calls multiple different types of callbacks via a generic void *(void*)
> + * function pointer type. This macro enables disabling that check for
> specific
> + * functions. */
> +#if __clang__ && __has_feature(undefined_behavior_sanitizer)
> +#define OVS_FUNCTION_PTR __attribute__((no_sanitize("function")))
> +#else
> +#define OVS_FUNCTION_PTR
> +#endif
> +
> #if __has_feature(c_thread_safety_attributes)
> /* "clang" annotations for thread safety check.
> *
> diff --git a/lib/ovs-rcu.c b/lib/ovs-rcu.c
> index 9e07d9bab..56608aa46 100644
> --- a/lib/ovs-rcu.c
> +++ b/lib/ovs-rcu.c
> @@ -327,6 +327,7 @@ ovsrcu_postpone__(void (*function)(void *aux), void *aux)
> }
>
> static bool
> +OVS_FUNCTION_PTR
> ovsrcu_call_postponed(void)
> {
> struct ovsrcu_cbset *cbset;
> --
> 2.39.3
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.
On Fri, Jan 19, 2024 at 01:40:03PM +0100, David Marchand wrote:
> On Fri, Jan 19, 2024 at 1:20 PM Simon Horman wrote:
> >
> > On Thu, Jan 18, 2024 at 02:00:18PM +0100, David Marchand wrote:
> > > Seen in GHA recently.
> > > Unit tests are checking conntracks relating to a destination ip address
> > > but the FORMAT_CT macro is not strict enough and would match unrelated
> > > conntracks too.
> > >
> > > Example:
> > > 148. system-traffic.at:6432: testing conntrack - DNAT with
> > > additional SNAT ...
> > > [...]
> > > ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack |
> > > grep "dst=10.1.1.1" |
> > > sed -e 's/port=[0-9]*/port=/g'
> > > -e 's/id=[0-9]*/id=/g'
> > > -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
> > > [...]
> > > @@ -1,2 +1,7 @@
> > > tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
> > > +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> > > +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
> > > +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> > > +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> > > +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> > >
> > > Fixes: 07659514c3c1 ("Add support for connection tracking.")
> > > Signed-off-by: David Marchand
> > > ---
> > > tests/system-common-macros.at | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
> > > index 01ebe364ee..07be29f673 100644
> > > --- a/tests/system-common-macros.at
> > > +++ b/tests/system-common-macros.at
> > > @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed
> > > 's/csum:.*/csum: /'])
> > > # and limit the output to the rows containing 'ip-addr'.
> > > #
> > > m4_define([FORMAT_CT],
> > > -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e
> > > 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' |
> > > sort | uniq]])
> > > +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e
> > > 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' |
> > > sort | uniq]])
> > >
> > > # NETNS_DAEMONIZE([namespace], [command], [pidfile])
> > > #
> >
> > Sorry, I feel I mist be missing something very obvious, but
> > I'm unsure why the match is on "dst=$1\>". I would have thought
> > the match would be "dst=$1," instead.
>
> \> matches the end of a word.
> Using , as a delimiter works too in this case.
Thanks, now I understand :)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 0/8] python: Miscelaneous flow parsing fixes.
On Wed, Jan 17, 2024 at 07:33:40PM +, Simon Horman wrote: > On Wed, Jan 17, 2024 at 12:18:49PM +0100, Adrian Moreno wrote: > > This series contains some miscelaneous fixes in the flow parsing > > library. > > > > V4: > > - Rebased and added a test to verify the length of the kv_lists > > V3: > > - Fixed pytests and added more tests for each fix. > > V2: > > - Rebased and dropped first patch. > > > > Adrian Moreno (8): > > python: ovs: flow: Fix typo in n_packets. > > python: tests: Add info and key tests for OFPFlows. > > python: ovs: flow: Add sample to nested actions. > > python: ovs: flow: Add dp hash and meter actions. > > python: tests: Refactor test_odp section testing. > > python: ovs: flow: Add idle_age to openflow flows. > > python: ovs: flow: Make check_pkt_len action a list. > > python: ovs: flow: Add meter_id to controller. > > Thanks Adrian, > > applied as: > - python: ovs: flow: Add meter_id to controller. > https://github.com/openvswitch/ovs/commit/253d90075874 > - python: ovs: flow: Make check_pkt_len action a list. > https://github.com/openvswitch/ovs/commit/ea44cafae235 > - python: ovs: flow: Add idle_age to openflow flows. > https://github.com/openvswitch/ovs/commit/32f6737b5cb1 > - python: tests: Refactor test_odp section testing. > https://github.com/openvswitch/ovs/commit/e72b7b6f174f > - python: ovs: flow: Add dp hash and meter actions. > https://github.com/openvswitch/ovs/commit/5e45091ea888 > - python: ovs: flow: Add sample to nested actions. > https://github.com/openvswitch/ovs/commit/ab7d089612cd > - python: tests: Add info and key tests for OFPFlows. > https://github.com/openvswitch/ovs/commit/9ef49ca85b9b > - python: ovs: flow: Fix typo in n_packets. > https://github.com/openvswitch/ovs/commit/6766424d > > As a follow-up I plan to backport this series as appropriate. Backport to branch-3.2: - python: ovs: flow: Add meter_id to controller. https://github.com/openvswitch/ovs/commit/f6757eb214f5 - python: ovs: flow: Make check_pkt_len action a list. https://github.com/openvswitch/ovs/commit/dae1ffc17b49 - python: ovs: flow: Add idle_age to openflow flows. https://github.com/openvswitch/ovs/commit/699d26d425ac - python: tests: Refactor test_odp section testing. https://github.com/openvswitch/ovs/commit/5bef199078e9 - python: ovs: flow: Add dp hash and meter actions. https://github.com/openvswitch/ovs/commit/e3f2eca15ddf - python: ovs: flow: Add sample to nested actions. https://github.com/openvswitch/ovs/commit/1072d0d22146 - python: tests: Add info and key tests for OFPFlows. https://github.com/openvswitch/ovs/commit/065bdb3e15e0 - python: ovs: flow: Fix typo in n_packets. https://github.com/openvswitch/ovs/commit/4374b1e64eb7 Backport to branch-3.1: - python: ovs: flow: Add meter_id to controller. https://github.com/openvswitch/ovs/commit/7bb4f13a3757 - python: ovs: flow: Make check_pkt_len action a list. https://github.com/openvswitch/ovs/commit/41b2ae516552 - python: ovs: flow: Add idle_age to openflow flows. https://github.com/openvswitch/ovs/commit/d0fc8a289281 - python: tests: Refactor test_odp section testing. https://github.com/openvswitch/ovs/commit/3e2791f8c6f2 - python: ovs: flow: Add dp hash and meter actions. https://github.com/openvswitch/ovs/commit/df59f9a225ce - python: ovs: flow: Add sample to nested actions. https://github.com/openvswitch/ovs/commit/7958e50a1e8c - python: tests: Add info and key tests for OFPFlows. https://github.com/openvswitch/ovs/commit/fc0fb041afd2 - python: ovs: flow: Fix typo in n_packets. https://github.com/openvswitch/ovs/commit/1d00d1da6f09 Backport to branch-3.0 with dependencies: - python: ovs: flow: Add meter_id to controller. https://github.com/openvswitch/ovs/commit/53b2c486c87b - python: ovs: flow: Make check_pkt_len action a list. https://github.com/openvswitch/ovs/commit/1eb9d67f9466 - python: ovs: flow: Add idle_age to openflow flows. https://github.com/openvswitch/ovs/commit/3cdd6943a0f0 - python: tests: Refactor test_odp section testing. https://github.com/openvswitch/ovs/commit/48e9e68cf71c - python: ovs: flow: Add dp hash and meter actions. https://github.com/openvswitch/ovs/commit/e1990bab95cc - python: ovs: flow: Add sample to nested actions. https://github.com/openvswitch/ovs/commit/7e46b2fb1011 - python: tests: Add info and key tests for OFPFlows. https://github.com/openvswitch/ovs/commit/53a346e62338 - python: ovs: flow: Fix typo in n_packets. https://github.com/openvswitch/ovs/commit/3539ceb46987 - python: Support case-insensitive OpenFlow actions. https://github.com/openvswitch/ovs/commit/c1253ba877a2 - python: Return list of actions for odp action clone. https://github.com/openvswitch/ovs/commit/063d784d7923 - python: Make key-value matching strict by default. https://github.com/openvswitch/ovs/commit/120f3dc4113a - python: Add explicit decoders for all ofp actions.
[ovs-dev] [PATCH] compiler: Fix errors in Clang 17 ubsan checks.
This patch attempts to fix a large number of ubsan error messages that
take the following form:
lib/netlink-notifier.c:237:13: runtime error: call to function
route_table_change through pointer to incorrect function type
'void (*)(const void *, void *)'
In Clang 17 the undefined behaviour sanatizer check for function
pointers was enabled by default, whereas it was previously disabled
while compiling C code. These warnings are a false positive in the case
of OVS, as our macros already check to make sure the function parameter
is the correct size.
So that check is disabled in the single function that is causing all of
the errors.
Signed-off-by: Mike Pattrick
---
include/openvswitch/compiler.h | 11 +++
lib/ovs-rcu.c | 1 +
2 files changed, 12 insertions(+)
diff --git a/include/openvswitch/compiler.h b/include/openvswitch/compiler.h
index 878c5c6a7..59a835f15 100644
--- a/include/openvswitch/compiler.h
+++ b/include/openvswitch/compiler.h
@@ -69,6 +69,17 @@
#define OVS_UNLIKELY(CONDITION) (!!(CONDITION))
#endif
+/* Clang 17's implementation of ubsan enables checking that function pointers
+ * match the type of the called function. This currently breaks ovs-rcu, which
+ * calls multiple different types of callbacks via a generic void *(void*)
+ * function pointer type. This macro enables disabling that check for specific
+ * functions. */
+#if __clang__ && __has_feature(undefined_behavior_sanitizer)
+#define OVS_FUNCTION_PTR __attribute__((no_sanitize("function")))
+#else
+#define OVS_FUNCTION_PTR
+#endif
+
#if __has_feature(c_thread_safety_attributes)
/* "clang" annotations for thread safety check.
*
diff --git a/lib/ovs-rcu.c b/lib/ovs-rcu.c
index 9e07d9bab..56608aa46 100644
--- a/lib/ovs-rcu.c
+++ b/lib/ovs-rcu.c
@@ -327,6 +327,7 @@ ovsrcu_postpone__(void (*function)(void *aux), void *aux)
}
static bool
+OVS_FUNCTION_PTR
ovsrcu_call_postponed(void)
{
struct ovsrcu_cbset *cbset;
--
2.39.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] checkpatch.py: Port checkpatch related changes from the OVS repo.
Dumitru Ceara writes:
> On 1/17/24 21:40, Mark Michelson wrote:
>> On 1/16/24 11:23, Numan Siddique wrote:
>>> On Tue, Jan 16, 2024 at 8:09 AM Dumitru Ceara wrote:
This picks up the following OVS changes:
00d3d4a7d375 ("checkpatch: Avoid catastrophic backtracking.")
d25c6bd8df37 ("checkpatch: Reorganize flagged words using a list.")
9a50170a805a ("checkpatch: Add suggestions to the spell checker.")
799f697e51ec ("checkpatch: Print subject field if misspelled or
missing.")
1b8fa4a66aa4 ("checkpatch: Add checks for the subject line.")
bf843fd439b2 ("checkpatch: Don't spell check Fixes tag.")
74bfe3701407 ("checkpatch: Add argument to skip committer signoff
check.")
21c61243fb75 ("checkpatch: Fix personal word list storage.")
915b97971d58 ("checkpatch.py: Load codespell dictionary.")
Signed-off-by: Dumitru Ceara
>>>
>>> Thanks for the patch.
>>>
>>> Acked-by: Numan Siddique
>>>
>>> Numan
>>
>> Thank you Dumitru and Numan.
>>
>> I merged this to main and all OVN branches back to 22.12.
>>
>
> Thanks, Numan and Mark!
>
>> I'm curious about something. Since we have the OVS submodule, and there
>> are no OVN-specific additions to checkpatch (that I'm aware of anyway),
>> would it make sense to remove the checkpatch from OVN and just rely on
>> the OVS submodule's checkpatch? This way, whenever we do a submodule
>> bump, we'd automatically pull in the checkpatch modifications.
>>
>> What do you think?
>>
>
> +1 I think we do have one OVN-specific change to checkpatch but I'm sure
> we can figure out how to have it ported to the OVS one if needed:
>
> https://github.com/ovn-org/ovn/commit/04a5527c6a8657b682450f71f7ee681512557b36
>
> On second thought maybe we don't even need this one ported. I think OVS
> has its own mitigation in place.
Yep - exactly.
> Another thing that took me by surprise is that 0-day bot always uses the
> OVS checkpatch script (that's why it didn't choke on Numan's I-P patch
> like OVN checkpatch did for me locally) - cc-ing Aaron.
It does? I thought it should be using the checkpatch that is included in
the OVN repo. That sounds like a bug. But if you decide to switch to
using the OVS one then maybe it becomes a feature. :)
> Regards,
> Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] OVN 24.03 Soft Freeze 19 January
On 1/16/24 14:45, Mark Michelson wrote: > Hi everyone, > > The soft freeze for OVN 24.03 is this Friday, 19 January, 2024. Please > ensure that any patches that introduce new features are posted to the > mailing list by that date if you wish to have them included in OVN 24.03. > > If you have a feature patch that you wish to be included in 24.03, > please post a link to the series in a reply to this mail. The OVN team > will do our best to review the desired changes. > Hi Mark, It's not a formal series but the final patch will look very similar to what I posted in this question: https://mail.openvswitch.org/pipermail/ovs-dev/2024-January/410847.html I'm mentioning this as a potential "feature" because the discussion is not over on this one and if the original authors really require the current (broken in my opinion) behavior then we'd have to advertise the "random" behavior and add a config for it. > After soft freeze, we will create the OVN 24.03 branch on 2 February. At > that point, no new feature patches will be merged into the 24.03 branch. > However, bug fixes will still be eligible for inclusion. > > Thank you, > Mark Michelson > Regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v5 08/16] northd: Refactor lflow management into a separate module.
On 1/18/24 22:39, Numan Siddique wrote:
>>> +void
>>> +ovn_dp_groups_clear(struct hmap *dp_groups)
>>> +{
>>> +struct ovn_dp_group *dpg;
>>> +HMAP_FOR_EACH_POP (dpg, node, dp_groups) {
>>> +bitmap_free(dpg->bitmap);
>>> +free(dpg);
>> This is duplicated in dec_ovn_dp_group_ref(). Also, should we assert
>> that all refcounts are 0 here?
> I don't think we can. Since ovn_dp_groups_clear() will be called
> whenever a full recompute
> happens. In this case, we are destroying the internal data and
> rebuilding and the ref count may not be 0.
Hmm, OK, I think that might be fine.
Thanks for checking.
Regards,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.
On 19 Jan 2024, at 13:53, David Marchand wrote:
> On Fri, Jan 19, 2024 at 1:49 PM Ilya Maximets wrote:
>>
>> On 1/18/24 14:00, David Marchand wrote:
>>> Seen in GHA recently.
>>> Unit tests are checking conntracks relating to a destination ip address
>>> but the FORMAT_CT macro is not strict enough and would match unrelated
>>> conntracks too.
>>>
>>> Example:
>>> 148. system-traffic.at:6432: testing conntrack - DNAT with
>>> additional SNAT ...
>>> [...]
>>> ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack |
>>> grep "dst=10.1.1.1" |
>>> sed -e 's/port=[0-9]*/port=/g'
>>> -e 's/id=[0-9]*/id=/g'
>>> -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
>>> [...]
>>> @@ -1,2 +1,7 @@
>>> tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
>>> +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
>>> +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
>>> +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
>>> +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
>>> +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
>>>
>>> Fixes: 07659514c3c1 ("Add support for connection tracking.")
>>> Signed-off-by: David Marchand
>>> ---
>>> tests/system-common-macros.at | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
>>> index 01ebe364ee..07be29f673 100644
>>> --- a/tests/system-common-macros.at
>>> +++ b/tests/system-common-macros.at
>>> @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed
>>> 's/csum:.*/csum: /'])
>>> # and limit the output to the rows containing 'ip-addr'.
>>> #
>>> m4_define([FORMAT_CT],
>>> -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e
>>> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' |
>>> sort | uniq]])
>>> +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e
>>> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' |
>>> sort | uniq]])
>>>
>>> # NETNS_DAEMONIZE([namespace], [command], [pidfile])
>>> #
>>
>> I remembered why the macro is loose. We wanted to be able
>> to match on "subnets" by supplying only part of the address.
>>
>> There was at least one test that used this functionality.
>> Eelco removed it though here:
>>
>> https://github.com/openvswitch/ovs/commit/a80883f7682158c7a6955360ee852e8279f748e9
>>
>> Did you check if have any more instances of such tests?
>
> I did not.
>
>> They can be tricky to find, as we can supply 10.1.1.2 in order
>> to match 10.1.1.240, for example.
>
> Ok, you can discard my patch.
> Thanks.
But looking at most of the test cases when they put in an IP they mean that
specific IP not 10.1.1.20? But maybe your NS idea works better.
//Eelco
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] netdev-dpdk: Trigger port reconfiguration in main thread for resets.
On 1/18/24 17:15, David Marchand wrote: > When OVS (main thread) configures a DPDK netdev, it holds a netdev_dpdk > mutex lock. > As part of this configure operation, the net/iavf driver (used with i40e > VF devices) triggers a queue count change. The PF entity (serviced by a > kernel PF driver for example) handles this change and requests back that > the VF driver resets the VF device. The driver then completes the VF reset > operation on its side and waits for completion of the iavf-event thread > responsible for handling various VF device events. > > On the other hand, handling of the VF reset request in this iavf-event > thread results in notifying the application with a port reset request > (RTE_ETH_EVENT_INTR_RESET). The OVS reset callback tries to take a hold > of the same netdev_dpdk mutex and blocks the iavf-event thread. > > As a resut, the net/iavf driver (still running on OVS main thread) is > unable to complete as it is waiting for iavf-event to complete. > > To break from this situation, the OVS reset callback now won't take a > netdev_dpdk mutex. Instead, the port reset request is stored in a simple > RTE_ETH_MAXPORTS array associated to a seq object. > This is enough to let the VF driver complete this port initialisation. > The OVS main thread later handles the port reset request. > > More details in the DPDK upstream bz as this issue appeared following a > change in DPDK. > > Link: https://bugs.dpdk.org/show_bug.cgi?id=1337 > Signed-off-by: David Marchand > --- > Changes since v2: > - fixed build with clang, > - fixed indentation, > - updated NEWS, > > Changes since v1: > - converted to atomic accesses on netdev_dpdk_pending_reset[], > > --- > NEWS | 7 - > lib/netdev-dpdk.c | 76 +-- > 2 files changed, 61 insertions(+), 22 deletions(-) Thanks, David! Applied and backported to branch-3.3. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Avoid checks while preparing non-offloading packets.
On 1/18/24 19:34, Mike Pattrick wrote: > On Thu, Jan 18, 2024 at 11:40 AM Ilya Maximets wrote: >> >> Currently, dp_packet_ol_send_prepare() performs multiple checks for >> each offloading flag separately. That takes a noticeable amount of >> extra cycles for packets that do not have any offloading flags set. >> >> Skip most of the work if no checksumming flags are set. >> >> The change improves performance of direct forwarding between two >> virtio-user ports (V2V) by ~2.5 % and offsets all the negative >> effects of TSO support introduced recently. >> >> It adds an extra check to the offloading path, but it is not a >> default configuration and also should take much smaller hit due >> to lower number of larger packets. >> >> Signed-off-by: Ilya Maximets > > I tested this in a V2V configuration as well and saw a small > performance improvement across 6 runs. I also tested making the check > an inline function and calling into the full send prepare if it > passes, but that change didn't yield any noticeable performance > improvement over the current solution. > > Acked-by: Mike Pattrick > Thanks, Mike and Simon! Applied and backported to branch-3.3. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tests: mcast-snooping: Stop time for the group protocol test.
On 1/19/24 10:18, Simon Horman wrote:
> On Thu, Jan 18, 2024 at 08:25:23PM +0100, Ilya Maximets wrote:
>> Otherwise, it randomly fails due to age not being zero under load:
>>
>> tests/mcast-snooping.at:645: ovs-appctl mdb/show br0
>> --- -
>> +++ /at-groups/2592/stdout
>> @@ -1,5 +1,5 @@
>> port VLAN protocol GROUPAge
>> -1 0 IGMPv1224.1.1.1 0
>> +1 0 IGMPv1224.1.1.1 1
>>1 0 IGMPv2224.1.1.2 0
>>1 0 IGMPv3233.54.12.230 0
>>
>> Fixes: b222593bc69b ("mcast-snooping: Add group protocol to mdb/show
>> output.")
>> Signed-off-by: Ilya Maximets
>
> Thanks.
>
> On my otherwise idle system, I ran the test updated by this patch
> in a loop.
>
> Without this patch, it failed on the 55th iteration.
> With this patch it successfully ran 1000 iterations
> (the target for my experiment).
>
> Not being very scientific, I did not try this experiment a second time.
>
> Acked-by: Simon Horman
>
Thanks, Simon and Eelco!
Applied and backported to branch-3.3.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v5 16/16] northd: Add I-P for NB_Global and SB_Global.
On 1/11/24 16:34, num...@ovn.org wrote:
> From: Numan Siddique
>
> A new engine node "global_config" is added which handles the changes
> to NB_Global an SB_Global tables. It also creates these rows if
> not present.
>
> Without the I-P, any changes to the options column of these tables
> result in recompute of 'northd' and 'lflow' engine nodes.
>
> Signed-off-by: Numan Siddique
> ---
Hi Numan,
This looks ok overall. I just have a few minor comments, please see
below. If you address those feel free to add my ack:
Acked-by: Dumitru Ceara
Thanks,
Dumitru
> northd/aging.c| 21 +-
> northd/automake.mk| 2 +
> northd/en-global-config.c | 588 ++
> northd/en-global-config.h | 65 +
> northd/en-lflow.c | 11 +-
> northd/en-northd.c| 52 ++--
> northd/en-northd.h| 2 +-
> northd/en-sync-sb.c | 26 +-
> northd/inc-proc-northd.c | 38 ++-
> northd/northd.c | 230 +++
> northd/northd.h | 24 +-
> tests/ovn-northd.at | 256 +++--
> 12 files changed, 1017 insertions(+), 298 deletions(-)
> create mode 100644 northd/en-global-config.c
> create mode 100644 northd/en-global-config.h
>
> diff --git a/northd/aging.c b/northd/aging.c
> index cdf5f4464e..b76963a2dd 100644
> --- a/northd/aging.c
> +++ b/northd/aging.c
> @@ -15,6 +15,7 @@
>
> #include
>
> +#include "en-global-config.h"
> #include "lib/inc-proc-eng.h"
> #include "lib/ovn-nb-idl.h"
> #include "lib/ovn-sb-idl.h"
> @@ -347,15 +348,10 @@ aging_context_handle_timestamp(struct aging_context
> *ctx, int64_t timestamp,
> static uint32_t
> get_removal_limit(struct engine_node *node, const char *name)
> {
> -const struct nbrec_nb_global_table *nb_global_table =
> -EN_OVSDB_GET(engine_get_input("NB_nb_global", node));
> -const struct nbrec_nb_global *nb =
> -nbrec_nb_global_table_first(nb_global_table);
> -if (!nb) {
> -return 0;
> -}
> +struct ed_type_global_config *global_config =
> +engine_get_input_data("global_config", node);
>
> -return smap_get_uint(>options, name, 0);
> +return smap_get_uint(_config->nb_options, name, 0);
> }
>
> /* MAC binding aging */
> @@ -394,11 +390,14 @@ en_mac_binding_aging_run(struct engine_node *node, void
> *data OVS_UNUSED)
> {
> const struct engine_context *eng_ctx = engine_get_context();
> struct northd_data *northd_data = engine_get_input_data("northd", node);
> +struct ed_type_global_config *global_config =
> +engine_get_input_data("global_config", node);
> +
> struct aging_waker *waker =
> engine_get_input_data("mac_binding_aging_waker", node);
>
> if (!eng_ctx->ovnsb_idl_txn ||
> -!northd_data->features.mac_binding_timestamp ||
> +!global_config->features.mac_binding_timestamp ||
> time_msec() < waker->next_wake_msec) {
> return;
> }
> @@ -530,9 +529,11 @@ en_fdb_aging_run(struct engine_node *node, void *data
> OVS_UNUSED)
> const struct engine_context *eng_ctx = engine_get_context();
> struct northd_data *northd_data = engine_get_input_data("northd", node);
> struct aging_waker *waker = engine_get_input_data("fdb_aging_waker",
> node);
> +struct ed_type_global_config *global_config =
> +engine_get_input_data("global_config", node);
>
> if (!eng_ctx->ovnsb_idl_txn ||
> -!northd_data->features.fdb_timestamp ||
> +!global_config->features.fdb_timestamp ||
> time_msec() < waker->next_wake_msec) {
> return;
> }
> diff --git a/northd/automake.mk b/northd/automake.mk
> index 19abb0dece..d491973a8b 100644
> --- a/northd/automake.mk
> +++ b/northd/automake.mk
> @@ -8,6 +8,8 @@ northd_ovn_northd_SOURCES = \
> northd/northd.c \
> northd/northd.h \
> northd/ovn-northd.c \
> + northd/en-global-config.c \
> + northd/en-global-config.h \
> northd/en-northd.c \
> northd/en-northd.h \
> northd/en-lflow.c \
> diff --git a/northd/en-global-config.c b/northd/en-global-config.c
> new file mode 100644
> index 00..0d218f2ab5
> --- /dev/null
> +++ b/northd/en-global-config.c
> @@ -0,0 +1,588 @@
> +/*
> + * 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.
> + */
> +
> +#include
> +
> +#include
> +#include
> +#include
> +
> +/* OVS includes */
> +#include
Re: [ovs-dev] [PATCH] ovs-atomic: Fix inclusion of Clang header by GCC 14.
On 1/19/24 13:05, Simon Horman wrote:
> On Thu, Jan 18, 2024 at 04:30:06PM +, Simon Horman wrote:
>> On Thu, Jan 18, 2024 at 03:59:05PM +0100, Ilya Maximets wrote:
>>> GCC 14 started to advertise c_atomic extension, older versions didn't
>>> do that. Add check for __clang__, so GCC doesn't include headers
>>> designed for Clang.
>>>
>>> Another option would be to prefer stdatomic implementation instead,
>>> but some older versions of Clang are not able to use stdatomic.h
>>> supplied by GCC as described in commit:
>>> 07ece367fb5f ("ovs-atomic: Prefer Clang intrinsics over .")
>>>
>>> This change fixes OVS build with GCC on Fedora Rawhide (40).
>>>
>>> Reported-by: Jakob Meng
>>> Signed-off-by: Ilya Maximets
>>
>> Recheck-request: github-robot
>
> That run did not work either [1],
> but I was able to get a successful run in my own
> GitHub repository [2].
>
> [1] https://github.com/ovsrobot/ovs/actions/runs/7572217223/job/20625604785
> [2] https://github.com/horms/ovs/actions/runs/7581260421
>
> Acked-by: Simon Horman
>
Thanks, Simon, Jakob and Eelco!
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] system-common-macros: Fix conntrack matching.
On 19 Jan 2024, at 13:49, Ilya Maximets wrote:
> On 1/18/24 14:00, David Marchand wrote:
>> Seen in GHA recently.
>> Unit tests are checking conntracks relating to a destination ip address
>> but the FORMAT_CT macro is not strict enough and would match unrelated
>> conntracks too.
>>
>> Example:
>> 148. system-traffic.at:6432: testing conntrack - DNAT with
>> additional SNAT ...
>> [...]
>> ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack |
>> grep "dst=10.1.1.1" |
>> sed -e 's/port=[0-9]*/port=/g'
>> -e 's/id=[0-9]*/id=/g'
>> -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
>> [...]
>> @@ -1,2 +1,7 @@
>> tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
>> +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
>> +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
>> +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
>> +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
>> +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
>>
>> Fixes: 07659514c3c1 ("Add support for connection tracking.")
>> Signed-off-by: David Marchand
>> ---
>> tests/system-common-macros.at | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
>> index 01ebe364ee..07be29f673 100644
>> --- a/tests/system-common-macros.at
>> +++ b/tests/system-common-macros.at
>> @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed
>> 's/csum:.*/csum: /'])
>> # and limit the output to the rows containing 'ip-addr'.
>> #
>> m4_define([FORMAT_CT],
>> -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e
>> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' |
>> sort | uniq]])
>> +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e
>> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' |
>> sort | uniq]])
>>
>> # NETNS_DAEMONIZE([namespace], [command], [pidfile])
>> #
>
> I remembered why the macro is loose. We wanted to be able
> to match on "subnets" by supplying only part of the address.
>
> There was at least one test that used this functionality.
> Eelco removed it though here:
>
> https://github.com/openvswitch/ovs/commit/a80883f7682158c7a6955360ee852e8279f748e9
>
I guess the subnet mask part will work if we add the subnet dot ., i,e.
FORMAT_CT(10.1.1.)]
But this also requires some more change, i.e. making the dot not being a
wildcard. Something like this:
-[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e
's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | sort |
uniq]])
+[[grep "dst=$(echo $1 | sed -e 's/\./\\./g')\>" | \
> Did you check if have any more instances of such tests?
> They can be tricky to find, as we can supply 10.1.1.2 in order
> to match 10.1.1.240, for example.
>
> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.
On Fri, Jan 19, 2024 at 1:49 PM Ilya Maximets wrote:
>
> On 1/18/24 14:00, David Marchand wrote:
> > Seen in GHA recently.
> > Unit tests are checking conntracks relating to a destination ip address
> > but the FORMAT_CT macro is not strict enough and would match unrelated
> > conntracks too.
> >
> > Example:
> > 148. system-traffic.at:6432: testing conntrack - DNAT with
> > additional SNAT ...
> > [...]
> > ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack |
> > grep "dst=10.1.1.1" |
> > sed -e 's/port=[0-9]*/port=/g'
> > -e 's/id=[0-9]*/id=/g'
> > -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
> > [...]
> > @@ -1,2 +1,7 @@
> > tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
> > +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> > +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
> > +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> > +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> > +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> >
> > Fixes: 07659514c3c1 ("Add support for connection tracking.")
> > Signed-off-by: David Marchand
> > ---
> > tests/system-common-macros.at | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
> > index 01ebe364ee..07be29f673 100644
> > --- a/tests/system-common-macros.at
> > +++ b/tests/system-common-macros.at
> > @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed
> > 's/csum:.*/csum: /'])
> > # and limit the output to the rows containing 'ip-addr'.
> > #
> > m4_define([FORMAT_CT],
> > -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e
> > 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' |
> > sort | uniq]])
> > +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e
> > 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' |
> > sort | uniq]])
> >
> > # NETNS_DAEMONIZE([namespace], [command], [pidfile])
> > #
>
> I remembered why the macro is loose. We wanted to be able
> to match on "subnets" by supplying only part of the address.
>
> There was at least one test that used this functionality.
> Eelco removed it though here:
>
> https://github.com/openvswitch/ovs/commit/a80883f7682158c7a6955360ee852e8279f748e9
>
> Did you check if have any more instances of such tests?
I did not.
> They can be tricky to find, as we can supply 10.1.1.2 in order
> to match 10.1.1.240, for example.
Ok, you can discard my patch.
Thanks.
--
David Marchand
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.
On 1/18/24 14:00, David Marchand wrote:
> Seen in GHA recently.
> Unit tests are checking conntracks relating to a destination ip address
> but the FORMAT_CT macro is not strict enough and would match unrelated
> conntracks too.
>
> Example:
> 148. system-traffic.at:6432: testing conntrack - DNAT with
> additional SNAT ...
> [...]
> ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack |
> grep "dst=10.1.1.1" |
> sed -e 's/port=[0-9]*/port=/g'
> -e 's/id=[0-9]*/id=/g'
> -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
> [...]
> @@ -1,2 +1,7 @@
> tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
> +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
> +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
>
> Fixes: 07659514c3c1 ("Add support for connection tracking.")
> Signed-off-by: David Marchand
> ---
> tests/system-common-macros.at | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
> index 01ebe364ee..07be29f673 100644
> --- a/tests/system-common-macros.at
> +++ b/tests/system-common-macros.at
> @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed
> 's/csum:.*/csum: /'])
> # and limit the output to the rows containing 'ip-addr'.
> #
> m4_define([FORMAT_CT],
> -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e
> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | sort
> | uniq]])
> +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e
> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | sort
> | uniq]])
>
> # NETNS_DAEMONIZE([namespace], [command], [pidfile])
> #
I remembered why the macro is loose. We wanted to be able
to match on "subnets" by supplying only part of the address.
There was at least one test that used this functionality.
Eelco removed it though here:
https://github.com/openvswitch/ovs/commit/a80883f7682158c7a6955360ee852e8279f748e9
Did you check if have any more instances of such tests?
They can be tricky to find, as we can supply 10.1.1.2 in order
to match 10.1.1.240, for example.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.
On Fri, Jan 19, 2024 at 1:20 PM Simon Horman wrote:
>
> On Thu, Jan 18, 2024 at 02:00:18PM +0100, David Marchand wrote:
> > Seen in GHA recently.
> > Unit tests are checking conntracks relating to a destination ip address
> > but the FORMAT_CT macro is not strict enough and would match unrelated
> > conntracks too.
> >
> > Example:
> > 148. system-traffic.at:6432: testing conntrack - DNAT with
> > additional SNAT ...
> > [...]
> > ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack |
> > grep "dst=10.1.1.1" |
> > sed -e 's/port=[0-9]*/port=/g'
> > -e 's/id=[0-9]*/id=/g'
> > -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
> > [...]
> > @@ -1,2 +1,7 @@
> > tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
> > +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> > +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
> > +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> > +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> > +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> >
> > Fixes: 07659514c3c1 ("Add support for connection tracking.")
> > Signed-off-by: David Marchand
> > ---
> > tests/system-common-macros.at | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
> > index 01ebe364ee..07be29f673 100644
> > --- a/tests/system-common-macros.at
> > +++ b/tests/system-common-macros.at
> > @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed
> > 's/csum:.*/csum: /'])
> > # and limit the output to the rows containing 'ip-addr'.
> > #
> > m4_define([FORMAT_CT],
> > -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e
> > 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' |
> > sort | uniq]])
> > +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e
> > 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' |
> > sort | uniq]])
> >
> > # NETNS_DAEMONIZE([namespace], [command], [pidfile])
> > #
>
> Sorry, I feel I mist be missing something very obvious, but
> I'm unsure why the match is on "dst=$1\>". I would have thought
> the match would be "dst=$1," instead.
\> matches the end of a word.
Using , as a delimiter works too in this case.
--
David Marchand
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7 4/6] ofproto: Add JSON output for 'dpif/show' command.
On Thu, Jan 18, 2024 at 04:26:55PM +0100, jm...@redhat.com wrote: > From: Jakob Meng > > The 'dpif/show' command now supports machine-readable JSON output in > addition to the plain-text output for humans. An example would be: > > ovs-appctl --format json dpif/show > > Reported-at: https://bugzilla.redhat.com/1824861 > Signed-off-by: Jakob Meng Recheck-request: github-robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7 1/6] Add global option for JSON output to ovs-appctl.
On Thu, Jan 18, 2024 at 04:39:56PM +, Simon Horman wrote: > On Thu, Jan 18, 2024 at 04:26:52PM +0100, jm...@redhat.com wrote: > > From: Jakob Meng > > > > For monitoring systems such as Prometheus it would be beneficial if > > OVS would expose statistics in a machine-readable format. > > ... > > Recheck-request: github-robot Rerun was successful :) https://github.com/ovsrobot/ovs/actions/runs/7572473760 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] netdev-dpdk: Trigger port reconfiguration in main thread for resets.
On Thu, Jan 18, 2024 at 05:15:00PM +0100, David Marchand wrote: > When OVS (main thread) configures a DPDK netdev, it holds a netdev_dpdk > mutex lock. > As part of this configure operation, the net/iavf driver (used with i40e > VF devices) triggers a queue count change. The PF entity (serviced by a > kernel PF driver for example) handles this change and requests back that > the VF driver resets the VF device. The driver then completes the VF reset > operation on its side and waits for completion of the iavf-event thread > responsible for handling various VF device events. > > On the other hand, handling of the VF reset request in this iavf-event > thread results in notifying the application with a port reset request > (RTE_ETH_EVENT_INTR_RESET). The OVS reset callback tries to take a hold > of the same netdev_dpdk mutex and blocks the iavf-event thread. > > As a resut, the net/iavf driver (still running on OVS main thread) is > unable to complete as it is waiting for iavf-event to complete. > > To break from this situation, the OVS reset callback now won't take a > netdev_dpdk mutex. Instead, the port reset request is stored in a simple > RTE_ETH_MAXPORTS array associated to a seq object. > This is enough to let the VF driver complete this port initialisation. > The OVS main thread later handles the port reset request. > > More details in the DPDK upstream bz as this issue appeared following a > change in DPDK. > > Link: https://bugs.dpdk.org/show_bug.cgi?id=1337 > Signed-off-by: David Marchand For the record: We seem to have a situation with unreliable test results not related to this patch. To that end the automated workflow run failed [1]. However, I did get a successful run in my own repository [2]. [1] https://github.com/ovsrobot/ovs/actions/runs/7573251006 [2] https://github.com/horms/ovs/actions/runs/7581284248 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Avoid checks while preparing non-offloading packets.
On Thu, Jan 18, 2024 at 05:38:19PM +0100, Ilya Maximets wrote: > Currently, dp_packet_ol_send_prepare() performs multiple checks for > each offloading flag separately. That takes a noticeable amount of > extra cycles for packets that do not have any offloading flags set. > > Skip most of the work if no checksumming flags are set. > > The change improves performance of direct forwarding between two > virtio-user ports (V2V) by ~2.5 % and offsets all the negative > effects of TSO support introduced recently. > > It adds an extra check to the offloading path, but it is not a > default configuration and also should take much smaller hit due > to lower number of larger packets. > > Signed-off-by: Ilya Maximets Acked-by: Simon Horman We seem to have a situation with unreliable test results not related to this patch. To that end the automated workflow run failed [1]. However, I did get a successful run in my own repository [2]. [1] https://github.com/ovsrobot/ovs/actions/runs/7573402009 [2] https://github.com/horms/ovs/actions/runs/7581285912 ... ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: Use proper field for lookup_nd
On 1/19/24 10:28, Xavier Simonart wrote:
> Hi Ales
>
> The patch looks good to me, Thanks!
> Acked-by: Xavier Simonart
>
> Thanks
> Xavier
>
> On Thu, Jan 18, 2024 at 3:54 PM Ales Musil wrote:
>
>> We are intentionally skipping ND NA with LLA as source.
>> However, this doesn't work when the ND NA has LLA source,
>> but the target address is global one. In that case we
>> would skip update of already existing entry when
>> always_learn_from_arp_request is set to false.
>>
>> Use ND target address in the check instead of source.
>>
>> Fixes: 5e0cb03605ea ("northd: Add logical flow to skip GARP with LLA")
>> Reported-at: https://issues.redhat.com/browse/FDP-283
>> Signed-off-by: Ales Musil
>> ---
>> northd/northd.c | 4 ++--
>> tests/ovn.at| 25 ++---
>> 2 files changed, 24 insertions(+), 5 deletions(-)
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 952f8200d..de15ca101 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -13171,9 +13171,9 @@ build_neigh_learning_flows_for_lrouter(
>> * address, the all-nodes multicast address. */
>> ds_clear(actions);
>> ds_put_format(actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
>> - " = lookup_nd(inport, ip6.src, nd.tll); "
>> + " = lookup_nd(inport, nd.target, nd.tll); "
>> REGBIT_LOOKUP_NEIGHBOR_IP_RESULT
>> - " = lookup_nd_ip(inport, ip6.src); next;");
>> + " = lookup_nd_ip(inport, nd.target);
>> next;");
>> ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 110,
>>"nd_na && ip6.src == fe80::/10 && ip6.dst ==
>> ff00::/8",
>>ds_cstr(actions));
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 2dd46fd79..26c516ef9 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -5385,10 +5385,11 @@ test_arp() {
>> }
>>
>> test_na() {
>> -local inport=$1 sha=$2 spa=$3
>> +local inport=$1 sha=$2 spa=$3 src=${4-$3}
>> local request=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff',
>> src='${sha}')/ \
>> - IPv6(dst='ff01::1', src='${spa}')/ \
>> - ICMPv6ND_NA(tgt='${spa}')")
>> + IPv6(dst='ff01::1', src='${src}')/ \
>> + ICMPv6ND_NA(tgt='${spa}')/ \
>> + ICMPv6NDOptDstLLAddr(lladdr='${sha}')")
>>
>> hv=hv`vif_to_hv $inport`
>> as $hv ovs-appctl netdev-dummy/receive vif$inport $request
>> @@ -5464,6 +5465,24 @@ for i in 1 2; do
>> done
>> done
>>
>> +# Make sure that we can update existing entry with
>> +# "always_learn_from_arp_request=false" even when the source of NA src is
>> LLA.
>> +ovn-sbctl --all destroy mac_binding
>> +ovn-nbctl --wait=hv set logical_router lr0
>> options:always_learn_from_arp_request=false
Thanks, Ales and Xavier! I added the missing "check" calls here and
pushed this to main and backported it all the way down to 22.03.
Regards,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.
On Thu, Jan 18, 2024 at 02:00:18PM +0100, David Marchand wrote:
> Seen in GHA recently.
> Unit tests are checking conntracks relating to a destination ip address
> but the FORMAT_CT macro is not strict enough and would match unrelated
> conntracks too.
>
> Example:
> 148. system-traffic.at:6432: testing conntrack - DNAT with
> additional SNAT ...
> [...]
> ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack |
> grep "dst=10.1.1.1" |
> sed -e 's/port=[0-9]*/port=/g'
> -e 's/id=[0-9]*/id=/g'
> -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
> [...]
> @@ -1,2 +1,7 @@
> tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
> +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
> +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
>
> Fixes: 07659514c3c1 ("Add support for connection tracking.")
> Signed-off-by: David Marchand
> ---
> tests/system-common-macros.at | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
> index 01ebe364ee..07be29f673 100644
> --- a/tests/system-common-macros.at
> +++ b/tests/system-common-macros.at
> @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed
> 's/csum:.*/csum: /'])
> # and limit the output to the rows containing 'ip-addr'.
> #
> m4_define([FORMAT_CT],
> -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e
> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | sort
> | uniq]])
> +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e
> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' | sort
> | uniq]])
>
> # NETNS_DAEMONIZE([namespace], [command], [pidfile])
> #
Sorry, I feel I mist be missing something very obvious, but
I'm unsure why the match is on "dst=$1\>". I would have thought
the match would be "dst=$1," instead.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v5 15/16] northd: Add northd change handler for sync_to_sb_lb node.
On 1/11/24 16:34, num...@ovn.org wrote:
> From: Numan Siddique
>
> Any changes to northd engine node due to load balancers
> are now handled in 'sync_to_sb_lb' node to sync the changed
> load balancers to SB load balancers.
>
> The logic to sync the SB load balancers is changed a bit and it
> now mimics the SB lflow sync.
>
> Below are the scale testing results done with all the patches applied
> in this series using ovn-heater. The test ran the scenario -
> ocp-500-density-heavy.yml [1].
>
> The resuts are:
>
> ---
> Min (s) Median (s) 90%ile (s) 99%ile
> (s) Max (s) Mean (s)Total (s) Count Failed
> ---
> Iteration Total 0.1368831.1290161.192001
> 1.2041671.2127280.66501783.127099 125 0
> Namespace.add_ports 0.0052160.0057360.007034
> 0.0154860.0189780.0062110.776373125 0
> WorkerNode.bind_port 0.0350300.0460820.052469
> 0.0582930.0603110.04597311.493259 250 0
> WorkerNode.ping_port 0.0050570.0067271.047692
> 1.0692531.0713360.26689666.724094 250 0
> ---
>
> The results with the present main [2] are:
>
> ---
> Min (s) Median (s) 90%ile (s) 99%ile
> (s) Max (s) Mean (s)Total (s) Count Failed
> ---
> Iteration Total 0.1354912.2238053.311270
> 3.3390783.3453461.729172216.146495 125 0
> Namespace.add_ports 0.0053800.0057440.006819
> 0.0187730.0208000.0062920.786532125 0
> WorkerNode.bind_port 0.0341790.0460550.053488
> 0.0588010.0710430.04611711.529311 250 0
> WorkerNode.ping_port 0.0049560.0069523.086952
> 3.1917433.1928070.791544197.886026 250 0
> ---
>
> [1] -
> https://github.com/ovn-org/ovn-heater/blob/main/test-scenarios/ocp-500-density-heavy.yml
> [2] - 2a12cda890a7("controller, northd: Wait for cleanup before replying to
> exit")
>
> Signed-off-by: Numan Siddique
> ---
Hi Numan,
I only have a few minor comments, please see inline. With those
addressed I think the patch is correct:
Acked-by: Dumitru Ceara
Thanks,
Dumitru
> northd/en-sync-sb.c | 505 +--
> northd/inc-proc-northd.c | 1 +
> northd/lflow-mgr.c | 196 ++-
> northd/lflow-mgr.h | 23 +-
> northd/northd.c | 238 --
> northd/northd.h | 6 -
> tests/ovn-northd.at | 164 ++---
> 7 files changed, 705 insertions(+), 428 deletions(-)
>
> diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
> index d39cbbf2e6..80f3621bb9 100644
> --- a/northd/en-sync-sb.c
> +++ b/northd/en-sync-sb.c
> @@ -18,17 +18,21 @@
> #include
> #include
>
> +/* OVS includes. */
> #include "lib/svec.h"
> #include "openvswitch/util.h"
>
> +/* OVN includes. */
> #include "en-lr-nat.h"
> #include "en-lr-stateful.h"
> #include "en-sync-sb.h"
> +#include "lb.h"
> #include "lib/inc-proc-eng.h"
> #include "lib/lb.h"
> #include "lib/ovn-nb-idl.h"
> #include "lib/ovn-sb-idl.h"
> #include "lib/ovn-util.h"
> +#include "lflow-mgr.h"
> #include "northd.h"
>
> #include "openvswitch/vlog.h"
> @@ -51,6 +55,40 @@ static void build_port_group_address_set(const struct
> nbrec_port_group *,
> struct svec *ipv4_addrs,
> struct svec *ipv6_addrs);
>
> +struct sb_lb_table;
> +struct sb_lb_record;
These are defined just below. Let's move the prototypes after the
struct definition?
> +static void sb_lb_table_init(struct sb_lb_table *);
> +static void sb_lb_table_clear(struct sb_lb_table *);
> +static void sb_lb_table_destroy(struct sb_lb_table *);
> +
Re: [ovs-dev] [PATCH] ovs-atomic: Fix inclusion of Clang header by GCC 14.
On Thu, Jan 18, 2024 at 04:30:06PM +, Simon Horman wrote:
> On Thu, Jan 18, 2024 at 03:59:05PM +0100, Ilya Maximets wrote:
> > GCC 14 started to advertise c_atomic extension, older versions didn't
> > do that. Add check for __clang__, so GCC doesn't include headers
> > designed for Clang.
> >
> > Another option would be to prefer stdatomic implementation instead,
> > but some older versions of Clang are not able to use stdatomic.h
> > supplied by GCC as described in commit:
> > 07ece367fb5f ("ovs-atomic: Prefer Clang intrinsics over .")
> >
> > This change fixes OVS build with GCC on Fedora Rawhide (40).
> >
> > Reported-by: Jakob Meng
> > Signed-off-by: Ilya Maximets
>
> Recheck-request: github-robot
That run did not work either [1],
but I was able to get a successful run in my own
GitHub repository [2].
[1] https://github.com/ovsrobot/ovs/actions/runs/7572217223/job/20625604785
[2] https://github.com/horms/ovs/actions/runs/7581260421
Acked-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v5 14/16] northd: Add a noop handler for northd SB mac binding.
On 1/11/24 16:34, num...@ovn.org wrote: > From: Numan Siddique > > northd engine node uses the sb mac binding table to > cleanup mac binding entries for deleted logical ports > and datapaths. Any update to SB mac binding doesn't > change the northd engine node state or data. Hence > it is ok to add a noop_handler. > > Presently, mac_binding_aging node depends on SB mac binding > too and it falls back to full recompute for any SB mac binding > changes. It needs to be evaluated if mac_binding_aging > really needs to handle SB mac binding updates. If not, we > can omit the SB mac binding updates (ovsdb_idl_omit_alert()) > and also remove the noop_handler this patch adds for northd node. > > Signed-off-by: Numan Siddique > --- > northd/inc-proc-northd.c | 12 +++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index f7c3d2bcf5..40a9e5e06c 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -177,7 +177,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(_northd, _sb_mirror, NULL); > engine_add_input(_northd, _sb_meter, NULL); > engine_add_input(_northd, _sb_datapath_binding, NULL); > -engine_add_input(_northd, _sb_mac_binding, NULL); > engine_add_input(_northd, _sb_dns, NULL); > engine_add_input(_northd, _sb_ha_chassis_group, NULL); > engine_add_input(_northd, _sb_ip_multicast, NULL); > @@ -186,6 +185,17 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(_northd, _sb_static_mac_binding, NULL); > engine_add_input(_northd, _sb_chassis_template_var, NULL); > > +/* northd engine node uses the sb mac binding table to > + * cleanup mac binding entries for deleted logical ports > + * and datapaths. Any update to SB mac binding doesn't > + * change the northd engine node state or data. Hence > + * it is ok to add a noop_handler here. > + * Note: mac_binding_aging engine node depends on SB mac binding > + * and it results in full recompute for any changes to it. > + * */ > +engine_add_input(_northd, _sb_mac_binding, > + engine_noop_handler); > + > engine_add_input(_northd, _sb_port_binding, > northd_sb_port_binding_handler); > engine_add_input(_northd, _nb_nb_global, Makes sense to me: Acked-by: Dumitru Ceara Thanks! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v5 13/16] northd: Add ls_stateful handler for lflow engine node.
On 1/11/24 16:33, num...@ovn.org wrote:
> From: Numan Siddique
>
> Signed-off-by: Numan Siddique
> ---
Hi Numan,
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index dcce79510a..f7c3d2bcf5 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -228,11 +228,11 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> engine_add_input(_lflow, _sb_logical_flow, NULL);
> engine_add_input(_lflow, _sb_multicast_group, NULL);
> engine_add_input(_lflow, _sb_igmp_group, NULL);
> -engine_add_input(_lflow, _ls_stateful, NULL);
> engine_add_input(_lflow, _sb_logical_dp_group, NULL);
> engine_add_input(_lflow, _northd, lflow_northd_handler);
> engine_add_input(_lflow, _port_group, lflow_port_group_handler);
> engine_add_input(_lflow, _lr_stateful, lflow_lr_stateful_handler);
> +engine_add_input(_lflow, _ls_stateful, lflow_ls_stateful_handler);
>
> engine_add_input(_sync_to_sb_addr_set, _nb_address_set,
> sync_to_sb_addr_set_nb_address_set_handler);
> diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
> index 6cb2a367fe..d81b13a25c 100644
> --- a/northd/lflow-mgr.c
> +++ b/northd/lflow-mgr.c
> @@ -442,7 +442,7 @@ lflow_ref_unlink_lflows(struct lflow_ref *lflow_ref)
> BITMAP_FOR_EACH_1 (index, lrn->dpgrp_bitmap_len,
> lrn->dpgrp_bitmap) {
> if (dec_dp_refcnt(>lflow->dp_refcnts_map, index)) {
> -bitmap_set0(lrn->lflow->dpg_bitmap, lrn->dp_index);
> +bitmap_set0(lrn->lflow->dpg_bitmap, index);
Hmm, is this something that should go in "[PATCH ovn v5 01/16] northd:
Refactor the northd change tracking."?
Thanks,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v5 12/16] northd: Add lr_stateful handler for lflow engine node.
On 1/11/24 16:33, num...@ovn.org wrote:
> From: Numan Siddique
>
> Signed-off-by: Numan Siddique
> ---
Hi Numan,
[...]
> @@ -16647,6 +16605,86 @@ lflow_handle_northd_lb_changes(struct ovsdb_idl_txn
> *ovnsb_txn,
> return true;
> }
>
> +bool
> +lflow_handle_lr_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn,
> +struct lr_stateful_tracked_data *trk_data,
> +struct lflow_input *lflow_input,
> +struct lflow_table *lflows)
> +{
> +struct lr_stateful_record *lr_stateful_rec;
> +struct hmapx_node *hmapx_node;
> +
> +HMAPX_FOR_EACH (hmapx_node, _data->crupdated) {
> +lr_stateful_rec = hmapx_node->data;
> +/* Unlink old lflows. */
> +lflow_ref_unlink_lflows(lr_stateful_rec->lflow_ref);
> +
> +/* Generate new lflows. */
> +struct ds match = DS_EMPTY_INITIALIZER;
> +struct ds actions = DS_EMPTY_INITIALIZER;
> +
> +build_lr_stateful_flows(lr_stateful_rec, lflows,
> lflow_input->ls_ports,
> +lflow_input->lr_ports, , ,
> +lflow_input->meter_groups,
> +lflow_input->features);
> +
> +/* Sync the new flows to SB. */
> +bool handled = lflow_ref_sync_lflows(
> +lr_stateful_rec->lflow_ref, lflows, ovnsb_txn,
> +lflow_input->ls_datapaths, lflow_input->lr_datapaths,
> +lflow_input->ovn_internal_version_changed,
> +lflow_input->sbrec_logical_flow_table,
> +lflow_input->sbrec_logical_dp_group_table);
> +if (!handled) {
We leak match and actions here.
> +return false;
> +}
> +
> +struct ovn_port *op;
> +HMAP_FOR_EACH (op, dp_node, _stateful_rec->od->ports) {
> +lflow_ref_unlink_lflows(op->stateful_lflow_ref);
> +
> +build_lbnat_lflows_iterate_by_lrp(op,
> + lflow_input->lr_stateful_table,
> + lflow_input->meter_groups,
> + , ,
> + lflows);
> +
> +handled = lflow_ref_sync_lflows(
> +op->stateful_lflow_ref, lflows, ovnsb_txn,
> +lflow_input->ls_datapaths, lflow_input->lr_datapaths,
> +lflow_input->ovn_internal_version_changed,
> +lflow_input->sbrec_logical_flow_table,
> +lflow_input->sbrec_logical_dp_group_table);
> +if (!handled) {
We leak match and actions here.
> +return false;
> +}
> +
> +if (op->peer && op->peer->nbsp) {
> +lflow_ref_unlink_lflows(op->peer->stateful_lflow_ref);
> +
> +build_lbnat_lflows_iterate_by_lsp(
> +op->peer, lflow_input->lr_stateful_table, ,
> ,
> +lflows);
> +
> +handled = lflow_ref_sync_lflows(
> +op->peer->stateful_lflow_ref, lflows, ovnsb_txn,
> +lflow_input->ls_datapaths, lflow_input->lr_datapaths,
> +lflow_input->ovn_internal_version_changed,
> +lflow_input->sbrec_logical_flow_table,
> +lflow_input->sbrec_logical_dp_group_table);
> +if (!handled) {
We leak match and actions here.
> +return false;
> +}
> +}
> +}
> +
> +ds_destroy();
> +ds_destroy();
> +}
> +
> +return true;
> +}
> +
> static bool
> mirror_needs_update(const struct nbrec_mirror *nb_mirror,
> const struct sbrec_mirror *sb_mirror)
Regards,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v5 11/16] northd: Handle lb changes in lflow engine.
On 1/11/24 16:32, num...@ovn.org wrote:
> From: Numan Siddique
>
> Since northd tracked data has the changed lb data, northd
> engine handler for lflow engine now handles the lb changes
> incrementally. All the lflows generated for each lb is
The first sentence is a bit confusing. What about re-phrasing it to:
Since northd tracked data has the changed lb information, the lflow
change handler for northd inputs can now handle lb updates incrementally.
> stored in the ovn_lb_datapaths->lflow_ref and this is used
> similar to how we handle ovn_port changes.
>
> Signed-off-by: Numan Siddique
> ---
> northd/en-lflow.c | 11 ++---
> northd/lb.c | 3 ++
> northd/lb.h | 26
> northd/lflow-mgr.c | 47 +-
> northd/northd.c | 98 +++--
> northd/northd.h | 4 ++
> tests/ovn-northd.at | 30 ++
> 7 files changed, 184 insertions(+), 35 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index fef9a1352d..205605578f 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -123,11 +123,6 @@ lflow_northd_handler(struct engine_node *node,
> return false;
> }
>
> -/* Fall back to recompute if load balancers have changed. */
> -if (northd_has_lbs_in_tracked_data(_data->trk_data)) {
> -return false;
> -}
> -
> const struct engine_context *eng_ctx = engine_get_context();
> struct lflow_data *lflow_data = data;
>
> @@ -140,6 +135,12 @@ lflow_northd_handler(struct engine_node *node,
> return false;
> }
>
> +if (!lflow_handle_northd_lb_changes(eng_ctx->ovnsb_idl_txn,
> +_data->trk_data.trk_lbs,
> +_input, lflow_data->lflow_table)) {
Nit: indentation
> +return false;
> +}
> +
> engine_set_node_state(node, EN_UPDATED);
> return true;
> }
> diff --git a/northd/lb.c b/northd/lb.c
> index e6c8a51911..5fca41e049 100644
> --- a/northd/lb.c
> +++ b/northd/lb.c
> @@ -21,6 +21,7 @@
>
> /* OVN includes */
> #include "lb.h"
> +#include "lflow-mgr.h"
> #include "lib/lb.h"
> #include "northd.h"
>
> @@ -33,6 +34,7 @@ ovn_lb_datapaths_create(const struct ovn_northd_lb *lb,
> size_t n_ls_datapaths,
> lb_dps->lb = lb;
> lb_dps->nb_ls_map = bitmap_allocate(n_ls_datapaths);
> lb_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths);
> +lb_dps->lflow_ref = lflow_ref_create();
>
> return lb_dps;
> }
> @@ -42,6 +44,7 @@ ovn_lb_datapaths_destroy(struct ovn_lb_datapaths *lb_dps)
> {
> bitmap_free(lb_dps->nb_lr_map);
> bitmap_free(lb_dps->nb_ls_map);
> +lflow_ref_destroy(lb_dps->lflow_ref);
> free(lb_dps);
> }
>
> diff --git a/northd/lb.h b/northd/lb.h
> index eeef2ea260..de677ca4ef 100644
> --- a/northd/lb.h
> +++ b/northd/lb.h
> @@ -20,6 +20,8 @@
> #include "openvswitch/hmap.h"
> #include "uuid.h"
>
> +struct lflow_ref;
> +
> struct ovn_lb_datapaths {
> struct hmap_node hmap_node;
>
> @@ -29,6 +31,30 @@ struct ovn_lb_datapaths {
>
> size_t n_nb_lr;
> unsigned long *nb_lr_map;
> +
> +/* Reference of lflows generated for this load balancer.
> + *
> + * This data is initialized and destroyed by the en_northd node, but
> + * populated and used only by the en_lflow node. Ideally this data should
> + * be maintained as part of en_lflow's data (struct lflow_data): a hash
> + * index from ovn_port key to lflows. However, it would be less
> efficient
> + * and more complex:
> + *
> + * 1. It would require an extra search (using the index) to find the
> + * lflows.
> + *
> + * 2. Building the index needs to be thread-safe, using either a global
> + * lock which is obviously less efficient, or hash-based lock array which
> + * is more complex.
> + *
> + * Maintaining the lflow_ref here is more straightforward. The drawback
> is
> + * that we need to keep in mind that this data belongs to en_lflow node,
> + * so never access it from any other nodes.
> + *
> + * 'lflow_ref' is used to reference logical flows generated for this
> + * load balancer.
> + */
> +struct lflow_ref *lflow_ref;
> };
>
> struct ovn_lb_datapaths *ovn_lb_datapaths_create(const struct ovn_northd_lb
> *,
> diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
> index 3cf9696f6e..6cb2a367fe 100644
> --- a/northd/lflow-mgr.c
> +++ b/northd/lflow-mgr.c
> @@ -375,7 +375,15 @@ struct lflow_ref_node {
> /* The lflow. */
> struct ovn_lflow *lflow;
>
> -/* Index id of the datapath this lflow_ref_node belongs to. */
> +/* Indicates of the lflow was added with dp_group or not using
> + * ovn_lflow_add_with_dp_group() macro. */
> +bool dpgrp_lflow;
> +/* dpgrp bitmap and bitmap length. Valid only of dpgrp_lflow is true. */
> +unsigned long *dpgrp_bitmap;
> +size_t dpgrp_bitmap_len;
Instead of
Re: [ovs-dev] [PATCH ovn v5 08/16] northd: Refactor lflow management into a separate module.
On 1/11/24 16:31, num...@ovn.org wrote:
> +
> +void
> +lflow_table_add_lflow(struct lflow_table *lflow_table,
> + const struct ovn_datapath *od,
> + const unsigned long *dp_bitmap, size_t dp_bitmap_len,
> + enum ovn_stage stage, uint16_t priority,
> + const char *match, const char *actions,
> + const char *io_port, const char *ctrl_meter,
> + const struct ovsdb_idl_row *stage_hint,
> + const char *where,
> + struct lflow_ref *lflow_ref)
> +OVS_EXCLUDED(fake_hash_mutex)
> +{
> +struct ovs_mutex *hash_lock;
> +uint32_t hash;
> +
> +ovs_assert(!od ||
> + ovn_stage_to_datapath_type(stage) ==
> ovn_datapath_get_type(od));
> +
> +hash = ovn_logical_flow_hash(ovn_stage_get_table(stage),
> + ovn_stage_get_pipeline(stage),
> + priority, match,
> + actions);
> +
> +hash_lock = lflow_hash_lock(_table->entries, hash);
> +struct ovn_lflow *lflow =
> +do_ovn_lflow_add(lflow_table, od, dp_bitmap,
> + dp_bitmap_len, hash, stage,
> + priority, match, actions,
> + io_port, ctrl_meter, stage_hint, where);
> +
> +if (lflow_ref) {
> +/* lflow referencing is only supported if 'od' is not NULL. */
> +ovs_assert(od);
> +
> +struct lflow_ref_node *lrn =
> +lflow_ref_node_find(_ref->lflow_ref_nodes, lflow, hash);
> +if (!lrn) {
> +lrn = xzalloc(sizeof *lrn);
> +lrn->lflow = lflow;
> +lrn->dp_index = od->index;
> +ovs_list_insert(_ref->lflows_ref_list,
> +>lflow_list_node);
> +inc_dp_refcnt(>dp_refcnts_map, lrn->dp_index);
> +ovs_list_insert(>referenced_by, >ref_list_node);
> +
> +hmap_insert(_ref->lflow_ref_nodes, >ref_node, hash);
> +}
> +
> +lrn->linked = true;
> +}
> +
> +lflow_hash_unlock(hash_lock);
> +
> +}
> +
This part is not thread safe.
If two threads try to add logical flows that have different hashes and
lflow_ref is not NULL we're going to have a race condition when
inserting to the _ref->lflow_ref_nodes hash map because the two
threads will take different locks.
That might corrupt the map.
I guess, if we don't want to cause more performance degradation we
should maintain as many lflow_ref instances as we do hash_locks, i.e.,
LFLOW_HASH_LOCK_MASK + 1. Will that even be possible?
Wdyt?
Regards,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v5 10/16] northd: Move ovn_lb_datapaths from lib to northd module.
On 1/11/24 16:32, num...@ovn.org wrote:
> From: Numan Siddique
>
> Signed-off-by: Numan Siddique
> ---
In this case I think we should move all northd specific LB code from
lib/lb.c to northd/lb.c. We should probably move all ovn-controller
specific LB code from lib/lb.c to a new controller/lb.c.
I tried that here:
https://github.com/dceara/ovn/commit/3f95e98a92828847ead8aa330702ea28b002590f
Feel free to use it if it looks OK to you too.
One more minor comment inline.
Thanks,
Dumitru
> lib/lb.c| 96 ---
> lib/lb.h| 57 ---
> northd/automake.mk | 4 +-
> northd/en-lr-stateful.c | 1 +
> northd/lb.c | 121
> northd/lb.h | 82 +++
> northd/northd.c | 1 +
> 7 files changed, 208 insertions(+), 154 deletions(-)
> create mode 100644 northd/lb.c
> create mode 100644 northd/lb.h
>
> diff --git a/lib/lb.c b/lib/lb.c
> index d0d562b6fb..991f20299d 100644
> --- a/lib/lb.c
> +++ b/lib/lb.c
> @@ -1071,99 +1071,3 @@ remove_ips_from_lb_ip_set(struct ovn_lb_ip_set *lb_ips,
> }
> }
> }
> -
> -/* lb datapaths functions */
> -struct ovn_lb_datapaths *
> -ovn_lb_datapaths_create(const struct ovn_northd_lb *lb, size_t
> n_ls_datapaths,
> -size_t n_lr_datapaths)
> -{
> -struct ovn_lb_datapaths *lb_dps = xzalloc(sizeof *lb_dps);
> -lb_dps->lb = lb;
> -lb_dps->nb_ls_map = bitmap_allocate(n_ls_datapaths);
> -lb_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths);
> -
> -return lb_dps;
> -}
> -
> -struct ovn_lb_datapaths *
> -ovn_lb_datapaths_find(const struct hmap *lb_dps_map,
> - const struct uuid *lb_uuid)
> -{
> -struct ovn_lb_datapaths *lb_dps;
> -size_t hash = uuid_hash(lb_uuid);
> -HMAP_FOR_EACH_WITH_HASH (lb_dps, hmap_node, hash, lb_dps_map) {
> -if (uuid_equals(_dps->lb->nlb->header_.uuid, lb_uuid)) {
> -return lb_dps;
> -}
> -}
> -return NULL;
> -}
> -
> -void
> -ovn_lb_datapaths_destroy(struct ovn_lb_datapaths *lb_dps)
> -{
> -bitmap_free(lb_dps->nb_lr_map);
> -bitmap_free(lb_dps->nb_ls_map);
> -free(lb_dps);
> -}
> -
> -void
> -ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths *lb_dps, size_t n,
> -struct ovn_datapath **ods)
> -{
> -for (size_t i = 0; i < n; i++) {
> -if (!bitmap_is_set(lb_dps->nb_lr_map, ods[i]->index)) {
> -bitmap_set1(lb_dps->nb_lr_map, ods[i]->index);
> -lb_dps->n_nb_lr++;
> -}
> -}
> -}
> -
> -void
> -ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *lb_dps, size_t n,
> -struct ovn_datapath **ods)
> -{
> -for (size_t i = 0; i < n; i++) {
> -if (!bitmap_is_set(lb_dps->nb_ls_map, ods[i]->index)) {
> -bitmap_set1(lb_dps->nb_ls_map, ods[i]->index);
> -lb_dps->n_nb_ls++;
> -}
> -}
> -}
> -
> -struct ovn_lb_group_datapaths *
> -ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group,
> - size_t max_ls_datapaths,
> - size_t max_lr_datapaths)
> -{
> -struct ovn_lb_group_datapaths *lb_group_dps =
> -xzalloc(sizeof *lb_group_dps);
> -lb_group_dps->lb_group = lb_group;
> -lb_group_dps->ls = xmalloc(max_ls_datapaths * sizeof *lb_group_dps->ls);
> -lb_group_dps->lr = xmalloc(max_lr_datapaths * sizeof *lb_group_dps->lr);
> -
> -return lb_group_dps;
> -}
> -
> -void
> -ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths *lb_group_dps)
> -{
> -free(lb_group_dps->ls);
> -free(lb_group_dps->lr);
> -free(lb_group_dps);
> -}
> -
> -struct ovn_lb_group_datapaths *
> -ovn_lb_group_datapaths_find(const struct hmap *lb_group_dps_map,
> -const struct uuid *lb_group_uuid)
> -{
> -struct ovn_lb_group_datapaths *lb_group_dps;
> -size_t hash = uuid_hash(lb_group_uuid);
> -
> -HMAP_FOR_EACH_WITH_HASH (lb_group_dps, hmap_node, hash,
> lb_group_dps_map) {
> -if (uuid_equals(_group_dps->lb_group->uuid, lb_group_uuid)) {
> -return lb_group_dps;
> -}
> -}
> -return NULL;
> -}
> diff --git a/lib/lb.h b/lib/lb.h
> index b8e3c1e8fb..90ac39ee5a 100644
> --- a/lib/lb.h
> +++ b/lib/lb.h
> @@ -167,63 +167,6 @@ void ovn_lb_group_reinit(
> const struct nbrec_load_balancer_group *,
> const struct hmap *lbs);
>
> -struct ovn_lb_datapaths {
> -struct hmap_node hmap_node;
> -
> -const struct ovn_northd_lb *lb;
> -size_t n_nb_ls;
> -unsigned long *nb_ls_map;
> -
> -size_t n_nb_lr;
> -unsigned long *nb_lr_map;
> -};
> -
> -struct ovn_lb_datapaths *ovn_lb_datapaths_create(const struct ovn_northd_lb
> *,
> - size_t n_ls_datapaths,
> - size_t
Re: [ovs-dev] [PATCH ovn] northd: Use proper field for lookup_nd
Hi Ales
The patch looks good to me, Thanks!
Acked-by: Xavier Simonart
Thanks
Xavier
On Thu, Jan 18, 2024 at 3:54 PM Ales Musil wrote:
> We are intentionally skipping ND NA with LLA as source.
> However, this doesn't work when the ND NA has LLA source,
> but the target address is global one. In that case we
> would skip update of already existing entry when
> always_learn_from_arp_request is set to false.
>
> Use ND target address in the check instead of source.
>
> Fixes: 5e0cb03605ea ("northd: Add logical flow to skip GARP with LLA")
> Reported-at: https://issues.redhat.com/browse/FDP-283
> Signed-off-by: Ales Musil
> ---
> northd/northd.c | 4 ++--
> tests/ovn.at| 25 ++---
> 2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 952f8200d..de15ca101 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -13171,9 +13171,9 @@ build_neigh_learning_flows_for_lrouter(
> * address, the all-nodes multicast address. */
> ds_clear(actions);
> ds_put_format(actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
> - " = lookup_nd(inport, ip6.src, nd.tll); "
> + " = lookup_nd(inport, nd.target, nd.tll); "
> REGBIT_LOOKUP_NEIGHBOR_IP_RESULT
> - " = lookup_nd_ip(inport, ip6.src); next;");
> + " = lookup_nd_ip(inport, nd.target);
> next;");
> ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 110,
>"nd_na && ip6.src == fe80::/10 && ip6.dst ==
> ff00::/8",
>ds_cstr(actions));
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 2dd46fd79..26c516ef9 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -5385,10 +5385,11 @@ test_arp() {
> }
>
> test_na() {
> -local inport=$1 sha=$2 spa=$3
> +local inport=$1 sha=$2 spa=$3 src=${4-$3}
> local request=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff',
> src='${sha}')/ \
> - IPv6(dst='ff01::1', src='${spa}')/ \
> - ICMPv6ND_NA(tgt='${spa}')")
> + IPv6(dst='ff01::1', src='${src}')/ \
> + ICMPv6ND_NA(tgt='${spa}')/ \
> + ICMPv6NDOptDstLLAddr(lladdr='${sha}')")
>
> hv=hv`vif_to_hv $inport`
> as $hv ovs-appctl netdev-dummy/receive vif$inport $request
> @@ -5464,6 +5465,24 @@ for i in 1 2; do
> done
> done
>
> +# Make sure that we can update existing entry with
> +# "always_learn_from_arp_request=false" even when the source of NA src is
> LLA.
> +ovn-sbctl --all destroy mac_binding
> +ovn-nbctl --wait=hv set logical_router lr0
> options:always_learn_from_arp_request=false
> +
> +sha="f0:00:00:00:00:11"
> +spa6="fd00::abcd:1"
> +
> +test_na 11 $sha $spa6
> +wait_row_count MAC_Binding 1 ip=\"$spa6\" mac=\"$sha\"
> +
> +sha="f0:00:00:00:00:12"
> +lla6="fe80::abcd:1"
> +
> +test_na 11 $sha $spa6 $lla6
> +wait_row_count MAC_Binding 1 ip=\"$spa6\" mac=\"$sha\"
> +check_row_count MAC_Binding 0 ip=\"$lla6\"
> +
> # Gracefully terminate daemons
> OVN_CLEANUP([hv1], [hv2])
>
> --
> 2.43.0
>
> ___
> 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] tests: mcast-snooping: Stop time for the group protocol test.
On Thu, Jan 18, 2024 at 08:25:23PM +0100, Ilya Maximets wrote:
> Otherwise, it randomly fails due to age not being zero under load:
>
> tests/mcast-snooping.at:645: ovs-appctl mdb/show br0
> --- -
> +++ /at-groups/2592/stdout
> @@ -1,5 +1,5 @@
> port VLAN protocol GROUPAge
> -1 0 IGMPv1224.1.1.1 0
> +1 0 IGMPv1224.1.1.1 1
>1 0 IGMPv2224.1.1.2 0
>1 0 IGMPv3233.54.12.230 0
>
> Fixes: b222593bc69b ("mcast-snooping: Add group protocol to mdb/show output.")
> Signed-off-by: Ilya Maximets
Thanks.
On my otherwise idle system, I ran the test updated by this patch
in a loop.
Without this patch, it failed on the 55th iteration.
With this patch it successfully ran 1000 iterations
(the target for my experiment).
Not being very scientific, I did not try this experiment a second time.
Acked-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
