[ovs-dev] [PATCH 13/14] Add support for multiple OpenFlow controllers on a single bridge.
Justin Pettit
jpettit at nicira.com
Tue Apr 20 07:46:49 UTC 2010
On Apr 8, 2010, at 5:07 PM, Ben Pfaff wrote:
> diff --git a/ofproto/fail-open.c b/ofproto/fail-open.c
> index eca1417..c3ff897 100644
> --- a/ofproto/fail-open.c
> +++ b/ofproto/fail-open.c
> @@ -68,8 +68,8 @@
>
> +/* Returns the number of seconds for which all controllers have been
> + * disconnected. */
> +static int
> +failure_duration(const struct fail_open *fo)
> +{
> + int min_failure_duration;
> + size_t i;
> +
> + min_failure_duration = 0;
> + for (i = 0; i < fo->n_controllers; i++) {
> + int failure_duration = rconn_failure_duration(fo->controllers[i]);
> + min_failure_duration = MAX(min_failure_duration, failure_duration);
Should this be MIN?
> diff --git a/ofproto/fail-open.h b/ofproto/fail-open.h
> index 900d587..c4b8716 100644
> --- a/ofproto/fail-open.h
> +++ b/ofproto/fail-open.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009 Nicira Networks.
> + * Copyright (c) 2008, 2009, 2010 Nicira Networks.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -32,10 +32,8 @@ struct switch_status;
> * creates flows with this priority). */
> #define FAIL_OPEN_PRIORITY 70000
No special value here like 0xf0f0f0f0? :-)
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 10d8161..00ebfe2 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
>
> +/* ofproto supports two kinds of OpenFlow connections:
> + *
> + * - "Controller connections": Connections to ordinary OpenFlow controllers.
> + * ofproto maintains persistent connections to these controllers and by
> + * default sends them asynchronous messages such as packet-ins.
> + *
> + * - "Management connections", e.g. from ovs-ofctl. When these connections
> + * drop, it is the other side's responsibility to reconnect them if
> + * necessary. ofproto does not send them asynchronous messages by default.
> + */
> +enum ofconn_type {
> + OFCONN_CONTROLLER, /* An OpenFlow controller. */
> + OFCONN_MANAGEMENT /* A (transient) management connection. */
> +};
I don't have a better proposal at the moment, but this is going to get confusing with the management/config protocol.
> +static void
> +add_controller(struct ofproto *ofproto, const struct ofproto_controller *c)
>
> + ofconn = ofconn_create(ofproto, rconn_create(5, 8), OFCONN_CONTROLLER);
It might be good to write a comment that the probe_interval and max_backoff will be set by update_controller() later. Otherwise, these magic numbers are a bit confusing. Actually, it may just be a good idea to mention at the function-level description that update_controller() needs to be called after add_controller().
> @@ -847,7 +942,19 @@ ofproto_run1(struct ofproto *p)
>
> + LIST_FOR_EACH (ofconn, struct ofconn, node, &p->all_conns) {
> + if (ofconn->type == OFCONN_CONTROLLER) {
> + rconn_add_monitor(ofconn->rconn, vconn);
> + goto success;
> + }
> + }
> + VLOG_INFO_RL(&rl, "no controller connection to monitor");
> + vconn_close(vconn);
> +
> + success:;
As I mentioned in a previous patch, this just looks weird to me. :-)
> @@ -920,14 +1027,9 @@ ofproto_wait(struct ofproto *p)
> if (p->in_band) {
> in_band_wait(p->in_band);
> }
> - if (p->discovery) {
> - discovery_wait(p->discovery);
> - }
> if (p->fail_open) {
> fail_open_wait(p->fail_open);
> }
> - pinsched_wait(p->miss_sched);
> - pinsched_wait(p->action_sched);
> if (p->sflow) {
> ofproto_sflow_wait(p->sflow);
> }
I'm sure I missed some subtlety, but why don't we need to call pinsched_wait() and discovery_wait() anymore?
> static void
> +send_packet_in(struct ofproto *ofproto, struct ofpbuf *packet)
>
> + LIST_FOR_EACH (ofconn, struct ofconn, node, &ofproto->all_conns) {
> + if (ofconn->miss_send_len
> + || (msg->type == _ODPL_ACTION_NR
> + && ofconn->type == OFCONN_CONTROLLER)) {
Looking through the OpenFlow 1.0 spec, I believe that we are supposed to send a packet_in for misses even if the miss_send_len is zero...it's just supposed to be of zero length.
> diff --git a/utilities/ovs-openflowd.c b/utilities/ovs-openflowd.c
>
> @@ -266,16 +267,18 @@ parse_options(int argc, char *argv[], struct ofsettings *s)
>
> + controller_opts.probe_interval = 0;
As mentioned in an earlier patch, I think this is supposed to have a default of 5. It looks to me like this will cause probes to be turned off by default based on the code in ofproto.c.
> case OPT_FAIL_MODE:
> if (!strcmp(optarg, "open")) {
> - s->controller.fail = OFPROTO_FAIL_STANDALONE;
> + controller_opts.fail = OFPROTO_FAIL_STANDALONE;
> } else if (!strcmp(optarg, "closed")) {
> - s->controller.fail = OFPROTO_FAIL_SECURE;
> + controller_opts.fail = OFPROTO_FAIL_SECURE;
Should we transition to using "standalone" and "secure" on the command line? We could continue to use "open" and "closed" as undocumented options for a while. "open" and "closed" have always been confusing.
> diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
>
> +For each of these commands, a \fIbridge\fR of \fBdefault\fR applies
> +the configuration as the default for any bridge that has not been
> +explicitly configured. Otherwise, \fIbridge\fR must name a bridge,
> +and the settings apply only to that bridge. (Omitting \fIbridge\fR
> +entirely usually has the same effect as specifying \fBdefault\fR.)
I think it would be clearer if we always required a "bridge" argument. I feel like a lot of people get confused about where the controller setting has been configured as it is.
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
>
> +static const char *
> +get_fail_mode(struct ovsrec_controller **controllers, size_t n_controllers)
> +{
> + const char *fail_mode;
> + size_t i;
> +
> + fail_mode = NULL;
> + for (i = 0; i < n_controllers; i++) {
> + const char *s = controllers[i]->fail_mode;
> + if (s) {
> + if (!strcmp(s, "secure")) {
> + return s;
I'm not sure it's documented that "secure" on any bridge trumps any other setting. Also, does it make sense to have the fail-mode setting attached to the controller as opposed to the bridge, anyway?
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
>
> @@ -1587,8 +1541,74 @@ bridge_reconfigure_controller(const struct ovsrec_open_vswitch
> + if (!c->local_netmask || !inet_aton(c->local_netmask, &mask)) {
This line doesn't fit in 80 columns.
> + oc->band = (!c->connection_mode
> + || !strcmp(c->connection_mode, "out-of-band")
> + ? OFPROTO_IN_BAND
> + : OFPROTO_OUT_OF_BAND);
I think the string comparison should be "in-band".
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index d3f3efb..ad1263d 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -16,7 +16,7 @@
> </column>
>
> <column name="controller">
> - Default <ref table="Controller"/> used by bridges. May be
> + Default OpenFlow <ref table="Controller"/> set used by bridges. May be
> overridden on a per-bridge basis by the <ref table="Bridge"
> column="controller"/> column in <ref table="Bridge"/>.
> </column>
> @@ -104,10 +104,11 @@
>
> <group title="OpenFlow Configuration">
> <column name="controller">
> - OpenFlow controller. If unset, defaults to that specified by
> - <ref column="controller" table="Open_vSwitch"/> in the
> - <ref table="Open_vSwitch"/> table. If the default is also unset, then
> - no OpenFlow controller will be used.
> + OpenFlow controller set. If unset, defaults to the set of
> + controllers specified by <ref column="controller"
> + table="Open_vSwitch"/> in the <ref table="Open_vSwitch"/>
> + table. If the default is also unset, then no OpenFlow
> + controllers will be used.
> </column>
Do you think it's worth explicitly mentioning that there's no merging of controllers between Bridge and Open_vSwitch tables (i.e., that entries in a Bridge trump any in the Open_vSwitch table)?
> connecting to the controller forever.</dd>
> </dl>
> </p>
> - <p>If this value is unset, the default is
> - implementation-specific.</p>
> + <p>If this value is unset, the default is implementation-specific.</p>
> + <p>When more than one controller is configured,
> + <ref column="fail_mode"/> is considered only when none of the
> + configured controllers can be contacted. At that point, the bridge
> + enters secure mode if any of the controllers'
> + <ref column="fail_mode"/> is set to <code>secure</code>. Otherwise,
> + it enters standalone mode if at least one <ref column="fail_mode"/>
> + is set to <code>standalone</code>. If none of the
> + <ref column="fail_mode"/> values are set, the default is
> + implementation-defined.</p>
Should we point out where these sorts of implementation-defined things can be located?
> + <column name="local_gateway">
> + The IP address of the gateway to configure on the local port, as a
> + string, e.g. <code>192.168.0.1</code>. Leave this column unset if
> + this network has no gateway.
Did you want to mention that it's ignored if target is "discover"? This was done for the "local_ip" and "local_netmask" columns.
--Justin
More information about the dev
mailing list