[ovs-dev] [PATCH 3/3] connmgr: Only send role status messages to OpenFlow 1.4+ controllers.

Justin Pettit jpettit at nicira.com
Sat Jul 26 00:07:58 UTC 2014


Acked-by: Justin Pettit <jpettit at nicira.com>


On Tue, Jul 22, 2014 at 3:58 PM, Ben Pfaff <blp at nicira.com> wrote:

> Only OpenFlow 1.4 and later support role status messages, but this code
> tried to send them to all controllers, which caused an assertion failure.
>
> Also, add tests to check that role status messages work, and that they
> don't cause trouble with OF1.2.
>
> Reported-by: Anup Khadka <khadka.py at gmail.com>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  AUTHORS           |    1 +
>  lib/ofp-util.c    |   27 ++++++----
>  ofproto/connmgr.c |    5 +-
>  tests/ofproto.at  |  147
> ++++++++++++++++++++++++++++++++++++++++++-----------
>  4 files changed, 139 insertions(+), 41 deletions(-)
>
> diff --git a/AUTHORS b/AUTHORS
> index 3e97bbe..b505a07 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -161,6 +161,7 @@ Andreas Beckmann        debian at abeckmann.de
>  Andrei Andone           andrei.andone at softvision.ro
>  Anshuman Manral         anshuman.manral at outlook.com
>  Anton Matsiuk           anton.matsiuk at gmail.com
> +Anup Khadka             khadka.py at gmail.com
>  Anuprem Chalvadi        achalvadi at vmware.com
>  Ariel Tubaltsev         atubaltsev at vmware.com
>  Atzm Watanabe           atzm at stratosphere.co.jp
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 912def9..a91031c 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -4977,22 +4977,31 @@ ofputil_encode_role_reply(const struct ofp_header
> *request,
>      return buf;
>  }
>
> +/* Encodes "role status" message 'status' for sending in the given
> + * 'protocol'.  Returns the role status message, if 'protocol' supports
> them,
> + * otherwise a null pointer. */
>  struct ofpbuf *
>  ofputil_encode_role_status(const struct ofputil_role_status *status,
>                             enum ofputil_protocol protocol)
>  {
> -    struct ofpbuf *buf;
>      enum ofp_version version;
> -    struct ofp14_role_status *rstatus;
>
>      version = ofputil_protocol_to_ofp_version(protocol);
> -    buf = ofpraw_alloc_xid(OFPRAW_OFPT14_ROLE_STATUS, version, htonl(0),
> 0);
> -    rstatus = ofpbuf_put_zeros(buf, sizeof *rstatus);
> -    rstatus->role = htonl(status->role);
> -    rstatus->reason = status->reason;
> -    rstatus->generation_id = htonll(status->generation_id);
> -
> -    return buf;
> +    if (version >= OFP14_VERSION) {
> +        struct ofp14_role_status *rstatus;
> +        struct ofpbuf *buf;
> +
> +        buf = ofpraw_alloc_xid(OFPRAW_OFPT14_ROLE_STATUS, version,
> htonl(0),
> +                               0);
> +        rstatus = ofpbuf_put_zeros(buf, sizeof *rstatus);
> +        rstatus->role = htonl(status->role);
> +        rstatus->reason = status->reason;
> +        rstatus->generation_id = htonll(status->generation_id);
> +
> +        return buf;
> +    } else {
> +        return NULL;
> +    }
>  }
>
>  enum ofperr
> diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
> index be99529..05fe0c4 100644
> --- a/ofproto/connmgr.c
> +++ b/ofproto/connmgr.c
> @@ -920,8 +920,9 @@ ofconn_send_role_status(struct ofconn *ofconn,
> uint32_t role, uint8_t reason)
>      ofconn_get_master_election_id(ofconn, &status.generation_id);
>
>      buf = ofputil_encode_role_status(&status,
> ofconn_get_protocol(ofconn));
> -
> -    ofconn_send(ofconn, buf, NULL);
> +    if (buf) {
> +        ofconn_send(ofconn, buf, NULL);
> +    }
>  }
>
>  /* Changes 'ofconn''s role to 'role'.  If 'role' is OFPCR12_ROLE_MASTER
> then
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 729ef7b..3a55ce3 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -1875,41 +1875,128 @@ dnl This test checks that the role
> request/response messaging works
>  dnl and that generation_id is handled properly.
>  AT_SETUP([ofproto - controller role (OpenFlow 1.2)])
>  OVS_VSWITCHD_START
> -AT_CHECK([ovs-ofctl -O OpenFlow12 monitor br0 --detach --no-chdir
> --pidfile])
> -
> -ovs-appctl -t ovs-ofctl ofctl/barrier
> -ovs-appctl -t ovs-ofctl ofctl/set-output-file monitor.log
> -: > expout
> -: > experr
> +ON_EXIT([kill `cat c1.pid c2.pid`])
> +
> +# Start two ovs-ofctl controller processes.
> +AT_CAPTURE_FILE([monitor1.log])
> +AT_CAPTURE_FILE([expout1])
> +AT_CAPTURE_FILE([experr1])
> +AT_CAPTURE_FILE([monitor2.log])
> +AT_CAPTURE_FILE([expout2])
> +AT_CAPTURE_FILE([experr2])
> +for i in 1 2; do
> +     AT_CHECK([ovs-ofctl -O OpenFlow12 monitor br0 --detach --no-chdir
> --pidfile=`pwd`/c$i.pid --unixctl=`pwd`/c$i])
> +    ovs-appctl -t `pwd`/c$i ofctl/barrier
> +    ovs-appctl -t `pwd`/c$i ofctl/set-output-file monitor$i.log
> +    : > expout$i
> +    : > experr$i
> +
> +    # find out current role
> +    ovs-appctl -t `pwd`/c$i ofctl/send
> 031800180000000200000000000000000000000000000000
> +    echo >>experr$i "send: OFPT_ROLE_REQUEST (OF1.2): role=nochange"
> +    echo >>expout$i "OFPT_ROLE_REPLY (OF1.2): role=equal"
> +done
>
> -# find out current role
> -ovs-appctl -t ovs-ofctl ofctl/send
> 031800180000000200000000000000000000000000000000
> -echo >>experr "send: OFPT_ROLE_REQUEST (OF1.2) (xid=0x2): role=nochange"
> -echo >>expout "OFPT_ROLE_REPLY (OF1.2) (xid=0x2): role=equal"
> +# controller 1: Become slave (generation_id is initially undefined, so
> +# 2^63+2 should not be stale)
> +ovs-appctl -t `pwd`/c1 ofctl/send
> 031800180000000300000003000000008000000000000002
> +echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.2): role=slave
> generation_id=9223372036854775810"
> +echo >>expout1 "OFPT_ROLE_REPLY (OF1.2): role=slave
> generation_id=9223372036854775810"
> +
> +# controller 2: Become master.
> +ovs-appctl -t `pwd`/c2 ofctl/send
> 031800180000000300000002000000008000000000000003
> +echo >>experr2 "send: OFPT_ROLE_REQUEST (OF1.2): role=master
> generation_id=9223372036854775811"
> +echo >>expout2 "OFPT_ROLE_REPLY (OF1.2): role=master
> generation_id=9223372036854775811"
> +
> +# controller 1: Try to become the master using a stale generation ID
> +ovs-appctl -t `pwd`/c1 ofctl/send
> 031800180000000400000002000000000000000000000003
> +echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.2): role=master
> generation_id=3"
> +echo >>expout1 "OFPT_ERROR (OF1.2): OFPRRFC_STALE"
> +echo >>expout1 "OFPT_ROLE_REQUEST (OF1.2): role=master generation_id=3"
> +
> +# controller 1: Become master using a valid generation ID
> +ovs-appctl -t `pwd`/c1 ofctl/send
> 031800180000000500000002000000000000000000000001
> +echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.2): role=master
> generation_id=1"
> +echo >>expout1 "OFPT_ROLE_REPLY (OF1.2): role=master generation_id=1"
> +
> +for i in 1 2; do
> +    ovs-appctl -t `pwd`/c$i ofctl/barrier
> +    echo >>expout$i "OFPT_BARRIER_REPLY (OF1.2):"
> +done
>
> -# Become slave (generation_id is initially undefined, so 2^63+2 should
> not be stale)
> -ovs-appctl -t ovs-ofctl ofctl/send
> 031800180000000300000003000000008000000000000002
> -echo >>experr "send: OFPT_ROLE_REQUEST (OF1.2) (xid=0x3): role=slave
> generation_id=9223372036854775810"
> -echo >>expout "OFPT_ROLE_REPLY (OF1.2) (xid=0x3): role=slave
> generation_id=9223372036854775810"
> +# Check output.
> +for i in 1 2; do
> +    cp expout$i expout
> +    AT_CHECK([grep -v '^send:' monitor$i.log | STRIP_XIDS], [0], [expout])
> +    cp experr$i expout
> +    AT_CHECK([grep '^send:' monitor$i.log | STRIP_XIDS], [0], [expout])
> +done
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
>
> -# Try to become the master using a stale generation ID
> -ovs-appctl -t ovs-ofctl ofctl/send
> 031800180000000400000002000000000000000000000002
> -echo >>experr "send: OFPT_ROLE_REQUEST (OF1.2) (xid=0x4): role=master
> generation_id=2"
> -echo >>expout "OFPT_ERROR (OF1.2) (xid=0x4): OFPRRFC_STALE"
> -echo >>expout "OFPT_ROLE_REQUEST (OF1.2) (xid=0x4): role=master
> generation_id=2"
> -
> -# Become master using a valid generation ID
> -ovs-appctl -t ovs-ofctl ofctl/send
> 031800180000000500000002000000000000000000000001
> -echo >>experr "send: OFPT_ROLE_REQUEST (OF1.2) (xid=0x5): role=master
> generation_id=1"
> -echo >>expout "OFPT_ROLE_REPLY (OF1.2) (xid=0x5): role=master
> generation_id=1"
> -ovs-appctl -t ovs-ofctl ofctl/barrier
> -echo >>expout "OFPT_BARRIER_REPLY (OF1.2) (xid=0x3):"
> +dnl This test checks that the role request/response messaging works,
> +dnl that generation_id is handled properly, and that role status update
> +dnl messages are sent when a controller's role gets changed from master
> +dnl to slave.
> +AT_SETUP([ofproto - controller role (OpenFlow 1.4)])
> +OVS_VSWITCHD_START
> +ON_EXIT([kill `cat c1.pid c2.pid`])
> +
> +# Start two ovs-ofctl controller processes.
> +AT_CAPTURE_FILE([monitor1.log])
> +AT_CAPTURE_FILE([expout1])
> +AT_CAPTURE_FILE([experr1])
> +AT_CAPTURE_FILE([monitor2.log])
> +AT_CAPTURE_FILE([expout2])
> +AT_CAPTURE_FILE([experr2])
> +for i in 1 2; do
> +     AT_CHECK([ovs-ofctl -O OpenFlow14 monitor br0 --detach --no-chdir
> --pidfile=`pwd`/c$i.pid --unixctl=`pwd`/c$i])
> +    ovs-appctl -t `pwd`/c$i ofctl/barrier
> +    ovs-appctl -t `pwd`/c$i ofctl/set-output-file monitor$i.log
> +    : > expout$i
> +    : > experr$i
> +
> +    # find out current role
> +    ovs-appctl -t `pwd`/c$i ofctl/send
> 051800180000000200000000000000000000000000000000
> +    echo >>experr$i "send: OFPT_ROLE_REQUEST (OF1.4): role=nochange"
> +    echo >>expout$i "OFPT_ROLE_REPLY (OF1.4): role=equal"
> +done
>
> -AT_CHECK([grep -v '^send:' monitor.log], [0], [expout])
> -mv experr expout
> -AT_CHECK([grep '^send:' monitor.log], [0], [expout])
> +# controller 1: Become slave (generation_id is initially undefined, so
> +# 2^63+2 should not be stale)
> +ovs-appctl -t `pwd`/c1 ofctl/send
> 051800180000000300000003000000008000000000000002
> +echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.4): role=slave
> generation_id=9223372036854775810"
> +echo >>expout1 "OFPT_ROLE_REPLY (OF1.4): role=slave
> generation_id=9223372036854775810"
> +
> +# controller 2: Become master.
> +ovs-appctl -t `pwd`/c2 ofctl/send
> 051800180000000300000002000000008000000000000003
> +echo >>experr2 "send: OFPT_ROLE_REQUEST (OF1.4): role=master
> generation_id=9223372036854775811"
> +echo >>expout2 "OFPT_ROLE_REPLY (OF1.4): role=master
> generation_id=9223372036854775811"
> +
> +# controller 1: Try to become the master using a stale generation ID
> +ovs-appctl -t `pwd`/c1 ofctl/send
> 051800180000000400000002000000000000000000000003
> +echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.4): role=master
> generation_id=3"
> +echo >>expout1 "OFPT_ERROR (OF1.4): OFPRRFC_STALE"
> +echo >>expout1 "OFPT_ROLE_REQUEST (OF1.4): role=master generation_id=3"
> +
> +# controller 1: Become master using a valid generation ID
> +ovs-appctl -t `pwd`/c1 ofctl/send
> 051800180000000500000002000000000000000000000001
> +echo >>experr1 "send: OFPT_ROLE_REQUEST (OF1.4): role=master
> generation_id=1"
> +echo >>expout1 "OFPT_ROLE_REPLY (OF1.4): role=master generation_id=1"
> +echo >>expout2 "OFPT_ROLE_STATUS (OF1.4): role=slave generation_id=1
> reason=master_request"
> +
> +for i in 1 2; do
> +    ovs-appctl -t `pwd`/c$i ofctl/barrier
> +    echo >>expout$i "OFPT_BARRIER_REPLY (OF1.4):"
> +done
>
> -ovs-appctl -t ovs-ofctl exit
> +# Check output.
> +for i in 1 2; do
> +    cp expout$i expout
> +    AT_CHECK([grep -v '^send:' monitor$i.log | STRIP_XIDS], [0], [expout])
> +    cp experr$i expout
> +    AT_CHECK([grep '^send:' monitor$i.log | STRIP_XIDS], [0], [expout])
> +done
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>



More information about the dev mailing list