On Wed, May 30, 2018 at 2:52 PM, Han Zhou <zhou...@gmail.com> wrote:
>
>
> On Tue, May 29, 2018 at 11:40 PM, Numan Siddique <nusid...@redhat.com>
> wrote:
> >
> > On Wed, May 30, 2018 at 5:31 AM, aginwala <aginw...@asu.edu> wrote:
> >
> > > Thanks for the Review.
> > >
> > > On Wed, May 23, 2018 at 11:36 AM, Numan Siddique <nusid...@redhat.com>
> > > wrote:
> > >
> > >> On Wed, May 9, 2018 at 12:13 AM, aginwala <amgin...@gmail.com> wrote:
> > >>
> > >> > using pacemaker so that controllers can be placed in different fault
> > >> > domains.
> > >> >
> > >> > Signed-off-by: aginwala <aginw...@ebay.com>
> > >> >
> > >>
> > >> I see the below warning when applying the patch
> > >>
> > >> .git/rebase-apply/patch:153: new blank line at EOF.
> > >> +
> > >> warning: 1 line adds whitespace errors.
> > >>
> > > >>> Sure. Will fix.
> > >
> > >>
> > >> > ---
> > >> > ovn/utilities/ovndb-servers.ocf | 83
> ++++++++++++++++++++++++++++++
> > >>
> > >> > ++---------
> > >> > 1 file changed, 65 insertions(+), 18 deletions(-)
> > >> >
> > >> > diff --git a/ovn/utilities/ovndb-servers.ocf
> > >> > b/ovn/utilities/ovndb-servers.ocf
> > >> > index 164b6bc..1b4b6ab 100755
> > >> > --- a/ovn/utilities/ovndb-servers.ocf
> > >> > +++ b/ovn/utilities/ovndb-servers.ocf
> > >> > @@ -9,6 +9,8 @@
> > >> > : ${SB_MASTER_PROTO_DEFAULT="tcp"}
> > >> > : ${MANAGE_NORTHD_DEFAULT="no"}
> > >> > : ${INACTIVE_PROBE_DEFAULT="5000"}
> > >> > +: ${LISTEN_ON_MASTER_IP_ONLY_DEFAULT="yes"}
> > >> > +: ${LISTEN_ON_SLAVE_DEFAULT="yes"}
> > >> >
> > >> > CRM_MASTER="${HA_SBIN_DIR}/crm_master -l reboot"
> > >> > CRM_ATTR_REPL_INFO="${HA_SBIN_DIR}/crm_attribute --type crm_config
> > >> > --name OVN_REPL_INFO -s ovn_ovsdb_master_server"
> > >> > @@ -21,6 +23,14 @@ SB_MASTER_PROTO=${OCF_RESKEY_s
> > >> b_master_protocol:-${SB_
> > >> > MASTER_PROTO_DEFAULT}}
> > >> > MANAGE_NORTHD=${OCF_RESKEY_manage_northd:-${MANAGE_
> NORTHD_DEFAULT}}
> > >> > INACTIVE_PROBE=${OCF_RESKEY_inactive_probe_interval:-${
> > >> > INACTIVE_PROBE_DEFAULT}}
> > >> >
> > >> > +# In order for pacemaker to work with LB, we can keep
> > >> > LISTEN_ON_MASTER_IP_ONLY
> > >> > +# to false and pass LB vip IP while creating pcs resource.
> > >> > +LISTEN_ON_MASTER_IP_ONLY=${OCF_RESKEY_listen_on_master_
> > >> > ip_only:-${LISTEN_ON_MASTER_IP_ONLY_DEFAULT}}
> > >> > +
> > >> > +# In order for pacemaker to work with LB, we can also set
> > >> LISTEN_ON_SLAVE
> > >> > +# to false so that slaves do not listen on 0.0.0.0.
> > >> > +LISTEN_ON_SLAVE=${OCF_RESKEY_listen_on_slave:-${LISTEN_ON_S
> > >> LAVE_DEFAULT}}
> > >> >
> > >>
> > >>
> > >> You need to define these parameters in the <parameters> section here -
> > >> https://github.com/openvswitch/ovs/blob/master/ovn/
> > >> utilities/ovndb-servers.ocf#L118
> > >> Otherwise we will not be able to pass these options when starting the
> > >> resource. Ideally we want to do
> > >> "pcs resource create ovndb_servers .... listen_on_master_ip_only=no
> > >> listen_on_slave=no
> > >>
> > > >> Thanks for the pointer. I will update that and re-test.
> > >
> > >>
> > >>
> > >> +
> > >> > # Invalid IP address is an address that can never exist in the
> > >> network, as
> > >> > # mentioned in rfc-5737. The ovsdb servers connects to this IP
> address
> > >> > till
> > >> > # a master is promoted and the IPAddr2 resource is started.
> > >> > @@ -157,22 +167,24 @@ ovsdb_server_notify() {
> > >> > ${OVN_CTL} --ovn-manage-ovsdb=no start_northd
> > >> > fi
> > >> >
> > >> > - conn=`ovn-nbctl get NB_global . connections`
> > >> > - if [ "$conn" == "[]" ]
> > >> > - then
> > >> > - ovn-nbctl -- --id=@conn_uuid create Connection \
> > >> > + # TODO: Need to troubleshoot as to removing target is ok as
> > >> well.
> > >>
> > >> + if [ "x${LISTEN_ON_MASTER_IP_ONLY}" = xyes ]; then
> > >> >
> > >>
> > >> If LB VIP is used, you don't want to set the probe interval ?
> > >
> > > >>> I thought we can stick with common probe interval for any case.
> Hence,
> > > kept it.
> > >
> > >>
> > >> > + conn=`ovn-nbctl get NB_global . connections`
> > >> > + if [ "$conn" == "[]" ]
> > >> > + then
> > >> > + ovn-nbctl -- --id=@conn_uuid create Connection \
> > >> > target="p${NB_MASTER_PROTO}\:${NB_MASTER_PORT}\:${MASTER_IP}" \
> > >> > inactivity_probe=$INACTIVE_PROBE -- set NB_Global .
> > >> > connections=@conn_uuid
> > >> > - fi
> > >> > + fi
> > >> >
> > >> > - conn=`ovn-sbctl get SB_global . connections`
> > >> > - if [ "$conn" == "[]" ]
> > >> > - then
> > >> > - ovn-sbctl -- --id=@conn_uuid create Connection \
> > >> > + conn=`ovn-sbctl get SB_global . connections`
> > >> > + if [ "$conn" == "[]" ]
> > >> > + then
> > >> > + ovn-sbctl -- --id=@conn_uuid create Connection \
> > >> > target="p${SB_MASTER_PROTO}\:${SB_MASTER_PORT}\:${MASTER_IP}" \
> > >> > inactivity_probe=$INACTIVE_PROBE -- set SB_Global .
> > >> > connections=@conn_uuid
> > >> > + fi
> > >> > fi
> > >> > -
> > >> > else
> > >> > if [ "$MANAGE_NORTHD" = "yes" ]; then
> > >> > # Stop ovn-northd service. Set --ovn-manage-ovsdb=no so
> > >> that
> > >> > @@ -295,15 +307,15 @@ ovsdb_server_start() {
> > >> >
> > >> > set ${OVN_CTL}
> > >> >
> > >> > - set $@ --db-nb-addr=${MASTER_IP} --db-nb-port=${NB_MASTER_PORT}
> > >> > - set $@ --db-sb-addr=${MASTER_IP} --db-sb-port=${SB_MASTER_PORT}
> > >> > + # For LB vip to talk to master pool member on a specific tcp
> port,
> > >> we
> > >> > need
> > >> > + # to listen on 0.0.0.0.instead of master_ip
> > >> > + if [ "x${LISTEN_ON_MASTER_IP_ONLY}" = xno ]; then
> > >> > + set $@ --db-nb-port=${NB_MASTER_PORT}
> > >> > + set $@ --db-sb-port=${SB_MASTER_PORT}
> > >> >
> > >> > - if [ "x${NB_MASTER_PROTO}" = xtcp ]; then
> > >> > - set $@ --db-nb-create-insecure-remote=yes
> > >> > - fi
> > >> > -
> > >> > - if [ "x${SB_MASTER_PROTO}" = xtcp ]; then
> > >> > - set $@ --db-sb-create-insecure-remote=yes
> > >> > + else
> > >> > + set $@ --db-nb-addr=${MASTER_IP}
> --db-nb-port=${NB_MASTER_PORT}
> > >> > + set $@ --db-sb-addr=${MASTER_IP}
> --db-sb-port=${SB_MASTER_PORT}
> > >> > fi
> > >> >
> > >> > if [ "x${present_master}" = x ]; then
> > >> > @@ -313,15 +325,44 @@ ovsdb_server_start() {
> > >> > # Force all copies to come up as slaves by pointing them
> into
> > >> > # space and let pacemaker pick one to promote:
> > >> > #
> > >> > + if [ "x${NB_MASTER_PROTO}" = xtcp ]; then
> > >> > + set $@ --db-nb-create-insecure-remote=yes
> > >> > + fi
> > >> > +
> > >> > + if [ "x${SB_MASTER_PROTO}" = xtcp ]; then
> > >> > + set $@ --db-sb-create-insecure-remote=yes
> > >> > + fi
> > >> > set $@ --db-nb-sync-from-addr=${INVALID_IP_ADDRESS}
> > >> > --db-sb-sync-from-addr=${INVALID_IP_ADDRESS}
> > >> >
> > >> > elif [ ${present_master} != ${host_name} ]; then
> > >> > + if [ "x${LISTEN_ON_SLAVE}" = xno ]; then
> > >> > + # TODO: for using LB vip, need to test for ssl.
> > >> > + set $@ --db-nb-create-insecure-remote=no
> > >> >
> > >>
> > >> The default value of "db-nb-create-insecure-remote" is no. So there
> is no
> > >> need to set this. We can just have
> > >> if [ "x${LISTEN_ON_SLAVE} = xyes ] ; then
> > >> if ...
> > >> ...
> > >> fi
> > >
> > > >>> Sure let me re-change and test. Will update the new patch
> accordingly.
> > >
> > >> + set $@ --db-sb-create-insecure-remote=no
> > >> > + else
> > >> > + if [ "x${NB_MASTER_PROTO}" = xtcp ]; then
> > >> > + set $@ --db-nb-create-insecure-remote=yes
> > >> > + fi
> > >> > +
> > >> > + if [ "x${SB_MASTER_PROTO}" = xtcp ]; then
> > >> > + set $@ --db-sb-create-insecure-remote=yes
> > >> > + fi
> > >> > + fi
> > >> > # An existing master is active, connect to it
> > >> > set $@ --db-nb-sync-from-addr=${MASTER_IP}
> > >> > --db-sb-sync-from-addr=${MASTER_IP}
> > >> > set $@ --db-nb-sync-from-port=${NB_MASTER_PORT}
> > >> > set $@ --db-nb-sync-from-proto=${NB_MASTER_PROTO}
> > >> > set $@ --db-sb-sync-from-port=${SB_MASTER_PORT}
> > >> > set $@ --db-sb-sync-from-proto=${SB_MASTER_PROTO}
> > >> > +
> > >> > + else
> > >> > + if [ "x${NB_MASTER_PROTO}" = xtcp ]; then
> > >> > + set $@ --db-nb-create-insecure-remote=yes
> > >> > + fi
> > >> > +
> > >> > + if [ "x${SB_MASTER_PROTO}" = xtcp ]; then
> > >> > + set $@ --db-sb-create-insecure-remote=yes
> > >> > + fi
> > >> > fi
> > >> >
> > >> > $@ start_ovsdb
> > >> > @@ -416,6 +457,11 @@ ovsdb_server_promote() {
> > >> > ;;
> > >> > esac
> > >> >
> > >> > + if [ "x${LISTEN_ON_SLAVE}" = xno ]; then
> > >>
> > >>
> > >> The use of LISTEN_ON_SLAVE param in "ovsdb_server_promote" is very
> > >> confusing. I think it should
> > >> be possible to have just one param - something like -
> > >> "master_ip_pacemaker_resource=(yes/no)" (or some thing better ).
> > >> Thoughts ?
> > >
> > > >>> Sure, how about master_ip_lb_resource to avoid confusion? So you
> > > mean, just use this variable and completely get rid of LISTEN_ON_SLAVE
> > > and LISTEN_ON_MASTER_IP_ONLY?
> > >
> >
> > 'master_ip_lb_resource' seems fine to me. Thats' right. Just have one
> > parameter.
> >
> >
> > Will there be a case where the user can select
> > >> listen_on_master_ip_only=no and
> > >> listen_on_slave=yes ?
> > >>
> > > >>> No.
> >
> 'master_ip_lb_resource' seems a little confusing to me. What does it
> really mean?
> The desired behavior is, when the flag is "no", do the default:
>
> - On both master node and slave node, listen to MASTER_IP only
>
> When the flag is "yes", it works with an external LB:
>
> 1) On mater node, listen on 0.0.0.0 (because the MASTER_IP doesn't belong
> to the node)
> 2) On slave node, don't listen on TCP port (because we want the LB to be
> able to tell which one is the master according to whether the TCP port is
> open on the node)
>
> I am ok with just one parameter, but I'd suggest to make it more
> straighforward. I am also wondering, can we unify point 2), i.e. never
> listen on TCP port for slave node? Numan, do you have any use case for
> accessing slave node DBs using TCP?
>
> >>> Thanks Han for the review. We agreed on this name just to avoid
confusion that IP is coming from LB and not pacemaker ipaddr2 resource. I
can update the documentation to avoid confusion since documentation have no
details of using LB VIP. Let me know if that needs to be in same or
different patch. Also, as discussed for the promote logic, we are
restarting ovs for LB use case and you want to unify it for pacemaker vip
resource too to address point2.
$@ start_ovsdb@@ -416,6 +457,11 @@ ovsdb_server_promote() {
;;
esac
+ if [ "x${MASTER_IP_LB_RESOURCE}" = xyes ]; then+ #
Restart ovs so that new master can listen on tcp port+
${OVN_CTL} stop_ovsdb+ ovsdb_server_start
However, I can add TODO there to see if its ok to unify too. Will wait for
inputs on Numan too for the same.
Thanks,
> Han
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
