[ovs-dev] [PATCH v2 1/3] ovs-vswitchd: An option to wait for userspace > flow restore to complete. (Gurucharan Shetty)

Thu May 30 15:12:16 UTC 2013

On Wed, May 29, 2013 at 7:18 PM, Jing Ai <ai_jing2000 at hotmail.com> wrote:

>
>
> ------------------------------
> Date: Wed, 29 May 2013 13:46:05 -0700
> Subject: Re: [ovs-dev] [PATCH v2 1/3] ovs-vswitchd: An option to wait for
> userspace > flow restore to complete. (Gurucharan Shetty)
> From: shettyg at nicira.com
> To: ai_jing2000 at hotmail.com
> CC: dev at openvswitch.org
>
>
> On Wed, May 29, 2013 at 1:23 PM, Jing Ai <ai_jing2000 at hotmail.com> wrote:
>
> > While upgrading openvswitch, it helps to restore openflow flows before
> > starting packet processing. Typically we want to restart openvswitch,
> > add the openflow flows and then start packet processing.
> >
> > To do this, we look for the other_config:flow-restore-wait column
> > in the Open_vSwitch table during startup. If set as true, we disable
> > receiving packets from the datapath, expiring or flushing flows and
> > running any periodic ofproto activities. This option does not prevent
> > the addition and deletion of ports. Once this option is set to false,
> > we return to normal processing.
> >
> > An upcoming commit will use this feature in Open vSwitch startup scripts.
>
> Will this patch leave ongoing traffic not disturbed while upgrading OVS?
>
> Yes. That is one intent for just a "restart --save-flows=yes". The other
> intent is to not setup new flows till the userspace flows are restored. For
> force-reload-kmod (when kernel module is replaced), the intent is not to
> setup new flows without having all the userspace flows replaced.
>
>
> I am asking since I notice that the existing ports, which keep packet
> flowing, in the fast datapath (i.e., kernel) will be garbage collected in
> open_dpif_backer.
>
> I don't think that is true. The goal is to keep the datapath's ports to be
> the same as seen in OVSDB. A OVS upgrade can result in either just a
> "restart", in which case only userspace is restarted or a
> 'force-reload-kmod', in which case the kernel module is replaced too (in
> this case there will be traffic disruption for a few seconds). In both
> cases the expectation is that the kernel will have the ports for the
> interfaces in OVSDB. If not, we will add it.
>
> Yes, I took another look of the codes and you are right, existing ports
> will be preserved.
>
> One more question, there are two flavors of OVS restart by sending either
> TERM signal or KILL signal (refer to stop_daemon() in ovs-lib). For the
> TERM signal case, it seems would trigger a graceful shutdown of
> ovs-vswitchd and thereby all kernel flows and vports would be deleted. If
> it is the case, existing traffic would be impacted.
>

No. It does not delete kernel flows or delete ports in either cases.

>
> Tahnks,
> Guru
>
>
>
>
>
>
> Best,
> Jing
>
> >
> > Bug #16086.
> > Signed-off-by: Gurucharan Shetty <gshetty at nicira.com>
> > ---
> > ofproto/ofproto-dpif.c | 55
> +++++++++++++++++++++++++++++++++++++++++++++---
> > vswitchd/bridge.c | 11 ++++++++++
> > vswitchd/vswitch.xml | 17 +++++++++++++++
> > 3 files changed, 80 insertions(+), 3 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index c4f7d25..31f04f3 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -72,6 +72,8 @@ COVERAGE_DEFINE(facet_suppress);
> > * flow translation. */
> > #define MAX_RESUBMIT_RECURSION 64
> >
> > +extern bool flow_restore_wait;
> > +
> > /* Number of implemented OpenFlow tables. */
> > enum { N_TABLES = 255 };
> > enum { TBL_INTERNAL = N_TABLES - 1 }; /* Used for internal hidden rules.
> */
> > @@ -671,6 +673,7 @@ struct dpif_backer {
> > struct tag_set revalidate_set; /* Revalidate only matching facets. */
> >
> > struct hmap drop_keys; /* Set of dropped odp keys. */
> > + bool recv_set_enable; /* Enables or disables receiving packets. */
> > };
> >
> > /* All existing ofproto_backer instances, indexed by ofproto->up.type. */
> > @@ -947,6 +950,21 @@ type_run(const char *type)
> > push_all_stats();
> > }
> >
> > + /* If vswitchd started with other_config:flow_restore_wait set as
> "true",
> > + * and the configuration has now changed to "false", enable receiving
> > + * packets from the datapath. */
> > + if (!backer->recv_set_enable && !flow_restore_wait) {
> > + backer->recv_set_enable = true;
> > +
> > + error = dpif_recv_set(backer->dpif, backer->recv_set_enable);
> > + if (error) {
> > + VLOG_ERR("Failed to enable receiving packets in dpif.");
> > + return error;
> > + }
> > + dpif_flow_flush(backer->dpif);
> > + backer->need_revalidate = REV_RECONFIGURE;
> > + }
> > +
> > if (backer->need_revalidate
> > || !tag_set_is_empty(&backer->revalidate_set)) {
> > struct tag_set revalidate_set = backer->revalidate_set;
> > @@ -1040,7 +1058,10 @@ type_run(const char *type)
> > }
> > }
> >
> > - if (timer_expired(&backer->next_expiration)) {
> > + if (!backer->recv_set_enable) {
> > + /* A minimum of 1000 ms delay. */
> > + timer_set_duration(&backer->next_expiration, 1000);
> > + } else if (timer_expired(&backer->next_expiration)) {
> > int delay = expire(backer);
> > timer_set_duration(&backer->next_expiration, delay);
> > }
> > @@ -1106,6 +1127,11 @@ dpif_backer_run_fast(struct dpif_backer *backer,
> int max_batch)
> > {
> > unsigned int work;
> >
> > + /* If recv_set_enable is false, we should not handle upcalls. */
> > + if (!backer->recv_set_enable) {
> > + return 0;
> > + }
> > +
> > /* Handle one or more batches of upcalls, until there's nothing left to
> do
> > * or until we do a fixed total amount of work.
> > *
> > @@ -1305,9 +1331,16 @@ open_dpif_backer(const char *type, struct
> dpif_backer **backerp)
> > backer->need_revalidate = 0;
> > simap_init(&backer->tnl_backers);
> > tag_set_init(&backer->revalidate_set);
> > + if (flow_restore_wait) {
> > + backer->recv_set_enable = false;
> > + } else {
> > + backer->recv_set_enable = true;
> > + }
> > *backerp = backer;
> >
> > - dpif_flow_flush(backer->dpif);
> > + if (backer->recv_set_enable) {
> > + dpif_flow_flush(backer->dpif);
> > + }
> >
> > /* Loop through the ports already on the datapath and remove any
> > * that we don't need anymore. */
> > @@ -1331,7 +1364,7 @@ open_dpif_backer(const char *type, struct
> dpif_backer **backerp)
> >
> > shash_add(&all_dpif_backers, type, backer);
> >
> > - error = dpif_recv_set(backer->dpif, true);
> > + error = dpif_recv_set(backer->dpif, backer->recv_set_enable);
> > if (error) {
> > VLOG_ERR("failed to listen on datapath of type %s: %s",
> > type, strerror(error));
> > @@ -1569,6 +1602,12 @@ run_fast(struct ofproto *ofproto_)
> > struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> > struct ofport_dpif *ofport;
> >
> > + /* Do not perform any periodic activity required by 'ofproto' while
> > + * waiting for flow restore to complete. */
> > + if (flow_restore_wait) {
> > + return 0;
> > + }
> > +
> > HMAP_FOR_EACH (ofport, up.hmap_node, &ofproto->up.ports) {
> > port_run_fast(ofport);
> > }
> > @@ -1584,6 +1623,12 @@ run(struct ofproto *ofproto_)
> > struct ofbundle *bundle;
> > int error;
> >
> > + /* Do not perform any periodic activity required by 'ofproto' while
> > + * waiting for flow restore to complete. */
> > + if (flow_restore_wait) {
> > + return 0;
> > + }
> > +
> > if (!clogged) {
> > complete_operations(ofproto);
> > }
> > @@ -1658,6 +1703,10 @@ wait(struct ofproto *ofproto_)
> > struct ofport_dpif *ofport;
> > struct ofbundle *bundle;
> >
> > + if (flow_restore_wait) {
> > + return;
> > + }
> > +
> > if (!clogged && !list_is_empty(&ofproto->completions)) {
> > poll_immediate_wake();
> > }
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index e10036c..db70b77 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -183,6 +183,9 @@ static long long int iface_stats_timer = LLONG_MIN;
> > #define OFP_PORT_ACTION_WINDOW 10
> > static bool reconfiguring = false;
> >
> > +/* The default value of true waits for flow restore. */
> > +bool flow_restore_wait = true;
> > +
> > static void add_del_bridges(const struct ovsrec_open_vswitch *);
> > static void bridge_update_ofprotos(void);
> > static void bridge_create(const struct ovsrec_bridge *);
> > @@ -2321,6 +2324,14 @@ bridge_run(void)
> > * returns immediately. */
> > bridge_init_ofproto(cfg);
> >
> > + /* bridge_run(), when called for the first time will always have
> > + * flow_restore_wait as true. Once the value is false, we no longer
> > + * need to check its value. */
> > + if (flow_restore_wait) {
> > + flow_restore_wait = smap_get_bool(&cfg->other_config,
> > + "flow-restore-wait", false);
> > + }
> > +
> > /* Let each datapath type do the work that it needs to do. */
> > sset_init(&types);
> > ofproto_enumerate_types(&types);
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index 4396779..3896946 100644
> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > @@ -104,6 +104,23 @@
> > column or to <code>false</code> to explicitly disable it.
> > </column>
> >
> > + <column name="other_config" key="flow-restore-wait"
> > + type='{"type": "boolean"}'>
> > + <p>
> > + When <code>ovs-vswitchd</code> is started with this value set as
> > + <code>true</code>, it will neither flush or expire previously set
> > + datapath flows nor will it send and receive any packets to or from
> > + the datapath. When this value is later set to <code>false</code>,
> > + <code>ovs-vswitchd</code> will start receiving packets from the
> > + datapath and re-setup the flows.
> > + </p>
> > + <p>
> > + This option is only useful when used during Open vSwitch upgrades or
> > + restarts and flow restoration is needed before processing any new
> > + packets.
> > + </p>
> > + </column>
> > +
> > <column name="statistics" key="cpu"
> > type='{"type": "integer", "minInteger": 1}'>
> > <p>
> > --
> > 1.7.9.5
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130530/552d43e6/attachment-0003.html>