[ovs-dev] [PATCH v2 1/3] ovs-vswitchd: An option to wait for userspace > flow restore to complete. (Gurucharan Shetty)
Jing Ai
ai_jing2000 at hotmail.com
Thu May 30 21:33:01 UTC 2013
Date: Thu, 30 May 2013 08:12:16 -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 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. You are right. I just found we trap the SIGTERM and use "ovs-appctl -t daemon exit" in our config. This is why I said all kernel datapath config was gone. Thanks a lot!
Best,Jing
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/ac9bc8eb/attachment-0003.html>
More information about the dev
mailing list