[ovs-dev] [PATCH 14/14] ofproto: Add support for master/slave controller coordination.

[Ben Pfaff]

On Tue, Apr 20, 2010 at 12:51:48AM -0700, Justin Pettit wrote:
> On Apr 8, 2010, at 5:07 PM, Ben Pfaff wrote:
> 
> > @@ -54,6 +59,34 @@ struct nicira_header {
> > };
> > OFP_ASSERT(sizeof(struct nicira_header) == 16);
> > 
> > +/* Configures the "role" of the sending controller:
[...]
> On my first read through this, it wasn't really clear to me the
> difference between "Master" and "Other".  By reading through the
> source, I now know that "Master" and "Slave" are essentially a
> pairing.  And "Other" is completely outside of their interactions.  I
> think it might be good to be a little more explicit.

Sure.  I changed the comment to this:

/* Configures the "role" of the sending controller.  The default role is:
 *
 *    - Other (NX_ROLE_OTHER), which allows the controller access to all
 *      OpenFlow features.
 *
 * The other possible roles are a related pair:
 *
 *    - Master (NX_ROLE_MASTER) is equivalent to Other, except that there may
 *      be at most one Master controller at a time: when a controller
 *      configures itself as Master, any existing Master is demoted to the
 *      Slave role.
 *
 *    - Slave (NX_ROLE_SLAVE) allows the controller read-only access to
 *      OpenFlow features.  In particular attempts to modify the flow table
 *      will be rejected with an OFPBRC_EPERM error.
 *
 *      Slave controllers also do not receive asynchronous messages
 *      (OFPT_PACKET_IN, OFPT_FLOW_REMOVED, OFPT_PORT_STATUS).
 */

> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index 00ebfe2..8de3183 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > 
> > @@ -2050,7 +2056,7 @@ handle_set_config(struct ofproto *p, struct ofconn *ofconn,
> >     }
> >     flags = ntohs(osc->flags);
> > 
> > -    if (ofconn->type == OFCONN_CONTROLLER) {
> > +    if (ofconn->type == OFCONN_CONTROLLER && ofconn->role != NX_ROLE_SLAVE) {
> 
> Is there a reason you didn't call reject_slave_controller() to
> generate an EPERM in this case?

It's because I've always had a notion that some of the attributes set by
OFPT_SET_CONFIG, like miss_send_len, were per-connection instead of
global.  I guess that none of the current set of attributes really
affects a slave controller, but I don't want to rule out that
possibility for future attributes.  (And it does seem reasonable for a
slave controller to set its miss_send_len, even if the value doesn't
matter until it switches itself back to the Master role.)

Thank you for your comments!