Re: [ovs-dev] [PATCH ovn 4/4] pinctrl: Skip non-local mac bindings in run_buffered_binding().
Acked-by: Mark Michelson On 11/1/24 09:29, Dumitru Ceara wrote: There's no point to process MAC binding updates for non-local datapaths. If they were local before but not anymore we don't really have a reason to send any buffered packets for them anymore. Fixes: bbd5e0d81c16 ("controller: improve buffered packets management") Signed-off-by: Dumitru Ceara --- controller/pinctrl.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/controller/pinctrl.c b/controller/pinctrl.c index dfb2560a97..07cd140e02 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -191,6 +191,7 @@ static void init_buffered_packets_ctx(void); static void destroy_buffered_packets_ctx(void); static void run_buffered_binding(const struct sbrec_mac_binding_table *mac_binding_table, + const struct hmap *local_datapaths, struct ovsdb_idl_index *sbrec_port_binding_by_key, struct ovsdb_idl_index *sbrec_datapath_binding_by_key, struct ovsdb_idl_index *sbrec_port_binding_by_name, @@ -4176,7 +4177,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, sbrec_port_binding_by_key, sbrec_igmp_groups, sbrec_ip_multicast_opts); -run_buffered_binding(mac_binding_table, sbrec_port_binding_by_key, +run_buffered_binding(mac_binding_table, local_datapaths, + sbrec_port_binding_by_key, sbrec_datapath_binding_by_key, sbrec_port_binding_by_name, sbrec_mac_binding_by_lport_ip); @@ -4943,6 +4945,7 @@ run_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn, static void run_buffered_binding(const struct sbrec_mac_binding_table *mac_binding_table, + const struct hmap *local_datapaths, struct ovsdb_idl_index *sbrec_port_binding_by_key, struct ovsdb_idl_index *sbrec_datapath_binding_by_key, struct ovsdb_idl_index *sbrec_port_binding_by_name, @@ -4961,6 +4964,10 @@ run_buffered_binding(const struct sbrec_mac_binding_table *mac_binding_table, continue; } +if (!get_local_datapath(local_datapaths, smb->datapath->tunnel_key)) { +continue; +} + const struct sbrec_port_binding *pb = lport_lookup_by_name( sbrec_port_binding_by_name, smb->logical_port); ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 4/4] pinctrl: Skip non-local mac bindings in run_buffered_binding().
Acked-by: Mark Michelson On 11/1/24 09:29, Dumitru Ceara wrote: There's no point to process MAC binding updates for non-local datapaths. If they were local before but not anymore we don't really have a reason to send any buffered packets for them anymore. Fixes: bbd5e0d81c16 ("controller: improve buffered packets management") Signed-off-by: Dumitru Ceara --- controller/pinctrl.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/controller/pinctrl.c b/controller/pinctrl.c index dfb2560a97..07cd140e02 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -191,6 +191,7 @@ static void init_buffered_packets_ctx(void); static void destroy_buffered_packets_ctx(void); static void run_buffered_binding(const struct sbrec_mac_binding_table *mac_binding_table, + const struct hmap *local_datapaths, struct ovsdb_idl_index *sbrec_port_binding_by_key, struct ovsdb_idl_index *sbrec_datapath_binding_by_key, struct ovsdb_idl_index *sbrec_port_binding_by_name, @@ -4176,7 +4177,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, sbrec_port_binding_by_key, sbrec_igmp_groups, sbrec_ip_multicast_opts); -run_buffered_binding(mac_binding_table, sbrec_port_binding_by_key, +run_buffered_binding(mac_binding_table, local_datapaths, + sbrec_port_binding_by_key, sbrec_datapath_binding_by_key, sbrec_port_binding_by_name, sbrec_mac_binding_by_lport_ip); @@ -4943,6 +4945,7 @@ run_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn, static void run_buffered_binding(const struct sbrec_mac_binding_table *mac_binding_table, + const struct hmap *local_datapaths, struct ovsdb_idl_index *sbrec_port_binding_by_key, struct ovsdb_idl_index *sbrec_datapath_binding_by_key, struct ovsdb_idl_index *sbrec_port_binding_by_name, @@ -4961,6 +4964,10 @@ run_buffered_binding(const struct sbrec_mac_binding_table *mac_binding_table, continue; } +if (!get_local_datapath(local_datapaths, smb->datapath->tunnel_key)) { +continue; +} + const struct sbrec_port_binding *pb = lport_lookup_by_name( sbrec_port_binding_by_name, smb->logical_port); ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/4] mac-cache: Properly handle deletion of SB mac_bindings.
Acked-by: Mark Michelson On 11/1/24 09:29, Dumitru Ceara wrote: SB.MAC_Binding doesn't store a strong reference to SB.Port_Binding. Instead it stores the port binding 'logical_port' name. As the mac-binding cache in ovn-controller stored entries indexed by [datapath-key, port-key, ip] it meant that when processing SB.MAC_Binding changes (including deletion) it had to first lookup the corresponding SB.Port_Binding. If the logical port associated with the MAC binding is deleted in the same transaction then looking it up in the IDL index fails (deleted IDL records are not indexed). This leads to stale entries staying in the mac cache and in consequence to potential use after free (of the SB MAC_Binding record) or to crashes if the statctrl module tries to update timestamps of MAC_Bindings that happened to have been removed in the current iteration (but still linger in the cache). In practice we don't need the port-key we can simply use the port binding UUID as unique index. However, the same struct mac_binding type is used by pinctrl to temporarily efficiently store buffered packets for yet to be resolved MAC bindings. In those cases there's no SB MAC_Binding record yet so the logical ingress port tunnel_key is used as index. > The two ways of using the MAC binding maps are orthogonal, that is, we will never use a single map to store both types of bindings. In order to avoid issues we now include the SB MAC_Binding UUID into the key of the struct mac_binding_data. Reported-at: https://issues.redhat.com/browse/FDP-906 Fixes: b57f60a617b4 ("controller: Add MAC cache I-P node") Signed-off-by: Dumitru Ceara --- controller/mac-cache.c | 77 ++--- controller/mac-cache.h | 30 +++--- controller/ovn-controller.c | 67 ++ controller/pinctrl.c| 24 +--- tests/ovn.at| 110 5 files changed, 236 insertions(+), 72 deletions(-) diff --git a/controller/mac-cache.c b/controller/mac-cache.c index de45d7a6a5..d6ba9e20ee 100644 --- a/controller/mac-cache.c +++ b/controller/mac-cache.c @@ -142,21 +142,18 @@ mac_cache_thresholds_clear(struct mac_cache_data *data) } /* MAC binding. */ -struct mac_binding * +void mac_binding_add(struct hmap *map, struct mac_binding_data mb_data, long long timestamp) { struct mac_binding *mb = mac_binding_find(map, &mb_data); if (!mb) { mb = xmalloc(sizeof *mb); -mb->sbrec_mb = NULL; hmap_insert(map, &mb->hmap_node, mac_binding_data_hash(&mb_data)); } mb->data = mb_data; mb->timestamp = timestamp; - -return mb; } void @@ -181,24 +178,37 @@ mac_binding_find(const struct hmap *map, } bool -mac_binding_data_from_sbrec(struct mac_binding_data *data, -const struct sbrec_mac_binding *mb, -struct ovsdb_idl_index *sbrec_pb_by_name) +mac_binding_data_parse(struct mac_binding_data *data, + uint32_t dp_key, uint32_t port_key, + const char *ip_str, const char *mac_str) { -const struct sbrec_port_binding *pb = -lport_lookup_by_name(sbrec_pb_by_name, mb->logical_port); +struct eth_addr mac; +struct in6_addr ip; -if (!pb || !pb->datapath || !ip46_parse(mb->ip, &data->ip) || -!eth_addr_from_string(mb->mac, &data->mac)) { +if (!ip46_parse(ip_str, &ip) || !eth_addr_from_string(mac_str, &mac)) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); -VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s, mac=%s, " - "logical_port=%s", mb->ip, mb->mac, mb->logical_port); +VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s, mac=%s", + ip_str, mac_str); return false; } -data->dp_key = mb->datapath->tunnel_key; -data->port_key = pb->tunnel_key; +mac_binding_data_init(data, dp_key, port_key, ip, mac); +return true; +} + +bool +mac_binding_data_from_sbrec(struct mac_binding_data *data, +const struct sbrec_mac_binding *mb) +{ +/* This explicitly sets the port_key to 0 as port_binding tunnel_keys + * can change. Instead use add the SB.MAC_Binding UUID as key; this + * makes the mac_binding_data key unique. */ +if (!mac_binding_data_parse(data, mb->datapath->tunnel_key, 0, +mb->ip, mb->mac)) { +return false; +} +data->sbrec = mb; return true; } @@ -211,6 +221,30 @@ mac_bindings_clear(struct hmap *map) } } +void +mac_bindings_to_string(const struct hmap *map, struct ds *out_data) +{ +struct mac_binding *mb; +HMAP_FO
Re: [ovs-dev] [PATCH ovn 1/4] pinctrl: Use correct map size in pinctrl_handle_put_fdb().
Acked-by: Mark Michelson On 11/1/24 09:29, Dumitru Ceara wrote: Fixes: fb96ae36793a ("controller: Merge the mac-cache and mac-learn.") Signed-off-by: Dumitru Ceara --- controller/pinctrl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller/pinctrl.c b/controller/pinctrl.c index b891435c12..dfb3130aa2 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -8981,7 +8981,7 @@ static void pinctrl_handle_put_fdb(const struct flow *md, const struct flow *headers) OVS_REQUIRES(pinctrl_mutex) { -if (hmap_count(&put_mac_bindings) >= MAX_FDB_ENTRIES) { +if (hmap_count(&put_fdbs) >= MAX_FDB_ENTRIES) { COVERAGE_INC(pinctrl_drop_put_mac_binding); return; } ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Revert "controller: Properly handle localnet flows in I+P.".
Thanks for the fix Xavier. Acked-by: Mark Michelson On 10/31/24 13:10, Xavier Simonart wrote: This reverts commit edc064b4c589ab1bb69352523481bd6d997aa1ca. In setups with multiple localnet ports w/o vlans (or multiple localnet ports with the same vlans), that patch was trying to create the same flows with action conjunction multiple times, which caused the following assert: 0 0x777cd834 in __pthread_kill_implementation () from /lib64/libc.so.6 1 0x7777b8ee in raise () from /lib64/libc.so.6 2 0x777638ff in abort () from /lib64/libc.so.6 3 0x0042f861 in flow_is_preferred (a=0xafe9a0, b=0xaf9380) at controller/ofctrl.c:966 4 0x0042f340 in link_installed_to_desired (i=0xb2eaf0, d=0xafe9a0) at controller/ofctrl.c:987 5 0x0042c17c in update_installed_flows_by_track (flow_table=0x813a80, bc=0x7ffcc740, installed_flows=0x7894e0 , msgs=0x7ffcc790) at controller/ofctrl.c:2583 6 0x0042af14 in ofctrl_put (lflow_table=0x810180, pflow_table=0x813a80, pending_ct_zones=0x8129b0, pending_lb_tuples=0x80e030, sbrec_meter_by_name=0x7e6840, req_cfg=0, lflows_changed=true, pflows_changed=true) at controller/ofctrl.c:2826 7 0x0045015c in main (argc=1, argv=0x7fffe218) at controller/ovn-controller.c:5788 Reverting that patch means that flows such as table_id=0, priority=180, vlan_tci=0x/0x1000, actions=conjunction(100,2/2) remains after the localnet port is deleted. Fixes: edc064b4c589 ("controller: Properly handle localnet flows in I+P.") Reported-at: https://issues.redhat.com/browse/FDP-926 Signed-off-by: Xavier Simonart --- controller/physical.c | 9 ++-- tests/ovn.at | 98 +++ 2 files changed, 46 insertions(+), 61 deletions(-) diff --git a/controller/physical.c b/controller/physical.c index 2aaa16cbd..c6db4f376 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -699,7 +699,7 @@ put_replace_chassis_mac_flows(const struct shash *ct_zones, put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 180, rport_binding->header_.uuid.parts[0], -&match, ofpacts_p, &localnet_port->header_.uuid); +&match, ofpacts_p, hc_uuid); /* Provide second search criteria, i.e localnet port's * vlan ID for conjunction flow */ @@ -719,7 +719,7 @@ put_replace_chassis_mac_flows(const struct shash *ct_zones, conj->clause = 1; ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 180, rport_binding->header_.uuid.parts[0], -&match, ofpacts_p, &localnet_port->header_.uuid); +&match, ofpacts_p, hc_uuid); } } @@ -2393,9 +2393,8 @@ physical_handle_flows_for_lport(const struct sbrec_port_binding *pb, struct local_datapath *ldp = get_local_datapath(p_ctx->local_datapaths, pb->datapath->tunnel_key); -if (!strcmp(pb->type, "external") || -!strcmp(pb->type, "patch") || !strcmp(pb->type, "l3gateway")) { -/* Those lports have a dependency on the localnet port. +if (!strcmp(pb->type, "external")) { +/* External lports have a dependency on the localnet port. * We need to remove the flows of the localnet port as well * and re-consider adding the flows for it. */ diff --git a/tests/ovn.at b/tests/ovn.at index 10cd7a79b..dfb08fd1e 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -39495,62 +39495,6 @@ OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) OVN_CLEANUP([hv1]) AT_CLEANUP ]) -OVN_FOR_EACH_NORTHD([ -AT_SETUP([localnet port flows after deletion]) -ovn_start -net_add n1 - -check ovn-nbctl ls-add sw0 - -for i in 1 2; do -check ovn-nbctl lsp-add sw0 sw0-p${i} -- lsp-set-addresses sw0-p${i} "00:00:10:01:02:0${i} 10.0.0.${i}" -sim_add hv${i} -as hv${i} -ovs-vsctl add-br br-phys -ovn_attach n1 br-phys 192.168.0.${i} -ovs-vsctl set open . external_ids:ovn-bridge-mappings=physnet1:br-phys -ovs-vsctl add-port br-int vif${i} -- \ -set Interface vif${i} external-ids:iface-id=sw0-p${i} \ - options:tx_pcap=hv${i}/vif${i}-tx.pcap \ - options:rxq_pcap=hv${i}/vif${i}-rx.pcap -done - -check ovn-nbctl lr-add lr0 -check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.254/24 -check ovn-nbctl lsp-add sw0 sw0-lr0 -check ovn-nbctl lsp-set-type sw0-lr0 router -check ovn-nbctl lsp-set-addresses sw0-lr0 router -check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 - -check ovn-nbctl --wait=hv sync -wait_for_ports_up - -# We should not have any flows in table OFTABLE_PHY_TO_LOG from in_port different from vif1 and ovn-hv2-0
[ovs-dev] [PATCH ovn v2] northd: Track max ACL tiers more accurately.
When ACL tiers were introduced, the code kept track of the highest ACL tier so that when iterating through ACL tiers, we would not attempt to advance the current tier past the highest configured tier. Unfortunately, keeping track of a single max ACL tier doesn't work when ACLs are evaluated in three separate places. ACLs can be evaluated on ingress before load balancing, on ingress after load balancing, and on egress. By only keeping track of a single max ACL tier, it means that we could perform superfluous checks if one stage of ACLs has a higher max tier than other stages. As an example, if ingress pre-load balancing ACLs have a maximum tier of 1, and egress ACLs have a maximum tier of 2, then it means that for all stages of ACLs, we will evaluate tiers 0, 1, and 2 of ACLs, even though only one stage of ACLs uses tier 2. >From a pure functionality standpoint, this doesn't cause any issues. Even if we advance the tier past the highest configured value, it results in a no-op and the same net result happens. However, the addition of sampling into ACLs has caused an unwanted side effect. In the example scenario above, let's say the tier 1 ACL in the ingress pre-load balancing stage evaluates to "pass". After the evaluation, we send a sample for the "pass" result. We then advance the tier to 2, then move back to ACL evaluation. There are no tier 2 ACLs, so we move on to the sampling stage again. We then send a second sample for the previous "pass" result from tier 1. The result is confusing since we've sent two samples for the same ACL evaluation. To remedy this, we now track the max ACL tier in each of the stages where ACLs are evaluated. Now there are no superfluous ACL evaluations and no superfluous samples sent either. Reported-at: https://issues.redhat.com/browse/FDP-760 Signed-off-by: Mark Michelson --- v1 -> v2: * Add a comment to the memcmp() call to caution in case the acl_tier struct is altered and has padding added. * Switch to using MAX macro when assigning max ACL tiers. * Add a system test to ensure the referenced issue is fixed. For the record, this test fails on the main branch, but passes with this patch. --- northd/en-ls-stateful.c | 54 +++-- northd/en-ls-stateful.h | 8 +- northd/northd.c | 47 ++- tests/ovn-northd.at | 34 ++ tests/system-ovn.at | 51 ++ 5 files changed, 174 insertions(+), 20 deletions(-) diff --git a/northd/en-ls-stateful.c b/northd/en-ls-stateful.c index 44f74ea08..11e97aa9b 100644 --- a/northd/en-ls-stateful.c +++ b/northd/en-ls-stateful.c @@ -204,16 +204,22 @@ ls_stateful_port_group_handler(struct engine_node *node, void *data_) ovn_datapaths_find_by_index(input_data.ls_datapaths, ls_stateful_rec->ls_index); bool had_stateful_acl = ls_stateful_rec->has_stateful_acl; -uint64_t max_acl_tier = ls_stateful_rec->max_acl_tier; +struct acl_tier old_max = ls_stateful_rec->max_acl_tier; bool had_acls = ls_stateful_rec->has_acls; bool modified = false; ls_stateful_record_reinit(ls_stateful_rec, od, ls_pg, input_data.ls_port_groups); +struct acl_tier new_max = ls_stateful_rec->max_acl_tier; + +/* Using memcmp for struct acl_tier is fine since there is no padding + * in the struct. However, if the structure is changed, the memcmp + * may need to be updated to compare individual struct fields. + */ if ((had_stateful_acl != ls_stateful_rec->has_stateful_acl) || (had_acls != ls_stateful_rec->has_acls) -|| max_acl_tier != ls_stateful_rec->max_acl_tier) { +|| memcmp(&old_max, &new_max, sizeof(old_max))) { modified = true; } @@ -365,7 +371,8 @@ ls_stateful_record_set_acl_flags(struct ls_stateful_record *ls_stateful_rec, const struct ls_port_group_table *ls_pgs) { ls_stateful_rec->has_stateful_acl = false; -ls_stateful_rec->max_acl_tier = 0; +memset(&ls_stateful_rec->max_acl_tier, 0, + sizeof ls_stateful_rec->max_acl_tier); ls_stateful_rec->has_acls = false; if (ls_stateful_record_set_acl_flags_(ls_stateful_rec, od->nbs->acls, @@ -391,6 +398,38 @@ ls_stateful_record_set_acl_flags(struct ls_stateful_record *ls_stateful_rec, } } +static void +update_ls_max_acl_tier(struct ls_stateful_record *ls_stateful_rec, + const struct nbrec_acl *acl) +{ +if (!acl->tier) { +return; +} + +uint64_t *tier; + +if (!strcmp(acl->direction, "from-lport")) { +if (smap_get_bool(&acl->options, "apply
Re: [ovs-dev] [PATCH ovn] northd: Track max ACL tiers more accurately.
On 10/8/24 06:30, Ales Musil wrote: On Fri, Sep 27, 2024 at 7:15 PM Mark Michelson <mailto:mmich...@redhat.com>> wrote: When ACL tiers were introduced, the code kept track of the highest ACL tier so that when iterating through ACL tiers, we would not attempt to advance the current tier past the highest configured tier. Unfortunately, keeping track of a single max ACL tier doesn't work when ACLs are evaluated in three separate places. ACLs can be evaluated on ingress before load balancing, on ingress after load balancing, and on egress. By only keeping track of a single max ACL tier, it means that we could perform superfluous checks if one stage of ACLs has a higher max tier than other stages. As an example, if ingress pre-load balancing ACLs have a maximum tier of 1, and egress ACLs have a maximum tier of 2, then it means that for all stages of ACLs, we will evaluate tiers 0, 1, and 2 of ACLs, even though only one stage of ACLs uses tier 2. From a pure functionality standpoint, this doesn't cause any issues. Even if we advance the tier past the highest configured value, it results in a no-op and the same net result happens. However, the addition of sampling into ACLs has caused an unwanted side effect. In the example scenario above, let's say the tier 1 ACL in the ingress pre-load balancing stage evaluates to "pass". After the evaluation, we send a sample for the "pass" result. We then advance the tier to 2, then move back to ACL evaluation. There are no tier 2 ACLs, so we move on to the sampling stage again. We then send a second sample for the previous "pass" result from tier 1. The result is confusing since we've sent two samples for the same ACL evaluation. To remedy this, we now track the max ACL tier in each of the stages where ACLs are evaluated. Now there are no superfluous ACL evaluations and no superfluous samples sent either. Reported-at: https://issues.redhat.com/browse/FDP-760 <https://issues.redhat.com/browse/FDP-760> Signed-off-by: Mark Michelson mailto:mmich...@redhat.com>> --- Hi Mark, thank you for the patch. I'm not sure if this fixes the originally reported issue. For that issue we should generate generic sample flows that also match on tier. That should result in samples being generated for the appropriate tier. Either way we should have a test case to mimic the behavior described by the FDP-760. I have a few additional comments down below. I think your suggestion would also fix the issue, but my method does as well. If you check the ovn-trace output on the issue, you can see that the only reason that ACLs get re-evaluated for tier 2 is that the ACL_ACTION stage sends control back to the ACL_EVAL stage since it mistakenly thinks there's another tier to evaluate. By not sending control back to ACL_EVAL after tier 1, there is no chance of sending a bogus sample since we won't reach the ACL_SAMPLE stage again. That being said, I can incorporate your suggestion into v2 of the patch since there may be other weird corner cases that could trigger phantom samples besides a bad max ACL tier count. You're correct about the test case. I had taken the shortcut of making sure that evaluation wouldn't reach a phantom tier just by confirming logical flows. But having a system test to ensure the proper behavior is a good call. northd/en-ls-stateful.c | 48 ++--- northd/en-ls-stateful.h | 8 ++- northd/northd.c | 47 ++-- tests/ovn-northd.at <http://ovn-northd.at> | 34 + 4 files changed, 117 insertions(+), 20 deletions(-) diff --git a/northd/en-ls-stateful.c b/northd/en-ls-stateful.c index 44f74ea08..eb751d8dd 100644 --- a/northd/en-ls-stateful.c +++ b/northd/en-ls-stateful.c @@ -204,16 +204,18 @@ ls_stateful_port_group_handler(struct engine_node *node, void *data_) ovn_datapaths_find_by_index(input_data.ls_datapaths, ls_stateful_rec->ls_index); bool had_stateful_acl = ls_stateful_rec->has_stateful_acl; - uint64_t max_acl_tier = ls_stateful_rec->max_acl_tier; + struct acl_tier old_max = ls_stateful_rec->max_acl_tier; bool had_acls = ls_stateful_rec->has_acls; bool modified = false; ls_stateful_record_reinit(ls_stateful_rec, od, ls_pg, input_data.ls_port_groups); + struct acl_tier new_max = ls_stateful_rec->max_acl_tier; + if ((had_stateful_acl != ls_stateful_rec->has_stateful_acl) || (had_acls != ls_stateful_rec->has_ac
Re: [ovs-dev] [PATCH ovn 3/5] northd: Use the same UUID for SB representation of DNS.
Acked-by: Mark Michelson On 10/18/24 06:09, Ales Musil wrote: The NB DNS has exactly one corresponding row in SB database. Use the NB UUID for the SB representation which makes the processing easier and the link between the rows is obvious at first glance. Signed-off-by: Ales Musil --- controller/pinctrl.c | 76 northd/northd.c | 21 +++- 2 files changed, 45 insertions(+), 52 deletions(-) diff --git a/controller/pinctrl.c b/controller/pinctrl.c index b891435c1..8a681ede7 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -3273,6 +3273,8 @@ put_be32(struct ofpbuf *buf, ovs_be32 x) } struct dns_data { +struct hmap_node hmap_node; +struct uuid uuid; uint64_t *dps; size_t n_dps; struct smap records; @@ -3280,7 +3282,20 @@ struct dns_data { bool delete; }; -static struct shash dns_cache = SHASH_INITIALIZER(&dns_cache); +static struct hmap dns_cache = HMAP_INITIALIZER(&dns_cache); + +static struct dns_data * +dns_cache_find_data(const struct uuid *uuid) +{ +struct dns_data *dns_data; +size_t hash = uuid_hash(uuid); +HMAP_FOR_EACH_WITH_HASH (dns_data, hmap_node, hash, &dns_cache) { +if (uuid_equals(&dns_data->uuid, uuid)) { +return dns_data; +} +} +return NULL; +} /* Called by pinctrl_run(). Runs within the main ovn-controller * thread context. */ @@ -3288,25 +3303,20 @@ static void sync_dns_cache(const struct sbrec_dns_table *dns_table) OVS_REQUIRES(pinctrl_mutex) { -struct shash_node *iter; -SHASH_FOR_EACH (iter, &dns_cache) { -struct dns_data *d = iter->data; -d->delete = true; +struct dns_data *dns_data; +HMAP_FOR_EACH (dns_data, hmap_node, &dns_cache) { +dns_data->delete = true; } const struct sbrec_dns *sbrec_dns; SBREC_DNS_TABLE_FOR_EACH (sbrec_dns, dns_table) { -const char *dns_id = smap_get(&sbrec_dns->external_ids, "dns_id"); -if (!dns_id) { -continue; -} - -struct dns_data *dns_data = shash_find_data(&dns_cache, dns_id); +const struct uuid *uuid = &sbrec_dns->header_.uuid; +dns_data = dns_cache_find_data(uuid); if (!dns_data) { dns_data = xmalloc(sizeof *dns_data); smap_init(&dns_data->records); smap_init(&dns_data->options); -shash_add(&dns_cache, dns_id, dns_data); +hmap_insert(&dns_cache, &dns_data->hmap_node, uuid_hash(uuid)); dns_data->n_dps = 0; dns_data->dps = NULL; } else { @@ -,14 +3343,13 @@ sync_dns_cache(const struct sbrec_dns_table *dns_table) } } -SHASH_FOR_EACH_SAFE (iter, &dns_cache) { -struct dns_data *d = iter->data; -if (d->delete) { -shash_delete(&dns_cache, iter); -smap_destroy(&d->records); -smap_destroy(&d->options); -free(d->dps); -free(d); +HMAP_FOR_EACH_SAFE (dns_data, hmap_node, &dns_cache) { +if (dns_data->delete) { +hmap_remove(&dns_cache, &dns_data->hmap_node); +smap_destroy(&dns_data->records); +smap_destroy(&dns_data->options); +free(dns_data->dps); +free(dns_data); } } } @@ -3348,14 +3357,12 @@ sync_dns_cache(const struct sbrec_dns_table *dns_table) static void destroy_dns_cache(void) { -struct shash_node *iter; -SHASH_FOR_EACH_SAFE (iter, &dns_cache) { -struct dns_data *d = iter->data; -shash_delete(&dns_cache, iter); -smap_destroy(&d->records); -smap_destroy(&d->options); -free(d->dps); -free(d); +struct dns_data *dns_data; +HMAP_FOR_EACH_POP (dns_data, hmap_node, &dns_cache) { +smap_destroy(&dns_data->records); +smap_destroy(&dns_data->options); +free(dns_data->dps); +free(dns_data); } } @@ -3543,17 +3550,16 @@ pinctrl_handle_dns_lookup( uint64_t dp_key = ntohll(pin->flow_metadata.flow.metadata); const char *answer_data = NULL; bool ovn_owned = false; -struct shash_node *iter; -SHASH_FOR_EACH (iter, &dns_cache) { -struct dns_data *d = iter->data; -ovn_owned = smap_get_bool(&d->options, "ovn-owned", false); -for (size_t i = 0; i < d->n_dps; i++) { -if (d->dps[i] == dp_key) { +struct dns_data *dns_data; +HMAP_FOR_EACH (dns_data, hmap_node, &dns_cache) { +ovn_owned = smap_get_bool(&dns_data->options, "ovn-owned", false); +for (size_t i = 0; i < dns_data->n_dps; i++) { +
Re: [ovs-dev] [PATCH ovn 1/5] northd: Use the same UUID for SB representation of Static_Mac_Binding.
Acked-by: Mark Michelson On 10/18/24 06:09, Ales Musil wrote: The NB Static_Mac_Binding has exactly one corresponding row in SB database. Use the NB UUID for the SB representation which makes the processing easier and the link between the rows is obvious at first glance. Signed-off-by: Ales Musil --- lib/automake.mk| 2 -- lib/static-mac-binding-index.c | 43 -- lib/static-mac-binding-index.h | 27 - northd/en-northd.c | 4 northd/inc-proc-northd.c | 6 - northd/northd.c| 34 ++- northd/northd.h| 1 - tests/ovn-northd.at| 2 ++ 8 files changed, 9 insertions(+), 110 deletions(-) delete mode 100644 lib/static-mac-binding-index.c delete mode 100644 lib/static-mac-binding-index.h diff --git a/lib/automake.mk b/lib/automake.mk index b69e854b0..25e516406 100644 --- a/lib/automake.mk +++ b/lib/automake.mk @@ -44,8 +44,6 @@ lib_libovn_la_SOURCES = \ lib/inc-proc-eng.h \ lib/lb.c \ lib/lb.h \ - lib/static-mac-binding-index.c \ - lib/static-mac-binding-index.h \ lib/stopwatch-names.h \ lib/vif-plug-provider.h \ lib/vif-plug-provider.c \ diff --git a/lib/static-mac-binding-index.c b/lib/static-mac-binding-index.c deleted file mode 100644 index fecc9b74f..0 --- a/lib/static-mac-binding-index.c +++ /dev/null @@ -1,43 +0,0 @@ -/* Copyright (c) 2021 - * - * 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 "lib/static-mac-binding-index.h" -#include "lib/ovn-sb-idl.h" - -struct ovsdb_idl_index * -static_mac_binding_index_create(struct ovsdb_idl *idl) -{ -return ovsdb_idl_index_create2(idl, - &sbrec_static_mac_binding_col_logical_port, - &sbrec_static_mac_binding_col_ip); -} - -const struct sbrec_static_mac_binding * -static_mac_binding_lookup(struct ovsdb_idl_index *smb_index, - const char *logical_port, const char *ip) -{ -struct sbrec_static_mac_binding *target = -sbrec_static_mac_binding_index_init_row(smb_index); -sbrec_static_mac_binding_index_set_logical_port(target, logical_port); -sbrec_static_mac_binding_index_set_ip(target, ip); - -struct sbrec_static_mac_binding *smb = -sbrec_static_mac_binding_index_find(smb_index, target); -sbrec_static_mac_binding_index_destroy_row(target); - -return smb; -} diff --git a/lib/static-mac-binding-index.h b/lib/static-mac-binding-index.h deleted file mode 100644 index 3d4ff06a2..0 --- a/lib/static-mac-binding-index.h +++ /dev/null @@ -1,27 +0,0 @@ -/* Copyright (c) 2021 - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at: - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#ifndef OVN_STATIC_MAC_BINDING_INDEX_H -#define OVN_STATIC_MAC_BINDING_INDEX_H 1 - -struct ovsdb_idl; - -struct ovsdb_idl_index *static_mac_binding_index_create(struct ovsdb_idl *); -const struct sbrec_static_mac_binding *static_mac_binding_lookup( -struct ovsdb_idl_index *smb_index, -const char *logical_port, -const char *ip); - -#endif /* lib/static-mac-binding-index.h */ diff --git a/northd/en-northd.c b/northd/en-northd.c index 24ed31517..97e90b832 100644 --- a/northd/en-northd.c +++ b/northd/en-northd.c @@ -57,10 +57,6 @@ northd_get_input_data(struct engine_node *node, engine_ovsdb_node_get_index( engine_get_input("SB_ip_multicast", node), "sbrec_ip_mcast_by_dp"); -input_data->sbrec_static_mac_binding_by_lport_ip = -engine_ovsdb_node_get_index( -engine_get_input("SB_static_mac_binding", node), -"sbrec_static_mac_binding_by_lport_ip"); input_data->sbrec_fdb_by_dp_and_port = engine_ovsdb_node_get_index
Re: [ovs-dev] [PATCH ovn 5/5] northd: Use the same UUID for SB representation of Load_Balancer.
Acked-by: Mark Michelson On 10/18/24 06:09, Ales Musil wrote: The NB Load_Balancer has exactly one corresponding row in SB database. Use the NB UUID for the SB representation which makes the processing easier and the link between the rows is obvious at first glance. Signed-off-by: Ales Musil --- northd/en-sync-sb.c | 80 ++-- northd/en-sync-sb.h | 2 - northd/inc-proc-northd.c | 4 +- tests/ovn-northd.at | 3 +- 4 files changed, 16 insertions(+), 73 deletions(-) diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c index 9bd8a1fc6..d9dc25eb8 100644 --- a/northd/en-sync-sb.c +++ b/northd/en-sync-sb.c @@ -225,7 +225,6 @@ struct sb_lb_record { const struct sbrec_load_balancer *sbrec_lb; struct ovn_dp_group *ls_dpg; struct ovn_dp_group *lr_dpg; -struct uuid sb_uuid; }; struct sb_lb_table { @@ -268,7 +267,6 @@ static bool sync_changed_lbs(struct sb_lb_table *, struct ovn_datapaths *ls_datapaths, struct ovn_datapaths *lr_datapaths, struct chassis_features *); -static bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *); void * en_sync_to_sb_lb_init(struct engine_node *node OVS_UNUSED, @@ -346,23 +344,6 @@ sync_to_sb_lb_northd_handler(struct engine_node *node, void *data_) return true; } -bool -sync_to_sb_lb_sb_load_balancer(struct engine_node *node, void *data OVS_UNUSED) -{ -const struct sbrec_load_balancer_table *sb_load_balancer_table = -EN_OVSDB_GET(engine_get_input("SB_load_balancer", node)); - -/* The only reason to handle SB.Load_Balancer updates is to detect - * spurious records being created in clustered databases due to - * lack of indexing on the SB.Load_Balancer table. All other changes - * are valid and performed by northd, the only write-client for - * this table. */ -if (check_sb_lb_duplicates(sb_load_balancer_table)) { -return false; -} -return true; -} - /* sync_to_sb_pb engine node functions. * This engine node syncs the SB Port Bindings (partly). * en_northd engine create the SB Port binding rows and @@ -690,14 +671,7 @@ sb_lb_table_build_and_sync( const struct sbrec_load_balancer *sbrec_lb; SBREC_LOAD_BALANCER_TABLE_FOR_EACH_SAFE (sbrec_lb, sb_lb_table) { -const char *nb_lb_uuid = smap_get(&sbrec_lb->external_ids, "lb_id"); -struct uuid lb_uuid; -if (!nb_lb_uuid || !uuid_from_string(&lb_uuid, nb_lb_uuid)) { -sbrec_load_balancer_delete(sbrec_lb); -continue; -} - -sb_lb = sb_lb_table_find(&tmp_sb_lbs, &lb_uuid); +sb_lb = sb_lb_table_find(&tmp_sb_lbs, &sbrec_lb->header_.uuid); if (sb_lb) { sb_lb->sbrec_lb = sbrec_lb; bool success = sync_sb_lb_record(sb_lb, sbrec_lb, sb_dpgrp_table, @@ -751,17 +725,9 @@ sync_sb_lb_record(struct sb_lb_record *sb_lb, pre_sync_lr_dpg = sb_lb->lr_dpg; if (!sbrec_lb) { -sb_lb->sb_uuid = uuid_random(); -sbrec_lb = sbrec_load_balancer_insert_persist_uuid(ovnsb_txn, -&sb_lb->sb_uuid); -char *lb_id = xasprintf( -UUID_FMT, UUID_ARGS(&lb_dps->lb->nlb->header_.uuid)); -const struct smap external_ids = -SMAP_CONST1(&external_ids, "lb_id", lb_id); -sbrec_load_balancer_set_external_ids(sbrec_lb, &external_ids); -free(lb_id); +const struct uuid *nb_uuid = &lb_dps->lb->nlb->header_.uuid; +sbrec_lb = sbrec_load_balancer_insert_persist_uuid(ovnsb_txn, nb_uuid); } else { -sb_lb->sb_uuid = sbrec_lb->header_.uuid; sbrec_ls_dp_group = chassis_features->ls_dpg_column ? sbrec_lb->ls_datapath_group @@ -923,13 +889,11 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, HMAPX_FOR_EACH (hmapx_node, &trk_lbs->deleted) { lb_dps = hmapx_node->data; - -sb_lb = sb_lb_table_find(&sb_lbs->entries, - &lb_dps->lb->nlb->header_.uuid); +const struct uuid *nb_uuid = &lb_dps->lb->nlb->header_.uuid; +sb_lb = sb_lb_table_find(&sb_lbs->entries, nb_uuid); if (sb_lb) { const struct sbrec_load_balancer *sbrec_lb = -sbrec_load_balancer_table_get_for_uuid(sb_lb_table, - &sb_lb->sb_uuid); +sbrec_load_balancer_table_get_for_uuid(sb_lb_table, nb_uuid); if (sbrec_lb) { sbrec_load_balancer_delete(sbrec_lb); } @@ -941,9 +905,9 @@ sync_changed_lbs(s
Re: [ovs-dev] [PATCH ovn 2/5] northd: Use the same UUID for SB representation of Chassis_Template_Var.
Acked-by: Mark Michelson On 10/18/24 06:09, Ales Musil wrote: The NB Chassis_Template_Var has exactly one corresponding row in SB database. Use the NB UUID for the SB representation which makes the processing easier and the link between the rows is obvious at first glance. Signed-off-by: Ales Musil --- northd/northd.c | 30 +++--- tests/ovn-northd.at | 2 ++ 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index fb34d0231..0dac661b0 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -18283,37 +18283,37 @@ sync_template_vars( const struct nbrec_chassis_template_var_table *nbrec_ch_template_var_table, const struct sbrec_chassis_template_var_table *sbrec_ch_template_var_table) { -struct shash nb_tvs = SHASH_INITIALIZER(&nb_tvs); - const struct nbrec_chassis_template_var *nb_tv; const struct sbrec_chassis_template_var *sb_tv; -NBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH ( -nb_tv, nbrec_ch_template_var_table) { -shash_add(&nb_tvs, nb_tv->chassis, nb_tv); -} - SBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH_SAFE ( sb_tv, sbrec_ch_template_var_table) { -nb_tv = shash_find_and_delete(&nb_tvs, sb_tv->chassis); +nb_tv = nbrec_chassis_template_var_table_get_for_uuid( +nbrec_ch_template_var_table, &sb_tv->header_.uuid); if (!nb_tv) { sbrec_chassis_template_var_delete(sb_tv); continue; } + if (!smap_equal(&sb_tv->variables, &nb_tv->variables)) { -sbrec_chassis_template_var_set_variables(sb_tv, - &nb_tv->variables); +sbrec_chassis_template_var_set_variables(sb_tv, &nb_tv->variables); } } -struct shash_node *node; -SHASH_FOR_EACH (node, &nb_tvs) { -nb_tv = node->data; -sb_tv = sbrec_chassis_template_var_insert(ovnsb_txn); +NBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH ( +nb_tv, nbrec_ch_template_var_table) { +const struct uuid *nb_uuid = &nb_tv->header_.uuid; +sb_tv = sbrec_chassis_template_var_table_get_for_uuid( +sbrec_ch_template_var_table, nb_uuid); +if (sb_tv) { +continue; +} + +sb_tv = sbrec_chassis_template_var_insert_persist_uuid(ovnsb_txn, + nb_uuid); sbrec_chassis_template_var_set_chassis(sb_tv, nb_tv->chassis); sbrec_chassis_template_var_set_variables(sb_tv, &nb_tv->variables); } -shash_destroy(&nb_tvs); } static void diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index bcee2d231..ab65ddb44 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -10138,7 +10138,9 @@ check ovn-nbctl set Chassis_Template_Var hv2 variables:tv=v2 AS_BOX([Ensure values are propagated to SB]) check ovn-nbctl --wait=sb sync check_column "tv=v1" sb:Chassis_Template_Var variables chassis="hv1" +check_column "$(fetch_column nb:Chassis_Template_Var _uuid chassis=\"hv1\")" sb:Chassis_Template_Var _uuid chassis="hv1" check_column "tv=v2" sb:Chassis_Template_Var variables chassis="hv2" +check_column "$(fetch_column nb:Chassis_Template_Var _uuid chassis=\"hv2\")" sb:Chassis_Template_Var _uuid chassis="hv2" AS_BOX([Ensure SB is reconciled]) check ovn-sbctl --all destroy Chassis_Template_Var ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 4/5] northd: Use the same UUID for SB representation of Mirror.
Acked-by: Mark Michelson On 10/18/24 06:09, Ales Musil wrote: The NB Mirror has exactly one corresponding row in SB database. Use the NB UUID for the SB representation which makes the processing easier and the link between the rows is obvious at first glance. Signed-off-by: Ales Musil --- northd/northd.c | 41 - 1 file changed, 16 insertions(+), 25 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index a81defe49..3b9db5b3c 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -18096,23 +18096,21 @@ mirror_needs_update(const struct nbrec_mirror *nb_mirror, } static void -sync_mirrors_iterate_nb_mirror(struct ovsdb_idl_txn *ovnsb_txn, - const char *mirror_name, - const struct nbrec_mirror *nb_mirror, - struct shash *sb_mirrors) +sync_nb_and_sb_mirror(struct ovsdb_idl_txn *ovnsb_txn, + const char *mirror_name, + const struct nbrec_mirror *nb_mirror, + const struct sbrec_mirror_table *sbrec_mirror_table) { -const struct sbrec_mirror *sb_mirror; -bool new_sb_mirror = false; +const struct uuid *nb_uuid = &nb_mirror->header_.uuid; +const struct sbrec_mirror *sb_mirror = +sbrec_mirror_table_get_for_uuid(sbrec_mirror_table, nb_uuid); -sb_mirror = shash_find_data(sb_mirrors, mirror_name); if (!sb_mirror) { -sb_mirror = sbrec_mirror_insert(ovnsb_txn); +sb_mirror = sbrec_mirror_insert_persist_uuid(ovnsb_txn, nb_uuid); sbrec_mirror_set_name(sb_mirror, mirror_name); -shash_add(sb_mirrors, sb_mirror->name, sb_mirror); -new_sb_mirror = true; } -if (new_sb_mirror || mirror_needs_update(nb_mirror, sb_mirror)) { +if (mirror_needs_update(nb_mirror, sb_mirror)) { sbrec_mirror_set_filter(sb_mirror, nb_mirror->filter); sbrec_mirror_set_index(sb_mirror, nb_mirror->index); sbrec_mirror_set_sink(sb_mirror, nb_mirror->sink); @@ -18125,26 +18123,19 @@ sync_mirrors(struct ovsdb_idl_txn *ovnsb_txn, const struct nbrec_mirror_table *nbrec_mirror_table, const struct sbrec_mirror_table *sbrec_mirror_table) { -struct shash sb_mirrors = SHASH_INITIALIZER(&sb_mirrors); - const struct sbrec_mirror *sb_mirror; -SBREC_MIRROR_TABLE_FOR_EACH (sb_mirror, sbrec_mirror_table) { -shash_add(&sb_mirrors, sb_mirror->name, sb_mirror); +SBREC_MIRROR_TABLE_FOR_EACH_SAFE (sb_mirror, sbrec_mirror_table) { +if (!nbrec_mirror_table_get_for_uuid(nbrec_mirror_table, + &sb_mirror->header_.uuid)) { +sbrec_mirror_delete(sb_mirror); +} } const struct nbrec_mirror *nb_mirror; NBREC_MIRROR_TABLE_FOR_EACH (nb_mirror, nbrec_mirror_table) { -sync_mirrors_iterate_nb_mirror(ovnsb_txn, nb_mirror->name, nb_mirror, - &sb_mirrors); -shash_find_and_delete(&sb_mirrors, nb_mirror->name); -} - -struct shash_node *node, *next; -SHASH_FOR_EACH_SAFE (node, next, &sb_mirrors) { -sbrec_mirror_delete(node->data); -shash_delete(&sb_mirrors, node); +sync_nb_and_sb_mirror(ovnsb_txn, nb_mirror->name, + nb_mirror, sbrec_mirror_table); } -shash_destroy(&sb_mirrors); } /* ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-24.09 1/2] Set release date for 24.09.1.
Signed-off-by: Mark Michelson --- NEWS | 3 ++- debian/changelog | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index 201d5d40f..33531012a 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,6 @@ -OVN v24.09.1 - xx xxx +OVN v24.09.1 - 17 Oct 2024 -- + - Bug fixes OVN v24.09.0 - 13 Sep 2024 -- diff --git a/debian/changelog b/debian/changelog index dc867602c..a50569bb6 100644 --- a/debian/changelog +++ b/debian/changelog @@ -2,7 +2,7 @@ ovn (24.09.1-1) unstable; urgency=low [ OVN team ] * New upstream version - -- OVN team Fri, 13 Sep 2024 19:09:31 -0400 + -- OVN team Thu, 17 Oct 2024 15:12:04 -0400 ovn (24.09.0-1) unstable; urgency=low -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-24.09 0/2] Release patches for v24.09.1.
Mark Michelson (2): Set release date for 24.09.1. Prepare for 24.09.2. NEWS | 6 +- configure.ac | 2 +- debian/changelog | 8 +++- 3 files changed, 13 insertions(+), 3 deletions(-) -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-24.09 2/2] Prepare for 24.09.2.
Signed-off-by: Mark Michelson --- NEWS | 3 +++ configure.ac | 2 +- debian/changelog | 6 ++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 33531012a..f46106349 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,6 @@ +OVN v24.09.2 - xx xxx +-- + OVN v24.09.1 - 17 Oct 2024 -- - Bug fixes diff --git a/configure.ac b/configure.ac index 53c834faa..a6ca69a30 100644 --- a/configure.ac +++ b/configure.ac @@ -13,7 +13,7 @@ # limitations under the License. AC_PREREQ(2.63) -AC_INIT(ovn, 24.09.1, b...@openvswitch.org) +AC_INIT(ovn, 24.09.2, b...@openvswitch.org) AC_CONFIG_MACRO_DIR([m4]) AC_CONFIG_AUX_DIR([build-aux]) AC_CONFIG_HEADERS([config.h]) diff --git a/debian/changelog b/debian/changelog index a50569bb6..e5729aa26 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +ovn (24.09.2-1) unstable; urgency=low + [ OVN team ] + * New upstream version + + -- OVN team Thu, 17 Oct 2024 15:12:04 -0400 + ovn (24.09.1-1) unstable; urgency=low [ OVN team ] * New upstream version -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-24.03 2/2] Prepare for 24.03.5.
Signed-off-by: Mark Michelson --- NEWS | 3 +++ configure.ac | 2 +- debian/changelog | 6 ++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 447727c66..f728b0ef8 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,6 @@ +OVN v24.03.5 - xx xxx +-- + OVN v24.03.4 - 17 Oct 2024 -- - Bug fixes diff --git a/configure.ac b/configure.ac index 6a05ba90b..5a36a0057 100644 --- a/configure.ac +++ b/configure.ac @@ -13,7 +13,7 @@ # limitations under the License. AC_PREREQ(2.63) -AC_INIT(ovn, 24.03.4, b...@openvswitch.org) +AC_INIT(ovn, 24.03.5, b...@openvswitch.org) AC_CONFIG_MACRO_DIR([m4]) AC_CONFIG_AUX_DIR([build-aux]) AC_CONFIG_HEADERS([config.h]) diff --git a/debian/changelog b/debian/changelog index 6476191d6..b2ae35f96 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +ovn (24.03.5-1) unstable; urgency=low + [ OVN team ] + * New upstream version + + -- OVN team Thu, 17 Oct 2024 15:12:02 -0400 + ovn (24.03.4-1) unstable; urgency=low [ OVN team ] * New upstream version -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-23.09 2/2] Prepare for 23.09.7.
Signed-off-by: Mark Michelson --- NEWS | 3 +++ configure.ac | 2 +- debian/changelog | 6 ++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 8b898f58e..282771a87 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,6 @@ +OVN v23.09.7 - xx xxx +-- + OVN v23.09.6 - 17 Oct 2024 -- - Bug fixes diff --git a/configure.ac b/configure.ac index 055d339e3..43411d1c5 100644 --- a/configure.ac +++ b/configure.ac @@ -13,7 +13,7 @@ # limitations under the License. AC_PREREQ(2.63) -AC_INIT(ovn, 23.09.6, b...@openvswitch.org) +AC_INIT(ovn, 23.09.7, b...@openvswitch.org) AC_CONFIG_MACRO_DIR([m4]) AC_CONFIG_AUX_DIR([build-aux]) AC_CONFIG_HEADERS([config.h]) diff --git a/debian/changelog b/debian/changelog index 9cc5d7274..75d939a2f 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +ovn (23.09.7-1) unstable; urgency=low + [ OVN team ] + * New upstream version + + -- OVN team Thu, 17 Oct 2024 15:07:00 -0400 + ovn (23.09.6-1) unstable; urgency=low [ OVN team ] * New upstream version -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-23.09 0/2] Release patches for v23.09.6.
Mark Michelson (2): Set release date for 23.09.6. Prepare for 23.09.7. NEWS | 6 +- configure.ac | 2 +- debian/changelog | 8 +++- 3 files changed, 13 insertions(+), 3 deletions(-) -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-24.03 0/2] Release patches for v24.03.4.
Mark Michelson (2): Set release date for 24.03.4. Prepare for 24.03.5. NEWS | 6 +- configure.ac | 2 +- debian/changelog | 8 +++- 3 files changed, 13 insertions(+), 3 deletions(-) -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-24.03 1/2] Set release date for 24.03.4.
Signed-off-by: Mark Michelson --- NEWS | 3 ++- debian/changelog | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index 0fb8d24cc..447727c66 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,6 @@ -OVN v24.03.4 - xx xxx +OVN v24.03.4 - 17 Oct 2024 -- + - Bug fixes OVN v24.03.3 - 16 Aug 2024 -- diff --git a/debian/changelog b/debian/changelog index 02d897994..6476191d6 100644 --- a/debian/changelog +++ b/debian/changelog @@ -2,7 +2,7 @@ ovn (24.03.4-1) unstable; urgency=low [ OVN team ] * New upstream version - -- OVN team Fri, 16 Aug 2024 13:51:03 -0400 + -- OVN team Thu, 17 Oct 2024 15:12:02 -0400 ovn (24.03.3-1) unstable; urgency=low [ OVN team ] -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-23.09 1/2] Set release date for 23.09.6.
Signed-off-by: Mark Michelson --- NEWS | 3 ++- debian/changelog | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index 9265e0836..8b898f58e 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,6 @@ -OVN v23.09.6 - xx xxx +OVN v23.09.6 - 17 Oct 2024 -- + - Bug fixes OVN v23.09.5 - 16 Aug 2024 -- diff --git a/debian/changelog b/debian/changelog index 244bb95ce..9cc5d7274 100644 --- a/debian/changelog +++ b/debian/changelog @@ -2,7 +2,7 @@ ovn (23.09.6-1) unstable; urgency=low [ OVN team ] * New upstream version - -- OVN team Fri, 16 Aug 2024 13:51:01 -0400 + -- OVN team Thu, 17 Oct 2024 15:07:00 -0400 OVN (23.09.5-1) unstable; urgency=low [ OVN team ] -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] tests: Add a test case for patch ports not created by OVN.
Thanks Numan. Acked-by: Mark Michelson On 9/18/24 16:19, num...@ovn.org wrote: From: Numan Siddique When ovn-controller creates patch ports for ovn-bridge-mappings, it sets external_ids:ovn-localnet-port= in the OVS Port. This indicates that OVN owns and manages these patch ports. It's possible that CMS may create patch ports externally connecting br-int to CMS managed OVS bridges. This patch adds a test case to ensure that ovn-controller doesn't modify or delete these patch ports in anyway. Reported-at: https://issues.redhat.com/browse/FDP-734 Signed-off-by: Numan Siddique --- tests/ovn.at | 114 +++ 1 file changed, 114 insertions(+) diff --git a/tests/ovn.at b/tests/ovn.at index 4b6e8132f0..e6c8421539 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -38926,3 +38926,117 @@ OVN_CHECK_PACKETS([hv/vif1-tx.pcap], [expected-vif1]) AT_CLEANUP ]) + +AT_SETUP([Patch ports not owned by OVN]) + +AT_KEYWORDS([ovn]) +ovn_start + +net_add n1 +sim_add hv1 +ovs-vsctl add-br br-eth0 +ovs-vsctl add-br br-eth1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.20 + +ovs-vsctl set Open_vSwitch . external_ids:ovn-bridge-mappings=physnet1:br-eth0,physnet2:br-eth1 + +check ovn-nbctl ls-add sw0 +check ovn-nbctl lsp-add sw0 sw0p1 + +check ovn-nbctl --wait=hv sync + +check ovn-appctl -t ovn-controller debug/ignore-startup-delay + +# Waits until the OVS database contains exactly the specified patch ports. +# Each argument should be of the form BRIDGE PORT PEER. +check_patch_ports () { +# Generate code to check that the set of patch ports is exactly as +# specified. +echo 'ovs-vsctl -f csv -d bare --no-headings --columns=name find Interface type=patch | sort' > query +for patch +do +echo $patch +done | cut -d' ' -f 2 | sort > expout + +# Generate code to verify that the configuration of each patch +# port is correct. +for patch +do +set $patch; bridge=$1 port=$2 peer=$3 +echo >>query "ovs-vsctl iface-to-br $port -- get Interface $port type options" +echo >>expout "$bridge +patch +{peer=$peer}" +done + +# Run the query until we get the expected result (or until a timeout). +# +# (We use sed to drop all "s from output because ovs-vsctl quotes some +# of the port names but not others.) +AT_CAPTURE_FILE([query]) +AT_CAPTURE_FILE([expout]) +AT_CAPTURE_FILE([stdout]) +OVS_WAIT_UNTIL([. ./query | sed 's/"//g' > stdout #" +diff -u stdout expout >/dev/null]) +} + +# Initially there should be no patch ports. +check_patch_ports + +check ovn-nbctl lsp-add sw0 ln-sw0 +check ovn-nbctl lsp-set-type ln-sw0 localnet +check ovn-nbctl lsp-set-addresses ln-sw0 unknown +check ovn-nbctl --wait=hv lsp-set-options ln-sw0 network_name=physnet1 + +# There should still be no patch ports. +check_patch_ports + +as hv1 +check ovs-vsctl \ +-- add-port br-int vif1 \ +-- set Interface vif1 external_ids:iface-id=sw0p1 + +# ovn-controller should bind the interface. +wait_for_ports_up + +AS_BOX([Checking for patch ports to be created by OVN for ln-sw0]) +check_patch_ports \ +'br-int patch-br-int-to-ln-sw0 patch-ln-sw0-to-br-int' \ +'br-eth0 patch-ln-sw0-to-br-int patch-br-int-to-ln-sw0' + + +check ovs-vsctl add-port br-int patch-br-int-to-br-eth0 \ + -- set interface patch-br-int-to-br-eth0 options:peer=patch-br-eth0-to-br-int \ + -- set interface patch-br-int-to-br-eth0 type=patch \ + -- add-port br-eth0 patch-br-eth0-to-br-int \ + -- set interface patch-br-eth0-to-br-int options:peer=patch-br-int-to-br-eth0 \ + -- set interface patch-br-eth0-to-br-int type=patch \ + +AS_BOX([Checking for patch ports after creating manual patch ports]) + +check_patch_ports \ +'br-int patch-br-int-to-ln-sw0 patch-ln-sw0-to-br-int' \ +'br-eth0 patch-ln-sw0-to-br-int patch-br-int-to-ln-sw0' \ +'br-int patch-br-int-to-br-eth0 patch-br-eth0-to-br-int' \ +'br-eth0 patch-br-eth0-to-br-int patch-br-int-to-br-eth0' + +# Delete the localnet port. ovn-controller should not +# delete the manually created ones. +check ovn-nbctl --wait=hv lsp-del ln-sw0 + +check_patch_ports \ +'br-int patch-br-int-to-br-eth0 patch-br-eth0-to-br-int' \ +'br-eth0 patch-br-eth0-to-br-int patch-br-int-to-br-eth0' + +# Set external_ids:ovn-localnet-port to the manually created patch port. +# ovn-controller should delete the patch ports now as we are setting +# OVN as its owner. +check ovs-vsctl set port patch-br-int-to-br-eth0 external_ids:ovn-localnet-port=foo +ovn-nbctl --wait=hv sync + +AS_BOX([Checking for patch ports after setting external_ids:ovn-localnet-port to the manually created patch port]) +check_patch_ports + +OVN_CLEANUP([hv1]) +AT_CLEANUP ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: Don't monitor most of northbound external IDs.
On 9/20/24 04:22, Ilya Maximets wrote: On 9/20/24 07:26, Han Zhou wrote: On Tue, Sep 17, 2024 at 8:54 AM Ilya Maximets wrote: ovn-northd copies external IDs from Logical Switch, Router and their Port records to corresponding Southbound Datapath and Port Binding records. IDs in other tables are not used by northd in any way, so there is no point in monitoring them. CMSes tend to create a huge amount of external IDs for every record to the point where they can take literally half of the database data. In high scale clusters that can be several hundreds of MB. Not monitoring them saves a lot of time and memory while downloading initial database snapshots on the first connection and should also reduce the ongoing cost while new resources are being created. This will also help avoiding unnecessary re-computes when external IDs are updated without changing any other data. Tested on a 500 MB Northbound DB that contains 1M ACLs created by ovn-kubernetes in a test cluster mimicking a real world setup. Just curious, is the ovn-k8s in the test cluster configured with OVN IC mode or central mode? It is an IC cluster. However, AFAIU, ovn-kubernetes duplicates most of ACLs among AZs, so the database from a central mode setup wouldn't be much different in size. It'll have more ports and some other things, but nearly not enough to compete with the amount of space occupied by ACLs. Before the change it took 20 seconds for the ovsdb-server to send out an initial database snapshot and 19 seconds for ovn-northd to receive it, parse and run a full recompute, consuming 5.4 GB of RAM. With the change it takes 15 seconds on the database side and 11 seconds for the ovn-northd, consuming 2.9 GB of RAM. (Note: the test was performed in a sandbox with no OVN chassis connected, so northd didn't generate a lot of logical flows for those ACLs.) So, we saved: - 25% of CPU time on the database side. - 42% of CPU time on the ovn-northd side. - 2.5 GB (46%) of RAM on ovn-northd. Signed-off-by: Ilya Maximets --- northd/ovn-northd.c | 36 1 file changed, 36 insertions(+) diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index d71114f35..89ef4e870 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -820,6 +820,42 @@ main(int argc, char *argv[]) ovsdb_idl_omit_alert(ovnnb_idl_loop.idl, &nbrec_nb_global_col_hv_cfg_timestamp); +/* Ignore northbound external IDs, except for logical switch, router and + * their ports, for which the external IDs are propagated to corresponding + * southbound datapath and port binding records. */ +const struct ovsdb_idl_column *external_ids[] = { +&nbrec_acl_col_external_ids, +&nbrec_address_set_col_external_ids, +&nbrec_bfd_col_external_ids, +&nbrec_chassis_template_var_col_external_ids, +&nbrec_connection_col_external_ids, +&nbrec_copp_col_external_ids, +&nbrec_dhcp_options_col_external_ids, +&nbrec_dhcp_relay_col_external_ids, +&nbrec_dns_col_external_ids, +&nbrec_forwarding_group_col_external_ids, +&nbrec_gateway_chassis_col_external_ids, +&nbrec_ha_chassis_col_external_ids, +&nbrec_ha_chassis_group_col_external_ids, +&nbrec_load_balancer_col_external_ids, +&nbrec_load_balancer_health_check_col_external_ids, +&nbrec_logical_router_policy_col_external_ids, +&nbrec_logical_router_static_route_col_external_ids, +&nbrec_meter_col_external_ids, +&nbrec_meter_band_col_external_ids, +&nbrec_mirror_col_external_ids, +&nbrec_nat_col_external_ids, +&nbrec_nb_global_col_external_ids, +&nbrec_port_group_col_external_ids, +&nbrec_qos_col_external_ids, +&nbrec_ssl_col_external_ids, +&nbrec_sample_collector_col_external_ids, +&nbrec_sampling_app_col_external_ids, +}; +for (size_t i = 0; i < ARRAY_SIZE(external_ids); i++) { +ovsdb_idl_omit(ovnnb_idl_loop.idl, external_ids[i]); +} + unixctl_command_register("nb-connection-status", "", 0, 0, ovn_conn_show, ovnnb_idl_loop.idl); -- 2.46.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev Thanks Ilya. The result numbers are impressive and the change is simple and straightforward. Acked-by: Han Zhou Thanks! Best regards, Ilya Maximets. Thank you Ilya and Han. I pushed this to main. I didn't push to any other branches since this is a performance improvement. If you think this should be pushed to other branches, let me know. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mail
Re: [ovs-dev] [PATCH ovn] ipam: Do not report error for static assigned IPs.
Aside from some typos in the commit message ("staticatically" and "swith"), this looks good to me. Thanks Lorenzo! Acked-by: Mark Michelson On 9/17/24 06:20, Lorenzo Bianconi wrote: Do not report error in ipam_insert_ip routine for addresses staticatically assigned to ovn logical {swith,router} ports since they have precedence on addressed dynamically assigned by ipam. Reported-at: https://issues.redhat.com/browse/FDP-752 Signed-off-by: Lorenzo Bianconi --- northd/ipam.c | 6 +++--- northd/ipam.h | 2 +- northd/northd.c| 13 +++-- northd/test-ipam.c | 2 +- 4 files changed, 12 insertions(+), 11 deletions(-) diff --git a/northd/ipam.c b/northd/ipam.c index 4448a7607..04fa357a5 100644 --- a/northd/ipam.c +++ b/northd/ipam.c @@ -48,7 +48,7 @@ destroy_ipam_info(struct ipam_info *info) } bool -ipam_insert_ip(struct ipam_info *info, uint32_t ip) +ipam_insert_ip(struct ipam_info *info, uint32_t ip, bool dynamic) { if (!info->allocated_ipv4s) { return true; @@ -56,8 +56,8 @@ ipam_insert_ip(struct ipam_info *info, uint32_t ip) if (ip >= info->start_ipv4 && ip < (info->start_ipv4 + info->total_ipv4s)) { -if (bitmap_is_set(info->allocated_ipv4s, - ip - info->start_ipv4)) { +if (dynamic && bitmap_is_set(info->allocated_ipv4s, + ip - info->start_ipv4)) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); VLOG_WARN_RL(&rl, "%s: Duplicate IP set: " IP_FMT, info->id, IP_ARGS(htonl(ip))); diff --git a/northd/ipam.h b/northd/ipam.h index 447412769..6240f9ab7 100644 --- a/northd/ipam.h +++ b/northd/ipam.h @@ -22,7 +22,7 @@ void init_ipam_info(struct ipam_info *info, const struct smap *config, void destroy_ipam_info(struct ipam_info *info); -bool ipam_insert_ip(struct ipam_info *info, uint32_t ip); +bool ipam_insert_ip(struct ipam_info *info, uint32_t ip, bool dynamic); uint32_t ipam_get_unused_ip(struct ipam_info *info); diff --git a/northd/northd.c b/northd/northd.c index a267cd5f8..95c3f0e07 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -1464,13 +1464,13 @@ ovn_port_get_peer(const struct hmap *lr_ports, struct ovn_port *op) } static void -ipam_insert_ip_for_datapath(struct ovn_datapath *od, uint32_t ip) +ipam_insert_ip_for_datapath(struct ovn_datapath *od, uint32_t ip, bool dynamic) { if (!od) { return; } -ipam_insert_ip(&od->ipam_info, ip); +ipam_insert_ip(&od->ipam_info, ip, dynamic); } static void @@ -1487,7 +1487,7 @@ ipam_insert_lsp_addresses(struct ovn_datapath *od, for (size_t j = 0; j < laddrs->n_ipv4_addrs; j++) { uint32_t ip = ntohl(laddrs->ipv4_addrs[j].addr); -ipam_insert_ip_for_datapath(od, ip); +ipam_insert_ip_for_datapath(od, ip, false); } } @@ -1519,7 +1519,7 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op) * about a duplicate IP address. */ if (ip != op->peer->od->ipam_info.start_ipv4) { -ipam_insert_ip_for_datapath(op->peer->od, ip); +ipam_insert_ip_for_datapath(op->peer->od, ip, false); } } } @@ -1744,7 +1744,8 @@ update_unchanged_dynamic_addresses(struct dynamic_address_update *update) } if (update->ipv4 == NONE && update->current_addresses.n_ipv4_addrs) { ipam_insert_ip_for_datapath(update->op->od, - ntohl(update->current_addresses.ipv4_addrs[0].addr)); + ntohl(update->current_addresses.ipv4_addrs[0].addr), + true); } } @@ -1872,7 +1873,7 @@ update_dynamic_addresses(struct dynamic_address_update *update) ipam_insert_mac(&mac, true); if (ip4) { -ipam_insert_ip_for_datapath(update->od, ntohl(ip4)); +ipam_insert_ip_for_datapath(update->od, ntohl(ip4), true); ds_put_format(&new_addr, " "IP_FMT, IP_ARGS(ip4)); } if (!IN6_ARE_ADDR_EQUAL(&ip6, &in6addr_any)) { diff --git a/northd/test-ipam.c b/northd/test-ipam.c index c449599e3..3b6ad1891 100644 --- a/northd/test-ipam.c +++ b/northd/test-ipam.c @@ -44,7 +44,7 @@ test_ipam_get_unused_ip(struct ovs_cmdl_context *ctx) uint32_t next_ip = ipam_get_unused_ip(&info); ds_put_format(&output, IP_FMT "\n", IP_ARGS(htonl(next_ip))); if (next_ip) { -ovs_assert(ipam_insert_ip(&info, next_ip)); +ovs_assert(ipam_insert_ip(&info, next_ip, true)); } } ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovn-ctl: Add run_sb_relay_ovsdb subcommand.
On 10/16/24 15:45, Mark Michelson wrote: On 10/10/24 12:22, Aaron Conole wrote: Michał Nasiadka writes: It was just my first patch and didn’t know what I should do with those robot posted failures (basically missing signed-off-by). What should I do now to get that moved forward please? It would be best to repost (v2) with the correct signed-off-by line, I think. Replying with a new signoff will have patchwork include both. But an OVN maintainer may have a different opinion. Aaron is correct here. To send a new version, send a new email with "v2" in the subject line: [PATCH ovn v2] ovn-ctl: Add run_sb_relay_ovsdb_subcommand That's the typical way to get a new version of a patch posted, whether due to review comments or because the bot spots something that breaks guidelines. Now, having said that, this is a small patch, so I think we can skip all that this time. Acked-by: Mark Michelson I'll make sure the sign-off is correct and get this merged. Thanks for contributing! I pushed this to main. Thanks again! Best regards, Michal On 27 Sep 2024, at 21:49, Aaron Conole wrote: Michał Nasiadka writes: This patch adds missing subcommand for running SB Relay in foreground. Signed-off-by: Michał Nasiadka --- Seems these were done as top-post replied to the robot. Was there any reason why? utilities/ovn-ctl | 8 1 file changed, 8 insertions(+) diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl index d7ca9e758..c0b797273 100755 --- a/utilities/ovn-ctl +++ b/utilities/ovn-ctl @@ -499,6 +499,11 @@ run_sb_ovsdb() { start_sb_ovsdb } +run_sb_relay_ovsdb() { + DB_SB_RELAY_DETACH=no + start_sb_relay_ovsdb +} + run_ic_nb_ovsdb() { DB_IC_NB_DETACH=no start_ic_nb_ovsdb @@ -1469,6 +1474,9 @@ case $command in run_sb_ovsdb) run_sb_ovsdb ;; + run_sb_relay_ovsdb) + run_sb_relay_ovsdb + ;; run_ic_nb_ovsdb) run_ic_nb_ovsdb ;; -- 2.34.1 On 11 Sep 2024, at 14:51, Michał Nasiadka wrote: This patch adds missing subcommand for running SB Relay in foreground. Signed-off-by: Michał Nasiadka --- utilities/ovn-ctl | 8 1 file changed, 8 insertions(+) diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl index d7ca9e758..c0b797273 100755 --- a/utilities/ovn-ctl +++ b/utilities/ovn-ctl @@ -499,6 +499,11 @@ run_sb_ovsdb() { start_sb_ovsdb } +run_sb_relay_ovsdb() { + DB_SB_RELAY_DETACH=no + start_sb_relay_ovsdb +} + run_ic_nb_ovsdb() { DB_IC_NB_DETACH=no start_ic_nb_ovsdb @@ -1469,6 +1474,9 @@ case $command in run_sb_ovsdb) run_sb_ovsdb ;; + run_sb_relay_ovsdb) + run_sb_relay_ovsdb + ;; run_ic_nb_ovsdb) run_ic_nb_ovsdb ;; -- 2.34.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovn-ctl: Add run_sb_relay_ovsdb subcommand.
On 10/10/24 12:22, Aaron Conole wrote: Michał Nasiadka writes: It was just my first patch and didn’t know what I should do with those robot posted failures (basically missing signed-off-by). What should I do now to get that moved forward please? It would be best to repost (v2) with the correct signed-off-by line, I think. Replying with a new signoff will have patchwork include both. But an OVN maintainer may have a different opinion. Aaron is correct here. To send a new version, send a new email with "v2" in the subject line: [PATCH ovn v2] ovn-ctl: Add run_sb_relay_ovsdb_subcommand That's the typical way to get a new version of a patch posted, whether due to review comments or because the bot spots something that breaks guidelines. Now, having said that, this is a small patch, so I think we can skip all that this time. Acked-by: Mark Michelson I'll make sure the sign-off is correct and get this merged. Thanks for contributing! Best regards, Michal On 27 Sep 2024, at 21:49, Aaron Conole wrote: Michał Nasiadka writes: This patch adds missing subcommand for running SB Relay in foreground. Signed-off-by: Michał Nasiadka --- Seems these were done as top-post replied to the robot. Was there any reason why? utilities/ovn-ctl | 8 1 file changed, 8 insertions(+) diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl index d7ca9e758..c0b797273 100755 --- a/utilities/ovn-ctl +++ b/utilities/ovn-ctl @@ -499,6 +499,11 @@ run_sb_ovsdb() { start_sb_ovsdb } +run_sb_relay_ovsdb() { +DB_SB_RELAY_DETACH=no +start_sb_relay_ovsdb +} + run_ic_nb_ovsdb() { DB_IC_NB_DETACH=no start_ic_nb_ovsdb @@ -1469,6 +1474,9 @@ case $command in run_sb_ovsdb) run_sb_ovsdb ;; +run_sb_relay_ovsdb) +run_sb_relay_ovsdb +;; run_ic_nb_ovsdb) run_ic_nb_ovsdb ;; -- 2.34.1 On 11 Sep 2024, at 14:51, Michał Nasiadka wrote: This patch adds missing subcommand for running SB Relay in foreground. Signed-off-by: Michał Nasiadka --- utilities/ovn-ctl | 8 1 file changed, 8 insertions(+) diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl index d7ca9e758..c0b797273 100755 --- a/utilities/ovn-ctl +++ b/utilities/ovn-ctl @@ -499,6 +499,11 @@ run_sb_ovsdb() { start_sb_ovsdb } +run_sb_relay_ovsdb() { +DB_SB_RELAY_DETACH=no +start_sb_relay_ovsdb +} + run_ic_nb_ovsdb() { DB_IC_NB_DETACH=no start_ic_nb_ovsdb @@ -1469,6 +1474,9 @@ case $command in run_sb_ovsdb) run_sb_ovsdb ;; +run_sb_relay_ovsdb) +run_sb_relay_ovsdb +;; run_ic_nb_ovsdb) run_ic_nb_ovsdb ;; -- 2.34.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC] Service Insertion support in OVN
out how my proposal compares to yours? Is there something that my proposal misses that yours covers? Do you think there are some ideas that we could merge from both proposals, perhaps? Thanks, Mark Michelson [1] I originally wrote this document in a Red Hat-owned instance of Google Docs. This is a copy made on a public instance of Google Docs. On 10/14/24 18:31, Sragdhara Datta Chaudhuri wrote: This is a proposal to add service insertion support in OVN. (The terms “Service” and “Network Function” have been used interchangeably here) 1. Introduction The objective is to insert a service in the path of outbound/inbound traffic from/to a port-group (set of logical switch ports). Here are some of the highlights (these are described in detail later): - A new entity network-function (NF) would be introduced. It contains a pair of LSPs. The CMS would designate one as inport and the other as outport. - Insertion of a service in the traffic path requires that the request packet (client to server) be redirected to the inport and once the same packet comes out of the outport, forward it to the destination - The service VM should not modify the packet, i.e. the IP header, the source and destination MAC addresses and VLAN tag, remain unchanged. The service VM will be like a bump in the wire. - Statefulness is to be maintained, i.e. the response traffic also needs to go through the same pair of LSPs but in reverse, i.e. it will enter through outport and come out from inport port. - For future load balancing support across multiple NFs, a new entity network-function-group (NFG) will be added as well. It contains a list of NFs. - The access-list would accept a NFG as parameter and traffic matching the ACL would be redirected to the associated port. This parameter is accepted for stateful allow action only. - Service insertion would be supported for both from-lport and to-lport ACLs. New stage would be introduced in the logical switch ingress and egress pipeline for this. - For the service port we need to disable port security check, fdb learning and multicast/broadcast forwarding. There is already support for the first one. For the remaining two, new boolean options would be added. - A new entity network_function_health_check will be introduced. This would store the configuration parameters related to health monitoring. A packet passthrough type of health check would be supported where ovn-controller would periodically inject probe packets into the inport and monitor packets coming out of the outport. - If the traffic redirection involves cross-host traffic (e.g. source VM and service VM are on different hosts), packets would be tunneled to and from the service VM's host. This needs change in the tables maintained by ovn-controller locally on each host. - If the port-group to which the ACL is being applied has members spread across multiple LSs, CMS needs to create network function child ports on each of these LSs. The redirection rules in each LS will use the child ports on that LS. 2. NB tables = New NB tables — Network_Function: Each row contains {inport, outport, health_check} Network_Function_Group: Each row contains a list of Network_Function entities. Min and max length of this list is 1. It also contains an id (unique value between 1 and 255). Network_Function_Health_Check: Each row contains configuration for probes in options field: {interval, timeout, success_count, failure_count} "Network_Function_Health_Check": { "columns": { "options": { "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}, "external_ids": { "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}}, "isRoot": false}, "Network_Function": { "columns": { "name": {"type": "string"}, "outport": {"type": {"key": {"type": "uuid", "refTable": "Logical_Switch_Port", "refType": "strong"}, "min": 1, "max": 1}}, "inport": {"type": {"key": {"type": "uuid", "refTable": "Logical_Switch_Port",
Re: [ovs-dev] [PATCH ovn v2] northd, controller: Use ct_next to get the CT state for direct SNAT.
On 9/30/24 15:13, Mark Michelson wrote: Thanks Ales, Acked-by: Mark Michelson I have one note below, but it doesn't affect the patch itself. The patch should be committed as-is. I pushed this change to main and all branches back to 24.03. The conflicts in branch-24.03 were easy to fix up. The conflicts on branch-23.09 are not. The cited issue in the commit message explicitly mentions that they do not see the problem on branch-23.09 anyway. On 8/29/24 01:20, Ales Musil wrote: On Wed, Aug 28, 2024 at 11:28 PM Brian Haley wrote: Hi Ales, Hello Brian, I was able to test this along with a neutron patch [0] to program '0.0.0.0/0' as the snat logical_ip address and it seemed to work with various combinations of fixed and floating IP addresses. You can add this if you like: Tested-by: Brian Haley Thanks for fixing this! Thank you for testing it out. Is there any guess on how far back this could be applied? I'm guessing it would need Martin's other snat fix? So the original commit is applied to 24.03 onwards, so I will make sure it gets backported there. AFAIK 23.09 doesn't have this issue, not sure if anyone tested 23.06 and 23.03. -Brian [0] https://review.opendev.org/c/openstack/neutron/+/926495 On 8/27/24 4:52 AM, Ales Musil wrote: In order to get the direct SNAT access working we need to commit new connections so the reply is not marked as invalid. The CT state to determine if the connection should be committed was populated by ct_snat action, however this action also performs the NAT part if the connection is already known and there is an entry for that. This would cause issues when the there is combination of FIPs and SNAT with very broad logical IP. To prevent that use ct_next only which will populate the state but won't do the NAT part which can be done later on if needed according to the conditions. At the same time add support for ct_next in SNAT zone as ct_next was assuming that the zone is always DNAT. Fixes: 40136a2f2c84 ("northd: Fix direct access to SNAT network.") Reported-at: https://issues.redhat.com/browse/FDP-744 Signed-off-by: Ales Musil --- v2: Make sure we don't SNAT FIP reply traffic --- controller/chassis.c | 8 include/ovn/actions.h | 1 + include/ovn/features.h | 1 + lib/actions.c | 33 + northd/en-global-config.c | 10 ++ northd/en-global-config.h | 1 + northd/northd.c | 8 +++- ovn-sb.xml | 2 ++ tests/ovn-northd.at | 23 ++- tests/ovn.at | 16 tests/system-ovn.at | 10 -- 11 files changed, 89 insertions(+), 24 deletions(-) diff --git a/controller/chassis.c b/controller/chassis.c index 2991a0af3..ee839084a 100644 --- a/controller/chassis.c +++ b/controller/chassis.c @@ -390,6 +390,7 @@ chassis_build_other_config(const struct ovs_chassis_cfg *ovs_cfg, smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true"); smap_replace(config, OVN_FEATURE_SAMPLE_WITH_REGISTERS, ovs_cfg->sample_with_regs ? "true" : "false"); + smap_replace(config, OVN_FEATURE_CT_NEXT_ZONE, "true"); } /* @@ -549,6 +550,12 @@ chassis_other_config_changed(const struct ovs_chassis_cfg *ovs_cfg, return true; } + if (!smap_get_bool(&chassis_rec->other_config, + OVN_FEATURE_CT_NEXT_ZONE, + false)) { + return true; + } + return false; } @@ -706,6 +713,7 @@ update_supported_sset(struct sset *supported) sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2); sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE); sset_add(supported, OVN_FEATURE_SAMPLE_WITH_REGISTERS); + sset_add(supported, OVN_FEATURE_CT_NEXT_ZONE); } static void diff --git a/include/ovn/actions.h b/include/ovn/actions.h index c8dd66ed8..a95a0daf7 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -260,6 +260,7 @@ struct ovnact_push_pop { /* OVNACT_CT_NEXT. */ struct ovnact_ct_next { struct ovnact ovnact; + bool dnat_zone; uint8_t ltable; /* Logical table ID of next table. */ }; diff --git a/include/ovn/features.h b/include/ovn/features.h index 4275f7526..3566ab60f 100644 --- a/include/ovn/features.h +++ b/include/ovn/features.h @@ -30,6 +30,7 @@ #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2" #define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone" #define OVN_FEATURE_SAMPLE_WITH_REGISTERS "ovn-sample-with-registers" +#define OVN_FEATURE_CT_NEXT_ZONE "ct-next-zone" /* OVS datapath supported features. Based on availability OVN might generate * different types of openflows. diff --git a/lib/actions.c b/lib/action
Re: [ovs-dev] [PATCH ovn] ci: Pin scapy version.
Thanks Ales. Acked-by: Mark Michelson Since this is causing CI failures, and the patch is so simple I pushed the change to all branches that have scapy in a py-requirements.txt file. That means I pushed the change to branch-23.03 up to main. On 10/1/24 08:31, Ales Musil wrote: The latest scapy version (2.6.0) fails to properly evaulate if .config directory already exists [0]. Pin the version to 2.5.0 for the time being as that one was working without any issues. [0] https://github.com/secdev/scapy/issues/4546 Signed-off-by: Ales Musil --- utilities/containers/py-requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utilities/containers/py-requirements.txt b/utilities/containers/py-requirements.txt index 8a3e977aa..1b55042c8 100644 --- a/utilities/containers/py-requirements.txt +++ b/utilities/containers/py-requirements.txt @@ -1,6 +1,6 @@ flake8>=6.1.0 meson>=1.4,<1.5 -scapy +scapy==2.5.0 sphinx<8.0 # https://github.com/sphinx-doc/sphinx/issues/12711 setuptools pyelftools ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] northd, controller: Use ct_next to get the CT state for direct SNAT.
Thanks Ales, Acked-by: Mark Michelson I have one note below, but it doesn't affect the patch itself. The patch should be committed as-is. On 8/29/24 01:20, Ales Musil wrote: On Wed, Aug 28, 2024 at 11:28 PM Brian Haley wrote: Hi Ales, Hello Brian, I was able to test this along with a neutron patch [0] to program '0.0.0.0/0' as the snat logical_ip address and it seemed to work with various combinations of fixed and floating IP addresses. You can add this if you like: Tested-by: Brian Haley Thanks for fixing this! Thank you for testing it out. Is there any guess on how far back this could be applied? I'm guessing it would need Martin's other snat fix? So the original commit is applied to 24.03 onwards, so I will make sure it gets backported there. AFAIK 23.09 doesn't have this issue, not sure if anyone tested 23.06 and 23.03. -Brian [0] https://review.opendev.org/c/openstack/neutron/+/926495 On 8/27/24 4:52 AM, Ales Musil wrote: In order to get the direct SNAT access working we need to commit new connections so the reply is not marked as invalid. The CT state to determine if the connection should be committed was populated by ct_snat action, however this action also performs the NAT part if the connection is already known and there is an entry for that. This would cause issues when the there is combination of FIPs and SNAT with very broad logical IP. To prevent that use ct_next only which will populate the state but won't do the NAT part which can be done later on if needed according to the conditions. At the same time add support for ct_next in SNAT zone as ct_next was assuming that the zone is always DNAT. Fixes: 40136a2f2c84 ("northd: Fix direct access to SNAT network.") Reported-at: https://issues.redhat.com/browse/FDP-744 Signed-off-by: Ales Musil --- v2: Make sure we don't SNAT FIP reply traffic --- controller/chassis.c | 8 include/ovn/actions.h | 1 + include/ovn/features.h| 1 + lib/actions.c | 33 + northd/en-global-config.c | 10 ++ northd/en-global-config.h | 1 + northd/northd.c | 8 +++- ovn-sb.xml| 2 ++ tests/ovn-northd.at | 23 ++- tests/ovn.at | 16 tests/system-ovn.at | 10 -- 11 files changed, 89 insertions(+), 24 deletions(-) diff --git a/controller/chassis.c b/controller/chassis.c index 2991a0af3..ee839084a 100644 --- a/controller/chassis.c +++ b/controller/chassis.c @@ -390,6 +390,7 @@ chassis_build_other_config(const struct ovs_chassis_cfg *ovs_cfg, smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true"); smap_replace(config, OVN_FEATURE_SAMPLE_WITH_REGISTERS, ovs_cfg->sample_with_regs ? "true" : "false"); +smap_replace(config, OVN_FEATURE_CT_NEXT_ZONE, "true"); } /* @@ -549,6 +550,12 @@ chassis_other_config_changed(const struct ovs_chassis_cfg *ovs_cfg, return true; } +if (!smap_get_bool(&chassis_rec->other_config, + OVN_FEATURE_CT_NEXT_ZONE, + false)) { +return true; +} + return false; } @@ -706,6 +713,7 @@ update_supported_sset(struct sset *supported) sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2); sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE); sset_add(supported, OVN_FEATURE_SAMPLE_WITH_REGISTERS); +sset_add(supported, OVN_FEATURE_CT_NEXT_ZONE); } static void diff --git a/include/ovn/actions.h b/include/ovn/actions.h index c8dd66ed8..a95a0daf7 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -260,6 +260,7 @@ struct ovnact_push_pop { /* OVNACT_CT_NEXT. */ struct ovnact_ct_next { struct ovnact ovnact; +bool dnat_zone; uint8_t ltable;/* Logical table ID of next table. */ }; diff --git a/include/ovn/features.h b/include/ovn/features.h index 4275f7526..3566ab60f 100644 --- a/include/ovn/features.h +++ b/include/ovn/features.h @@ -30,6 +30,7 @@ #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2" #define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone" #define OVN_FEATURE_SAMPLE_WITH_REGISTERS "ovn-sample-with-registers" +#define OVN_FEATURE_CT_NEXT_ZONE "ct-next-zone" /* OVS datapath supported features. Based on availability OVN might generate * different types of openflows. diff --git a/lib/actions.c b/lib/actions.c index c12d087e7..2e05d4134 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -701,13 +701,32 @@ parse_CT_NEXT(struct action_context *ctx) } add_prerequisite(ctx, "ip"); -ovnact_put_CT_NEXT(ctx->ovnacts)->ltable = ctx->pp->cur_ltable + 1; +struct ovnact_ct_next *ct_next =
Re: [ovs-dev] [PATCH ovn v2] Documentation: Define experimental features.
On 9/19/24 11:25, Numan Siddique wrote: On Wed, Aug 14, 2024 at 5:20 AM Martin Kalcok wrote: Hi Mark, this definition looks good to me. Thanks for the clarification on how to document this. Martin. On 13 Aug 2024, at 22:33, Mark Michelson wrote: During a recent OVN community call, it was questioned what it means for a feature to be marked experimental. This documentation change aims to clarify what it means when a feature is marked experimental. Signed-off-by: Mark Michelson --- v1 -> v2: * Added follow-up question that answers how features are marked xperimental. --- Documentation/faq/general.rst | 54 +++ 1 file changed, 54 insertions(+) diff --git a/Documentation/faq/general.rst b/Documentation/faq/general.rst index 831ca0445..df4952ef5 100644 --- a/Documentation/faq/general.rst +++ b/Documentation/faq/general.rst @@ -119,3 +119,57 @@ Q: How can I contribute to the OVN Community? questions. You can also suggest improvements to documentation. If you have a feature or bug you would like to work on, send a mail to one of the :doc:`mailing lists `. + +Q: What does it mean when a feature is marked "experimental"? + +A: Experimental features are marked this way because of one of +several reasons: + +* The developer was only able to test the feature in a limited + environment. Therefore the feature may not always work as intended + in all environments. + +* During review, the potential for failure was noticed, but the + circumstances that would lead to that failure were hard to nail + down or were strictly theoretical. + +* What exists in OVN may be an early version of a more fleshed-out + feature to come in a later version. + +* The feature was developed against a draft RFC that is subject to + change when the RFC is published. + +* The feature was developed based on observations of how a specific + vendor implements a feature, rather than using IETF standards or + other documentated specifications. + +A feature may be declared experimental for other reasons as well, +but the above are the most common. When a feature is marked +experimental, it has the following properties: + +* The feature must be opt-in. The feature must be disabled by + default. When the feature is disabled, it must have no bearing + on other OVN functionality. + +* Configuration and implementation details of the feature are + subject to change between major or minor versions of OVN. + +* Users make use of this feature at their own risk. Users are free + to file issues against the feature, but developers are more likely + to prioritize work on non-experimental features first. + +* Experimental features may be removed. For instance, if an + experimental feature exposes a security risk, it may be removed + rather than repaired. + +The hope is that experimental features will eventually lose the +"experimental" marker and become a core feature. However, there is +no specific test or process defined for when a feature no longer +needs to be considered experimental. This typically will be decided +collectively by OVN maintainers. + +Q: How is a feature marked "experimental"? + +A: Experimental features must contain the following note in their man +pages (ovn-nb.5, ovn-sb.5, ovn-controller.8, etc): "NOTE: this feature +is experimental and may be subject to removal/change in the future.: Acked-by: Numan Siddique Numan I merged this to main. -- 2.45.2 ___ 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 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 6/8] tests: Wait for controller exit before restart.
On 7/26/24 05:59, Xavier Simonart wrote: Hi Mark Thanks for the review. Sorry, I completely lost track of this one. On Thu, Jun 6, 2024 at 10:59 PM Mark Michelson <mailto:mmich...@redhat.com>> wrote: Hi Xavier, This is the only patch in the series for which I have a recommendation. The rest all look good to me. On 6/4/24 09:10, Xavier Simonart wrote: > In some rare cases, and despite "recent" changes to wait for cleanup > before replying to exit, ovn-controller was still running when trying > to restart it. > > Signed-off-by: Xavier Simonart mailto:xsimo...@redhat.com>> > --- > tests/ovn-macros.at <http://ovn-macros.at> | 9 + > tests/ovn.at <http://ovn.at> | 17 ++--- > 2 files changed, 15 insertions(+), 11 deletions(-) > > diff --git a/tests/ovn-macros.at <http://ovn-macros.at> b/tests/ovn-macros.at <http://ovn-macros.at> > index 47ada5c70..71a46db8b 100644 > --- a/tests/ovn-macros.at <http://ovn-macros.at> > +++ b/tests/ovn-macros.at <http://ovn-macros.at> > @@ -1107,6 +1107,15 @@ m4_define([OVN_CHECK_SCAPY_EDNS_CLIENT_SUBNET_SUPPORT], > AT_SKIP_IF([! echo "from scapy.layers.dns import EDNS0ClientSubnet" | python 2>&1 > /dev/null]) > ]) > > +m4_define([OVN_CONTROLLER_EXIT_RESTART], I think this should just be called "OVN_CONTROLLER_RESTART". The goal is to restart ovn-controller, so I think the simpler name makes sense. It's true that we are using an "exit" command for ovn-appctl, but that is more of an implementation detail than anything else. I agree that the name is confusing, but I am not sure that OVN_CONTROLLER_RESTART is better. The macro is executing "ovn-appctl -t ovn-controller exit --restart" (i.e. exit the controller w/o cleaning up databases) , and then wait for controller exit. After the macro is executed, the controller is stopped (exited). ovn-controller restart is executed later, with a different command. OVN_CONTROLLER_RESTART would let me think that the macro is restarting ovn-controller. What about OVN_CONTROLLER_EXIT ? WDYT? That's fine by me. > + [TMPPID=$(cat $1/ovn-controller.pid) > + AT_CHECK([as $1 ovn-appctl -t ovn-controller exit --restart]) > + # Make sure ovn-controller stopped so that a future restart will not fail. > + # Checking debug/status is running is not enough as there might be a small race condition. > + echo "Waiting for pid $TMPPID" > + OVS_WAIT_WHILE([kill -0 $TMPPID 2>/dev/null]) > +]) > + > m4_define([OFTABLE_PHY_TO_LOG], [0]) > m4_define([OFTABLE_LOG_INGRESS_PIPELINE], [8]) > m4_define([OFTABLE_OUTPUT_LARGE_PKT_DETECT], [37]) > diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at <http://ovn.at> > index 2dd0dfd2e..8fa26c192 100644 > --- a/tests/ovn.at <http://ovn.at> > +++ b/tests/ovn.at <http://ovn.at> > @@ -20615,7 +20615,7 @@ echo $expected | ovstest test-ovn expr-to-packets > expected > OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected]) > > # Stop ovn-controller on hv2 with --restart flag > -as hv2 ovs-appctl -t ovn-controller exit --restart > +OVN_CONTROLLER_EXIT_RESTART([hv2]) > > # Now send the packet again. This time, it should still arrive > OVS_WAIT_UNTIL([as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"]) > @@ -29316,7 +29316,7 @@ check test "$hvt2" -gt 0 > # Kill ovn-controller on chassis hv3, so that it won't update nb_cfg. > # Then wait for 9 out of 10 > sleep 1 > -check as hv3 ovn-appctl -t ovn-controller exit --restart > +OVN_CONTROLLER_EXIT_RESTART([hv3]) > wait_for_ports_up > ovn-nbctl --wait=sb sync > wait_row_count Chassis_Private 9 name!=hv3 nb_cfg=2 > @@ -36252,7 +36252,7 @@ check_tunnel_port hv1 br-int hv2@192.168.0.2 <mailto:hv2@192.168.0.2>%192.168.0.1 > check_tunnel_port hv2 br-int hv1@192.168.0.1 <mailto:hv1@192.168.0.1>%192.168.0.2 > > # Stop ovn-controller on hv1 > -check as hv1 ovn-appctl -t ovn-controller exit --restart > +OVN_CONTROLLER_EXIT_RESTART([hv1]) > > # The tunnel should remain intact > check_tunnel_port hv1 br-int hv2@192.168.0.2 <mailto:hv2@192.168.0.2>%192.168.0.1 > @@ -36281,7 +36281,7 @@ check_tunnel_port hv2 br-int1 hv1@192.168.0.1 <mailto:hv1@192.168.0.1>%192.168.0.2 > check grep -q "Cl
[ovs-dev] [PATCH ovn] northd: Track max ACL tiers more accurately.
When ACL tiers were introduced, the code kept track of the highest ACL tier so that when iterating through ACL tiers, we would not attempt to advance the current tier past the highest configured tier. Unfortunately, keeping track of a single max ACL tier doesn't work when ACLs are evaluated in three separate places. ACLs can be evaluated on ingress before load balancing, on ingress after load balancing, and on egress. By only keeping track of a single max ACL tier, it means that we could perform superfluous checks if one stage of ACLs has a higher max tier than other stages. As an example, if ingress pre-load balancing ACLs have a maximum tier of 1, and egress ACLs have a maximum tier of 2, then it means that for all stages of ACLs, we will evaluate tiers 0, 1, and 2 of ACLs, even though only one stage of ACLs uses tier 2. >From a pure functionality standpoint, this doesn't cause any issues. Even if we advance the tier past the highest configured value, it results in a no-op and the same net result happens. However, the addition of sampling into ACLs has caused an unwanted side effect. In the example scenario above, let's say the tier 1 ACL in the ingress pre-load balancing stage evaluates to "pass". After the evaluation, we send a sample for the "pass" result. We then advance the tier to 2, then move back to ACL evaluation. There are no tier 2 ACLs, so we move on to the sampling stage again. We then send a second sample for the previous "pass" result from tier 1. The result is confusing since we've sent two samples for the same ACL evaluation. To remedy this, we now track the max ACL tier in each of the stages where ACLs are evaluated. Now there are no superfluous ACL evaluations and no superfluous samples sent either. Reported-at: https://issues.redhat.com/browse/FDP-760 Signed-off-by: Mark Michelson --- northd/en-ls-stateful.c | 48 ++--- northd/en-ls-stateful.h | 8 ++- northd/northd.c | 47 ++-- tests/ovn-northd.at | 34 + 4 files changed, 117 insertions(+), 20 deletions(-) diff --git a/northd/en-ls-stateful.c b/northd/en-ls-stateful.c index 44f74ea08..eb751d8dd 100644 --- a/northd/en-ls-stateful.c +++ b/northd/en-ls-stateful.c @@ -204,16 +204,18 @@ ls_stateful_port_group_handler(struct engine_node *node, void *data_) ovn_datapaths_find_by_index(input_data.ls_datapaths, ls_stateful_rec->ls_index); bool had_stateful_acl = ls_stateful_rec->has_stateful_acl; -uint64_t max_acl_tier = ls_stateful_rec->max_acl_tier; +struct acl_tier old_max = ls_stateful_rec->max_acl_tier; bool had_acls = ls_stateful_rec->has_acls; bool modified = false; ls_stateful_record_reinit(ls_stateful_rec, od, ls_pg, input_data.ls_port_groups); +struct acl_tier new_max = ls_stateful_rec->max_acl_tier; + if ((had_stateful_acl != ls_stateful_rec->has_stateful_acl) || (had_acls != ls_stateful_rec->has_acls) -|| max_acl_tier != ls_stateful_rec->max_acl_tier) { +|| memcmp(&old_max, &new_max, sizeof(old_max))) { modified = true; } @@ -365,7 +367,8 @@ ls_stateful_record_set_acl_flags(struct ls_stateful_record *ls_stateful_rec, const struct ls_port_group_table *ls_pgs) { ls_stateful_rec->has_stateful_acl = false; -ls_stateful_rec->max_acl_tier = 0; +memset(&ls_stateful_rec->max_acl_tier, 0, + sizeof ls_stateful_rec->max_acl_tier); ls_stateful_rec->has_acls = false; if (ls_stateful_record_set_acl_flags_(ls_stateful_rec, od->nbs->acls, @@ -391,6 +394,36 @@ ls_stateful_record_set_acl_flags(struct ls_stateful_record *ls_stateful_rec, } } +static void +update_ls_max_acl_tier(struct ls_stateful_record *ls_stateful_rec, + const struct nbrec_acl *acl) +{ +if (!acl->tier) { +return; +} + +if (!strcmp(acl->direction, "from-lport")) { +if (smap_get_bool(&acl->options, "apply-after-lb", false)) { +if (acl->tier > ls_stateful_rec->max_acl_tier.ingress_post_lb) { +ls_stateful_rec->max_acl_tier.ingress_post_lb = acl->tier; +} +} else if (acl->tier > ls_stateful_rec->max_acl_tier.ingress_pre_lb) { +ls_stateful_rec->max_acl_tier.ingress_pre_lb = acl->tier; +} +} else if (acl->tier > ls_stateful_rec->max_acl_tier.egress) { +ls_stateful_rec->max_acl_tier.egress = acl->tier; +} +} + +static bool +ls_acl_tiers_are_maxed_out(struct acl_tier *acl_tier, + uint64_t max_allowed_acl_tier) +{ +retu
[ovs-dev] [PATCH ovn branch-24.09 2/2] Prepare for 24.09.1.
The date shown in this patch reflects when I made the patch. I will re-generate the patches before pushing so that the date will reflect the actual release date of ovn 24.09.0. Signed-off-by: Mark Michelson --- NEWS | 3 +++ configure.ac | 2 +- debian/changelog | 6 ++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 2f0c6e907..640227be2 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,6 @@ +OVN v24.09.1 - xx xxx +-- + OVN v24.09.0 - 12 Sep 2024 -- - Added a new logical switch port option "pkt_clone_type". diff --git a/configure.ac b/configure.ac index cd6a5d0c9..53c834faa 100644 --- a/configure.ac +++ b/configure.ac @@ -13,7 +13,7 @@ # limitations under the License. AC_PREREQ(2.63) -AC_INIT(ovn, 24.09.0, b...@openvswitch.org) +AC_INIT(ovn, 24.09.1, b...@openvswitch.org) AC_CONFIG_MACRO_DIR([m4]) AC_CONFIG_AUX_DIR([build-aux]) AC_CONFIG_HEADERS([config.h]) diff --git a/debian/changelog b/debian/changelog index 53feecde8..c7bfbf657 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +ovn (24.09.1-1) unstable; urgency=low + [ OVN team ] + * New upstream version + + -- OVN team Thu, 12 Sep 2024 14:16:30 -0400 + ovn (24.09.0-1) unstable; urgency=low * New upstream version -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-24.09 0/2] Release patches for v24.09.0.
Mark Michelson (2): Set release date for 24.09.0. Prepare for 24.09.1. NEWS | 5 - configure.ac | 2 +- debian/changelog | 8 +++- 3 files changed, 12 insertions(+), 3 deletions(-) -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-24.09 1/2] Set release date for 24.09.0.
The date shown in this patch reflects when I made the patch. I will re-generate the patches before pushing so that the date will reflect the actual release date of ovn 24.09.0. Signed-off-by: Mark Michelson --- NEWS | 2 +- debian/changelog | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index 29d60274e..2f0c6e907 100644 --- a/NEWS +++ b/NEWS @@ -1,4 +1,4 @@ -OVN v24.09.0 - xx xxx +OVN v24.09.0 - 12 Sep 2024 -- - Added a new logical switch port option "pkt_clone_type". If the value is set to "mc_unknown", packets destined to the port gets diff --git a/debian/changelog b/debian/changelog index 8168d1e83..53feecde8 100644 --- a/debian/changelog +++ b/debian/changelog @@ -2,7 +2,7 @@ ovn (24.09.0-1) unstable; urgency=low * New upstream version - -- OVN team Fri, 09 Aug 2024 09:56:41 -0400 + -- OVN team Thu, 12 Sep 2024 14:16:30 -0400 ovn (24.03.0-1) unstable; urgency=low -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 ovn] controller: Accept unicast dhcp-discover in pinctrl_handle_put_dhcp_opts().
Thanks Lorenzo! Acked-by: Mark Michelson On 9/12/24 12:40, Lorenzo Bianconi wrote: According to RFC 2131 section 4.4.4, when the DHCP client knows the address of a DHCP server, in either INIT or REBOOTING state, the client may use that address in the DHCPDISCOVER or DHCPREQUEST rather than the IP broadcast address. Fix pinctrl_handle_put_dhcp_opts implementation according to the RFC 2131. Reported-at: https://issues.redhat.com/browse/FDP-765 Signed-off-by: Lorenzo Bianconi --- Changes in v2: - remove unnecessary ip.dst checks in pinctrl_handle_put_dhcp_opts since we already filter the packets using the server_ip in the related logical flow. - move the new test item at the bottom of the ovn test in order to reduce changes. --- controller/pinctrl.c | 5 - tests/ovn.at | 16 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/controller/pinctrl.c b/controller/pinctrl.c index c86b4f940..b891435c1 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -2598,11 +2598,6 @@ pinctrl_handle_put_dhcp_opts( switch (dhcp_opts.dhcp_msg_type) { case DHCP_MSG_DISCOVER: msg_type = DHCP_MSG_OFFER; -if (in_flow->nw_dst != htonl(INADDR_BROADCAST)) { -static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); -VLOG_WARN_RL(&rl, "DHCP DISCOVER must be Broadcast"); -goto exit; -} break; case DHCP_MSG_REQUEST: { msg_type = DHCP_MSG_ACK; diff --git a/tests/ovn.at b/tests/ovn.at index 4b6e8132f..729e652ed 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -7463,6 +7463,22 @@ expected_dhcp_opts=${boofile}33040e100104ff0003040a0136040a01 test_dhcp 20 1 f001 01 0 $ciaddr $offer_ip $request_ip 1 0 ff11 $server_ip 02 $expected_dhcp_opts compare_dhcp_packets 1 +# -- + +# Send unicast DHCPDISCOVER. +reset_pcap_file hv1-vif1 hv1/vif1 +reset_pcap_file hv1-vif2 hv1/vif2 +rm -f 1.expected +rm -f 2.expected + +offer_ip=`ip_to_hex 10 0 0 4` +server_ip=`ip_to_hex 10 0 0 1` +ciaddr=`ip_to_hex 0 0 0 0` +request_ip=0 +expected_dhcp_opts=4311626f6f7466696c655f6e616d655f616c7433040e100104ff0003040a0136040a01 +test_dhcp 21 1 f001 01 0 $ciaddr $offer_ip $request_ip 0 1 $offer_ip $server_ip ff11 $server_ip 02 $expected_dhcp_opts +compare_dhcp_packets 1 + as hv1 OVS_APP_EXIT_AND_WAIT([ovn-controller]) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 0/6] Fix unit, system and ovn-ic tests.
I merged this to main and branch-24.09. Thanks! On 9/3/24 16:38, Mark Michelson wrote: With the exception of the sign-off error on patch 6, this all looks good to me. Acked-by: Mark Michelson On 8/30/24 03:41, Xavier Simonart wrote: Xavier Simonart (6): tests: Fix flaky "MAC binding aging". tests: Fix flaky "Sampling_App incremental processing". tests: Fix flaky "load-balancer template IPv4". tests: Fix multiple ovn-ic race conditions. tests: Fix flaky ACL Sampling system tests. tests: Fix flaky BFD system test. tests/ovn-ic.at | 47 ++--- tests/ovn-northd.at | 1 + tests/ovn.at | 8 +--- tests/system-ovn.at | 16 ++- 4 files changed, 48 insertions(+), 24 deletions(-) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] controller: Do not remove snat-ct-zone requested by the CMS.
Excellent, thanks Lorenzo! Acked-by: Mark Michelson On 9/10/24 09:51, Lorenzo Bianconi wrote: Do not check if snat-ct-zones are compliant with min/max boundary since they have been explicitly requested by the CMS and this will trigger an unnecessary NXT_CT_FLUSH_ZONE of that zones on ovn-controller restart. Fixes: f2363f49f6a4 ("controller: Add the capability to specify a min/max value for ct_zone.") Fixes: 493ef704a973 ("controller: Prepare structure around CT zone limiting.") Reported-at: https://issues.redhat.com/browse/FDP-773 Co-developed-by: Ales Musil Signed-off-by: Lorenzo Bianconi --- controller/ct-zone.c| 14 --- tests/ovn-controller.at | 89 ++--- 2 files changed, 75 insertions(+), 28 deletions(-) diff --git a/controller/ct-zone.c b/controller/ct-zone.c index 77eb16ac9..469a8fc54 100644 --- a/controller/ct-zone.c +++ b/controller/ct-zone.c @@ -216,12 +216,15 @@ ct_zones_update(const struct sset *local_lports, struct shash_node *node; SHASH_FOR_EACH_SAFE (node, &ctx->current) { struct ct_zone *ct_zone = node->data; -if (!sset_contains(&all_users, node->name) || -ct_zone->zone < min_ct_zone || ct_zone->zone > max_ct_zone) { +if (!sset_contains(&all_users, node->name)) { ct_zone_remove(ctx, node->name); } else if (!simap_find(&req_snat_zones, node->name)) { -bitmap_set1(unreq_snat_zones_map, ct_zone->zone); -simap_put(&unreq_snat_zones, node->name, ct_zone->zone); +if (ct_zone->zone < min_ct_zone || ct_zone->zone > max_ct_zone) { +ct_zone_remove(ctx, node->name); +} else { +bitmap_set1(unreq_snat_zones_map, ct_zone->zone); +simap_put(&unreq_snat_zones, node->name, ct_zone->zone); +} } } @@ -249,10 +252,11 @@ ct_zones_update(const struct sset *local_lports, struct ct_zone *ct_zone = shash_find_data(&ctx->current, snat_req_node->name); +bool flush = !(ct_zone && ct_zone->zone == snat_req_node->data); if (ct_zone && ct_zone->zone != snat_req_node->data) { ct_zone_remove(ctx, snat_req_node->name); } -ct_zone_add(ctx, snat_req_node->name, snat_req_node->data, true); +ct_zone_add(ctx, snat_req_node->name, snat_req_node->data, flush); } /* xxx This is wasteful to assign a zone to each port--even if no diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index dcab5f2e9..a43c6cffb 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -3140,12 +3140,12 @@ ovn_start check_ct_zone_min() { min_val=$1 -OVS_WAIT_UNTIL([test $(ovn-appctl -t ovn-controller ct-zone-list | awk '{print $2}' | sort | head -n1) -ge ${min_val}]) +OVS_WAIT_UNTIL([test $(ovn-appctl -t ovn-controller ct-zone-list | awk '{printf "%02d\n", $2}' | sort | head -n1) -ge ${min_val}]) } check_ct_zone_max() { max_val=$1 -AT_CHECK([test $(ovn-appctl -t ovn-controller ct-zone-list | awk '{print $2}' | sort | tail -n1) -le ${max_val}]) +AT_CHECK([test $(ovn-appctl -t ovn-controller ct-zone-list | awk '{printf "%02d\n", $2}' | sort | tail -n1) -le ${max_val}]) } net_add n1 @@ -3158,44 +3158,87 @@ check ovn-appctl -t ovn-controller vlog/set dbg:ct_zone check ovs-vsctl add-port br-int lsp0 \ -- set Interface lsp0 external-ids:iface-id=lsp0 +check ovs-vsctl add-port br-int lsp1 \ +-- set Interface lsp1 external-ids:iface-id=lsp1 check ovn-nbctl lr-add lr +check ovn-nbctl ls-add ls0 +check ovn-nbctl ls-add ls1 -check ovn-nbctl ls-add ls -check ovn-nbctl lsp-add ls ls-lr -check ovn-nbctl lsp-set-type ls-lr router -check ovn-nbctl lsp-set-addresses ls-lr router -check ovn-nbctl lrp-add lr lr-ls 00:00:00:00:00:01 10.0.0.1 +check ovn-nbctl set logical_router lr options:chassis=hv1 + +check ovn-nbctl lrp-add lr lr-ls0 00:00:00:00:ff:01 10.0.0.1/24 +check ovn-nbctl lsp-add ls0 ls0-lr +check ovn-nbctl lsp-set-type ls0-lr router +check ovn-nbctl lsp-set-addresses ls0-lr 00:00:00:00:ff:01 +check ovn-nbctl lsp-set-options ls0-lr router-port=lr-ls0 -check ovn-nbctl lsp-add ls lsp0 +check ovn-nbctl lsp-add ls0 lsp0 check ovn-nbctl lsp-set-addresses lsp0 "00:00:00:00:00:02 10.0.0.2" -check ovn-nbctl lrp-add lr lrp-gw 01:00:00:00:00:01 172.16.0.1 -check ovn-nbctl lrp-set-gateway-chassis lrp-gw hv1 +check ovn-nbctl lrp-add lr lr-ls1 00:00:00:00:ff:02 172.16.0.1/24 +check ovn-nbctl lsp-add ls1 ls1-lr +check ovn-nbctl lsp-set-type ls1-lr router +check ovn-nbctl lsp-set-addresses ls1-lr 00:00:00:00:ff:02 +check ovn-nbctl lsp-set-options ls1
Re: [ovs-dev] [PATCH ovn v2 2/2] todo: Add note into TODO about PMTUD.
Thanks Ales. Acked-by: Mark Michelson On 8/20/24 05:01, Ales Musil wrote: Add note into TODO that we should use PMTUD when it is supported by all datapaths. Signed-off-by: Ales Musil --- TODO.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/TODO.rst b/TODO.rst index 17d539ad6..ac4202f36 100644 --- a/TODO.rst +++ b/TODO.rst @@ -130,3 +130,6 @@ OVN To-do List * Move the lflow build parallel processing from northd.c to lflow-mgr.c This would also ensure that the variables declared in northd.c (eg. thread_lflow_counter) are not externed in lflow-mgr.c. + +* Remove flows with `check_pkt_larger` when userspace datapath can handle + PMTUD. (https://issues.redhat.com/browse/FDP-256) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 1/2] physical: Prevent wrong FDB to be learned with multichassis port.
Looks good to me, Ales. Acked-by: Mark Michelson On 8/20/24 05:01, Ales Musil wrote: If multichassis VIF is set with "unknown" address it might store wrong MAC address into the FDB table. The ICMP Need frag is generated by swapping the MAC src and dst of the original packet, however the inport remains the same. As a consequence the match on the inport to learn FDB will store source MAC address which is the original destination address, that leads to redirection of traffic the VIF which is wrong. Swap the inport and outport for the ICMP error to prevent this issue it also makes more sense as the ICMP is supposed to be generated by entity along the way and not by the original VIF. Note that those flows is still needed as userspace datapath is not capable of PMTUD yet. Reported-at: https://issues.redhat.com/browse/FDP-620 Signed-off-by: Ales Musil --- v2: Rebase on top of latest main. Fix the typo in the test and add note into the commit message. --- controller/physical.c | 6 ++ tests/ovn.at | 24 +--- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/controller/physical.c b/controller/physical.c index 9e04ad5f2..55edbd54e 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -1258,6 +1258,12 @@ reply_imcp_error_if_pkt_too_big(struct ovn_desired_flow_table *flow_table, ofpact_put_set_field( &inner_ofpacts, mf_from_id(MFF_LOG_FLAGS), &value, &mask); +/* inport <-> outport */ +put_stack(MFF_LOG_INPORT, ofpact_put_STACK_PUSH(&inner_ofpacts)); +put_stack(MFF_LOG_OUTPORT, ofpact_put_STACK_PUSH(&inner_ofpacts)); +put_stack(MFF_LOG_INPORT, ofpact_put_STACK_POP(&inner_ofpacts)); +put_stack(MFF_LOG_OUTPORT, ofpact_put_STACK_POP(&inner_ofpacts)); + /* eth.src <-> eth.dst */ put_stack(MFF_ETH_DST, ofpact_put_STACK_PUSH(&inner_ofpacts)); put_stack(MFF_ETH_SRC, ofpact_put_STACK_PUSH(&inner_ofpacts)); diff --git a/tests/ovn.at b/tests/ovn.at index 50c9f04da..bbb1bee35 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -15740,14 +15740,17 @@ m4_define([MULTICHASSIS_PATH_MTU_DISCOVERY_TEST], second_mac=00:00:00:00:00:02 multi1_mac=00:00:00:00:00:f0 multi2_mac=00:00:00:00:00:f1 + external_mac=00:00:00:00:ee:ff first_ip=10.0.0.1 second_ip=10.0.0.2 multi1_ip=10.0.0.10 multi2_ip=10.0.0.20 + external_ip=10.0.0.30 first_ip6=abcd::1 second_ip6=abcd::2 multi1_ip6=abcd::f0 multi2_ip6=abcd::f1 + external_ip6=abcd::eeff check ovn-nbctl ls-add ls0 check ovn-nbctl lsp-add ls0 first @@ -15866,6 +15869,24 @@ m4_define([MULTICHASSIS_PATH_MTU_DISCOVERY_TEST], reset_env + AS_BOX([Multi with "unknown" to external doesn't produce wrong FDB]) + len=3000 + check ovn-nbctl --wait=hv lsp-set-addresses multi1 unknown + + packet=$(send_ip_packet multi1 1 $multi1_mac $external_mac $multi1_ip $external_ip $(payload $len) 1 ${expected_ip_mtu}) + echo $packet >> hv1/multi1.expected + + packet=$(send_ip6_packet multi1 1 $multi1_mac $external_mac $multi1_ip6 $external_ip6 $(payload $len) 1 ${expected_ip_mtu}) + echo $packet >> hv1/multi1.expected + + check_pkts + reset_env + + check_row_count fdb 0 mac="$external_mac" + ovn-sbctl --all destroy fdb + + check ovn-nbctl --wait=hv lsp-set-addresses multi1 "${multi1_mac} ${multi1_ip} ${multi1_ip6}" + AS_BOX([Packets of proper size are delivered from multichassis to regular ports]) len=1000 @@ -15965,9 +15986,6 @@ m4_define([MULTICHASSIS_PATH_MTU_DISCOVERY_TEST], packet=$(send_ip6_packet multi1 1 $multi1_mac $multi2_mac $multi1_ip6 $multi2_ip6 $(payload $len) 1) echo $packet >> hv1/multi1.expected - check_pkts - reset_env - AS_BOX([MTU updates are honored in ICMP Path MTU calculation]) set_mtu() { ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovs: Move the submodule to the official v3.4.0 release.
Acked-by: Mark Michelson On 8/20/24 07:03, Ilya Maximets wrote: OVS v3.4.0 contains several bug fixes in comparison to the current submodule commit. They may not be very important for OVN itself, but it's better to use an officially released version than a random commit on a branch. Signed-off-by: Ilya Maximets --- ovs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ovs b/ovs index 0aa14d912..c598c05c8 16 --- a/ovs +++ b/ovs @@ -1 +1 @@ -Subproject commit 0aa14d912d9a29d07ebc727007a1f21e3639eea5 +Subproject commit c598c05c85b2d38874a0ce8f7f088f6aae4fdabc ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] controller: Accept unicast dhcp-discover in pinctrl_handle_put_dhcp_opts().
Hi Lorenzo, I think this can be done more simply. In northd, the logical flow we create for the put_dhcp_opts() action has the following match conditions: ip4.src == {OFFER_IP, 0.0.0.0} && ip4.dst == {SERVER_IP, 255.255.255.255} Since the flow already filters based on destination IP address, it should mean that pinctrl_handle_put_dhcp_opts() will only be called if the destination IP address is the configured OVN server IP, or if it is broadcast. Therefore, checking for an IP match in pinctrl_handle_put_dhcp_opts() is unnecessary. The "case DHCP_MSG_DISCOVER:" section simply doesn't need to check in_flow->nw_dst at all. We can just set the msg_type to DHCP_MSG_OFFER and break. What do you think? Thanks, Mark Michelson PS One final item: Why is the test altered so that the new test case is at the beginning? This causes the sequence numbers of all subsequent test_dhcp calls to get bumped by one. Would it be easier to add the new test case to the end, and therefore not have to update so much of the test? On 9/5/24 05:43, Lorenzo Bianconi wrote: According to RFC 2131 section 4.4.4, when the DHCP client knows the address of a DHCP server, in either INIT or REBOOTING state, the client may use that address in the DHCPDISCOVER or DHCPREQUEST rather than the IP broadcast address. Fix pinctrl_handle_put_dhcp_opts implementation according to the RFC 2131. Reported-at: https://issues.redhat.com/browse/FDP-765 Signed-off-by: Lorenzo Bianconi --- controller/pinctrl.c | 48 ++- tests/ovn.at | 53 2 files changed, 76 insertions(+), 25 deletions(-) diff --git a/controller/pinctrl.c b/controller/pinctrl.c index c86b4f940..d3096c2d3 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -2596,14 +2596,50 @@ pinctrl_handle_put_dhcp_opts( uint8_t msg_type = 0; switch (dhcp_opts.dhcp_msg_type) { -case DHCP_MSG_DISCOVER: +case DHCP_MSG_DISCOVER: { msg_type = DHCP_MSG_OFFER; -if (in_flow->nw_dst != htonl(INADDR_BROADCAST)) { -static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); -VLOG_WARN_RL(&rl, "DHCP DISCOVER must be Broadcast"); -goto exit; +if (in_flow->nw_dst == htonl(INADDR_BROADCAST)) { +break; } -break; + +const char *ptr = reply_dhcp_opts_ptr->data; +const char *ptr_end = ptr + reply_dhcp_opts_ptr->size; +ovs_be32 server_id = htonl(INADDR_BROADCAST); + +while (ptr < ptr_end) { +struct dhcp_opt_header *in_dhcp_opt = +(struct dhcp_opt_header *) ptr; + +/* RFC 2131 section 4.4.4. + * When the DHCP client knows the address of a DHCP server, in + * either INIT or REBOOTING state, the client may use that address + * in the DHCPDISCOVER or DHCPREQUEST rather than the IP broadcast + * address. + */ +if (in_dhcp_opt->code == OVN_DHCP_OPT_CODE_SERVER_ID) { +server_id = get_unaligned_be32(DHCP_OPT_PAYLOAD(in_dhcp_opt)); +break; +} + +ptr += sizeof *in_dhcp_opt; +if (ptr > ptr_end) { +break; +} +ptr += in_dhcp_opt->len; +if (ptr > ptr_end) { +break; +} +} + +if (in_flow->nw_dst == server_id) { +break; +} + +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); +VLOG_WARN_RL(&rl, "DHCP DISCOVER invalid address: "IP_FMT, + IP_ARGS(in_flow->nw_dst)); +goto exit; +} case DHCP_MSG_REQUEST: { msg_type = DHCP_MSG_ACK; if (dhcp_opts.request_ip != *offer_ip) { diff --git a/tests/ovn.at b/tests/ovn.at index acf18c4e0..f21249b3e 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -7047,6 +7047,21 @@ compare_dhcp_packets 1 # -- +# Send unicast DHCPDISCOVER. +reset_pcap_file hv1-vif1 hv1/vif1 +reset_pcap_file hv1-vif2 hv1/vif2 +rm -f 1.expected +rm -f 2.expected +offer_ip=`ip_to_hex 10 0 0 4` +server_ip=`ip_to_hex 10 0 0 1` +ciaddr=`ip_to_hex 0 0 0 0` +request_ip=0 +expected_dhcp_opts=33040e100104ff0003040a0136040a01 +test_dhcp 2 1 f001 01 0 $ciaddr $offer_ip $request_ip 0 1 $offer_ip $server_ip ff11 $server_ip 02 $expected_dhcp_opts +compare_dhcp_packets 1 + +# -- + # ovs-ofctl also resumes the packets and this causes other ports to receive # the DHCP request packet. So reset the pcap files so that its easier to test. reset_pcap_file hv1-vif1 hv1/vif1 @@ -7061,7 +7076,7 @@ server_ip=`ip_to
Re: [ovs-dev] [PATCH ovn branch-24.09 1/2] Set release date for 24.09.0.
On 9/6/24 11:37, Dumitru Ceara wrote: On 9/6/24 12:59, Dumitru Ceara wrote: On 9/5/24 19:11, Mark Michelson wrote: Signed-off-by: Mark Michelson --- Hi Mark, NEWS | 2 +- debian/changelog | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index 29d60274e..92ebe6e9c 100644 --- a/NEWS +++ b/NEWS @@ -1,4 +1,4 @@ -OVN v24.09.0 - xx xxx +OVN v24.09.0 - 06 Sep 2024 -- - Added a new logical switch port option "pkt_clone_type". If the value is set to "mc_unknown", packets destined to the port gets diff --git a/debian/changelog b/debian/changelog index 8168d1e83..f31208d7d 100644 --- a/debian/changelog +++ b/debian/changelog @@ -2,7 +2,7 @@ ovn (24.09.0-1) unstable; urgency=low * New upstream version - -- OVN team Fri, 09 Aug 2024 09:56:41 -0400 + -- OVN team Thu, 06 Sep 2024 13:05:51 -0400 This should be: Fri, 06 Sep. With that addressed: Acked-by: Dumitru Ceara Sorry for the last minute bug report but it seems there's a regression that was introduced in the 24.09 code: https://issues.redhat.com/browse/FDP-773 It's about ovn-controller incorrectly flushing conntrack zones that it used to own when it gets restarted. Git bisect points to: https://github.com/ovn-org/ovn/commit/f2363f49f6a46e784b1d8dad33eee733aa09380c Mark, what do you think we should do? This bug potentially disrupts the dataplane (for ovn-kubernetes at least) during upgrades. Regards, Dumitru Releasing with a known regression is a bad idea. Let's get this fixed, with the goal being to release in a week (13 September). I'll remove this review from patchwwork and post a new set of patches next week. Thanks, Mark Michelson ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-24.09 2/2] Prepare for 24.09.1.
Signed-off-by: Mark Michelson --- NEWS | 3 +++ configure.ac | 2 +- debian/changelog | 6 ++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 92ebe6e9c..beaf5fcf8 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,6 @@ +OVN v24.09.1 - xx xxx +-- + OVN v24.09.0 - 06 Sep 2024 -- - Added a new logical switch port option "pkt_clone_type". diff --git a/configure.ac b/configure.ac index cd6a5d0c9..53c834faa 100644 --- a/configure.ac +++ b/configure.ac @@ -13,7 +13,7 @@ # limitations under the License. AC_PREREQ(2.63) -AC_INIT(ovn, 24.09.0, b...@openvswitch.org) +AC_INIT(ovn, 24.09.1, b...@openvswitch.org) AC_CONFIG_MACRO_DIR([m4]) AC_CONFIG_AUX_DIR([build-aux]) AC_CONFIG_HEADERS([config.h]) diff --git a/debian/changelog b/debian/changelog index f31208d7d..4eec415d9 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +ovn (24.09.1-1) unstable; urgency=low + [ OVN team ] + * New upstream version + + -- OVN team Thu, 06 Sep 2024 13:05:51 -0400 + ovn (24.09.0-1) unstable; urgency=low * New upstream version -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-24.09 1/2] Set release date for 24.09.0.
Signed-off-by: Mark Michelson --- NEWS | 2 +- debian/changelog | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index 29d60274e..92ebe6e9c 100644 --- a/NEWS +++ b/NEWS @@ -1,4 +1,4 @@ -OVN v24.09.0 - xx xxx +OVN v24.09.0 - 06 Sep 2024 -- - Added a new logical switch port option "pkt_clone_type". If the value is set to "mc_unknown", packets destined to the port gets diff --git a/debian/changelog b/debian/changelog index 8168d1e83..f31208d7d 100644 --- a/debian/changelog +++ b/debian/changelog @@ -2,7 +2,7 @@ ovn (24.09.0-1) unstable; urgency=low * New upstream version - -- OVN team Fri, 09 Aug 2024 09:56:41 -0400 + -- OVN team Thu, 06 Sep 2024 13:05:51 -0400 ovn (24.03.0-1) unstable; urgency=low -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-24.09 0/2] Release patches for v24.09.0.
Mark Michelson (2): Set release date for 24.09.0. Prepare for 24.09.1. NEWS | 5 - configure.ac | 2 +- debian/changelog | 8 +++- 3 files changed, 12 insertions(+), 3 deletions(-) -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] controller: Avoid quadratic complexity for multi-chassis ports.
Thanks Ales, it took a bit to verify that the behavior was still correct, but it looks good to me. Acked-by: Mark Michelson On 8/26/24 09:15, Ales Musil wrote: The processing for ports had a quadratic complexity when MTU change was involved. Avoid the unnecessary processing and do the lookup loop only once and only in case it was actually needed. In addition to the quadratic complexity the condition to do the flow recompute was wrong and this resulted for the lookup in physical_handle_flows_for_lport() to run for all ports. The performance difference is very noticeable, see the number below in a test with 800 LSPs: Before: physical_flow_output, handler for input if_status_mgr took 2072ms After: physical_flow_output, handler for input if_status_mgr took 4ms Fixes: cdd8dea88b3d ("Track interface MTU in if-status-mgr") Fixes: 7084cf437421 ("Always funnel multichassis port traffic through tunnels") Co-authored-by: Ilya Maximets Signed-off-by: Ilya Maximets Signed-off-by: Ales Musil --- controller/ovn-controller.c | 17 +- controller/physical.c | 49 --- controller/physical.h | 3 ++ tests/ovn.at| 67 + 4 files changed, 93 insertions(+), 43 deletions(-) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 854d80bdf..baced288e 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -4456,22 +4456,7 @@ pflow_output_if_status_mgr_handler(struct engine_node *node, } if (pb->n_additional_chassis) { /* Update flows for all ports in datapath. */ -struct sbrec_port_binding *target = -sbrec_port_binding_index_init_row( -p_ctx.sbrec_port_binding_by_datapath); -sbrec_port_binding_index_set_datapath(target, pb->datapath); - -const struct sbrec_port_binding *binding; -SBREC_PORT_BINDING_FOR_EACH_EQUAL ( -binding, target, p_ctx.sbrec_port_binding_by_datapath) { -bool removed = sbrec_port_binding_is_deleted(binding); -if (!physical_handle_flows_for_lport(binding, removed, &p_ctx, - &pfo->flow_table)) { -destroy_physical_ctx(&p_ctx); -return false; -} -} -sbrec_port_binding_index_destroy_row(target); +physical_multichassis_reprocess(pb, &p_ctx, &pfo->flow_table); } else { /* If any multichassis ports, update flows for the port. */ bool removed = sbrec_port_binding_is_deleted(pb); diff --git a/controller/physical.c b/controller/physical.c index 9e04ad5f2..36a146a58 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -2397,33 +2397,9 @@ physical_handle_flows_for_lport(const struct sbrec_port_binding *pb, } } -if (ldp) { -bool multichassis_state_changed = ( -!!pb->additional_chassis == -!!shash_find(&ldp->multichassis_ports, pb->logical_port) -); -if (multichassis_state_changed) { -if (pb->additional_chassis) { -add_local_datapath_multichassis_port( -ldp, pb->logical_port, pb); -} else { -remove_local_datapath_multichassis_port( -ldp, pb->logical_port); -} - -struct sbrec_port_binding *target = -sbrec_port_binding_index_init_row( -p_ctx->sbrec_port_binding_by_datapath); -sbrec_port_binding_index_set_datapath(target, ldp->datapath); - -const struct sbrec_port_binding *port; -SBREC_PORT_BINDING_FOR_EACH_EQUAL ( -port, target, p_ctx->sbrec_port_binding_by_datapath) { -ofctrl_remove_flows(flow_table, &port->header_.uuid); -physical_eval_port_binding(p_ctx, port, flow_table); -} -sbrec_port_binding_index_destroy_row(target); -} +if (sbrec_port_binding_is_updated( +pb, SBREC_PORT_BINDING_COL_ADDITIONAL_CHASSIS) || removed) { +physical_multichassis_reprocess(pb, p_ctx, flow_table); } if (!removed) { @@ -2440,6 +2416,25 @@ physical_handle_flows_for_lport(const struct sbrec_port_binding *pb, return true; } +void +physical_multichassis_reprocess(const struct sbrec_port_binding *pb, +struct physical_ctx *p_ctx, +struct ovn_desired_flow_table *flow_table) +{ +struct sbrec_port_binding *target = +sbrec_port_binding_index_init_row( +p_ctx->sbrec_port_binding_by_datapath); +sbrec_port_binding_index_set_datapath(target, pb->datap
Re: [ovs-dev] [PATCH ovn] news: Fix indentation for an entry.
Thank you Vladislav. I acked and merged this to main and branch-24.09. On 9/2/24 08:05, Vladislav Odintsov wrote: Signed-off-by: Vladislav Odintsov --- NEWS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 318f63195..e0a48aaa7 100644 --- a/NEWS +++ b/NEWS @@ -59,7 +59,7 @@ OVN v24.09.0 - xx xxx - The NB_Global.debug_drop_domain_id configured value is now overridden by the ID associated with the Sampling_App record created for drop sampling (Sampling_App.type configured as "drop"). -- Add support for ACL sampling through the new Sample_Collector and Sample + - Add support for ACL sampling through the new Sample_Collector and Sample tables. Sampling is supported for both traffic that creates new connections and for traffic that is part of an existing connection. - Add "external_ids:ovn-encap-ip-default" config for ovn-controller to ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 0/6] Fix unit, system and ovn-ic tests.
With the exception of the sign-off error on patch 6, this all looks good to me. Acked-by: Mark Michelson On 8/30/24 03:41, Xavier Simonart wrote: Xavier Simonart (6): tests: Fix flaky "MAC binding aging". tests: Fix flaky "Sampling_App incremental processing". tests: Fix flaky "load-balancer template IPv4". tests: Fix multiple ovn-ic race conditions. tests: Fix flaky ACL Sampling system tests. tests: Fix flaky BFD system test. tests/ovn-ic.at | 47 ++--- tests/ovn-northd.at | 1 + tests/ovn.at| 8 +--- tests/system-ovn.at | 16 ++- 4 files changed, 48 insertions(+), 24 deletions(-) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/2] ct_zones: Fix virtual ports.
Thanks Xavier, Acked-by: Mark Michelson On 8/27/24 09:46, Xavier Simonart wrote: Do not allocate ct_zones for virtual ports. Signed-off-by: Xavier Simonart --- controller/ct-zone.c| 11 +-- controller/ct-zone.h| 3 ++- controller/ovn-controller.c | 5 - tests/ovn.at| 26 +- 4 files changed, 40 insertions(+), 5 deletions(-) diff --git a/controller/ct-zone.c b/controller/ct-zone.c index 77eb16ac9..2385b700d 100644 --- a/controller/ct-zone.c +++ b/controller/ct-zone.c @@ -19,6 +19,7 @@ #include "binding.h" #include "chassis.h" #include "ct-zone.h" +#include "lport.h" #include "local_data.h" #include "openvswitch/vlog.h" @@ -169,7 +170,8 @@ out: void ct_zones_update(const struct sset *local_lports, const struct ovsrec_open_vswitch_table *ovs_table, -const struct hmap *local_datapaths, struct ct_zone_ctx *ctx) +const struct hmap *local_datapaths, struct ct_zone_ctx *ctx, +struct ovsdb_idl_index *sbrec_port_binding_by_name) { int min_ct_zone, max_ct_zone; const char *user; @@ -179,8 +181,13 @@ ct_zones_update(const struct sset *local_lports, struct simap unreq_snat_zones = SIMAP_INITIALIZER(&unreq_snat_zones); const char *local_lport; +const struct sbrec_port_binding *pb; SSET_FOR_EACH (local_lport, local_lports) { -sset_add(&all_users, local_lport); +pb = lport_lookup_by_name(sbrec_port_binding_by_name, + local_lport); +if (!pb || strcmp(pb->type, "virtual")) { +sset_add(&all_users, local_lport); +} } /* Local patched datapath (gateway routers) need zones assigned. */ diff --git a/controller/ct-zone.h b/controller/ct-zone.h index 6df03975c..fcb100619 100644 --- a/controller/ct-zone.h +++ b/controller/ct-zone.h @@ -73,7 +73,8 @@ void ct_zones_restore(struct ct_zone_ctx *ctx, void ct_zones_update(const struct sset *local_lports, const struct ovsrec_open_vswitch_table *ovs_table, const struct hmap *local_datapaths, - struct ct_zone_ctx *ctx); + struct ct_zone_ctx *ctx, + struct ovsdb_idl_index *sbrec_port_binding_by_name); void ct_zones_commit(const struct ovsrec_bridge *br_int, const struct ovsrec_datapath *ovs_dp, struct ovsdb_idl_txn *ovs_idl_txn, diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index a10f1af8a..911c7f162 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1174,6 +1174,7 @@ struct ed_type_runtime_data { struct shash local_active_ports_ras; struct sset *postponed_ports; +struct ovsdb_idl_index *sbrec_port_binding_by_name; }; /* struct ed_type_runtime_data has the below members for tracking the @@ -1438,6 +1439,7 @@ en_runtime_data_run(struct engine_node *node, void *data) binding_run(&b_ctx_in, &b_ctx_out); rt_data->localnet_learn_fdb = b_ctx_out.localnet_learn_fdb; +rt_data->sbrec_port_binding_by_name = b_ctx_in.sbrec_port_binding_by_name; engine_set_node_state(node, EN_UPDATED); } @@ -2230,7 +2232,8 @@ en_ct_zones_run(struct engine_node *node, void *data) ct_zones_restore(&ct_zones_data->ctx, ovs_table, dp_table, br_int); ct_zones_update(&rt_data->local_lports, ovs_table, -&rt_data->local_datapaths, &ct_zones_data->ctx); +&rt_data->local_datapaths, &ct_zones_data->ctx, +rt_data->sbrec_port_binding_by_name); ct_zones_limits_sync(&ct_zones_data->ctx, &rt_data->local_datapaths, &rt_data->lbinding_data.lports); diff --git a/tests/ovn.at b/tests/ovn.at index 4d9beea1b..ff7281e59 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -38951,8 +38951,29 @@ check ovn-nbctl lrp-add r1 r1-ls2 f0:00:00:00:00:02 192.168.3.1/24 \ -- lsp-add ls2 ls2-r1 \ -- set Logical_Switch_Port ls2-r1 type=router options:router-port=r1-ls2 addresses=router +check ovn-nbctl lsp-add ls1 ls1-vir \ +-- lsp-set-addresses ls1-vir "50:54:00:00:00:10 192.168.2.2" \ +-- lsp-set-type ls1-vir virtual \ +-- set logical_switch_port ls1-vir options:virtual-ip=192.168.2.2 \ +-- set logical_switch_port ls1-vir options:virtual-parents=lsp1 + check ovn-nbctl --wait=hv sync +send_garp() { +hv=$1 +dev=$2 +eth_src=$3 +ip=$4 + +packet=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${eth_src}')/ \ + ARP(op=2, hwsrc='${eth_src}
Re: [ovs-dev] [PATCH ovn 1/2] ct_zones: Fix ct_zone removal on port type change.
Thanks Xavier, it looks good to me. Acked-by: Mark Michelson On 8/27/24 09:46, Xavier Simonart wrote: In ct_zones I+P, when ports changed to patch ports (e.g. from l3gateway, for which ct_zone has been allocated), delete the ct_zone. Signed-off-by: Xavier Simonart --- controller/binding.c| 10 +++ controller/ovn-controller.c | 9 ++- tests/ovn.at| 142 3 files changed, 160 insertions(+), 1 deletion(-) diff --git a/controller/binding.c b/controller/binding.c index bfdeb99b9..d3fe127f7 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -2934,6 +2934,16 @@ consider_patch_port_for_local_datapaths(const struct sbrec_port_binding *pb, b_ctx_out->tracked_dp_bindings); } } + +if (sbrec_port_binding_is_updated(pb, SBREC_PORT_BINDING_COL_TYPE) && + (pb->chassis == b_ctx_in->chassis_rec || +is_additional_chassis(pb, b_ctx_in->chassis_rec))) { +remove_local_lports(pb->logical_port, b_ctx_out); +release_lport(pb, b_ctx_in->chassis_rec, + !b_ctx_in->ovnsb_idl_txn, + b_ctx_out->tracked_dp_bindings, + b_ctx_out->if_mgr); +} } static bool diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 854d80bdf..a10f1af8a 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -2312,9 +2312,16 @@ ct_zones_runtime_data_handler(struct engine_node *node, void *data) && strcmp(t_lport->pb->type, "localnet")) { /* We allocate zone-id's only to VIF, localport, l3gateway, * and localnet lports. */ +if (sbrec_port_binding_is_updated(t_lport->pb, + SBREC_PORT_BINDING_COL_TYPE)) { +updated |= ct_zone_handle_port_update(&ct_zones_data->ctx, + t_lport->pb, + false, &scan_start, + min_ct_zone, max_ct_zone); +} + continue; } - bool port_updated = t_lport->tracked_type == TRACKED_RESOURCE_NEW || t_lport->tracked_type == TRACKED_RESOURCE_UPDATED; diff --git a/tests/ovn.at b/tests/ovn.at index 50c9f04da..4d9beea1b 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -38924,3 +38924,145 @@ OVN_CHECK_PACKETS([hv/vif1-tx.pcap], [expected-vif1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ct_zones]) +AT_KEYWORDS([ct_zones]) + +ovn_start + +net_add n1 +for i in 1 2; do +check ovn-nbctl ls-add ls${i} +ovn-nbctl lsp-add ls${i} lsp${i} +sim_add hv$i +as hv$i +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.$i +ovs-vsctl -- add-port br-int vif${i} -- set Interface vif${i} external-ids:iface-id=lsp${i} +done + +check ovn-nbctl lr-add r1 +check ovn-nbctl lrp-add r1 r1-ls1 f0:00:00:00:00:01 192.168.2.1/24 \ +-- lsp-add ls1 ls1-r1 \ +-- set Logical_Switch_Port ls1-r1 type=router options:router-port=r1-ls1 addresses=router + +check ovn-nbctl lrp-add r1 r1-ls2 f0:00:00:00:00:02 192.168.3.1/24 \ +-- lsp-add ls2 ls2-r1 \ +-- set Logical_Switch_Port ls2-r1 type=router options:router-port=r1-ls2 addresses=router + +check ovn-nbctl --wait=hv sync + +AS_BOX([With patch ports]) +zone_list_hv1=$(as hv1 ovn-appctl -t ovn-controller ct-zone-list | sort) +AT_CHECK([echo "$zone_list_hv1" | sed "s/ [[0-9]]*/ ??/" | sort], [0], [dnl +ls1_dnat ?? +ls1_snat ?? +ls2_dnat ?? +ls2_snat ?? +lsp1 ?? +r1_dnat ?? +r1_snat ?? +]) + +zone_list_hv2=$(as hv2 ovn-appctl -t ovn-controller ct-zone-list | sort) +AT_CHECK([echo "$zone_list_hv2" | sed "s/ [[0-9]]*/ ??/" | sort], [0], [dnl +ls1_dnat ?? +ls1_snat ?? +ls2_dnat ?? +ls2_snat ?? +lsp2 ?? +r1_dnat ?? +r1_snat ?? +]) + +AS_BOX([With patch ports after recompute]) +check as hv1 ovn-appctl -t ovn-controller inc-engine/recompute +check as hv2 ovn-appctl -t ovn-controller inc-engine/recompute +zone_list_hv1_after_recompute=$(as hv1 ovn-appctl -t ovn-controller ct-zone-list | sort) +zone_list_hv2_after_recompute=$(as hv2 ovn-appctl -t ovn-controller ct-zone-list | sort) +AT_CHECK([test "X$zone_list_hv1" = "X$zone_list_hv1_after_recompute"]) +AT_CHECK([test "X$zone_list_hv2" = "X$zone_list_hv2_after_recompute"]) + +AS_BOX([With l3gw in hv2]) +check ovn-nbctl --wait=hv set Logical_Router r1 options:chassis=hv2 +# ls2 should now only be a local datapath in hv2. +# However, ovn-controller does not remove datapath from list of ldp when needed, so ls2 is still seen as local in hv1. +zone_
Re: [ovs-dev] [PATCH ovn v2 1/3] Northd: Start tracking virtual port binding requests.
Hi Mohammad, Correct me if I'm wrong, but I don't think this code handles the I-P recompute case properly. In ls_handle_lsp_changes(), you add a tracked virtual port in the case where a logical switch port is added, and you remove the tracked virtual port in the case where a logical switch port is removed. However, in the case where a logical switch port changes and results in a recompute, I think that should result in the destruction of all tracked virtual ports. Then when the recompute happens, build_ports() will reconstruct the hmap of tracked virtual ports. Currently, if a recompute is needed, since the tracked virtual ports are not cleared, it can result in duplicated entries in the hmap, or worse, it could result in stale entries. There are additional notes inline below. On 8/1/24 06:44, Mohammad Heib wrote: Northd handles virtual port binding requests received by ovn-controllers without tracking those requests or saving any info about the last binding requests and the number of requests received for an individual virtual port. This patch adds a basic tracking mechanism for each virtual port that future patches will use to limit/pause the controller from sending binding requests for a specific virtual port if this port overflows the system by such requests. Signed-off-by: Mohammad Heib --- northd/northd.c | 88 + northd/northd.h | 2 ++ northd/ovn-northd.c | 3 ++ 3 files changed, 93 insertions(+) diff --git a/northd/northd.c b/northd/northd.c index a8a0b6f94..89d5b2936 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3757,6 +3757,79 @@ build_lb_port_related_data( build_lswitch_lbs_from_lrouter(lr_datapaths, lb_dps_map, lb_group_dps_map); } +/* + * These functions implements the binding request tracking for a virtual + * port which can be used to limit virtual port binding requests + * and avoid system overflow. + * + * Virtual port binding requests must not exceed + * VPORT_MAX_BINDING_REQUEST_TRESHOLD within a VPORT_BINDING_TIMEFRAME, + * otherwise, this vport must be defined as overflowed and should limit + * the binding request in this port for a certain time. + */ +#define VPORT_BINDING_TIMEFRAME 1 +#define VPORT_MAX_BINDING_REQUEST_TRESHOLD 15 + +static struct hmap tracked_virtual_ports; + +struct tracked_virtual_port { +struct hmap_node node; +/* + * Use port name instaed of ovn_port refrence to make s/instaed/instead/ s/refrence/reference/ + * sure that virtual port tracking data will be permanent accross s/accross/across/ + * northd loops and we can keep track the target ports. + */ +char *name; +long long int First_bind_in_tframe; +size_t Bind_request_cnt; Please do not capitalize the names of struct fields. +}; + +static struct tracked_virtual_port * +find_tracked_virtual_port(const char *name) { +struct tracked_virtual_port *vport; +HMAP_FOR_EACH (vport, node, &tracked_virtual_ports) { The hmap uses the name as the hash function, so there is no reason to traverse the entire hmap to find the port. Instead, hash the input "name" parameter and use HMAP_FOR_EACH_WITH_HASH() so that less of the hmap has to be traversed. +if (!strcmp(name, vport->name)) { +return vport; +} +} +return NULL; +} + +static void +add_to_tracked_virtual_ports(const char *name) { +struct tracked_virtual_port *vport = find_tracked_virtual_port(name); +if (!vport) { +vport = xmalloc(sizeof *vport); +vport->name = xstrdup(name); +vport->First_bind_in_tframe = 0; +vport->Bind_request_cnt = 0; +hmap_insert(&tracked_virtual_ports, &vport->node, +hash_string(name, 0)); +} +} + +static void +remove_from_tracked_virtual_ports(const char *name) { +struct tracked_virtual_port *vport = find_tracked_virtual_port(name); +if (vport) { +free(vport->name); +hmap_remove(&tracked_virtual_ports, &vport->node); +free(vport); +} +} + +void init_tracked_virtual_ports(void) { +hmap_init(&tracked_virtual_ports); +} + +void destroy_tracked_virtual_ports(void) { +struct tracked_virtual_port *vport; +HMAP_FOR_EACH_SAFE (vport, node, &tracked_virtual_ports) { +remove_from_tracked_virtual_ports(vport->name); This construct results in an O(n^2) complexity algorithm since the hmap is traversed, and then remove_from_tracked_virtual_ports() traverses the hmap again to find the vport before removing it. One way to fix this would be to separate remove_from_tracked_virtual_ports() into two steps: step 1 (inline): Find the vport and remove it from the hmap step 2: Create a function (e.g. tracked_vport_destroy()) that frees a vport. Call that function here. Then here in destroy_tracked_virtual_ports(), you can use HMAP_FOR_EACH_POP() and call tracked_vport_destroy() on each vport directly. +
Re: [ovs-dev] [PATCH ovn v2 2/3] Northd: Pause virtual port binding requests for crowded ports.
On 8/1/24 06:44, Mohammad Heib wrote: ovn-controller sends binding requests to update the virtual parent of a virtual port to northd, in some cases those requests are not handled immediately and ovn-controller keeps sending requests over and over which can lead to flooding northd with these requests. This patch add the ability to pause virtual ports that send so many binding requests to northd. Signed-off-by: Mohammad Heib --- northd/northd.c | 98 + 1 file changed, 98 insertions(+) diff --git a/northd/northd.c b/northd/northd.c index 89d5b2936..a2782031d 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3093,6 +3093,15 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, "qdisc_queue_id", "%d", queue_id); } +if (smap_get_bool(&op->sb->options, +"binding_request_pause", false)) { +long long int p_time = smap_get_ullong(&op->sb->options, +"binding_request_pause_ts", 0); +smap_add_format(&options, "binding_request_pause_ts", +"%lld", p_time); +smap_add(&options, "binding_request_pause", "true"); +} + if (smap_get_bool(&op->od->nbs->other_config, "vlan-passthru", false)) { smap_add(&options, "vlan-passthru", "true"); } @@ -3830,6 +3839,90 @@ void destroy_tracked_virtual_ports(void) { hmap_destroy(&tracked_virtual_ports); } +/* + * For every virtual port that send request to update thier virtual_parent + * This function will update the following Port_binding options if needed: + * + * 1. tracked_virtual_port record belongs to this virtual port was created + *when this port created. This tracked struct have two main Fields: + * + * a. First_bind_in_tframe: this field will be set to the time that + * binding request were reicved for this vport for the first time + * within a timeframe. + * + * b. Bind_request_cnt: this filed will be incresses every time a binding + * request recived for that virtual port. + * + * + * 2. For each binding request received for a specific virtual port + * check if the time diff between now and the first time that a + * binding request were recived for this port within a pre-define + * timeframe is less than that timeframe. + * + * 3. If the previous condition true increase Bind_request_cnt and + * check if the total recived binding request recived for this port + * within a time fram exceeded the VPORT_MAX_BINDING_REQUEST_TRESHOLD + * set the Port_binding options: + * + * PB:OPTIONS:binding_request_pause=true + * PB:OPTIONS:binding_request_pause_ts=time_now + * + * + * 4. When ovn-controller recived a new GARP for this virtual port + * before sending a binding request update to northd it will check + * if the port have binding_request_pause=true, ovn-controller will do + * the following: + * + * If the PB:OPTIONS:binding_request_pause_ts + 10 seconds greater + * than the time now (GARP processing time), drop the GARP packet. + * + * Otherwise, set the PB:OPTIONS:binding_request_pause=false and resume + * binding request handling on this virtual port. + * Please run the above comment block through a spelling and grammar checker. + * + */ +static void +vport_binding_request_exceed_threshold(struct ovn_port *op) +{ +struct tracked_virtual_port * vport = + find_tracked_virtual_port(op->key); +if (op->sb != NULL) { There's nothing necessarily wrong with comparing to NULL, but the rest of the codebase tends use "if (op->sb)" instead, so this sticks out. +/* This port already paused or not found ignore it */ +if ((smap_get_bool(&op->sb->options, "binding_request_pause", + false) == true) || !vport) { Similarly, the " == true" here sticks out as odd since it is not typically done this way in OVS/OVN code. Most other instances (including in this patch) just use "if (smap_get_bool())" +return; +} +} + +long long int cur_time = time_msec(); + +/* Still in the range of the time frame. */ +if ((vport->First_bind_in_tframe + VPORT_BINDING_TIMEFRAME) > cur_time) { +if (++vport->Bind_request_cnt > VPORT_MAX_BINDING_REQUEST_TRESHOLD) { +if (op->sb != NULL) { Nit: Instead of three separate nested ifs, you could combine these into one if statement. This reduces the indentation level of the code inside. +static struct vlog_rate_limit rl = +VLOG_RATE_LIMIT_INIT(1, 1); +VLOG_WARN_RL(&rl, "Pausing virtual port %s from sending" +" binding requests for few seconds. " +" This port was paused in order to reduce the load on the" +
Re: [ovs-dev] [PATCH ovn v2 3/3] controller: Drop binding requests for paused virtual port.
On 8/1/24 06:44, Mohammad Heib wrote: Drop the binding requests for a virtual port if that port set to pause in northd. Signed-off-by: Mohammad Heib --- controller/pinctrl.c | 39 ++- tests/ovn.at | 91 2 files changed, 128 insertions(+), 2 deletions(-) diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 7cbb0cf81..7420f2009 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -7057,11 +7057,16 @@ struct put_vport_binding { /* This vport record Only relevant if "new_record" is true. */ bool new_record; +/* The creation time in pinctrl thread */ +long long int creation_time; }; /* Contains "struct put_vport_binding"s. */ static struct hmap put_vport_bindings; +/* pause duration for port that set puased in northd. */ +#define PAUSE_DURATION 1 + /* * Validate if the vport_binding record that was added * by the pinctrl thread is still relevant and needs @@ -7145,7 +7150,7 @@ run_put_vport_binding(struct ovsdb_idl_txn *ovnsb_idl_txn OVS_UNUSED, struct ovsdb_idl_index *sbrec_datapath_binding_by_key, struct ovsdb_idl_index *sbrec_port_binding_by_key, const struct sbrec_chassis *chassis, - const struct put_vport_binding *vpb) + struct put_vport_binding *vpb) { /* Convert logical datapath and logical port key into lport. */ const struct sbrec_port_binding *pb = lport_lookup_by_key( @@ -7159,6 +7164,35 @@ run_put_vport_binding(struct ovsdb_idl_txn *ovnsb_idl_txn OVS_UNUSED, return; } +if (smap_get(&pb->options, "binding_request_pause")) { +long long int p_time = smap_get_ullong(&pb->options, + "binding_request_pause_ts", 0); +/* Pause duration for this port still relevant, drop this + * binding request, and set vpb->new_record=false to make sure + * that it will be deleted from the list when flushing the list. + */ +if ((p_time + PAUSE_DURATION) > vpb->creation_time) { +vpb->new_record = false; +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); +VLOG_DBG_RL(&rl, + "Virtual lport %s drop binding request port " + "in pause state\n", pb->logical_port); + +return; +} else { +VLOG_INFO("Virtual lport %s binding requests paused " + "for 10 seconds, resume binding requests handling.", The log message here is a bit misleading, since "Virtual lport binding requests paused for 10 seconds" sounds like you are about to pause the requests for 10 seconds, not that the requests have been paused for 10 seconds already. In addition, saying "10 seconds" here is not a great idea since the PAUSE_DURATION could change. I think the message could just be "Virtual lport %s binding requests resumed". It's more succinct this way and doesn't try to refer to any potential values that could change. + pb->logical_port); +struct smap options; +smap_clone(&options, &pb->options); +smap_remove(&options, "binding_request_pause"); +smap_remove(&options, "binding_request_pause_ts"); +sbrec_port_binding_set_options(pb, &options); +smap_destroy(&options); + +} +} + /* pinctrl module updates the port binding only for type 'virtual'. */ if (!strcmp(pb->type, "virtual")) { const struct sbrec_port_binding *parent = lport_lookup_by_key( @@ -7187,7 +7221,7 @@ run_put_vport_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn, return; } -const struct put_vport_binding *vpb; +struct put_vport_binding *vpb; HMAP_FOR_EACH (vpb, hmap_node, &put_vport_bindings) { run_put_vport_binding(ovnsb_idl_txn, sbrec_datapath_binding_by_key, sbrec_port_binding_by_key, chassis, vpb); @@ -7232,6 +7266,7 @@ pinctrl_handle_bind_vport( vpb->vport_key = vport_key; vpb->vport_parent_key = vport_parent_key; vpb->new_record = true; +vpb->creation_time = time_msec(); Like with patch 2, since this is being synced with a timestamp from another machine, this should use time_wall_msec() instead of time_msec(). notify_pinctrl_main(); } diff --git a/tests/ovn.at b/tests/ovn.at index b31afbfb3..408505ee9 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -22442,6 +22442,97 @@ OVN_CLEANUP([hv1], [hv2]) AT_CLEANUP ]) +# Create 10 HV's each have 3 VIF ports that all +# sends Garp at the same time to bind vport sw0-vir +# this will create high pressure on SB/North and will +# lead to a transaction dropping by SB. +# +# Northd must be able to detect such cases and pause +# binding requests for this specific port for a certain +# amoun
Re: [ovs-dev] [PATCH ovn] tests: Prevent netcat from forking.
I pushed this to main and branch-24.09. Thanks Ales! On 8/27/24 10:28, Ales Musil wrote: Netcat is able to handle multiple connections when "-k, --keep-open" "-e command, --exec command" by forking the netcat. Prevent this behavior by allowing only one connection at a time. This also prevents the test getting stuck on cleanup as the netcat PID wouldn't be added to the cleanup script. Fixes: d15b12da6fe6 ("northd: Add ACL Sampling.") Signed-off-by: Ales Musil --- tests/system-ovn.at | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 0831a2108..78020ecda 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -13169,12 +13169,12 @@ dnl And wait for it to be up and running. OVS_WAIT_UNTIL([ovs-ofctl dump-ipfix-flow br-int | grep -q '1 ids']) dnl Start UDP echo server on vm2. -NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l 1000], [nc-vm2-1000.pid]) -NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l 1010], [nc-vm2-1010.pid]) -NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l 2000], [nc-vm2-2000.pid]) -NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l 2010], [nc-vm2-2010.pid]) -NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l 3000], [nc-vm2-3000.pid]) -NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l 3010], [nc-vm2-3010.pid]) +NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l -m 1 1000], [nc-vm2-1000.pid]) +NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l -m 1 1010], [nc-vm2-1010.pid]) +NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l -m 1 2000], [nc-vm2-2000.pid]) +NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l -m 1 2010], [nc-vm2-2010.pid]) +NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l -m 1 3000], [nc-vm2-3000.pid]) +NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l -m 1 3010], [nc-vm2-3010.pid]) dnl Send traffic (2 packets) to the UDP LB1 (hits the from-lport ACL). NS_CHECK_EXEC([vm1], [(echo a; sleep 1; echo a) | nc --send-only -u 43.43.43.43 1000]) @@ -13337,7 +13337,7 @@ dnl And wait for it to be up and running. OVS_WAIT_UNTIL([ovs-ofctl dump-ipfix-flow br-int | grep -q '1 ids']) dnl Start UDP echo server on vm2. -NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l 1000], [nc-vm2-1000.pid]) +NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l -m 1 1000], [nc-vm2-1000.pid]) dnl Send traffic to the UDP server (hits both ACL tiers). NS_CHECK_EXEC([vm1], [echo a | nc --send-only -u 42.42.42.3 1000]) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] tests: Prevent netcat from forking.
Thanks Ales. Acked-by: Mark Michelson On 8/27/24 10:28, Ales Musil wrote: Netcat is able to handle multiple connections when "-k, --keep-open" "-e command, --exec command" by forking the netcat. Prevent this behavior by allowing only one connection at a time. This also prevents the test getting stuck on cleanup as the netcat PID wouldn't be added to the cleanup script. Fixes: d15b12da6fe6 ("northd: Add ACL Sampling.") Signed-off-by: Ales Musil --- tests/system-ovn.at | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 0831a2108..78020ecda 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -13169,12 +13169,12 @@ dnl And wait for it to be up and running. OVS_WAIT_UNTIL([ovs-ofctl dump-ipfix-flow br-int | grep -q '1 ids']) dnl Start UDP echo server on vm2. -NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l 1000], [nc-vm2-1000.pid]) -NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l 1010], [nc-vm2-1010.pid]) -NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l 2000], [nc-vm2-2000.pid]) -NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l 2010], [nc-vm2-2010.pid]) -NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l 3000], [nc-vm2-3000.pid]) -NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l 3010], [nc-vm2-3010.pid]) +NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l -m 1 1000], [nc-vm2-1000.pid]) +NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l -m 1 1010], [nc-vm2-1010.pid]) +NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l -m 1 2000], [nc-vm2-2000.pid]) +NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l -m 1 2010], [nc-vm2-2010.pid]) +NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l -m 1 3000], [nc-vm2-3000.pid]) +NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l -m 1 3010], [nc-vm2-3010.pid]) dnl Send traffic (2 packets) to the UDP LB1 (hits the from-lport ACL). NS_CHECK_EXEC([vm1], [(echo a; sleep 1; echo a) | nc --send-only -u 43.43.43.43 1000]) @@ -13337,7 +13337,7 @@ dnl And wait for it to be up and running. OVS_WAIT_UNTIL([ovs-ofctl dump-ipfix-flow br-int | grep -q '1 ids']) dnl Start UDP echo server on vm2. -NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l 1000], [nc-vm2-1000.pid]) +NETNS_DAEMONIZE([vm2], [nc -e /bin/cat -k -u -v -l -m 1 1000], [nc-vm2-1000.pid]) dnl Send traffic to the UDP server (hits both ACL tiers). NS_CHECK_EXEC([vm1], [echo a | nc --send-only -u 42.42.42.3 1000]) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 4/4] ovn-ic: Fix memory leak.
Hi Xavier, I understand how the patch works; it ensures that we don't try to create the IC port binding if the binding will just end up getting deleted during route_run(). However, the fact this is needed is unintuitive to me. It seems odd that there is a memory leak when deleting the port binding. Does this only happen because the transaction in which the port binding is inserted has not been committed? Could this be considered a bug in the IDL? Is this a problem with any key-value mapping in the database, or is it specifically only for external-ids? My main concerns here are 1) There are probably other places in our code where this sort of sequence could (or does) happen. It may not be quite as easy to stop the initial insertion of the object as it is in this case. 2) Consider what happens when the IC code is updated and we add a new reason to delete the newly-inserted port binding either in route_run() or some other place. Now create_isb_pb() needs to be updated to account for this as well. This seems like something that is easy to miss by both the coder and reviewer. If this could be corrected at the IDL/DB level, that would make much more sense to me. What do you think? On 8/27/24 08:55, Xavier Simonart wrote: Direct leak of 64 byte(s) in 1 object(s) allocated from: 0 0x56f127 in calloc (/root/master-ovn/ic/ovn-ic+0x56f127) 1 0x731de2 in xcalloc__ /root/master-ovn/ovs/lib/util.c:125:31 2 0x731e10 in xzalloc__ /root/master-ovn/ovs/lib/util.c:135:12 3 0x731ed5 in xzalloc /root/master-ovn/ovs/lib/util.c:169:12 4 0x706139 in ovsdb_idl_txn_add_map_op /root/master-ovn/ovs/lib/ovsdb-idl.c:4178:29 5 0x705ff8 in ovsdb_idl_txn_write_partial_map /root/master-ovn/ovs/lib/ovsdb-idl.c:4335:5 6 0x5b2f67 in update_isb_pb_external_ids /root/master-ovn/ic/ovn-ic.c:588:5 7 0x5ab08e in create_isb_pb /root/master-ovn/ic/ovn-ic.c:733:5 8 0x5ab08e in port_binding_run /root/master-ovn/ic/ovn-ic.c:821:21 9 0x5ab08e in ovn_db_run /root/master-ovn/ic/ovn-ic.c:1901:5 10 0x5a8143 in main /root/master-ovn/ic/ovn-ic.c:2337:21 11 0x7f58bd0f9b74 in __libc_start_main (/lib64/libc.so.6+0x27b74) This happens when, in a single transaction, we run something like - isb_pb = icsbrec_port_binding_insert(ctx->ovnisb_txn); - icsbrec_port_binding_update_external_ids_setkey(isb_pb, "router-id",uuid_s); - icsbrec_port_binding_delete(isb_pb) Signed-off-by: Xavier Simonart --- ic/ovn-ic.c | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c index 18d1df039..dfefa538e 100644 --- a/ic/ovn-ic.c +++ b/ic/ovn-ic.c @@ -121,6 +121,9 @@ Options:\n\ stream_usage("database", true, true, false); } +static const char * +get_lrp_name_by_ts_port_name(struct ic_context *ctx, const char *ts_port_name); + static const struct icsbrec_availability_zone * az_run(struct ic_context *ctx) { @@ -710,13 +713,19 @@ create_isb_pb(struct ic_context *ctx, const char *ts_name, uint32_t pb_tnl_key) { +if (!get_lrp_name_by_ts_port_name(ctx, sb_pb->logical_port)) { +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); +VLOG_WARN_RL(&rl, "ignoring port %s on ts %s because " + "logical router port is not found in NB.", + sb_pb->logical_port, ts_name); +return; +} const struct icsbrec_port_binding *isb_pb = icsbrec_port_binding_insert(ctx->ovnisb_txn); icsbrec_port_binding_set_availability_zone(isb_pb, az); icsbrec_port_binding_set_transit_switch(isb_pb, ts_name); icsbrec_port_binding_set_logical_port(isb_pb, sb_pb->logical_port); icsbrec_port_binding_set_tunnel_key(isb_pb, pb_tnl_key); - const char *address = get_lrp_address_for_sb_pb(ctx, sb_pb); if (address) { icsbrec_port_binding_set_address(isb_pb, address); ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Documentation: Add inclusive-language documentation.
On 8/21/24 04:51, Simon Horman wrote: On Tue, Aug 20, 2024 at 02:03:39PM -0400, Mark Michelson wrote: A recent series of commits to OVN made some changes to the language used in code, comments, and documentation to be more inclusive. This constitutes a follow-up that clarifies in the documentation what the policy is and where the list of words to avoid can be found. When starting this task, I found a commit to OVS [1] written by Simon Horman that added basically everything I wanted to say. With Simon's permission, I have copied the contents of that commit, changing all instances of "Open vSwitch" to "OVN". Simon is credited as a co-author on this commit. [1] https://github.com/openvswitch/ovs/commit/df5e5cf4318a Signed-off-by: Mark Michelson Co-authored-by: Simon Horman FWIIW, Acked-by: Simon Horman Thank you Simon, both for your initial contribution and your review. I pushed this to main, branch-24.09, branch-24.03, and branch-23.09. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 ovn 0/3] Revert ECMP_Nexthop monitor support
Hi Lorenzo, thanks for the follow-up patches. From my perspective, Acked-by: Mark Michelson On 8/26/24 10:14, Lorenzo Bianconi wrote: Revert current implementation of ECMP_Nexthop monitor feature since it can trigger over-flush of CT entries committed by incoming traffic. In particular, at the moment there is no way to link the next-hop IP address (so the ct-entry ID) to the incoming traffic identified by the ethernet source mac address and so to the ct_lablel committed by the following logical flow: table=10(lr_in_ecmp_stateful), priority=100 , match=(inport == "R1_ext" && ip4.dst == 10.0.0.0/24 && (ct.new || ct.est)), action=(ct_commit { ct_label.ecmp_reply_eth = eth.src; ct_mark.ecmp_reply_port = 2; ct_label.label = 1; }; next;) Remove the feature while working on an improved approach. Changes in v3: - rebase on top of ovn main branch Changes in v2: - Bump ovn-sb.ovsschema version to 20.37.0. Lorenzo Bianconi (3): Revert "ofctrl: Introduce ecmp_nexthop_monitor." Revert "northd: Add nexhop id in ct_label.label." Revert "northd: Introduce ECMP_Nexthop table in SB db." controller/ofctrl.c | 54 --- controller/ofctrl.h | 2 - controller/ovn-controller.c | 2 - lib/ovn-util.h | 2 - northd/en-lflow.c | 3 -- northd/en-northd.c | 35 northd/en-northd.h | 4 -- northd/inc-proc-northd.c| 8 +-- northd/northd.c | 105 northd/northd.h | 11 ovn-sb.ovsschema| 18 +-- ovn-sb.xml | 31 --- tests/ovn-northd.at | 4 -- tests/ovn.at| 4 +- tests/system-ovn.at | 79 ++- 15 files changed, 44 insertions(+), 318 deletions(-) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] checkpatch: Add check for non-inclusive language.
On 8/26/24 08:43, Ilya Maximets wrote: On 8/21/24 22:31, Mark Michelson wrote: This takes the approach of hard-coding the words in a text file, rather than trying to use a dynamic list of words. The inclusive naming project provides a way to download their word list as JSON, but this is avoided here for a few reasons: * Only the base form of words are provided. We would need to analyze the part of speech and try to generate the other forms of the words. * Some entries are more "example" than anything. For example, they provide "blackhat-whitehat" as a single entry, even though you're much more likely to come across the individual words, rather than this specific hyphenated variety. Similarly, "whitelist" is provided in the word list but "blacklist" is not. If it turns out that the word list updates frequently, then it may be worth moving to a more dynamic approach, at the expense of accuracy. For now, this seems like a nice approach. We do not consider this an error in checkpatch, but a warning. There are some cases where non-inclusive words are acceptable, (such as ovs_abort(), or referring to the "master" branch of a third-party repo). This is why the warning only suggests to consider an alternative. Hi, Mark. I'm not sure this a right thing to do. The point of avoiding non-inclusive language is to not have instances of it in the repo. But this change explicitly adds all the non-inclusive words to the repo and that IMO contradicts the goal. My 2c. Best regards, Ilya Maximets. Your point is 100% valid. As a follow-up, do you have any ideas for how we can add automated checks for non-inclusive language without listing offending words in our repo? I mentioned in the commit message the shortcomings of attempting a dynamic approach, but maybe there's some middle ground between fully-dynamic and a static list of words that I'm not thinking of. Thanks, Mark Michelson ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] controller: Add bfd_chassis engine.
Thanks for the patch Max, it looks good to me. Acked-by: Mark Michelson On 8/22/24 02:37, Max Lamprecht via dev wrote: This adds a new engine node "bfd_chassis" which handles changes on the Sb_ha_chassis_group table. This improves heavily performance at scale. Without that patch we are permanently reconciling bfd_run and the ovn-controller is capped at 100% cpu usage. Especially on gateway chassis. 3000 HA_Chassis_Group (many ref_chassis) 1500 Chassis performance trace: - 98.86% main - 89.55% bfd_run - 89.23% bfd_calculate_chassis - 28.07% sset_add__ - 27.86% sset_add - 18.20% smap_get_bool - 13.58% sset_destroy Signed-off-by: Max Lamprecht Co-authored-by: Ihtisham ul Haq Signed-off-by: Ihtisham ul Haq Co-authored-by: Maxim Korezkij Signed-off-by: Maxim Korezkij --- controller/bfd.c| 4 ++- controller/ovn-controller.c | 66 + 2 files changed, 62 insertions(+), 8 deletions(-) diff --git a/controller/bfd.c b/controller/bfd.c index 22a8c6695..c91fa0b13 100644 --- a/controller/bfd.c +++ b/controller/bfd.c @@ -154,7 +154,9 @@ bfd_calculate_chassis( if (smap_get_bool(&ref_ch->other_config, "is-remote", false)) { continue; } -sset_add(&grp_chassis, ref_ch->name); +/* we have bfd_setup_required == true anyway, so we skip adding + * it to an sset that we later move to another sset again. */ +sset_add(bfd_chassis, ref_ch->name); } } else { /* This is not an HA chassis. Check if this chassis is present diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 854d80bdf..9fc5f4a0f 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -841,6 +841,7 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) #define SB_NODES \ SB_NODE(sb_global, "sb_global") \ SB_NODE(chassis, "chassis") \ +SB_NODE(ha_chassis_group, "ha_chassis_group") \ SB_NODE(encap, "encap") \ SB_NODE(address_set, "address_set") \ SB_NODE(port_group, "port_group") \ @@ -3290,6 +3291,56 @@ en_mac_cache_cleanup(void *data) hmap_destroy(&cache_data->fdbs); } +static void * +en_bfd_chassis_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED) +{ +return NULL; +} + +static void +en_bfd_chassis_run(struct engine_node *node, void *data OVS_UNUSED) +{ + + +const struct ovsrec_open_vswitch_table *ovs_table = +EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); + + +const struct ovsrec_bridge_table *bridge_table = +EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); +const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table); +const char *chassis_id = get_ovs_chassis_id(ovs_table); +const struct sbrec_ha_chassis_group_table *ha_chassis_grp_table = +EN_OVSDB_GET(engine_get_input("SB_ha_chassis_group", node)); + + + +struct ovsdb_idl_index *sbrec_chassis_by_name = +engine_ovsdb_node_get_index( +engine_get_input("SB_chassis", node), +"name"); +const struct sbrec_chassis *chassis += chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); +ovs_assert(chassis); +const struct sbrec_sb_global_table *sb_global_table = +EN_OVSDB_GET(engine_get_input("SB_sb_global", node)); + +const struct ovsrec_interface_table *iface_table = +EN_OVSDB_GET(engine_get_input("OVS_interface", node)); +stopwatch_start(BFD_RUN_STOPWATCH_NAME, time_msec()); +bfd_run(iface_table, +br_int, chassis, +ha_chassis_grp_table, +sb_global_table); +stopwatch_stop(BFD_RUN_STOPWATCH_NAME, time_msec()); + +engine_set_node_state(node, EN_UPDATED); +} + +static void +en_bfd_chassis_cleanup(void *data OVS_UNUSED){} + /* Engine node which is used to handle the Non VIF data like * - OVS patch ports * - Tunnel ports and the related chassis information. @@ -5025,6 +5076,7 @@ main(int argc, char *argv[]) ENGINE_NODE(if_status_mgr, "if_status_mgr"); ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lb_data, "lb_data"); ENGINE_NODE(mac_cache, "mac_cache"); +ENGINE_NODE(bfd_chassis, "bfd_chassis"); #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); SB_NODES @@ -5064,6 +5116,12 @@ main(int argc, char *argv[]) engine_add_input(&en_if_status_mgr, &en_ovs_interface, if_status_mgr_ovs_interface_handler); +engine_add_input(&en_bfd_chassis, &en_ovs_open_vswitch, NULL); +engine_add_input(&en_bfd_chassis, &en
Re: [ovs-dev] [PATCH ovn 3/3] Revert "northd: Introduce ECMP_Nexthop table in SB db."
Hi Lorenzo, I have one complaint below, otherwise the series looks good to me. On 8/22/24 10:30, Lorenzo Bianconi wrote: This reverts commit aeae21335a8bccbb9fe7746fcf4ed2d2a0e1c7a4. Signed-off-by: Lorenzo Bianconi --- lib/ovn-util.h | 2 -- northd/en-northd.c | 35 northd/en-northd.h | 4 --- northd/inc-proc-northd.c | 7 +--- northd/northd.c | 70 northd/northd.h | 10 -- ovn-sb.ovsschema | 18 ++- ovn-sb.xml | 31 -- tests/ovn-northd.at | 4 --- 9 files changed, 3 insertions(+), 178 deletions(-) diff --git a/lib/ovn-util.h b/lib/ovn-util.h index 622fec531..7b98b9b9a 100644 --- a/lib/ovn-util.h +++ b/lib/ovn-util.h @@ -38,8 +38,6 @@ #define STT_TUNNEL_OVERHEAD 18 #define VXLAN_TUNNEL_OVERHEAD 30 -#define ECMP_NEXTHOP_IDS_LEN 65535 - struct eth_addr; struct nbrec_logical_router_port; struct ovsrec_flow_sample_collector_set_table; diff --git a/northd/en-northd.c b/northd/en-northd.c index 63f93bbf4..34f0a7df7 100644 --- a/northd/en-northd.c +++ b/northd/en-northd.c @@ -404,25 +404,6 @@ en_bfd_sync_run(struct engine_node *node, void *data) engine_set_node_state(node, EN_UPDATED); } -void -en_ecmp_nexthop_run(struct engine_node *node, void *data) -{ -const struct engine_context *eng_ctx = engine_get_context(); -struct static_routes_data *static_routes_data = -engine_get_input_data("static_routes", node); -struct ecmp_nexthop_data *enh_data = data; -const struct sbrec_ecmp_nexthop_table *sbrec_ecmp_nexthop_table = -EN_OVSDB_GET(engine_get_input("SB_ecmp_nexthop", node)); - -ecmp_nexthop_destroy(data); -ecmp_nexthop_init(data); -build_ecmp_nexthop_table(eng_ctx->ovnsb_idl_txn, - &static_routes_data->parsed_routes, - &enh_data->nexthops, - sbrec_ecmp_nexthop_table); -engine_set_node_state(node, EN_UPDATED); -} - void *en_northd_init(struct engine_node *node OVS_UNUSED, struct engine_arg *arg OVS_UNUSED) @@ -473,16 +454,6 @@ void return data; } -void -*en_ecmp_nexthop_init(struct engine_node *node OVS_UNUSED, - struct engine_arg *arg OVS_UNUSED) -{ -struct ecmp_nexthop_data *data = xzalloc(sizeof *data); - -ecmp_nexthop_init(data); -return data; -} - void en_northd_cleanup(void *data) { @@ -555,9 +526,3 @@ en_bfd_sync_cleanup(void *data) { bfd_destroy(data); } - -void -en_ecmp_nexthop_cleanup(void *data) -{ -ecmp_nexthop_destroy(data); -} diff --git a/northd/en-northd.h b/northd/en-northd.h index 2666cc67e..631a7c17a 100644 --- a/northd/en-northd.h +++ b/northd/en-northd.h @@ -42,9 +42,5 @@ bool bfd_sync_northd_change_handler(struct engine_node *node, void *data OVS_UNUSED); void en_bfd_sync_run(struct engine_node *node, void *data); void en_bfd_sync_cleanup(void *data OVS_UNUSED); -void en_ecmp_nexthop_run(struct engine_node *node, void *data); -void *en_ecmp_nexthop_init(struct engine_node *node OVS_UNUSED, - struct engine_arg *arg OVS_UNUSED); -void en_ecmp_nexthop_cleanup(void *data); #endif /* EN_NORTHD_H */ diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 06c7ee2b8..1f79916a5 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -103,8 +103,7 @@ static unixctl_cb_func chassis_features_list; SB_NODE(fdb, "fdb") \ SB_NODE(static_mac_binding, "static_mac_binding") \ SB_NODE(chassis_template_var, "chassis_template_var") \ -SB_NODE(logical_dp_group, "logical_dp_group") \ -SB_NODE(ecmp_nexthop, "ecmp_nexthop") +SB_NODE(logical_dp_group, "logical_dp_group") enum sb_engine_node { #define SB_NODE(NAME, NAME_STR) SB_##NAME, @@ -163,7 +162,6 @@ static ENGINE_NODE(route_policies, "route_policies"); static ENGINE_NODE(static_routes, "static_routes"); static ENGINE_NODE(bfd, "bfd"); static ENGINE_NODE(bfd_sync, "bfd_sync"); -static ENGINE_NODE(ecmp_nexthop, "ecmp_nexthop"); void inc_proc_northd_init(struct ovsdb_idl_loop *nb, struct ovsdb_idl_loop *sb) @@ -266,9 +264,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_bfd_sync, &en_route_policies, NULL); engine_add_input(&en_bfd_sync, &en_northd, bfd_sync_northd_change_handler); -engine_add_input(&en_ecmp_nexthop, &en_sb_ecmp_nexthop, NULL); -engine_add_input(&en_ecmp_nexthop, &en_static_routes, NULL); - engine_add_input(&en_sync_meters, &en_nb_acl, NULL); engine_add_input(&en_sync_meters, &en_nb_meter, NULL); engine_add_input(&en_sync_meters, &en_sb_meter, NULL); diff --git a/northd/northd.c b/northd/northd.c index 51ee5db1d..fb18fec90 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -10670,
[ovs-dev] [PATCH ovn] checkpatch: Add check for non-inclusive language.
This takes the approach of hard-coding the words in a text file, rather than trying to use a dynamic list of words. The inclusive naming project provides a way to download their word list as JSON, but this is avoided here for a few reasons: * Only the base form of words are provided. We would need to analyze the part of speech and try to generate the other forms of the words. * Some entries are more "example" than anything. For example, they provide "blackhat-whitehat" as a single entry, even though you're much more likely to come across the individual words, rather than this specific hyphenated variety. Similarly, "whitelist" is provided in the word list but "blacklist" is not. If it turns out that the word list updates frequently, then it may be worth moving to a more dynamic approach, at the expense of accuracy. For now, this seems like a nice approach. We do not consider this an error in checkpatch, but a warning. There are some cases where non-inclusive words are acceptable, (such as ovs_abort(), or referring to the "master" branch of a third-party repo). This is why the warning only suggests to consider an alternative. On a side note, running this patch through the updated checkpatch.py is pretty funny. Signed-off-by: Mark Michelson --- tests/checkpatch.at | 38 utilities/checkpatch.py | 30 utilities/excluded_word_list.txt | 59 3 files changed, 127 insertions(+) create mode 100644 utilities/excluded_word_list.txt diff --git a/tests/checkpatch.at b/tests/checkpatch.at index 6ac0e51f3..79c02b229 100755 --- a/tests/checkpatch.at +++ b/tests/checkpatch.at @@ -642,5 +642,43 @@ try_checkpatch \ #8 FILE: tests/something.at:1: C_H_E_C_K([as gw1 ovs-ofctl dump-flows br-int table=42 | grep "dl_dst=00:00:02:01:02:04" | wc -l], [0], [[1]]) " +AT_CLEANUP + +AT_SETUP([checkpatch - non-inclusive words]) +# This test does not extensively test every single word in the list of +# non-inclusive words. + +# Try a simple exact match with a single word +try_checkpatch \ + "COMMON_PATCH_HEADER ++/* Let's be sure this change doesn't cripple our performance */ +" \ +"WARNING: Non-inclusive word 'cripple' found. Consider replacing. +#8 FILE: A.c:1: +/* Let's be sure this change doesn't cripple our performance */ +" +# Put more than one word on the line, and use different forms of the word. +try_checkpatch \ + "COMMON_PATCH_HEADER ++/* The grandfathers are hallucinating again */ +" \ +"WARNING: Non-inclusive word 'grandfathers' found. Consider replacing. +WARNING: Non-inclusive word 'hallucinating' found. Consider replacing. +#8 FILE: A.c:1: +/* The grandfathers are hallucinating again */ +" + +# And finally, make sure punctuation, etc. don't interfere. +try_checkpatch \ + "COMMON_PATCH_HEADER ++/* Set up master/slave tribe, but don't abort! */ +" \ +"WARNING: Non-inclusive word 'abort' found. Consider replacing. +WARNING: Non-inclusive word 'master' found. Consider replacing. +WARNING: Non-inclusive word 'slave' found. Consider replacing. +WARNING: Non-inclusive word 'tribe' found. Consider replacing. +#8 FILE: A.c:1: +/* Set up master/slave tribe, but don't abort! */ +" AT_CLEANUP diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py index 35204daa2..9a06cf0a1 100755 --- a/utilities/checkpatch.py +++ b/utilities/checkpatch.py @@ -19,6 +19,8 @@ import getopt import os import re import sys +import functools +from pathlib import Path RETURN_CHECK_INITIAL_STATE = 0 RETURN_CHECK_STATE_WITH_RETURN = 1 @@ -582,6 +584,32 @@ def empty_return_with_brace(line): return False +@functools.cache +def load_excluded_words(): +parent_dir = Path(__file__).parent +with open(parent_dir / "excluded_word_list.txt", "r") as f: +return [line.strip() for line in f] + + +def contains_non_inclusive_words(line): +# This returns true if a word is found that falls afoul of our inclusive +# language guidelines. The list of words is sourced from the Tier 1, Tier 2, +# and Tier 3 word lists from https://inclusivenaming.org/word-lists/ . + +excluded_words = load_excluded_words() + +problem_found = False +for word in excluded_words: +match = re.search(rf'\b{word}\b', line, flags=re.IGNORECASE) +if match: +print_warning( +f"Non-inclusive word '{word}' found. Consider replacing." +) +problem_found = True + +return problem_found + + file_checks = [ {'regex': __regex_added_doc_rst, 'check': chec
[ovs-dev] [PATCH ovn] Documentation: Add inclusive-language documentation.
A recent series of commits to OVN made some changes to the language used in code, comments, and documentation to be more inclusive. This constitutes a follow-up that clarifies in the documentation what the policy is and where the list of words to avoid can be found. When starting this task, I found a commit to OVS [1] written by Simon Horman that added basically everything I wanted to say. With Simon's permission, I have copied the contents of that commit, changing all instances of "Open vSwitch" to "OVN". Simon is credited as a co-author on this commit. [1] https://github.com/openvswitch/ovs/commit/df5e5cf4318a Signed-off-by: Mark Michelson Co-authored-by: Simon Horman --- Documentation/automake.mk | 1 + Documentation/index.rst | 1 + .../contributing/inclusive-language.rst | 57 +++ .../internals/contributing/index.rst | 1 + 4 files changed, 60 insertions(+) create mode 100644 Documentation/internals/contributing/inclusive-language.rst diff --git a/Documentation/automake.mk b/Documentation/automake.mk index c6cc37e49..5f7500fb7 100644 --- a/Documentation/automake.mk +++ b/Documentation/automake.mk @@ -56,6 +56,7 @@ DOC_SOURCE = \ Documentation/internals/security.rst \ Documentation/internals/contributing/index.rst \ Documentation/internals/contributing/backporting-patches.rst \ + Documentation/internals/contributing/inclusive-language.rst \ Documentation/internals/contributing/coding-style.rst \ Documentation/internals/contributing/documentation-style.rst \ Documentation/internals/contributing/submitting-patches.rst \ diff --git a/Documentation/index.rst b/Documentation/index.rst index 04e757505..9fb298c28 100644 --- a/Documentation/index.rst +++ b/Documentation/index.rst @@ -81,6 +81,7 @@ Learn more about the Open Virtual Network (OVN) project and about how you can co - **Contributing:** :doc:`internals/contributing/submitting-patches` | :doc:`internals/contributing/backporting-patches` | + :doc:`internals/contributing/inclusive-language` | :doc:`internals/contributing/coding-style` - **Maintaining:** :doc:`internals/maintainers` | diff --git a/Documentation/internals/contributing/inclusive-language.rst b/Documentation/internals/contributing/inclusive-language.rst new file mode 100644 index 0..65e9c4fbd --- /dev/null +++ b/Documentation/internals/contributing/inclusive-language.rst @@ -0,0 +1,57 @@ +.. + 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. + + Convention for heading levels in OVN documentation: + + === Heading 0 (reserved for the title in a document) + --- Heading 1 + ~~~ Heading 2 + +++ Heading 3 + ''''''' Heading 4 + + Avoid deeper levels because they do not render well. + +== +Inclusive Language +== + +In order to help facilitate an inclusive environment in the OVN +community we recognise the role of language in framing our +communication with each other. It is important that terms that +may exclude people through racial, cultural or other bias, are avoided +as they may make people feel excluded. + +We recognise that this is subjective, and to some extent is a journey. +But we also recognise that we cannot begin that journey without taking +positive action. To this end OVN is adopting the practice of an +inclusive word list, which helps to guide the use of language within +the project. + +.. _word list: + +Word List +- + +The intent of this document is to formally document the acceptance of a +inclusive word list by OVN. Accordingly, this document specifies +use of the use the `Inclusive Naming Word List +<https://inclusivenaming.org/word-lists/>`__ v1.0 (the word list) for +OVN. + +The adoption of the word list intended that this act as a guide for +developers creating patches to the OVN repository, including both +source code and documentation. And to aid maintainers in their role of +shepherding changes into the repository. + +Further steps to align usage of language in OVN, including clarification +of application of the word list, to new and existing work, may follow. diff --git a/Documentation/internals/contributing/index.rst b/Documentation/internals/contributing/index.rst index ba6b6094e..9dab48110 100
[ovs-dev] [PATCH ovn branch-24.03 2/2] Prepare for 24.03.4.
Signed-off-by: Mark Michelson --- NEWS | 3 +++ configure.ac | 2 +- debian/changelog | 6 ++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index faadba24c..c7f2c5833 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,6 @@ +OVN v24.03.4 - xx xxx +-- + OVN v24.03.3 - 15 Aug 2024 -- - Bug fixes diff --git a/configure.ac b/configure.ac index 688fcd6f9..6a05ba90b 100644 --- a/configure.ac +++ b/configure.ac @@ -13,7 +13,7 @@ # limitations under the License. AC_PREREQ(2.63) -AC_INIT(ovn, 24.03.3, b...@openvswitch.org) +AC_INIT(ovn, 24.03.4, b...@openvswitch.org) AC_CONFIG_MACRO_DIR([m4]) AC_CONFIG_AUX_DIR([build-aux]) AC_CONFIG_HEADERS([config.h]) diff --git a/debian/changelog b/debian/changelog index 18c301c0b..1341f950d 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +OVN (24.03.4-1) unstable; urgency=low + [ OVN team ] + * New upstream version + + -- OVN team Thu, 15 Aug 2024 15:10:20 -0400 + ovn (24.03.3-1) unstable; urgency=low [ OVN team ] * New upstream version -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-24.03 1/2] Set release date for 24.03.3.
Signed-off-by: Mark Michelson --- NEWS | 3 ++- debian/changelog | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index 9e4a52f46..faadba24c 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,6 @@ -OVN v24.03.3 - xx xxx +OVN v24.03.3 - 15 Aug 2024 -- + - Bug fixes OVN v24.03.2 - 09 May 2024 -- diff --git a/debian/changelog b/debian/changelog index 6b58383f4..18c301c0b 100644 --- a/debian/changelog +++ b/debian/changelog @@ -2,7 +2,7 @@ ovn (24.03.3-1) unstable; urgency=low [ OVN team ] * New upstream version - -- OVN team Thu, 09 May 2024 15:10:30 -0400 + -- OVN team Thu, 15 Aug 2024 15:10:20 -0400 ovn (24.03.2-1) unstable; urgency=low [ OVN team ] -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-23.06 2/2] Prepare for 23.06.6.
Signed-off-by: Mark Michelson --- NEWS | 3 +++ configure.ac | 2 +- debian/changelog | 6 ++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 62db12415..ff0a01842 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,6 @@ +OVN v23.06.6 - xx xxx +-- + OVN v23.06.5 - 15 Aug 2024 -- - Bug fixes diff --git a/configure.ac b/configure.ac index fda6ff4af..341d0423e 100644 --- a/configure.ac +++ b/configure.ac @@ -13,7 +13,7 @@ # limitations under the License. AC_PREREQ(2.63) -AC_INIT(ovn, 23.06.5, b...@openvswitch.org) +AC_INIT(ovn, 23.06.6, b...@openvswitch.org) AC_CONFIG_MACRO_DIR([m4]) AC_CONFIG_AUX_DIR([build-aux]) AC_CONFIG_HEADERS([config.h]) diff --git a/debian/changelog b/debian/changelog index d03cad6d3..70c09c618 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +OVN (23.06.6-1) unstable; urgency=low + [ OVN team ] + * New upstream version + + -- OVN team Thu, 15 Aug 2024 15:07:30 -0400 + OVN (23.06.5-1) unstable; urgency=low [ OVN team ] * New upstream version -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-24.03 0/2] Release patches for v24.03.3.
Mark Michelson (2): Set release date for 24.03.3. Prepare for 24.03.4. NEWS | 6 +- configure.ac | 2 +- debian/changelog | 8 +++- 3 files changed, 13 insertions(+), 3 deletions(-) -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-23.09 1/2] Set release date for 23.09.5.
Signed-off-by: Mark Michelson --- NEWS | 3 ++- debian/changelog | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index 472594d16..52ad9bdbf 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,6 @@ -OVN v23.09.5 - xx xxx +OVN v23.09.5 - 15 Aug 2024 -- + - Bug fixes OVN v23.09.4 - 09 May 2024 -- diff --git a/debian/changelog b/debian/changelog index b19d71513..33c7db3f1 100644 --- a/debian/changelog +++ b/debian/changelog @@ -2,7 +2,7 @@ OVN (23.09.5-1) unstable; urgency=low [ OVN team ] * New upstream version - -- OVN team Thu, 09 May 2024 15:10:37 -0400 + -- OVN team Thu, 15 Aug 2024 15:10:16 -0400 OVN (23.09.4-1) unstable; urgency=low [ OVN team ] -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-23.09 2/2] Prepare for 23.09.6.
Signed-off-by: Mark Michelson --- NEWS | 3 +++ configure.ac | 2 +- debian/changelog | 6 ++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 52ad9bdbf..54b2e78db 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,6 @@ +OVN v23.09.6 - xx xxx +-- + OVN v23.09.5 - 15 Aug 2024 -- - Bug fixes diff --git a/configure.ac b/configure.ac index 4d73bf7b3..055d339e3 100644 --- a/configure.ac +++ b/configure.ac @@ -13,7 +13,7 @@ # limitations under the License. AC_PREREQ(2.63) -AC_INIT(ovn, 23.09.5, b...@openvswitch.org) +AC_INIT(ovn, 23.09.6, b...@openvswitch.org) AC_CONFIG_MACRO_DIR([m4]) AC_CONFIG_AUX_DIR([build-aux]) AC_CONFIG_HEADERS([config.h]) diff --git a/debian/changelog b/debian/changelog index 33c7db3f1..c2ec64455 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +OVN (23.09.6-1) unstable; urgency=low + [ OVN team ] + * New upstream version + + -- OVN team Thu, 15 Aug 2024 15:10:16 -0400 + OVN (23.09.5-1) unstable; urgency=low [ OVN team ] * New upstream version -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-23.09 0/2] Release patches for v23.09.5.
Mark Michelson (2): Set release date for 23.09.5. Prepare for 23.09.6. NEWS | 6 +- configure.ac | 2 +- debian/changelog | 8 +++- 3 files changed, 13 insertions(+), 3 deletions(-) -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-23.06 1/2] Set release date for 23.06.5.
Signed-off-by: Mark Michelson --- NEWS | 3 ++- debian/changelog | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index 115db073a..62db12415 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,6 @@ -OVN v23.06.5 - xx xxx +OVN v23.06.5 - 15 Aug 2024 -- + - Bug fixes OVN v23.06.4 - 09 May 2024 -- diff --git a/debian/changelog b/debian/changelog index 728f23336..d03cad6d3 100644 --- a/debian/changelog +++ b/debian/changelog @@ -2,7 +2,7 @@ OVN (23.06.5-1) unstable; urgency=low [ OVN team ] * New upstream version - -- OVN team Thu, 09 May 2024 15:10:42 -0400 + -- OVN team Thu, 15 Aug 2024 15:07:30 -0400 OVN (23.06.4-1) unstable; urgency=low [ OVN team ] -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn branch-23.06 0/2] Release patches for v23.06.5.
Mark Michelson (2): Set release date for 23.06.5. Prepare for 23.06.6. NEWS | 6 +- configure.ac | 2 +- debian/changelog | 8 +++- 3 files changed, 13 insertions(+), 3 deletions(-) -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] Support selection fields for ECMP routes.
Hi Karthik, thanks for the patch. The protocol items introduced in v2 are interesting. It highlights that there are inherent flaws in allowing L4 selection fields to be used in what is intended to be a L3 feature. IMO, there shouldn't be a need to discriminate based on L4 protocol when trying to route packets. The current patch does some things I don't particularly like with regards to L4 protocol: * The number of ECMP flows has tripled as a result of this patch. * The patch adds undocumented "dst_port" and "src_port" selection fields as options. * The patch has no SCTP support. (and if it did, then the number of ECMP flows would quadruple instead of tripling) I would say that since ECMP is a pure L3 feature, L4 selection fields should not be used at all as part of its hash function. As you show in your patch, you have to add protocol prerequisites to the match in case a L4-protocol-specific selection field is added. How to enforce this is difficult. We could add code to northd that specifically checks for the various L4 selection fields and rejects them if they are present (falling back to the default selection method as a result). Or we could document that L4 selection fields should not be used, but let the user shoot themselves in the foot if they absolutely insist on adding any. One thing that comes to mind for this particular feature is: what is the motivation? Why do you want to be able to hash on specific fields instead of using the default hash? Maybe that'll help me to understand why the L4 protocol considerations are here. I have some additional findings inline below. On 8/13/24 05:29, karthi...@nutanix.com wrote: From: Karthik Chandrashekar - This patch adds the ability to specify a custom set of packet headers for hash computation for ECMP routes similar to the support that was added for LB in 5af304e7478adcf5ac50ed41e96a55bebebff3e8 - ECMP routes by default use dp_hash as a selection_method for OVS flows. When ecmp_selection_fields is specified, the selection_method will be hash with the specified list of fields used for computing the hash. - For simplicity, list of fields that are used in the select action is a union of all the fields specified in each Logical_Route_Static_Route that is part of a given ECMP route. - In order to allow match based on L4 port numbers, the lr_in_ip_routing rules have been split into separate lflows with protocol specific fields when src_port or dst_port is specified in the ecmp_selectioin_fields. (This is based on the requirement that pre-requisites of fields must be provided by any flows that output to the group) Signed-off-by: Karthik Chandrashekar --- v2: - Install separate logical flows for TCP and UDP in lr_in_ip_routing. - Add more test coverage. --- --- include/ovn/actions.h | 1 + lib/actions.c | 55 +- northd/northd.c | 125 +++--- ovn-nb.xml| 17 +++ tests/ovn-northd.at | 42 tests/ovn.at | 242 -- utilities/ovn-nbctl.c | 17 ++- 7 files changed, 450 insertions(+), 49 deletions(-) diff --git a/include/ovn/actions.h b/include/ovn/actions.h index 88cf4de79..a9af1e38e 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -338,6 +338,7 @@ struct ovnact_select { struct ovnact_select_dst *dsts; size_t n_dsts; uint8_t ltable; /* Logical table ID of next table. */ +char *hash_fields; struct expr_field res_field; }; diff --git a/lib/actions.c b/lib/actions.c index 37676ef81..ac5d1dbd5 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -1534,11 +1534,19 @@ parse_select_action(struct action_context *ctx, struct expr_field *res_field) struct ovnact_select_dst *dsts = NULL; size_t allocated_dsts = 0; size_t n_dsts = 0; +bool requires_hash_fields = false; +char *hash_fields = NULL; lexer_get(ctx->lexer); /* Skip "select". */ lexer_get(ctx->lexer); /* Skip '('. */ -while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) { +if (lexer_match_id(ctx->lexer, "values")) { +lexer_force_match(ctx->lexer, LEX_T_EQUALS); +requires_hash_fields = true; +} + +while (!lexer_match(ctx->lexer, LEX_T_SEMICOLON) && + !lexer_match(ctx->lexer, LEX_T_RPAREN)) { This while statement allows for some odd syntax to be accepted. For instance, the following would be allowed: select(values=1=50, 2=100) hash_fields=ip_dst) One way to avoid this is to change which token to look for based on whether require_hash_fields is true. enum lex_type value_end_token = requires_hash_fields ? LEX_T_SEMICOLON : LEX_T_PAREN; while (lexer_match(ctx->lexer, value_end_token) { ... struct ovnact_select_dst dst; if (!action_parse_uint16(ctx, &dst.id, "id")) { free(dsts); @@ -1574,11 +1582,39 @@ parse_select_action(struct action_context *
Re: [ovs-dev] [PATCH ovn 0/2] nft in tests.
I pushed this series to main, branch-24.09, branch-24.03, and branch-23.09. On 8/12/24 08:14, Xavier Simonart wrote: nft was used in some tests, while nft related packages might not be installed. Xavier Simonart (2): tests: Skip some tests if nft not installed. ci: Add nftables to containers. tests/ovn-macros.at| 12 +++- utilities/containers/fedora/Dockerfile | 1 + utilities/containers/ubuntu/Dockerfile | 1 + 3 files changed, 9 insertions(+), 5 deletions(-) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 0/2] ovn-fake-multinode tests.
I merged this series to main and branch-24.09. On 8/12/24 08:13, Xavier Simonart wrote: Xavier Simonart (2): multinode: Increase maximum execution time. multinode: Fix test "ovn multinode NAT ...". .github/workflows/ovn-fake-multinode-tests.yml | 2 +- tests/multinode.at | 4 2 files changed, 5 insertions(+), 1 deletion(-) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2] Documentation: Define experimental features.
During a recent OVN community call, it was questioned what it means for a feature to be marked experimental. This documentation change aims to clarify what it means when a feature is marked experimental. Signed-off-by: Mark Michelson --- v1 -> v2: * Added follow-up question that answers how features are marked xperimental. --- Documentation/faq/general.rst | 54 +++ 1 file changed, 54 insertions(+) diff --git a/Documentation/faq/general.rst b/Documentation/faq/general.rst index 831ca0445..df4952ef5 100644 --- a/Documentation/faq/general.rst +++ b/Documentation/faq/general.rst @@ -119,3 +119,57 @@ Q: How can I contribute to the OVN Community? questions. You can also suggest improvements to documentation. If you have a feature or bug you would like to work on, send a mail to one of the :doc:`mailing lists `. + +Q: What does it mean when a feature is marked "experimental"? + +A: Experimental features are marked this way because of one of +several reasons: + +* The developer was only able to test the feature in a limited + environment. Therefore the feature may not always work as intended + in all environments. + +* During review, the potential for failure was noticed, but the + circumstances that would lead to that failure were hard to nail + down or were strictly theoretical. + +* What exists in OVN may be an early version of a more fleshed-out + feature to come in a later version. + +* The feature was developed against a draft RFC that is subject to + change when the RFC is published. + +* The feature was developed based on observations of how a specific + vendor implements a feature, rather than using IETF standards or + other documentated specifications. + +A feature may be declared experimental for other reasons as well, +but the above are the most common. When a feature is marked +experimental, it has the following properties: + +* The feature must be opt-in. The feature must be disabled by + default. When the feature is disabled, it must have no bearing + on other OVN functionality. + +* Configuration and implementation details of the feature are + subject to change between major or minor versions of OVN. + +* Users make use of this feature at their own risk. Users are free + to file issues against the feature, but developers are more likely + to prioritize work on non-experimental features first. + +* Experimental features may be removed. For instance, if an + experimental feature exposes a security risk, it may be removed + rather than repaired. + +The hope is that experimental features will eventually lose the +"experimental" marker and become a core feature. However, there is +no specific test or process defined for when a feature no longer +needs to be considered experimental. This typically will be decided +collectively by OVN maintainers. + +Q: How is a feature marked "experimental"? + +A: Experimental features must contain the following note in their man +pages (ovn-nb.5, ovn-sb.5, ovn-controller.8, etc): "NOTE: this feature +is experimental and may be subject to removal/change in the future.: -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/2] multinode: Fix test "ovn multinode NAT ...".
Thanks Xavier. Acked-by: Mark Michelson On 8/12/24 08:13, Xavier Simonart wrote: Test was not properly deleting ports created by previous tests, which could cause the test to fail. Fixes: 8d13579bf5b3 ("Add support for centralize routing for distributed gw ports.") Signed-off-by: Xavier Simonart --- tests/multinode.at | 4 1 file changed, 4 insertions(+) diff --git a/tests/multinode.at b/tests/multinode.at index a04ce7597..a0eb8fc67 100644 --- a/tests/multinode.at +++ b/tests/multinode.at @@ -1062,6 +1062,10 @@ check_fake_multinode_setup # Delete the multinode NB and OVS resources before starting the test. cleanup_multinode_resources +m_as ovn-chassis-1 ip link del sw0p1-p +m_as ovn-chassis-2 ip link del sw0p2-p +m_as ovn-chassis-2 ip link del sw1p1-p + check multinode_nbctl ls-add sw0 check multinode_nbctl lsp-add sw0 sw0-port1 check multinode_nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:03 10.0.0.3 1000::3" ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 1/2] multinode: Increase maximum execution time.
Thanks Xavier, Acked-by: Mark Michelson On 8/12/24 08:13, Xavier Simonart wrote: Maximum execution time was set to 15 minutes. Last successfull multinode test, which run 10 tests, took > 12 minutes. Since then, 2 tests got added, so we might hit the 15 minutes maximum execution time. Increase the time to 30 minutes. Signed-off-by: Xavier Simonart --- .github/workflows/ovn-fake-multinode-tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ovn-fake-multinode-tests.yml b/.github/workflows/ovn-fake-multinode-tests.yml index 795dafc22..f3f25ddf3 100644 --- a/.github/workflows/ovn-fake-multinode-tests.yml +++ b/.github/workflows/ovn-fake-multinode-tests.yml @@ -72,7 +72,7 @@ jobs: multinode-tests: runs-on: ubuntu-22.04 -timeout-minutes: 15 +timeout-minutes: 30 needs: [build] strategy: fail-fast: false ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/2] ci: Add nftables to containers.
Thanks Xavier. Acked-by: Mark Michelson On 8/12/24 08:14, Xavier Simonart wrote: Signed-off-by: Xavier Simonart --- utilities/containers/fedora/Dockerfile | 1 + utilities/containers/ubuntu/Dockerfile | 1 + 2 files changed, 2 insertions(+) diff --git a/utilities/containers/fedora/Dockerfile b/utilities/containers/fedora/Dockerfile index 4dce1e32b..02e177a9f 100755 --- a/utilities/containers/fedora/Dockerfile +++ b/utilities/containers/fedora/Dockerfile @@ -28,6 +28,7 @@ RUN dnf -y update \ libtool \ net-tools \ nfdump \ +nftables \ ninja-build \ nmap-ncat \ numactl-devel \ diff --git a/utilities/containers/ubuntu/Dockerfile b/utilities/containers/ubuntu/Dockerfile index 073afa876..8a31fce59 100755 --- a/utilities/containers/ubuntu/Dockerfile +++ b/utilities/containers/ubuntu/Dockerfile @@ -34,6 +34,7 @@ RUN apt update -y \ ncat \ net-tools \ nfdump \ +nftables \ ninja-build \ python3-dev \ python3-pip \ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 1/2] tests: Skip some tests if nft not installed.
Thanks Xavier, Acked-by: Mark Michelson On 8/12/24 08:14, Xavier Simonart wrote: If nft related packages are not installed, stop_ovsdb_controller_updates macro does nothing, and race conditions it is supposed to create won't happen. Skip the tests using stop_ovsdb_controller_updates if nft is not available. Also, fail those tests if nft fails. Fixes: 8d46e542767b ("tests: Add macros to pause controller updates.") Fixes: 39eb73d92a21 ("tests: Remove almost duplicate macros.") Signed-off-by: Xavier Simonart --- tests/ovn-macros.at | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at index 4fd941e55..77d1515f6 100644 --- a/tests/ovn-macros.at +++ b/tests/ovn-macros.at @@ -1109,16 +1109,18 @@ stop_ovsdb_controller_updates() { TCP_PORT=$1 echo Stopping updates from ovn-controller to ovsdb using port $TCP_PORT on_exit 'nft list tables | grep ovn-test && nft delete table ip ovn-test' - nft add table ip ovn-test - nft 'add chain ip ovn-test INPUT { type filter hook input priority 0; policy accept; }' - nft add rule ip ovn-test INPUT tcp dport $TCP_PORT counter drop + # Report the test as skipped if proper nft related packages are not installed. + AT_SKIP_IF([! which nft]) + AT_CHECK([nft add table ip ovn-test]) + AT_CHECK([nft 'add chain ip ovn-test INPUT { type filter hook input priority 0; policy accept; }']) + AT_CHECK([nft add rule ip ovn-test INPUT tcp dport $TCP_PORT counter drop]) } restart_ovsdb_controller_updates() { TCP_PORT=$1 echo Restarting updates from ovn-controller to ovsdb - nft list ruleset | grep $TCP_PORT - nft delete table ip ovn-test + AT_CHECK([nft list ruleset | grep $TCP_PORT], [0], [ignore]) + AT_CHECK([nft delete table ip ovn-test]) } trim_zeros() { ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] OVN branch-24.09 created
Hi everyone, I have just created branch-24.09 for OVN. At this point, with the exception of one specific patch, no new features will be allowed into the branch. We will focus on testing and bug-fixing for the next 5 weeks and plan to release 24.09.0 on 6 September, 2024. The one feature patch that is still a candidate for inclusion is https://patchwork.ozlabs.org/project/ovn/patch/20240809184140.3399288-1-martin.kal...@canonical.com/ (or rather, a follow-up version of the patch will be a candidate). This is because of discussions made by devs about this particular patch, plus the fact that it is an opt-in experimental feature. We plan to get this integrated as soon as possible next week. Thank you to everyone who helped contribute to this version, and I look forward to the upcoming release. Mark Michelson ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [Patch ovn v7] northd: Routing protocol port redirection.
Martin et al., I plan to commit the branch patches and to create the 24.09 branch shortly. Since we stated intent to get this patch into 24.09, we will work to get it into main and branch-24.09 as soon as we can next week. Thanks, Mark Michelson On 8/9/24 14:40, Martin Kalcok wrote: This change adds two new LRP options: * routing-protocol-redirect * routing-protocols These allow redirection of a routing protocol traffic to an Logical Switch Port. This enables external routing daemons to listen on an interface bound to an LSP and effectively act as if they were listening on (and speaking from) LRP's IP address. Option 'routing-protocols' takes a comma-separated list of routing protocols whose traffic should be redirected. Currently supported are BGP (tcp 179) and BFD (udp 3784). Option 'routing-protocol-redirect' expects a string with an LSP name. When both of these options are set, any traffic entering LS that's destined for LRP's IP addresses (including IPv6 LLA) and routing protocol's port number, is redirected to the LSP specified in the 'routing-protocol-redirect' value. NOTE: this feature is experimental and may be subject to removal/change in the future. Signed-off-by: Martin Kalcok --- I'm posting this v7 patch just as a preview. northd occasionally crashes in the added datapath tests. It's an issue I can't seem to reproduce in my test deployment, only during the tests. Until that's resolved, this patch is not suitable for merging. Compared to v6, this patch includes: Fix for cloning of ARP/ND messages. Now the messages are properly delivered to both, redirect port and the LRP. It also adds functional test that verifies ability of hosts on internal networks to communicate with the external networks uninterupted, even when the worwarding is enabled northd/northd.c | 225 northd/northd.h | 7 ++ northd/ovn-northd.8.xml | 54 ++ ovn-nb.xml | 42 tests/ovn-northd.at | 93 + tests/system-ovn.at | 120 + 6 files changed, 541 insertions(+) diff --git a/northd/northd.c b/northd/northd.c index 0c73e70df..9f2fd5278 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -13935,6 +13935,229 @@ build_arp_resolve_flows_for_lrp(struct ovn_port *op, } } +static void +build_routing_protocols_redirect_rule__( +const char *s_addr, const char *redirect_port_name, int protocol_port, +const char *proto, bool is_ipv6, struct ovn_port *ls_peer, +struct lflow_table *lflows, struct ds *match, struct ds *actions) +{ +int ip_ver = is_ipv6 ? 6 : 4; +ds_clear(actions); +ds_put_format(actions, "outport = \"%s\"; output;", redirect_port_name); + +/* Redirect packets in the input pipeline destined for LR's IP + * and the routing protocol's port to the LSP specified in + * 'routing-protocol-redirect' option.*/ +ds_clear(match); +ds_put_format(match, "ip%d.dst == %s && %s.dst == %d", ip_ver, s_addr, + proto, protocol_port); +ovn_lflow_add(lflows, ls_peer->od, S_SWITCH_IN_L2_LKUP, 100, + ds_cstr(match), + ds_cstr(actions), + ls_peer->lflow_ref); + +/* To accomodate "peer" nature of the routing daemons, redirect also + * replies to the daemons' client requests. */ +ds_clear(match); +ds_put_format(match, "ip%d.dst == %s && %s.src == %d", ip_ver, s_addr, + proto, protocol_port); +ovn_lflow_add(lflows, ls_peer->od, S_SWITCH_IN_L2_LKUP, 100, + ds_cstr(match), + ds_cstr(actions), + ls_peer->lflow_ref); +} + +static void +apply_routing_protocols_redirect__( +const char *s_addr, const char *redirect_port_name, int protocol_flags, +bool is_ipv6, struct ovn_port *ls_peer, struct lflow_table *lflows, +struct ds *match, struct ds *actions) +{ +if (protocol_flags & REDIRECT_BGP) { +build_routing_protocols_redirect_rule__(s_addr, redirect_port_name, +179, "tcp", is_ipv6, ls_peer, +lflows, match, actions); +} + +if (protocol_flags & REDIRECT_BFD) { +build_routing_protocols_redirect_rule__(s_addr, redirect_port_name, +3784, "udp", is_ipv6, ls_peer, +lflows, match, actions); +} + +/* Because the redirected port shares IP and MAC addresses with the LRP, + * special consideration needs to be given to the signaling protocols. */ +if (is_ipv6) { +/* Ensure that redirect port rec
Re: [ovs-dev] [PATCH ovn v2] test: Fix flaky I-P test.
I merged this to main. On 8/9/24 11:39, Mark Michelson wrote: Thanks Ales. Acked-by: Mark Michelson On 8/8/24 09:03, Ales Musil wrote: The test was checking if there was any recompute after all operations. This proven to be flaky because there might have been "random" recompute happening in the middle due to the following events: 1) SB becomes read only 2) non_vif_data handler for OVS_interface fails 3) Recompute of non_vif_data is not allowed because SB is read only 4) non_vif_data is marked as cancelled 5) Next engine run will do force recompute of all nodes To prevent that check specifically for the handler that is supposed to gracefully handle the interface changes. Fixes: 603f38da062b ("controller: Remove OvS iface type check in I-P processing.") Signed-off-by: Ales Musil --- v2: Address comment from Ilya: - Add recompute case to the test so if the debug format changes the test fails. - Add missing checks for ovs-vsctl. --- tests/ovn-controller.at | 43 + 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 898981867..dcab5f2e9 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -3308,51 +3308,60 @@ net_add n1 sim_add hv1 ovs-vsctl add-br br-phys ovn_attach n1 br-phys 192.168.0.20 +ovn-appctl vlog/set inc_proc_eng:dbg check ovn-nbctl ls-add ls0 check ovn-nbctl lsp-add ls0 vif -ovn-appctl inc-engine/clear-stats +m4_define([HANDLER_MESSAGE], [runtime_data, recompute (failed handler for input ovs_interface_shadow)]) -ovs-vsctl -- add-port br-int vif -- \ +check ovs-vsctl -- add-port br-int vif -- \ set Interface vif external-ids:iface-id=vif wait_row_count Port_Binding 1 logical_port="vif" up=true +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl del-port br-int vif +check ovs-vsctl del-port br-int vif wait_row_count Port_Binding 1 logical_port="vif" up=false +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl add-port br-int vif -- \ +check ovs-vsctl add-port br-int vif -- \ set Interface vif type=dummy -- \ set Interface vif external-ids:iface-id=vif wait_row_count Port_Binding 1 logical_port="vif" up=true +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl del-port br-int vif +check ovs-vsctl del-port br-int vif wait_row_count Port_Binding 1 logical_port="vif" up=false +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl add-port br-int vif -- \ +check ovs-vsctl add-port br-int vif -- \ set Interface vif type=geneve -- \ set Interface vif options:remote_ip=1.1.1.1 external-ids:iface-id=vif wait_row_count Port_Binding 1 logical_port="vif" up=true +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl del-port br-int vif +check ovs-vsctl del-port br-int vif wait_row_count Port_Binding 1 logical_port="vif" up=false +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) # Make sure that setting iface in two different transaction doesn't # cause recompute. -ovs-vsctl add-port br-int vif -ovs-vsctl set Interface vif external-ids:iface-id=vif +check ovs-vsctl add-port br-int vif +check ovs-vsctl set Interface vif external-ids:iface-id=vif wait_row_count Port_Binding 1 logical_port="vif" up=true +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl del-port br-int vif +check ovs-vsctl del-port br-int vif wait_row_count Port_Binding 1 logical_port="vif" up=false +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -AT_CHECK([ovn-appctl inc-engine/show-stats runtime_data |\ - sed "s/- compute:\s\+[[0-9]]\+/- compute: ??/"], [0], [dnl -Node: runtime_data -- recompute: 0 -- compute: ?? -- cancel: 0 -]) +# Make sure that setting dummy iface in two different transactions +# causes recompute. +check ovs-vsctl add-port br-int vif -- \ + set Interface vif type=dummy +check ovs-vsctl set Interface vif external-ids:iface-id=vif +wait_row_count Port_Binding 1 logical_port="vif" up=true +AT_CHECK([test $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log) -ge 1]) OVN_CLEANUP([hv1]) AT_CLEANUP ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] test: Fix flaky I-P test.
Thanks Ales. Acked-by: Mark Michelson On 8/8/24 09:03, Ales Musil wrote: The test was checking if there was any recompute after all operations. This proven to be flaky because there might have been "random" recompute happening in the middle due to the following events: 1) SB becomes read only 2) non_vif_data handler for OVS_interface fails 3) Recompute of non_vif_data is not allowed because SB is read only 4) non_vif_data is marked as cancelled 5) Next engine run will do force recompute of all nodes To prevent that check specifically for the handler that is supposed to gracefully handle the interface changes. Fixes: 603f38da062b ("controller: Remove OvS iface type check in I-P processing.") Signed-off-by: Ales Musil --- v2: Address comment from Ilya: - Add recompute case to the test so if the debug format changes the test fails. - Add missing checks for ovs-vsctl. --- tests/ovn-controller.at | 43 + 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 898981867..dcab5f2e9 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -3308,51 +3308,60 @@ net_add n1 sim_add hv1 ovs-vsctl add-br br-phys ovn_attach n1 br-phys 192.168.0.20 +ovn-appctl vlog/set inc_proc_eng:dbg check ovn-nbctl ls-add ls0 check ovn-nbctl lsp-add ls0 vif -ovn-appctl inc-engine/clear-stats +m4_define([HANDLER_MESSAGE], [runtime_data, recompute (failed handler for input ovs_interface_shadow)]) -ovs-vsctl -- add-port br-int vif -- \ +check ovs-vsctl -- add-port br-int vif -- \ set Interface vif external-ids:iface-id=vif wait_row_count Port_Binding 1 logical_port="vif" up=true +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl del-port br-int vif +check ovs-vsctl del-port br-int vif wait_row_count Port_Binding 1 logical_port="vif" up=false +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl add-port br-int vif -- \ +check ovs-vsctl add-port br-int vif -- \ set Interface vif type=dummy -- \ set Interface vif external-ids:iface-id=vif wait_row_count Port_Binding 1 logical_port="vif" up=true +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl del-port br-int vif +check ovs-vsctl del-port br-int vif wait_row_count Port_Binding 1 logical_port="vif" up=false +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl add-port br-int vif -- \ +check ovs-vsctl add-port br-int vif -- \ set Interface vif type=geneve -- \ set Interface vif options:remote_ip=1.1.1.1 external-ids:iface-id=vif wait_row_count Port_Binding 1 logical_port="vif" up=true +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl del-port br-int vif +check ovs-vsctl del-port br-int vif wait_row_count Port_Binding 1 logical_port="vif" up=false +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) # Make sure that setting iface in two different transaction doesn't # cause recompute. -ovs-vsctl add-port br-int vif -ovs-vsctl set Interface vif external-ids:iface-id=vif +check ovs-vsctl add-port br-int vif +check ovs-vsctl set Interface vif external-ids:iface-id=vif wait_row_count Port_Binding 1 logical_port="vif" up=true +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -ovs-vsctl del-port br-int vif +check ovs-vsctl del-port br-int vif wait_row_count Port_Binding 1 logical_port="vif" up=false +AT_CHECK([test 0 = $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log)]) -AT_CHECK([ovn-appctl inc-engine/show-stats runtime_data |\ - sed "s/- compute:\s\+[[0-9]]\+/- compute: ??/"], [0], [dnl -Node: runtime_data -- recompute:0 -- compute: ?? -- cancel: 0 -]) +# Make sure that setting dummy iface in two different transactions +# causes recompute. +check ovs-vsctl add-port br-int vif -- \ +set Interface vif type=dummy +check ovs-vsctl set Interface vif external-ids:iface-id=vif +wait_row_count Port_Binding 1 logical_port="vif" up=true +AT_CHECK([test $(grep -c "HANDLER_MESSAGE" hv1/ovn-controller.log) -ge 1]) OVN_CLEANUP([hv1]) AT_CLEANUP ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 2/2] Prepare for post-24.09.0.
--- NEWS | 3 +++ configure.ac | 2 +- debian/changelog | 6 ++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index d8717810a..208992764 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,6 @@ +Post v24.09.0 +- + OVN v24.09.0 - xx xxx -- - Added a new logical switch port option "pkt_clone_type". diff --git a/configure.ac b/configure.ac index cd6a5d0c9..4a63b84d4 100644 --- a/configure.ac +++ b/configure.ac @@ -13,7 +13,7 @@ # limitations under the License. AC_PREREQ(2.63) -AC_INIT(ovn, 24.09.0, b...@openvswitch.org) +AC_INIT(ovn, 24.09.90, b...@openvswitch.org) AC_CONFIG_MACRO_DIR([m4]) AC_CONFIG_AUX_DIR([build-aux]) AC_CONFIG_HEADERS([config.h]) diff --git a/debian/changelog b/debian/changelog index 8168d1e83..d5277acad 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +ovn (24.09.90-1) unstable; urgency=low + + * New upstream version + + -- OVN team Fri, 09 Aug 2024 09:56:41 -0400 + ovn (24.09.0-1) unstable; urgency=low * New upstream version -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 1/2] Prepare for 24.09.0.
--- NEWS | 4 ++-- configure.ac | 2 +- debian/changelog | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/NEWS b/NEWS index 5b939211f..d8717810a 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,5 @@ -Post v24.03.0 -- +OVN v24.09.0 - xx xxx +-- - Added a new logical switch port option "pkt_clone_type". If the value is set to "mc_unknown", packets destined to the port gets cloned to all unknown ports connected to the same Logical Switch. diff --git a/configure.ac b/configure.ac index 6a6b0db6a..cd6a5d0c9 100644 --- a/configure.ac +++ b/configure.ac @@ -13,7 +13,7 @@ # limitations under the License. AC_PREREQ(2.63) -AC_INIT(ovn, 24.03.90, b...@openvswitch.org) +AC_INIT(ovn, 24.09.0, b...@openvswitch.org) AC_CONFIG_MACRO_DIR([m4]) AC_CONFIG_AUX_DIR([build-aux]) AC_CONFIG_HEADERS([config.h]) diff --git a/debian/changelog b/debian/changelog index ebf4df7e9..8168d1e83 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,8 +1,8 @@ -ovn (24.03.90-1) unstable; urgency=low +ovn (24.09.0-1) unstable; urgency=low * New upstream version - -- OVN team Fri, 01 Mar 2024 14:06:45 -0500 + -- OVN team Fri, 09 Aug 2024 09:56:41 -0400 ovn (24.03.0-1) unstable; urgency=low -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10 1/3] northd: Introduce ECMP_Nexthop table in SB db.
I bumped the version and recalculated the checksum. I then pushed the series to main in OVN. On 8/9/24 06:12, Ilya Maximets wrote: On 8/9/24 10:27, Lorenzo Bianconi wrote: Introduce ECMP_Nexthop table in the SB db in order to track active ecmp-symmetric-reply connections and flush stale ones. Acked-by: Mark Michelson Signed-off-by: Lorenzo Bianconi --- lib/ovn-util.h | 2 ++ northd/en-northd.c | 35 northd/en-northd.h | 4 +++ northd/inc-proc-northd.c | 7 +++- northd/northd.c | 70 northd/northd.h | 10 ++ ovn-sb.ovsschema | 16 - ovn-sb.xml | 31 ++ tests/ovn-northd.at | 4 +++ 9 files changed, 177 insertions(+), 2 deletions(-) This patches is missing the "ovn" tag, so it went to a wrong patchwork. Marked as not applicable. If OVN maintainers want to pick it up, here it is: https://patchwork.ozlabs.org/project/openvswitch/cover/cover.1723190435.git.lorenzo.bianc...@redhat.com/ But see one comment below. diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema index ec39fdd81..cc7810e98 100644 --- a/ovn-sb.ovsschema +++ b/ovn-sb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Southbound", "version": "20.35.0", -"cksum": "2897301415 31493", +"cksum": "2767238276 32154", There needs to be a version increase as well. May be fixed on commit, I guess. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v11 ovn] northd: Add bfd, static_routes, route_policies and bfd_sync nodes to I-P engine.
On 8/8/24 12:22, Han Zhou wrote: On Wed, Aug 7, 2024 at 2:36 PM Han Zhou wrote: On Wed, Aug 7, 2024 at 10:29 AM Lorenzo Bianconi wrote: On Tue, Aug 6, 2024 at 9:53 AM Lorenzo Bianconi wrote: [...] -void -bfd_cleanup_connections(const struct nbrec_bfd_table *nbrec_bfd_table, -struct hmap *bfd_map) +static bool +bfd_is_port_running(const struct hmap *bfd_map, const char *port) { -const struct nbrec_bfd *nb_bt; struct bfd_entry *bfd_e; - -NBREC_BFD_TABLE_FOR_EACH (nb_bt, nbrec_bfd_table) { -bfd_e = bfd_port_lookup(bfd_map, nb_bt->logical_port, nb_bt->dst_ip); -if (!bfd_e) { -continue; -} - -if (!bfd_e->ref && strcmp(nb_bt->status, "admin_down")) { -/* no user for this bfd connection */ -nbrec_bfd_set_status(nb_bt, "admin_down"); +HMAP_FOR_EACH (bfd_e, hmap_node, bfd_map) { I like the idea of checking the existence of the bfd entry to decide if a port has bfd instead of relying on the has_bfd status. However, I have performance concerns for this loop. The bfd_port_lookup() searches by hashing both logical_port and dst_ip, but here for logical_port it just iterates all bfd entries, which looks unreasonable. It may be better to add a hindex for logical_port. Hi Han, I did not get what you mean here, can you please provide more details? Do you mean another hmap to link the logical_port name to the bfd_entry? The goal is to avoid this O(n) iteration looking for a given logical_port. Since multiple bfd entries can have the same logical_port name, it would be better to use the struct hindex instead of struct hmap to link logical_port name to the bfd_entry. ack, fine. Since in this case we just need to know if the logical_port is used in at least one bfd_entry, I guess a simap to count number of bfd_entry for a given logical port is enough, what do you think? Yes, or it can be just a sset, if we only care about the existence (0 or >0) but not the actual count. To unblock the ECMP series, I am ok to merge it as is for now, and optimize with sset later, preferably before the release. Acked-by: Han Zhou Thanks Han. I merged this to main. We'll get the optimization done before release. It looks like Lorenzo replied with a WIP for this. +if (!strcmp(bfd_e->logical_port, port)) { +return true; } } - -HMAP_FOR_EACH_POP (bfd_e, hmap_node, bfd_map) { -free(bfd_e); -} +return false; } + #define BFD_DEF_MINTX 1000 /* 1s */ #define BFD_DEF_MINRX 1000 /* 1s */ #define BFD_DEF_DETECT_MULT 5 @@ -9809,51 +9835,88 @@ static int bfd_get_unused_port(unsigned long *bfd_src_ports) return port + BFD_UDP_SRC_PORT_START; } -void -build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn, -const struct nbrec_bfd_table *nbrec_bfd_table, -const struct sbrec_bfd_table *sbrec_bfd_table, -const struct hmap *lr_ports, struct hmap *bfd_connections) +static char * +bfd_get_connection_status(const struct nbrec_bfd *nb_bt, + const struct hmap *rp_bfd_connections, + const struct hmap *sr_bfd_connections) { -struct hmap sb_only = HMAP_INITIALIZER(&sb_only); -const struct sbrec_bfd *sb_bt; -unsigned long *bfd_src_ports; -struct bfd_entry *bfd_e; -uint32_t hash; +struct bfd_entry *bfd_rp, *bfd_sr; -bfd_src_ports = bitmap_allocate(BFD_UDP_SRC_PORT_LEN); +bfd_rp = bfd_port_lookup(rp_bfd_connections, nb_bt->logical_port, + nb_bt->dst_ip); +if (!bfd_rp) { +bfd_sr = bfd_port_lookup(sr_bfd_connections, nb_bt->logical_port, + nb_bt->dst_ip); +if (!bfd_sr) { +return "admin_down"; +} +} -SBREC_BFD_TABLE_FOR_EACH (sb_bt, sbrec_bfd_table) { -bfd_e = xmalloc(sizeof *bfd_e); -bfd_e->sb_bt = sb_bt; -hash = hash_string(sb_bt->dst_ip, 0); -hash = hash_string(sb_bt->logical_port, hash); -hmap_insert(&sb_only, &bfd_e->hmap_node, hash); -bitmap_set1(bfd_src_ports, sb_bt->src_port - BFD_UDP_SRC_PORT_START); +return bfd_rp ? bfd_rp->status : bfd_sr->status; +} + +void +bfd_table_sync(struct ovsdb_idl_txn *ovnsb_txn, + const struct nbrec_bfd_table *nbrec_bfd_table, + const struct hmap *lr_ports, + const struct hmap *bfd_connections, + const struct hmap *rp_bfd_connections, + const struct hmap *sr_bfd_connections, + struct hmap *sync_bfd_connections) +{ +if (!ovnsb_txn) { +return; +} + +unsigned long *bfd_src_ports = bitmap_allocate(BFD_UDP_SRC_PORT_LEN); + +struct bfd_entry *bfd_e; +HMAP_FOR_EACH (bfd_e, hmap_node, bfd_connections) { +struct bfd_entry *e = bfd_alloc_entry(sync_bfd_connections, + bfd_
Re: [ovs-dev] [PATCH ovn v4] northd: Fix logical router load-balancer nat rules when using DGP.
Hi Roberto, Overall, I think this approach doesn't work except for the most simple configurations. Reading the code, the approach appears to be the following: 1) On each chassis with a DGP, add a flow that will load balance using ct_lb_mark() 2) On each chassis with a DGP, avoid the undnat operation by replacing the typical undat action with a "next;" action. 3) On each chassis with a DGP, install a stateless SNAT flow so that the source IP address is changed back into load balancer VIP address. The idea is that the traffic can be load balanced on any chassis with a DGP, then the reply traffic can be statelessly SNATted on any other chassis since the conntrack state is not needed to statelessly SNAT the reply traffic back to the load balancer VIP. There are several issues with this approach * The ct_lb_mark() flows have several ip4.dst=X actions added to them. However, these actions don't actually accomplish anything, because the ct_lb_mark() action at the end is what ultimately will select a new destination for the traffic. * The changes to the undnat flows ignore the lb_force_snat_ip and lb_skip_snat_ip options. * The stateless SNATs will not handle related traffic properly. For instance, if there is a TCP session that is load balanced, and the backend sends an ICMP message, the ICMP message will not be SNATted properly. In order to handle load balancing with DGPs bound to multiple chassis, the approach either has to be 1) Ensure that the traffic will always be redirected to the chassis where the load balancing occurred. 2) Distribute conntrack state. Currently (2) is not something that I know is supported, so (1) is the better option. I suppose you or someone else could write a patch that works this way, but honestly I feel like restricting load balancing to a single DGP makes this more easy and reliable. (2) may eventually become a reality. I know that there is currently research being done to determine the feasibility and scalability of distributed conntrack. If that comes to fruition, then it'll be much easier to support load balancing on logical routers with multiple DGPs. On 7/23/24 15:40, Roberto Bartzen Acosta via dev wrote: This commit fixes the build_distr_lrouter_nat_flows_for_lb function to include one NAT stateless flow entry for each DGP in use. Since we have added support to create multiple gateway ports per logical router, it's necessary to include in the LR nat rules pipeline a specific entry for each attached DGP. Otherwise, the ingress traffic is only redirected when the incoming LRP matches the chassis_resident field. Considering that DNAT rules for DGPs were implemented with the need to configure the DGP-related gateway-port column, the load-balancer NAT rule configuration can use a similar idea. In this case, we don't know the LRP responsible for the incoming traffic, and therefore we need to automatically apply a stateless NAT rule to the load-balancer on all DGPs to allow inbound traffic. After applying this patch, the incoming and/or outgoing traffic can pass through any chassis where the DGP resides without having problems with CT state. Reported-at: https://bugs.launchpad.net/ubuntu/+source/ovn/+bug/2054322 Fixes: 15348b7b806f ("ovn-northd: Multiple distributed gateway port support.") Signed-off-by: Roberto Bartzen Acosta --- northd/en-lr-stateful.c | 12 --- northd/northd.c | 96 - tests/ovn-northd.at | 222 3 files changed, 293 insertions(+), 37 deletions(-) diff --git a/northd/en-lr-stateful.c b/northd/en-lr-stateful.c index baf1bd2f8..f09691af6 100644 --- a/northd/en-lr-stateful.c +++ b/northd/en-lr-stateful.c @@ -516,18 +516,6 @@ lr_stateful_record_create(struct lr_stateful_table *table, table->array[od->index] = lr_stateful_rec; -/* Load balancers are not supported (yet) if a logical router has multiple - * distributed gateway port. Log a warning. */ -if (lr_stateful_rec->has_lb_vip && lr_has_multiple_gw_ports(od)) { -static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); -VLOG_WARN_RL(&rl, "Load-balancers are configured on logical " - "router %s, which has %"PRIuSIZE" distributed " - "gateway ports. Load-balancer is not supported " - "yet when there is more than one distributed " - "gateway port on the router.", - od->nbr->name, od->n_l3dgw_ports); -} - return lr_stateful_rec; } diff --git a/northd/northd.c b/northd/northd.c index 6898daa00..853d58f29 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -11026,31 +11026,30 @@ static void build_distr_lrouter_nat_flows_for_lb(struct lrouter_nat_lb_flows_ctx *ctx, enum lrouter_nat_lb_flow_type type, struct ovn_datapath *od, -
Re: [ovs-dev] [PATCH ovn v2] controller: Remove OvS iface type check in I-P processing.
I pushed this to main. On 7/30/24 15:05, Mark Michelson wrote: Thanks Ales, especially for the explanation for why the type check is still maintained. Acked-by: Mark Michelson On 7/24/24 08:53, Ales Musil wrote: The handler for OvS interface in runtime data was checking interface type before proceeding with the I-P processing. The type is not necessary because the main thing that is interesting for OVN is the iface-id, if the interface doesn't have any it won't be processed regardless of the type. The processing would happen anyway, however it would be more costly because it would lead to full recompute of the whole runtime data node. The type check remains for the purpose of not regressing when iface id is set in different transaction than the actual interface creation for the internal and empty type. Reported-at: https://github.com/ovn-org/ovn/issues/174 Reported-at: https://issues.redhat.com/browse/FDP-255 Signed-off-by: Ales Musil --- v2: Rebase on top of current main. Address comment from Xavier: - Make sure we are not causing recompute if iface-id is set in separate transaction. --- controller/binding.c | 14 +- tests/ovn-controller.at | 57 + 2 files changed, 64 insertions(+), 7 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index b7e7f4874..f4f5f9407 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -2630,17 +2630,17 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, const struct ovsrec_interface *iface_rec; OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec, b_ctx_in->iface_table) { - if (!is_iface_vif(iface_rec)) { - /* Right now we are not handling ovs_interface changes of - * other types. This can be enhanced to handle of - * types - patch and tunnel. */ + const char *iface_id = smap_get(&iface_rec->external_ids, "iface-id"); + const char *old_iface_id = smap_get(b_ctx_out->local_iface_ids, + iface_rec->name); + if (!iface_id && !old_iface_id && !is_iface_vif(iface_rec)) { + /* Right now we are not handling ovs_interface changes if the + * interface doesn't have iface-id or didn't have it + * previously. */ handled = false; break; } - const char *iface_id = smap_get(&iface_rec->external_ids, "iface-id"); - const char *old_iface_id = smap_get(b_ctx_out->local_iface_ids, - iface_rec->name); const char *cleared_iface_id = NULL; if (!ovsrec_interface_is_deleted(iface_rec)) { int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 9cb099e68..208645504 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -3127,3 +3127,60 @@ OVS_WAIT_UNTIL([grep -q 'tcp:127.0.0.1:1235: connected' hv1/ovn-controller.log]) OVN_CLEANUP([hv1]) AT_CLEANUP + +AT_SETUP([ovn-controller - I-P different port types]) +AT_KEYWORDS([ovn]) +ovn_start + +net_add n1 +sim_add hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.20 + +check ovn-nbctl ls-add ls0 +check ovn-nbctl lsp-add ls0 vif + +ovn-appctl inc-engine/clear-stats + +ovs-vsctl -- add-port br-int vif -- \ + set Interface vif external-ids:iface-id=vif +wait_row_count Port_Binding 1 logical_port="vif" up=true + +ovs-vsctl del-port br-int vif +wait_row_count Port_Binding 1 logical_port="vif" up=false + +ovs-vsctl add-port br-int vif -- \ + set Interface vif type=dummy -- \ + set Interface vif external-ids:iface-id=vif +wait_row_count Port_Binding 1 logical_port="vif" up=true + +ovs-vsctl del-port br-int vif +wait_row_count Port_Binding 1 logical_port="vif" up=false + +ovs-vsctl add-port br-int vif -- \ + set Interface vif type=geneve -- \ + set Interface vif options:remote_ip=1.1.1.1 external-ids:iface-id=vif +wait_row_count Port_Binding 1 logical_port="vif" up=true + +ovs-vsctl del-port br-int vif +wait_row_count Port_Binding 1 logical_port="vif" up=false + +# Make sure that setting iface in two different transaction doesn't +# cause recompute. +ovs-vsctl add-port br-int vif +ovs-vsctl set Interface vif external-ids:iface-id=vif +wait_row_count Port_Binding 1 logical_port="vif" up=true + +ovs-vsctl del-port br-int vif +wait_row_count Port_Binding 1 logical_port="vif" up=false + +AT_CHECK([ovn-appctl inc-engine/show-stats runtime_data |\ + sed "s/- compute:\s\+[[0-9]]\+/- compute: ??/"], [0], [dnl +Node: runtime_data +- recompute: 0 +- compute: ?? +- cancel: 0 +]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v7 9/9] northd: Allow flow simplification for ACL sampling.
Thanks for all the updates Ales and Dumitru, Acked-by: Mark Michelson This means I have acked all patches in the series now. On 8/7/24 02:51, Dumitru Ceara wrote: From: Ales Musil Currently, OVN would generate up to 2 flows per sample, depending on the configuration. Add optimization that can reduce the number of flows added into the ACL pipeline down to 3 per collector. This optimization can be achieved only when the sample action with registers is supported in OvS and the sample has only single collector. The single collector per sample should be the case in most configurations, usually even the same collector for all samples which greatly reduces the number of flows per ACL with sampling. If there are more collectors per sample or the OvS feature is not supported, the implementation will fall back to flows per sample. Reported-at: https://issues.redhat.com/browse/FDP-709 Signed-off-by: Ales Musil --- V7: - Addressed Nadia's comment: - Increased number of ct mark bits used for storing the collector id to 8. - Addressed Mark's comment: - cleaned up conditional match build. V6: - Rebased. - Removed Dumitru's ack. - Store (newly created) Sample_Collector.id in ct state - instead of the actual set-id to avoid ambiguity when multiple probabilities are used with the same collector set id. - Fixed bug with stateful to-lport ACLs on router ports. - Reduced number of ct mark bits used for storing the collector id to 4. V5: - Address Ilya's comments: - Explicitly set acl_observation_stage enum values. - Added Dumitru's ack --- include/ovn/logical-fields.h | 2 + lib/logical-fields.c | 8 + northd/northd.c | 273 ++-- tests/ovn-northd.at | 388 +-- tests/ovn.at | 2 + tests/system-ovn.at | 10 +- 6 files changed, 553 insertions(+), 130 deletions(-) diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h index ce79b501cf..d6c4a9b6b3 100644 --- a/include/ovn/logical-fields.h +++ b/include/ovn/logical-fields.h @@ -197,6 +197,8 @@ const struct ovn_field *ovn_field_from_name(const char *name); #define OVN_CT_NATTED_BIT 1 #define OVN_CT_LB_SKIP_SNAT_BIT 2 #define OVN_CT_LB_FORCE_SNAT_BIT 3 +#define OVN_CT_OBS_STAGE_1ST_BIT 4 +#define OVN_CT_OBS_STAGE_END_BIT 5 #define OVN_CT_BLOCKED 1 #define OVN_CT_NATTED 2 diff --git a/lib/logical-fields.c b/lib/logical-fields.c index 0c187e1c84..134d2674fd 100644 --- a/lib/logical-fields.c +++ b/lib/logical-fields.c @@ -165,6 +165,14 @@ ovn_init_symtab(struct shash *symtab) OVN_CT_STR(OVN_CT_LB_FORCE_SNAT_BIT) "]", WR_CT_COMMIT); +expr_symtab_add_subfield_scoped(symtab, "ct_mark.obs_stage", NULL, +"ct_mark[" +OVN_CT_STR(OVN_CT_OBS_STAGE_1ST_BIT) ".." +OVN_CT_STR(OVN_CT_OBS_STAGE_END_BIT) +"]", +WR_CT_COMMIT); +expr_symtab_add_subfield_scoped(symtab, "ct_mark.obs_collector_id", NULL, +"ct_mark[16..23]", WR_CT_COMMIT); expr_symtab_add_field_scoped(symtab, "ct_label", MFF_CT_LABEL, NULL, false, WR_CT_COMMIT); diff --git a/northd/northd.c b/northd/northd.c index 13f9faba31..70b13c8c98 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -144,8 +144,20 @@ static bool vxlan_mode; #define REGBIT_ACL_VERDICT_ALLOW "reg8[16]" #define REGBIT_ACL_VERDICT_DROP "reg8[17]" #define REGBIT_ACL_VERDICT_REJECT "reg8[18]" +#define REGBIT_ACL_OBS_STAGE "reg8[19..20]" #define REG_ACL_TIER "reg8[30..31]" +enum acl_observation_stage { +ACL_OBS_FROM_LPORT = 0, +ACL_OBS_FROM_LPORT_AFTER_LB = 1, +ACL_OBS_TO_LPORT= 2, +ACL_OBS_STAGE_MAX +}; + +/* enum acl_observation_stage_t values must fit in the 2 bits of + * REGBIT_ACL_OBS_STAGE .*/ +BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2)); + /* Indicate that this packet has been recirculated using egress * loopback. This allows certain checks to be bypassed, such as a * logical router dropping packets with source IP address equals @@ -189,6 +201,8 @@ static bool vxlan_mode; * domain and point ID. */ #define REG_OBS_POINT_ID_NEW "reg3" #define REG_OBS_POINT_ID_EST "reg9" +#define REG_OBS_COLLECTOR_ID_NEW "reg8[0..7]" +#define REG_OBS_COLLECTOR_ID_EST "reg8[8..15]" /* Register used for temporarily store ECMP eth.src to avoid masked ct_label * access. It doesn't really occupy registers because the content of t
Re: [ovs-dev] [PATCH ovn] northd: Allow Load balancer to be attached to router with multiple GW ports
Hi I just wanted to weigh in. On 8/6/24 13:26, Roberto Bartzen Acosta via dev wrote: Hi Priyankar, Em ter., 6 de ago. de 2024 às 12:35, Priyankar Jain < priyankar.j...@nutanix.com> escreveu: Hi, On 06/08/24 6:15 pm, Roberto Bartzen Acosta wrote: Hi Priyankar, So, what is the difference between the patch and the one already proposed [1]? Ahh I just got to know about this patch. Looks same to me. The change you are proposing has already been proposed before, and in discussion with Mark we came to the conclusion that this breaks the DNAT case when using multiple chassis. So, I pushed the v4 [2][3] with Stateless NAT implementation to solve this problem. I agree that this patch looks a lot like v1 of Roberto's series. I can't comment on v4 of the patch yet since I haven't had a chance to look at it yet. I have it on my list to get reviewed before the end of the week. I reviewed this patch. If we are using stateless NAT, does that mean packet won't be passed through CT in the egress direction? If so, wouldn't this cause TCP state machine to break wrt CT (and CT table marking the packets as invalid). say SYN gets commited to CT (ingress pipeline ct_lb_mark action), syn-ACK won't be commited as only egress will run on GW node. Again the ACK would be commited. So are we sure wrt sequence numbers/window scaling, we are safe wrt TCP CT state machine here? My proposal does not affect LB egress since the output from the backends is still done integrated with the ls_in_lb pipeline, and the DGP egress flow depends on how you configure the routing/external NAT (e.g. OVN-IC uses direct routing for this). In addition, the v4 patch adds the ability to receive packets in the lr_in_dnat pipeline via different chassis_resident and, additionally, deliver the packet to ct_lb_mark to process the CT state in the LB pipeline before delivering it to the backend IPs (regardless of the chassis that receives the traffic). Also, as i mentioned in my previous reply, our usecase is much more restricted and routing ensures ingress during request (from outside to LR) and egress ( for undnat) runs on the gw node. The problem with this is that there's nothing in the attached patch to enforce this restricted scneario. It's possible for people to shoot themselves in the foot. Have you ever applied the patch v4 to your use case? Considering the test cases you write in your patch, the tests passed with my version v4 without any issues. What do you think? ## ## ## ovn 24.03.90 test suite. ## ## ## OVN end-to-end tests 533: Multiple DGP and LB traffic -- parallelization=yes -- ovn_monitor_all=yes ok 534: Multiple DGP and LB traffic -- parallelization=yes -- ovn_monitor_all=no ok Thanks, Roberto Thanks, Priyankar [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2024-2DFebruary_411981.html&d=DwIFaQ&c=s883GpUCOChKOHiocYtGcg&r=BWfApgwqQ_BTXd_yEq_kwWLM-KTqkdPvolDMCJmUagg&m=WfKrYrekTgMkBBiSsurQ7bYA31uPfqnE-Ybpq06XApWiO9Ij2MlCi8dO02I-kZf0&s=JR1CKXLiNrDGVt85mXYTutzBqhpCpiABOutl2TMPWEE&e= [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2024-2DJuly_415977.html&d=DwIFaQ&c=s883GpUCOChKOHiocYtGcg&r=BWfApgwqQ_BTXd_yEq_kwWLM-KTqkdPvolDMCJmUagg&m=WfKrYrekTgMkBBiSsurQ7bYA31uPfqnE-Ybpq06XApWiO9Ij2MlCi8dO02I-kZf0&s=QA8-pREOG2X_f-Hk8zDEeXxyV6jOoNx3pRxuKAIhsCk&e= [3] https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovsrobot_ovn_commit_9379c81152df2bcd0626269988efb8ac36d4f624&d=DwIFaQ&c=s883GpUCOChKOHiocYtGcg&r=BWfApgwqQ_BTXd_yEq_kwWLM-KTqkdPvolDMCJmUagg&m=WfKrYrekTgMkBBiSsurQ7bYA31uPfqnE-Ybpq06XApWiO9Ij2MlCi8dO02I-kZf0&s=axc06cz9XfMSiEKgrD6QmmoYwrBVxBnm1T2gEx-_byY&e= Thanks, Roberto Em ter., 6 de ago. de 2024 às 07:57, Priyankar Jain < priyankar.j...@nutanix.com> escreveu: Hi, Please find my reply inline. Thanks, Priyankar On 31/07/24 4:46 am, Numan Siddique wrote: On Wed, Jul 17, 2024 at 5:04 AM Priyankar Jain wrote: This commit removes the restriction of support LB for router with only <= 1 Distributed gateway ports. Added datapath and logical flows validation cases. Signed-off-by: Priyankar Jain Hi Priyankar, Thanks for the patch. Can you please tell me the use case ? Lets say a logical router lr1 has sw1 (10.0.0.0/24) , sw2 (20.0.0.0/24) and sw3 (30.0.0.0/24) and the corresponding gw ports are bound to chassis ch1, ch2 and ch3 respectively. Let's say we attach load balancers - lb1 (10.0.0.40:80 -> 20.0.0.40:80), lb2 (20.0.0.50:80 -> 30.0.0.50:80) and lb3 (30.0.0.60:80 -> 10.0.0.60:80) to lr1. If I understand correctly, routing for sw1 (10.0.0.0/24) is handled on the chassis ch1 and so on for other switches. This patch generates the below logical flows in the router pipeline for load balancer lb1, lb2 and lb3 table=8 (lr_in_dnat ), priority=110 , match=(ct.new && !ct.rel && ip4 && ip4.dst == 10.0.0.40 && tcp && tcp.d
Re: [ovs-dev] [PATCH ovn v6 9/9] northd: Allow flow simplification for ACL sampling.
On 8/6/24 05:44, Dumitru Ceara wrote: From: Ales Musil Currently, OVN would generate up to 2 flows per sample, depending on the configuration. Add optimization that can reduce the number of flows added into the ACL pipeline down to 3 per collector. This optimization can be achieved only when the sample action with registers is supported in OvS and the sample has only single collector. The single collector per sample should be the case in most configurations, usually even the same collector for all samples which greatly reduces the number of flows per ACL with sampling. If there are more collectors per sample or the OvS feature is not supported, the implementation will fall back to flows per sample. Reported-at: https://issues.redhat.com/browse/FDP-709 Signed-off-by: Ales Musil --- V6: - Rebased. - Removed Dumitru's ack. - Store (newly created) Sample_Collector.id in ct state - instead of the actual set-id to avoid ambiguity when multiple probabilities are used with the same collector set id. - Fixed bug with stateful to-lport ACLs on router ports. - Reduced number of ct mark bits used for storing the collector id to 4. V5: - Address Ilya's comments: - Explicitly set acl_observation_stage enum values. - Added Dumitru's ack --- include/ovn/logical-fields.h | 2 + lib/logical-fields.c | 8 + northd/northd.c | 273 ++-- tests/ovn-northd.at | 388 +-- tests/ovn.at | 2 + tests/system-ovn.at | 10 +- 6 files changed, 553 insertions(+), 130 deletions(-) diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h index ce79b501cf..d6c4a9b6b3 100644 --- a/include/ovn/logical-fields.h +++ b/include/ovn/logical-fields.h @@ -197,6 +197,8 @@ const struct ovn_field *ovn_field_from_name(const char *name); #define OVN_CT_NATTED_BIT 1 #define OVN_CT_LB_SKIP_SNAT_BIT 2 #define OVN_CT_LB_FORCE_SNAT_BIT 3 +#define OVN_CT_OBS_STAGE_1ST_BIT 4 +#define OVN_CT_OBS_STAGE_END_BIT 5 #define OVN_CT_BLOCKED 1 #define OVN_CT_NATTED 2 diff --git a/lib/logical-fields.c b/lib/logical-fields.c index 0c187e1c84..00bbc4a1c4 100644 --- a/lib/logical-fields.c +++ b/lib/logical-fields.c @@ -165,6 +165,14 @@ ovn_init_symtab(struct shash *symtab) OVN_CT_STR(OVN_CT_LB_FORCE_SNAT_BIT) "]", WR_CT_COMMIT); +expr_symtab_add_subfield_scoped(symtab, "ct_mark.obs_stage", NULL, +"ct_mark[" +OVN_CT_STR(OVN_CT_OBS_STAGE_1ST_BIT) ".." +OVN_CT_STR(OVN_CT_OBS_STAGE_END_BIT) +"]", +WR_CT_COMMIT); +expr_symtab_add_subfield_scoped(symtab, "ct_mark.obs_collector_id", NULL, +"ct_mark[16..19]", WR_CT_COMMIT); expr_symtab_add_field_scoped(symtab, "ct_label", MFF_CT_LABEL, NULL, false, WR_CT_COMMIT); diff --git a/northd/northd.c b/northd/northd.c index 13f9faba31..b1201bcc86 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -144,8 +144,20 @@ static bool vxlan_mode; #define REGBIT_ACL_VERDICT_ALLOW "reg8[16]" #define REGBIT_ACL_VERDICT_DROP "reg8[17]" #define REGBIT_ACL_VERDICT_REJECT "reg8[18]" +#define REGBIT_ACL_OBS_STAGE "reg8[19..20]" #define REG_ACL_TIER "reg8[30..31]" +enum acl_observation_stage { +ACL_OBS_FROM_LPORT = 0, +ACL_OBS_FROM_LPORT_AFTER_LB = 1, +ACL_OBS_TO_LPORT= 2, +ACL_OBS_STAGE_MAX +}; + +/* enum acl_observation_stage_t values must fit in the 2 bits of + * REGBIT_ACL_OBS_STAGE .*/ +BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2)); + /* Indicate that this packet has been recirculated using egress * loopback. This allows certain checks to be bypassed, such as a * logical router dropping packets with source IP address equals @@ -189,6 +201,8 @@ static bool vxlan_mode; * domain and point ID. */ #define REG_OBS_POINT_ID_NEW "reg3" #define REG_OBS_POINT_ID_EST "reg9" +#define REG_OBS_COLLECTOR_ID_NEW "reg8[0..3]" +#define REG_OBS_COLLECTOR_ID_EST "reg8[4..7]" /* Register used for temporarily store ECMP eth.src to avoid masked ct_label * access. It doesn't really occupy registers because the content of the @@ -228,12 +242,13 @@ static bool vxlan_mode; * ++--+ G | | * | R7 | UNUSED | 1 | | * ++--+---+---+ - * || LB_AFF_MATCH_PORT | - * || (>= IN_LB_AFF_CHECK && <= IN_LB_AFF_LEARN) | - * ++--+ - * | R9 |
Re: [ovs-dev] [PATCH ovn v6 0/9] Add ACL Sampling using per-flow IPFIX.
For patches 1-8: Acked-by: Mark Michelson I have a single small comment on patch 9. On 8/6/24 05:44, Dumitru Ceara wrote: This series adds support for sampling packets processed by ACLs by using per-flow IPFIX. This new feature allows users to configure (potentially) different sampling options for ACL matched traffic that creates new connections or that is forwarded on existing connections. This work is based on Adrian's original RFC: https://patchwork.ozlabs.org/project/ovn/cover/20221018155936.1394396-1-amore...@redhat.com/ In order for the whole feature to work properly some pre-requisite work is done: - patch 1: fixes the QoS logical flow documentation. This is needed because the sampling patches need to insert new tables and numbers were inconsistent. - patch 2: fixes a bug in the way ACLs with labels are processed when the switches also have load balancers configured The feature itself is implemented by the last 3 patches: - patch 3: adds support for users to configure different types of sampling applications (drop debug, acl-new-traffic, acl-established-traffic) - patch 4: combines the already existing drop debug sampling configuration with the new sampling application configuration (giving priority to the latter) - patch 5: adds sampling support to ACLs Patches 6-9 implement an optimization and reduce the number of logical and openflow rules for the case when sampling is enabled for ACLs with a single collector (the common case). This optimization requires the recently added OVS support for sampling with observation IDs passed directly from fields [0]. [0] https://github.com/openvswitch/ovs/commit/1aa9e137fe36a810271415d79735dedfedfc9f6e Changes in V6: - Addressed (some) review comments from Ilya (individual changes listed in each patch). Most important changes: - Changed sample_collector schema to add unique ID (4 bit): this fixes the case with multiple probabilities per set_id and reduces the number of register and ct-mark bits used. - Made Sample table non-root (this needs changes to ovn-nbctl acl-add command too). Not addressed review comments: - Didn't use the single collector per sample_config type suggestion because OVN-K8s needs the flexibility of using different collectors (or multiple collectors) per ACL. Fixed a bug with sampling on to-lport ACLs when they're hit in the egress pipeline towards logical routers. Changes in V5: - Addressed review comments from Numan and Ilya (individual changes listed in each patch). The most important change is the NB.Sampling_App 'name' column change to 'type' along with shortening of the strings representing allowed app types. Changes in V4: - Addressed review comments from Mark, Ales and Numan (individual changes listed in each patch). - Dropped first 4 patches of V3 because they were already accepted. - Added a first 1/5 patch to fix documentation that I needed to touch too. - Added Ales as co-author of patch 5, he provided most of the incremental changes that were added to that patch in v4. - Included Ales' patches (6-9) to reduce the number of sampling flows when the underlying OVS instance supports sampling with IDs taken from fields (or registers). Changes in V3: - Addressed Ilya's comment and bumped NB schema version on patch 8. I didn't bump it on patch 6 too because I don't think these two commits will ever be separated in different releases. Changes in V2: - Addressed Adrian's comments on patch 8. - Fixed unit test failure in patch 2. Adrian Moreno (1): northd: Add ACL Sampling. Ales Musil (4): features: Make querying of OpenFlow features more versatile. features: Add detection for sample with registers. actions: Add support for sample with register. northd: Allow flow simplification for ACL sampling. Dumitru Ceara (4): northd: Fix up logical flow documentation for QoS. northd: Commit from-lport ACL label (and state) when LBs are used. northd: Add Sampling_App table. northd: Override NB_Global drop sampling id with Sampling_App config. NEWS | 6 + controller/chassis.c | 15 + controller/lflow.h | 12 +- include/ovn/actions.h | 16 +- include/ovn/features.h | 5 + include/ovn/logical-fields.h | 2 + lib/actions.c | 12 +- lib/features.c | 360 --- lib/logical-fields.c | 12 + lib/ovn-util.h | 2 +- northd/automake.mk | 2 + northd/debug.c | 12 +- northd/debug.h | 3 +- northd/en-global-config.c | 41 +- northd/en-global-config.h | 1 + northd/en-lflow.c | 5 + northd/en-
Re: [ovs-dev] [PATCH ovn v2] Add support for centralize routing for distributed gw ports.
I know my Ack is already on this, but I had another look, and just to reiterate: Acked-by: Mark Michelson On 7/29/24 22:38, num...@ovn.org wrote: From: Numan Siddique Consider a deployment with the below logical resources: 1. A bridged logical switch 'public' with a port - P1 and a localnet port ln-public. 2. A logical router 'R' 3. Logical switch 'public' connected to R via logical switch/router port peers (public-R and R-public). 4. R-public is distributed gateway port with its network as 172.16.0.0/24 5. NATs (dnat_and_snat) configured in 'R'. 6. And a few overlay logical switches S1, S2 to R. Any traffic from logical port - P1 of public logical switch destined to S1 or S2's logical ports goes out of the source chassis (where P1 resides) via the localnet port and reaches the gateway chassis which handles the routing. There are couple of traffic flow scenarios which doesn't work if the logical switch 'public' doesn't have a localnet port. 1. Traffic from port - P1 destined to logical switches S1 or S2 gets dropped in the source chassis. The packet enters the router R's pipeline, but it gets dropped in the 'lr_in_admission' stage since the logical flow to allow traffic destined to the distributed gateway port MAC is installed only on the gateway chassis. 2. NAT doesn't work as expected. In order to suppose this use case (of a logical switch not having a localnet port, but has a distributed gateway port and NATs), this patch supports the option 'centralize_routing', which can be configured on the distributed gateway port (R-public in the example above). If this option is set, then routing is centralized on the gateway chassis for the traffic destined to the R-public's networks (172.16.0.0/24 for the above example). Traffic from P1 will be tunnelled to the gateway chassis. ovn-northd creates a chassisresident port (cr-public-R) for the logical switch port - public-R, along with cr-R-public inorder to centralize the traffic. This feature gets enabled for the distributed gateway port R-public if - The above option is set to true in the R-public's options column. - The logical switch 'public' doesn't have any localnet ports. - And R-public is the only distributed gateway port of R. Distributed NAT (i.e if external_mac and router_port is set) is not supported and instead the router port mac is used for such traffic and centralized on the gateway chassis. Reported-at: https://issues.redhat.com/browse/FDP-364 Acked-by: Mark Michelson Signed-off-by: Numan Siddique --- v1 -> v2 --- * Corrected the NEWS item entry for the new option. * Rebased and resolved conflicts. Note: This patch is the continuation from this series - https://patchwork.ozlabs.org/project/ovn/patch/20240606214432.168750-1-num...@ovn.org/ Resetted the version number since the new option changed from LSP to LRP. NEWS | 3 + controller/physical.c | 4 + northd/northd.c | 257 +++ northd/northd.h | 1 + ovn-nb.xml| 34 +++ tests/multinode-macros.at | 2 +- tests/multinode.at| 177 + tests/ovn-northd.at | 516 +- tests/ovn.at | 8 +- 9 files changed, 952 insertions(+), 50 deletions(-) diff --git a/NEWS b/NEWS index 87e326f21e..8440a74677 100644 --- a/NEWS +++ b/NEWS @@ -47,6 +47,9 @@ Post v24.03.0 - Add support for CT zone limit that can be specified per LR (options:ct-zone-limit), LS (other_config:ct-zone-limit) or LSP (options:ct-zone-limit). + - A new LRP option 'centralize_routing' has been added to a +distributed gateway port to centralize routing if the logical +switch of its peer doesn't have a localnet port. OVN v24.03.0 - 01 Mar 2024 -- diff --git a/controller/physical.c b/controller/physical.c index 3c0200c383..9e04ad5f22 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -1610,6 +1610,10 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, ct_zones); put_zones_ofpacts(&zone_ids, ofpacts_p); +/* Clear the MFF_INPORT. Its possible that the same packet may + * go out from the same tunnel inport. */ +put_load(ofp_to_u16(OFPP_NONE), MFF_IN_PORT, 0, 16, ofpacts_p); + /* Resubmit to table 41. */ put_resubmit(OFTABLE_CHECK_LOOPBACK, ofpacts_p); } diff --git a/northd/northd.c b/northd/northd.c index 5c2fd74ff1..4f59d4f1a3 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -2107,6 +2107,55 @@ parse_lsp_addrs(struct ovn_port *op) } } +static struct ovn_port * +create_cr_port(struct ovn_port *op, st
Re: [ovs-dev] [PATCH v9 ovn 3/3] ofctrl: Introduce ecmp_nexthop_monitor.
Thanks a bunch, Lorenzo! Acked-by: Mark Michelson On 8/2/24 09:24, Lorenzo Bianconi wrote: Introduce ecmp_nexthop_monitor in ovn-controller in order to track and flush ecmp-symmetric reply ct entires when requested by the CMS (e.g removing the related static routes). Signed-off-by: Lorenzo Bianconi --- controller/ofctrl.c | 54 + controller/ofctrl.h | 2 ++ controller/ovn-controller.c | 2 ++ tests/system-ovn.at | 8 ++ 4 files changed, 66 insertions(+) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index f9387d375..e023cab9b 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -45,6 +45,7 @@ #include "ovn/actions.h" #include "lib/extend-table.h" #include "lib/lb.h" +#include "lib/ovn-util.h" #include "openvswitch/poll-loop.h" #include "physical.h" #include "openvswitch/rconn.h" @@ -389,9 +390,16 @@ struct meter_band_entry { static struct shash meter_bands; +static unsigned long *ecmp_nexthop_ids; + static void ofctrl_meter_bands_destroy(void); static void ofctrl_meter_bands_clear(void); +static void ecmp_nexthop_monitor_run( +const struct sbrec_ecmp_nexthop_table *enh_table, +struct ovs_list *msgs); + + /* MFF_* field ID for our Geneve option. In S_TLV_TABLE_MOD_SENT, this is * the option we requested (we don't know whether we obtained it yet). In * S_CLEAR_FLOWS or S_UPDATE_FLOWS, this is really the option we have. */ @@ -430,6 +438,7 @@ ofctrl_init(struct ovn_extend_table *group_table, groups = group_table; meters = meter_table; shash_init(&meter_bands); +ecmp_nexthop_ids = bitmap_allocate(ECMP_NEXTHOP_IDS_LEN); } /* S_NEW, for a new connection. @@ -877,6 +886,7 @@ ofctrl_destroy(void) expr_symtab_destroy(&symtab); shash_destroy(&symtab); ofctrl_meter_bands_destroy(); +bitmap_free(ecmp_nexthop_ids); } uint64_t @@ -2306,6 +2316,47 @@ add_meter(struct ovn_extend_table_info *m_desired, ofctrl_meter_bands_alloc(sb_meter, m_desired, msgs); } +static void +ecmp_nexthop_monitor_flush_ct_entry(uint64_t id, struct ovs_list *msgs) +{ +ovs_u128 mask = { +/* ct_labels.label BITS[96-127] */ +.u64.hi = 0x, +}; +ovs_u128 nexthop = { +.u64.hi = id << 32, +}; +struct ofp_ct_match match = { +.labels = nexthop, +.labels_mask = mask, +}; +struct ofpbuf *msg = ofp_ct_match_encode(&match, NULL, + rconn_get_version(swconn)); +ovs_list_push_back(msgs, &msg->list_node); +} + +static void +ecmp_nexthop_monitor_run(const struct sbrec_ecmp_nexthop_table *enh_table, + struct ovs_list *msgs) +{ +unsigned long *ids = bitmap_allocate(ECMP_NEXTHOP_IDS_LEN); + +const struct sbrec_ecmp_nexthop *sbrec_ecmp_nexthop; +SBREC_ECMP_NEXTHOP_TABLE_FOR_EACH (sbrec_ecmp_nexthop, enh_table) { +bitmap_set1(ids, sbrec_ecmp_nexthop->id); +} + +int id; +BITMAP_FOR_EACH_1 (id, ECMP_NEXTHOP_IDS_LEN, ecmp_nexthop_ids) { +if (!bitmap_is_set(ids, id)) { +ecmp_nexthop_monitor_flush_ct_entry(id, msgs); +} +} + +bitmap_free(ecmp_nexthop_ids); +ecmp_nexthop_ids = ids; +} + static void installed_flow_add(struct ovn_flow *d, struct ofputil_bundle_ctrl_msg *bc, @@ -2664,6 +2715,7 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table, struct shash *pending_ct_zones, struct hmap *pending_lb_tuples, struct ovsdb_idl_index *sbrec_meter_by_name, + const struct sbrec_ecmp_nexthop_table *enh_table, uint64_t req_cfg, bool lflows_changed, bool pflows_changed) @@ -2704,6 +2756,8 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table, /* OpenFlow messages to send to the switch to bring it up-to-date. */ struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs); +ecmp_nexthop_monitor_run(enh_table, &msgs); + /* Iterate through ct zones that need to be flushed. */ struct shash_node *iter; SHASH_FOR_EACH(iter, pending_ct_zones) { diff --git a/controller/ofctrl.h b/controller/ofctrl.h index 129e3b6ad..33953a8a4 100644 --- a/controller/ofctrl.h +++ b/controller/ofctrl.h @@ -31,6 +31,7 @@ struct ofpbuf; struct ovsrec_bridge; struct ovsrec_open_vswitch_table; struct sbrec_meter_table; +struct sbrec_ecmp_nexthop_table; struct shash; struct ovn_desired_flow_table { @@ -59,6 +60,7 @@ void ofctrl_put(struct ovn_desired_flow_table *lflow_table, struct shash *pending_ct_zones, struct hmap *pending_lb_tuples, struct ovsdb_idl_index *sbrec_meter_by_name, +const struct sbrec_ecmp_nexthop_table *enh_t
Re: [ovs-dev] [PATCH v9 ovn 1/3] northd: Introduce ECMP_Nexthop table in SB db.
Thanks Lorenzo! Acked-by: Mark Michelson On 8/2/24 09:24, Lorenzo Bianconi wrote: Introduce ECMP_Nexthop table in the SB db in order to track active ecmp-symmetric-reply connections and flush stale ones. Signed-off-by: Lorenzo Bianconi --- lib/ovn-util.h | 2 ++ northd/en-northd.c | 35 northd/en-northd.h | 4 +++ northd/inc-proc-northd.c | 7 +++- northd/northd.c | 70 northd/northd.h | 10 ++ ovn-sb.ovsschema | 18 +-- ovn-sb.xml | 31 ++ tests/ovn-northd.at | 4 +++ 9 files changed, 178 insertions(+), 3 deletions(-) diff --git a/lib/ovn-util.h b/lib/ovn-util.h index ae971ce5a..6963b85dc 100644 --- a/lib/ovn-util.h +++ b/lib/ovn-util.h @@ -38,6 +38,8 @@ #define STT_TUNNEL_OVERHEAD 18 #define VXLAN_TUNNEL_OVERHEAD 30 +#define ECMP_NEXTHOP_IDS_LEN 65535 + struct eth_addr; struct nbrec_logical_router_port; struct ovsrec_flow_sample_collector_set_table; diff --git a/northd/en-northd.c b/northd/en-northd.c index dd2b4353d..6358ae4a5 100644 --- a/northd/en-northd.c +++ b/northd/en-northd.c @@ -404,6 +404,25 @@ en_bfd_sync_run(struct engine_node *node, void *data) engine_set_node_state(node, EN_UPDATED); } +void +en_ecmp_nexthop_run(struct engine_node *node, void *data) +{ +const struct engine_context *eng_ctx = engine_get_context(); +struct static_routes_data *static_routes_data = +engine_get_input_data("static_routes", node); +struct ecmp_nexthop_data *enh_data = data; +const struct sbrec_ecmp_nexthop_table *sbrec_ecmp_nexthop_table = +EN_OVSDB_GET(engine_get_input("SB_ecmp_nexthop", node)); + +ecmp_nexthop_destroy(data); +ecmp_nexthop_init(data); +build_ecmp_nexthop_table(eng_ctx->ovnsb_idl_txn, + &static_routes_data->parsed_routes, + &enh_data->nexthops, + sbrec_ecmp_nexthop_table); +engine_set_node_state(node, EN_UPDATED); +} + void *en_northd_init(struct engine_node *node OVS_UNUSED, struct engine_arg *arg OVS_UNUSED) @@ -454,6 +473,16 @@ void return data; } +void +*en_ecmp_nexthop_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED) +{ +struct ecmp_nexthop_data *data = xzalloc(sizeof *data); + +ecmp_nexthop_init(data); +return data; +} + void en_northd_cleanup(void *data) { @@ -490,3 +519,9 @@ en_bfd_sync_cleanup(void *data) { bfd_destroy(data); } + +void +en_ecmp_nexthop_cleanup(void *data) +{ +ecmp_nexthop_destroy(data); +} diff --git a/northd/en-northd.h b/northd/en-northd.h index f2d0ce8ba..48b9f1d7e 100644 --- a/northd/en-northd.h +++ b/northd/en-northd.h @@ -41,5 +41,9 @@ bool bfd_sync_northd_change_handler(struct engine_node *node, void *data OVS_UNUSED); void en_bfd_sync_run(struct engine_node *node, void *data); void en_bfd_sync_cleanup(void *data OVS_UNUSED); +void en_ecmp_nexthop_run(struct engine_node *node, void *data); +void *en_ecmp_nexthop_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED); +void en_ecmp_nexthop_cleanup(void *data); #endif /* EN_NORTHD_H */ diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 7d4c28b13..f00ad3643 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -101,7 +101,8 @@ static unixctl_cb_func chassis_features_list; SB_NODE(fdb, "fdb") \ SB_NODE(static_mac_binding, "static_mac_binding") \ SB_NODE(chassis_template_var, "chassis_template_var") \ -SB_NODE(logical_dp_group, "logical_dp_group") +SB_NODE(logical_dp_group, "logical_dp_group") \ +SB_NODE(ecmp_nexthop, "ecmp_nexthop") enum sb_engine_node { #define SB_NODE(NAME, NAME_STR) SB_##NAME, @@ -159,6 +160,7 @@ static ENGINE_NODE(route_policies, "route_policies"); static ENGINE_NODE(static_routes, "static_routes"); static ENGINE_NODE(bfd, "bfd"); static ENGINE_NODE(bfd_sync, "bfd_sync"); +static ENGINE_NODE(ecmp_nexthop, "ecmp_nexthop"); void inc_proc_northd_init(struct ovsdb_idl_loop *nb, struct ovsdb_idl_loop *sb) @@ -258,6 +260,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_bfd_sync, &en_route_policies, NULL); engine_add_input(&en_bfd_sync, &en_northd, bfd_sync_northd_change_handler); +engine_add_input(&en_ecmp_nexthop, &en_sb_ecmp_nexthop, NULL); +engine_add_input(&en_ecmp_nexthop, &en_static_routes, NULL); + engine_add_input(&en_sync_meters, &en_nb_acl, NULL); engine_add_input(&en_sync_meters,
[ovs-dev] [RFC ovn] Documentation: Define experimental features.
During a recent OVN community call, it was questioned what it means for a feature to be marked experimental. This documentation change aims to clarify what it means when a feature is marked experimental. Signed-off-by: Mark Michelson --- Documentation/faq/general.rst | 48 +++ 1 file changed, 48 insertions(+) diff --git a/Documentation/faq/general.rst b/Documentation/faq/general.rst index 831ca0445..a6292acd0 100644 --- a/Documentation/faq/general.rst +++ b/Documentation/faq/general.rst @@ -119,3 +119,51 @@ Q: How can I contribute to the OVN Community? questions. You can also suggest improvements to documentation. If you have a feature or bug you would like to work on, send a mail to one of the :doc:`mailing lists `. + +Q: What does it mean when a feature is marked "experimental"? + +A: Experimental features are marked this way because of one of +several reasons: + +* The developer was only able to test the feature in a limited + environment. Therefore the feature may not always work as intended + in all environments. + +* During review, the potential for failure was noticed, but the + circumstances that would lead to that failure were hard to nail + down or were strictly theoretical. + +* What exists in OVN may be an early version of a more fleshed-out + feature to come in a later version. + +* The feature was developed against a draft RFC that is subject to + change when the RFC is published. + +* The feature was developed based on observations of how a specific + vendor implements a feature, rather than using IETF standards or + other documentated specifications. + +A feature may be declared experimental for other reasons as well, +but the above are the most common. When a feature is marked +experimental, it has the following properties: + +* The feature must be opt-in. The feature must be disabled by + default. When the feature is disabled, it must have no bearing + on other OVN functionality. + +* Configuration and implementation details of the feature are + subject to change between major or minor versions of OVN. + +* Users make use of this feature at their own risk. Users are free + to file issues against the feature, but developers are more likely + to prioritize work on non-experimental features first. + +* Experimental features may be removed. For instance, if an + experimental feature exposes a security risk, it may be removed + rather than repaired. + +The hope is that experimental features will eventually lose the +"experimental" marker and become a core feature. However, there is +no specific test or process defined for when a feature no longer +needs to be considered experimental. This typically will be decided +collectively by OVN maintainers. -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev