[ovs-dev] [PATCH v10 ovn] Add a new logical switch port type - 'virtual'
From: Numan Siddique
This new type is added for the following reasons:
- When a load balancer is created in an OpenStack deployment with Octavia
service, it creates a logical port 'VIP' for the virtual ip.
- This logical port is not bound to any VIF.
- Octavia service creates a service VM (with another logical port 'P' which
belongs to the same logical switch)
- The virtual ip 'VIP' is configured on this service VM.
- This service VM provides the load balancing for the VIP with the configured
backend IPs.
- Octavia service can be configured to create few service VMs with
active-standby mode
with the active VM configured with the VIP. The VIP can move between
these service nodes.
Presently there are few problems:
- When a floating ip (externally reachable IP) is associated to the VIP and if
the compute nodes have external connectivity then the external traffic
cannot
reach the VIP using the floating ip as the VIP logical port would be down.
dnat_and_snat entry in NAT table for this vip will have 'external_mac' and
'logical_port' configured.
- The only way to make it work is to clear the 'external_mac' entry so that
the gateway chassis does the DNAT for the VIP.
To solve these problems, this patch proposes a new logical port type - virtual.
CMS when creating the logical port for the VIP, should
- set the type as 'virtual'
- configure the VIP in the options - Logical_Switch_Port.options:virtual-ip
- And set the virtual parents in the options
Logical_Switch_Port.options:virtual-parents.
These virtual parents are the one which can be configured with the VIP.
If suppose the virtual_ip is configured to 10.0.0.10 on a virtual logical port
'sw0-vip'
and the virtual_parents are set to - [sw0-p1, sw0-p2] then below logical flows
are added in the
lsp_in_arp_rsp logical switch pipeline
- table=11(ls_in_arp_rsp), priority=100,
match=(inport == "sw0-p1" && !is_chassis_resident("sw0-vip") &&
((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) ||
(arp.op == 2 && arp.spa == 10.0.0.10))),
action=(bind_vport("sw0-vip", inport); next;)
- table=11(ls_in_arp_rsp), priority=100,
match=(inport == "sw0-p2" && !is_chassis_resident("sw0-vip") &&
((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) ||
(arp.op == 2 && arp.spa == 10.0.0.10))),
action=(bind_vport("sw0-vip", inport); next;)
The action bind_vport will claim the logical port - sw0-vip on the chassis
where this action
is executed. Since the port - sw0-vip is claimed by a chassis, the
dnat_and_snat rule for
the VIP will be handled by the compute node.
Co-authored-by: Ben Pfaff
Signed-off-by: Ben Pfaff
Acked-by: Gurucharan Shetty
Signed-off-by: Numan Siddique
---
v9 -> v10
* Resubmitting targeting OVN repo.
v8 -> v9
===
* Added entry in NEWS.
v7 -> v8
===
* Applied the code suggestions from Ben.
v6 -> v7
* Resolved merge conflicts.
v5 -> v6
* Resolved conflicts after rebasing to latest master in tests/ovn.at
v4 -> v5
===
* Rebased to master to resolve merge conflicts.
v3 -> v4
===
* Addressed the review comment and removed the code in northd which
referenced the Southbound db state while adding the logical flows. Instead
using the ovn match - is_chassis_resident() - which I should have used
it in the first place.
v2 -> v3
===
* Addressed the review comments from Ben - deleted the new columns -
virtual_ip and virtual_parents from Logical_Switch_Port and instead
is making use of options column for this purpose.
v1 -> v2
* In v1, was not updating the 'put_vport_binding' struct if it already
exists in the put_vport_bindings hmap in the function -
pinctrl_handle_bind_vport().
In v2 handled it.
* Improved the if else check in binding.c when releasing the lports.
NEWS| 1 +
controller/binding.c| 30 +++-
controller/pinctrl.c| 174 +++
include/ovn/actions.h | 18 ++-
lib/actions.c | 59
lib/ovn-util.c | 1 +
northd/ovn-northd.8.xml | 61 +++-
northd/ovn-northd.c | 306 ++--
ovn-nb.xml | 45 ++
ovn-sb.ovsschema| 6 +-
ovn-sb.xml | 46 ++
tests/ovn.at| 290 +
tests/test-ovn.c| 1 +
utilities/ovn-trace.c | 3 +
14 files changed, 954 insertions(+), 87 deletions(-)
diff --git a/NEWS b/NEWS
index 293531db0..f47698470 100644
--- a/NEWS
+++ b/NEWS
@@ -38,6 +38,7 @@ Post-v2.11.0
* Support for Transport Zones, a way to separate chassis into
logical groups which results in tunnels only been formed between
members of the same transport zone(s).
+ * Support for new logical switch port type - 'virtual'.
- New QoS type "linux-netem" on Linux.
- Added
[ovs-dev] [branch 2.12] ovn-controller: Fix the chassis row recreation issue
From: Numan Siddique
Before the commit [1], ovn-controller would always recreate its
chassis row if deleted externally. After this commit, it no longer
recreates it. This is regression and needs to be fixed.
[1] - 242f1799fc22("ovn-controller: Refactor chassis.c to abstract the string
parsing")
Fixes: 242f1799fc22("ovn-controller: Refactor chassis.c to abstract the string
parsing")
Signed-off-by: Numan Siddique
(cherry picked from ovn repo commit b114775978a501dabd08bb15192940e574d45420)
---
ovn/controller/chassis.c | 4
tests/ovn-controller.at | 29 +
2 files changed, 33 insertions(+)
diff --git a/ovn/controller/chassis.c b/ovn/controller/chassis.c
index 04b98d86c..b74a42cc8 100644
--- a/ovn/controller/chassis.c
+++ b/ovn/controller/chassis.c
@@ -486,6 +486,10 @@ chassis_get_record(struct ovsdb_idl_txn *ovnsb_idl_txn,
if (!chassis_rec) {
VLOG_WARN("Could not find Chassis : stored (%s) ovs (%s)",
chassis_info_id(_state), chassis_id);
+if (ovnsb_idl_txn) {
+/* Recreate the chassis record. */
+chassis_rec = sbrec_chassis_insert(ovnsb_idl_txn);
+}
}
} else {
chassis_rec =
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 343c2abed..63b2581c0 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -292,3 +292,32 @@ as ovn-sb
OVS_APP_EXIT_AND_WAIT([ovsdb-server])
AT_CLEANUP
+
+# Checks that ovn-controller recreates its chassis record when deleted
externally.
+AT_SETUP([ovn-controller - Chassis self record])
+AT_KEYWORDS([ovn])
+ovn_init_db ovn-sb
+
+net_add n1
+sim_add hv
+as hv
+ovs-vsctl \
+-- add-br br-phys \
+-- add-br br-eth0 \
+-- add-br br-eth1 \
+-- add-br br-eth2
+ovn_attach n1 br-phys 192.168.0.1
+
+OVS_WAIT_UNTIL([test xhv = x`ovn-sbctl --columns name --bare find chassis`])
+# Delete the chassis "hv"
+ovn-sbctl chassis-del hv
+# ovn-controller should recreate its chassis row.
+OVS_WAIT_UNTIL([test xhv = x`ovn-sbctl --columns name --bare find chassis`])
+
+# Gracefully terminate daemons
+OVN_CLEANUP_SBOX([hv])
+OVN_CLEANUP_VSWITCH([main])
+as ovn-sb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+AT_CLEANUP
--
2.21.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Fix the chassis row recreation issue
On Mon, Jul 29, 2019 at 10:54 PM wrote:
> From: Numan Siddique
>
> Before the commit [1], ovn-controller would always recreate its
> chassis row if deleted externally. After this commit, it no longer
> recreates it. This is regression and needs to be fixed.
>
> [1] - 242f1799fc22("ovn-controller: Refactor chassis.c to abstract the
> string parsing")
>
> Fixes: 242f1799fc22("ovn-controller: Refactor chassis.c to abstract the
> string parsing")
> Acked-by: Dumitru Ceara
> Acked-by: Han Zhou
> Signed-off-by: Numan Siddique
>
Thanks for the reviews. I applied this to master.
Numan
> ---
> controller/chassis.c| 4
> tests/ovn-controller.at | 29 +
> 2 files changed, 33 insertions(+)
>
> diff --git a/controller/chassis.c b/controller/chassis.c
> index 8d9f7c8d0..937c5574b 100644
> --- a/controller/chassis.c
> +++ b/controller/chassis.c
> @@ -486,6 +486,10 @@ chassis_get_record(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> if (!chassis_rec) {
> VLOG_WARN("Could not find Chassis : stored (%s) ovs (%s)",
>chassis_info_id(_state), chassis_id);
> +if (ovnsb_idl_txn) {
> +/* Recreate the chassis record. */
> +chassis_rec = sbrec_chassis_insert(ovnsb_idl_txn);
> +}
> }
> } else {
> chassis_rec =
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 343c2abed..63b2581c0 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -292,3 +292,32 @@ as ovn-sb
> OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>
> AT_CLEANUP
> +
> +# Checks that ovn-controller recreates its chassis record when deleted
> externally.
> +AT_SETUP([ovn-controller - Chassis self record])
> +AT_KEYWORDS([ovn])
> +ovn_init_db ovn-sb
> +
> +net_add n1
> +sim_add hv
> +as hv
> +ovs-vsctl \
> +-- add-br br-phys \
> +-- add-br br-eth0 \
> +-- add-br br-eth1 \
> +-- add-br br-eth2
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +OVS_WAIT_UNTIL([test xhv = x`ovn-sbctl --columns name --bare find
> chassis`])
> +# Delete the chassis "hv"
> +ovn-sbctl chassis-del hv
> +# ovn-controller should recreate its chassis row.
> +OVS_WAIT_UNTIL([test xhv = x`ovn-sbctl --columns name --bare find
> chassis`])
> +
> +# Gracefully terminate daemons
> +OVN_CLEANUP_SBOX([hv])
> +OVN_CLEANUP_VSWITCH([main])
> +as ovn-sb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +AT_CLEANUP
> --
> 2.21.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] [ovn-northd] Don't emit ICMP need to frag on LRP with no IPv4 address
On Fri, Jul 26, 2019 at 1:52 AM Mark Michelson wrote:
> LGTM
>
> Acked-by: Mark Michelson
>
> On 7/25/19 11:38 AM, Daniel Alvarez wrote:
> > Prior to this patch, when a LRP has only IPv6 addresses, ovn-northd
> > will crash (SIGSEV) because the current code injects a flow to
> > emit the ICMP need-to-frag from its IPv4 address which does not
> > exist.
> >
> > This patch is adding a check to skip the flow installation in case
> > the port does not have any IPv4 address.
> >
> > Signed-off-by: Daniel Alvarez
>
Thanks. I applied this to master.
Can you please submit a back port patch to branch 2.12 ? I think we need
this fix.
Thanks
Numan
> > ---
> > ovn/northd/ovn-northd.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index eb6c47cad..3542ba72f 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -7705,7 +7705,8 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
> > for (size_t i = 0; i < od->nbr->n_ports; i++) {
> > struct ovn_port *rp = ovn_port_find(ports,
> >
> od->nbr->ports[i]->name);
> > -if (!rp || rp == od->l3dgw_port) {
> > +if (!rp || rp == od->l3dgw_port ||
> > +!rp->lrp_networks.ipv4_addrs) {
> > continue;
> > }
> > ds_clear();
> >
>
> ___
> 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] [ovn] OVN split: Remove make "modules_install" target
On Tue, Jul 30, 2019 at 2:40 AM Mark Michelson wrote: > Acked-by: Mark Michelson > On 7/29/19 8:59 AM, lmart...@redhat.com wrote: > > From: Lucas Alvares Gomes > > > > This patch is removing the make "modules_install" target for the new OVN > > repository. > > > > The target is not be needed for OVN (and is currently broken) becasue the > > kernel modules should be compiled and installed using the OVS repository. > > > > Signed-off-by: Lucas Alvares Gomes > Thanks. I applied this to master. Numan > > --- > > Makefile.am | 5 - > > 1 file changed, 5 deletions(-) > > > > diff --git a/Makefile.am b/Makefile.am > > index e3dea1912..2a629b425 100644 > > --- a/Makefile.am > > +++ b/Makefile.am > > @@ -484,11 +484,6 @@ install-data-local: $(INSTALL_DATA_LOCAL) > > uninstall-local: $(UNINSTALL_LOCAL) > > .PHONY: $(DIST_HOOKS) $(CLEAN_LOCAL) $(INSTALL_DATA_LOCAL) > $(UNINSTALL_LOCAL) > > > > -modules_install: > > -if LINUX_ENABLED > > - cd datapath/linux && $(MAKE) modules_install > > -endif > > - > > dist-docs: > > VERSION=$(VERSION) MAKE='$(MAKE)' $(srcdir)/build-aux/dist-docs > $(srcdir) $(docs) > > .PHONY: dist-docs > > > > ___ > 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 2/2] ovn-controller: Fix flow installation latency caused by recompute.
On Tue, Jul 30, 2019 at 9:39 AM Numan Siddique wrote:
>
>
> On Tue, Jul 30, 2019 at 7:10 AM Han Zhou wrote:
>
>> From: Han Zhou
>>
>> When there are in-flight flow-installations pending to ovs-vswitchd,
>> current incremental processing logic prioritizes new change handling.
>> However, in scenarios where frequent recomputes are triggered, the
>> later recompute would block the flow-installation for previously
>> computed flows because recompute usually takes long time, especially
>> when there are large number of flows. This results in worse latency
>> than the version without incremental processing in specific scale
>> test scenarios.
>>
>> While we can simply fix the problem by prioritizing flow installation
>> rather than new change handling, it can cause the incremental
>> processing to degrade to always recompute in certain scenarios when
>> there are some changes triggering recomputes, followed by a lot of
>> continously coming changes that can be handled incrementally. Because
>> OVSDB change tracker cannot preserve changes across iterations, once
>> the recompute is triggered and resulted in a lot of pending messages
>> to ovs-vswitchd, and if we choose to skip the engine_run()
>> in the next iteration when a incrementally processible change comes,
>> we miss the opportunity to handle that tracked change and will have
>> to trigger recompute again in the next next iteration, and so on, if
>> such changes come continously.
>>
>> This patch solves the problem by introducing engine_set_abort_recompute(),
>> so that we can prioritize new change handling if the change can be
>> incrementally processed, but if the change triggers recompute, we
>> abort there without spending CPU on the recompute to avoid blocking
>> the previous computed flow installation.
>>
>> Reported-by: Daniel Alvarez Sanchez
>> Reported-by: Numan Siddique
>> Reported-at:
>> https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048822.html
>> Tested-by: Numan Siddique
>> Acked-by: Numan Siddique
>> Acked-by: Mark Michelson
>> Signed-off-by: Han Zhou
>> ---
>>
>
> Thanks. I applied this series to master.
>
> Numan
>
I think we need this fix for branch 2.12, can you please post the back port
patch.
Thanks
Numan
>
>
>> controller/ofctrl.c | 2 +-
>> controller/ofctrl.h | 1 +
>> controller/ovn-controller.c | 30 +++---
>> lib/inc-proc-eng.c | 26 ++
>> lib/inc-proc-eng.h | 9 +++--
>> 5 files changed, 58 insertions(+), 10 deletions(-)
>>
>> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>> index 4d24a8b..8928205 100644
>> --- a/controller/ofctrl.c
>> +++ b/controller/ofctrl.c
>> @@ -985,7 +985,7 @@ add_meter(struct ovn_extend_table_info *m_desired,
>> * in the correct state and not backlogged with existing flow_mods. (Our
>> * criteria for being backlogged appear very conservative, but the socket
>> * between ovn-controller and OVS provides some buffering.) */
>> -static bool
>> +bool
>> ofctrl_can_put(void)
>> {
>> if (state != S_UPDATE_FLOWS
>> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
>> index 114c9ef..1e9ac16 100644
>> --- a/controller/ofctrl.h
>> +++ b/controller/ofctrl.h
>> @@ -51,6 +51,7 @@ void ofctrl_put(struct ovn_desired_flow_table *,
>> const struct sbrec_meter_table *,
>> int64_t nb_cfg,
>> bool flow_changed);
>> +bool ofctrl_can_put(void);
>> void ofctrl_wait(void);
>> void ofctrl_destroy(void);
>> int64_t ofctrl_get_cur_cfg(void);
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index d3b28b9..ad33d17 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -1896,6 +1896,7 @@ main(int argc, char *argv[])
>>
>> uint64_t engine_run_id = 0;
>> uint64_t old_engine_run_id = 0;
>> +bool engine_aborted = false;
>>
>> unsigned int ovs_cond_seqno = UINT_MAX;
>> unsigned int ovnsb_cond_seqno = UINT_MAX;
>> @@ -1982,7 +1983,30 @@ main(int argc, char *argv[])
>> stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME,
>> time_msec());
>> if (ovnsb_idl_txn) {
>> -engine_run(_flow_output, ++engine_run_id);
>> +if (!ofctrl_can_put()) {
>> +/* When there are in-flight messages pending
>> to
>> + * ovs-vswitchd, we should hold on
>> recomputing so
>> + * that the previous flow installations
>> won't be
>> + * delayed. However, we still want to try if
>> + * recompute is not needed and we can quickly
>> + * incrementally process the new changes, to
>> avoid
>> + * unnecessarily forced recomputes later
>> on. This
>> + * is
Re: [ovs-dev] [PATCH ovn 2/2] ovn-controller: Fix flow installation latency caused by recompute.
On Tue, Jul 30, 2019 at 7:10 AM Han Zhou wrote:
> From: Han Zhou
>
> When there are in-flight flow-installations pending to ovs-vswitchd,
> current incremental processing logic prioritizes new change handling.
> However, in scenarios where frequent recomputes are triggered, the
> later recompute would block the flow-installation for previously
> computed flows because recompute usually takes long time, especially
> when there are large number of flows. This results in worse latency
> than the version without incremental processing in specific scale
> test scenarios.
>
> While we can simply fix the problem by prioritizing flow installation
> rather than new change handling, it can cause the incremental
> processing to degrade to always recompute in certain scenarios when
> there are some changes triggering recomputes, followed by a lot of
> continously coming changes that can be handled incrementally. Because
> OVSDB change tracker cannot preserve changes across iterations, once
> the recompute is triggered and resulted in a lot of pending messages
> to ovs-vswitchd, and if we choose to skip the engine_run()
> in the next iteration when a incrementally processible change comes,
> we miss the opportunity to handle that tracked change and will have
> to trigger recompute again in the next next iteration, and so on, if
> such changes come continously.
>
> This patch solves the problem by introducing engine_set_abort_recompute(),
> so that we can prioritize new change handling if the change can be
> incrementally processed, but if the change triggers recompute, we
> abort there without spending CPU on the recompute to avoid blocking
> the previous computed flow installation.
>
> Reported-by: Daniel Alvarez Sanchez
> Reported-by: Numan Siddique
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048822.html
> Tested-by: Numan Siddique
> Acked-by: Numan Siddique
> Acked-by: Mark Michelson
> Signed-off-by: Han Zhou
> ---
>
Thanks. I applied this series to master.
Numan
> controller/ofctrl.c | 2 +-
> controller/ofctrl.h | 1 +
> controller/ovn-controller.c | 30 +++---
> lib/inc-proc-eng.c | 26 ++
> lib/inc-proc-eng.h | 9 +++--
> 5 files changed, 58 insertions(+), 10 deletions(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 4d24a8b..8928205 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -985,7 +985,7 @@ add_meter(struct ovn_extend_table_info *m_desired,
> * in the correct state and not backlogged with existing flow_mods. (Our
> * criteria for being backlogged appear very conservative, but the socket
> * between ovn-controller and OVS provides some buffering.) */
> -static bool
> +bool
> ofctrl_can_put(void)
> {
> if (state != S_UPDATE_FLOWS
> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> index 114c9ef..1e9ac16 100644
> --- a/controller/ofctrl.h
> +++ b/controller/ofctrl.h
> @@ -51,6 +51,7 @@ void ofctrl_put(struct ovn_desired_flow_table *,
> const struct sbrec_meter_table *,
> int64_t nb_cfg,
> bool flow_changed);
> +bool ofctrl_can_put(void);
> void ofctrl_wait(void);
> void ofctrl_destroy(void);
> int64_t ofctrl_get_cur_cfg(void);
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index d3b28b9..ad33d17 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1896,6 +1896,7 @@ main(int argc, char *argv[])
>
> uint64_t engine_run_id = 0;
> uint64_t old_engine_run_id = 0;
> +bool engine_aborted = false;
>
> unsigned int ovs_cond_seqno = UINT_MAX;
> unsigned int ovnsb_cond_seqno = UINT_MAX;
> @@ -1982,7 +1983,30 @@ main(int argc, char *argv[])
> stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME,
> time_msec());
> if (ovnsb_idl_txn) {
> -engine_run(_flow_output, ++engine_run_id);
> +if (!ofctrl_can_put()) {
> +/* When there are in-flight messages pending
> to
> + * ovs-vswitchd, we should hold on
> recomputing so
> + * that the previous flow installations won't
> be
> + * delayed. However, we still want to try if
> + * recompute is not needed and we can quickly
> + * incrementally process the new changes, to
> avoid
> + * unnecessarily forced recomputes later on.
> This
> + * is because the OVSDB change tracker cannot
> + * preserve tracked changes across
> iterations. If
> + * change tracking is improved, we can simply
> skip
> + * this round of engine_run and
Re: [ovs-dev] [PATCH 05/12] ct-dpif: Add conntrack timeout policy support in dpif layer
On Mon, Jul 29, 2019 at 3:51 PM Yi-Hung Wei wrote: > On Mon, Jul 29, 2019 at 1:12 PM Darrell Ball wrote: > >> > "is_default" - can you explain this one ? > >> > >> This flag is used to configure the default timeout policy in the > >> datapath. If 'is_default' is true, it will set the provided timeout > >> policy to be the default timeout policy. The default timeout policy is > >> documented in vswitchd/vswitch.xml > >> > > > > Below is what I see the the schema xml file under timeout policy > > Please add description about the 'default' timeout policy under > > "CT_Timeout_Policy" - let me know if you need help, as I can submit a > patch. > > It is a few lines before what you pasted below. > > > Connection tracking zone configuration > > > Connection tracking timeout policy for this zone. If timeout policy is > not specified, defaults to the timeout policy in the default zone. If > the timeout policy in default zone is not specified, defaults to the > default timeouts in the system. > > > > The overall purpose of these columns is described under Common > Columns at the beginning of this document. > > > > > Since we decided to remove the default zone timeout policy defaulting, I think this all becomes moot. > > > -Yi-Hung > > > > > > Connection tracking timeout policy configuration > > > > > > > > The timeouts column contains key-value pairs used > > to configure connection tracking timeouts in a datapath. > > Key-value pairs that are not supported by a datapath are > > ignored. > > > > > > > > > > TCP SYN sent timeout. > > > > > > > > TCP SYN receive timeout. > > > > > > > > TCP established timeout. > > > > > > > > TCP FIN wait timeout. > > > > > > > > TCP close wait timeout. > > > > > > > > TCP last ACK timeout. > > > > > > > > TCP time wait timeout. > > > > > > > > TCP close timeout. > > > > > > > > TCP syn sent2 timeout. > > > > > > > > TCP retransmit timeout. > > > > > > > > TCP unacknowledgment timeout. > > > > > > > > > > > > First UDP packet timeout. > > > > > > > > The timeout in the state that source host sends more than one > packet > > but the destination host has never sent one backs. > > > > > > > > UDP packets seen in both directions timeout. > > > > > > > > > > > > First ICMP timeout. > > > > > > > > ICMP reply timeout. > > > > > > > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovn-controller: Fix flow installation latency caused by recompute.
On Mon, Jul 29, 2019 at 5:05 PM 0-day Robot wrote: > > Bleep bloop. Greetings Han Zhou, I am a robot and I have tried out your patch. > Thanks for your contribution. > > I encountered some error that I wasn't expecting. See the details below. > > > build: > depbase=`echo controller/ofctrl.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\ > gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./ovs/include -I ./ovs/include -I ./ovs/lib -I ./ovs/lib -I ./ovs -I ./ovs -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT controller/ofctrl.o -MD -MP -MF $depbase.Tpo -c -o controller/ofctrl.o controller/ofctrl.c &&\ > mv -f $depbase.Tpo $depbase.Po > depbase=`echo controller/pinctrl.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\ > gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./ovs/include -I ./ovs/include -I ./ovs/lib -I ./ovs/lib -I ./ovs -I ./ovs -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT controller/pinctrl.o -MD -MP -MF $depbase.Tpo -c -o controller/pinctrl.o controller/pinctrl.c &&\ > mv -f $depbase.Tpo $depbase.Po > depbase=`echo controller/patch.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\ > gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./ovs/include -I ./ovs/include -I ./ovs/lib -I ./ovs/lib -I ./ovs -I ./ovs -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT controller/patch.o -MD -MP -MF $depbase.Tpo -c -o controller/patch.o controller/patch.c &&\ > mv -f $depbase.Tpo $depbase.Po > depbase=`echo controller/ovn-controller.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\ > gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./ovs/include -I ./ovs/include -I ./ovs/lib -I ./ovs/lib -I ./ovs -I ./ovs -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT controller/ovn-controller.o -MD -MP -MF $depbase.Tpo -c -o controller/ovn-controller.o controller/ovn-controller.c &&\ > mv -f $depbase.Tpo $depbase.Po > controller/ovn-controller.c: In function ‘main’: > controller/ovn-controller.c:2001:33: error: implicit declaration of function ‘engine_set_abort_recompute’ [-Werror=implicit-function-declaration] > engine_set_abort_recompute(true); > ^ > controller/ovn-controller.c:2002:48: error: void value not ignored as it ought to be > engine_aborted = engine_run(_flow_output, > ^ > cc1: all warnings being treated as errors > make[2]: *** [controller/ovn-controller.o] Error 1 > make[2]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/OVN' > make[1]: *** [all-recursive] Error 1 > make[1]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/OVN' > make: *** [all] Error 2 > > > Please check this out. If you feel there has been an error, please email acon...@bytheb.org > > Thanks, > 0-day Robot I was resending the patch that has been sent over OVS repo. However, the compile failed due to the header file path problem: -#include "ovn/lib/inc-proc-eng.h" +#include "lib/inc-proc-eng.h" I updated with a new patch series: https://patchwork.ozlabs.org/project/openvswitch/list/?series=122106 Thanks, Han ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 2/2] ovn-controller: Fix flow installation latency caused by recompute.
From: Han Zhou
When there are in-flight flow-installations pending to ovs-vswitchd,
current incremental processing logic prioritizes new change handling.
However, in scenarios where frequent recomputes are triggered, the
later recompute would block the flow-installation for previously
computed flows because recompute usually takes long time, especially
when there are large number of flows. This results in worse latency
than the version without incremental processing in specific scale
test scenarios.
While we can simply fix the problem by prioritizing flow installation
rather than new change handling, it can cause the incremental
processing to degrade to always recompute in certain scenarios when
there are some changes triggering recomputes, followed by a lot of
continously coming changes that can be handled incrementally. Because
OVSDB change tracker cannot preserve changes across iterations, once
the recompute is triggered and resulted in a lot of pending messages
to ovs-vswitchd, and if we choose to skip the engine_run()
in the next iteration when a incrementally processible change comes,
we miss the opportunity to handle that tracked change and will have
to trigger recompute again in the next next iteration, and so on, if
such changes come continously.
This patch solves the problem by introducing engine_set_abort_recompute(),
so that we can prioritize new change handling if the change can be
incrementally processed, but if the change triggers recompute, we
abort there without spending CPU on the recompute to avoid blocking
the previous computed flow installation.
Reported-by: Daniel Alvarez Sanchez
Reported-by: Numan Siddique
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048822.html
Tested-by: Numan Siddique
Acked-by: Numan Siddique
Acked-by: Mark Michelson
Signed-off-by: Han Zhou
---
controller/ofctrl.c | 2 +-
controller/ofctrl.h | 1 +
controller/ovn-controller.c | 30 +++---
lib/inc-proc-eng.c | 26 ++
lib/inc-proc-eng.h | 9 +++--
5 files changed, 58 insertions(+), 10 deletions(-)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 4d24a8b..8928205 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -985,7 +985,7 @@ add_meter(struct ovn_extend_table_info *m_desired,
* in the correct state and not backlogged with existing flow_mods. (Our
* criteria for being backlogged appear very conservative, but the socket
* between ovn-controller and OVS provides some buffering.) */
-static bool
+bool
ofctrl_can_put(void)
{
if (state != S_UPDATE_FLOWS
diff --git a/controller/ofctrl.h b/controller/ofctrl.h
index 114c9ef..1e9ac16 100644
--- a/controller/ofctrl.h
+++ b/controller/ofctrl.h
@@ -51,6 +51,7 @@ void ofctrl_put(struct ovn_desired_flow_table *,
const struct sbrec_meter_table *,
int64_t nb_cfg,
bool flow_changed);
+bool ofctrl_can_put(void);
void ofctrl_wait(void);
void ofctrl_destroy(void);
int64_t ofctrl_get_cur_cfg(void);
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index d3b28b9..ad33d17 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1896,6 +1896,7 @@ main(int argc, char *argv[])
uint64_t engine_run_id = 0;
uint64_t old_engine_run_id = 0;
+bool engine_aborted = false;
unsigned int ovs_cond_seqno = UINT_MAX;
unsigned int ovnsb_cond_seqno = UINT_MAX;
@@ -1982,7 +1983,30 @@ main(int argc, char *argv[])
stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME,
time_msec());
if (ovnsb_idl_txn) {
-engine_run(_flow_output, ++engine_run_id);
+if (!ofctrl_can_put()) {
+/* When there are in-flight messages pending to
+ * ovs-vswitchd, we should hold on recomputing so
+ * that the previous flow installations won't be
+ * delayed. However, we still want to try if
+ * recompute is not needed and we can quickly
+ * incrementally process the new changes, to avoid
+ * unnecessarily forced recomputes later on. This
+ * is because the OVSDB change tracker cannot
+ * preserve tracked changes across iterations. If
+ * change tracking is improved, we can simply skip
+ * this round of engine_run and continue processing
+ * acculated changes incrementally later when
+ * ofctrl_can_put() returns true. */
+if (!engine_aborted) {
+engine_set_abort_recompute(true);
+
[ovs-dev] [PATCH ovn 1/2] ovn-controller: Fix path for lib/inc-proc-eng.h after OVN split.
From: Han Zhou Signed-off-by: Han Zhou --- controller/ovn-controller.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 12c919a..d3b28b9 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -63,7 +63,7 @@ #include "timeval.h" #include "timer.h" #include "stopwatch.h" -#include "ovn/lib/inc-proc-eng.h" +#include "lib/inc-proc-eng.h" VLOG_DEFINE_THIS_MODULE(main); -- 2.1.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Good News
You have been chosen to receive the sum of $1,200,000.00 (One Million, Two Hundred Thousand United States Dollars) in my ongoing charity program. For more details, E-mail: charitylisajohnrobinson@gmail.com ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovn-controller: Fix flow installation latency caused by recompute.
Bleep bloop. Greetings Han Zhou, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. build: depbase=`echo controller/ofctrl.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\ gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./ovs/include -I ./ovs/include -I ./ovs/lib -I ./ovs/lib -I ./ovs -I ./ovs -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT controller/ofctrl.o -MD -MP -MF $depbase.Tpo -c -o controller/ofctrl.o controller/ofctrl.c &&\ mv -f $depbase.Tpo $depbase.Po depbase=`echo controller/pinctrl.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\ gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./ovs/include -I ./ovs/include -I ./ovs/lib -I ./ovs/lib -I ./ovs -I ./ovs -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT controller/pinctrl.o -MD -MP -MF $depbase.Tpo -c -o controller/pinctrl.o controller/pinctrl.c &&\ mv -f $depbase.Tpo $depbase.Po depbase=`echo controller/patch.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\ gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./ovs/include -I ./ovs/include -I ./ovs/lib -I ./ovs/lib -I ./ovs -I ./ovs -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT controller/patch.o -MD -MP -MF $depbase.Tpo -c -o controller/patch.o controller/patch.c &&\ mv -f $depbase.Tpo $depbase.Po depbase=`echo controller/ovn-controller.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\ gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./ovs/include -I ./ovs/include -I ./ovs/lib -I ./ovs/lib -I ./ovs -I ./ovs -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT controller/ovn-controller.o -MD -MP -MF $depbase.Tpo -c -o controller/ovn-controller.o controller/ovn-controller.c &&\ mv -f $depbase.Tpo $depbase.Po controller/ovn-controller.c: In function ‘main’: controller/ovn-controller.c:2001:33: error: implicit declaration of function ‘engine_set_abort_recompute’ [-Werror=implicit-function-declaration] engine_set_abort_recompute(true); ^ controller/ovn-controller.c:2002:48: error: void value not ignored as it ought to be engine_aborted = engine_run(_flow_output, ^ cc1: all warnings being treated as errors make[2]: *** [controller/ovn-controller.o] Error 1 make[2]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/OVN' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/OVN' make: *** [all] Error 2 Please check this out. If you feel there has been an error, please email acon...@bytheb.org Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] ovn-northd: Fix ARP respond flows flapping.
From: Han Zhou
>From ovn-controller debug log it is seen that when creating a lsp
in NB, a lflow for ARP respond is added and then deleted in SB
by northd (the flow will be added again when the port is bound
to a chassis). This causes unnecessary handling from ovn-controller.
The root cause is lsp_is_up() returns true when the column is not
set, when the lsp is just created. So northd adds the ARP respond
flow in SB lflow table. At the same time it will create port-binding
in SB without chassis binding. Then in the next iteration northd
will process that port-binding change and notice that there is no
chassis binding for this lsp, so it will set the "up" to false,
which causes northd to delete the ARP respond flow.
The fix is to make sure when "up" is not set, it is regarded as
false by default.
Signed-off-by: Han Zhou
---
northd/ovn-northd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index bed2993..020cde4 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -3495,7 +3495,7 @@ lsp_is_enabled(const struct nbrec_logical_switch_port
*lsp)
static bool
lsp_is_up(const struct nbrec_logical_switch_port *lsp)
{
-return !lsp->up || *lsp->up;
+return lsp->up && *lsp->up;
}
static bool
--
2.1.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] ovn-controller: Fix flow installation latency caused by recompute.
From: Han Zhou
When there are in-flight flow-installations pending to ovs-vswitchd,
current incremental processing logic prioritizes new change handling.
However, in scenarios where frequent recomputes are triggered, the
later recompute would block the flow-installation for previously
computed flows because recompute usually takes long time, especially
when there are large number of flows. This results in worse latency
than the version without incremental processing in specific scale
test scenarios.
While we can simply fix the problem by prioritizing flow installation
rather than new change handling, it can cause the incremental
processing to degrade to always recompute in certain scenarios when
there are some changes triggering recomputes, followed by a lot of
continously coming changes that can be handled incrementally. Because
OVSDB change tracker cannot preserve changes across iterations, once
the recompute is triggered and resulted in a lot of pending messages
to ovs-vswitchd, and if we choose to skip the engine_run()
in the next iteration when a incrementally processible change comes,
we miss the opportunity to handle that tracked change and will have
to trigger recompute again in the next next iteration, and so on, if
such changes come continously.
This patch solves the problem by introducing engine_set_abort_recompute(),
so that we can prioritize new change handling if the change can be
incrementally processed, but if the change triggers recompute, we
abort there without spending CPU on the recompute to avoid blocking
the previous computed flow installation.
Reported-by: Daniel Alvarez Sanchez
Reported-by: Numan Siddique
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048822.html
Tested-by: Numan Siddique
Acked-by: Numan Siddique
Acked-by: Mark Michelson
Signed-off-by: Han Zhou
---
controller/ofctrl.c | 2 +-
controller/ofctrl.h | 1 +
controller/ovn-controller.c | 30 +++---
lib/inc-proc-eng.c | 26 ++
lib/inc-proc-eng.h | 9 +++--
5 files changed, 58 insertions(+), 10 deletions(-)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 4d24a8b..8928205 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -985,7 +985,7 @@ add_meter(struct ovn_extend_table_info *m_desired,
* in the correct state and not backlogged with existing flow_mods. (Our
* criteria for being backlogged appear very conservative, but the socket
* between ovn-controller and OVS provides some buffering.) */
-static bool
+bool
ofctrl_can_put(void)
{
if (state != S_UPDATE_FLOWS
diff --git a/controller/ofctrl.h b/controller/ofctrl.h
index 114c9ef..1e9ac16 100644
--- a/controller/ofctrl.h
+++ b/controller/ofctrl.h
@@ -51,6 +51,7 @@ void ofctrl_put(struct ovn_desired_flow_table *,
const struct sbrec_meter_table *,
int64_t nb_cfg,
bool flow_changed);
+bool ofctrl_can_put(void);
void ofctrl_wait(void);
void ofctrl_destroy(void);
int64_t ofctrl_get_cur_cfg(void);
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 12c919a..84e46be 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1896,6 +1896,7 @@ main(int argc, char *argv[])
uint64_t engine_run_id = 0;
uint64_t old_engine_run_id = 0;
+bool engine_aborted = false;
unsigned int ovs_cond_seqno = UINT_MAX;
unsigned int ovnsb_cond_seqno = UINT_MAX;
@@ -1982,7 +1983,30 @@ main(int argc, char *argv[])
stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME,
time_msec());
if (ovnsb_idl_txn) {
-engine_run(_flow_output, ++engine_run_id);
+if (!ofctrl_can_put()) {
+/* When there are in-flight messages pending to
+ * ovs-vswitchd, we should hold on recomputing so
+ * that the previous flow installations won't be
+ * delayed. However, we still want to try if
+ * recompute is not needed and we can quickly
+ * incrementally process the new changes, to avoid
+ * unnecessarily forced recomputes later on. This
+ * is because the OVSDB change tracker cannot
+ * preserve tracked changes across iterations. If
+ * change tracking is improved, we can simply skip
+ * this round of engine_run and continue processing
+ * acculated changes incrementally later when
+ * ofctrl_can_put() returns true. */
+if (!engine_aborted) {
+engine_set_abort_recompute(true);
+
Re: [ovs-dev] [PATCH 08/12] datapath-config: Consume datapath, CT_Zone, and CT_Timeout_Policy tables
On Fri, Jul 26, 2019 at 1:22 PM William Tu wrote:
> > +static void
> > +datapath_update_ct_zone_config(struct datapath *dp, struct dpif *dpif,
> > + unsigned int idl_seqno)
> > +{
> > +const struct ovsrec_datapath *dp_cfg = dp->cfg;
> > +struct ovsrec_ct_timeout_policy *tp_cfg;
> > +struct ovsrec_ct_zone *zone_cfg;
> > +struct ct_timeout_policy *tp;
> > +struct ct_zone *zone;
> > +uint16_t zone_id;
> > +bool new_zone;
> > +size_t i;
> > +
> > +for (i = 0; i < dp_cfg->n_ct_zones; i++) {
> > +/* Update ct_zone config */
> > +zone_cfg = dp_cfg->value_ct_zones[i];
> > +zone_id = dp_cfg->key_ct_zones[i];
> > +zone = ct_zone_lookup(>ct_zones, zone_id);
> > +if (!zone) {
> > +new_zone = true;
> > +zone = ct_zone_alloc(zone_id);
> > +} else {
> > +new_zone = false;
> > +}
> > +zone->last_used_seqno = idl_seqno;
> > +
> > +/* Update timeout policy */
> missing .
> > +tp_cfg = zone_cfg->timeout_policy;
> > +tp = ct_timeout_policy_lookup(>ct_tps, _cfg->header_.uuid);
> > +if (!tp) {
> > +tp = ct_timeout_policy_alloc(tp_cfg, idl_seqno);
> > +cmap_insert(>ct_tps, >node, uuid_hash(>uuid));
> > +if (dpif) {
> > +ct_dpif_add_timeout_policy(dpif, false, >cdtp);
> > +}
> > +} else {
> > +if (ct_timeout_policy_update(tp_cfg, tp, idl_seqno)) {
> > +if (dpif) {
> > +ct_dpif_add_timeout_policy(dpif, false, >cdtp);
> > +}
> > +}
> > +}
> > +tp->last_used_seqno = idl_seqno;
> > +
> > +/* Update default timeout policy */
>
> missing . and some places below
>
> > +if (!zone_id && tp->last_updated_seqno == idl_seqno) {
> > +ct_dpif_add_timeout_policy(dpif, true, >cdtp);
>
> Do we need to check 'if (dpif)' here?
> You have the checking above, but not here. Or should we just return
> error when dpif is NULL.
Thanks for review. I will check if dpif is NULL and address the
comment issue in v2.
Thanks,
-Yi-Hung
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 05/12] ct-dpif: Add conntrack timeout policy support in dpif layer
On Mon, Jul 29, 2019 at 1:12 PM Darrell Ball wrote: >> > "is_default" - can you explain this one ? >> >> This flag is used to configure the default timeout policy in the >> datapath. If 'is_default' is true, it will set the provided timeout >> policy to be the default timeout policy. The default timeout policy is >> documented in vswitchd/vswitch.xml >> > > Below is what I see the the schema xml file under timeout policy > Please add description about the 'default' timeout policy under > "CT_Timeout_Policy" - let me know if you need help, as I can submit a patch. It is a few lines before what you pasted below. Connection tracking zone configuration Connection tracking timeout policy for this zone. If timeout policy is not specified, defaults to the timeout policy in the default zone. If the timeout policy in default zone is not specified, defaults to the default timeouts in the system. The overall purpose of these columns is described under Common Columns at the beginning of this document. -Yi-Hung > > > Connection tracking timeout policy configuration > > > > The timeouts column contains key-value pairs used > to configure connection tracking timeouts in a datapath. > Key-value pairs that are not supported by a datapath are > ignored. > > > > > TCP SYN sent timeout. > > > > TCP SYN receive timeout. > > > > TCP established timeout. > > > > TCP FIN wait timeout. > > > > TCP close wait timeout. > > > > TCP last ACK timeout. > > > > TCP time wait timeout. > > > > TCP close timeout. > > > > TCP syn sent2 timeout. > > > > TCP retransmit timeout. > > > > TCP unacknowledgment timeout. > > > > > > First UDP packet timeout. > > > > The timeout in the state that source host sends more than one packet > but the destination host has never sent one backs. > > > > UDP packets seen in both directions timeout. > > > > > > First ICMP timeout. > > > > ICMP reply timeout. > > > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] История государства Российского — уникальная видеоколлекция в отличном качестве. 13_05_2019 02_03 198707
ИСТОРИЯ ГОСУДАРСТВА РОССИЙСКОГО Цикл "История государства Российского" созданный по фундаментальному одноименному труду выдающегося литератора и историка российской культуры 19 века Николая Михайловича Карамзина и дополненный сочинениями Николая Ивановича Костомарова «Русская история в жизнеописаниях ее главнейших деятелей» и Сергея Михайловича Соловьёва «История России с древнейших времен», состоит из 500 серий. Проект охватывает события истории Российского государства от момента его создания и до конца правления Екатерины II. Каждая серия - плод усилий большого творческого коллектива, состоящего из десятков людей разных профессий: модельеров трехмерной графики, которые выполняют сотни фигур воинов, крестьян, ремесленников, модели городов; аниматоров, оживляющих персонажи; "компоузеров", собирающих, как из конструктора, серию в целом. И, разумеется, редакторов, художников-постановщиков, музыкантов... И в результате на экране - "ожившие" древние карты, сцены походов и битв, виды древнерусских городов и исторически достоверные пейзажи, тщательно проработанные детали одежды, оружия, утвари наших предков, их дальних и ближних соседей, отвечающие реалиям сменяющихся исторических эпох. "История государства Российского" - это не просто учебник истории. Главная задача проекта - вызвать интерес к ней, показать, что наша история - не пожелтевшие страницы пыльных фолиантов, а увлекательное повествование о жизни великой страны, яркие биографии совершенно реальных людей, умом и волей которых формировались русское государство и нация в целом. Закадровый текст читает певец и композитор Юрий Шевчук. ! Список серий, описание каждой из них, вы можете увидеть в прикреплённом к письму файле ! Коллекция состоит из 500 серий. Записана на внешний USB накопитель (флешка). Проблем с воспроизведением не возникнет, можно смотреть на компьютере, планшете, смартфоне, телевизоре и т.д. Запись на внешний USB накопитель имеет ряд преимуществ в сравнении с обычными DVD дисками, USB накопитель гораздо легче, занимает меньше места, обладает высокой надёжностью сохранности записей, а это значит, что наша коллекция будет радовать Вас много лет. Мы гарантируем отличное качество всех записей. На самом носителе создана продуманная структура, все записи разнесены по каталогам, имеются плейлисты, прописаны теги, а также полный список вошедших записей, поэтому проблем с поиском и навигацией не возникнет. Стоимость коллекции на внешнем USB накопителе — 6500 (Шесть Тысяч Пятьсот) Рублей. Продаются только вместе. Доставка включена в стоимость. Доставка только почтой по всей России, сроки 7-14 суток с момента отправки. Оплата в момент получения заказа на почте наложенным платежом. У нас нет курьерской доставки — только почтой, в том числе и по Москве. Для оформления заказа просьба не забывать указывать: --- Ваш почтовый индекс (пишите правильный индекс — это ускорит доставку); --- Ваш город и точный адрес (название улицы, номер дома и номер квартиры); --- Ф.И.О. получателя и ОБЯЗАТЕЛЬНО номер контактного телефона (лучше сотовый); Заказы\вопросы направляйте по адресу: historyruss...@cwhflash.ru Мы очень ответственно относимся к качеству нашего товара, поэтому перед отправкой всё дополнительно проверяется, как следствие отправка бракованной продукции сведена к нулю. Товар упаковывается в специальный ударостойкий материал, что в значительной степени уменьшает риск повреждения при транспортировке. Если вдруг с полученным товаром возникнут проблемы, то все наши покупатели всегда могут рассчитывать на квалифицированную техническую поддержку. Мы никогда не отказываемся от гарантийных обязательств, в случае проблемы Вы можете рассчитывать на замену, почтовые расходы мы берём на себя. По вашему желанию, данная коллекция может быть записана на DVD диски. Для записи используются надёжные DVD диски со специальным покрытием, которое повышает устойчивость диска к механическим повреждениям, таким как трещины и царапины, а это значит, что наша коллекция будет радовать Вас много лет. Коллекция упакована в пластиковые боксы (slim-dvd), имеет красивые и продуманные обложки, с обратной стороны которых указан список вошедших на каждый диск серий и другая полезная информация, поэтому проблем с поиском и навигацией не возникнет. Если хотите приобрести коллекцию, записанную на DVD дисках, то в этом случае просьба сообщить нам об этом в своей заявке, цена прежняя, как у версии на внешнем USB накопителе (флешка) — 6500 (Шесть Тысяч Пятьсот) Рублей. Если вы не хотите больше получать от нас письма, отправьте нам письмо с темой “deletemail” и Ваш адрес навсегда будет удален автоматически. 13_05_2019 02_03 198707 ovs-dev@openvswitch.org ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovn] OVN split: Remove make "modules_install" target
Acked-by: Mark Michelson On 7/29/19 8:59 AM, lmart...@redhat.com wrote: From: Lucas Alvares Gomes This patch is removing the make "modules_install" target for the new OVN repository. The target is not be needed for OVN (and is currently broken) becasue the kernel modules should be compiled and installed using the OVS repository. Signed-off-by: Lucas Alvares Gomes --- Makefile.am | 5 - 1 file changed, 5 deletions(-) diff --git a/Makefile.am b/Makefile.am index e3dea1912..2a629b425 100644 --- a/Makefile.am +++ b/Makefile.am @@ -484,11 +484,6 @@ install-data-local: $(INSTALL_DATA_LOCAL) uninstall-local: $(UNINSTALL_LOCAL) .PHONY: $(DIST_HOOKS) $(CLEAN_LOCAL) $(INSTALL_DATA_LOCAL) $(UNINSTALL_LOCAL) -modules_install: -if LINUX_ENABLED - cd datapath/linux && $(MAKE) modules_install -endif - dist-docs: VERSION=$(VERSION) MAKE='$(MAKE)' $(srcdir)/build-aux/dist-docs $(srcdir) $(docs) .PHONY: dist-docs ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 03/12] ovs-vsctl: Add datapath and CT zone commands.
added one more comment.
On Fri, Jul 26, 2019 at 4:10 PM Darrell Ball wrote:
> added one more comment for now
>
>
> On Fri, Jul 26, 2019 at 11:13 AM Darrell Ball wrote:
>
>> Thanks for the patch
>>
>> Not a full review; just some initial testing
>>
>>
>> 1/ AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
>> icmp_reply_blah=3])])
>>
>> The above syntax is NOT flagged as an error
>>
>>
>> 2/ AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-tp netdev zone=2
>> icmp_first=2 icmp_reply=3])])
>>
>> The above "--may-exist" option fails with
>> +ovs-vsctl: 'add-zone-tp' command has no '--may-exist' option
>>
>> AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=1])])
>> is also failing
>> +ovs-vsctl: 'del-zone-tp' command has no '--if-exists' option
>>
>> Please support both "--may-exist" and "--if-exists"
>>
>>
>> 3/ The below should fail, but it is accepted.
>>
>> AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
>> icmp_reply=3])])
>> AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
>> icmp_reply=3])])
>>
>>
>> 4/ The below fails (which is good), but the error is in idl, rather than
>> the 'del-zone-tp' command
>>
>> AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])])
>> AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])])
>> fails with
>> +2019-07-26T17:56:10Z|2|ovsdb_idl|WARN|Trying to delete a key that
>> doesn't exist in the map.
>>
>>
>>
>> 5/ Please support --may-exist for add-dp
>>
>> 6/ Please support --if-exists for del-dp
>>
>>
>> 7/ Few comments below
>>
>>
>> Thanks Darrell
>>
>>
>> On Thu, Jul 25, 2019 at 4:26 PM Yi-Hung Wei wrote:
>>
>>> From: William Tu
>>>
>>> The patch adds the following commands
>>> $ ovs-vsctl {add,del,list}-dp
>>> for creating/deleting/listing the datapath, and
>>> $ ovs-vsctl {add,del,list}-zone-tp
>>> for conntrack zones and timeout policies.
>>>
>>> Signed-off-by: William Tu
>>> ---
>>> tests/ovs-vsctl.at | 20 +++-
>>> utilities/ovs-vsctl.8.in | 29 ++
>>> utilities/ovs-vsctl.c| 245
>>> +++
>>> 3 files changed, 292 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
>>> index 77604c58a2bc..8854138ecb1e 100644
>>> --- a/tests/ovs-vsctl.at
>>> +++ b/tests/ovs-vsctl.at
>>> @@ -805,6 +805,22 @@ AT_CHECK(
>>>[RUN_OVS_VSCTL([--if-exists remove netflow x targets '"1.2.3.4:567
>>> "'])])
>>> AT_CHECK(
>>>[RUN_OVS_VSCTL([--if-exists clear netflow x targets])])
>>> +
>>> +AT_CHECK([RUN_OVS_VSCTL([add-dp netdev])])
>>> +AT_CHECK([RUN_OVS_VSCTL([add-dp system])])
>>> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
>>> icmp_reply=2])])
>>> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout
>>> Policies: icmp_first=1 icmp_reply=2
>>> +])
>>> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
>>> icmp_reply=3])])
>>>
>>
>> Add all possible keys as part of positive tests so we know thye work
>>
>>
>>
>>
>>> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout
>>> Policies: icmp_first=1 icmp_reply=2
>>> +Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3
>>> +])
>>> +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])])
>>> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
>>> Policies: icmp_first=2 icmp_reply=3
>>> +])
>>> +AT_CHECK([RUN_OVS_VSCTL([del-dp netdev])])
>>> +AT_CHECK([RUN_OVS_VSCTL([list-dp | sed 's/ uuid.*$//'])], [0], [system
>>> +])
>>> OVS_VSCTL_CLEANUP
>>> AT_CLEANUP
>>>
>>> @@ -890,10 +906,10 @@ AT_CHECK([RUN_OVS_VSCTL([set bridge br0
>>> flood_vlans=-1])],
>>> AT_CHECK([RUN_OVS_VSCTL([set bridge br0 flood_vlans=4096])],
>>>[1], [], [ovs-vsctl: constraint violation: 4096 is not in the valid
>>> range 0 to 4095 (inclusive)
>>> ])
>>> -AT_CHECK([RUN_OVS_VSCTL([set c br1 'connection-mode=xyz'])],
>>> +AT_CHECK([RUN_OVS_VSCTL([set controller br1 'connection-mode=xyz'])],
>>>
>>
>> unrelated change
>>
>>
>>
>>>[1], [], [[ovs-vsctl: constraint violation: xyz is not one of the
>>> allowed values ([in-band, out-of-band])
>>> ]])
>>> -AT_CHECK([RUN_OVS_VSCTL([set c br1 connection-mode:x=y])],
>>> +AT_CHECK([RUN_OVS_VSCTL([set controller br1 connection-mode:x=y])],
>>>
>>
>> unrelated change
>>
>>
>>
>>>[1], [], [ovs-vsctl: cannot specify key to set for non-map column
>>> connection_mode
>>> ])
>>> AT_CHECK([RUN_OVS_VSCTL([add bridge br1 datapath_id x y])],
>>> diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
>>> index 7c09df79bd29..f8ec995247e7 100644
>>> --- a/utilities/ovs-vsctl.8.in
>>> +++ b/utilities/ovs-vsctl.8.in
>>> @@ -353,6 +353,35 @@ list.
>>> Prints the name of the bridge that contains \fIiface\fR on standard
>>> output.
>>> .
>>> +.SS "Datapath Commands"
>>> +These commands examine and manipulate Open vSwitch datapath.
>>> +.
>>> +.IP "\fBadd\-dp \fIdatapath\fR"
>>> +Creates a new datapath named \fIdatapath\fR. Use "netdev" for userspace
>>> +datapath
Re: [ovs-dev] [PATCH 00/12] Support zone-based conntrack timeout policy
On Mon, Jul 29, 2019 at 11:53 AM Yi-Hung Wei wrote: > Hi Ilya, > > Thanks for your comment. > > On Mon, Jul 29, 2019 at 2:22 AM Ilya Maximets > wrote: > > > > Hi everyone, > > > > My 2 cents for the feature design: > > > > From the user's perspective: > > > > * 'add-dp'/'del-dp' commands looks very strange. > > "I didn't add datapath into ovsdb, why it exists and switches packets?" > > "I deleted the datapath from the OVS, why it still exists and switches > packets?" > > > > If you're implementing the configuration like this, 'datapath' should > > own the bridges and interfaces, i.e. datapath should be created > manually > > on 'add-dp' and automatically on adding the first bridge on that > datapath. > > All the bridges and interfaces must be deleted/destroyed on 'del-dp'. > > > Or you need to rename your tables and commands to not look like this. > I agree that a "datapath" should be auto-created when bridge of that type is created. This came up in internal e-mails and I lost track of it. It is a bit strange to call this column 'datapath'; 'datapath-config' might be a bit better and reflects what it is really is. I think renaming would clear things up considerably and avoid confusion. > > > > From the developer's perspective: > > > > * Right now 'ofproto-dpif' is the only module that manages datapath > interfaces > > and it knows that (there are specific comments in the code). 'dpif's > has > > no reference counts and it's slightly unsafe to manage them outside of > > 'ofproto-dpif'. > > You're adding the side module that allowed to open dpif (and it's not > able > > to delete it, that is the possible cause if issues) and use it without > > noticing any other modules. This breaks the hierarchical structure of > OVS. > > > > * Right now most of the datapath configuration is done via 'other_config' > > and corresponding dpif_set_config() callback. Since you're introducing > > datapath-config module, it should take care of all of this staff. And > this > > will require significant rework of the current datapath configuration > scheme. > > > > * 'reconfigure_datapath' is an ambiguous name. > > > > Solution for above issues might be not introducing the new modules at > all. > > Everything could be handled like we're handling meters, but with OVSDB > as the > > configuration source. On configuration change bridge layer will call > ofproto > > layer that will pass configuration to ofproto-dpif and, finally, dpif > layer. > > Inside 'struct dpif' in dpif.c module you could track all the > configuration > > and pass all the required changes to the dpif-provider via callbacks. > > This way everything will work fine without breaking current OVS > hierarchy. > > Thanks for your suggestion about the datapath-config part. I think it > makes sense to implement what datapath-config does inside dpif.c > rather than introduce a new module. I will make proper change for > that in v2. > > > > > > * DB scheme looks just overcomplicated. 3 additional tables which > references > > others just to store a string to integer map. > > I think that it might be much easier to create a single 'CT_Zones' > table > > with all the required columns: > > 'id', 'tcp_syn_sent', 'tcp_syn_recv', ..., 'icmp_reply'. > > This is not a big deal since you need to describe every single field in > > vswitch.xml anyway. This will also allow you to check support of > particular > > field on the stage of adding value to the database. > > If you really need to distinguish zones by the datapath type (which is > not > > obvious), you may add 'datapath_type' column, just like we have in a > 'Bridge' > > table. > > As for the database schema, we intend to make CT_Zone table references > to CT_Timeout_Policy table because some other zone-based feature can > be configured through ovsdb later on. For example, we can have a new > column in CT_Zone table that stores 'limit' as an integer to support > the zone limit feature (limiting number of connection in a zone). It > is currently configured through dpctl commands. > > I understand your concern on the complication that introduced by the > datapath table. Let me think about it more carefully and go back to > you later. > > Thanks, > > -Yi-Hung > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 05/12] ct-dpif: Add conntrack timeout policy support in dpif layer
On Mon, Jul 29, 2019 at 12:37 PM Yi-Hung Wei wrote:
> On Fri, Jul 26, 2019 at 11:41 AM Darrell Ball wrote:
> >
> > Thanks for the patch
> >
> > I found this patch hard to review since it does not contain
> implementations
> > The same comment applies to Patch 6
> > I think Patches 5-7 can be combined into one patch, which will make
> review easier.
>
> Thanks Darrell for the review. Sure, I can squash patch 5-7 altogether in
> v2.
>
>
> >> +struct ct_dpif_timeout_policy {
> >> +uint32_tid; /* id that uniquely identify a timeout
> policy. */
> >> +uint32_tpresent;/* If a timeout attribute is present set
> the
> >> + * corresponding bit. */
> >>
> >> +uint32_tattrs[CT_DPIF_TP_ATTR_MAX]; /* An array that
> specifies
> >> + * timeout attribute
> values */
> >
> >
> > I think you can make attrs of type 'int32_t' and use '-1' timeout for
> 'not present' and then
> > remove the 'present' field
>
> The timeout value is uint32_t in the kernel, so I will keep it as
> uint32_t. I find the present flag to be handy when doing conversion
> from ct-dpif layer to dpif-netlink layer, and as you mentioned in the
> following e-mail, I will keep it as is.
>
>
> >> --- a/lib/dpif-netlink.c
> >> +++ b/lib/dpif-netlink.c
> >> @@ -3434,6 +3434,12 @@ const struct dpif_class dpif_netlink_class = {
> >> dpif_netlink_ct_set_limits,
> >> dpif_netlink_ct_get_limits,
> >> dpif_netlink_ct_del_limits,
> >> +NULL, /* ct_set_timeout_policy */
> >> +NULL, /* ct_get_timeout_policy */
> >> +NULL, /* ct_del_timeout_policy */
> >> +NULL, /* ct_timeout_policy_dump_start */
> >> +NULL, /* ct_timeout_policy_dump_next */
> >> +NULL, /* ct_timeout_policy_dump_done */
> >
> >
> > I found this patch hard to review since it does not contain
> implementations
> > The same comment applies to Patch 6
> > I think Patches 5-7 can be combined into one patch, which will make
> review easier.
> >
> >
> >>
> >> NULL, /* ipf_set_enabled */
> >> NULL, /* ipf_set_min_frag */
> >> NULL, /* ipf_set_max_nfrags */
> >> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> >> index 12898b9e3c6d..3460ef8aa98d 100644
> >> --- a/lib/dpif-provider.h
> >> +++ b/lib/dpif-provider.h
> >> @@ -80,6 +80,7 @@ dpif_flow_dump_thread_init(struct
> dpif_flow_dump_thread *thread,
> >> struct ct_dpif_dump_state;
> >> struct ct_dpif_entry;
> >> struct ct_dpif_tuple;
> >> +struct ct_dpif_timeout_policy;
> >>
> >> /* 'dpif_ipf_proto_status' and 'dpif_ipf_status' are presently in
> >> * sync with 'ipf_proto_status' and 'ipf_status', but more
> >> @@ -498,6 +499,48 @@ struct dpif_class {
> >> * list of 'struct ct_dpif_zone_limit' entries. */
> >> int (*ct_del_limits)(struct dpif *, const struct ovs_list
> *zone_limits);
> >>
> >> +/* Connection tracking timeout policy */
> >> +
> >> +/* A connection tracking timeout policy contains a list of timeout
> >> + * attributes that specifies timeout values on various connection
> states.
> >> + * In a datapath, the timeout policy is identified by a 4 bytes
> unsigned
> >> + * integer, and the unsupported timeout attributes are ignored.
> >> + * When a connection is committed it can be associated with a
> timeout
> >> + * policy, or it defaults to the default timeout policy. */
> >> +
> >> +/* Add timeout policy '*tp' into the datapath. If 'is_default' is
> true
> >
> >
> > "is_default" - can you explain this one ?
>
> This flag is used to configure the default timeout policy in the
> datapath. If 'is_default' is true, it will set the provided timeout
> policy to be the default timeout policy. The default timeout policy is
> documented in vswitchd/vswitch.xml
>
>
Below is what I see the the schema xml file under timeout policy
Please add description about the 'default' timeout policy under
"CT_Timeout_Policy" - let me know if you need help, as I can submit a patch.
Connection tracking timeout policy configuration
The timeouts column contains key-value pairs used
to configure connection tracking timeouts in a datapath.
Key-value pairs that are not supported by a datapath are
ignored.
TCP SYN sent timeout.
TCP SYN receive timeout.
TCP established timeout.
TCP FIN wait timeout.
TCP close wait timeout.
TCP last ACK timeout.
TCP time wait timeout.
TCP close timeout.
TCP syn sent2 timeout.
Re: [ovs-dev] [PATCH 1/1] stream_ssl: fix important memory leak in ssl_connect() function
Thanks for this patch. Worked on this leak before but didn't find the fix.
Tested-by: Yifeng Sun
Reviewed-by: Yifeng Sun
On Fri, Jul 26, 2019 at 1:11 AM Damijan Skvarc wrote:
>
>
> While checking valgrind reports after running "make check-valgrind" I have
> noticed
> reports for several tests similar to the following:
>
>
> ==5345== Memcheck, a memory error detector
> ==5345== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
> ==5345== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
> ==5345== Command: ovsdb-client
> --private-key=/home/damijan.skvarc/doma/ovs/tests/testpki-privkey.pem
> --certificate=/home/damijan.skvarc/doma/ovs/tests/testpki-cert.pem
> --ca-cert=/home/damijan.skvarc/doma/ovs/tests/testpki-cacert.pem transact
> ssl:127.0.0.1:40111 \ \ \ ["ordinals",
> ==5345== \ \ \ \ \ \ {"op":\ "update",
> ==5345== \ \ \ \ \ \ \ "table":\ "ordinals",
> ==5345== \ \ \ \ \ \ \ "where":\ [["number",\ "==",\ 1]],
> ==5345== \ \ \ \ \ \ \ "row":\ {"number":\ 2,\ "name":\ "old\ two"}},
> ==5345== \ \ \ \ \ \ {"op":\ "update",
> ==5345== \ \ \ \ \ \ \ "table":\ "ordinals",
> ==5345== \ \ \ \ \ \ \ "where":\ [["name",\ "==",\ "two"]],
> ==5345== \ \ \ \ \ \ \ "row":\ {"number":\ 1,\ "name":\ "old\ one"}}]
> ==5345== Parent PID: 5344
> ==5345==
> ==5345==
> ==5345== HEAP SUMMARY:
> ==5345== in use at exit: 116,551 bytes in 3,341 blocks
> ==5345== total heap usage: 5,134 allocs, 1,793 frees, 412,290 bytes
> allocated
> ==5345==
> ==5345== 6,221 (184 direct, 6,037 indirect) bytes in 1 blocks are definitely
> lost in loss record 498 of 500
> ==5345==at 0x4C2DB8F: malloc (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==5345==by 0x5105E77: CRYPTO_malloc (in
> /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
> ==5345==by 0x51E1D23: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
> ==5345==by 0x51E4861: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
> ==5345==by 0x51E5414: ASN1_item_ex_d2i (in
> /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
> ==5345==by 0x51E546A: ASN1_item_d2i (in
> /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
> ==5345==by 0x4E56B27: ??? (in /lib/x86_64-linux-gnu/libssl.so.1.0.0)
> ==5345==by 0x4E5BA11: ??? (in /lib/x86_64-linux-gnu/libssl.so.1.0.0)
> ==5345==by 0x4E65145: ??? (in /lib/x86_64-linux-gnu/libssl.so.1.0.0)
> ==5345==by 0x4522DF: ssl_connect (stream-ssl.c:530)
> ==5345==by 0x443D38: scs_connecting (stream.c:315)
> ==5345==by 0x443D38: stream_connect (stream.c:338)
> ==5345==by 0x443FA1: stream_open_block (stream.c:266)
> ==5345==by 0x40AB79: open_jsonrpc (ovsdb-client.c:507)
> ==5345==by 0x40AB79: open_rpc (ovsdb-client.c:143)
> ==5345==by 0x40B06B: do_transact__ (ovsdb-client.c:871)
> ==5345==by 0x40B245: do_transact (ovsdb-client.c:893)
> ==5345==by 0x405F76: main (ovsdb-client.c:282)
> ==5345==
> ==5345== LEAK SUMMARY:
> ==5345==definitely lost: 184 bytes in 1 blocks
> ==5345==indirectly lost: 6,037 bytes in 117 blocks
> ==5345== possibly lost: 0 bytes in 0 blocks
> ==5345==still reachable: 110,330 bytes in 3,223 blocks
> ==5345== suppressed: 0 bytes in 0 blocks
> ==5345== Reachable blocks (those to which a pointer was found) are not shown.
> ==5345== To see them, rerun with: --leak-check=full --show-leak-kinds=all
> ==5345==
> ==5345== For counts of detected and suppressed errors, rerun with: -v
> ==5345== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
>
>
>
> This report was extracted from "index uniqueness checking" test and complains
> about
> leaking memory in ovsdb-client application. The problem is not huge, since
> ovsdb-client
> is CLI tool which is constantly reinvoked/restarted, thus leaked memory is
> not accumulated.
>
> More problematic issue is that for the same test valgrind reports the similar
> problem also for
> ovsdb-server:
>
>
> ==5290== Memcheck, a memory error detector
> ==5290== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
> ==5290== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
> ==5290== Command: ovsdb-server --log-file --detach --no-chdir --pidfile
> --private-key=/home/damijan.skvarc/doma/ovs/tests/testpki-privkey2.pem
> --certificate=/home/damijan.skvarc/doma/ovs/tests/testpki-cert2.pem
> --ca-cert=/home/damijan.skvarc/doma/ovs/tests/testpki-cacert.pem
> --remote=pssl:0:127.0.0.1 db
> ==5290== Parent PID: 5289
> ==5290==
> ==5292== Warning: noted but unhandled ioctl 0x2403 with no size/direction
> hints.
> ==5292==This could cause spurious value errors to appear.
> ==5292==See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a
> proper wrapper.
> ==5292== Warning: noted but unhandled ioctl 0x2400 with no size/direction
> hints.
> ==5292==This could cause spurious value errors to appear.
> ==5292==See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a
> proper wrapper.
> ==5290==
> ==5290==
Re: [ovs-dev] [PATCH 07/12] dpif-netlink: Add conntrack timeout policy support
On Fri, Jul 26, 2019 at 10:15 AM William Tu wrote:
> > +static void
> > +dpif_netlink_format_tp_name(uint32_t id, uint16_t l3num, uint8_t l4num,
> > +struct ds *tp_name)
> > +{
> > +ds_clear(tp_name);
> > +ds_put_format(tp_name, "%s%"PRIu32"_", NL_TP_NAME_PREFIX, id);
> > +ct_dpif_format_ipproto(tp_name, l4num);
> > +
> > +if (l3num == AF_INET) {
> > +ds_put_cstr(tp_name, "4");
> > +} else if (l3num == AF_INET6 && l4num != IPPROTO_ICMPV6) {
>
> Why excluding IPPROTO_ICMPV6 above?
Thanks for review.
It is because ct_dpif_format_ipproto returns "icmpv6" for
IPPROTO_ICMPV6 and "icmp" for "IPPROTO_ICMP", and I found it to be
confusing to have ovs_tp__icmpv66 as the timeout policy name.
> > +#define CT_DPIF_TO_NL_TP_MAPPING(PROTO1, PROTO2, ATTR1, ATTR2) \
> > +if (nl_tp->present & (1 << CTA_TIMEOUT_##PROTO2##_##ATTR2)) { \
> > +tp->present |= 1 << CT_DPIF_TP_ATTR_##PROTO1##_##ATTR1; \
> > +tp->attrs[CT_DPIF_TP_ATTR_##PROTO1##_##ATTR1] = \
> > +nl_tp->attrs[CTA_TIMEOUT_##PROTO2##_##ATTR2]; \
> > +}
> > +
> > +static void
> > +dpif_netlink_set_ct_dpif_tp_tcp_attrs(const struct nl_ct_timeout_policy
> > *nl_tp,
> > + struct ct_dpif_timeout_policy *tp)
> > +{
> > +CT_DPIF_TO_NL_TP_TCP_MAPPINGS
>
> Is this better to renamed as CT_DPIF_FROM_NL_TP_TCP_MAPPINGS?
>
> You're using the same macro name, one for
> setting the nl_tp->attrs from tp->attrs, the other for
> setting the tp->attrs from nl_tp_attrs
Thanks for the suggestion. As our offline discussion, it is confusing
to have "_TO_" in the marco name, I will get rid of it.
> > +static int
> > +dpif_netlink_ct_add_timeout_policy(struct dpif *dpif OVS_UNUSED,
> > + bool is_default,
> > + const struct ct_dpif_timeout_policy *tp)
> > +{
> > +#ifdef _WIN32
> > +return EOPNOTSUPP;
> > +#else
> > +struct nl_ct_timeout_policy nl_tp;
> > +struct ds ds = DS_EMPTY_INITIALIZER;
> > +int i, err;
> > +
> > +for (i = 0; i < ARRAY_SIZE(tp_protos); ++i) {
> > +dpif_netlink_format_tp_name(tp->id, tp_protos[i].l3num,
> > +tp_protos[i].l4num, );
> > +ovs_strlcpy(nl_tp.name, ds_cstr(), sizeof nl_tp.name);
> > +nl_tp.l3num = tp_protos[i].l3num;
> > +nl_tp.l4num = tp_protos[i].l4num;
> > +dpif_netlink_get_nl_tp_attrs(tp, tp_protos[i].l4num, _tp);
> > +if (!is_default) {
> > +err = nl_ct_set_timeout_policy(_tp);
> > +} else if (tp_protos[i].l3num == AF_INET) {
> > +/* The default timeout policy is shared between AF_INET and
> > + * AF_INET6 in the kernel. So configure AF_INET is sufficient.
> > */
> > +err = nl_ct_set_default_timeout_policy(_tp);
> > +}
> > +if (err) {
> > +VLOG_INFO("failed to set timeout policy %s (%s)", nl_tp.name,
> > + ovs_strerror(err));
> ds_destroy();
Thanks, I will destroy the dynamic string properly in all the
following cases in v2.
> > +static int
> > +dpif_netlink_ct_get_timeout_policy(struct dpif *dpif OVS_UNUSED,
> > + bool is_default, uint32_t tp_id,
> > + struct ct_dpif_timeout_policy *tp)
> > +{
> > +#ifdef _WIN32
> > +return EOPNOTSUPP;
> > +#else
> if _WIN32 is alway return EOPNOTSUPP,
> is it better if we aggregate all 6 functions and have a larger
> #ifdef _WIN32
> // all six functions return EOPNOTSUPP
> #else
> // actual implementations
> #endif
Sure, I will make proper change to make the code looks clearly in the
next version.
> > +struct nl_ct_timeout_policy nl_tp;
> > +struct ds nl_tp_name = DS_EMPTY_INITIALIZER;
> > +int i, err;
> > +
> > +tp->id = tp_id;
> > +tp->present = 0;
> > +for (i = 0; i < ARRAY_SIZE(tp_protos); ++i) {
> > +if (!is_default) {
> > +dpif_netlink_format_tp_name(tp_id, tp_protos[i].l3num,
> > +tp_protos[i].l4num, _tp_name);
> > +err = nl_ct_get_timeout_policy(ds_cstr(_tp_name), _tp);
> > +} else if (tp_protos[i].l3num == AF_INET) {
> > +/* The default timeout is shared between AF_INET and AF_INET6
> > + * in the kernel. So get from AF_INET is sufficient. */
> Then why checking 'tp_protos[i].l3num == AF_INET'?
> What happens when tp_protos[i].l3num == AF_INET6? then 'err' becomes
> uninitialized.
This function is called from ct-dpif to query the timeout policy
stored in the kernel. It will loop through all L3/L4 pairs (ipv4
tcp/udp/icmp and ipv6 tcp/udp/icmpv6). The main purpose for this
check is to skip AF_INET6 cases for default timeout since it does not
distingush the ipv4 and ipv6 cases in the kernel.
> > +static int
> > +dpif_netlink_ct_del_timeout_policy(struct dpif
Re: [ovs-dev] [PATCH 06/12] ct-dpif: Add timeout policy related utility functions.
On Fri, Jul 26, 2019 at 9:04 AM William Tu wrote: > > --- a/lib/ct-dpif.c > > +++ b/lib/ct-dpif.c > > +static bool > > +ct_dpif_set_timeout_policy_attr(struct ct_dpif_timeout_policy *tp, > > + uint32_t attr, uint32_t value) > > There is a little mis-aligned above. > > Otherwise LGTM > Acked-by: William Tu Thanks for review, I will fix the indention issue in v2. -Yi-Hung ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 05/12] ct-dpif: Add conntrack timeout policy support in dpif layer
On Fri, Jul 26, 2019 at 11:41 AM Darrell Ball wrote:
>
> Thanks for the patch
>
> I found this patch hard to review since it does not contain implementations
> The same comment applies to Patch 6
> I think Patches 5-7 can be combined into one patch, which will make review
> easier.
Thanks Darrell for the review. Sure, I can squash patch 5-7 altogether in v2.
>> +struct ct_dpif_timeout_policy {
>> +uint32_tid; /* id that uniquely identify a timeout policy.
>> */
>> +uint32_tpresent;/* If a timeout attribute is present set the
>> + * corresponding bit. */
>>
>> +uint32_tattrs[CT_DPIF_TP_ATTR_MAX]; /* An array that specifies
>> + * timeout attribute values
>> */
>
>
> I think you can make attrs of type 'int32_t' and use '-1' timeout for 'not
> present' and then
> remove the 'present' field
The timeout value is uint32_t in the kernel, so I will keep it as
uint32_t. I find the present flag to be handy when doing conversion
from ct-dpif layer to dpif-netlink layer, and as you mentioned in the
following e-mail, I will keep it as is.
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -3434,6 +3434,12 @@ const struct dpif_class dpif_netlink_class = {
>> dpif_netlink_ct_set_limits,
>> dpif_netlink_ct_get_limits,
>> dpif_netlink_ct_del_limits,
>> +NULL, /* ct_set_timeout_policy */
>> +NULL, /* ct_get_timeout_policy */
>> +NULL, /* ct_del_timeout_policy */
>> +NULL, /* ct_timeout_policy_dump_start */
>> +NULL, /* ct_timeout_policy_dump_next */
>> +NULL, /* ct_timeout_policy_dump_done */
>
>
> I found this patch hard to review since it does not contain implementations
> The same comment applies to Patch 6
> I think Patches 5-7 can be combined into one patch, which will make review
> easier.
>
>
>>
>> NULL, /* ipf_set_enabled */
>> NULL, /* ipf_set_min_frag */
>> NULL, /* ipf_set_max_nfrags */
>> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
>> index 12898b9e3c6d..3460ef8aa98d 100644
>> --- a/lib/dpif-provider.h
>> +++ b/lib/dpif-provider.h
>> @@ -80,6 +80,7 @@ dpif_flow_dump_thread_init(struct dpif_flow_dump_thread
>> *thread,
>> struct ct_dpif_dump_state;
>> struct ct_dpif_entry;
>> struct ct_dpif_tuple;
>> +struct ct_dpif_timeout_policy;
>>
>> /* 'dpif_ipf_proto_status' and 'dpif_ipf_status' are presently in
>> * sync with 'ipf_proto_status' and 'ipf_status', but more
>> @@ -498,6 +499,48 @@ struct dpif_class {
>> * list of 'struct ct_dpif_zone_limit' entries. */
>> int (*ct_del_limits)(struct dpif *, const struct ovs_list *zone_limits);
>>
>> +/* Connection tracking timeout policy */
>> +
>> +/* A connection tracking timeout policy contains a list of timeout
>> + * attributes that specifies timeout values on various connection
>> states.
>> + * In a datapath, the timeout policy is identified by a 4 bytes unsigned
>> + * integer, and the unsupported timeout attributes are ignored.
>> + * When a connection is committed it can be associated with a timeout
>> + * policy, or it defaults to the default timeout policy. */
>> +
>> +/* Add timeout policy '*tp' into the datapath. If 'is_default' is true
>
>
> "is_default" - can you explain this one ?
This flag is used to configure the default timeout policy in the
datapath. If 'is_default' is true, it will set the provided timeout
policy to be the default timeout policy. The default timeout policy is
documented in vswitchd/vswitch.xml
>>
>> + * make the timeout policy to be the default timeout policy. */
>> +int (*ct_add_timeout_policy)(struct dpif *, bool is_default,
>> + const struct ct_dpif_timeout_policy *tp);
>> +/* Gets a timeout policy and stores that into '*tp'.
>
>
>
>>
>> If 'is_default' is
>> + * true, sets '*tp' to the default timeout policy. Otherwise, gets the
>
>
> The above text does not make sense:
> "sets" ?
>
> "is_default" - can you explain this one ?
I re-wreite the comment as following.
/* Gets a timeout policy and stores that into '*tp'. If 'is_default' is
* true, gets default timeout policy. Otherwise, gets the timeout
* policy identified by 'tp_id'. */
int (*ct_get_timeout_policy)(struct dpif *, bool is_default,
uint32_t tp_id,
struct ct_dpif_timeout_policy *tp);
Thanks,
-Yi-Hung
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] Remove OVN.
Bleep bloop. Greetings Mark Michelson, I am a robot and I have tried out your
patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line has non-spaces leading whitespace
#4774 FILE: NEWS:5:
* OVN has been removed from this repository. It now exists as a
WARNING: Line has non-spaces leading whitespace
#4775 FILE: NEWS:6:
separate project. You can find it at
WARNING: Line has non-spaces leading whitespace
#4776 FILE: NEWS:7:
https://github.com/ovn-org/ovn.git
WARNING: Line is 105 characters long (recommended limit is 79)
#7008 FILE: lib/db-ctl-base.xml:43:
defined by a get or create command within the
same ovs-vsctl
WARNING: Line is 85 characters long (recommended limit is 79)
#7035 FILE: lib/db-ctl-base.xml:382:
--wait-until, to prevent ovs-vsctl from
terminating
Lines checked: 95359, Warnings: 5, Errors: 0
build:
mv tests/system-dpdk-testsuite.tmp tests/system-dpdk-testsuite
\
{ sed -n -e '/%AUTHORS%/q' -e p < ./debian/copyright.in; \
sed '34,/^$/d' ./AUTHORS.rst | \
sed -n -e '/^$/q' -e 's/^/ /p'; \
sed -e '34,/%AUTHORS%/d' ./debian/copyright.in; \
} > debian/copyright
(printf '\043 Generated automatically -- do not modify!-*-
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.12.90,g') <
./rhel/openvswitch-dkms.spec.in > openvswitch-dkms.spec.tmp || exit 1; if cmp
-s openvswitch-dkms.spec.tmp rhel/openvswitch-dkms.spec; then touch
rhel/openvswitch-dkms.spec; rm openvswitch-dkms.spec.tmp; else mv
openvswitch-dkms.spec.tmp rhel/openvswitch-dkms.spec; fi
(printf '\043 Generated automatically -- do not modify!-*-
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.12.90,g') <
./rhel/kmod-openvswitch-rhel6.spec.in > kmod-openvswitch-rhel6.spec.tmp || exit
1; if cmp -s kmod-openvswitch-rhel6.spec.tmp rhel/kmod-openvswitch-rhel6.spec;
then touch rhel/kmod-openvswitch-rhel6.spec; rm
kmod-openvswitch-rhel6.spec.tmp; else mv kmod-openvswitch-rhel6.spec.tmp
rhel/kmod-openvswitch-rhel6.spec; fi
(printf '\043 Generated automatically -- do not modify!-*-
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.12.90,g') <
./rhel/openvswitch-kmod-fedora.spec.in > openvswitch-kmod-fedora.spec.tmp ||
exit 1; if cmp -s openvswitch-kmod-fedora.spec.tmp
rhel/openvswitch-kmod-fedora.spec; then touch
rhel/openvswitch-kmod-fedora.spec; rm openvswitch-kmod-fedora.spec.tmp; else mv
openvswitch-kmod-fedora.spec.tmp rhel/openvswitch-kmod-fedora.spec; fi
(printf '\043 Generated automatically -- do not modify!-*-
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.12.90,g') <
./rhel/openvswitch.spec.in > openvswitch.spec.tmp || exit 1; if cmp -s
openvswitch.spec.tmp rhel/openvswitch.spec; then touch rhel/openvswitch.spec;
rm openvswitch.spec.tmp; else mv openvswitch.spec.tmp rhel/openvswitch.spec; fi
(printf '\043 Generated automatically -- do not modify!-*-
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.12.90,g') <
./rhel/openvswitch-fedora.spec.in > openvswitch-fedora.spec.tmp || exit 1; if
cmp -s openvswitch-fedora.spec.tmp rhel/openvswitch-fedora.spec; then touch
rhel/openvswitch-fedora.spec; rm openvswitch-fedora.spec.tmp; else mv
openvswitch-fedora.spec.tmp rhel/openvswitch-fedora.spec; fi
(printf '\043 Generated automatically -- do not modify!-*-
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.12.90,g') \
< ./xenserver/openvswitch-xen.spec.in > openvswitch-xen.spec.tmp ||
exit 1; \
if cmp -s openvswitch-xen.spec.tmp xenserver/openvswitch-xen.spec; then touch
xenserver/openvswitch-xen.spec; rm openvswitch-xen.spec.tmp; else mv
openvswitch-xen.spec.tmp xenserver/openvswitch-xen.spec; fi
make[3]: Entering directory
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/datapath'
make[3]: Leaving directory
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/datapath'
NEWS
See above for files that use tabs for indentation.
Please use spaces instead.
make[2]: *** [check-tabs] Error 1
make[2]: Leaving directory
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make: *** [all] Error 2
Please check this out. If you feel there has been an error, please email
acon...@bytheb.org
Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 00/12] Support zone-based conntrack timeout policy
Hi Ilya, Thanks for your comment. On Mon, Jul 29, 2019 at 2:22 AM Ilya Maximets wrote: > > Hi everyone, > > My 2 cents for the feature design: > > From the user's perspective: > > * 'add-dp'/'del-dp' commands looks very strange. > "I didn't add datapath into ovsdb, why it exists and switches packets?" > "I deleted the datapath from the OVS, why it still exists and switches > packets?" > > If you're implementing the configuration like this, 'datapath' should > own the bridges and interfaces, i.e. datapath should be created manually > on 'add-dp' and automatically on adding the first bridge on that datapath. > All the bridges and interfaces must be deleted/destroyed on 'del-dp'. > > Or you need to rename your tables and commands to not look like this. > > From the developer's perspective: > > * Right now 'ofproto-dpif' is the only module that manages datapath interfaces > and it knows that (there are specific comments in the code). 'dpif's has > no reference counts and it's slightly unsafe to manage them outside of > 'ofproto-dpif'. > You're adding the side module that allowed to open dpif (and it's not able > to delete it, that is the possible cause if issues) and use it without > noticing any other modules. This breaks the hierarchical structure of OVS. > > * Right now most of the datapath configuration is done via 'other_config' > and corresponding dpif_set_config() callback. Since you're introducing > datapath-config module, it should take care of all of this staff. And this > will require significant rework of the current datapath configuration > scheme. > > * 'reconfigure_datapath' is an ambiguous name. > > Solution for above issues might be not introducing the new modules at all. > Everything could be handled like we're handling meters, but with OVSDB as > the > configuration source. On configuration change bridge layer will call ofproto > layer that will pass configuration to ofproto-dpif and, finally, dpif layer. > Inside 'struct dpif' in dpif.c module you could track all the configuration > and pass all the required changes to the dpif-provider via callbacks. > This way everything will work fine without breaking current OVS hierarchy. Thanks for your suggestion about the datapath-config part. I think it makes sense to implement what datapath-config does inside dpif.c rather than introduce a new module. I will make proper change for that in v2. > > * DB scheme looks just overcomplicated. 3 additional tables which references > others just to store a string to integer map. > I think that it might be much easier to create a single 'CT_Zones' table > with all the required columns: > 'id', 'tcp_syn_sent', 'tcp_syn_recv', ..., 'icmp_reply'. > This is not a big deal since you need to describe every single field in > vswitch.xml anyway. This will also allow you to check support of particular > field on the stage of adding value to the database. > If you really need to distinguish zones by the datapath type (which is not > obvious), you may add 'datapath_type' column, just like we have in a > 'Bridge' > table. As for the database schema, we intend to make CT_Zone table references to CT_Timeout_Policy table because some other zone-based feature can be configured through ovsdb later on. For example, we can have a new column in CT_Zone table that stores 'limit' as an integer to support the zone limit feature (limiting number of connection in a zone). It is currently configured through dpctl commands. I understand your concern on the complication that introduced by the datapath table. Let me think about it more carefully and go back to you later. Thanks, -Yi-Hung ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 00/12] Support zone-based conntrack timeout policy
On Fri, Jul 26, 2019 at 2:19 PM William Tu wrote: > > > I did my first round of review by just reading through the code. > I plan to test it next week. > > btw, can you update NEWS? > > Thanks > William Thanks William for review. I will address your comments and add NEWS in v2. Thank, -Yi-Hung ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] Remove OVN.
Bleep bloop. Greetings Mark Michelson, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Author Mark Michelson needs to sign off. WARNING: Line is 105 characters long (recommended limit is 79) #6991 FILE: lib/db-ctl-base.xml:43: defined by a get or create command within the same ovs-vsctl WARNING: Line is 85 characters long (recommended limit is 79) #7018 FILE: lib/db-ctl-base.xml:382: --wait-until, to prevent ovs-vsctl from terminating Lines checked: 95342, Warnings: 2, Errors: 1 Please check this out. If you feel there has been an error, please email acon...@bytheb.org Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4 ovn] ovn-northd: Add the option to pause and resume
From: Numan Siddique
This patch adds 3 unixctl socket comments - pause, resume and is-paused.
Usage: ovs-appctl -t ovn-northd pause/resume/is-paused
This feature will be useful if the CMS wants to
- deploy OVN DB servers in active/passive mode and
- run ovn-northd on all these nodes and use unix ctl sockets to
connect to the local OVN DB servers.
On the nodes where OVN Db ovsdb-servers are in passive mode, the local
ovn-northds
will process the DB changes and compute logical flows to be thrown out later,
because write transactions are not allowed by these ovsdb-servers. It results in
unncessary CPU usage.
With these commands, CMS can pause ovn-northd on these node. A node
which becomes master, can resume the ovn-northd.
One use case is to use this feature in ovn-kubernetes with the above deployment
model.
Acked-by: Mark Michelson
Signed-off-by: Numan Siddique
---
v3 -> v4
* Submitted the patch for the OVN repo
v2 -> v3
===
* Resolved merge conflicts.
v1 -> v2
===
* Addressed the review comments from Ben and add more documentation
about the runtime options added by this patch.
* v1 had an issue - When paused, it was not even waking up to process
the IDL updates. In v2, the main thread, wakes up to process any
IDL updates, but doesn't do any logical flow computations.
northd/ovn-northd.8.xml | 48
northd/ovn-northd.c | 121 ++--
tests/ovn-northd.at | 38 +
3 files changed, 179 insertions(+), 28 deletions(-)
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index d2267de0e..1d0243656 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -70,6 +70,23 @@
Causes ovn-northd to gracefully terminate.
+
+ pause
+
+Pauses the ovn-northd operation from processing any Northbound and
+Southbound database changes.
+
+
+ resume
+
+Resumes the ovn-northd operation to process Northbound and
+Southbound database contents and generate logical flows.
+
+
+ is-paused
+
+Returns "true" if ovn-northd is currently paused, "false" otherwise.
+
@@ -82,6 +99,37 @@
of ovn-northd will automatically take over.
+ Active-Standby with multiple OVN DB servers
+
+ You may run multiple OVN DB servers in an OVN deployment with:
+
+
+ OVN DB servers deployed in active/passive mode with one active
+ and multiple passive ovsdb-servers.
+
+
+
+ ovn-northd also deployed on all these nodes,
+ using unix ctl sockets to connect to the local OVN DB servers.
+
+
+
+
+
+ In such deployments, the ovn-northds on the passive nodes will process
+ the DB changes and compute logical flows to be thrown out later,
+ because write transactions are not allowed by the passive ovsdb-servers.
+ It results in unnecessary CPU usage.
+
+
+
+ With the help of runtime management command pause, you can
+ pause ovn-northd on these nodes. When a passive node
+ becomes master, you can use the runtime management command
+ resume to resume the ovn-northd to process the
+ DB changes.
+
+
Logical Flow Table Structure
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index bed2993c2..fcb19b8a1 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -52,6 +52,9 @@
VLOG_DEFINE_THIS_MODULE(ovn_northd);
static unixctl_cb_func ovn_northd_exit;
+static unixctl_cb_func ovn_northd_pause;
+static unixctl_cb_func ovn_northd_resume;
+static unixctl_cb_func ovn_northd_is_paused;
struct northd_context {
struct ovsdb_idl *ovnnb_idl;
@@ -9182,6 +9185,7 @@ main(int argc, char *argv[])
struct unixctl_server *unixctl;
int retval;
bool exiting;
+bool paused;
fatal_ignore_sigpipe();
ovs_cmdl_proctitle_init(argc, argv);
@@ -9196,6 +9200,10 @@ main(int argc, char *argv[])
exit(EXIT_FAILURE);
}
unixctl_command_register("exit", "", 0, 0, ovn_northd_exit, );
+unixctl_command_register("pause", "", 0, 0, ovn_northd_pause, );
+unixctl_command_register("resume", "", 0, 0, ovn_northd_resume, );
+unixctl_command_register("is-paused", "", 0, 0, ovn_northd_is_paused,
+ );
daemonize_complete();
@@ -9384,34 +9392,51 @@ main(int argc, char *argv[])
/* Main loop. */
exiting = false;
+paused = false;
while (!exiting) {
-struct northd_context ctx = {
-.ovnnb_idl = ovnnb_idl_loop.idl,
-.ovnnb_txn = ovsdb_idl_loop_run(_idl_loop),
-.ovnsb_idl = ovnsb_idl_loop.idl,
-.ovnsb_txn = ovsdb_idl_loop_run(_idl_loop),
-.sbrec_ha_chassis_grp_by_name = sbrec_ha_chassis_grp_by_name,
-.sbrec_mcast_group_by_name_dp =
Re: [ovs-dev] [PATCH V2 1/1] dpif-netlink: Log eth type 0x1234 not offloadable
ping On 7/11/2019 4:11 PM, Roi Dayan wrote: > > On 2019-07-03 9:11 PM, Ben Pfaff wrote: >> On Wed, Jul 03, 2019 at 04:58:06AM +, Eli Britstein wrote: >>> Ethernet type 0x1234 is used for testing and not being offloadable. For >>> testing offloadable features, log about using this value. >>> >>> Signed-off-by: Eli Britstein >>> Acked-by: Roi Dayan >>> Signed-off-by: Eli Britstein >> Acked-by: Ben Pfaff >> > ping. can we merge this? > thanks ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] Fix the chassis row recreation issue
From: Numan Siddique
Before the commit [1], ovn-controller would always recreate its
chassis row if deleted externally. After this commit, it no longer
recreates it. This is regression and needs to be fixed.
[1] - 242f1799fc22("ovn-controller: Refactor chassis.c to abstract the string
parsing")
Fixes: 242f1799fc22("ovn-controller: Refactor chassis.c to abstract the string
parsing")
Acked-by: Dumitru Ceara
Acked-by: Han Zhou
Signed-off-by: Numan Siddique
---
controller/chassis.c| 4
tests/ovn-controller.at | 29 +
2 files changed, 33 insertions(+)
diff --git a/controller/chassis.c b/controller/chassis.c
index 8d9f7c8d0..937c5574b 100644
--- a/controller/chassis.c
+++ b/controller/chassis.c
@@ -486,6 +486,10 @@ chassis_get_record(struct ovsdb_idl_txn *ovnsb_idl_txn,
if (!chassis_rec) {
VLOG_WARN("Could not find Chassis : stored (%s) ovs (%s)",
chassis_info_id(_state), chassis_id);
+if (ovnsb_idl_txn) {
+/* Recreate the chassis record. */
+chassis_rec = sbrec_chassis_insert(ovnsb_idl_txn);
+}
}
} else {
chassis_rec =
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 343c2abed..63b2581c0 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -292,3 +292,32 @@ as ovn-sb
OVS_APP_EXIT_AND_WAIT([ovsdb-server])
AT_CLEANUP
+
+# Checks that ovn-controller recreates its chassis record when deleted
externally.
+AT_SETUP([ovn-controller - Chassis self record])
+AT_KEYWORDS([ovn])
+ovn_init_db ovn-sb
+
+net_add n1
+sim_add hv
+as hv
+ovs-vsctl \
+-- add-br br-phys \
+-- add-br br-eth0 \
+-- add-br br-eth1 \
+-- add-br br-eth2
+ovn_attach n1 br-phys 192.168.0.1
+
+OVS_WAIT_UNTIL([test xhv = x`ovn-sbctl --columns name --bare find chassis`])
+# Delete the chassis "hv"
+ovn-sbctl chassis-del hv
+# ovn-controller should recreate its chassis row.
+OVS_WAIT_UNTIL([test xhv = x`ovn-sbctl --columns name --bare find chassis`])
+
+# Gracefully terminate daemons
+OVN_CLEANUP_SBOX([hv])
+OVN_CLEANUP_VSWITCH([main])
+as ovn-sb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+AT_CLEANUP
--
2.21.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v9] ovn: Add a new logical switch port type - 'virtual'
On Fri, Jul 19, 2019 at 12:53 AM Numan Siddique wrote:
>
>
> On Thu, Jul 18, 2019 at 11:20 PM Guru Shetty wrote:
>
>>
>>
>> On Thu, 18 Jul 2019 at 07:58, wrote:
>>
>>> From: Numan Siddique
>>>
>>> This new type is added for the following reasons:
>>>
>>> - When a load balancer is created in an OpenStack deployment with
>>> Octavia
>>> service, it creates a logical port 'VIP' for the virtual ip.
>>>
>>> - This logical port is not bound to any VIF.
>>>
>>> - Octavia service creates a service VM (with another logical port 'P'
>>> which
>>> belongs to the same logical switch)
>>>
>>> - The virtual ip 'VIP' is configured on this service VM.
>>>
>>> - This service VM provides the load balancing for the VIP with the
>>> configured
>>> backend IPs.
>>>
>>> - Octavia service can be configured to create few service VMs with
>>> active-standby mode
>>> with the active VM configured with the VIP. The VIP can move between
>>> these service nodes.
>>>
>>> Presently there are few problems:
>>>
>>> - When a floating ip (externally reachable IP) is associated to the
>>> VIP and if
>>> the compute nodes have external connectivity then the external
>>> traffic cannot
>>> reach the VIP using the floating ip as the VIP logical port would be
>>> down.
>>> dnat_and_snat entry in NAT table for this vip will have
>>> 'external_mac' and
>>> 'logical_port' configured.
>>>
>>> - The only way to make it work is to clear the 'external_mac' entry so
>>> that
>>> the gateway chassis does the DNAT for the VIP.
>>>
>>> To solve these problems, this patch proposes a new logical port type -
>>> virtual.
>>> CMS when creating the logical port for the VIP, should
>>>
>>> - set the type as 'virtual'
>>>
>>> - configure the VIP in the options -
>>> Logical_Switch_Port.options:virtual-ip
>>>
>>> - And set the virtual parents in the options
>>>Logical_Switch_Port.options:virtual-parents.
>>>These virtual parents are the one which can be configured with the
>>> VIP.
>>>
>>> If suppose the virtual_ip is configured to 10.0.0.10 on a virtual
>>> logical port 'sw0-vip'
>>> and the virtual_parents are set to - [sw0-p1, sw0-p2] then below logical
>>> flows are added in the
>>> lsp_in_arp_rsp logical switch pipeline
>>>
>>> - table=11(ls_in_arp_rsp), priority=100,
>>>match=(inport == "sw0-p1" && !is_chassis_resident("sw0-vip") &&
>>> ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10)
>>> ||
>>>(arp.op == 2 && arp.spa == 10.0.0.10))),
>>>action=(bind_vport("sw0-vip", inport); next;)
>>> - table=11(ls_in_arp_rsp), priority=100,
>>>match=(inport == "sw0-p2" && !is_chassis_resident("sw0-vip") &&
>>> ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10)
>>> ||
>>>(arp.op == 2 && arp.spa == 10.0.0.10))),
>>>action=(bind_vport("sw0-vip", inport); next;)
>>>
>>> The action bind_vport will claim the logical port - sw0-vip on the
>>> chassis where this action
>>> is executed. Since the port - sw0-vip is claimed by a chassis, the
>>> dnat_and_snat rule for
>>> the VIP will be handled by the compute node.
>>>
>>> Co-authored-by: Ben Pfaff
>>>
>>
Hi Ben,
I will be submitting this patch targeting the new OVN repo tomorrow.
I will put your signed-off-by tag since I have co authored you in this
patch.
I hope that's fine.
Thanks
Numan
> Signed-off-by: Numan Siddique
>>>
>>
>> Thanks for answering the questions. Looks like Ben already looked at the
>> code styling etc. I only looked at the logic. It felt like the right way
>> would have been to update NB with the active backend. But you probably
>> don't get that information. So, it looks okay to solve it this way. But
>> openstack will likely keep adding more things and you will likely have to
>> keep adding more such new types of logical ports.
>>
>
> Thanks for the review. I agree. I hope this will be the last patch to
> address open stack specific ones :).
>
> Although we may have a problem when CMS keeps updating the VIP with the
> active backend.
> There could be a mac_binding entry for the VIP in south db and when the
> VIP moves from one logical port to other,
> we may resolve the wrong or old mac for the VIP.
>
> We have to some how delete stale mac_Binding table entries. There is
> already a thread started by Daniel on this topic.
>
> Thanks
> Numan
>
>
>
>>
>> Acked-by: Gurucharan Shetty
>>
>>
>>> ---
>>> v8 -> v9
>>> ===
>>> * Added entry in NEWS.
>>>
>>> v7 -> v8
>>> ===
>>> * Applied the code suggestions from Ben.
>>>
>>> v6 -> v7
>>>
>>> * Resolved merge conflicts.
>>>
>>> v5 -> v6
>>>
>>> * Resolved conflicts after rebasing to latest master in tests/ovn.at
>>>
>>> v4 -> v5
>>> ===
>>> * Rebased to master to resolve merge conflicts.
>>>
>>> v3 -> v4
>>> ===
>>> * Addressed the review comment and removed the code in northd which
>>> referenced the Southbound db state while adding the logical flows.
>>>
Re: [ovs-dev] [PATCH v3 ovn] Include common ovn header files from include/ovn instead of ovs/include/ovn
On Mon, Jul 29, 2019 at 5:23 PM Dumitru Ceara wrote:
> On Mon, Jul 29, 2019 at 1:02 PM wrote:
> >
> > From: Numan Siddique
> >
> > For the other header files present in lib/, the previous commit [1]
> > changed the path. But few were left out. This patch fixes them too.
> >
> > Also updated the end comments in the header files with the correct path.
> >
> > [1] - a469954c00c4("Include ovn header files from lib/ instead of
> ovn/lib/")
> >
> > Signed-off-by: Numan Siddique
>
> Acked-by: Dumitru Ceara
>
>
Thanks for the review. I pushed it to master.
Numan
> > ---
> >
> > v2 -> v3
> > ==
> > * Updated the end comments in the header files.
> >
> > v1 -> v2
> > ===
> > * Addressed Dumitru's comments and updated lib/chassis-index.c
> >
> >
> > Makefile.am | 2 ++
> > controller/binding.h| 2 +-
> > controller/chassis.h| 2 +-
> > controller/encaps.h | 2 +-
> > controller/ip-mcast.h | 2 +-
> > controller/lflow.h | 2 +-
> > controller/lport.h | 2 +-
> > controller/ofctrl.h | 2 +-
> > controller/ovn-controller.h | 2 +-
> > controller/patch.h | 2 +-
> > controller/physical.h | 2 +-
> > controller/pinctrl.h| 2 +-
> > lib/acl-log.h | 2 +-
> > lib/chassis-index.c | 4 ++--
> > lib/chassis-index.h | 2 +-
> > lib/extend-table.h | 2 +-
> > lib/inc-proc-eng.h | 2 +-
> > lib/ip-mcast-index.c| 4 ++--
> > lib/ip-mcast-index.h| 2 +-
> > lib/mcast-group-index.h | 2 +-
> > lib/ovn-sb-idl.ann | 4 ++--
> > 21 files changed, 25 insertions(+), 23 deletions(-)
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index e3dea1912..4fe0d2899 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -19,6 +19,8 @@ AM_CPPFLAGS = $(SSL_CFLAGS)
> > AM_LDFLAGS = $(SSL_LDFLAGS)
> > AM_LDFLAGS += $(OVS_LDFLAGS)
> >
> > +AM_CPPFLAGS += -I $(top_srcdir)/include
> > +
> > if WIN32
> > AM_CPPFLAGS += -I $(top_srcdir)/ovs/include
> > AM_CPPFLAGS += -I $(top_srcdir)/ovs/lib
> > diff --git a/controller/binding.h b/controller/binding.h
> > index 8d9492630..bae162ede 100644
> > --- a/controller/binding.h
> > +++ b/controller/binding.h
> > @@ -54,4 +54,4 @@ bool binding_evaluate_port_binding_changes(
> > struct sset *active_tunnels,
> > struct sset *local_lports);
> >
> > -#endif /* ovn/binding.h */
> > +#endif /* controller/binding.h */
> > diff --git a/controller/chassis.h b/controller/chassis.h
> > index 16a131a3b..eb46ca3fc 100644
> > --- a/controller/chassis.h
> > +++ b/controller/chassis.h
> > @@ -43,4 +43,4 @@ bool chassis_get_mac(const struct sbrec_chassis
> *chassis,
> > struct eth_addr *chassis_mac);
> > const char *chassis_get_id(void);
> >
> > -#endif /* ovn/chassis.h */
> > +#endif /* controller/chassis.h */
> > diff --git a/controller/encaps.h b/controller/encaps.h
> > index afa41830a..c919d18e6 100644
> > --- a/controller/encaps.h
> > +++ b/controller/encaps.h
> > @@ -45,4 +45,4 @@ bool encaps_tunnel_id_parse(const char *tunnel_id,
> char **chassis_id,
> > bool encaps_tunnel_id_match(const char *tunnel_id, const char
> *chassis_id,
> > const char *encap_ip);
> >
> > -#endif /* ovn/encaps.h */
> > +#endif /* controller/encaps.h */
> > diff --git a/controller/ip-mcast.h b/controller/ip-mcast.h
> > index 6014f43d5..b3447d4c7 100644
> > --- a/controller/ip-mcast.h
> > +++ b/controller/ip-mcast.h
> > @@ -49,4 +49,4 @@ void igmp_group_delete(const struct sbrec_igmp_group
> *g);
> > bool igmp_group_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > struct ovsdb_idl_index *igmp_groups);
> >
> > -#endif /* ovn/controller/ip-mcast.h */
> > +#endif /* controller/ip-mcast.h */
> > diff --git a/controller/lflow.h b/controller/lflow.h
> > index 4e1086eb6..54da00b49 100644
> > --- a/controller/lflow.h
> > +++ b/controller/lflow.h
> > @@ -181,4 +181,4 @@ void lflow_handle_changed_neighbors(
> >
> > void lflow_destroy(void);
> >
> > -#endif /* ovn/lflow.h */
> > +#endif /* controller/lflow.h */
> > diff --git a/controller/lport.h b/controller/lport.h
> > index 7dcd5bee0..2d4bb7164 100644
> > --- a/controller/lport.h
> > +++ b/controller/lport.h
> > @@ -49,4 +49,4 @@ const struct sbrec_multicast_group
> *mcgroup_lookup_by_dp_name(
> > struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath,
> > const struct sbrec_datapath_binding *, const char *name);
> >
> > -#endif /* ovn/lport.h */
> > +#endif /* controller/lport.h */
> > diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> > index ed8918aae..114c9ef65 100644
> > --- a/controller/ofctrl.h
> > +++ b/controller/ofctrl.h
> > @@ -84,4 +84,4 @@ void ofctrl_check_and_add_flow(struct
> ovn_desired_flow_table *,
> > bool ofctrl_is_connected(void);
> > void ofctrl_set_probe_interval(int probe_interval);
> >
> > -#endif /* ovn/ofctrl.h */
> > +#endif /* controller/ofctrl.h */
> >
Re: [ovs-dev] How can we improve veth and tap performance in OVS DPDK?
On 29.07.2019 19:07, Ilya Maximets wrote: >> Hi, all >> We’re trying OVS DPDK in openstack cloud, but a big warn makes us hesitate. >> Floating IP and qrouter use tap interfaces which are attached into br-int, >> SNAT also should use similar way, so OVS DPDK will impact on VM network >> performance significantly, I believe many cloud providers have deployed OVS >> DPDK, my questions are: >> >> 1. Do we have some known ways to improve this? > > As RedHat OSP guide suggests, you could use any SDN controller (like > OpenDayLight) > or, alternatively, you could use OVN as a network provider for OpenStack. > This way all the required functionality will be handled by the OpenFlow rules > inside OVS without necessity to send traffic over veths and taps to Linux > Kernel. > >> 2. Is there any existing effort for this? Veth in kubernetes should >> have the same performance issue in OVS DPDK case. > > It makes no sense right now to run OVS-DPDK on veth pairs in Kubernetes. > The only benefit from OVS-DPDK in K8s might be from using virtio-vhost-user I meant virtio-user ports. > ports instead of veths for container networking. But this is not implemented. > Running DPDK apps inside K8s containers has a lot of unresolved issues right > now. > > One approach that could improve performance of veths and taps in the future is > using AF_XDP sockets which are supported in OVS now. But AF_XDP doesn't work > properly for virtual interfaces (veths, taps) yet due to issues in Linux > Kernel. > >> >> I also found a very weird issue. I added two veth pairs into ovs bridge and >> ovs DPDK bridge, for ovs case, iperf3 can work well, but it can’t for OVS >> DPDK case, what’s wrong. > > This is exactly same issue as we already discussed previously. Disable tx > offloading > on veth pairs and everything will work. > > Best regards, Ilya Maximets. > > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] How can we improve veth and tap performance in OVS DPDK?
> Hi, all > We’re trying OVS DPDK in openstack cloud, but a big warn makes us hesitate. > Floating IP and qrouter use tap interfaces which are attached into br-int, > SNAT also should use similar way, so OVS DPDK will impact on VM network > performance significantly, I believe many cloud providers have deployed OVS > DPDK, my questions are: > > 1. Do we have some known ways to improve this? As RedHat OSP guide suggests, you could use any SDN controller (like OpenDayLight) or, alternatively, you could use OVN as a network provider for OpenStack. This way all the required functionality will be handled by the OpenFlow rules inside OVS without necessity to send traffic over veths and taps to Linux Kernel. > 2. Is there any existing effort for this? Veth in kubernetes should > have the same performance issue in OVS DPDK case. It makes no sense right now to run OVS-DPDK on veth pairs in Kubernetes. The only benefit from OVS-DPDK in K8s might be from using virtio-vhost-user ports instead of veths for container networking. But this is not implemented. Running DPDK apps inside K8s containers has a lot of unresolved issues right now. One approach that could improve performance of veths and taps in the future is using AF_XDP sockets which are supported in OVS now. But AF_XDP doesn't work properly for virtual interfaces (veths, taps) yet due to issues in Linux Kernel. > > I also found a very weird issue. I added two veth pairs into ovs bridge and > ovs DPDK bridge, for ovs case, iperf3 can work well, but it can’t for OVS > DPDK case, what’s wrong. This is exactly same issue as we already discussed previously. Disable tx offloading on veth pairs and everything will work. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Remove OVN.
Hi Mark, The patch fails to apply. Can you please rebase ? Also can you please update NEWS about the OVN separation ? Thanks Numan On Sat, Jul 27, 2019 at 12:36 AM 0-day Robot wrote: > Bleep bloop. Greetings Mark Michelson, I am a robot and I have tried out > your patch. > Thanks for your contribution. > > I encountered some error that I wasn't expecting. See the details below. > > > git-am: > Failed to merge in the changes. > Patch failed at 0001 Remove OVN. > The copy of the patch that failed is found in: > > > /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch > When you have resolved this problem, run "git am --resolved". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > > > Please check this out. If you feel there has been an error, please email > acon...@bytheb.org > > Thanks, > 0-day Robot > ___ > 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
[ovs-dev] Cierre de Inscripciones
Es, para mí, un placer poder invitarte a nuestro curso en línea "Métodos y sistemas de costos", que se estará llevando a cabo el día Miércoles 07 de Agosto con un horario de 10:00 a 17:00 hrs. (hora del centro de México). Objetivos Específicos: - Conocerá la diferencia entre costos y gastos, su presentación contable, su tratamiento fiscal y definiciones de lo que es un sistema de costos. - Conocerá la clasificación de los costos según su grado de variabilidad e dentificará cuáles son los costos directos y costos indirectos. - Conocerá que sistemas de costos existen y en qué momento se determinan o calculan cada uno y los métodos de control de costos existentes y en qué casos son aplicables. Te invito a que participes con nosotros y conozcas los beneficios de la capacitación en línea. Teléfonos: (045) 55 15 54 66 30. (045) 55 85 56 72 93. (045) 55 30 16 70 85 Solicita información respondiendo a este correo con la palabra COSTOS, junto con los siguientes datos: Nombre: Correo electrónico: Número telefónico: ¡Qué tengas un excelente día! En caso de que haya recibido este correo sin haberlo solicitado o si desea dejar de recibir nuestra promoción favor de responder con la palabra baja o enviar un correo a ba...@innovalearn.net ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] How can we improve veth and tap performance in OVS DPDK?
Hi, all
We’re trying OVS DPDK in openstack cloud, but a big warn makes us hesitate.
Floating IP and qrouter use tap interfaces which are attached into br-int,
SNAT also should use similar way, so OVS DPDK will impact on VM network
performance significantly, I believe many cloud providers have deployed OVS
DPDK, my questions are:
1. Do we have some known ways to improve this?
2. Is there any existing effort for this? Veth in kubernetes should
have the same performance issue in OVS DPDK case.
I also found a very weird issue. I added two veth pairs into ovs bridge and
ovs DPDK bridge, for ovs case, iperf3 can work well, but it can’t for OVS
DPDK case, what’s wrong.
$ sudo ./my-ovs-vsctl show
2a67c1d9-51dc-4728-bb3e-405f2f49e2b1
Bridge br-int
Port "veth3-br"
Interface "veth3-br"
Port "dpdk0"
Interface "dpdk0"
type: dpdk
options: {dpdk-devargs=":00:08.0"}
Port br-int
Interface br-int
type: internal
Port "veth2-br"
Interface "veth2-br"
Port "dpdk1"
Interface "dpdk1"
type: dpdk
options: {dpdk-devargs=":00:09.0"}
Port "veth4-br"
Interface "veth4-br"
Port "veth1-br"
Interface "veth1-br"
$ sudo ip netns exec ns1 ifconfig veth1
veth1 Link encap:Ethernet HWaddr 26:32:e8:f3:1e:2a
inet addr:20.1.1.1 Bcast:20.1.1.255 Mask:255.255.255.0
inet6 addr: fe80::2432:e8ff:fef3:1e2a/64 Scope:Link
UP BROADCAST RUNNING MULTICAST MTU:1500 Metric:1
RX packets:809 errors:0 dropped:0 overruns:0 frame:0
TX packets:20 errors:0 dropped:0 overruns:0 carrier:0
collisions:0 txqueuelen:1000
RX bytes:66050 (66.0 KB) TX bytes:1580 (1.5 KB)
$ sudo ip netns exec ns2 ifconfig veth2
veth2 Link encap:Ethernet HWaddr 82:71:3b:41:d1:ec
inet addr:20.1.1.2 Bcast:20.1.1.255 Mask:255.255.255.0
inet6 addr: fe80::8071:3bff:fe41:d1ec/64 Scope:Link
UP BROADCAST RUNNING MULTICAST MTU:1500 Metric:1
RX packets:862 errors:0 dropped:0 overruns:0 frame:0
TX packets:26 errors:0 dropped:0 overruns:0 carrier:0
collisions:0 txqueuelen:1000
RX bytes:70436 (70.4 KB) TX bytes:2024 (2.0 KB)
$ sudo ip netns exec ns2 ping 20.1.1.1
PING 20.1.1.1 (20.1.1.1) 56(84) bytes of data.
64 bytes from 20.1.1.1: icmp_seq=1 ttl=64 time=0.353 ms
64 bytes from 20.1.1.1: icmp_seq=2 ttl=64 time=0.322 ms
64 bytes from 20.1.1.1: icmp_seq=3 ttl=64 time=0.333 ms
64 bytes from 20.1.1.1: icmp_seq=4 ttl=64 time=0.329 ms
64 bytes from 20.1.1.1: icmp_seq=5 ttl=64 time=0.340 ms
^C
--- 20.1.1.1 ping statistics ---
5 packets transmitted, 5 received, 0% packet loss, time 4099ms
rtt min/avg/max/mdev = 0.322/0.335/0.353/0.019 ms
$ sudo ip netns exec ns1 iperf3 -s -i 10 &
[2] 2851
[1] Exit 1 sudo ip netns exec ns1 iperf3 -s -i 10
$ ---
Server listening on 5201
---
$ sudo ip netns exec ns2 iperf3 -t 60 -i 10 -c 20.1.1.1
iperf3: error - unable to connect to server: Connection timed out
$
iperf3 has always been hanging there, then exit because of timeout, what's
wrong here?
$ sudo ./my-ovs-ofctl -Oopenflow13 dump-flows br-int
cookie=0x0, duration=1076.396s, table=0, n_packets=1522, n_bytes=124264,
priority=0 actions=NORMAL
$
The below is Redhat OSP document for your reference.
https://access.redhat.com/documentation/en-us/red_hat_openstack_platform/12/
html/network_functions_virtualization_planning_and_configuration_guide/part-
dpdk-configure
8.8. Known limitations
There are certain limitations when configuring OVS-DPDK with Red Hat
OpenStack Platform for the NFV use case:
* Use Linux bonds for control plane networks. Ensure both PCI devices
used in the bond are on the same NUMA node for optimum performance. Neutron
Linux bridge configuration is not supported by Red Hat.
* Huge pages are required for every instance running on the hosts with
OVS-DPDK. If huge pages are not present in the guest, the interface appears
but does not function.
* There is a performance degradation of services that use tap devices,
because these devices do not support DPDK. For example, services such as
DVR, FWaaS, and LBaaS use tap devices.
* With OVS-DPDK, you can enable DVR with netdev datapath, but this has
poor performance and is not suitable for a production environment. DVR uses
kernel namespace and tap devices to perform the routing.
* To ensure the DVR routing performs well with OVS-DPDK, you need to
use a controller such as ODL which implements routing as OpenFlow rules.
With OVS-DPDK, OpenFlow routing removes the bottleneck introduced by the
Linux kernel
Re: [ovs-dev] [PATCH RFC v2 0/8] Introduce connection tracking tc offload
Hi. This patch-set has some checkpatch issues. It'll be good if you could fix them before submitting the next version. Best regards, Ilya Maximets. On 04.07.2019 17:28, Paul Blakey wrote: > Hi, > > The following patches add connection tracking offload to tc. > > We plan on offloading the datapath rules to netdev one to one to tc rules. > We'll be using upcoming act_ct tc module which is currently under review in > netdev for the datapath ct() action. > Tc chains and tc goto chain action for the recirc_id() match and recirc() > action. > cls_flower will do the matching on skb conntrack metadata for the ct_state > matches. > > The patchset for act_ct and cls_flower is here: > https://lwn.net/Articles/791584/ > > So datapath ovs connection tracking rules: > recirc_id(0),in_port(ens1f0_0),ct_state(-trk),... actions:ct(zone=2),recirc(2) > recirc_id(2),in_port(ens1f0_0),ct_state(+new+trk),ct_mark(0xbb),... > actions:ct(commit,zone=2,nat(src=5.5.5.7),mark=0xbb),ens1f0_1 > recirc_id(2),in_port(ens1f0_0),ct_state(+est+trk),ct_mark(0xbb),... > actions:ct(zone=2,nat),ens1f0_1 > > recirc_id(1),in_port(ens1f0_1),ct_state(-trk),... actions:ct(zone=2),recirc(1) > recirc_id(1),in_port(ens1f0_1),ct_state(+est+trk),... > actions:ct(zone=2,nat),ens1f0_0 > > Will be translated to these: > $ tc filter add dev ens1f0_0 ingress \ > prio 1 chain 0 proto ip \ > flower ip_proto tcp ct_state -trk \ > action ct zone 2 pipe \ > action goto chain 2 > $ tc filter add dev ens1f0_0 ingress \ > prio 1 chain 2 proto ip \ > flower ct_state +trk+new \ > action ct zone 2 commit mark 0xbb nat src addr 5.5.5.7 pipe \ > action mirred egress redirect dev ens1f0_1 > $ tc filter add dev ens1f0_0 ingress \ > prio 1 chain 2 proto ip \ > flower ct_zone 2 ct_mark 0xbb ct_state +trk+est \ > action ct nat pipe \ > action mirred egress redirect dev ens1f0_1 > > $ tc filter add dev ens1f0_1 ingress \ > prio 1 chain 0 proto ip \ > flower ip_proto tcp ct_state -trk \ > action ct zone 2 pipe \ > action goto chain 1 > $ tc filter add dev ens1f0_1 ingress \ > prio 1 chain 1 proto ip \ > flower ct_zone 2 ct_mark 0xbb ct_state +trk+est \ > action ct nat pipe \ > action mirred egress redirect dev ens1f0_0 > > > Changlog: > V1->V2: > Renamed netdev-tc-offloads to netdev-offload-tc (sorry about double email) > > Paul Blakey (8): > match: Add match_set_ct_zone_masked helper > compat: Add tc ct action and flower matches defines for older kernels > tc: Introduce tc_id to specify a tc filter > netdev-offload-tc: Implement netdev tc flush via tc filter del > netdev-offload-tc: Add recirculation support via tc chains > netdev-offload-tc: Add conntrack support > netdev-offload-tc: Add conntrack label and mark support > netdev-offload-tc: Add conntrack nat support > > acinclude.m4 | 6 +- > include/linux/automake.mk| 3 +- > include/linux/pkt_cls.h | 50 +++- > include/linux/tc_act/tc_ct.h | 41 +++ > include/openvswitch/match.h | 1 + > lib/dpif-netlink.c | 5 + > lib/match.c | 10 +- > lib/netdev-linux.c | 6 +- > lib/netdev-offload-tc.c | 595 > ++- > lib/tc.c | 411 -- > lib/tc.h | 75 +- > 11 files changed, 921 insertions(+), 282 deletions(-) > create mode 100644 include/linux/tc_act/tc_ct.h > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netdev-afxdp: Error when no XDP program loaded.
On 24.07.2019 18:21, William Tu wrote: > netdev-afxdp requires XDP program to be loaded. When prog_id == 0, > it indicates no XDP program, so return error and free resources. > > Signed-off-by: William Tu > --- > v2: combining if statement to avoid duplication > --- > lib/netdev-afxdp.c | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) Thanks! Applied to master. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH RFC v2 0/8] Introduce connection tracking tc offload
On 7/23/2019 6:42 PM, Marcelo Ricardo Leitner wrote: > On Sat, Jul 20, 2019 at 08:26:59AM +, Paul Blakey wrote: >> Hi Marcelo, thanks for reporting this, can you dump the datapath rules via >> ovs-appctl dpctl/dump-flows -m --names >> Also running "tc filter show dev ns2-veth-ab ingress" while it happened can >> show us more details. >> >> I'll try and reproduce it on my end. >> How often does this happen? and what is the setup ? > Quite often. > Setup is: > ns1 ns2 > --. .-- >| v-- br0 --v | > ns1-veth-ba--xns1-veth-ab ns2-veth-abx-- ns2-veth-ba >| | > --' '-- > > Seems related to the skb_ext patches.. with a kernel with more > debugging stuff enabled, I'm seeing: > > [ 17.467576] = > [ 17.468717] WARNING: suspicious RCU usage > [ 17.469687] 5.2.0.c1f3d.g83fb7bc8ff16+ #2 Not tainted > [ 17.471496] - > [ 17.472922] net/sched/sch_ingress.c:52 suspicious > rcu_dereference_protected() usage! > [ 17.474715] > other info that might help us debug this: > > [ 17.476219] > rcu_scheduler_active = 2, debug_locks = 1 > [ 17.477408] no locks held by ovs-vswitchd/1063. > [ 17.478267] > stack backtrace: > [ 17.479545] CPU: 10 PID: 1063 Comm: ovs-vswitchd Not tainted > 5.2.0.c1f3d.g83fb7bc8ff16+ #2 > [ 17.481932] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS > 1.12.0-2.fc30 04/01/2014 > [ 17.483725] Call Trace: > [ 17.484179] dump_stack+0x85/0xc0 > [ 17.484782] ingress_tcf_block+0x4c/0x50 [sch_ingress] > [ 17.485717] __tcf_block_find+0x28/0x80 > [ 17.486419] tc_new_tfilter+0x1b6/0x960 > [ 17.487093] ? tc_del_tfilter+0x720/0x720 > [ 17.487854] rtnetlink_rcv_msg+0x389/0x4b0 > [ 17.488590] ? netlink_deliver_tap+0x95/0x400 > [ 17.489343] ? rtnl_dellink+0x2d0/0x2d0 > [ 17.490034] netlink_rcv_skb+0x49/0x110 > [ 17.490726] netlink_unicast+0x171/0x200 > [ 17.491409] netlink_sendmsg+0x21e/0x3e0 > [ 17.492096] sock_sendmsg+0x5e/0x60 > [ 17.492792] ___sys_sendmsg+0x2ae/0x330 > [ 17.493725] ? ___sys_recvmsg+0x159/0x1f0 > [ 17.494734] ? up_write+0x1c/0xc0 > [ 17.495515] ? ext4_file_write_iter+0xd1/0x3b0 > [ 17.496624] ? find_held_lock+0x2b/0x80 > [ 17.497362] ? ksys_write+0xc0/0xe0 > [ 17.498000] __sys_sendmsg+0x59/0xa0 > [ 17.498653] do_syscall_64+0x5c/0xa0 > [ 17.499307] entry_SYSCALL_64_after_hwframe+0x49/0xbe > [ 17.500225] RIP: 0033:0x7f6bd4ab2b45 > > And I could trigger a crash with this trace: > (crash was on a different run, while the RCU trace above and outputs below are > from the same run) > > [ 382.670318] BUG: kernel NULL pointer dereference, address: 0030 > [ 382.671927] #PF: supervisor read access in kernel mode > [ 382.673070] #PF: error_code(0x) - not-present page > [ 382.674170] PGD 0 P4D 0 > [ 382.674873] Oops: [#1] SMP NOPTI > [ 382.675753] CPU: 1 PID: 6988 Comm: nc Kdump: loaded Not tainted > 5.2.0.c1f3d.g83fb7bc8ff16+ #1 > [ 382.677350] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS > 1.12.0-2.fc30 04/01/2014 > [ 382.679255] RIP: 0010:tcf_classify+0x104/0x1e0 > [ 382.680942] Code: 5c 41 5d 41 5e 41 5f c3 48 8b 55 00 be 01 00 00 00 4c 89 > ff 48 89 14 24 e8 79 e3 f9 ff 48 8b 14 24 48 85 c0 0f 84 cb 00 00 > 00 <48> 8b 4a 30 8b 49 40 89 08 41 8d 45 01 41 83 fd 03 7e ac e8 84 37 > [ 382.684792] RSP: 0018:bd19000f4dd0 EFLAGS: 00010282 > [ 382.685746] RAX: 9e04772b0008 RBX: 9e0479088360 RCX: > 0001 > [ 382.686929] RDX: RSI: 0001 RDI: > 9e047ad9a140 > [ 382.688144] RBP: bd19000f4e40 R08: 9e047b870b60 R09: > 0002 > [ 382.689347] R10: 9e04550ccd68 R11: 9e0476050a00 R12: > 0001 > [ 382.690534] R13: R14: 9e0479088360 R15: > 9e0479229ce0 > [ 382.691730] FS: 7efd4d75bb80() GS:9e047b84() > knlGS: > [ 382.693033] CS: 0010 DS: ES: CR0: 80050033 > [ 382.694061] CR2: 0030 CR3: 000163abc000 CR4: > 003406e0 > [ 382.695283] Call Trace: > [ 382.695962] > [ 382.696573] __netif_receive_skb_core+0x3c0/0xcf0 > [ 382.697492] ? reweight_entity+0x15b/0x1a0 > [ 382.698340] __netif_receive_skb_one_core+0x37/0x90 > [ 382.699274] process_backlog+0x9c/0x150 > [ 382.700199] net_rx_action+0xff/0x350 > [ 382.701093] __do_softirq+0xee/0x2ff > [ 382.701976] do_softirq_own_stack+0x2a/0x40 > [ 382.702948] > [ 382.703626] do_softirq.part.0+0x41/0x50 > [ 382.704556] __local_bh_enable_ip+0x4b/0x50 > [ 382.705536] ip_finish_output2+0x1a9/0x580 > [ 382.706518]
[ovs-dev] [ovn] OVN split: Remove make "modules_install" target
From: Lucas Alvares Gomes This patch is removing the make "modules_install" target for the new OVN repository. The target is not be needed for OVN (and is currently broken) becasue the kernel modules should be compiled and installed using the OVS repository. Signed-off-by: Lucas Alvares Gomes --- Makefile.am | 5 - 1 file changed, 5 deletions(-) diff --git a/Makefile.am b/Makefile.am index e3dea1912..2a629b425 100644 --- a/Makefile.am +++ b/Makefile.am @@ -484,11 +484,6 @@ install-data-local: $(INSTALL_DATA_LOCAL) uninstall-local: $(UNINSTALL_LOCAL) .PHONY: $(DIST_HOOKS) $(CLEAN_LOCAL) $(INSTALL_DATA_LOCAL) $(UNINSTALL_LOCAL) -modules_install: -if LINUX_ENABLED - cd datapath/linux && $(MAKE) modules_install -endif - dist-docs: VERSION=$(VERSION) MAKE='$(MAKE)' $(srcdir)/build-aux/dist-docs $(srcdir) $(docs) .PHONY: dist-docs -- 2.22.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 ovn] Include common ovn header files from include/ovn instead of ovs/include/ovn
On Mon, Jul 29, 2019 at 1:02 PM wrote:
>
> From: Numan Siddique
>
> For the other header files present in lib/, the previous commit [1]
> changed the path. But few were left out. This patch fixes them too.
>
> Also updated the end comments in the header files with the correct path.
>
> [1] - a469954c00c4("Include ovn header files from lib/ instead of ovn/lib/")
>
> Signed-off-by: Numan Siddique
Acked-by: Dumitru Ceara
> ---
>
> v2 -> v3
> ==
> * Updated the end comments in the header files.
>
> v1 -> v2
> ===
> * Addressed Dumitru's comments and updated lib/chassis-index.c
>
>
> Makefile.am | 2 ++
> controller/binding.h| 2 +-
> controller/chassis.h| 2 +-
> controller/encaps.h | 2 +-
> controller/ip-mcast.h | 2 +-
> controller/lflow.h | 2 +-
> controller/lport.h | 2 +-
> controller/ofctrl.h | 2 +-
> controller/ovn-controller.h | 2 +-
> controller/patch.h | 2 +-
> controller/physical.h | 2 +-
> controller/pinctrl.h| 2 +-
> lib/acl-log.h | 2 +-
> lib/chassis-index.c | 4 ++--
> lib/chassis-index.h | 2 +-
> lib/extend-table.h | 2 +-
> lib/inc-proc-eng.h | 2 +-
> lib/ip-mcast-index.c| 4 ++--
> lib/ip-mcast-index.h| 2 +-
> lib/mcast-group-index.h | 2 +-
> lib/ovn-sb-idl.ann | 4 ++--
> 21 files changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index e3dea1912..4fe0d2899 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -19,6 +19,8 @@ AM_CPPFLAGS = $(SSL_CFLAGS)
> AM_LDFLAGS = $(SSL_LDFLAGS)
> AM_LDFLAGS += $(OVS_LDFLAGS)
>
> +AM_CPPFLAGS += -I $(top_srcdir)/include
> +
> if WIN32
> AM_CPPFLAGS += -I $(top_srcdir)/ovs/include
> AM_CPPFLAGS += -I $(top_srcdir)/ovs/lib
> diff --git a/controller/binding.h b/controller/binding.h
> index 8d9492630..bae162ede 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -54,4 +54,4 @@ bool binding_evaluate_port_binding_changes(
> struct sset *active_tunnels,
> struct sset *local_lports);
>
> -#endif /* ovn/binding.h */
> +#endif /* controller/binding.h */
> diff --git a/controller/chassis.h b/controller/chassis.h
> index 16a131a3b..eb46ca3fc 100644
> --- a/controller/chassis.h
> +++ b/controller/chassis.h
> @@ -43,4 +43,4 @@ bool chassis_get_mac(const struct sbrec_chassis *chassis,
> struct eth_addr *chassis_mac);
> const char *chassis_get_id(void);
>
> -#endif /* ovn/chassis.h */
> +#endif /* controller/chassis.h */
> diff --git a/controller/encaps.h b/controller/encaps.h
> index afa41830a..c919d18e6 100644
> --- a/controller/encaps.h
> +++ b/controller/encaps.h
> @@ -45,4 +45,4 @@ bool encaps_tunnel_id_parse(const char *tunnel_id, char
> **chassis_id,
> bool encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id,
> const char *encap_ip);
>
> -#endif /* ovn/encaps.h */
> +#endif /* controller/encaps.h */
> diff --git a/controller/ip-mcast.h b/controller/ip-mcast.h
> index 6014f43d5..b3447d4c7 100644
> --- a/controller/ip-mcast.h
> +++ b/controller/ip-mcast.h
> @@ -49,4 +49,4 @@ void igmp_group_delete(const struct sbrec_igmp_group *g);
> bool igmp_group_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> struct ovsdb_idl_index *igmp_groups);
>
> -#endif /* ovn/controller/ip-mcast.h */
> +#endif /* controller/ip-mcast.h */
> diff --git a/controller/lflow.h b/controller/lflow.h
> index 4e1086eb6..54da00b49 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -181,4 +181,4 @@ void lflow_handle_changed_neighbors(
>
> void lflow_destroy(void);
>
> -#endif /* ovn/lflow.h */
> +#endif /* controller/lflow.h */
> diff --git a/controller/lport.h b/controller/lport.h
> index 7dcd5bee0..2d4bb7164 100644
> --- a/controller/lport.h
> +++ b/controller/lport.h
> @@ -49,4 +49,4 @@ const struct sbrec_multicast_group
> *mcgroup_lookup_by_dp_name(
> struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath,
> const struct sbrec_datapath_binding *, const char *name);
>
> -#endif /* ovn/lport.h */
> +#endif /* controller/lport.h */
> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> index ed8918aae..114c9ef65 100644
> --- a/controller/ofctrl.h
> +++ b/controller/ofctrl.h
> @@ -84,4 +84,4 @@ void ofctrl_check_and_add_flow(struct
> ovn_desired_flow_table *,
> bool ofctrl_is_connected(void);
> void ofctrl_set_probe_interval(int probe_interval);
>
> -#endif /* ovn/ofctrl.h */
> +#endif /* controller/ofctrl.h */
> diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h
> index be34a24c0..41feec378 100644
> --- a/controller/ovn-controller.h
> +++ b/controller/ovn-controller.h
> @@ -82,4 +82,4 @@ enum chassis_tunnel_type {
>
> uint32_t get_tunnel_type(const char *name);
>
> -#endif /* ovn/ovn-controller.h */
> +#endif /* controller/ovn-controller.h */
> diff --git
[ovs-dev] [PATCH] test: do not require python2 for CHECK_CONNTRACK macro
Do not strictly require python2 for CHECK_CONNTRACK macro definitions in
system-{kmod,userspace}-macros.at
Signed-off-by: Lorenzo Bianconi
---
tests/system-kmod-macros.at | 2 +-
tests/system-userspace-macros.at | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
index 554a61e9b..48e94642b 100644
--- a/tests/system-kmod-macros.at
+++ b/tests/system-kmod-macros.at
@@ -59,7 +59,7 @@ m4_define([CONFIGURE_VETH_OFFLOADS],
# kernel conntrack tables when the test is finished.
#
m4_define([CHECK_CONNTRACK],
-[AT_SKIP_IF([test $HAVE_PYTHON2 = no])
+[AT_SKIP_IF([test $HAVE_PYTHON = no])
m4_foreach([mod], [[nf_conntrack_ipv4], [nf_conntrack_ipv6], [nf_nat_ftp],
[nf_nat_tftp]],
[modprobe mod || echo "Module mod not loaded."
diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at
index 9d5f3bf41..a411e3d89 100644
--- a/tests/system-userspace-macros.at
+++ b/tests/system-userspace-macros.at
@@ -65,7 +65,7 @@ m4_define([CONFIGURE_VETH_OFFLOADS],
# Perform requirements checks for running conntrack tests.
#
m4_define([CHECK_CONNTRACK],
-[AT_SKIP_IF([test $HAVE_PYTHON2 = no])]
+[AT_SKIP_IF([test $HAVE_PYTHON = no])]
)
# CHECK_CONNTRACK_ALG()
--
2.21.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] OVN: fix DNAT/SNAT system-ovn unit tests
Fix conntrack checks in the following tests in tests/system-ovn.at:
- ovn -- DNAT and SNAT on distributed router - N/S
- ovn -- DNAT and SNAT on distributed router - E/W
Fixes: a6ee09882283 ("OVN: run local logical flows first in S_ROUTER_OUT_SNAT
table")
Signed-off-by: Lorenzo Bianconi
---
tests/system-ovn.at | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 10fbd2649..f88ad31e4 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -1334,11 +1334,13 @@ NS_CHECK_EXEC([foo2], [ping -q -c 3 -i 0.3 -w 2
172.16.1.2 | FORMAT_PING], \
])
# We verify that SNAT indeed happened via 'dump-conntrack' command.
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.4) | \
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.1) | \
sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl
-icmp,orig=(src=192.168.1.3,dst=172.16.1.2,id=,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.4,id=,type=0,code=0),zone=
+icmp,orig=(src=192.168.1.3,dst=172.16.1.2,id=,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.1,id=,type=0,code=0),zone=
])
+AT_CHECK([ovs-appctl dpctl/flush-conntrack])
+
# South-North SNAT: 'bar1' pings 'alice1'. But 'alice1' receives traffic
# from 172.16.1.1
NS_CHECK_EXEC([bar1], [ping -q -c 3 -i 0.3 -w 2 172.16.1.2 | FORMAT_PING], \
@@ -1507,12 +1509,14 @@ NS_CHECK_EXEC([foo1], [ping -q -c 3 -i 0.3 -w 2
172.16.1.4 | FORMAT_PING], \
# Check conntrack entries. First SNAT of 'foo1' address happens.
# Then DNAT of 'bar1' address happens (listed first below).
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.3) | \
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.4) | \
sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl
-icmp,orig=(src=172.16.1.3,dst=172.16.1.4,id=,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.3,id=,type=0,code=0),zone=
-icmp,orig=(src=192.168.1.2,dst=172.16.1.4,id=,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.3,id=,type=0,code=0),zone=
+icmp,orig=(src=172.16.1.1,dst=172.16.1.4,id=,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.1,id=,type=0,code=0),zone=
+icmp,orig=(src=192.168.1.2,dst=172.16.1.4,id=,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.1,id=,type=0,code=0),zone=
])
+AT_CHECK([ovs-appctl dpctl/flush-conntrack])
+
# East-West NAT: 'foo2' pings 'bar1' using 172.16.1.4.
NS_CHECK_EXEC([foo2], [ping -q -c 3 -i 0.3 -w 2 172.16.1.4 | FORMAT_PING], \
[0], [dnl
--
2.21.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 ovn] Include common ovn header files from include/ovn instead of ovs/include/ovn
From: Numan Siddique
For the other header files present in lib/, the previous commit [1]
changed the path. But few were left out. This patch fixes them too.
Also updated the end comments in the header files with the correct path.
[1] - a469954c00c4("Include ovn header files from lib/ instead of ovn/lib/")
Signed-off-by: Numan Siddique
---
v2 -> v3
==
* Updated the end comments in the header files.
v1 -> v2
===
* Addressed Dumitru's comments and updated lib/chassis-index.c
Makefile.am | 2 ++
controller/binding.h| 2 +-
controller/chassis.h| 2 +-
controller/encaps.h | 2 +-
controller/ip-mcast.h | 2 +-
controller/lflow.h | 2 +-
controller/lport.h | 2 +-
controller/ofctrl.h | 2 +-
controller/ovn-controller.h | 2 +-
controller/patch.h | 2 +-
controller/physical.h | 2 +-
controller/pinctrl.h| 2 +-
lib/acl-log.h | 2 +-
lib/chassis-index.c | 4 ++--
lib/chassis-index.h | 2 +-
lib/extend-table.h | 2 +-
lib/inc-proc-eng.h | 2 +-
lib/ip-mcast-index.c| 4 ++--
lib/ip-mcast-index.h| 2 +-
lib/mcast-group-index.h | 2 +-
lib/ovn-sb-idl.ann | 4 ++--
21 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index e3dea1912..4fe0d2899 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -19,6 +19,8 @@ AM_CPPFLAGS = $(SSL_CFLAGS)
AM_LDFLAGS = $(SSL_LDFLAGS)
AM_LDFLAGS += $(OVS_LDFLAGS)
+AM_CPPFLAGS += -I $(top_srcdir)/include
+
if WIN32
AM_CPPFLAGS += -I $(top_srcdir)/ovs/include
AM_CPPFLAGS += -I $(top_srcdir)/ovs/lib
diff --git a/controller/binding.h b/controller/binding.h
index 8d9492630..bae162ede 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -54,4 +54,4 @@ bool binding_evaluate_port_binding_changes(
struct sset *active_tunnels,
struct sset *local_lports);
-#endif /* ovn/binding.h */
+#endif /* controller/binding.h */
diff --git a/controller/chassis.h b/controller/chassis.h
index 16a131a3b..eb46ca3fc 100644
--- a/controller/chassis.h
+++ b/controller/chassis.h
@@ -43,4 +43,4 @@ bool chassis_get_mac(const struct sbrec_chassis *chassis,
struct eth_addr *chassis_mac);
const char *chassis_get_id(void);
-#endif /* ovn/chassis.h */
+#endif /* controller/chassis.h */
diff --git a/controller/encaps.h b/controller/encaps.h
index afa41830a..c919d18e6 100644
--- a/controller/encaps.h
+++ b/controller/encaps.h
@@ -45,4 +45,4 @@ bool encaps_tunnel_id_parse(const char *tunnel_id, char
**chassis_id,
bool encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id,
const char *encap_ip);
-#endif /* ovn/encaps.h */
+#endif /* controller/encaps.h */
diff --git a/controller/ip-mcast.h b/controller/ip-mcast.h
index 6014f43d5..b3447d4c7 100644
--- a/controller/ip-mcast.h
+++ b/controller/ip-mcast.h
@@ -49,4 +49,4 @@ void igmp_group_delete(const struct sbrec_igmp_group *g);
bool igmp_group_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *igmp_groups);
-#endif /* ovn/controller/ip-mcast.h */
+#endif /* controller/ip-mcast.h */
diff --git a/controller/lflow.h b/controller/lflow.h
index 4e1086eb6..54da00b49 100644
--- a/controller/lflow.h
+++ b/controller/lflow.h
@@ -181,4 +181,4 @@ void lflow_handle_changed_neighbors(
void lflow_destroy(void);
-#endif /* ovn/lflow.h */
+#endif /* controller/lflow.h */
diff --git a/controller/lport.h b/controller/lport.h
index 7dcd5bee0..2d4bb7164 100644
--- a/controller/lport.h
+++ b/controller/lport.h
@@ -49,4 +49,4 @@ const struct sbrec_multicast_group *mcgroup_lookup_by_dp_name(
struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath,
const struct sbrec_datapath_binding *, const char *name);
-#endif /* ovn/lport.h */
+#endif /* controller/lport.h */
diff --git a/controller/ofctrl.h b/controller/ofctrl.h
index ed8918aae..114c9ef65 100644
--- a/controller/ofctrl.h
+++ b/controller/ofctrl.h
@@ -84,4 +84,4 @@ void ofctrl_check_and_add_flow(struct ovn_desired_flow_table
*,
bool ofctrl_is_connected(void);
void ofctrl_set_probe_interval(int probe_interval);
-#endif /* ovn/ofctrl.h */
+#endif /* controller/ofctrl.h */
diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h
index be34a24c0..41feec378 100644
--- a/controller/ovn-controller.h
+++ b/controller/ovn-controller.h
@@ -82,4 +82,4 @@ enum chassis_tunnel_type {
uint32_t get_tunnel_type(const char *name);
-#endif /* ovn/ovn-controller.h */
+#endif /* controller/ovn-controller.h */
diff --git a/controller/patch.h b/controller/patch.h
index dd052cfd8..9018e4967 100644
--- a/controller/patch.h
+++ b/controller/patch.h
@@ -39,4 +39,4 @@ void patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
const struct ovsrec_bridge *br_int,
const struct sbrec_chassis *);
-#endif /* ovn/patch.h */
+#endif /*
Re: [ovs-dev] [PATCH 00/12] Support zone-based conntrack timeout policy
On 29.07.2019 12:21, Ilya Maximets wrote:
> On 26.07.2019 2:24, Yi-Hung Wei wrote:
>> This patch series enables zone-based conntrack timeout policy support in OVS.
>> Timeout policy is a set of timeout attributes that can be associated with a
>> connection when it is committed. Then, the connection tracking system will
>> expire a connection based on its connection state. For example, one use
>> case would be to extend the timeout of TCP connection in the established
>> state to avoid re-connect overhead. Or use case is to shorten the connection
>> timeout so that the system can reclaim resources faster.
>> The idea of zone-based conntrack timeout policy is to group connections
>> with similar characteristics in a conntrack zone, and assign timeout policy
>> to the conntrack zone. Therefore, all the connections in that zone will share
>> the same timeout policy.
>>
>> For zone-based timeout policy configuration, the association of conntrack
>> zone and conntrack timeout policy is defined per datapath in vswitch ovsdb
>> schema. User can program the database through ovs-vsctl or using ovsdb
>> protocol directly. Once the zone-based timeout policy configuration is
>> in the database, vswitchd will read those configuration and orgaznie it
>> in internal datapath strcture, and push the timeout policy into datapath.
>> Currenlty, only the kernel datapath supports customized timeout policy.
>
> Hi everyone,
>
> My 2 cents for the feature design:
>
>>From the user's perspective:
>
> * 'add-dp'/'del-dp' commands looks very strange.
> "I didn't add datapath into ovsdb, why it exists and switches packets?"
> "I deleted the datapath from the OVS, why it still exists and switches
> packets?"
>
> If you're implementing the configuration like this, 'datapath' should
> own the bridges and interfaces, i.e. datapath should be created manually
> on 'add-dp' and automatically on adding the first bridge on that datapath.
> All the bridges and interfaces must be deleted/destroyed on 'del-dp'.
>
> Or you need to rename your tables and commands to not look like this.
>
>>From the developer's perspective:
>
> * Right now 'ofproto-dpif' is the only module that manages datapath interfaces
> and it knows that (there are specific comments in the code). 'dpif's has
> no reference counts and it's slightly unsafe to manage them outside of
> 'ofproto-dpif'.
> You're adding the side module that allowed to open dpif (and it's not able
> to delete it, that is the possible cause if issues) and use it without
> noticing any other modules. This breaks the hierarchical structure of OVS.
>
> * Right now most of the datapath configuration is done via 'other_config'
> and corresponding dpif_set_config() callback. Since you're introducing
> datapath-config module, it should take care of all of this staff. And this
> will require significant rework of the current datapath configuration
> scheme.
>
> * 'reconfigure_datapath' is an ambiguous name.
>
> Solution for above issues might be not introducing the new modules at all.
> Everything could be handled like we're handling meters, but with OVSDB as
> the
> configuration source. On configuration change bridge layer will call ofproto
> layer that will pass configuration to ofproto-dpif and, finally, dpif layer.
> Inside 'struct dpif' in dpif.c module you could track all the configuration
> and pass all the required changes to the dpif-provider via callbacks.
> This way everything will work fine without breaking current OVS hierarchy.
>
> * DB scheme looks just overcomplicated. 3 additional tables which references
> others just to store a string to integer map.
> I think that it might be much easier to create a single 'CT_Zones' table
> with all the required columns:
> 'id', 'tcp_syn_sent', 'tcp_syn_recv', ..., 'icmp_reply'.
> This is not a big deal since you need to describe every single field in
> vswitch.xml anyway. This will also allow you to check support of particular
> field on the stage of adding value to the database.
Another option is a single 'CT_Zones' table with 'id' and 'timeouts' columns.
Where 'timeouts' is a map {string --> integer}.
So, something like this will work:
ovs-vsctl create CT_Zones 1 -- set CT_Zones 1 timeouts:icmp_first=60
timeouts:icmp_reply=60
> If you really need to distinguish zones by the datapath type (which is not
> obvious), you may add 'datapath_type' column, just like we have in a
> 'Bridge'
> table.
>
>
> Best regards, Ilya Maximets.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Include common ovn header files from include/ovn instead of ovs/include/ovn
On Mon, Jul 29, 2019 at 4:00 PM Dumitru Ceara wrote:
> On Mon, Jul 29, 2019 at 12:02 PM wrote:
> >
> > From: Numan Siddique
> >
> > For the other header files present in lib/, the previous commit [1]
> > changed the path. But few were left out. This patch fixes them too.
> >
> > [1] - a469954c00c4("Include ovn header files from lib/ instead of
> ovn/lib/")
> >
> > Signed-off-by: Numan Siddique
>
> Hi Numan,
>
> I think these two got missed too:
> lib/chassis-index.c
> lib/chassis-index.h
>
Thanks for pointing this out,
I submitted v2.
> Thanks,
> Dumitru
>
> > ---
> > Makefile.am | 2 ++
> > lib/ip-mcast-index.c | 4 ++--
> > lib/ovn-sb-idl.ann | 4 ++--
> > 3 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index e3dea1912..4fe0d2899 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -19,6 +19,8 @@ AM_CPPFLAGS = $(SSL_CFLAGS)
> > AM_LDFLAGS = $(SSL_LDFLAGS)
> > AM_LDFLAGS += $(OVS_LDFLAGS)
> >
> > +AM_CPPFLAGS += -I $(top_srcdir)/include
> > +
> > if WIN32
> > AM_CPPFLAGS += -I $(top_srcdir)/ovs/include
> > AM_CPPFLAGS += -I $(top_srcdir)/ovs/lib
> > diff --git a/lib/ip-mcast-index.c b/lib/ip-mcast-index.c
> > index 1f6ebc4ae..6b01041cc 100644
> > --- a/lib/ip-mcast-index.c
> > +++ b/lib/ip-mcast-index.c
> > @@ -15,8 +15,8 @@
> >
> > #include
> >
> > -#include "ovn/lib/ip-mcast-index.h"
> > -#include "ovn/lib/ovn-sb-idl.h"
> > +#include "lib/ip-mcast-index.h"
> > +#include "lib/ovn-sb-idl.h"
> >
> > struct ovsdb_idl_index *
> > ip_mcast_index_create(struct ovsdb_idl *idl)
> > diff --git a/lib/ovn-sb-idl.ann b/lib/ovn-sb-idl.ann
> > index e51238b92..22124b868 100644
> > --- a/lib/ovn-sb-idl.ann
> > +++ b/lib/ovn-sb-idl.ann
> > @@ -6,9 +6,9 @@
> > # it can generate more programmer-friendly data structures.
> >
> > s["idlPrefix"] = "sbrec_"
> > -s["idlHeader"] = "\"ovn/lib/ovn-sb-idl.h\""
> > +s["idlHeader"] = "\"lib/ovn-sb-idl.h\""
> >
> > -s["hDecls"] = '#include "ovn/lib/ovn-util.h"'
> > +s["hDecls"] = '#include "lib/ovn-util.h"'
> >
> > # Adds an integer column named 'column' to 'table' in 's'. The column
> > # values is calculated with 'expression' based on the values of the
> columns
> > --
> > 2.21.0
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCHv2 ovn] Include common ovn header files from include/ovn instead of ovs/include/ovn
From: Numan Siddique
For the other header files present in lib/, the previous commit [1]
changed the path. But few were left out. This patch fixes them too.
[1] - a469954c00c4("Include ovn header files from lib/ instead of ovn/lib/")
Signed-off-by: Numan Siddique
---
v1 -> v2
===
* Addressed Dumitru's comments and updated lib/chassis-index.c
Makefile.am | 2 ++
lib/chassis-index.c | 4 ++--
lib/ip-mcast-index.c | 4 ++--
lib/ovn-sb-idl.ann | 4 ++--
4 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index e3dea1912..4fe0d2899 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -19,6 +19,8 @@ AM_CPPFLAGS = $(SSL_CFLAGS)
AM_LDFLAGS = $(SSL_LDFLAGS)
AM_LDFLAGS += $(OVS_LDFLAGS)
+AM_CPPFLAGS += -I $(top_srcdir)/include
+
if WIN32
AM_CPPFLAGS += -I $(top_srcdir)/ovs/include
AM_CPPFLAGS += -I $(top_srcdir)/ovs/lib
diff --git a/lib/chassis-index.c b/lib/chassis-index.c
index 10f70fb4a..39066f4cc 100644
--- a/lib/chassis-index.c
+++ b/lib/chassis-index.c
@@ -13,8 +13,8 @@
*/
#include
-#include "ovn/lib/chassis-index.h"
-#include "ovn/lib/ovn-sb-idl.h"
+#include "lib/chassis-index.h"
+#include "lib/ovn-sb-idl.h"
struct ovsdb_idl_index *
chassis_index_create(struct ovsdb_idl *idl)
diff --git a/lib/ip-mcast-index.c b/lib/ip-mcast-index.c
index 1f6ebc4ae..6b01041cc 100644
--- a/lib/ip-mcast-index.c
+++ b/lib/ip-mcast-index.c
@@ -15,8 +15,8 @@
#include
-#include "ovn/lib/ip-mcast-index.h"
-#include "ovn/lib/ovn-sb-idl.h"
+#include "lib/ip-mcast-index.h"
+#include "lib/ovn-sb-idl.h"
struct ovsdb_idl_index *
ip_mcast_index_create(struct ovsdb_idl *idl)
diff --git a/lib/ovn-sb-idl.ann b/lib/ovn-sb-idl.ann
index e51238b92..22124b868 100644
--- a/lib/ovn-sb-idl.ann
+++ b/lib/ovn-sb-idl.ann
@@ -6,9 +6,9 @@
# it can generate more programmer-friendly data structures.
s["idlPrefix"] = "sbrec_"
-s["idlHeader"] = "\"ovn/lib/ovn-sb-idl.h\""
+s["idlHeader"] = "\"lib/ovn-sb-idl.h\""
-s["hDecls"] = '#include "ovn/lib/ovn-util.h"'
+s["hDecls"] = '#include "lib/ovn-util.h"'
# Adds an integer column named 'column' to 'table' in 's'. The column
# values is calculated with 'expression' based on the values of the columns
--
2.21.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Include common ovn header files from include/ovn instead of ovs/include/ovn
On Mon, Jul 29, 2019 at 12:02 PM wrote:
>
> From: Numan Siddique
>
> For the other header files present in lib/, the previous commit [1]
> changed the path. But few were left out. This patch fixes them too.
>
> [1] - a469954c00c4("Include ovn header files from lib/ instead of ovn/lib/")
>
> Signed-off-by: Numan Siddique
Hi Numan,
I think these two got missed too:
lib/chassis-index.c
lib/chassis-index.h
Thanks,
Dumitru
> ---
> Makefile.am | 2 ++
> lib/ip-mcast-index.c | 4 ++--
> lib/ovn-sb-idl.ann | 4 ++--
> 3 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index e3dea1912..4fe0d2899 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -19,6 +19,8 @@ AM_CPPFLAGS = $(SSL_CFLAGS)
> AM_LDFLAGS = $(SSL_LDFLAGS)
> AM_LDFLAGS += $(OVS_LDFLAGS)
>
> +AM_CPPFLAGS += -I $(top_srcdir)/include
> +
> if WIN32
> AM_CPPFLAGS += -I $(top_srcdir)/ovs/include
> AM_CPPFLAGS += -I $(top_srcdir)/ovs/lib
> diff --git a/lib/ip-mcast-index.c b/lib/ip-mcast-index.c
> index 1f6ebc4ae..6b01041cc 100644
> --- a/lib/ip-mcast-index.c
> +++ b/lib/ip-mcast-index.c
> @@ -15,8 +15,8 @@
>
> #include
>
> -#include "ovn/lib/ip-mcast-index.h"
> -#include "ovn/lib/ovn-sb-idl.h"
> +#include "lib/ip-mcast-index.h"
> +#include "lib/ovn-sb-idl.h"
>
> struct ovsdb_idl_index *
> ip_mcast_index_create(struct ovsdb_idl *idl)
> diff --git a/lib/ovn-sb-idl.ann b/lib/ovn-sb-idl.ann
> index e51238b92..22124b868 100644
> --- a/lib/ovn-sb-idl.ann
> +++ b/lib/ovn-sb-idl.ann
> @@ -6,9 +6,9 @@
> # it can generate more programmer-friendly data structures.
>
> s["idlPrefix"] = "sbrec_"
> -s["idlHeader"] = "\"ovn/lib/ovn-sb-idl.h\""
> +s["idlHeader"] = "\"lib/ovn-sb-idl.h\""
>
> -s["hDecls"] = '#include "ovn/lib/ovn-util.h"'
> +s["hDecls"] = '#include "lib/ovn-util.h"'
>
> # Adds an integer column named 'column' to 'table' in 's'. The column
> # values is calculated with 'expression' based on the values of the columns
> --
> 2.21.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] Include common ovn header files from include/ovn instead of ovs/include/ovn
From: Numan Siddique
For the other header files present in lib/, the previous commit [1]
changed the path. But few were left out. This patch fixes them too.
[1] - a469954c00c4("Include ovn header files from lib/ instead of ovn/lib/")
Signed-off-by: Numan Siddique
---
Makefile.am | 2 ++
lib/ip-mcast-index.c | 4 ++--
lib/ovn-sb-idl.ann | 4 ++--
3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index e3dea1912..4fe0d2899 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -19,6 +19,8 @@ AM_CPPFLAGS = $(SSL_CFLAGS)
AM_LDFLAGS = $(SSL_LDFLAGS)
AM_LDFLAGS += $(OVS_LDFLAGS)
+AM_CPPFLAGS += -I $(top_srcdir)/include
+
if WIN32
AM_CPPFLAGS += -I $(top_srcdir)/ovs/include
AM_CPPFLAGS += -I $(top_srcdir)/ovs/lib
diff --git a/lib/ip-mcast-index.c b/lib/ip-mcast-index.c
index 1f6ebc4ae..6b01041cc 100644
--- a/lib/ip-mcast-index.c
+++ b/lib/ip-mcast-index.c
@@ -15,8 +15,8 @@
#include
-#include "ovn/lib/ip-mcast-index.h"
-#include "ovn/lib/ovn-sb-idl.h"
+#include "lib/ip-mcast-index.h"
+#include "lib/ovn-sb-idl.h"
struct ovsdb_idl_index *
ip_mcast_index_create(struct ovsdb_idl *idl)
diff --git a/lib/ovn-sb-idl.ann b/lib/ovn-sb-idl.ann
index e51238b92..22124b868 100644
--- a/lib/ovn-sb-idl.ann
+++ b/lib/ovn-sb-idl.ann
@@ -6,9 +6,9 @@
# it can generate more programmer-friendly data structures.
s["idlPrefix"] = "sbrec_"
-s["idlHeader"] = "\"ovn/lib/ovn-sb-idl.h\""
+s["idlHeader"] = "\"lib/ovn-sb-idl.h\""
-s["hDecls"] = '#include "ovn/lib/ovn-util.h"'
+s["hDecls"] = '#include "lib/ovn-util.h"'
# Adds an integer column named 'column' to 'table' in 's'. The column
# values is calculated with 'expression' based on the values of the columns
--
2.21.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [[ovn]] OVN split: Fix modules_install
Hi, On Mon, Jul 29, 2019 at 10:25 AM Numan Siddique wrote: > > On Mon, Jul 29, 2019 at 2:30 PM wrote: > > > From: Lucas Alvares Gomes > > > > The Makefile for modules_install should point to the ovs subrepository > > to be able to compile the ovs kernel modules (otherwise the compilation > > fails). > > > > Signed-off-by: Lucas Alvares Gomes > > > > I think it is better to delete this target rather than fixing it. > Ideally OVS repo should be used to build the kernel module . > Got it, thanks for the advice. I was updating networking-ovn (the OpenStack driver for OVN) to use the new repository and found that problem. I will remove the target as you suggested and use the OVS repository to compile the kernel modules. Cheers, Lucas ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] OVN: Fix learning of neighbors from ARP/ND packets.
On Fri, Jul 26, 2019 at 8:33 AM Dumitru Ceara wrote:
>
> Add a restriction on the target protocol addresses to match the configured
> subnets. All other ARP/ND packets are invalid in this context.
>
> One exception is for ARP replies that are received for route next-hops
> that are only reachable via a port but can't be directly resolved
> through route lookups. Such support was introduced by commit:
>
> 6b785fd8fe29 ("ovn-util: Allow /32 IP addresses for router ports.")
>
> Reported-at: https://bugzilla.redhat.com/1729846
> Reported-by: Haidong Li
> CC: Han Zhou
> CC: Guru Shetty
> Fixes: b068454082f5 ("ovn-northd: Support learning neighbor from ARP
> request.")
> Signed-off-by: Dumitru Ceara
> ---
>
> v2:
> - Update commit message.
> - Implement the fix also for ARP replies and IPv6 ND.
Hi Han, Guru,
I resubmitted this patch against the ovn-org repo. The new patchwork
is: https://patchwork.ozlabs.org/patch/1138261/
Thanks,
Dumitru
> ---
> ovn/northd/ovn-northd.c | 95
> +
> 1 file changed, 81 insertions(+), 14 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index eb6c47c..1e3ec68 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -5815,10 +5815,32 @@ build_static_route_flow(struct hmap *lflows, struct
> ovn_datapath *od,
> if (is_ipv4) {
> if (out_port->lrp_networks.n_ipv4_addrs) {
> lrp_addr_s = out_port->lrp_networks.ipv4_addrs[0].addr_s;
> +
> +/* Explicitly allow ARP replies for the next-hop. */
> +struct ds match;
> +ds_init();
> +ds_put_format(, "inport == %s && arp.op == 2 && "
> + "arp.spa == %s", out_port->json_key,
> + route->nexthop);
> +ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 90,
> + ds_cstr(),
> + "put_arp(inport, arp.spa, arp.sha);");
> +ds_destroy();
> }
> } else {
> if (out_port->lrp_networks.n_ipv6_addrs) {
> lrp_addr_s = out_port->lrp_networks.ipv6_addrs[0].addr_s;
> +
> +/* Explicitly allow NA for the next-hop. */
> +struct ds match;
> +ds_init();
> +ds_put_format(, "inport == %s && nd_na && "
> + "ip6.src == %s", out_port->json_key,
> + route->nexthop);
> +ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 90,
> + ds_cstr(),
> + "put_nd(inport, nd.target, nd.tll);");
> +ds_destroy();
> }
> }
> }
> @@ -6159,10 +6181,6 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>"ip4.dst == 0.0.0.0/8",
>"drop;");
>
> -/* ARP reply handling. Use ARP replies to populate the logical
> - * router's ARP table. */
> -ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 90, "arp.op == 2",
> - "put_arp(inport, arp.spa, arp.sha);");
>
> /* Drop Ethernet local broadcast. By definition this traffic should
> * not be forwarded.*/
> @@ -6175,16 +6193,7 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
> ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 30,
>ds_cstr(), "drop;");
>
> -/* ND advertisement handling. Use advertisements to populate
> - * the logical router's ARP/ND table. */
> -ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 90, "nd_na",
> - "put_nd(inport, nd.target, nd.tll);");
>
> -/* Lean from neighbor solicitations that were not directed at
> - * us. (A priority-90 flow will respond to requests to us and
> - * learn the sender's mac address. */
> -ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 80, "nd_ns",
> - "put_nd(inport, ip6.src, nd.sll);");
>
> /* Pass other traffic not already handled to the next table for
> * routing. */
> @@ -6320,15 +6329,34 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>ds_cstr(), ds_cstr());
> }
>
> +/* ARP reply handling. Use ARP replies to populate the logical
> + * router's ARP table. */
> +for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> +ds_clear();
> +ds_put_format(, "inport == %s && arp.spa == %s/%u && "
> + "arp.tpa == %s/%u && arp.op == 2",
> + op->json_key,
> +
[ovs-dev] [PATCH ovn v2] OVN: Fix learning of neighbors from ARP/ND packets.
Add a restriction on the target protocol addresses to match the
configured subnets. All other ARP/ND packets are invalid in this
context.
One exception is for ARP replies that are received for route next-hops
that are only reachable via a port but can't be directly resolved
through route lookups. Such support was introduced by commit:
6b785fd8fe29 ("ovn-util: Allow /32 IP addresses for router ports.")
Reported-at: https://bugzilla.redhat.com/1729846
Reported-by: Haidong Li
CC: Han Zhou
CC: Guru Shetty
Fixes: b068454082f5 ("ovn-northd: Support learning neighbor from ARP request.")
Signed-off-by: Dumitru Ceara
---
v2:
- Update commit message.
- Implement the fix also for ARP replies and IPv6 ND.
Signed-off-by: Dumitru Ceara
---
northd/ovn-northd.c | 95 +
1 file changed, 81 insertions(+), 14 deletions(-)
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index bed2993..637e82c 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -5815,10 +5815,32 @@ build_static_route_flow(struct hmap *lflows, struct
ovn_datapath *od,
if (is_ipv4) {
if (out_port->lrp_networks.n_ipv4_addrs) {
lrp_addr_s = out_port->lrp_networks.ipv4_addrs[0].addr_s;
+
+/* Explicitly allow ARP replies for the next-hop. */
+struct ds match;
+ds_init();
+ds_put_format(, "inport == %s && arp.op == 2 && "
+ "arp.spa == %s", out_port->json_key,
+ route->nexthop);
+ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 90,
+ ds_cstr(),
+ "put_arp(inport, arp.spa, arp.sha);");
+ds_destroy();
}
} else {
if (out_port->lrp_networks.n_ipv6_addrs) {
lrp_addr_s = out_port->lrp_networks.ipv6_addrs[0].addr_s;
+
+/* Explicitly allow NA for the next-hop. */
+struct ds match;
+ds_init();
+ds_put_format(, "inport == %s && nd_na && "
+ "ip6.src == %s", out_port->json_key,
+ route->nexthop);
+ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 90,
+ ds_cstr(),
+ "put_nd(inport, nd.target, nd.tll);");
+ds_destroy();
}
}
}
@@ -6159,10 +6181,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
"ip4.dst == 0.0.0.0/8",
"drop;");
-/* ARP reply handling. Use ARP replies to populate the logical
- * router's ARP table. */
-ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 90, "arp.op == 2",
- "put_arp(inport, arp.spa, arp.sha);");
/* Drop Ethernet local broadcast. By definition this traffic should
* not be forwarded.*/
@@ -6175,16 +6193,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 30,
ds_cstr(), "drop;");
-/* ND advertisement handling. Use advertisements to populate
- * the logical router's ARP/ND table. */
-ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 90, "nd_na",
- "put_nd(inport, nd.target, nd.tll);");
-/* Lean from neighbor solicitations that were not directed at
- * us. (A priority-90 flow will respond to requests to us and
- * learn the sender's mac address. */
-ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 80, "nd_ns",
- "put_nd(inport, ip6.src, nd.sll);");
/* Pass other traffic not already handled to the next table for
* routing. */
@@ -6320,15 +6329,34 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
ds_cstr(), ds_cstr());
}
+/* ARP reply handling. Use ARP replies to populate the logical
+ * router's ARP table. */
+for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
+ds_clear();
+ds_put_format(, "inport == %s && arp.spa == %s/%u && "
+ "arp.tpa == %s/%u && arp.op == 2",
+ op->json_key,
+ op->lrp_networks.ipv4_addrs[i].network_s,
+ op->lrp_networks.ipv4_addrs[i].plen,
+ op->lrp_networks.ipv4_addrs[i].network_s,
+ op->lrp_networks.ipv4_addrs[i].plen);
+ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 90,
+ ds_cstr(),
+ "put_arp(inport, arp.spa, arp.sha);");
+}
+
Re: [ovs-dev] [[ovn]] OVN split: Fix modules_install
On Mon, Jul 29, 2019 at 2:30 PM wrote: > From: Lucas Alvares Gomes > > The Makefile for modules_install should point to the ovs subrepository > to be able to compile the ovs kernel modules (otherwise the compilation > fails). > > Signed-off-by: Lucas Alvares Gomes > I think it is better to delete this target rather than fixing it. Ideally OVS repo should be used to build the kernel module . Thanks Numan --- > Makefile.am | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Makefile.am b/Makefile.am > index e3dea1912..1a583cdf2 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -486,7 +486,7 @@ uninstall-local: $(UNINSTALL_LOCAL) > > modules_install: > if LINUX_ENABLED > - cd datapath/linux && $(MAKE) modules_install > + cd ovs/datapath/linux && $(MAKE) modules_install > endif > > dist-docs: > -- > 2.22.0 > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 00/12] Support zone-based conntrack timeout policy
On 26.07.2019 2:24, Yi-Hung Wei wrote: > This patch series enables zone-based conntrack timeout policy support in OVS. > Timeout policy is a set of timeout attributes that can be associated with a > connection when it is committed. Then, the connection tracking system will > expire a connection based on its connection state. For example, one use > case would be to extend the timeout of TCP connection in the established > state to avoid re-connect overhead. Or use case is to shorten the connection > timeout so that the system can reclaim resources faster. > The idea of zone-based conntrack timeout policy is to group connections > with similar characteristics in a conntrack zone, and assign timeout policy > to the conntrack zone. Therefore, all the connections in that zone will share > the same timeout policy. > > For zone-based timeout policy configuration, the association of conntrack > zone and conntrack timeout policy is defined per datapath in vswitch ovsdb > schema. User can program the database through ovs-vsctl or using ovsdb > protocol directly. Once the zone-based timeout policy configuration is > in the database, vswitchd will read those configuration and orgaznie it > in internal datapath strcture, and push the timeout policy into datapath. > Currenlty, only the kernel datapath supports customized timeout policy. Hi everyone, My 2 cents for the feature design: >From the user's perspective: * 'add-dp'/'del-dp' commands looks very strange. "I didn't add datapath into ovsdb, why it exists and switches packets?" "I deleted the datapath from the OVS, why it still exists and switches packets?" If you're implementing the configuration like this, 'datapath' should own the bridges and interfaces, i.e. datapath should be created manually on 'add-dp' and automatically on adding the first bridge on that datapath. All the bridges and interfaces must be deleted/destroyed on 'del-dp'. Or you need to rename your tables and commands to not look like this. >From the developer's perspective: * Right now 'ofproto-dpif' is the only module that manages datapath interfaces and it knows that (there are specific comments in the code). 'dpif's has no reference counts and it's slightly unsafe to manage them outside of 'ofproto-dpif'. You're adding the side module that allowed to open dpif (and it's not able to delete it, that is the possible cause if issues) and use it without noticing any other modules. This breaks the hierarchical structure of OVS. * Right now most of the datapath configuration is done via 'other_config' and corresponding dpif_set_config() callback. Since you're introducing datapath-config module, it should take care of all of this staff. And this will require significant rework of the current datapath configuration scheme. * 'reconfigure_datapath' is an ambiguous name. Solution for above issues might be not introducing the new modules at all. Everything could be handled like we're handling meters, but with OVSDB as the configuration source. On configuration change bridge layer will call ofproto layer that will pass configuration to ofproto-dpif and, finally, dpif layer. Inside 'struct dpif' in dpif.c module you could track all the configuration and pass all the required changes to the dpif-provider via callbacks. This way everything will work fine without breaking current OVS hierarchy. * DB scheme looks just overcomplicated. 3 additional tables which references others just to store a string to integer map. I think that it might be much easier to create a single 'CT_Zones' table with all the required columns: 'id', 'tcp_syn_sent', 'tcp_syn_recv', ..., 'icmp_reply'. This is not a big deal since you need to describe every single field in vswitch.xml anyway. This will also allow you to check support of particular field on the stage of adding value to the database. If you really need to distinguish zones by the datapath type (which is not obvious), you may add 'datapath_type' column, just like we have in a 'Bridge' table. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [[ovn]] OVN split: Fix modules_install
From: Lucas Alvares Gomes The Makefile for modules_install should point to the ovs subrepository to be able to compile the ovs kernel modules (otherwise the compilation fails). Signed-off-by: Lucas Alvares Gomes --- Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile.am b/Makefile.am index e3dea1912..1a583cdf2 100644 --- a/Makefile.am +++ b/Makefile.am @@ -486,7 +486,7 @@ uninstall-local: $(UNINSTALL_LOCAL) modules_install: if LINUX_ENABLED - cd datapath/linux && $(MAKE) modules_install + cd ovs/datapath/linux && $(MAKE) modules_install endif dist-docs: -- 2.22.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovsdb-idl: Mark row "parsed" in ovsdb_idl_txn_write__
Hi Ben, If we keep this fix as is can we please have it backported all the way to and including branch-2.10? Right now it's there in master and branch 2.12. ovn-nbctl daemon mode is available since branch-2.10 and might be affected by this leak as it creates new entries in the database. Thanks, Dumitru On Mon, Jul 22, 2019 at 9:58 AM Damijan Skvarc wrote: > > Hi Dumitru, > > The problem is that ovsdb_idl entity is part of library and can be used by > different application, where each application > instantiates its own parse/unparse callback functions. Library itself does > not know how these parse/unparse functions are > implemented thus it is not reliable to call unparse() function without > calling apparent parse() function first. This case > happens in case the application modifies one column, while common parse flag > insinuates unparse() > function of some another column is called. The most problematic case are > parse/unparse pairs where parse() > function allocates memory and its unparse() counterpart deallocates it. > Common parse flag would in this case cause some > memory would be freed despite it has not been allocated or even worse, some > memory could be deallocated multiple times. > I strongly believe this would lead soon or later to the application crash. > > However, since I am just a self-invited visitor on this project (just to > gain/discover some practices on github platform) you should > not rely on my opinion. Ben looks to be a moderator/architect of this project > and he should advice how to precede with this issue. > > regards, > Damijan > > > On Fri, Jul 19, 2019 at 10:12 AM Dumitru Ceara wrote: >> >> Hi Damijan, Ben, >> >> Damijan, sorry, I didn't realize you had already reported and fixed >> the problem in your pending pull request. I wouldn't have sent my >> patch otherwise. >> >> Regarding a single parsed field instead of parsed bits per column I >> had the impression that it's ok if unparsing is done for all columns. >> Right now, whenever we set a column we call ovsdb_idl_txn_write__() >> which unconditionally executes "(column->unparse)(row)" even if there >> was no old value set before parsing the new datum. As we zero out rows >> when we allocate them I assumed that this unparsing is safe. >> >> What do you guys think? >> >> Thanks, >> Dumitru >> >> On Fri, Jul 19, 2019 at 4:50 AM Damijan Skvarc >> wrote: >> > >> > https://mail.openvswitch.org/pipermail/ovs-dev/2019-July/360285.htmlhttps://mail.openvswitch.org/pipermail/ovs-dev/2019-July/360285.html >> > >> > On Thu, 18 Jul 2019, 20:12 Ben Pfaff, wrote: >> >> >> >> I guess I missed it. Will you please point it out to me, for example in >> >> the list archive? >> >> >> >> On Thu, Jul 18, 2019 at 06:58:34PM +0200, Damijan Skvarc wrote: >> >> > Hmm, the problem of "parsed" flag is that it identifies "all" columns of >> >> > certain row have been parsed, however there are CLI tools which modify >> >> > only >> >> > individual colums by calling ovsdb_idl_txn_write_() function. In this >> >> > case >> >> > and in case parsed flag would be set in ovsdb_idl_txn_write() function >> >> > then >> >> > unparsing procedure would be called also for columns which were not >> >> > parsed. >> >> > The problem could be overcome by having individual flag for each column. >> >> > This has been addresed in pending pull request. Apparent mail has been >> >> > sent >> >> > to dev list, but obviosly has been somehow overlooked. >> >> > br, damijan >> >> > >> >> > On Thu, 18 Jul 2019, 17:52 Ben Pfaff, wrote: >> >> > >> >> > > On Wed, Jul 17, 2019 at 09:05:04PM +0200, Dumitru Ceara wrote: >> >> > > > Once a column is set in a row using ovsdb_idl_txn_write__ we also >> >> > > > mark >> >> > > > the row "parsed". Otherwise the memory allocated by >> >> > > > sbrec__parse_() will never be freed. After marking the >> >> > > > row >> >> > > > "parsed", the ovsdb_idl_txn_disassemble function will properly free >> >> > > > the >> >> > > > newly allocated memory for the column (ovsdb_idl_row_unparse). >> >> > > >> >> > > Wow, good catch. I bet that's been there forever. >> >> > > >> >> > > The OVSDB IDL code is too complicated. It's too hard to spot the >> >> > > issues. I wish I saw a way to make it simpler. >> >> > > ___ >> >> > > 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 05/12] ct-dpif: Add conntrack timeout policy support in dpif layer
On Fri, Jul 26, 2019 at 11:41 AM Darrell Ball wrote:
> Thanks for the patch
>
> I found this patch hard to review since it does not contain implementations
> The same comment applies to Patch 6
> I think Patches 5-7 can be combined into one patch, which will make review
> easier.
>
> some other comments inline
>
> On Thu, Jul 25, 2019 at 4:27 PM Yi-Hung Wei wrote:
>
>> This patch defines the dpif interface for a datapath to support
>> adding, deleting, getting and dumping conntrack timeout policy.
>> The timeout policy is identified by a 4 bytes unsigned integer in
>> datapath, and it currently support timeout for TCP, UDP, and ICMP
>> protocols.
>>
>> Signed-off-by: Yi-Hung Wei
>> ---
>> lib/ct-dpif.c | 51
>> +++
>> lib/ct-dpif.h | 53
>> +
>> lib/dpif-netdev.c | 6 ++
>> lib/dpif-netlink.c | 6 ++
>> lib/dpif-provider.h | 43 +++
>> 5 files changed, 159 insertions(+)
>>
>> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
>> index 6ea7feb0ee35..ae347a9bb46d 100644
>> --- a/lib/ct-dpif.c
>> +++ b/lib/ct-dpif.c
>> @@ -760,3 +760,54 @@ ct_dpif_format_zone_limits(uint32_t default_limit,
>> ds_put_format(ds, ",count=%"PRIu32, zone_limit->count);
>> }
>> }
>> +
>> +int
>> +ct_dpif_add_timeout_policy(struct dpif *dpif, bool is_default,
>> + const struct ct_dpif_timeout_policy *tp)
>> +{
>> +return (dpif->dpif_class->ct_add_timeout_policy
>> +? dpif->dpif_class->ct_add_timeout_policy(dpif, is_default,
>> tp)
>> +: EOPNOTSUPP);
>> +}
>> +
>> +int
>> +ct_dpif_del_timeout_policy(struct dpif *dpif, uint32_t tp_id)
>> +{
>> +return (dpif->dpif_class->ct_del_timeout_policy
>> +? dpif->dpif_class->ct_del_timeout_policy(dpif, tp_id)
>> +: EOPNOTSUPP);
>> +}
>> +
>> +int
>> +ct_dpif_get_timeout_policy(struct dpif *dpif, bool is_default, uint32_t
>> tp_id,
>> + struct ct_dpif_timeout_policy *tp)
>> +{
>> +return (dpif->dpif_class->ct_get_timeout_policy
>> +? dpif->dpif_class->ct_get_timeout_policy(
>> +dpif, is_default, tp_id, tp) : EOPNOTSUPP);
>> +}
>> +
>> +int
>> +ct_dpif_timeout_policy_dump_start(struct dpif *dpif, void **statep)
>> +{
>> +return (dpif->dpif_class->ct_timeout_policy_dump_start
>> +? dpif->dpif_class->ct_timeout_policy_dump_start(dpif,
>> statep)
>> +: EOPNOTSUPP);
>> +}
>> +
>> +int
>> +ct_dpif_timeout_policy_dump_next(struct dpif *dpif, void *state,
>> + struct ct_dpif_timeout_policy **tp)
>> +{
>> +return (dpif->dpif_class->ct_timeout_policy_dump_next
>> +? dpif->dpif_class->ct_timeout_policy_dump_next(dpif, state,
>> tp)
>> +: EOPNOTSUPP);
>> +}
>> +
>> +int
>> +ct_dpif_timeout_policy_dump_done(struct dpif *dpif, void *state)
>> +{
>> +return (dpif->dpif_class->ct_timeout_policy_dump_done
>> +? dpif->dpif_class->ct_timeout_policy_dump_done(dpif, state)
>> +: EOPNOTSUPP);
>> +}
>> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
>> index 2f4906817946..9dc33bede527 100644
>> --- a/lib/ct-dpif.h
>> +++ b/lib/ct-dpif.h
>> @@ -225,6 +225,49 @@ struct ct_dpif_zone_limit {
>> struct ovs_list node;
>> };
>>
>> +#define CT_DPIF_TP_TCP_ATTRS \
>> +CT_DPIF_TP_TCP_ATTR(SYN_SENT) \
>> +CT_DPIF_TP_TCP_ATTR(SYN_RECV) \
>> +CT_DPIF_TP_TCP_ATTR(ESTABLISHED) \
>> +CT_DPIF_TP_TCP_ATTR(FIN_WAIT) \
>> +CT_DPIF_TP_TCP_ATTR(CLOSE_WAIT) \
>> +CT_DPIF_TP_TCP_ATTR(LAST_ACK) \
>> +CT_DPIF_TP_TCP_ATTR(TIME_WAIT) \
>> +CT_DPIF_TP_TCP_ATTR(CLOSE) \
>> +CT_DPIF_TP_TCP_ATTR(SYN_SENT2) \
>> +CT_DPIF_TP_TCP_ATTR(RETRANSMIT) \
>> +CT_DPIF_TP_TCP_ATTR(UNACK)
>> +
>> +#define CT_DPIF_TP_UDP_ATTRS \
>> +CT_DPIF_TP_UDP_ATTR(FIRST) \
>> +CT_DPIF_TP_UDP_ATTR(SINGLE) \
>> +CT_DPIF_TP_UDP_ATTR(MULTIPLE)
>> +
>> +#define CT_DPIF_TP_ICMP_ATTRS \
>> +CT_DPIF_TP_ICMP_ATTR(FIRST) \
>> +CT_DPIF_TP_ICMP_ATTR(REPLY)
>> +
>> +enum OVS_PACKED_ENUM ct_dpif_tp_attr {
>> +#define CT_DPIF_TP_TCP_ATTR(ATTR) CT_DPIF_TP_ATTR_TCP_##ATTR,
>> +CT_DPIF_TP_TCP_ATTRS
>> +#undef CT_DPIF_TP_TCP_ATTR
>> +#define CT_DPIF_TP_UDP_ATTR(ATTR) CT_DPIF_TP_ATTR_UDP_##ATTR,
>> +CT_DPIF_TP_UDP_ATTRS
>> +#undef CT_DPIF_TP_UDP_ATTR
>> +#define CT_DPIF_TP_ICMP_ATTR(ATTR) CT_DPIF_TP_ATTR_ICMP_##ATTR,
>> +CT_DPIF_TP_ICMP_ATTRS
>> +#undef CT_DPIF_TP_ICMP_ATTR
>> +CT_DPIF_TP_ATTR_MAX
>> +};
>> +
>> +struct ct_dpif_timeout_policy {
>> +uint32_tid; /* id that uniquely identify a timeout
>> policy. */
>> +uint32_tpresent;/* If a timeout attribute is present set the
>> + * corresponding bit. */
>
> +uint32_tattrs[CT_DPIF_TP_ATTR_MAX]; /* An array that specifies
>> +
