Re: [ovs-dev] [PATCH ovn v4 2/2] northd, controller: Use paused controller action for packet buffering.
On Wed, Apr 17, 2024 at 9:09 AM Ales Musil wrote:
> The current packet injection loses ct_state in the process. When
> the ct_state is lost we might commit to DNAT zone and perform
> zero SNAT after the packet injection. This causes the first session
> to be wrong as the reply packets are not unDNATted.
>
> Instead of re-injecting the packet back into the pipeline when
> we get the MAC binding, use paused controller action. The paused
> controller action stores ct_state, which is important for the behavior
> of the resumed packet.
>
> At the same time bump the OvS submodule latest branch-3.3. This is
> mainly for [0], which fixes metering for paused controller actions.
>
> In order to make sure that the paused action works during upgrade add
> the output implicitly. Once the upgrade is done northd will create option
> to inform controllers that the implicit action is no longer needed.
>
> [0] c560f6ca3257 ("ofproto-dpif-xlate: Fix continuations with associated
> metering.")
>
> Reported-at: https://issues.redhat.com/browse/FDP-439
> Signed-off-by: Ales Musil
> ---
> v4: Fix copy paste error in the global_handler.
> v3: Rebase on top of main.
> Add flag to ensure that the paused action works during upgrade.
> v2: Fix the Jira link and add ack from Mark.
> ---
> controller/lflow.c | 1 +
> controller/lflow.h | 1 +
> controller/mac-learn.c | 30
> controller/mac-learn.h | 9 ++--
> controller/ovn-controller.c | 21
> controller/pinctrl.c| 64 +
> include/ovn/actions.h | 3 ++
> lib/actions.c | 47 +-
> northd/en-global-config.c | 4 ++
> northd/northd.c | 6 +--
> ovs | 2 +-
> tests/multinode.at | 8
> tests/ovn-northd.at | 3 ++
> tests/ovn.at| 8 ++--
> tests/system-ovn.at | 95 +
> tests/test-ovn.c| 1 +
> 16 files changed, 240 insertions(+), 63 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 895d17d19..760ec0b41 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -874,6 +874,7 @@ add_matches_to_flow_table(const struct
> sbrec_logical_flow *lflow,
> .collector_ids = l_ctx_in->collector_ids,
> .lflow_uuid = lflow->header_.uuid,
> .dp_key = ldp->datapath->tunnel_key,
> +.explicit_arp_ns_output = l_ctx_in->explicit_arp_ns_output,
>
> .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
> .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE,
> diff --git a/controller/lflow.h b/controller/lflow.h
> index 9b7ffa19c..295d004f4 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -130,6 +130,7 @@ struct lflow_ctx_in {
> bool lb_hairpin_use_ct_mark;
> bool localnet_learn_fdb;
> bool localnet_learn_fdb_changed;
> +bool explicit_arp_ns_output;
> };
>
> struct lflow_ctx_out {
> diff --git a/controller/mac-learn.c b/controller/mac-learn.c
> index 071f01b4f..0c3b60c23 100644
> --- a/controller/mac-learn.c
> +++ b/controller/mac-learn.c
> @@ -199,15 +199,24 @@ ovn_fdb_add(struct hmap *fdbs, uint32_t dp_key,
> struct eth_addr mac,
> /* packet buffering functions */
>
> struct packet_data *
> -ovn_packet_data_create(struct ofpbuf ofpacts,
> - const struct dp_packet *original_packet)
> +ovn_packet_data_create(const struct ofputil_packet_in *pin,
> + const struct ofpbuf *continuation)
> {
> struct packet_data *pd = xmalloc(sizeof *pd);
>
> -pd->ofpacts = ofpacts;
> -/* clone the packet to send it later with correct L2 address */
> -pd->p = dp_packet_clone_data(dp_packet_data(original_packet),
> - dp_packet_size(original_packet));
> +pd->pin = (struct ofputil_packet_in) {
> +.packet = xmemdup(pin->packet, pin->packet_len),
> +.packet_len = pin->packet_len,
> +.flow_metadata = pin->flow_metadata,
> +.reason = pin->reason,
> +.table_id = pin->table_id,
> +.cookie = pin->cookie,
> +/* Userdata are empty on purpose,
> + * it is not needed for the continuation. */
> +.userdata = NULL,
> +.userdata_len = 0,
> +};
> +pd->continuation = ofpbuf_clone(continuation);
>
> return pd;
> }
> @@ -216,8 +225,8 @@ ovn_packet_data_create(struct ofpbuf ofpacts,
> void
> ovn_packet_data_destroy(struct packet_data *pd)
> {
> -dp_packet_delete(pd->p);
> -ofpbuf_uninit(>ofpacts);
> +free(pd->pin.packet);
> +ofpbuf_delete(pd->continuation);
> free(pd);
> }
>
> @@ -307,7 +316,10 @@ ovn_buffered_packets_ctx_run(struct
> buffered_packets_ctx *ctx,
>
> struct packet_data *pd;
> LIST_FOR_EACH_POP (pd, node, >queue) {
> -struct eth_header *eth = dp_packet_data(pd->p);
> +struct dp_packet packet;
> +
[ovs-dev] [PATCH ovn v4 2/2] northd, controller: Use paused controller action for packet buffering.
The current packet injection loses ct_state in the process. When
the ct_state is lost we might commit to DNAT zone and perform
zero SNAT after the packet injection. This causes the first session
to be wrong as the reply packets are not unDNATted.
Instead of re-injecting the packet back into the pipeline when
we get the MAC binding, use paused controller action. The paused
controller action stores ct_state, which is important for the behavior
of the resumed packet.
At the same time bump the OvS submodule latest branch-3.3. This is
mainly for [0], which fixes metering for paused controller actions.
In order to make sure that the paused action works during upgrade add
the output implicitly. Once the upgrade is done northd will create option
to inform controllers that the implicit action is no longer needed.
[0] c560f6ca3257 ("ofproto-dpif-xlate: Fix continuations with associated
metering.")
Reported-at: https://issues.redhat.com/browse/FDP-439
Signed-off-by: Ales Musil
---
v4: Fix copy paste error in the global_handler.
v3: Rebase on top of main.
Add flag to ensure that the paused action works during upgrade.
v2: Fix the Jira link and add ack from Mark.
---
controller/lflow.c | 1 +
controller/lflow.h | 1 +
controller/mac-learn.c | 30
controller/mac-learn.h | 9 ++--
controller/ovn-controller.c | 21
controller/pinctrl.c| 64 +
include/ovn/actions.h | 3 ++
lib/actions.c | 47 +-
northd/en-global-config.c | 4 ++
northd/northd.c | 6 +--
ovs | 2 +-
tests/multinode.at | 8
tests/ovn-northd.at | 3 ++
tests/ovn.at| 8 ++--
tests/system-ovn.at | 95 +
tests/test-ovn.c| 1 +
16 files changed, 240 insertions(+), 63 deletions(-)
diff --git a/controller/lflow.c b/controller/lflow.c
index 895d17d19..760ec0b41 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -874,6 +874,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow
*lflow,
.collector_ids = l_ctx_in->collector_ids,
.lflow_uuid = lflow->header_.uuid,
.dp_key = ldp->datapath->tunnel_key,
+.explicit_arp_ns_output = l_ctx_in->explicit_arp_ns_output,
.pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
.ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE,
diff --git a/controller/lflow.h b/controller/lflow.h
index 9b7ffa19c..295d004f4 100644
--- a/controller/lflow.h
+++ b/controller/lflow.h
@@ -130,6 +130,7 @@ struct lflow_ctx_in {
bool lb_hairpin_use_ct_mark;
bool localnet_learn_fdb;
bool localnet_learn_fdb_changed;
+bool explicit_arp_ns_output;
};
struct lflow_ctx_out {
diff --git a/controller/mac-learn.c b/controller/mac-learn.c
index 071f01b4f..0c3b60c23 100644
--- a/controller/mac-learn.c
+++ b/controller/mac-learn.c
@@ -199,15 +199,24 @@ ovn_fdb_add(struct hmap *fdbs, uint32_t dp_key, struct
eth_addr mac,
/* packet buffering functions */
struct packet_data *
-ovn_packet_data_create(struct ofpbuf ofpacts,
- const struct dp_packet *original_packet)
+ovn_packet_data_create(const struct ofputil_packet_in *pin,
+ const struct ofpbuf *continuation)
{
struct packet_data *pd = xmalloc(sizeof *pd);
-pd->ofpacts = ofpacts;
-/* clone the packet to send it later with correct L2 address */
-pd->p = dp_packet_clone_data(dp_packet_data(original_packet),
- dp_packet_size(original_packet));
+pd->pin = (struct ofputil_packet_in) {
+.packet = xmemdup(pin->packet, pin->packet_len),
+.packet_len = pin->packet_len,
+.flow_metadata = pin->flow_metadata,
+.reason = pin->reason,
+.table_id = pin->table_id,
+.cookie = pin->cookie,
+/* Userdata are empty on purpose,
+ * it is not needed for the continuation. */
+.userdata = NULL,
+.userdata_len = 0,
+};
+pd->continuation = ofpbuf_clone(continuation);
return pd;
}
@@ -216,8 +225,8 @@ ovn_packet_data_create(struct ofpbuf ofpacts,
void
ovn_packet_data_destroy(struct packet_data *pd)
{
-dp_packet_delete(pd->p);
-ofpbuf_uninit(>ofpacts);
+free(pd->pin.packet);
+ofpbuf_delete(pd->continuation);
free(pd);
}
@@ -307,7 +316,10 @@ ovn_buffered_packets_ctx_run(struct buffered_packets_ctx
*ctx,
struct packet_data *pd;
LIST_FOR_EACH_POP (pd, node, >queue) {
-struct eth_header *eth = dp_packet_data(pd->p);
+struct dp_packet packet;
+dp_packet_use_const(, pd->pin.packet, pd->pin.packet_len);
+
+struct eth_header *eth = dp_packet_data();
eth->eth_dst = mac;
ovs_list_push_back(>ready_packets_data, >node);
diff --git a/controller/mac-learn.h b/controller/mac-learn.h
