On Thu, May 30, 2013 at 1:36 PM, Ben Pfaff <b...@nicira.com> wrote:
> 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.
>
> I was intending to say that don't wake up immediately because of me. I can
wait till 1000 ms.
Previously expire() would setup a MIN(x, 1000). Since I do not have 'x'
now, I set 1000 ms.
I think we both mean the same thing? How would the following sound?
/* Wake up before a max of 1000ms. */
> > + 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:
>
Okay.
> > @@ -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:
>
Okay.
> > @@ -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:
>
Okay.
I will send a v4.
>
> > + 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
