Re: [ovs-dev] [PATCH ovn 2/2] controller: Flush CT for removed LB backends
On 2/15/23 16:40, Ales Musil wrote:
>>> +void
>>> +ovn_lb_five_tuple_find_and_delete(struct hmap *tuples,
>>> + const struct ovn_lb_five_tuple *tuple)
>>> +{
>>> +uint32_t hash = ovn_lb_five_tuple_hash(tuple);
>>> +
>>> +struct ovn_lb_five_tuple *node;
>>> +HMAP_FOR_EACH_WITH_HASH (node, hmap_node, hash, tuples) {
>>> +if (ipv6_addr_equals(>vip_ip, >vip_ip) &&
>>> +ipv6_addr_equals(>backend_ip, >backend_ip) &&
>>> +tuple->vip_port == node->vip_port &&
>>> +tuple->backend_port == node->backend_port &&
>>> +tuple->proto == node->proto) {
>>> +break;
>> I'd change this to:
>>
>> hmap_remove(tuples, >hmap_node);
>> free(node);
>> return;
>>
> Is it safe to remove within HMAP_FOR_EACH_WITH_HASH?
>
We return so it should be.
Regards,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/2] controller: Flush CT for removed LB backends
On Wed, Feb 15, 2023 at 3:19 PM Dumitru Ceara wrote:
> On 1/24/23 12:46, Ales Musil wrote:
> > Remove CT for LB backends that are affected by any
> > LB change. This way there shouldn't be any stale entry
> > in the CT. This feature is enabled by default but is dependent
> > on OvS support for CT flush via OFP extension.
> >
> > Reported-at: https://bugzilla.redhat.com/1839103
> > Signed-off-by: Ales Musil
> > ---
>
> Hi, Ales,
>
> Thanks for working on this, it solves a problem we've been having for
> along time!
>
> A few comments inline.
>
Hi Dumitru,
thank you for the review. Everything should be addressed in v2.
>
> > controller/ofctrl.c | 43
> > controller/ofctrl.h | 1 +
> > controller/ovn-controller.c | 61 +++-
> > include/ovn/features.h | 2 +
> > lib/features.c | 5 +
> > lib/lb.c| 70 +
> > lib/lb.h| 19
> > tests/ovn.at| 65
> > tests/system-ovn.at | 190
> > 9 files changed, 455 insertions(+), 1 deletion(-)
> >
> > diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> > index 6c631ea41..d922f3206 100644
> > --- a/controller/ofctrl.c
> > +++ b/controller/ofctrl.c
> > @@ -30,6 +30,7 @@
> > #include "openvswitch/match.h"
> > #include "openvswitch/ofp-actions.h"
> > #include "openvswitch/ofp-bundle.h"
> > +#include "openvswitch/ofp-ct.h"
> > #include "openvswitch/ofp-flow.h"
> > #include "openvswitch/ofp-group.h"
> > #include "openvswitch/ofp-match.h"
> > @@ -43,6 +44,7 @@
> > #include "ovn-controller.h"
> > #include "ovn/actions.h"
> > #include "lib/extend-table.h"
> > +#include "lib/lb.h"
> > #include "openvswitch/poll-loop.h"
> > #include "physical.h"
> > #include "openvswitch/rconn.h"
> > @@ -2485,6 +2487,38 @@ update_installed_flows_by_track(struct
> ovn_desired_flow_table *flow_table,
> > }
> > }
> >
> > +static void
> > +add_ct_flush_tuple(const struct ovn_lb_five_tuple *tuple,
> > + struct ovs_list *msgs)
> > +{
> > +if (VLOG_IS_DBG_ENABLED()) {
> > +struct ds ds = DS_EMPTY_INITIALIZER;
> > +
> > +ds_put_cstr(, "Flushing CT for 5-tuple: vip=");
> > +ipv6_format_mapped(>vip_ip, );
> > +ds_put_format(, ":%"PRIu16", backend=", tuple->vip_port);
> > +ipv6_format_mapped(>backend_ip, );
> > +ds_put_format(, ":%"PRIu16", protocol=%"PRIu8,
> > + tuple->backend_port, tuple->proto);
> > +VLOG_DBG("%s", ds_cstr());
> > +
> > +ds_destroy();
> > +}
> > +
> > +struct ofp_ct_match match = {0};
> > +
> > +match.ip_proto = tuple->proto;
> > +
> > +match.tuple_orig.dst = tuple->vip_ip;
> > +match.tuple_orig.dst_port = htons(tuple->vip_port);
> > +
> > +match.tuple_reply.src = tuple->backend_ip;
> > +match.tuple_reply.src_port = htons(tuple->backend_port);
> > +
>
> Isn't this simpler?
>
> struct ofp_ct_match match = {
> .ip_proto = tuple->proto,
> .tuple_orig.dst = tuple->vip_ip,
> .tuple_orig.dst_port = htons(tuple->vip_port),
> .tuple_reply.src = tuple->backend_ip,
> .tuple_reply.src_port = htons(tuple->backend_port),
> };
>
It certainly is. Even easier to read. :)
>
> > +struct ofpbuf *msg = ofp_ct_match_encode(, NULL,
> OFP15_VERSION);
> > +ovs_list_push_back(msgs, >list_node);
> > +}
> > +
> > bool
> > ofctrl_has_backlog(void)
> > {
> > @@ -2524,6 +2558,7 @@ void
> > ofctrl_put(struct ovn_desired_flow_table *lflow_table,
> > struct ovn_desired_flow_table *pflow_table,
> > struct shash *pending_ct_zones,
> > + struct hmap *pending_lb_tuples,
> > const struct sbrec_meter_table *meter_table,
> > uint64_t req_cfg,
> > bool lflows_changed,
> > @@ -2754,6 +2789,14 @@ ofctrl_put(struct ovn_desired_flow_table
> *lflow_table,
> > /* Sync the contents of meters->desired to meters->existing. */
> > ovn_extend_table_sync(meters);
> >
> > +if (ovs_feature_is_supported(OVS_CT_TUPLE_FLUSH_SUPPORT)) {
> > +struct ovn_lb_five_tuple *tuple;
> > +HMAP_FOR_EACH_POP (tuple, hmap_node, pending_lb_tuples) {
> > +add_ct_flush_tuple(tuple, );
> > +free(tuple);
> > +}
> > +}
> > +
> > if (!ovs_list_is_empty()) {
> > /* Add a barrier to the list of messages. */
> > struct ofpbuf *barrier =
> ofputil_encode_barrier_request(OFP15_VERSION);
> > diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> > index 396824512..f5751e3ee 100644
> > --- a/controller/ofctrl.h
> > +++ b/controller/ofctrl.h
> > @@ -58,6 +58,7 @@ enum mf_field_id ofctrl_get_mf_field_id(void);
> > void ofctrl_put(struct ovn_desired_flow_table *lflow_table,
> > struct ovn_desired_flow_table *pflow_table,
> > struct shash *pending_ct_zones,
> > +
Re: [ovs-dev] [PATCH ovn 2/2] controller: Flush CT for removed LB backends
On 1/24/23 12:46, Ales Musil wrote:
> Remove CT for LB backends that are affected by any
> LB change. This way there shouldn't be any stale entry
> in the CT. This feature is enabled by default but is dependent
> on OvS support for CT flush via OFP extension.
>
> Reported-at: https://bugzilla.redhat.com/1839103
> Signed-off-by: Ales Musil
> ---
Hi, Ales,
Thanks for working on this, it solves a problem we've been having for
along time!
A few comments inline.
> controller/ofctrl.c | 43
> controller/ofctrl.h | 1 +
> controller/ovn-controller.c | 61 +++-
> include/ovn/features.h | 2 +
> lib/features.c | 5 +
> lib/lb.c| 70 +
> lib/lb.h| 19
> tests/ovn.at| 65
> tests/system-ovn.at | 190
> 9 files changed, 455 insertions(+), 1 deletion(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 6c631ea41..d922f3206 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -30,6 +30,7 @@
> #include "openvswitch/match.h"
> #include "openvswitch/ofp-actions.h"
> #include "openvswitch/ofp-bundle.h"
> +#include "openvswitch/ofp-ct.h"
> #include "openvswitch/ofp-flow.h"
> #include "openvswitch/ofp-group.h"
> #include "openvswitch/ofp-match.h"
> @@ -43,6 +44,7 @@
> #include "ovn-controller.h"
> #include "ovn/actions.h"
> #include "lib/extend-table.h"
> +#include "lib/lb.h"
> #include "openvswitch/poll-loop.h"
> #include "physical.h"
> #include "openvswitch/rconn.h"
> @@ -2485,6 +2487,38 @@ update_installed_flows_by_track(struct
> ovn_desired_flow_table *flow_table,
> }
> }
>
> +static void
> +add_ct_flush_tuple(const struct ovn_lb_five_tuple *tuple,
> + struct ovs_list *msgs)
> +{
> +if (VLOG_IS_DBG_ENABLED()) {
> +struct ds ds = DS_EMPTY_INITIALIZER;
> +
> +ds_put_cstr(, "Flushing CT for 5-tuple: vip=");
> +ipv6_format_mapped(>vip_ip, );
> +ds_put_format(, ":%"PRIu16", backend=", tuple->vip_port);
> +ipv6_format_mapped(>backend_ip, );
> +ds_put_format(, ":%"PRIu16", protocol=%"PRIu8,
> + tuple->backend_port, tuple->proto);
> +VLOG_DBG("%s", ds_cstr());
> +
> +ds_destroy();
> +}
> +
> +struct ofp_ct_match match = {0};
> +
> +match.ip_proto = tuple->proto;
> +
> +match.tuple_orig.dst = tuple->vip_ip;
> +match.tuple_orig.dst_port = htons(tuple->vip_port);
> +
> +match.tuple_reply.src = tuple->backend_ip;
> +match.tuple_reply.src_port = htons(tuple->backend_port);
> +
Isn't this simpler?
struct ofp_ct_match match = {
.ip_proto = tuple->proto,
.tuple_orig.dst = tuple->vip_ip,
.tuple_orig.dst_port = htons(tuple->vip_port),
.tuple_reply.src = tuple->backend_ip,
.tuple_reply.src_port = htons(tuple->backend_port),
};
> +struct ofpbuf *msg = ofp_ct_match_encode(, NULL, OFP15_VERSION);
> +ovs_list_push_back(msgs, >list_node);
> +}
> +
> bool
> ofctrl_has_backlog(void)
> {
> @@ -2524,6 +2558,7 @@ void
> ofctrl_put(struct ovn_desired_flow_table *lflow_table,
> struct ovn_desired_flow_table *pflow_table,
> struct shash *pending_ct_zones,
> + struct hmap *pending_lb_tuples,
> const struct sbrec_meter_table *meter_table,
> uint64_t req_cfg,
> bool lflows_changed,
> @@ -2754,6 +2789,14 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
> /* Sync the contents of meters->desired to meters->existing. */
> ovn_extend_table_sync(meters);
>
> +if (ovs_feature_is_supported(OVS_CT_TUPLE_FLUSH_SUPPORT)) {
> +struct ovn_lb_five_tuple *tuple;
> +HMAP_FOR_EACH_POP (tuple, hmap_node, pending_lb_tuples) {
> +add_ct_flush_tuple(tuple, );
> +free(tuple);
> +}
> +}
> +
> if (!ovs_list_is_empty()) {
> /* Add a barrier to the list of messages. */
> struct ofpbuf *barrier =
> ofputil_encode_barrier_request(OFP15_VERSION);
> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> index 396824512..f5751e3ee 100644
> --- a/controller/ofctrl.h
> +++ b/controller/ofctrl.h
> @@ -58,6 +58,7 @@ enum mf_field_id ofctrl_get_mf_field_id(void);
> void ofctrl_put(struct ovn_desired_flow_table *lflow_table,
> struct ovn_desired_flow_table *pflow_table,
> struct shash *pending_ct_zones,
> +struct hmap *pending_lb_tuples,
> const struct sbrec_meter_table *,
> uint64_t nb_cfg,
> bool lflow_changed,
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index b3170973a..ec373215b 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2660,6 +2660,8 @@ load_balancers_by_dp_cleanup(struct hmap *lbs)
> struct
[ovs-dev] [PATCH ovn 2/2] controller: Flush CT for removed LB backends
Remove CT for LB backends that are affected by any
LB change. This way there shouldn't be any stale entry
in the CT. This feature is enabled by default but is dependent
on OvS support for CT flush via OFP extension.
Reported-at: https://bugzilla.redhat.com/1839103
Signed-off-by: Ales Musil
---
controller/ofctrl.c | 43
controller/ofctrl.h | 1 +
controller/ovn-controller.c | 61 +++-
include/ovn/features.h | 2 +
lib/features.c | 5 +
lib/lb.c| 70 +
lib/lb.h| 19
tests/ovn.at| 65
tests/system-ovn.at | 190
9 files changed, 455 insertions(+), 1 deletion(-)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 6c631ea41..d922f3206 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -30,6 +30,7 @@
#include "openvswitch/match.h"
#include "openvswitch/ofp-actions.h"
#include "openvswitch/ofp-bundle.h"
+#include "openvswitch/ofp-ct.h"
#include "openvswitch/ofp-flow.h"
#include "openvswitch/ofp-group.h"
#include "openvswitch/ofp-match.h"
@@ -43,6 +44,7 @@
#include "ovn-controller.h"
#include "ovn/actions.h"
#include "lib/extend-table.h"
+#include "lib/lb.h"
#include "openvswitch/poll-loop.h"
#include "physical.h"
#include "openvswitch/rconn.h"
@@ -2485,6 +2487,38 @@ update_installed_flows_by_track(struct
ovn_desired_flow_table *flow_table,
}
}
+static void
+add_ct_flush_tuple(const struct ovn_lb_five_tuple *tuple,
+ struct ovs_list *msgs)
+{
+if (VLOG_IS_DBG_ENABLED()) {
+struct ds ds = DS_EMPTY_INITIALIZER;
+
+ds_put_cstr(, "Flushing CT for 5-tuple: vip=");
+ipv6_format_mapped(>vip_ip, );
+ds_put_format(, ":%"PRIu16", backend=", tuple->vip_port);
+ipv6_format_mapped(>backend_ip, );
+ds_put_format(, ":%"PRIu16", protocol=%"PRIu8,
+ tuple->backend_port, tuple->proto);
+VLOG_DBG("%s", ds_cstr());
+
+ds_destroy();
+}
+
+struct ofp_ct_match match = {0};
+
+match.ip_proto = tuple->proto;
+
+match.tuple_orig.dst = tuple->vip_ip;
+match.tuple_orig.dst_port = htons(tuple->vip_port);
+
+match.tuple_reply.src = tuple->backend_ip;
+match.tuple_reply.src_port = htons(tuple->backend_port);
+
+struct ofpbuf *msg = ofp_ct_match_encode(, NULL, OFP15_VERSION);
+ovs_list_push_back(msgs, >list_node);
+}
+
bool
ofctrl_has_backlog(void)
{
@@ -2524,6 +2558,7 @@ void
ofctrl_put(struct ovn_desired_flow_table *lflow_table,
struct ovn_desired_flow_table *pflow_table,
struct shash *pending_ct_zones,
+ struct hmap *pending_lb_tuples,
const struct sbrec_meter_table *meter_table,
uint64_t req_cfg,
bool lflows_changed,
@@ -2754,6 +2789,14 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
/* Sync the contents of meters->desired to meters->existing. */
ovn_extend_table_sync(meters);
+if (ovs_feature_is_supported(OVS_CT_TUPLE_FLUSH_SUPPORT)) {
+struct ovn_lb_five_tuple *tuple;
+HMAP_FOR_EACH_POP (tuple, hmap_node, pending_lb_tuples) {
+add_ct_flush_tuple(tuple, );
+free(tuple);
+}
+}
+
if (!ovs_list_is_empty()) {
/* Add a barrier to the list of messages. */
struct ofpbuf *barrier = ofputil_encode_barrier_request(OFP15_VERSION);
diff --git a/controller/ofctrl.h b/controller/ofctrl.h
index 396824512..f5751e3ee 100644
--- a/controller/ofctrl.h
+++ b/controller/ofctrl.h
@@ -58,6 +58,7 @@ enum mf_field_id ofctrl_get_mf_field_id(void);
void ofctrl_put(struct ovn_desired_flow_table *lflow_table,
struct ovn_desired_flow_table *pflow_table,
struct shash *pending_ct_zones,
+struct hmap *pending_lb_tuples,
const struct sbrec_meter_table *,
uint64_t nb_cfg,
bool lflow_changed,
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index b3170973a..ec373215b 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -2660,6 +2660,8 @@ load_balancers_by_dp_cleanup(struct hmap *lbs)
struct ed_type_lb_data {
/* Locally installed 'struct ovn_controller_lb' by UUID. */
struct hmap local_lbs;
+/* 'struct ovn_lb_five_tuple' removed during last run. */
+struct hmap removed_tuples;
/* Load balancer <-> resource cross reference */
struct objdep_mgr deps_mgr;
/* Objects processed in the current engine execution.
@@ -2684,6 +2686,48 @@ struct lb_data_ctx_in {
const struct smap *template_vars;
};
+static void
+lb_data_removed_five_tuples_add(struct ed_type_lb_data *lb_data,
+const struct ovn_controller_lb *lb)
+{
+if (!ovs_feature_is_supported(OVS_CT_TUPLE_FLUSH_SUPPORT)) {
+return;
+}
+
+for (size_t i
