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 <gshe...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
