[ovs-dev] [PATCH v3 1/4] ovs-vswitchd: An option to wait for userspace flow restore to complete.

Ben Pfaff blp at nicira.com
Thu May 30 20:36:16 UTC 2013


On Thu, May 30, 2013 at 09:19:35AM +0000, Gurucharan Shetty 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.
> 
> Bug #16086.
> Signed-off-by: Gurucharan Shetty <gshetty at nicira.com>

Thanks!  I think that this is close.  Sorry to nitpick, but I still
have some comments.

> @@ -1025,7 +1041,10 @@ type_run(const char *type)
>          }
>      }
>  
> -    if (timer_expired(&backer->next_expiration)) {
> +    if (!backer->recv_set_enable) {
> +        /* A minimum of 1000 ms delay. */

Doesn't think actually cause the main loop to wake up after at most
1000 ms, that is, a *maximum* of 1000 ms?  Or perhaps I misunderstand
the intent.

> +        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);
>      }

Here, in run(), I think that we should always run
complete_operations(), because this relates to updating the OpenFlow
flow table.  It looks OK to me to skip the rest:
> @@ -1567,6 +1600,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 (ofproto_get_flow_restore_wait()) {
> +        return 0;
> +    }
> +
>      if (!clogged) {
>          complete_operations(ofproto);
>      }

Similarly, here in wait(), I would suggest we move
ofproto_get_flow_restore_wait() below the !clogged check:
> @@ -1641,6 +1680,10 @@ wait(struct ofproto *ofproto_)
>      struct ofport_dpif *ofport;
>      struct ofbundle *bundle;
>  
> +    if (ofproto_get_flow_restore_wait()) {
> +        return;
> +    }
> +
>      if (!clogged && !list_is_empty(&ofproto->completions)) {
>          poll_immediate_wake();
>      }

> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 47b13d2..d5df6fa 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -71,6 +71,50 @@
>          The Citrix XenServer universally unique identifier for the physical
>          host as displayed by <code>xe host-list</code>.
>        </column>
> +
> +      <column name="other_config" key="flow-restore-wait"
> +              type='{"type": "boolean"}'>

I think we should have <p>...</p> around these paragraphs to make the
output easier to read:

> +        When <code>ovs-vswitchd</code> starts up, it has an empty flow table
> +        and therefore it handles all arriving packets in its default fashion
> +        according to its configuration, by dropping them or sending them to
> +        an OpenFlow controller or switching them as a standalone switch.
> +        This behavior is ordinarily desirable.  However, if
> +        <code>ovs-vswitchd</code> is restarting as part of a
> +        ``hot-upgrade,'' then this leads to a relatively long period during
> +        which packets are mishandled.
> +
> +        This option allows for improvement.  When <code>ovs-vswitchd</code>
> +        starts 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.
> +
> +        Thus, with this option, the procedure for a hot-upgrade of
> +        <code>ovs-vswitchd</code> becomes roughly the following:

Thanks,

Ben.



More information about the dev mailing list