Thanks for the review. Please see comments in line.
I will address the review comments and submit v2. Thanks Numan On Mon, Apr 2, 2018 at 8:48 AM, aginwala <[email protected]> wrote: > > > On Wed, Mar 28, 2018 at 4:37 AM, <[email protected]> wrote: > >> From: Numan Siddique <[email protected]> >> >> This patch adds the options to start clustered OVN db servers. >> To support this following options are added - >> '--db-nb-cluster-local-addr', >> '--db-nb-cluster-remote-addr', '--db-sb-cluster-local-addr' and >> '--db-sb-cluster-remote-addr'. If only '--db-(nb/sb)-cluster-local-addr' >> is defined >> then clustered db is created (using ovsdb-tool create-cluster). If both >> are defined, >> then the db is added to the cluster (using ovsdb-tool join-cluster) >> > > >>>Since it's using ovn-ctl to start the dbs in cluster including northd, > can we include that in commit message for clarity? > Also the discussion would be helpful to stamp: > https://mail.openvswitch.org/pipermail/ovs-discuss/2018-March/046470.html > > Ack. Will do. > >> Presently this patch doesn't handle the schema update scenario when >> restarting the >> clustered ovsdb-servers. This will be handled in a separate patch. >> >> (There are 2 checkpatch warnings 'Line length is >79-characters long' in >> ovn-ctl.8.xml) >> >> Signed-off-by: Numan Siddique <[email protected]> >> --- >> ovn/utilities/ovn-ctl | 175 ++++++++++++++++++++++++++++++ >> -------------- >> ovn/utilities/ovn-ctl.8.xml | 70 ++++++++++++++++++ >> utilities/ovs-lib.in | 71 +++++++++++++++++- >> 3 files changed, 259 insertions(+), 57 deletions(-) >> >> diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl >> index dc0c26159..ed8f41081 100755 >> --- a/ovn/utilities/ovn-ctl >> +++ b/ovn/utilities/ovn-ctl >> @@ -95,80 +95,131 @@ promote_ovnsb() { >> >> start_nb_ovsdb() { >> # Check and eventually start ovsdb-server for Northbound DB >> - if ! pidfile_is_running $DB_NB_PID; then >> - upgrade_db "$DB_NB_FILE" "$DB_NB_SCHEMA" 1>/dev/null 2>/dev/null >> + if pidfile_is_running $DB_SB_PID; then >> > > >>> Shouldn't it be DB_NB_PID? > Oops. Thanks for pointing this out. > > >> + return >> + fi >> >> - set ovsdb-server >> + mode="standalone" >> + if test ! -z "$DB_NB_CLUSTER_LOCAL_ADDR"; then >> + mode="cluster" >> + elif test ! -z "$DB_NB_SYNC_FROM_ADDR"; then >> + mode="active_passive" >> + echo "$DB_NB_SYNC_FROM_PROTO:$DB_NB_SYNC_FROM_ADDR:\ >> +$DB_NB_SYNC_FROM_PORT" > $ovnnb_active_conf_file >> + fi >> >> - if test X"$DB_NB_DETACH" != Xno; then >> - set "$@" --detach --monitor >> + if test X$mode = "Xcluster"; then >> + if test -z "$DB_NB_CLUSTER_REMOTE_ADDR"; then >> + create_cluster "$DB_NB_FILE" "$DB_NB_SCHEMA" \ >> +"$DB_NB_CLUSTER_LOCAL_ADDR" >> else >> - set exec "$@" >> + join_cluster "$DB_NB_FILE" "OVN_Northbound" \ >> +"$DB_NB_CLUSTER_LOCAL_ADDR" "$DB_NB_CLUSTER_REMOTE_ADDR" >> fi >> + else >> + upgrade_db "$DB_NB_FILE" "$DB_NB_SCHEMA" 1>/dev/null 2>/dev/null >> + fi >> >> - set "$@" $OVN_NB_LOG --log-file=$OVN_NB_LOGFILE >> - set "$@" --remote=punix:$DB_NB_SOCK --pidfile=$DB_NB_PID >> - set "$@" --remote=db:OVN_Northbound,NB_Global,connections >> - set "$@" --unixctl=ovnnb_db.ctl >> - set "$@" --private-key=db:OVN_Northbound,SSL,private_key >> - set "$@" --certificate=db:OVN_Northbound,SSL,certificate >> - set "$@" --ca-cert=db:OVN_Northbound,SSL,ca_cert >> - set "$@" --ssl-protocols=db:OVN_Northbound,SSL,ssl_protocols >> - set "$@" --ssl-ciphers=db:OVN_Northbound,SSL,ssl_ciphers >> - >> - if test X"$DB_NB_CREATE_INSECURE_REMOTE" = Xyes; then >> - set "$@" --remote=ptcp:$DB_NB_PORT:$DB_NB_ADDR >> - fi >> + set ovsdb-server >> >> - if test ! -z "$DB_NB_SYNC_FROM_ADDR"; then >> - echo "$DB_NB_SYNC_FROM_PROTO:$DB_NB >> _SYNC_FROM_ADDR:$DB_NB_SYNC_FROM_PORT" > $ovnnb_active_conf_file >> - fi >> + if test X"$DB_NB_DETACH" != Xno; then >> + set "$@" --detach --monitor >> + else >> + set exec "$@" >> + fi >> >> - if test -e $ovnnb_active_conf_file; then >> - set "$@" --sync-from=`cat $ovnnb_active_conf_file` >> - fi >> + set "$@" $OVN_NB_LOG --log-file=$OVN_NB_LOGFILE >> + set "$@" --remote=punix:$DB_NB_SOCK --pidfile=$DB_NB_PID >> + set "$@" --unixctl=ovnnb_db.ctl >> + >> + set "$@" --remote=db:OVN_Northbound,NB_Global,connections >> + set "$@" --private-key=db:OVN_Northbound,SSL,private_key >> + set "$@" --certificate=db:OVN_Northbound,SSL,certificate >> + set "$@" --ca-cert=db:OVN_Northbound,SSL,ca_cert >> + set "$@" --ssl-protocols=db:OVN_Northbound,SSL,ssl_protocols >> + set "$@" --ssl-ciphers=db:OVN_Northbound,SSL,ssl_ciphers >> + >> + if test X"$DB_NB_CREATE_INSECURE_REMOTE" = Xyes; then >> + set "$@" --remote=ptcp:$DB_NB_PORT:$DB_NB_ADDR >> + fi >> + >> + # Set '--sync-from' if ovnnb_active_conf_file is present and mode >> + # is 'active_passive' >> + if test -e $ovnnb_active_conf_file && test X$mode = >> "Xactive_passive"; then >> + set "$@" --sync-from=`cat $ovnnb_active_conf_file` >> + fi >> >> - $@ $DB_NB_FILE >> + $@ $DB_NB_FILE >> + >> + # Initialize the database if it's running standalone, active-passive, >> + # or is the first server in a cluster. >> + if test -z "$DB_NB_CLUSTER_REMOTE_ADDR"; then >> ovn-nbctl init >> fi >> } >> >> start_sb_ovsdb() { >> # Check and eventually start ovsdb-server for Southbound DB >> - if ! pidfile_is_running $DB_SB_PID; then >> - upgrade_db "$DB_SB_FILE" "$DB_SB_SCHEMA" 1>/dev/null 2>/dev/null >> + if pidfile_is_running $DB_SB_PID; then >> + return >> + fi >> + >> + mode="standalone" >> >> - set ovsdb-server >> + if test ! -z "$DB_SB_CLUSTER_LOCAL_ADDR"; then >> + mode="cluster" >> + elif test ! -z "$DB_SB_SYNC_FROM_ADDR"; then >> + mode="active_passive" >> + echo "$DB_SB_SYNC_FROM_PROTO:$DB_SB_SYNC_FROM_ADDR:\ >> +$DB_SB_SYNC_FROM_PORT" > $ovnsb_active_conf_file >> + fi >> >> - if test X"$DB_SB_DETACH" != Xno; then >> - set "$@" --detach --monitor >> + if test X$mode = "Xcluster"; then >> + if test -z "$DB_SB_CLUSTER_REMOTE_ADDR"; then >> + create_cluster "$DB_SB_FILE" "$DB_SB_SCHEMA" \ >> +"$DB_SB_CLUSTER_LOCAL_ADDR" >> else >> - set exec "$@" >> + join_cluster "$DB_SB_FILE" "OVN_Southbound" \ >> +"$DB_SB_CLUSTER_LOCAL_ADDR" "$DB_SB_CLUSTER_REMOTE_ADDR" >> fi >> + else >> + upgrade_db "$DB_SB_FILE" "$DB_SB_SCHEMA" 1>/dev/null 2>/dev/null >> + fi >> >> - set "$@" $OVN_SB_LOG --log-file=$OVN_SB_LOGFILE >> - set "$@" --remote=punix:$DB_SB_SOCK --pidfile=$DB_SB_PID >> - set "$@" --remote=db:OVN_Southbound,SB_Global,connections >> - set "$@" --unixctl=ovnsb_db.ctl >> - set "$@" --private-key=db:OVN_Southbound,SSL,private_key >> - set "$@" --certificate=db:OVN_Southbound,SSL,certificate >> - set "$@" --ca-cert=db:OVN_Southbound,SSL,ca_cert >> - set "$@" --ssl-protocols=db:OVN_Southbound,SSL,ssl_protocols >> - set "$@" --ssl-ciphers=db:OVN_Southbound,SSL,ssl_ciphers >> - >> - if test X"$DB_SB_CREATE_INSECURE_REMOTE" = Xyes; then >> - set "$@" --remote=ptcp:$DB_SB_PORT:$DB_SB_ADDR >> - fi >> + set ovsdb-server >> >> - if test ! -z "$DB_SB_SYNC_FROM_ADDR"; then >> - echo "$DB_SB_SYNC_FROM_PROTO:$DB_SB >> _SYNC_FROM_ADDR:$DB_SB_SYNC_FROM_PORT" > $ovnsb_active_conf_file >> - fi >> + if test X"$DB_SB_DETACH" != Xno; then >> + set "$@" --detach --monitor >> + else >> + set exec "$@" >> + fi >> >> - if test -e $ovnsb_active_conf_file; then >> - set "$@" --sync-from=`cat $ovnsb_active_conf_file` >> - fi >> + set "$@" $OVN_SB_LOG --log-file=$OVN_SB_LOGFILE >> + set "$@" --remote=punix:$DB_SB_SOCK --pidfile=$DB_SB_PID >> + set "$@" --unixctl=ovnsb_db.ctl >> + >> + set "$@" --remote=db:OVN_Southbound,SB_Global,connections >> + set "$@" --private-key=db:OVN_Southbound,SSL,private_key >> + set "$@" --certificate=db:OVN_Southbound,SSL,certificate >> + set "$@" --ca-cert=db:OVN_Southbound,SSL,ca_cert >> + set "$@" --ssl-protocols=db:OVN_Southbound,SSL,ssl_protocols >> + set "$@" --ssl-ciphers=db:OVN_Southbound,SSL,ssl_ciphers >> + >> + if test X"$DB_SB_CREATE_INSECURE_REMOTE" = Xyes; then >> + set "$@" --remote=ptcp:$DB_SB_PORT:$DB_SB_ADDR >> + fi >> + >> + # Set '--sync-from' if ovnsb_active_conf_file is present and mode >> + # is 'active_passive' >> + if test -e $ovnsb_active_conf_file && test X$mode = >> "Xactive_passive"; then >> + set "$@" --sync-from=`cat $ovnsb_active_conf_file` >> + fi >> >> - $@ $DB_SB_FILE >> + $@ $DB_SB_FILE >> + >> + # Initialize the database if it's running standalone, active-passive, >> + # or is the first server in a cluster. >> + if test -z "$DB_SB_CLUSTER_REMOTE_ADDR"; then >> ovn-sbctl init >> fi >> } >> @@ -236,7 +287,8 @@ start_northd () { >> exit >> fi >> fi >> - ovn_northd_params="--ovnnb-db=unix:$DB_NB_SOCK >> --ovnsb-db=unix:$DB_SB_SOCK" >> + ovn_northd_params="--ovnnb-db=$OVN_NORTHD_NB_DB \ >> + --ovnsb-db=$OVN_NORTHD_SB_DB" >> > > > else >> ovn_northd_params="`cat $ovn_northd_db_conf_file`" >> fi >> @@ -406,6 +458,13 @@ set_defaults () { >> >> DB_NB_DETACH="yes" >> DB_SB_DETACH="yes" >> + >> + DB_NB_CLUSTER_LOCAL_ADDR="" >> > > >>> Can we also include default NB_DB_CLUSTER_PORTS and SB_DB_CLUSTER_PORTS > as 6643 and 6644 respectively? > If user is willing to use different ports, only then he/she can over-ride. > If we want do this, then instead of the command "db-(nb/sb)-cluster-remote-addr" we need to add three commands "db-(nb/sb)-cluster-remote-ip", "db-(nb/sb)-cluster-remote-port" and "db-(nb/sb)-cluster-remote-prot" and similar commands instead of "db-(nb/sb)-cluster-local-addr". > >> + DB_NB_CLUSTER_REMOTE_ADDR="" >> + DB_SB_CLUSTER_LOCAL_ADDR="" >> + DB_SB_CLUSTER_REMOTE_ADDR="" >> + OVN_NORTHD_NB_DB="unix:$DB_NB_SOCK" >> + OVN_NORTHD_SB_DB="unix:$DB_SB_SOCK" >> } >> >> set_option () { >> @@ -494,6 +553,16 @@ File location options: >> --db-sb-sync-from-port=ADDR OVN Southbound active db tcp port >> (default: $DB_SB_SYNC_FROM_PORT) >> --db-sb-sync-from-proto=PROTO OVN Southbound active db transport >> (default: $DB_SB_SYNC_FROM_PROTO) >> --db-sb-create-insecure-remote=yes|no Create ptcp OVN Southbound >> remote (default: $DB_SB_CREATE_INSECURE_REMOTE) >> + --db-nb-cluster-local-addr=ADDR OVN_Northbound cluster local address \ >> + (default: $DB_NB_CLUSTER_LOCAL_ADDR) >> + --db-nb-cluster-remote-addr=ADDR OVN_Northbound cluster remote >> address \ >> + (default: $DB_NB_CLUSTER_REMOTE_ADDR) >> + --db-sb-cluster-local-addr=ADDR OVN_Southbound cluster local address \ >> + (default: $DB_SB_CLUSTER_LOCAL_ADDR) >> + --db-sb-cluster-remote-addr=ADDR OVN_Southbound cluster remote >> address \ >> + (default: $DB_SB_CLUSTER_REMOTE_ADDR) >> + --ovn-northd-nb-db=NB DB address(es) (default: $OVN_NORTHD_NB_DB) >> + --ovn-northd-sb-db=SB DB address(es) (default: $OVN_NORTHD_SB_DB) >> >> Default directories with "configure" option and environment variable >> override: >> logs: /usr/local/var/log/openvswitch (--with-logdir, OVS_LOGDIR) >> diff --git a/ovn/utilities/ovn-ctl.8.xml b/ovn/utilities/ovn-ctl.8.xml >> index 40defc9ec..ae81ef15e 100644 >> --- a/ovn/utilities/ovn-ctl.8.xml >> +++ b/ovn/utilities/ovn-ctl.8.xml >> @@ -66,6 +66,43 @@ >> <p><code>--db-sb-sync-from-addr=<var>IP ADDRESS</var></code></p> >> <p><code>--db-sb-sync-from-port=<var>PORT NUMBER</var></code></p> >> <p><code>--db-sb-sync-from-proto=<var>PROTO</var></code></p> >> + <p> >> + <code> >> + --ovn-northd-nb-db=<var>PROTO</var>:<var>IP ADDRESS</var>: >> + <var>PORT</var>.. >> + </code> >> + </p> >> + <p> >> + <code> >> + --ovn-northd-sb-db=<var>PROTO</var>:<var>IP ADDRESS</var>: >> + <var>PORT</var>.. >> + </code> >> + </p> >> + <h1> Clustering options </h1> >> + <p> >> + <code> >> + --db-nb-cluster-local-addr=<var>PROTO</var>: >> + <var>IP ADDRESS</var>:<var>PORT</var> >> + </code> >> + </p> >> + <p> >> + <code> >> + --db-nb-cluster-remote-addr=<var>PROTO</var>: >> + <var>IP ADDRESS</var>:<var>PORT</var> >> + </code> >> + </p> >> + <p> >> + <code> >> + --db-sb-cluster-local-addr=<var>PROTO</var>: >> + <var>IP ADDRESS</var>:<var>PORT</var> >> + </code> >> + </p> >> + <p> >> + <code> >> + --db-sb-cluster-remote-addr=<var>PROTO</var>: >> + <var>IP ADDRESS</var>:<var>PORT</var> >> + </code> >> + </p> >> >> <h1>Configuration files</h1> >> <p>Following are the optional configuration files. If present, it >> should be located in the etc dir</p> >> @@ -125,4 +162,37 @@ >> <p><code># ovn-ctl promote_ovnsb</code></p> >> <p><code># ovn-ctl --db-nb-sync-from-addr=x.x.x.x >> --db-nb-sync-from-port=6641 demote_ovnnb</code></p> >> <p><code># ovn-ctl --db-sb-sync-from-addr=x.x.x.x >> --db-sb-sync-from-port=6642 demote_ovnsb</code></p> >> + >> + <h2>Starting OVN ovsdb servers cluster on a node with IP x.x.x.x</h2> > > >>> The e.g. is just using tcp. Can we say that we can also do it with ssl > or not? Also we say that e.g. usage is for 3 node setup. > Ack. will do. > > >> + <p> >> + <code> >> + # ovn-ctl --db-nb-cluster-local-addr="tcp:x.x.x.x:6643" >> + --db-sb-cluster-local-addr="tcp:x.x.x.x:6644" start_ovsdb >> + </code> >> + </p> >> + >> + <h2>Starting OVN ovsdb-servers on a node with IP y.y.y.y and joining >> a cluster started at x.x.x.x</h2> >> + <p> >> + <code> >> + # ovn-ctl --db-nb-cluster-local-addr="tcp:x.x.x.x:6643" >> + --db-sb-cluster-local-addr="tcp:x.x.x.x:6644" >> + --db-nb-cluster-remote-addr="tcp:y.y.y.y:6643" >> + --db-sb-cluster-remote-addr="tcp:y.y.y.y:6644" start_ovsdb >> + </code> >> + </p> >> + >> + <h2>Starting OVN ovsdb-servers cluster and ovn-northd on a node with >> IP x.x.x.x with other cluster nodes - y.y.y.y and z.z.z.z</h2> > > >>> Shouldn't it be for z.z.z.z joining x.x.x.x and y.y.y.y? For northd we > can keep common statement as command is same for all three nodes. > >> + <p> >> + <code> >> + # ovn-ctl -db-nb-addr=x.x.x.x --db-nb-port=6641 >> + --db-nb-create-insecure-remote=yes >> + --db-nb-cluster-local-addr=tcp:x.x.x.x:6643 >> + --db-sb-addr=x.x.x.x --db-sb-port=6642 > > > >>> Since we have default ports in the ovn-ctl, can't we skip db-sb-port > and db-nb-port? Only pass if we are using other ports. > Sure. I will update the patch. > > > + --db-sb-create-insecure-remote=yes >> + --db-sb-cluster-local-addr=tcp:x.x.x.x:6644 >> + --ovn-northd-nb-db=tcp:x.x.x.x:6641,tcp:y.y.y.y:6641,tcp:z.z >> .z.z:6641 >> + --ovn-northd-sb-db=tcp:x.x.x.x:6642,tcp:y.y.y.y:6642,tcp:z.z >> .z.z:6642 >> + start_northd >> + </code> >> + </p> >> </manpage> >> diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in >> index cf6b6d296..6647e5b6c 100644 >> --- a/utilities/ovs-lib.in >> +++ b/utilities/ovs-lib.in >> @@ -399,6 +399,14 @@ create_db () { >> action "Creating empty database $DB_FILE" ovsdb_tool create >> "$DB_FILE" "$DB_SCHEMA" >> } >> >> +backup_db() { >> + DB_FILE="$1" >> + version=`ovsdb_tool db-version "$DB_FILE"` >> + cksum=`ovsdb_tool db-cksum "$DB_FILE" | awk '{print $1}'` >> + backup=$DB_FILE.backup$version-$cksum >> + action "Backing up database to $backup" cp "$DB_FILE" "$backup" || >> return 1 >> +} >> + >> upgrade_db () { >> DB_FILE="$1" >> DB_SCHEMA="$2" >> @@ -410,10 +418,10 @@ upgrade_db () { >> create_db "$DB_FILE" "$DB_SCHEMA" >> elif test X"`ovsdb_tool needs-conversion "$DB_FILE" "$DB_SCHEMA"`" = >> Xyes; then >> # Back up the old version. >> - version=`ovsdb_tool db-version "$DB_FILE"` >> - cksum=`ovsdb_tool db-cksum "$DB_FILE" | awk '{print $1}'` >> - backup=$DB_FILE.backup$version-$cksum >> - action "Backing up database to $backup" cp "$DB_FILE" "$backup" >> || return 1 >> + backup_db $DB_FILE >> + if [ "$?" != "0" ]; then >> + return 1 >> + fi >> >> # Compact database. This is important if the old schema did not >> enable >> # garbage collection (i.e. if it did not have any tables with >> "isRoot": >> @@ -613,3 +621,58 @@ restart () { >> flow_restore_complete >> add_managers >> } >> + >> + >> +create_cluster () { >> + DB_FILE="$1" >> + DB_SCHEMA="$2" >> + LOCAL_ADDR="$3" >> + >> + if test ! -e "$DB_FILE"; then >> + action "Creating cluster database $DB_FILE" ovsdb_tool >> create-cluster \ >> +"$DB_FILE" "$DB_SCHEMA" "$LOCAL_ADDR" >> + else >> + # DB file exists. Check if it is a standalone db or not. If it >> is a >> + # standalone db, create a clustered db from the standalone db >> + ovsdb_tool db-is-standalone $DB_FILE >> + if [ "$?" = "0" ]; then >> + backup_db $DB_FILE >> + if [ "$?" != "0" ]; then >> + return 1 >> + fi >> + >> + rm -f $DB_FILE >> + action "Creating cluster database from standalone db >> $DB_FILE" \ >> + ovsdb_tool create-cluster "$DB_FILE" "$backup" "$LOCAL_ADDR" >> + fi >> + >> + # TODO(numans) - If it is a clustered db, then >> + # - upgrade the db if required >> + # - compact the db. >> + fi >> +} >> + >> +join_cluster() { >> + DB_FILE="$1" >> + SCHEMA_NAME="$2" >> + LOCAL_ADDR="$3" >> + REMOTE_ADDR="$4" >> + >> + if test ! -e "$DB_FILE"; then >> + ovsdb_tool join-cluster "$DB_FILE" "$SCHEMA_NAME" "$LOCAL_ADDR" >> "$REMOTE_ADDR" >> + else >> + # DB file exists. Check if it is a standalone db or not. If it >> is a >> + # standalone db, backup the db and join the cluster. >> + ovsdb_tool db-is-standalone $DB_FILE >> + if [ "$?" = "0" ]; then >> + backup_db $DB_FILE >> + if [ "$?" != "0" ]; then >> + return 1 >> + fi >> + >> + rm -f $DB_FILE >> + action "Joining cluster database" \ >> + ovsdb_tool join-cluster "$DB_FILE" "$SCHEMA_NAME" >> "$LOCAL_ADDR" "$REMOTE_ADDR" >> + fi >> + fi >> +} >> -- >> 2.14.3 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
