Re: [ovs-dev] [PATCH] INSTALL.DPDK: remove experimental statement
> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Thomas F > > On 6/25/15 12:39 PM, Flavio Leitner wrote: > > On Tue, Jun 23, 2015 at 06:48:20PM -0700, Pravin Shelar wrote: > >> On Tue, Jun 23, 2015 at 11:42 AM, Ben Pfaff wrote: > >>> Do you two have an opinion on this? If DPDK support is pretty solid > >>> now then it makes sense to apply this to master and backport it to > >>> branch-2.4. > >> > >> Personally I would like to have better testing with vhost, tunneling > >> and PMD thread management before removing experimental status. > Once > >> that is done I will be more comfortable with it. > > > > What kind of test are you looking for? Indeed, vhost and tunneling > > are fairly new, yes, but ethernet ports are being tested for the whole > > 2.3 cycle. > > > > My concern is that a new DPDK related feature would push that again. > > Perhaps come up with a table stating features X status? > Flavio, This sounds like a practical approach toward additional testing that > will > help move this forward. +1 Maybe this should be a general approach for all features that are added to both the kernel and the userspace datapath. +1 for Flavio's approach. > > > > DPDK portsStable > > DPDK vhost-cuse Experimental/Obsolete > > DPDK vhost-user Experimental > > DPDK ivshmem ... > > DPDK PMD management ... > > > > fbl > > > > > > ___ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev > > > > > -- > Thomas F. Herbert > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] dpif-netdev: Check for PKT_RX_RSS_HASH flag.
> -Original Message- > From: Daniele Di Proietto [mailto:diproiet...@vmware.com] > Sent: Wednesday, June 24, 2015 5:00 PM > To: Traynor, Kevin > Cc: dev@openvswitch.org; Flavio Leitner; Panu Matilainen; Jesse Gross; Pravin > Shelar > Subject: Re: [ovs-dev] [PATCH] dpif-netdev: Check for PKT_RX_RSS_HASH flag. > > > > On 24/06/2015 09:47, "Traynor, Kevin" wrote: ..[snip].. > > > >I don't expect there will be a DPDK 2.0.1 release either. I'm optimistic > >we > > > >can get a standalone patch to fix the issue in DPDK 2.1 which we will have > > > >at the end of July. We could then roll DPDK 2.1 support into OVS master > >(and > > > >presumably OVS 2.5). > > > > > > > >The issue is fixed as part of the unified packet api changes but that > >won't > > > >be available (by default) until DPDK 2.2, so obviously we would prefer > >not to > > > >have to wait until then. > > > > I sent a patch to the list that implements the workaround. Thanks Daniele. FYI - the unified/non-unified packet type patches to fix this are on the dpdk-dev http://dpdk.org/ml/archives/dev/2015-June/020247.html http://dpdk.org/ml/archives/dev/2015-June/020112.html ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] Do not flush tx queue which is shared among CPUs since it is always flushed
> From: Pravin Shelar [mailto:pshe...@nicira.com]
>
> On Wed, Jun 17, 2015 at 3:17 AM, Gray, Mark D
> wrote:
> >
> >
> >> -Original Message-
> >> From: Daniele Di Proietto [mailto:diproiet...@vmware.com]
> >> Sent: Tuesday, June 16, 2015 6:45 PM
> >> To: Pravin Shelar; Wei li; Gray, Mark D
> >> Cc: d...@openvswitch.com
> >> Subject: Re: [ovs-dev] [PATCH v2] Do not flush tx queue which is
> >> shared among CPUs since it is always flushed
> >>
> >>
> >>
> >> On 16/06/2015 07:40, "Pravin Shelar" wrote:
> >>
> >> >On Mon, Jun 8, 2015 at 7:42 PM, Pravin Shelar
> wrote:
> >> >> On Mon, Jun 8, 2015 at 6:13 PM, Wei li wrote:
> >> >>> When tx queue is shared among CPUS,the pkts always be flush in
> >> >>>'netdev_dpdk_eth_send'
> >> >>> So it is unnecessarily for flushing in netdev_dpdk_rxq_recv
> >> >>>Otherwise tx will be accessed without locking
> >> >>>
> >> >>> Signed-off-by: Wei li
> >> >>> ---
> >> >>> v1->v2: fix typo
> >> >>>
> >> >>> lib/netdev-dpdk.c | 7 +--
> >> >>> 1 file changed, 5 insertions(+), 2 deletions(-)
> >> >>>
> >> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> >> >>> 63243d8..1e0a483 100644
> >> >>> --- a/lib/netdev-dpdk.c
> >> >>> +++ b/lib/netdev-dpdk.c
> >> >>> @@ -892,8 +892,11 @@ netdev_dpdk_rxq_recv(struct netdev_rxq
> >> *rxq_,
> >> >>>struct dp_packet **packets,
> >> >>> int nb_rx;
> >> >>>
> >> >>> /* There is only one tx queue for this core. Do not flush other
> >> >>> - * queueus. */
> >> >>> -if (rxq_->queue_id == rte_lcore_id()) {
> >> >>> + * queues.
> >> >>> + * Do not flush tx queue which is shared among CPUs
> >> >>> + * since it is always flushed */
> >> >>> +if (rxq_->queue_id == rte_lcore_id() &&
> >> >>> + OVS_LIKELY(!dev->txq_needs_locking)) {
> >> >>> dpdk_queue_flush(dev, rxq_->queue_id);
> >> >>> }
> >> >>
> >> >> Patch looks good, But Daniele has plan to get rid of queue
> >> >> flushing logic. do you see problem with doing this?
> >> >
> >> >Daniele,
> >> >I am not sure if we should fix this queue flushing logic if we are
> >> >going to remove it. So I would like to discuss it first.
> >> >You mentioned that the removing flushing logic results in
> >> >performance drop. Can you explain it? How much is performance drop
> >> >and which is the case?
> >>
> >> Hi Pravin,
> >>
> >> sorry for the late reply. I suspected that removing the queues in
> >> netdev- dpdk was going to have a performance impact in the following
> >> scenario: a batch of packets from different megaflows with the same
> >> action (output on port 1).
> >> Here's what happens:
> >>
> >> 1) With the queues in netdev-dpdk. dpif-netdev calls netdev_send()
> >> for each
> >>packet. netdev_dpdk_send() just copies the pointer in the queue.
> Before
> >>receiving the next batch dpdk_queue_flush() call rte_eth_tx_burst()
> with
> >>a whole batch of packets
> >> 2) Without the queues in netdev-dpdk. dpif-netdev calls netdev_send()
> >> for each
> >>packet. netdev_dpdk_send() calls rte_eth_tx_burst() for each packet.
> >>
> >> Scenario 2 could be slower because rte_eth_tx_burst() is called for
> >> each packet (instead of each batch). After some testing this turns
> >> out not to be the case.
> >> It appears that the bottleneck is not rte_eth_tx_burst(), and copying
> >> the pointers in the temporary queue makes the code slower.
> >>
> >> What do you think? Should we just remove the queues?
> >> CC'ing Mark since he expressed interest in the issue
> >
> > Every call to rte_eth_tx_burst() will generate an expensive MMIO
> > write. Best practice would be to burst as many packets as you can to
> > amortize the cost of the MMIO write. I am surprised at your findings
> > for (2) above. Maybe in this case the cost of queuing is greater than the
> MMIO write?
> >
>
> Hi Mark,
> Can you explain your test case where you do see performance flow down if
> there is no queuing in netdev-dpdk?
Hi Pravin,
I have seen this before from work we did in OVDK. However to help justify this
assertion here, I have tried to reproduce with DPDK and OVS.
I was able to reproduce using the DPDK l2fwding app using the following patch. I
also had to disable the vector pmd to allow me to receive mbufs 1 at a time
(CONFIG_RTE_IXGBE_INC_VECTOR=n in the DPDK .config file)
--- a/examples/l2fwd/main.c
+++ b/examples/l2fwd/main.c
@@ -222,11 +222,17 @@ l2fwd_send_packet(struct rte_mbuf *m, uint8_t port)
qconf->tx_mbufs[port].m_table[len] = m;
len++;
+#define ENABLE_QUEUES
+#ifdef ENABLE_QUEUES
/* enough pkts to be sent */
if (unlikely(len == MAX_PKT_BURST)) {
l2fwd_send_burst(qconf, MAX_PKT_BURST, port);
len = 0;
}
+#else
+l2fwd_send_burst(qconf, 1, port);
+len = 0;
+#endif
qconf->tx_mbufs[port].len = len;
return 0;
@@ -294,6 +300,7 @@ l2fwd_main_loop(void)
diff_tsc = cur_tsc - prev_tsc;
if (un
[ovs-dev] [ovn-controller-gw 0/4] Add ovn controller for VTEP enabled switch.
This series adds a ovn controller, ovn-controller-gw, for VTEP enabled switch. The high level architecture is similar to the ovn-controller. Of course the module implementation is vtep specific. To be able to test the ovn-controller-gw in autotest, this series include the implementation of ovn-sbctl which allows fine query and configuration of ovn-sb db. And the unit tests also illustrates how to ovn-controller-gw can be used. There is still few limitations listed in the last patch. Will work on address them. Then, I'd be really longing for a real vtep switch so that I can play my controller with it~ Go OVN! ~ Alex Wang (4): ovn: Add ovn/lib/libovn.sym to .gitignore. ofproto-macros.at: Allow user to specify file to check in check_logs(). ovn-sbctl: Add ovn-sbctl. ovn: Add controller for VTEP switch. manpages.mk | 12 + ovn/.gitignore|2 + ovn/automake.mk | 10 + ovn/controller-gw/.gitignore |1 + ovn/controller-gw/automake.mk | 11 + ovn/controller-gw/binding.c | 151 ++ ovn/controller-gw/binding.h | 25 + ovn/controller-gw/gateway.c | 388 +++ ovn/controller-gw/gateway.h | 24 + ovn/controller-gw/ovn-controller-gw.c | 272 +++ ovn/controller-gw/ovn-controller-gw.h | 45 ++ ovn/controller-gw/pipeline.c | 525 ovn/controller-gw/pipeline.h | 25 + ovn/lib/.gitignore|1 + ovn/ovn-sb.xml|3 +- ovn/ovn-sbctl.8.in| 159 +++ ovn/ovn-sbctl.c | 842 + tests/automake.mk |7 +- tests/ofproto-macros.at |9 +- tests/ovn-controller-gw.at| 229 + tests/ovn-sbctl.at| 61 +++ tests/testsuite.at|2 + 22 files changed, 2797 insertions(+), 7 deletions(-) create mode 100644 ovn/controller-gw/.gitignore create mode 100644 ovn/controller-gw/automake.mk create mode 100644 ovn/controller-gw/binding.c create mode 100644 ovn/controller-gw/binding.h create mode 100644 ovn/controller-gw/gateway.c create mode 100644 ovn/controller-gw/gateway.h create mode 100644 ovn/controller-gw/ovn-controller-gw.c create mode 100644 ovn/controller-gw/ovn-controller-gw.h create mode 100644 ovn/controller-gw/pipeline.c create mode 100644 ovn/controller-gw/pipeline.h create mode 100644 ovn/ovn-sbctl.8.in create mode 100644 ovn/ovn-sbctl.c create mode 100644 tests/ovn-controller-gw.at create mode 100644 tests/ovn-sbctl.at -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [ovn-controller-gw 3/4] ovn-sbctl: Add ovn-sbctl.
This commit adds ovn-sbctl to ovn family by using the db-ctl-base library. Signed-off-by: Alex Wang --- manpages.mk| 12 + ovn/.gitignore |2 + ovn/automake.mk|9 + ovn/ovn-sbctl.8.in | 159 ++ ovn/ovn-sbctl.c| 842 tests/automake.mk |5 +- tests/ovn-sbctl.at | 61 tests/testsuite.at |1 + 8 files changed, 1089 insertions(+), 2 deletions(-) create mode 100644 ovn/ovn-sbctl.8.in create mode 100644 ovn/ovn-sbctl.c create mode 100644 tests/ovn-sbctl.at diff --git a/manpages.mk b/manpages.mk index 3cec260..032cb26 100644 --- a/manpages.mk +++ b/manpages.mk @@ -1,5 +1,17 @@ # Generated automatically -- do not modify!-*- buffer-read-only: t -*- +ovn/ovn-sbctl.8: \ + ovn/ovn-sbctl.8.in \ + lib/db-ctl-base.man \ + lib/table.man \ + ovsdb/remote-active.man \ + ovsdb/remote-passive.man +ovn/ovn-sbctl.8.in: +lib/db-ctl-base.man: +lib/table.man: +ovsdb/remote-active.man: +ovsdb/remote-passive.man: + ovsdb/ovsdb-client.1: \ ovsdb/ovsdb-client.1.in \ lib/common-syn.man \ diff --git a/ovn/.gitignore b/ovn/.gitignore index 4c13616..2d4835a 100644 --- a/ovn/.gitignore +++ b/ovn/.gitignore @@ -7,3 +7,5 @@ /ovn-sb.pic /ovn-nbctl /ovn-nbctl.8 +/ovn-sbctl +/ovn-sbctl.8 diff --git a/ovn/automake.mk b/ovn/automake.mk index 459ee36..6d2063f 100644 --- a/ovn/automake.mk +++ b/ovn/automake.mk @@ -79,6 +79,15 @@ bin_PROGRAMS += ovn/ovn-nbctl ovn_ovn_nbctl_SOURCES = ovn/ovn-nbctl.c ovn_ovn_nbctl_LDADD = ovn/lib/libovn.la ovsdb/libovsdb.la lib/libopenvswitch.la +# ovn-sbctl +bin_PROGRAMS += ovn/ovn-sbctl +ovn_ovn_sbctl_SOURCES = ovn/ovn-sbctl.c +ovn_ovn_sbctl_LDADD = ovn/lib/libovn.la ovsdb/libovsdb.la lib/libopenvswitch.la + +MAN_ROOTS += ovn/ovn-sbctl.8.in +man_MANS += ovn/ovn-sbctl.8 +DISTCLEANFILES += ovn/ovn-sbctl.8 + include ovn/controller/automake.mk include ovn/lib/automake.mk include ovn/northd/automake.mk diff --git a/ovn/ovn-sbctl.8.in b/ovn/ovn-sbctl.8.in new file mode 100644 index 000..0cd317e --- /dev/null +++ b/ovn/ovn-sbctl.8.in @@ -0,0 +1,159 @@ +.\" -*- nroff -*- +.de IQ +. br +. ns +. IP "\\$1" +.. +.de ST +. PP +. RS -0.15in +. I "\\$1" +. RE +.. +.TH ovn\-sbctl 8 "@VERSION@" "Open vSwitch" "Open vSwitch Manual" +.\" This program's name: +.ds PN ovn\-sbctl +. +.SH NAME +ovn\-sbctl \- utility for querying and configuring \fBOVN_Southbound\fR database +. +.SH SYNOPSIS +\fBovn\-sbctl\fR [\fIoptions\fR] \fB\-\-\fR [\fIoptions\fR] \fIcommand +\fR[\fIargs\fR] [\fB\-\-\fR [\fIoptions\fR] \fIcommand \fR[\fIargs\fR]]... +. +.SH DESCRIPTION +The \fBovn\-sbctl\fR program configures \fBOVN_Southbound\fR database by +providing a high\-level interface to its configuration database. +See \fBovn\-sb\fR(5) for comprehensive documentation of +the database schema. +.PP +\fBovn\-sbctl\fR connects to an \fBovsdb\-server\fR process that +maintains an OVN_Southbound configuration database. Using this +connection, it queries and possibly applies changes to the database, +depending on the supplied commands. +.PP +\fBovn\-sbctl\fR can perform any number of commands in a single run, +implemented as a single atomic transaction against the database. +.PP +The \fBovn\-sbctl\fR command line begins with global options (see +\fBOPTIONS\fR below for details). The global options are followed by +one or more commands. Each command should begin with \fB\-\-\fR by +itself as a command-line argument, to separate it from the following +commands. (The \fB\-\-\fR before the first command is optional.) The +command +itself starts with command-specific options, if any, followed by the +command name and any arguments. +. +.SH OPTIONS +. +The following options affect the behavior \fBovn\-sbctl\fR as a whole. +Some individual commands also accept their own options, which are +given just before the command name. If the first command on the +command line has options, then those options must be separated from +the global options by \fB\-\-\fR. +. +.IP "\fB\-\-db=\fIserver\fR" +Sets \fIserver\fR as the database server that \fBovn\-sbctl\fR +contacts to query or modify configuration. The default is +\fBunix:@RUNDIR@/db.sock\fR. \fIserver\fR must take one of the +following forms: +.RS +.so ovsdb/remote-active.man +.so ovsdb/remote-passive.man +.RE +. +.IP "\fB\-\-no\-syslog\fR" +By default, \fBovn\-sbctl\fR logs its arguments and the details of any +changes that it makes to the system log. This option disables this +logging. +.IP +This option is equivalent to \fB\-\-verbose=sbctl:syslog:warn\fR. +. +.IP "\fB\-\-oneline\fR" +Modifies the output format so that the output for each command is printed +on a single line. New-line characters that would otherwise separate +lines are printed as \fB\\n\fR, and any instances of \fB\\\fR that +would otherwise appear in the output are doubled. +Prints a blank line for each command that has no output. +This option does not affect th
[ovs-dev] [ovn-controller-gw 2/4] ofproto-macros.at: Allow user to specify file to check in check_logs().
Later commit will call check_logs() and specify ovn-northd.log as check_logs
target.
Signed-off-by: Alex Wang
---
tests/ofproto-macros.at |9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
index 74b02b7..796b828 100644
--- a/tests/ofproto-macros.at
+++ b/tests/ofproto-macros.at
@@ -115,9 +115,12 @@ m4_define([OVS_VSWITCHD_START],
AT_CHECK([ovs-vsctl -- add-br br0 -- set bridge br0 datapath-type=dummy
other-config:datapath-id=fedcba9876543210 other-config:hwaddr=aa:55:aa:55:00:00
protocols=[[OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15]]
fail-mode=secure -- $1 m4_if([$2], [], [], [| ${PERL} $srcdir/uuidfilt.pl])],
[0], [$2])
])
+# check_logs scans through the specified files and report all WARN, ERR, EMER
+# log entries. User must provide the files to scan as $1. User can also
+# add custom sed filters in $2.
m4_divert_push([PREPARE_TESTS])
check_logs () {
-sed -n "$1
+sed -n "$2
/timeval.*Unreasonably long [[0-9]]*ms poll interval/d
/timeval.*faults: [[0-9]]* minor, [[0-9]]* major/d
/timeval.*disk: [[0-9]]* reads, [[0-9]]* writes/d
@@ -125,7 +128,7 @@ check_logs () {
/ovs_rcu.*blocked [[0-9]]* ms waiting for .* to quiesce/d
/|WARN|/p
/|ERR|/p
-/|EMER|/p" ovs-vswitchd.log ovsdb-server.log
+/|EMER|/p" $1
}
m4_divert_pop([PREPARE_TESTS])
@@ -138,7 +141,7 @@ m4_divert_pop([PREPARE_TESTS])
#
# OVS_VSWITCHD_STOP(["/expected error/d"])
m4_define([OVS_VSWITCHD_STOP],
- [AT_CHECK([check_logs $1])
+ [AT_CHECK([check_logs "ovsdb-server.log ovs-vswitchd.log" $1])
AT_CHECK([ovs-appctl -t ovs-vswitchd exit])
AT_CHECK([ovs-appctl -t ovsdb-server exit])])
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [ovn-controller-gw 1/4] ovn: Add ovn/lib/libovn.sym to .gitignore.
Signed-off-by: Alex Wang --- ovn/lib/.gitignore |1 + 1 file changed, 1 insertion(+) diff --git a/ovn/lib/.gitignore b/ovn/lib/.gitignore index 846df01..a80a1bc 100644 --- a/ovn/lib/.gitignore +++ b/ovn/lib/.gitignore @@ -1,3 +1,4 @@ +/libovn.sym /ovn-nb-idl.c /ovn-nb-idl.h /ovn-nb-idl.ovsidl -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofp-actions: Support mixing "conjunction" and "note" actions.
On Thu, Jun 25, 2015 at 04:27:00PM -0700, Andy Zhou wrote:
> On Mon, Jun 15, 2015 at 1:58 PM, Ben Pfaff wrote:
> > It doesn't make sense to mix "conjunction" actions with most other kinds
> > of actions. That's because flows with "conjunction" actions aren't ever
> > actually executed, so any actions mixed up with them would never do
> > anything useful. "note" actions are a little different because they never
> > do anything useful anyway: they are just there to allow a controller to
> > annotate flows. It makes as much sense to annotate a flow with
> > "conjunction" actions as it does to annotate any other flow, so this
> > commit makes this possible.
> >
> > Requested-by: Soner Sevinc
> > Signed-off-by: Ben Pfaff
> > -
> > static void
> > get_conjunctions(const struct ofputil_flow_mod *fm,
> > struct cls_conjunction **conjsp, size_t *n_conjsp)
> > @@ -4420,23 +4414,31 @@ get_conjunctions(const struct ofputil_flow_mod *fm,
> > struct cls_conjunction *conjs = NULL;
> > int n_conjs = 0;
> >
> > -if (is_conjunction(fm->ofpacts, fm->ofpacts_len)) {
> > -const struct ofpact *ofpact;
> > -int i;
> > +n_conjs = 0;
> I don't think this line is necessary. Otherwise, looks good.
Good point, thanks, removed.
> Acked-by: Andy Zhou
Thanks! Applied to master and branch-2.4.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [ovn-controller-gw RFC 4/4] ovn: Add controller for VTEP switch.
This commit adds a controller 'ovn-controller-gw' for controlling vtep enabled physical switches. Following are the short summary of each module: gateway.c = register the physical switches in vtep and constantly update the gateway_ports entries in Chassis table and the vlan_map in Gateway table. the vlan_map value is the combination of "physical_switch_name+physical_port _name+vlan_number." binding.c = scan through the Binding table, if there is a binding for the logical port in vlan_map, set the binding's chassis to the vtep switch. pipeline.c == scan through the Binding table, and create the Ucast_Macs_Remote in VTEP database for each logical port MAC in the same logical datapath (logical switch for vtep). also create the physical locators to each HV chassis in the same logical network. unit tests == test the ovn-controller-gw using vtep device simulated via ovs-vtep. please check the tests/ovn-controller-gw.at for details. Limitations: - How to make controller-gw connect to OVN_SB? my understanding is that vtep should not know the location of ovnsb. but there is no "external_ids" column in vtep schema. and the manager for vtep should be the controller-gw. so, the implementation requires user to specify both vtep, and ovnsb location from command line. - Assume each Chassis has only one Encap entry. Need to extend it to find the vxlan encap entry. - Require logical switch name in VTEP be the same as logical datapath uuid in ovnsb db. This provide the good consistency when creating the Ucast_Macs_Remote and Physical_Locators in vtep, since the logical switch name can be used directly to find the binding in ovn-sb. Signed-off-by: Alex Wang --- ovn/automake.mk |1 + ovn/controller-gw/.gitignore |1 + ovn/controller-gw/automake.mk | 11 + ovn/controller-gw/binding.c | 151 ++ ovn/controller-gw/binding.h | 25 ++ ovn/controller-gw/gateway.c | 388 ovn/controller-gw/gateway.h | 24 ++ ovn/controller-gw/ovn-controller-gw.c | 272 + ovn/controller-gw/ovn-controller-gw.h | 45 +++ ovn/controller-gw/pipeline.c | 525 + ovn/controller-gw/pipeline.h | 25 ++ ovn/ovn-sb.xml|3 +- tests/automake.mk |6 +- tests/ovn-controller-gw.at| 229 ++ tests/testsuite.at|1 + 15 files changed, 1703 insertions(+), 4 deletions(-) create mode 100644 ovn/controller-gw/.gitignore create mode 100644 ovn/controller-gw/automake.mk create mode 100644 ovn/controller-gw/binding.c create mode 100644 ovn/controller-gw/binding.h create mode 100644 ovn/controller-gw/gateway.c create mode 100644 ovn/controller-gw/gateway.h create mode 100644 ovn/controller-gw/ovn-controller-gw.c create mode 100644 ovn/controller-gw/ovn-controller-gw.h create mode 100644 ovn/controller-gw/pipeline.c create mode 100644 ovn/controller-gw/pipeline.h create mode 100644 tests/ovn-controller-gw.at diff --git a/ovn/automake.mk b/ovn/automake.mk index 6d2063f..475817a 100644 --- a/ovn/automake.mk +++ b/ovn/automake.mk @@ -89,6 +89,7 @@ man_MANS += ovn/ovn-sbctl.8 DISTCLEANFILES += ovn/ovn-sbctl.8 include ovn/controller/automake.mk +include ovn/controller-gw/automake.mk include ovn/lib/automake.mk include ovn/northd/automake.mk include ovn/utilities/automake.mk diff --git a/ovn/controller-gw/.gitignore b/ovn/controller-gw/.gitignore new file mode 100644 index 000..88ebb5c --- /dev/null +++ b/ovn/controller-gw/.gitignore @@ -0,0 +1 @@ +/ovn-controller-gw \ No newline at end of file diff --git a/ovn/controller-gw/automake.mk b/ovn/controller-gw/automake.mk new file mode 100644 index 000..356dfb5 --- /dev/null +++ b/ovn/controller-gw/automake.mk @@ -0,0 +1,11 @@ +bin_PROGRAMS += ovn/controller-gw/ovn-controller-gw +ovn_controller_gw_ovn_controller_gw_SOURCES = \ + ovn/controller-gw/binding.c \ + ovn/controller-gw/binding.h \ + ovn/controller-gw/gateway.c \ + ovn/controller-gw/gateway.h \ + ovn/controller-gw/pipeline.c \ + ovn/controller-gw/pipeline.h \ + ovn/controller-gw/ovn-controller-gw.c \ + ovn/controller-gw/ovn-controller-gw.h +ovn_controller_gw_ovn_controller_gw_LDADD = ovn/lib/libovn.la lib/libopenvswitch.la vtep/libvtep.la diff --git a/ovn/controller-gw/binding.c b/ovn/controller-gw/binding.c new file mode 100644 index 000..3d69635 --- /dev/null +++ b/ovn/controller-gw/binding.c @@ -0,0 +1,151 @@ +/* Copyright (c) 2015 Nicira, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE
Re: [ovs-dev] [PATCH] ofproto: Fix use-after-free in bridge destruction with groups.
On Thu, Jun 25, 2015 at 12:48:27PM +0900, Simon Horman wrote: > Hi Ben, > > On Tue, Jun 23, 2015 at 11:38:56AM -0700, Ben Pfaff wrote: > > Groups were not destroyed until after lots of other important bridge > > data had been destroyed, including the connection manager. There was an > > indirect dependency on the connection manager for bridge destruction > > because destroying a group also destroys all of the flows that reference > > the group, which in turn causes the ofmonitor to be invoked to report that > > the flows had been destroyed. This commit fixes the problem by destroying > > groups earlier. > > > > The problem can be observed by reverting the code changes in this commit > > then running "make check-valgrind" with the test that this commit > > introduces. > > > > Reported-by: Simon Horman > > Signed-off-by: Ben Pfaff > > Thanks for following up on this; your patch looks good to me. > > Reviewed-by: Simon Horman Thanks Simon, I applied this to master and branch-2.4. I don't know whether branch-2.3 has the problem, but the patch doesn't apply cleanly and valgrind doesn't report an error with the test on that branch. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovn-controller-gw 3/4] ovn-sbctl: Add ovn-sbctl.
Hey Ben, Russell, Want to discuss if we may want to refine ovn-nbctl to the same format as ovn-sbctl (using lib/db-ctl-cmds module). The benefit of such refinement is that ovn-nbctl can have the common ovsdb operations (e.g. list, set, create, ... like in ovs-vsctl). Since my testing does not require much from ovn-nb, and the ovn-nbctl is really well-written, I did not conduct the refinement in this series. But if you also think it makes sense to do so, I'd like to make the change~ Thanks, Alex Wang, On Fri, Jun 26, 2015 at 8:45 AM, Alex Wang wrote: > This commit adds ovn-sbctl to ovn family by using the db-ctl-base > library. > > Signed-off-by: Alex Wang > --- > manpages.mk| 12 + > ovn/.gitignore |2 + > ovn/automake.mk|9 + > ovn/ovn-sbctl.8.in | 159 ++ > ovn/ovn-sbctl.c| 842 > > tests/automake.mk |5 +- > tests/ovn-sbctl.at | 61 > tests/testsuite.at |1 + > 8 files changed, 1089 insertions(+), 2 deletions(-) > create mode 100644 ovn/ovn-sbctl.8.in > create mode 100644 ovn/ovn-sbctl.c > create mode 100644 tests/ovn-sbctl.at > > diff --git a/manpages.mk b/manpages.mk > index 3cec260..032cb26 100644 > --- a/manpages.mk > +++ b/manpages.mk > @@ -1,5 +1,17 @@ > # Generated automatically -- do not modify!-*- buffer-read-only: t -*- > > +ovn/ovn-sbctl.8: \ > + ovn/ovn-sbctl.8.in \ > + lib/db-ctl-base.man \ > + lib/table.man \ > + ovsdb/remote-active.man \ > + ovsdb/remote-passive.man > +ovn/ovn-sbctl.8.in: > +lib/db-ctl-base.man: > +lib/table.man: > +ovsdb/remote-active.man: > +ovsdb/remote-passive.man: > + > ovsdb/ovsdb-client.1: \ > ovsdb/ovsdb-client.1.in \ > lib/common-syn.man \ > diff --git a/ovn/.gitignore b/ovn/.gitignore > index 4c13616..2d4835a 100644 > --- a/ovn/.gitignore > +++ b/ovn/.gitignore > @@ -7,3 +7,5 @@ > /ovn-sb.pic > /ovn-nbctl > /ovn-nbctl.8 > +/ovn-sbctl > +/ovn-sbctl.8 > diff --git a/ovn/automake.mk b/ovn/automake.mk > index 459ee36..6d2063f 100644 > --- a/ovn/automake.mk > +++ b/ovn/automake.mk > @@ -79,6 +79,15 @@ bin_PROGRAMS += ovn/ovn-nbctl > ovn_ovn_nbctl_SOURCES = ovn/ovn-nbctl.c > ovn_ovn_nbctl_LDADD = ovn/lib/libovn.la ovsdb/libovsdb.la lib/ > libopenvswitch.la > > +# ovn-sbctl > +bin_PROGRAMS += ovn/ovn-sbctl > +ovn_ovn_sbctl_SOURCES = ovn/ovn-sbctl.c > +ovn_ovn_sbctl_LDADD = ovn/lib/libovn.la ovsdb/libovsdb.la lib/ > libopenvswitch.la > + > +MAN_ROOTS += ovn/ovn-sbctl.8.in > +man_MANS += ovn/ovn-sbctl.8 > +DISTCLEANFILES += ovn/ovn-sbctl.8 > + > include ovn/controller/automake.mk > include ovn/lib/automake.mk > include ovn/northd/automake.mk > diff --git a/ovn/ovn-sbctl.8.in b/ovn/ovn-sbctl.8.in > new file mode 100644 > index 000..0cd317e > --- /dev/null > +++ b/ovn/ovn-sbctl.8.in > @@ -0,0 +1,159 @@ > +.\" -*- nroff -*- > +.de IQ > +. br > +. ns > +. IP "\\$1" > +.. > +.de ST > +. PP > +. RS -0.15in > +. I "\\$1" > +. RE > +.. > +.TH ovn\-sbctl 8 "@VERSION@" "Open vSwitch" "Open vSwitch Manual" > +.\" This program's name: > +.ds PN ovn\-sbctl > +. > +.SH NAME > +ovn\-sbctl \- utility for querying and configuring \fBOVN_Southbound\fR > database > +. > +.SH SYNOPSIS > +\fBovn\-sbctl\fR [\fIoptions\fR] \fB\-\-\fR [\fIoptions\fR] \fIcommand > +\fR[\fIargs\fR] [\fB\-\-\fR [\fIoptions\fR] \fIcommand \fR[\fIargs\fR]]... > +. > +.SH DESCRIPTION > +The \fBovn\-sbctl\fR program configures \fBOVN_Southbound\fR database by > +providing a high\-level interface to its configuration database. > +See \fBovn\-sb\fR(5) for comprehensive documentation of > +the database schema. > +.PP > +\fBovn\-sbctl\fR connects to an \fBovsdb\-server\fR process that > +maintains an OVN_Southbound configuration database. Using this > +connection, it queries and possibly applies changes to the database, > +depending on the supplied commands. > +.PP > +\fBovn\-sbctl\fR can perform any number of commands in a single run, > +implemented as a single atomic transaction against the database. > +.PP > +The \fBovn\-sbctl\fR command line begins with global options (see > +\fBOPTIONS\fR below for details). The global options are followed by > +one or more commands. Each command should begin with \fB\-\-\fR by > +itself as a command-line argument, to separate it from the following > +commands. (The \fB\-\-\fR before the first command is optional.) The > +command > +itself starts with command-specific options, if any, followed by the > +command name and any arguments. > +. > +.SH OPTIONS > +. > +The following options affect the behavior \fBovn\-sbctl\fR as a whole. > +Some individual commands also accept their own options, which are > +given just before the command name. If the first command on the > +command line has options, then those options must be separated from > +the global options by \fB\-\-\fR. > +. > +.IP "\fB\-\-db=\fIserver\fR" > +Sets \fIserver\fR as the database server that \fBovn\-sbctl\fR > +contacts to q
Re: [ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.
Hi Ben,
I just tested out this patch and it seems to work as expected for me. I do have
a few quick questions/thoughts.
- One thing I noticed is if I create a logical_port but don't append the
port.uuid to the Logical_Switch.ports column it seems like the port isn't
showing up in ovsdb. I'm not sure if this is expected or I'm incorrect.
- Another thing I was thinking about is if requiring the caller to query ovsdb
for the logical_switch to obtain the switch_ports and then appending the newly
create logical port could be problematic. For one it seems like we need to lock
ovsdb for concurrent operations here. I was wondering if it would be possible
to keep previous schema where the logical_port stores a reference to it's
logical_switch. Though, when the logical_switch is deleted we would cascade the
deletes of the logical_ports on that switch? I also think it feels more natural
to have a reference from the logical_port to logical_switch because now it
seems like the schema allows one to create a logical_port without a
logical_switch? Perhaps ovsdb can't currently work in this way which you were
explaining in your commit message though I just wanted to make sure.
Thanks,
Aaron
From: Ben Pfaff
Sent: Thursday, June 25, 2015 4:57 PM
To: Aaron Rosen
Cc: dev@openvswitch.org
Subject: Re: [PATCH] ovn: Take advantage of OVSDB garbage collection in
OVN_Northbound schema.
Thanks. Of course this patch needs review on the OVS side as well; that
might take a few days.
On Thu, Jun 25, 2015 at 11:51:57PM +, Aaron Rosen wrote:
> Awesome thanks Ben! I'll update neutron tomorrow morning to work with this.
>
>
> > On Jun 25, 2015, at 5:35 PM, Ben Pfaff wrote:
> >
> > Until now, the OVN_Northbound schema has been designed to sidestep a
> > weakness in the OVSDB protocol when a column has a great deal of data in
> > it. In the current OVSDB protocol, whenever a column changes, the entire
> > new value of the column is sent to all of the clients that are monitoring
> > that column. That means that adding or removing a small amount of data,
> > say 1 element in a set, requires sending all of the data, which is
> > expensive if the column has a lot of data.
> >
> > One example of a column with potential to have a lot of data is the set of
> > ports within a logical switch, if a logical switch has a large number of
> > ports. Thus, the existing OVN_Northbound schema has each Logical_Port
> > point to its containing Logical_Switch instead of the other way around.
> > This sidesteps the problem because it does not use any large columns.
> >
> > The tradeoff that this forces, however, is that the schema cannot take
> > advantage of OVSDB's garbage collection feature, where it automatically
> > deletes rows that are unreferenced. That's a problem for Neutron because
> > of Neutron-internal races between deletion of a Logical_Switch and
> > creation of new Logical_Ports on the switch being deleted. When such a
> > race happens, OVSDB refuses to delete the Logical_Switch because of
> > references to it from the newly created Logical_Port (although Neutron
> > does delete the pre-existing logical ports).
> >
> > To solve the problem, this commit changes the OVN_Northbound schema to
> > use a set of ports within Logical_Switch. That will lead to large columns
> > for large logical switches; I plan to address that (though I don't have
> > code written) by enhancing the OVSDB protocol. With this commit applied,
> > the database will automatically cascade deleting a logical switch row to
> > delete all of its ports, ACLs, and its router port (if any).
> >
> > This commit makes some pretty pervasive changes to ovn-northd, but they
> > are mostly beneficial to the code readability because now it becomes
> > possible to trivially iterate through the ports that belong to a switch,
> > which was difficult before the schema change.
> >
> > This commit will break the Neutron integration until that is changed to
> > handle the new database schema.
> >
> > CC: Aaron Rosen
> > Signed-off-by: Ben Pfaff
> > ---
> > ovn/northd/ovn-northd.c | 318
> > ++--
> > ovn/ovn-nb.ovsschema| 39 +++---
> > ovn/ovn-nb.xml | 74 ++-
> > ovn/ovn-nbctl.c | 116 ++
> > 4 files changed, 282 insertions(+), 265 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index f37df77..c790a90 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -281,139 +281,116 @@ build_pipeline(struct northd_context *ctx)
> > }
> >
> > /* Table 0: Ingress port security. */
> > -const struct nbrec_logical_port *lport;
> > -NBREC_LOGICAL_PORT_FOR_EACH (lport, ctx->ovnnb_idl) {
> > -struct ds match = DS_EMPTY_INITIALIZER;
> > -ds_put_cstr(&match, "inport == ");
> > -json_string_escape(lport->name, &match);
> > -build_por
Re: [ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.
On Fri, Jun 26, 2015 at 05:20:16PM +, Aaron Rosen wrote: > I just tested out this patch and it seems to work as expected for > me. I do have a few quick questions/thoughts. > > - One thing I noticed is if I create a logical_port but don't append > the port.uuid to the Logical_Switch.ports column it seems like the > port isn't showing up in ovsdb. I'm not sure if this is expected or > I'm incorrect. That's a consequence of the garbage collection. A Logical_Port that isn't attached to a Logical_Switch gets destroyed. That's true if it was previously attached to a Logical_Switch that was just deleted, or if it was created within the current transaction and never attached to a Logical_Switch. > - Another thing I was thinking about is if requiring the caller to > query ovsdb for the logical_switch to obtain the switch_ports and then > appending the newly create logical port could be problematic. For one > it seems like we need to lock ovsdb for concurrent operations here. OVSDB doesn't support locking, so the equivalent that allows one to avoid problems with "dirty reads" is to verify that the ports are the expected set (the set before the transaction started) and make the transaction abort if it isn't correct. With the OSVDB Python IDL, that's just a matter of calling Row.verify() on the column in question before modifying the column. (If the transaction aborts, then you just retry it.) This is the purpose of the nbrec_logical_switch_verify_ports() calls in do_lport_add() and remove_lport() in ovn-nbctl.c as modified by this commit, by the way. > I was wondering if it would be possible to keep previous schema where > the logical_port stores a reference to it's logical_switch. Though, > when the logical_switch is deleted we would cascade the deletes of the > logical_ports on that switch? OVSDB only supports cascading with the schema in the form used by this comit. > I also think it feels more natural to have a reference from the > logical_port to logical_switch because now it seems like the schema > allows one to create a logical_port without a logical_switch? Perhaps > ovsdb can't currently work in this way which you were explaining in > your commit message though I just wanted to make sure. With this schema, an unattached logical_port can exist temporarily within a transaction but transaction commit will delete it, so that the unattached logical_port will never be visible to other clients (or in the database log). ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/2 v3][branch 2.4] datapath-windows: Rename 'vport->isPresentOnHv' to 'isAbsentOnHv'
Looking at the code, the flag 'vport->isPresentOnHv' is actually
indicating if the vport is present on the Hyper-V switch or not, but the
logic seems to be inverse. 'isPresentOnHv == TRUE' indicates that the
vport is not present on the Hyper-V switch. Eg. VXLAN port, would have
isPresentOnHv == TRUE.
In this patch, we rename the variable to reflect its meaning.
vport->isAbsentOnHv is TRUE iff:
- vport is bridge internal port
- vport is tunnel port
- vport was added from Hyper-V and also from OVS, but got deleted from
Hyper-V
Signed-off-by: Nithin Raju
---
v2: created a series
v3: addressed the issues Alin found during validation
---
datapath-windows/ovsext/Vport.c | 22 +++---
datapath-windows/ovsext/Vport.h | 2 +-
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c
index c35d459..a60e7e3 100644
--- a/datapath-windows/ovsext/Vport.c
+++ b/datapath-windows/ovsext/Vport.c
@@ -129,7 +129,7 @@ HvCreatePort(POVS_SWITCH_CONTEXT switchContext,
vport = OvsFindVportByHvNameW(gOvsSwitchContext,
portParam->PortFriendlyName.String,
portParam->PortFriendlyName.Length);
-if (vport && vport->isPresentOnHv == FALSE) {
+if (vport && vport->isAbsentOnHv == FALSE) {
OVS_LOG_ERROR("Port add failed since a port already exists on "
"the specified port Id: %u, ovsName: %s",
portParam->PortId, vport->ovsName);
@@ -138,7 +138,7 @@ HvCreatePort(POVS_SWITCH_CONTEXT switchContext,
}
if (vport != NULL) {
-ASSERT(vport->isPresentOnHv);
+ASSERT(vport->isAbsentOnHv);
ASSERT(vport->portNo != OVS_DPPORT_NUMBER_INVALID);
/*
@@ -153,7 +153,7 @@ HvCreatePort(POVS_SWITCH_CONTEXT switchContext,
status = STATUS_DATA_NOT_ACCEPTED;
goto create_port_done;
}
-vport->isPresentOnHv = FALSE;
+vport->isAbsentOnHv = FALSE;
} else {
vport = (POVS_VPORT_ENTRY)OvsAllocateVport();
if (vport == NULL) {
@@ -759,7 +759,7 @@ OvsAllocateVport(VOID)
}
RtlZeroMemory(vport, sizeof (OVS_VPORT_ENTRY));
vport->ovsState = OVS_STATE_UNKNOWN;
-vport->isPresentOnHv = FALSE;
+vport->isAbsentOnHv = FALSE;
vport->portNo = OVS_DPPORT_NUMBER_INVALID;
InitializeListHead(&vport->ovsNameLink);
@@ -1152,7 +1152,7 @@ OvsRemoveAndDeleteVport(PVOID usrParamsContext,
switch (vport->ovsType) {
case OVS_VPORT_TYPE_INTERNAL:
if (!vport->isBridgeInternal) {
-if (hvDelete && vport->isPresentOnHv == FALSE) {
+if (hvDelete && vport->isAbsentOnHv == FALSE) {
switchContext->internalPortId = 0;
switchContext->internalVport = NULL;
OvsInternalAdapterDown();
@@ -1200,7 +1200,7 @@ OvsRemoveAndDeleteVport(PVOID usrParamsContext,
*
* Both 'hvDelete' and 'ovsDelete' can be set to TRUE by the caller.
*/
-if (vport->isPresentOnHv == TRUE) {
+if (vport->isAbsentOnHv == TRUE) {
deletedOnHv = TRUE;
}
if (vport->portNo == OVS_DPPORT_NUMBER_INVALID) {
@@ -1208,7 +1208,7 @@ OvsRemoveAndDeleteVport(PVOID usrParamsContext,
}
if (hvDelete && !deletedOnHv) {
-vport->isPresentOnHv = TRUE;
+vport->isAbsentOnHv = TRUE;
if (vport->isExternal) {
ASSERT(vport->nicIndex != 0);
@@ -1447,7 +1447,7 @@ OvsClearAllSwitchVports(POVS_SWITCH_CONTEXT switchContext)
vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, portNoLink);
ASSERT(OvsIsTunnelVportType(vport->ovsType) ||
(vport->ovsType == OVS_VPORT_TYPE_INTERNAL &&
-vport->isBridgeInternal) || vport->isPresentOnHv == TRUE);
+vport->isBridgeInternal) || vport->isAbsentOnHv == TRUE);
OvsRemoveAndDeleteVport(NULL, switchContext, vport, TRUE, TRUE);
}
}
@@ -2198,7 +2198,7 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT
usrParamsCtx,
* Allow the vport to be deleted, because there is no
* corresponding hyper-v switch part.
*/
-vport->isPresentOnHv = TRUE;
+vport->isAbsentOnHv = TRUE;
} else {
goto Cleanup;
}
@@ -2523,7 +2523,7 @@ OvsTunnelVportPendingRemove(PVOID context,
}
}
-ASSERT(vport->isPresentOnHv == TRUE);
+ASSERT(vport->isAbsentOnHv == TRUE);
ASSERT(vport->portNo != OVS_DPPORT_NUMBER_INVALID);
/* Remove the port from the relevant lists. */
@@ -2600,7 +2600,7 @@ OvsTunnelVportPendingInit(PVOID context,
* Allow the vport to be deleted, because there is no
* corresponding hyper-v switch part.
*/
-vport->isPresentOnHv = TRUE;
+vport->isAbsentOnHv = TRUE;
if (vportAttrs[OVS_VPORT_ATTR_PORT_NO] != NULL)
[ovs-dev] [PATCH 1/2 v3][branch 2.4] datapath-windows: Code refactoring and fixes in Vport.c
In this patch, there a couple of fixes and some code refactoring:
1. During deletion of "internal" and "external" in
OvsRemoveAndDeleteVport(), we need to check if 'hvDelete' is TRUE before
updating the data structures. Added code comments explaining the
same.
2. Added a OvsRemoveTunnelPort() that gets called from
OvsRemoveAndDeletePort() for the special processing for tunnel ports.
3. Folded in OvsCleanupVportCommon() back into OvsRemoveAndDeletePort(),
since we only need a part of the functionality of
OvsCleanupVportCommon() to be called from
OvsTunnelVportPendingUninit(), and not the entire function.
4. Renamed OvsTunnelVportPendingUninit() to
OvsTunnelVportPendingRemove() since it is basically a "pending" version
of OvsVportTunnelRemove().
Validation:
- Add external port from Hyper-V, add external port from OVS, remove
external port from OVS, remove external port from Hyper-V. No ASSERT
hit.
- Add external port from Hyper-V, add external port from OVS, remove
external port from Hyper-V, remove external port from OVS. No ASSERT
hit.
- Vxlan tunnel port creation/deletion
- Stt tunnel port creation/deletion
- Ping on Vxlan/Stt tunnels
- Ovs Extension load/unload. There's an unrelated issue I found that is
reported in: https://github.com/openvswitch/ovs-issues/issues/86
Signed-off-by: Nithin Raju V
Reported-at: https://github.com/openvswitch/ovs-issues/issues/79
Reported-by: Alin Gabriel Serdean
Reported-by: Nithin Raju
---
v2: created a series
v3: addressed the issues Alin found during validation
---
datapath-windows/ovsext/Vport.c | 229
1 file changed, 117 insertions(+), 112 deletions(-)
diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c
index 9139545..c35d459 100644
--- a/datapath-windows/ovsext/Vport.c
+++ b/datapath-windows/ovsext/Vport.c
@@ -84,15 +84,15 @@ static POVS_VPORT_ENTRY
OvsFindVportByHvNameW(POVS_SWITCH_CONTEXT switchContext,
static NDIS_STATUS InitHvVportCommon(POVS_SWITCH_CONTEXT switchContext,
POVS_VPORT_ENTRY vport,
BOOLEAN newPort);
-static VOID OvsCleanupVportCommon(POVS_SWITCH_CONTEXT switchContext,
- POVS_VPORT_ENTRY vport,
- BOOLEAN hvSwitchPort,
- BOOLEAN hvDelete,
- BOOLEAN ovsDelete);
+static NTSTATUS OvsRemoveTunnelVport(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
+ POVS_SWITCH_CONTEXT switchContext,
+ POVS_VPORT_ENTRY vport,
+ BOOLEAN hvDelete,
+ BOOLEAN ovsDelete);
static VOID OvsTunnelVportPendingInit(PVOID context,
NTSTATUS status,
UINT32 *replyLen);
-static VOID OvsTunnelVportPendingUninit(PVOID context,
+static VOID OvsTunnelVportPendingRemove(PVOID context,
NTSTATUS status,
UINT32 *replyLen);
@@ -201,7 +201,7 @@ HvUpdatePort(POVS_SWITCH_CONTEXT switchContext,
/* Store the nic and the OVS states as Nic Create won't be called */
ovsState = vport->ovsState;
nicState = vport->nicState;
-
+
/*
* Currently only the port friendly name is being updated
* Make sure that no other properties are changed
@@ -1124,16 +1124,75 @@ InitOvsVportCommon(POVS_SWITCH_CONTEXT switchContext,
return STATUS_SUCCESS;
}
-static VOID
-OvsCleanupVportCommon(POVS_SWITCH_CONTEXT switchContext,
- POVS_VPORT_ENTRY vport,
- BOOLEAN hvSwitchPort,
- BOOLEAN hvDelete,
- BOOLEAN ovsDelete)
+
+/*
+ * --
+ * Provides functionality that is partly complementatry to
+ * InitOvsVportCommon()/InitHvVportCommon().
+ *
+ * 'hvDelete' indicates if caller is removing the vport as a result of the
+ * port being removed on the Hyper-V switch.
+ * 'ovsDelete' indicates if caller is removing the vport as a result of the
+ * port being removed from OVS userspace.
+ * --
+ */
+NTSTATUS
+OvsRemoveAndDeleteVport(PVOID usrParamsContext,
+POVS_SWITCH_CONTEXT switchContext,
+POVS_VPORT_ENTRY vport,
+BOOLEAN hvDelete,
+BOOLEAN ovsDelete)
{
+POVS_USER_PARAMS_CONTEXT usrParamsCtx =
+(POVS_USER_PARAMS_CONTEXT)usrParamsContext;
+BOOLEAN hvSwitchPort = FALSE;
BOOLEAN deletedOnOvs = FALSE;
BOOLEAN deletedOnHv = FALSE;
+switch (vport->ovsType) {
+case OVS_VPORT_TYPE_INTERNAL:
+if (!vport->isBridgeInternal) {
+if
Re: [ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.
Would it be possible to keep the lswitch.uuid value on the port like before, but not enforce the integrity there if the network does not exist. Then, have the garbage collector here remove the ports on the network for us when the network is deleted? This way we don't need to have any retry logic on our end. My concern is creating ports on a network is something that occurs usually in a very bursty manor and I think we'll often have to retry. If not I think retrying on our side is probably okay. Thanks, Aaron From: Ben Pfaff Sent: Friday, June 26, 2015 11:37 AM To: Aaron Rosen Cc: dev@openvswitch.org Subject: Re: [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema. On Fri, Jun 26, 2015 at 05:20:16PM +, Aaron Rosen wrote: > I just tested out this patch and it seems to work as expected for > me. I do have a few quick questions/thoughts. > > - One thing I noticed is if I create a logical_port but don't append > the port.uuid to the Logical_Switch.ports column it seems like the > port isn't showing up in ovsdb. I'm not sure if this is expected or > I'm incorrect. That's a consequence of the garbage collection. A Logical_Port that isn't attached to a Logical_Switch gets destroyed. That's true if it was previously attached to a Logical_Switch that was just deleted, or if it was created within the current transaction and never attached to a Logical_Switch. > - Another thing I was thinking about is if requiring the caller to > query ovsdb for the logical_switch to obtain the switch_ports and then > appending the newly create logical port could be problematic. For one > it seems like we need to lock ovsdb for concurrent operations here. OVSDB doesn't support locking, so the equivalent that allows one to avoid problems with "dirty reads" is to verify that the ports are the expected set (the set before the transaction started) and make the transaction abort if it isn't correct. With the OSVDB Python IDL, that's just a matter of calling Row.verify() on the column in question before modifying the column. (If the transaction aborts, then you just retry it.) This is the purpose of the nbrec_logical_switch_verify_ports() calls in do_lport_add() and remove_lport() in ovn-nbctl.c as modified by this commit, by the way. > I was wondering if it would be possible to keep previous schema where > the logical_port stores a reference to it's logical_switch. Though, > when the logical_switch is deleted we would cascade the deletes of the > logical_ports on that switch? OVSDB only supports cascading with the schema in the form used by this comit. > I also think it feels more natural to have a reference from the > logical_port to logical_switch because now it seems like the schema > allows one to create a logical_port without a logical_switch? Perhaps > ovsdb can't currently work in this way which you were explaining in > your commit message though I just wanted to make sure. With this schema, an unattached logical_port can exist temporarily within a transaction but transaction commit will delete it, so that the unattached logical_port will never be visible to other clients (or in the database log). ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] tunneling: Userspace datapath support for Geneve options.
On Thu, Jun 25, 2015 at 11:22:00AM -0700, Jesse Gross wrote:
> Currently the userspace datapath only supports Geneve in a
> basic mode - without options - since the rest of userspace
> previously didn't support options either. This enables the
> userspace datapath to send and receive options as well.
>
> The receive path for extracting the tunnel options isn't entirely
> optimal because it does a lookup on the options on a per-packet
> basis, rather than per-flow like the kernel does. This is not
> as straightforward to do in the userspace datapath since there
> is no translation step between packet formats used in packet vs.
> flow lookup. This can be optimized in the future and in the
> meantime option support is still useful for testing and simulation.
>
> Signed-off-by: Jesse Gross
That was fast!
This gives me a repeatable test failure (on i386), log attached.
"git am" says:
Applying: tunneling: Userspace datapath support for Geneve options.
/home/blp/nicira/ovs/.git/rebase-apply/patch:89: trailing whitespace.
gnh->critical = crit_opt ? 1 : 0;
/home/blp/nicira/ovs/.git/rebase-apply/patch:127: trailing whitespace.
warning: 2 lines add whitespace errors.
In ovs_parse_tnl_push(), I would consider changing
if (!ovs_scan_len(s, &n, "vni=0x%"SCNx32, &vni)) {
to
if (!ovs_scan_len(s, &n, "vni=%"SCNi32, &vni)) {
to allow a handwritten vni to be expressed in decimal (this is not
new code with this patch).
Also in ovs_parse_tnl_push(), it looks like the code now expects
'geneve()' to always end with a doubled ), but I think that it should
only do that if there are options.
The {} syntax looks a little odd nested inside so many (), did you
consider using something like
geneve(crit,vni=0x1c7,option(class=0x,type=0x80,len=4,0xa))
and then just allowing multiple "option"s directly inside geneve()?
# -*- compilation -*-
637. tunnel-push-pop.at:3: testing tunnel_push_pop - action ...
../../tests/tunnel-push-pop.at:5: ovsdb-tool create conf.db
$abs_top_srcdir/vswitchd/vswitch.ovsschema
../../tests/tunnel-push-pop.at:5: ovsdb-server --detach --no-chdir --pidfile
--log-file --remote=punix:$OVS_RUNDIR/db.sock
stderr:
2015-06-26T16:01:00Z|1|vlog|INFO|opened log file
/home/blp/nicira/ovs/_build/tests/testsuite.dir/0637/ovsdb-server.log
cat: cleanup: No such file or directory
../../tests/tunnel-push-pop.at:5: sed < stderr '
/vlog|INFO|opened log file/d
/ovsdb_server|INFO|ovsdb-server (Open vSwitch)/d'
../../tests/tunnel-push-pop.at:5: ovs-vsctl --no-wait init
../../tests/tunnel-push-pop.at:5: ovs-vswitchd --enable-dummy --disable-system
--detach --no-chdir --pidfile --log-file -vvconn -vofproto_dpif
stderr:
2015-06-26T16:01:00Z|1|vlog|INFO|opened log file
/home/blp/nicira/ovs/_build/tests/testsuite.dir/0637/ovs-vswitchd.log
2015-06-26T16:01:00Z|2|ovs_numa|INFO|Discovered 8 CPU cores on NUMA node 0
2015-06-26T16:01:00Z|3|ovs_numa|INFO|Discovered 1 NUMA nodes and 8 CPU cores
2015-06-26T16:01:00Z|4|reconnect|INFO|unix:/home/blp/nicira/ovs/_build/tests/testsuite.dir/0637/db.sock:
connecting...
2015-06-26T16:01:00Z|5|reconnect|INFO|unix:/home/blp/nicira/ovs/_build/tests/testsuite.dir/0637/db.sock:
connected
../../tests/tunnel-push-pop.at:5: sed < stderr '
/ovs_numa|INFO|Discovered /d
/vlog|INFO|opened log file/d
/vswitchd|INFO|ovs-vswitchd (Open vSwitch)/d
/reconnect|INFO|/d
/ofproto|INFO|using datapath ID/d
/ofproto|INFO|datapath ID changed to fedcba9876543210/d'
../../tests/tunnel-push-pop.at:5: ovs-vsctl -- add-br br0 -- set bridge br0
datapath-type=dummy other-config:datapath-id=fedcba9876543210
other-config:hwaddr=aa:55:aa:55:00:00
protocols=[OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15]
fail-mode=secure -- add-port br0 p0 -- set Interface p0 type=dummy
ofport_request=1
../../tests/tunnel-push-pop.at:6: ovs-vsctl add-br int-br -- set bridge int-br
datapath_type=dummy
../../tests/tunnel-push-pop.at:7: ovs-vsctl add-port int-br t2 -- set Interface
t2 type=vxlan \
options:remote_ip=1.1.2.92 options:key=123
ofport_request=2\
-- add-port int-br t1 -- set Interface t1 type=gre \
options:remote_ip=1.1.2.92 options:key=456
ofport_request=3\
-- add-port int-br t3 -- set Interface t3 type=vxlan \
options:remote_ip=1.1.2.93 options:out_key=flow
options:csum=true ofport_request=4\
-- add-port int-br t4 -- set Interface t4 type=geneve \
options:remote_ip=flow options:key=123 ofport_request=5\
../../tests/tunnel-push-pop.at:17: ovs-appctl dpif/show
../../tests/tunnel-push-pop.at:30: ovs-appctl ovs/route/add 1.1.2.92/24 br0
../../tests/tunnel-push-pop.at:32: ovs-appctl netdev-dummy/ip4addr br0
1.1.2.88/24
../../tests/tunnel-push-pop.at:35: ovs-ofctl add-flow br0 action=normal
../../tests/tunnel-push-pop.at:
Re: [ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.
OVSDB can't enforce those exact semantics. OVSDB can express operations like "add element X to this column (regardless of what's currently there)". If this were exposed through the IDL (currently it's not) then that would solve the problem with retries due to burstiness. Does that sound like a good approach? On Fri, Jun 26, 2015 at 07:11:10PM +, Aaron Rosen wrote: > Would it be possible to keep the lswitch.uuid value on the port like > before, but not enforce the integrity there if the network does not > exist. Then, have the garbage collector here remove the ports on the > network for us when the network is deleted? This way we don't need to > have any retry logic on our end. My concern is creating ports on a > network is something that occurs usually in a very bursty manor and I > think we'll often have to retry. If not I think retrying on our side > is probably okay. > > Thanks, > > Aaron > > > > > From: Ben Pfaff > Sent: Friday, June 26, 2015 11:37 AM > To: Aaron Rosen > Cc: dev@openvswitch.org > Subject: Re: [PATCH] ovn: Take advantage of OVSDB garbage collection in > OVN_Northbound schema. > > On Fri, Jun 26, 2015 at 05:20:16PM +, Aaron Rosen wrote: > > I just tested out this patch and it seems to work as expected for > > me. I do have a few quick questions/thoughts. > > > > - One thing I noticed is if I create a logical_port but don't append > > the port.uuid to the Logical_Switch.ports column it seems like the > > port isn't showing up in ovsdb. I'm not sure if this is expected or > > I'm incorrect. > > That's a consequence of the garbage collection. A Logical_Port that > isn't attached to a Logical_Switch gets destroyed. That's true if it > was previously attached to a Logical_Switch that was just deleted, or if > it was created within the current transaction and never attached to a > Logical_Switch. > > > - Another thing I was thinking about is if requiring the caller to > > query ovsdb for the logical_switch to obtain the switch_ports and then > > appending the newly create logical port could be problematic. For one > > it seems like we need to lock ovsdb for concurrent operations here. > > OVSDB doesn't support locking, so the equivalent that allows one to > avoid problems with "dirty reads" is to verify that the ports are the > expected set (the set before the transaction started) and make the > transaction abort if it isn't correct. With the OSVDB Python IDL, > that's just a matter of calling Row.verify() on the column in question > before modifying the column. (If the transaction aborts, then you just > retry it.) > > This is the purpose of the nbrec_logical_switch_verify_ports() calls in > do_lport_add() and remove_lport() in ovn-nbctl.c as modified by this > commit, by the way. > > > I was wondering if it would be possible to keep previous schema where > > the logical_port stores a reference to it's logical_switch. Though, > > when the logical_switch is deleted we would cascade the deletes of the > > logical_ports on that switch? > > OVSDB only supports cascading with the schema in the form used by this > comit. > > > I also think it feels more natural to have a reference from the > > logical_port to logical_switch because now it seems like the schema > > allows one to create a logical_port without a logical_switch? Perhaps > > ovsdb can't currently work in this way which you were explaining in > > your commit message though I just wanted to make sure. > > With this schema, an unattached logical_port can exist temporarily > within a transaction but transaction commit will delete it, so that the > unattached logical_port will never be visible to other clients (or in > the database log). ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.
That sounds like it would solve this problem for us :) In the delete logical_port case we'd just need to delete the port entry and the garbage collector will automatically handle updating the logical_switch.ports with the current ports right? ___ From: Ben Pfaff Sent: Friday, June 26, 2015 12:51 PM To: Aaron Rosen Cc: dev@openvswitch.org Subject: Re: [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema. OVSDB can't enforce those exact semantics. OVSDB can express operations like "add element X to this column (regardless of what's currently there)". If this were exposed through the IDL (currently it's not) then that would solve the problem with retries due to burstiness. Does that sound like a good approach? On Fri, Jun 26, 2015 at 07:11:10PM +, Aaron Rosen wrote: > Would it be possible to keep the lswitch.uuid value on the port like > before, but not enforce the integrity there if the network does not > exist. Then, have the garbage collector here remove the ports on the > network for us when the network is deleted? This way we don't need to > have any retry logic on our end. My concern is creating ports on a > network is something that occurs usually in a very bursty manor and I > think we'll often have to retry. If not I think retrying on our side > is probably okay. > > Thanks, > > Aaron > > > > > From: Ben Pfaff > Sent: Friday, June 26, 2015 11:37 AM > To: Aaron Rosen > Cc: dev@openvswitch.org > Subject: Re: [PATCH] ovn: Take advantage of OVSDB garbage collection in > OVN_Northbound schema. > > On Fri, Jun 26, 2015 at 05:20:16PM +, Aaron Rosen wrote: > > I just tested out this patch and it seems to work as expected for > > me. I do have a few quick questions/thoughts. > > > > - One thing I noticed is if I create a logical_port but don't append > > the port.uuid to the Logical_Switch.ports column it seems like the > > port isn't showing up in ovsdb. I'm not sure if this is expected or > > I'm incorrect. > > That's a consequence of the garbage collection. A Logical_Port that > isn't attached to a Logical_Switch gets destroyed. That's true if it > was previously attached to a Logical_Switch that was just deleted, or if > it was created within the current transaction and never attached to a > Logical_Switch. > > > - Another thing I was thinking about is if requiring the caller to > > query ovsdb for the logical_switch to obtain the switch_ports and then > > appending the newly create logical port could be problematic. For one > > it seems like we need to lock ovsdb for concurrent operations here. > > OVSDB doesn't support locking, so the equivalent that allows one to > avoid problems with "dirty reads" is to verify that the ports are the > expected set (the set before the transaction started) and make the > transaction abort if it isn't correct. With the OSVDB Python IDL, > that's just a matter of calling Row.verify() on the column in question > before modifying the column. (If the transaction aborts, then you just > retry it.) > > This is the purpose of the nbrec_logical_switch_verify_ports() calls in > do_lport_add() and remove_lport() in ovn-nbctl.c as modified by this > commit, by the way. > > > I was wondering if it would be possible to keep previous schema where > > the logical_port stores a reference to it's logical_switch. Though, > > when the logical_switch is deleted we would cascade the deletes of the > > logical_ports on that switch? > > OVSDB only supports cascading with the schema in the form used by this > comit. > > > I also think it feels more natural to have a reference from the > > logical_port to logical_switch because now it seems like the schema > > allows one to create a logical_port without a logical_switch? Perhaps > > ovsdb can't currently work in this way which you were explaining in > > your commit message though I just wanted to make sure. > > With this schema, an unattached logical_port can exist temporarily > within a transaction but transaction commit will delete it, so that the > unattached logical_port will never be visible to other clients (or in > the database log). ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] tunneling: Userspace datapath support for Geneve options.
On Fri, Jun 26, 2015 at 12:47 PM, Ben Pfaff wrote:
> On Thu, Jun 25, 2015 at 11:22:00AM -0700, Jesse Gross wrote:
>> Currently the userspace datapath only supports Geneve in a
>> basic mode - without options - since the rest of userspace
>> previously didn't support options either. This enables the
>> userspace datapath to send and receive options as well.
>>
>> The receive path for extracting the tunnel options isn't entirely
>> optimal because it does a lookup on the options on a per-packet
>> basis, rather than per-flow like the kernel does. This is not
>> as straightforward to do in the userspace datapath since there
>> is no translation step between packet formats used in packet vs.
>> flow lookup. This can be optimized in the future and in the
>> meantime option support is still useful for testing and simulation.
>>
>> Signed-off-by: Jesse Gross
>
> That was fast!
Believe it or not, I didn't actually write it within 10 minutes of
pushing the other patches :)
> This gives me a repeatable test failure (on i386), log attached.
This is due to the userspace tunneling patch that I applied yesterday
afternoon, after this patch was sent out. I fixed it in my rebased
version.
> "git am" says:
>
> Applying: tunneling: Userspace datapath support for Geneve options.
> /home/blp/nicira/ovs/.git/rebase-apply/patch:89: trailing whitespace.
> gnh->critical = crit_opt ? 1 : 0;
> /home/blp/nicira/ovs/.git/rebase-apply/patch:127: trailing whitespace.
>
> warning: 2 lines add whitespace errors.
Fixed.
> In ovs_parse_tnl_push(), I would consider changing
> if (!ovs_scan_len(s, &n, "vni=0x%"SCNx32, &vni)) {
> to
> if (!ovs_scan_len(s, &n, "vni=%"SCNi32, &vni)) {
> to allow a handwritten vni to be expressed in decimal (this is not
> new code with this patch).
Thanks, that's a good idea.
> Also in ovs_parse_tnl_push(), it looks like the code now expects
> 'geneve()' to always end with a doubled ), but I think that it should
> only do that if there are options.
This isn't new - before we used to scan for double parentheses as part
of the VNI, it's just broken out now. The second one is closing the
tunnel block (which is probably not great layering but that's how the
code is setup now). We have tests for both option and non-option
variants as well.
> The {} syntax looks a little odd nested inside so many (), did you
> consider using something like
> geneve(crit,vni=0x1c7,option(class=0x,type=0x80,len=4,0xa))
> and then just allowing multiple "option"s directly inside geneve()?
I guess I'm a little hesitant to do this because it currently matches
the kernel actions, which is nice from a consistency and code reuse
perspective. In turn, the kernel output is closely related to the
netlink formatting, which is nice for debugging if things are somehow
mangled.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.
On Fri, Jun 26, 2015 at 08:05:53PM +, Aaron Rosen wrote: > That sounds like it would solve this problem for us :) I think so. > In the delete logical_port case we'd just need to delete the port > entry and the garbage collector will automatically handle updating the > logical_switch.ports with the current ports right? If this proposed patch is applied, then it's the opposite: if you remove the port from Logical_Switch.ports, then the garbage collector will automatically delete the Logical_Port object (because there are no remaining references to it). ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] tunneling: Userspace datapath support for Geneve options.
On Fri, Jun 26, 2015 at 01:14:17PM -0700, Jesse Gross wrote:
> On Fri, Jun 26, 2015 at 12:47 PM, Ben Pfaff wrote:
> > On Thu, Jun 25, 2015 at 11:22:00AM -0700, Jesse Gross wrote:
> >> Currently the userspace datapath only supports Geneve in a
> >> basic mode - without options - since the rest of userspace
> >> previously didn't support options either. This enables the
> >> userspace datapath to send and receive options as well.
> >>
> >> The receive path for extracting the tunnel options isn't entirely
> >> optimal because it does a lookup on the options on a per-packet
> >> basis, rather than per-flow like the kernel does. This is not
> >> as straightforward to do in the userspace datapath since there
> >> is no translation step between packet formats used in packet vs.
> >> flow lookup. This can be optimized in the future and in the
> >> meantime option support is still useful for testing and simulation.
> >>
> >> Signed-off-by: Jesse Gross
> >
> > That was fast!
>
> Believe it or not, I didn't actually write it within 10 minutes of
> pushing the other patches :)
>
> > This gives me a repeatable test failure (on i386), log attached.
>
> This is due to the userspace tunneling patch that I applied yesterday
> afternoon, after this patch was sent out. I fixed it in my rebased
> version.
OK, great.
> > Also in ovs_parse_tnl_push(), it looks like the code now expects
> > 'geneve()' to always end with a doubled ), but I think that it should
> > only do that if there are options.
>
> This isn't new - before we used to scan for double parentheses as part
> of the VNI, it's just broken out now. The second one is closing the
> tunnel block (which is probably not great layering but that's how the
> code is setup now). We have tests for both option and non-option
> variants as well.
OK.
> > The {} syntax looks a little odd nested inside so many (), did you
> > consider using something like
> > geneve(crit,vni=0x1c7,option(class=0x,type=0x80,len=4,0xa))
> > and then just allowing multiple "option"s directly inside geneve()?
>
> I guess I'm a little hesitant to do this because it currently matches
> the kernel actions, which is nice from a consistency and code reuse
> perspective. In turn, the kernel output is closely related to the
> netlink formatting, which is nice for debugging if things are somehow
> mangled.
OK.
Acked-by: Ben Pfaff
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] tunneling: Userspace datapath support for Geneve options.
On Fri, Jun 26, 2015 at 2:16 PM, Ben Pfaff wrote:
> On Fri, Jun 26, 2015 at 01:14:17PM -0700, Jesse Gross wrote:
>> On Fri, Jun 26, 2015 at 12:47 PM, Ben Pfaff wrote:
>> > On Thu, Jun 25, 2015 at 11:22:00AM -0700, Jesse Gross wrote:
>> >> Currently the userspace datapath only supports Geneve in a
>> >> basic mode - without options - since the rest of userspace
>> >> previously didn't support options either. This enables the
>> >> userspace datapath to send and receive options as well.
>> >>
>> >> The receive path for extracting the tunnel options isn't entirely
>> >> optimal because it does a lookup on the options on a per-packet
>> >> basis, rather than per-flow like the kernel does. This is not
>> >> as straightforward to do in the userspace datapath since there
>> >> is no translation step between packet formats used in packet vs.
>> >> flow lookup. This can be optimized in the future and in the
>> >> meantime option support is still useful for testing and simulation.
>> >>
>> >> Signed-off-by: Jesse Gross
>> >
>> > That was fast!
>>
>> Believe it or not, I didn't actually write it within 10 minutes of
>> pushing the other patches :)
>>
>> > This gives me a repeatable test failure (on i386), log attached.
>>
>> This is due to the userspace tunneling patch that I applied yesterday
>> afternoon, after this patch was sent out. I fixed it in my rebased
>> version.
>
> OK, great.
>
>> > Also in ovs_parse_tnl_push(), it looks like the code now expects
>> > 'geneve()' to always end with a doubled ), but I think that it should
>> > only do that if there are options.
>>
>> This isn't new - before we used to scan for double parentheses as part
>> of the VNI, it's just broken out now. The second one is closing the
>> tunnel block (which is probably not great layering but that's how the
>> code is setup now). We have tests for both option and non-option
>> variants as well.
>
> OK.
>
>> > The {} syntax looks a little odd nested inside so many (), did you
>> > consider using something like
>> > geneve(crit,vni=0x1c7,option(class=0x,type=0x80,len=4,0xa))
>> > and then just allowing multiple "option"s directly inside geneve()?
>>
>> I guess I'm a little hesitant to do this because it currently matches
>> the kernel actions, which is nice from a consistency and code reuse
>> perspective. In turn, the kernel output is closely related to the
>> netlink formatting, which is nice for debugging if things are somehow
>> mangled.
>
> OK.
>
> Acked-by: Ben Pfaff
Thanks, applied to master.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 3/4] ovn-integrate: A new OVN utility script.
On Thu, Jun 25, 2015 at 02:37:41PM -0700, Ben Pfaff wrote: > On Thu, Jun 25, 2015 at 01:51:02PM -0700, Gurucharan Shetty wrote: > > > I was really heading more in the direction of suggesting a name that > > > mentioned docker. I'm OK with including the nics-to-bridge > > > functionality. > > > > I think it will be true for any other container formats too (if at > > all). And it is likely useful to setup VM based OVN solution too. > > OK, that explains the name then. > > I didn't really review the code yet since I was still trying to > understand how it fit in. I'll review it soon. It seems fine to me, except that I think it would still be useful to have a manpage. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.
inline From: Ben Pfaff Sent: Friday, June 26, 2015 2:12 PM To: Aaron Rosen Cc: dev@openvswitch.org Subject: Re: [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema. On Fri, Jun 26, 2015 at 08:05:53PM +, Aaron Rosen wrote: > That sounds like it would solve this problem for us :) I think so. > In the delete logical_port case we'd just need to delete the port > entry and the garbage collector will automatically handle updating the > logical_switch.ports with the current ports right? If this proposed patch is applied, then it's the opposite: if you remove the port from Logical_Switch.ports, then the garbage collector will automatically delete the Logical_Port object (because there are no remaining references to it). arosen - in that case I think we also need something that can ""remove element X from this column (regardless of what's currently there)" otherwise we'll have the same problem in the delete case no? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC v2 4/4] Introduce an openvswitch driver for Docker networking.
On Mon, Jun 22, 2015 at 02:19:45AM -0700, Gurucharan Shetty wrote: > Docker committed experimental support for multi-host > networking yesterday. This commit adds a driver that > works with that experimental support. Since Docker > code is not part of any official Docker releases yet, > this patch is sent as a RFC. > > Signed-off-by: Gurucharan Shetty This seems fine as an RFC. The documentation is especially helpful. I think you said that it's not ready for a final review anyhow, so I did not scrutinize the details. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4] ovs-ofctl: replace-flows and diff-flows support for multiple tables
On Thu, Jun 25, 2015 at 02:19:40AM -0700, Shashank Shanbhag wrote: > From: Shashank Shanbhag > > Fix replace-flows and diff-flows to modify/diff flows in multiple tables. > Add a --tables(-T) option that allows the user to specify a comma-separated > list of table indexes to replace/diff. > > Signed-off-by: Shashank Shanbhag > Acked-by: Romain Lenglet This is much closer! Thank you. Now, please read CodingStyle.md and carefully fix the violations. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.
On Fri, Jun 26, 2015 at 09:35:04PM +, Aaron Rosen wrote: > > In the delete logical_port case we'd just need to delete the port > > entry and the garbage collector will automatically handle updating the > > logical_switch.ports with the current ports right? > > If this proposed patch is applied, then it's the opposite: if you remove > the port from Logical_Switch.ports, then the garbage collector will > automatically delete the Logical_Port object (because there are no > remaining references to it). > > > arosen - in that case I think we also need something that can ""remove > element X from this column > (regardless of what's currently there)" otherwise we'll have the same problem > in the delete case no? Well, yes, I would provide that feature too, it uses the same mechanism. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2 v3][branch 2.4] datapath-windows: Code refactoring and fixes in Vport.c
Acked-by: Alin Gabriel Serdean
-Mesaj original-
De la: dev [mailto:dev-boun...@openvswitch.org] În numele Nithin Raju
Trimis: Friday, June 26, 2015 9:51 PM
Către: dev@openvswitch.org
Subiect: [ovs-dev] [PATCH 1/2 v3][branch 2.4] datapath-windows: Code
refactoring and fixes in Vport.c
In this patch, there a couple of fixes and some code refactoring:
1. During deletion of "internal" and "external" in
OvsRemoveAndDeleteVport(), we need to check if 'hvDelete' is TRUE before
updating the data structures. Added code comments explaining the
same.
2. Added a OvsRemoveTunnelPort() that gets called from
OvsRemoveAndDeletePort() for the special processing for tunnel ports.
3. Folded in OvsCleanupVportCommon() back into OvsRemoveAndDeletePort(),
since we only need a part of the functionality of
OvsCleanupVportCommon() to be called from
OvsTunnelVportPendingUninit(), and not the entire function.
4. Renamed OvsTunnelVportPendingUninit() to
OvsTunnelVportPendingRemove() since it is basically a "pending" version
of OvsVportTunnelRemove().
Validation:
- Add external port from Hyper-V, add external port from OVS, remove external
port from OVS, remove external port from Hyper-V. No ASSERT hit.
- Add external port from Hyper-V, add external port from OVS, remove external
port from Hyper-V, remove external port from OVS. No ASSERT hit.
- Vxlan tunnel port creation/deletion
- Stt tunnel port creation/deletion
- Ping on Vxlan/Stt tunnels
- Ovs Extension load/unload. There's an unrelated issue I found that is
reported in: https://github.com/openvswitch/ovs-issues/issues/86
Signed-off-by: Nithin Raju V
Reported-at: https://github.com/openvswitch/ovs-issues/issues/79
Reported-by: Alin Gabriel Serdean
Reported-by: Nithin Raju
---
v2: created a series
v3: addressed the issues Alin found during validation
---
datapath-windows/ovsext/Vport.c | 229
1 file changed, 117 insertions(+), 112 deletions(-)
diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c
index 9139545..c35d459 100644
--- a/datapath-windows/ovsext/Vport.c
+++ b/datapath-windows/ovsext/Vport.c
@@ -84,15 +84,15 @@ static POVS_VPORT_ENTRY
OvsFindVportByHvNameW(POVS_SWITCH_CONTEXT switchContext, static NDIS_STATUS
InitHvVportCommon(POVS_SWITCH_CONTEXT switchContext,
POVS_VPORT_ENTRY vport,
BOOLEAN newPort); -static VOID
OvsCleanupVportCommon(POVS_SWITCH_CONTEXT switchContext,
- POVS_VPORT_ENTRY vport,
- BOOLEAN hvSwitchPort,
- BOOLEAN hvDelete,
- BOOLEAN ovsDelete);
+static NTSTATUS OvsRemoveTunnelVport(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
+ POVS_SWITCH_CONTEXT switchContext,
+ POVS_VPORT_ENTRY vport,
+ BOOLEAN hvDelete,
+ BOOLEAN ovsDelete);
static VOID OvsTunnelVportPendingInit(PVOID context,
NTSTATUS status,
UINT32 *replyLen); -static VOID
OvsTunnelVportPendingUninit(PVOID context,
+static VOID OvsTunnelVportPendingRemove(PVOID context,
NTSTATUS status,
UINT32 *replyLen);
@@ -201,7 +201,7 @@ HvUpdatePort(POVS_SWITCH_CONTEXT switchContext,
/* Store the nic and the OVS states as Nic Create won't be called */
ovsState = vport->ovsState;
nicState = vport->nicState;
-
+
/*
* Currently only the port friendly name is being updated
* Make sure that no other properties are changed @@ -1124,16 +1124,75 @@
InitOvsVportCommon(POVS_SWITCH_CONTEXT switchContext,
return STATUS_SUCCESS;
}
-static VOID
-OvsCleanupVportCommon(POVS_SWITCH_CONTEXT switchContext,
- POVS_VPORT_ENTRY vport,
- BOOLEAN hvSwitchPort,
- BOOLEAN hvDelete,
- BOOLEAN ovsDelete)
+
+/*
+ *
+---
+---
+ * Provides functionality that is partly complementatry to
+ * InitOvsVportCommon()/InitHvVportCommon().
+ *
+ * 'hvDelete' indicates if caller is removing the vport as a result of
+the
+ * port being removed on the Hyper-V switch.
+ * 'ovsDelete' indicates if caller is removing the vport as a result of
+the
+ * port being removed from OVS userspace.
+ *
+---
+---
+ */
+NTSTATUS
+OvsRemoveAndDeleteVport(PVOID usrParamsContext,
+POVS_SWITCH_CONTEXT switchContext,
+POVS_VPORT_ENTRY vport,
+BOOLEAN hvDelete,
+BOOLEAN ovsDelete)
{
+POVS
Re: [ovs-dev] [PATCH 2/2 v3][branch 2.4] datapath-windows: Rename 'vport->isPresentOnHv' to 'isAbsentOnHv'
Acked-by: Alin Gabriel Serdean
-Mesaj original-
De la: dev [mailto:dev-boun...@openvswitch.org] În numele Nithin Raju
Trimis: Friday, June 26, 2015 9:51 PM
Către: dev@openvswitch.org
Subiect: [ovs-dev] [PATCH 2/2 v3][branch 2.4] datapath-windows: Rename
'vport->isPresentOnHv' to 'isAbsentOnHv'
Looking at the code, the flag 'vport->isPresentOnHv' is actually indicating if
the vport is present on the Hyper-V switch or not, but the logic seems to be
inverse. 'isPresentOnHv == TRUE' indicates that the vport is not present on the
Hyper-V switch. Eg. VXLAN port, would have isPresentOnHv == TRUE.
In this patch, we rename the variable to reflect its meaning.
vport->isAbsentOnHv is TRUE iff:
- vport is bridge internal port
- vport is tunnel port
- vport was added from Hyper-V and also from OVS, but got deleted from Hyper-V
Signed-off-by: Nithin Raju
---
v2: created a series
v3: addressed the issues Alin found during validation
---
datapath-windows/ovsext/Vport.c | 22 +++---
datapath-windows/ovsext/Vport.h | 2 +-
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c
index c35d459..a60e7e3 100644
--- a/datapath-windows/ovsext/Vport.c
+++ b/datapath-windows/ovsext/Vport.c
@@ -129,7 +129,7 @@ HvCreatePort(POVS_SWITCH_CONTEXT switchContext,
vport = OvsFindVportByHvNameW(gOvsSwitchContext,
portParam->PortFriendlyName.String,
portParam->PortFriendlyName.Length);
-if (vport && vport->isPresentOnHv == FALSE) {
+if (vport && vport->isAbsentOnHv == FALSE) {
OVS_LOG_ERROR("Port add failed since a port already exists on "
"the specified port Id: %u, ovsName: %s",
portParam->PortId, vport->ovsName); @@ -138,7 +138,7 @@
HvCreatePort(POVS_SWITCH_CONTEXT switchContext,
}
if (vport != NULL) {
-ASSERT(vport->isPresentOnHv);
+ASSERT(vport->isAbsentOnHv);
ASSERT(vport->portNo != OVS_DPPORT_NUMBER_INVALID);
/*
@@ -153,7 +153,7 @@ HvCreatePort(POVS_SWITCH_CONTEXT switchContext,
status = STATUS_DATA_NOT_ACCEPTED;
goto create_port_done;
}
-vport->isPresentOnHv = FALSE;
+vport->isAbsentOnHv = FALSE;
} else {
vport = (POVS_VPORT_ENTRY)OvsAllocateVport();
if (vport == NULL) {
@@ -759,7 +759,7 @@ OvsAllocateVport(VOID)
}
RtlZeroMemory(vport, sizeof (OVS_VPORT_ENTRY));
vport->ovsState = OVS_STATE_UNKNOWN;
-vport->isPresentOnHv = FALSE;
+vport->isAbsentOnHv = FALSE;
vport->portNo = OVS_DPPORT_NUMBER_INVALID;
InitializeListHead(&vport->ovsNameLink);
@@ -1152,7 +1152,7 @@ OvsRemoveAndDeleteVport(PVOID usrParamsContext,
switch (vport->ovsType) {
case OVS_VPORT_TYPE_INTERNAL:
if (!vport->isBridgeInternal) {
-if (hvDelete && vport->isPresentOnHv == FALSE) {
+if (hvDelete && vport->isAbsentOnHv == FALSE) {
switchContext->internalPortId = 0;
switchContext->internalVport = NULL;
OvsInternalAdapterDown(); @@ -1200,7 +1200,7 @@
OvsRemoveAndDeleteVport(PVOID usrParamsContext,
*
* Both 'hvDelete' and 'ovsDelete' can be set to TRUE by the caller.
*/
-if (vport->isPresentOnHv == TRUE) {
+if (vport->isAbsentOnHv == TRUE) {
deletedOnHv = TRUE;
}
if (vport->portNo == OVS_DPPORT_NUMBER_INVALID) { @@ -1208,7 +1208,7 @@
OvsRemoveAndDeleteVport(PVOID usrParamsContext,
}
if (hvDelete && !deletedOnHv) {
-vport->isPresentOnHv = TRUE;
+vport->isAbsentOnHv = TRUE;
if (vport->isExternal) {
ASSERT(vport->nicIndex != 0); @@ -1447,7 +1447,7 @@
OvsClearAllSwitchVports(POVS_SWITCH_CONTEXT switchContext)
vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY, portNoLink);
ASSERT(OvsIsTunnelVportType(vport->ovsType) ||
(vport->ovsType == OVS_VPORT_TYPE_INTERNAL &&
-vport->isBridgeInternal) || vport->isPresentOnHv == TRUE);
+vport->isBridgeInternal) || vport->isAbsentOnHv ==
+ TRUE);
OvsRemoveAndDeleteVport(NULL, switchContext, vport, TRUE, TRUE);
}
}
@@ -2198,7 +2198,7 @@ OvsNewVportCmdHandler(POVS_USER_PARAMS_CONTEXT
usrParamsCtx,
* Allow the vport to be deleted, because there is no
* corresponding hyper-v switch part.
*/
-vport->isPresentOnHv = TRUE;
+vport->isAbsentOnHv = TRUE;
} else {
goto Cleanup;
}
@@ -2523,7 +2523,7 @@ OvsTunnelVportPendingRemove(PVOID context,
}
}
-ASSERT(vport->isPresentOnHv == TRUE);
+ASSERT(vport->isAbsentOnHv == TRUE);
ASSERT(vport->portNo != OVS_DPPORT_NUMBER_INVALID);
/* Remove the port from the relevant lists. */
Re: [ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.
Sounds good, thanks Ben! From: Ben Pfaff Sent: Friday, June 26, 2015 2:50 PM To: Aaron Rosen Cc: dev@openvswitch.org Subject: Re: [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema. On Fri, Jun 26, 2015 at 09:35:04PM +, Aaron Rosen wrote: > > In the delete logical_port case we'd just need to delete the port > > entry and the garbage collector will automatically handle updating the > > logical_switch.ports with the current ports right? > > If this proposed patch is applied, then it's the opposite: if you remove > the port from Logical_Switch.ports, then the garbage collector will > automatically delete the Logical_Port object (because there are no > remaining references to it). > > > arosen - in that case I think we also need something that can ""remove > element X from this column > (regardless of what's currently there)" otherwise we'll have the same problem > in the delete case no? Well, yes, I would provide that feature too, it uses the same mechanism. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv2 1/2] vlog: abstract out interface to syslog daemon
On Thu, Jun 25, 2015 at 12:54:22PM -0700, Ansis Atteka wrote: > This patch helps to address two issues that are present on Ubuntu > 15.04 (and most likely other Linux distributions) where rsyslog daemon > is configured to relay log messages from OVS to a remote log collector > and syslog format being used is something other than the one defined in > RFC 3164. These two issues are: > > 1. libc syslog() function always adds RFC 3164 prefix to syslog >messages before sending them over /dev/log Unix domain socket. >This does not allow us to use libc syslog() function to log in >RFC 5424 format; and > > 2. rsyslogd daemon that comes with Ubuntu 15.04 is too old and >uses hardcoded syslog message parser when it received messages >over /dev/log UNIX domain socket. > > Solution to those two issues would be to use the newly introduced > --syslog-method=udp:127.0.0.1:514 command line argument when starting > OVS. > > Signed-Off-By: Ansis Atteka Thanks! I guess that we will deprecate --syslog-target later. The 'o' and 'b' in Signed-off-by are normally lowercase (some of the Git tools don't recognize them properly in uppercase). I think that syslog_libc_class and syslog_direct_class should be declared static. I think that the casts in vlog_set_syslog_method() can be removed. I would tend to remove the casts in syslog_*_create() too, e.g.: @@ -65,7 +65,7 @@ syslog_direct_create(const char *method) this->fd = -1; } -return (struct syslogger *)this; +return &this->parent; } static void This might be a shorter way to express the usage, in vlog_usage(): --syslog-method=(libc|unix:file|udp:ip:port)\n\ specify how to send messages to syslog daemon\n\ The formatted documentation looks better with indentation around the bulleted list: diff --git a/lib/vlog.man b/lib/vlog.man index b08b6b8..6994eec 100644 --- a/lib/vlog.man +++ b/lib/vlog.man @@ -79,6 +79,7 @@ a hostname. .IP "\fB\-\-syslog\-method=\fImethod\fR" Specify \fImethod\fR how syslog messages should be sent to syslog daemon. Following forms are supported: +.RS .IP \(bu \fBlibc\fR, use libc \fBsyslog()\fR function. This is the default behavior. Downside of using this options is that libc adds fixed prefix to every @@ -100,3 +101,4 @@ to listen on the specified UDP port, accidental iptables rules could be interfering with local syslog traffic and there are some security considerations that apply to UDP sockets, but do not apply to UNIX domain sockets. +.RE Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv2 2/2] vlog: add "vlog/list-pattern" command
On Thu, Jun 25, 2015 at 12:54:23PM -0700, Ansis Atteka wrote: > This patch allows to query logging format at the runtime for each destination > with "vlog/list-pattern" command. > > Signed-Off-By: Ansis Atteka Compiler warnings: ../lib/vlog.c:618:58: error: unused parameter 'argc' [-Werror,-Wunused-parameter] vlog_unixctl_list_pattern(struct unixctl_conn *conn, int argc, ^ ../lib/vlog.c:619:39: error: unused parameter 'argv' [-Werror,-Wunused-parameter] const char *argv[], void *aux OVS_UNUSED) ^ Assuming you fix those, Acked-by: Ben Pfaff Thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH]: ovsdb: add capability to dump table content in ovsdb-client
Added capability of displaying tables through an optional 'table' argument
to 'ovsdb-client dump'. When specified, ovsdb-client will iterate
through all tables in the chosen ovsdb, and create a transaction only
for that table instead of all, and then print the response for that
transaction.
Signed-off-by: Sabyasachi Sengupta
---
ovsdb/ovsdb-client.c | 105 +-
1 files changed, 69 insertions(+), 36 deletions(-)
diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
index 2942953..84474d0 100644
--- a/ovsdb/ovsdb-client.c
+++ b/ovsdb/ovsdb-client.c
@@ -255,8 +255,8 @@ usage(void)
"\n monitor [SERVER] [DATABASE] ALL\n"
"monitor all changes to all columns in all tables\n"
"in DATBASE on SERVER.\n"
- "\n dump [SERVER] [DATABASE]\n"
- "dump contents of DATABASE on SERVER to stdout\n"
+ "\n dump [SERVER] [DATABASE] [TABLE]\n"
+ "dump contents of table(s) in DATABASE on SERVER to stdout\n"
"\nThe default SERVER is unix:%s/db.sock.\n"
"The default DATABASE is Open_vSwitch.\n",
program_name, program_name, ovs_rundir());
@@ -1042,44 +1042,80 @@ dump_table(const struct ovsdb_table_schema *ts, struct
json_array *rows)
}
static void
-do_dump(struct jsonrpc *rpc, const char *database,
-int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
+create_table_transaction(struct json *transaction,
+ const struct shash_node *table)
+{
+const struct ovsdb_table_schema *ts = table->data;
+struct json *op, *columns;
+struct shash_node *node;
+
+columns = json_array_create_empty();
+SHASH_FOR_EACH (node, &ts->columns) {
+const struct ovsdb_column *column = node->data;
+
+if (strcmp(column->name, "_version")) {
+json_array_add(columns, json_string_create(column->name));
+}
+}
+
+op = json_object_create();
+json_object_put_string(op, "op", "select");
+json_object_put_string(op, "table", table->name);
+json_object_put(op, "where", json_array_create_empty());
+json_object_put(op, "columns", columns);
+json_array_add(transaction, op);
+}
+
+static void
+print_table(const struct json *op_result, const struct shash_node *table)
+{
+const struct ovsdb_table_schema *ts = table->data;
+struct json *rows;
+
+if (op_result->type != JSON_OBJECT
+|| !(rows = shash_find_data(json_object(op_result), "rows"))
+|| rows->type != JSON_ARRAY) {
+ovs_fatal(0, "%s table reply is not an object with a \"rows\" "
+ "member array: %s", ts->name, json_to_string(op_result, 0));
+}
+dump_table(ts, &rows->u.array);
+}
+
+static void
+do_dump(struct jsonrpc *rpc, const char *database, int argc, char *argv[])
{
struct jsonrpc_msg *request, *reply;
struct ovsdb_schema *schema;
struct json *transaction;
const struct shash_node **tables;
-size_t n_tables;
+size_t n_tables, n_tables_out;
+char *table_name = (argc == 1) ? argv[0] : NULL;
+bool table_found = false;
size_t i;
schema = fetch_schema(rpc, database);
tables = shash_sort(&schema->tables);
n_tables = shash_count(&schema->tables);
+n_tables_out = table_name ? 1 : n_tables;
/* Construct transaction to retrieve entire database. */
transaction = json_array_create_1(json_string_create(database));
for (i = 0; i < n_tables; i++) {
-const struct ovsdb_table_schema *ts = tables[i]->data;
-struct json *op, *columns;
-struct shash_node *node;
-
-columns = json_array_create_empty();
-SHASH_FOR_EACH (node, &ts->columns) {
-const struct ovsdb_column *column = node->data;
-
-if (strcmp(column->name, "_version")) {
-json_array_add(columns, json_string_create(column->name));
+if (table_name) {
+if (!strncmp(tables[i]->name, table_name,
+ MIN(strlen(tables[i]->name), strlen(table_name {
+create_table_transaction(transaction, tables[i]);
+table_found = true;
+break;
}
+} else {
+create_table_transaction(transaction, tables[i]);
}
-
-op = json_object_create();
-json_object_put_string(op, "op", "select");
-json_object_put_string(op, "table", tables[i]->name);
-json_object_put(op, "where", json_array_create_empty());
-json_object_put(op, "columns", columns);
-json_array_add(transaction, op);
+}
+if (table_name && !table_found) {
+ovs_fatal(0, "specified table '%s' does not exist", table_name);
}
/* Send request, get reply. */
@@ -1088,24 +1124,21 @@ do_dump(struct jsonrpc *rpc, const char *database,
/* Print database contents. */
if (reply->result->type != JSON_ARRAY
-|| reply->result->u.array.n
