| Message ID | 5629E4FA.8050300@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Oct 23, 2015 at 01:12:50PM +0530, Babu Shanmugam wrote: > This patch configures the ovs records for a new controller to br-int > and an endpoint for the new controller > > Signed-off-by: Babu Shanmugam <bschanmu@redhat.com> Doesn't it work to just start a second connection to the controller, in the same way that ofctrl connects to the controller?
Hi Ben, I understand that when an openflow controller is set on the switch, the switch will try to connect to the controller target and keeps alive the connection by a series of echo_requests and echo_replies. So, I thought, we need a passive connection that listens on the target that is configured for the controller and respond to the requests from the switch. I also thought this will help in responding to the packet-in requests as soon as it arrives. I hope my understanding is correct, if not, please correct me. Thank you, Babu On Sunday 25 October 2015 05:02 AM, Ben Pfaff wrote: > On Fri, Oct 23, 2015 at 01:12:50PM +0530, Babu Shanmugam wrote: >> This patch configures the ovs records for a new controller to br-int >> and an endpoint for the new controller >> >> Signed-off-by: Babu Shanmugam <bschanmu@redhat.com> > Doesn't it work to just start a second connection to the controller, in > the same way that ofctrl connects to the controller?
Sorry it's taken me so long to get back to this.
It works pretty much the same regardless of the direction of the
connection. The only important differences are:
* When you set up a Controller record, there can only be one
outgoing connection to a given controller, which means that
we'd be limited to a single OpenFlow channel for this purpose.
That might be OK for now but I suspect that we'll want to have
multiple connections eventually for different purposes or for
fairness or load balancing.
On the other hand, ovn-controller can make as many connections
to OVS as it likes.
* If ovn-controller connects to OVS, it needs to send a specific
message to turn on "packet-in"s, instead of getting them by
default when the connection completes.
There's no real difference regarding keeping the connection up; in
either case that's easy to handle.
It seems a little cleaner to avoid setting up the Controller record,
since there's nothing to clean up on exit in that case and nothing that
an administrator can screw up by modifying the database.
On Wed, Oct 28, 2015 at 05:32:27AM +0530, Babu Shanmugam wrote:
> Hi Ben,
> I understand that when an openflow controller is set on the switch, the
> switch will try to connect to the controller target and keeps alive the
> connection by a series of echo_requests and echo_replies. So, I thought, we
> need a passive connection that listens on the target that is configured for
> the controller and respond to the requests from the switch.
> I also thought this will help in responding to the packet-in requests as
> soon as it arrives. I hope my understanding is correct, if not, please
> correct me.
>
> Thank you,
> Babu
>
> On Sunday 25 October 2015 05:02 AM, Ben Pfaff wrote:
> >On Fri, Oct 23, 2015 at 01:12:50PM +0530, Babu Shanmugam wrote:
> >>This patch configures the ovs records for a new controller to br-int
> >>and an endpoint for the new controller
> >>
> >>Signed-off-by: Babu Shanmugam <bschanmu@redhat.com>
> >Doesn't it work to just start a second connection to the controller, in
> >the same way that ofctrl connects to the controller?
>
On Fri, Oct 23, 2015 at 01:12:50PM +0530, Babu Shanmugam wrote: > This patch configures the ovs records for a new controller to br-int > and an endpoint for the new controller > > Signed-off-by: Babu Shanmugam <bschanmu@redhat.com> This patch is wordwrapped so it's difficult to fully review the series. I'll try to make some comments anyway but my normal process involves applying and compiling patches.
Thank you for the review Ben. On Wednesday 11 November 2015 06:06 AM, Ben Pfaff wrote: > Sorry it's taken me so long to get back to this. > > It works pretty much the same regardless of the direction of the > connection. The only important differences are: > > * When you set up a Controller record, there can only be one > outgoing connection to a given controller, which means that > we'd be limited to a single OpenFlow channel for this purpose. > That might be OK for now but I suspect that we'll want to have > multiple connections eventually for different purposes or for > fairness or load balancing. I agree to this Ben, and I am working on it. > On the other hand, ovn-controller can make as many connections > to OVS as it likes. > > * If ovn-controller connects to OVS, it needs to send a specific > message to turn on "packet-in"s, instead of getting them by > default when the connection completes. Done! > > There's no real difference regarding keeping the connection up; in > either case that's easy to handle. > > It seems a little cleaner to avoid setting up the Controller record, > since there's nothing to clean up on exit in that case and nothing that > an administrator can screw up by modifying the database. I agree. > > On Wed, Oct 28, 2015 at 05:32:27AM +0530, Babu Shanmugam wrote: >> Hi Ben, >> I understand that when an openflow controller is set on the switch, the >> switch will try to connect to the controller target and keeps alive the >> connection by a series of echo_requests and echo_replies. So, I thought, we >> need a passive connection that listens on the target that is configured for >> the controller and respond to the requests from the switch. >> I also thought this will help in responding to the packet-in requests as >> soon as it arrives. I hope my understanding is correct, if not, please >> correct me. >> >> Thank you, >> Babu >> >> On Sunday 25 October 2015 05:02 AM, Ben Pfaff wrote: >>> On Fri, Oct 23, 2015 at 01:12:50PM +0530, Babu Shanmugam wrote: >>>> This patch configures the ovs records for a new controller to br-int >>>> and an endpoint for the new controller >>>> >>>> Signed-off-by: Babu Shanmugam <bschanmu@redhat.com> >>> Doesn't it work to just start a second connection to the controller, in >>> the same way that ofctrl connects to the controller?
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c index 3f29b25..343de28 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -21,6 +21,7 @@ #include <getopt.h> #include <signal.h> #include <stdlib.h> +#include <unistd.h> #include <string.h> #include "command-line.h" @@ -56,6 +57,7 @@ static unixctl_cb_func ct_zone_list; #define DEFAULT_BRIDGE_NAME "br-int" static void parse_options(int argc, char *argv[]); +static char const *get_switch_controller_path(void); OVS_NO_RETURN static void usage(void); static char *ovs_remote; @@ -100,10 +102,58 @@ get_bridge(struct ovsdb_idl *ovs_idl, const char *br_name) return NULL; } +static char const * +get_switch_controller_path(void) +{ + static char *path = NULL; + + if (!path) { + path = xasprintf("%s/%s.%ld.sock", + ovs_rundir(), "ovn-controller", (long int) getpid()); + } + return path; +} + +static void +ovs_bridge_set_controller(struct controller_ctx *ctx, + struct ovsrec_bridge const *br) +{ + struct ovsrec_controller *controller; + struct ovsdb_datum const *bctrl, *target; + bool set = false; + static char *proto = NULL; + + if (!proto) { + proto = xasprintf("unix:%s", get_switch_controller_path()); + } + + if (!br || !ctx || !ctx->ovs_idl_txn || !ctx->ovs_idl) { + return; + } + + bctrl = ovsrec_bridge_get_controller(br, OVSDB_TYPE_UUID); + if (bctrl && bctrl->n > 0) { + struct ovsrec_controller const *ctrler; + + ctrler = + ovsrec_controller_get_for_uuid(ctx->ovs_idl, &bctrl->keys[0].uuid); + target = ovsrec_controller_get_target(ctrler, OVSDB_TYPE_STRING); + if (strcmp(target->keys[0].string, proto) != 0) { + set = true; + } + } else { + set = true; + } + if (set) { + controller = ovsrec_controller_insert(ctx->ovs_idl_txn); + ovsrec_controller_set_target(controller, proto); + ovsrec_bridge_set_controller(br, &controller, 1); + } +} + static const struct ovsrec_bridge * create_br_int(struct controller_ctx *ctx, - const struct ovsrec_open_vswitch *cfg, - const char *bridge_name) + const struct ovsrec_open_vswitch *cfg, const char
This patch configures the ovs records for a new controller to br-int and an endpoint for the new controller Signed-off-by: Babu Shanmugam <bschanmu@redhat.com> --- ovn/controller/ovn-controller.c | 62 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 3 deletions(-) *bridge_name) { if (!ctx->ovs_idl_txn) { return NULL; @@ -158,8 +208,9 @@ get_br_int(struct controller_ctx *ctx) const struct ovsrec_bridge *br; br = get_bridge(ctx->ovs_idl, br_int_name); if (!br) { - return create_br_int(ctx, cfg, br_int_name); + br = create_br_int(ctx, cfg, br_int_name); } + ovs_bridge_set_controller(ctx, br); return br; } @@ -246,6 +297,11 @@ main(int argc, char *argv[]) ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_bridge_col_name); ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_bridge_col_fail_mode); ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_bridge_col_other_config); + ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_bridge_col_controller); + ovsdb_idl_add_table(ovs_idl_loop.idl, &ovsrec_table_controller); + ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_controller_col_target); + ovsdb_idl_add_column(ovs_idl_loop.idl, + &ovsrec_controller_col_is_connected); chassis_register_ovs_idl(ovs_idl_loop.idl); encaps_register_ovs_idl(ovs_idl_loop.idl); binding_register_ovs_idl(ovs_idl_loop.idl);