On 02/08/2016 01:02 PM, Michael wrote:
> Title fixed and updated ovn-northd to the new db paths.
Some minor tips on conventions for emailing patches:
When you send another revision, you don't need to specify what changed
in the commit message itself. Notes about what has changed can go after
the "---" where you see the diffstat. That part is not included in the
patch or commit message and is just dropped when the patch is applied.
The "--annotate" argument to "git send-email" makes it easy to add notes
like this.
It's also customary to include a revision number in the Subject. In
this case, it would be "[PATCH v2] ...". It's even better if you can
have this message set an In-Reply-To header so it shows up as a
follow-up to the v1 patch when threading messages. You can set this
with "git send-email".
> From 9052644fd5ed07583540292fcd0cb5ddc47d633e Mon Sep 17 00:00:00 2001
> From: Michael Arnaldi <m...@mymoneyex.com>
> Date: Mon, 8 Feb 2016 18:56:51 +0100
> Subject: [PATCH] Separating ovn nb&sb databases
Again, this ends up as the body of the commit message, which isn't waht
you want. I suspect you're pasting the patch into an email?
I strongly suggest using "git send-email" to ensure things are formatted
correctly.
> Signed-off-by: Michael Arnaldi <m...@mymoneyex.com>
> ---
> ovn/northd/ovn-northd.c | 33 ++++++++++++++++++--------
> ovn/utilities/ovn-ctl | 59
> ++++++++++++++++++++++++++++++++---------------
> ovn/utilities/ovn-nbctl.c | 2 +-
> ovn/utilities/ovn-sbctl.c | 3 ++-
> 4 files changed, 66 insertions(+), 31 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index e6271cf..bc61255 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -52,7 +52,8 @@ struct northd_context {
> static const char *ovnnb_db;
> static const char *ovnsb_db;
>
> -static const char *default_db(void);
> +static const char *default_nb_db(void);
> +static const char *default_sb_db(void);
>
> /* Pipeline stages. */
>
> @@ -167,7 +168,7 @@ Options:\n\
> -h, --help display this help message\n\
> -o, --options list available options\n\
> -V, --version display version information\n\
> -", program_name, program_name, default_db(), default_db());
> +", program_name, program_name, default_nb_db(), default_sb_db());
> daemon_usage();
> vlog_usage();
> stream_usage("database", true, true, false);
> @@ -1786,15 +1787,26 @@ ovnsb_db_run(struct northd_context *ctx)
> }
>
>
> -static char *default_db_;
> +static char *default_nb_db_;
>
> static const char *
> -default_db(void)
> +default_nb_db(void)
> {
> - if (!default_db_) {
> - default_db_ = xasprintf("unix:%s/db.sock", ovs_rundir());
> + if (!default_nb_db_) {
> + default_nb_db_ = xasprintf("unix:%s/ovn/run/db_nb.sock",
> ovs_rundir());
I saw your reasoning for the new directory in a previous reply. I think
I'd prefer not having that as the default. I'm hoping the requirement
of shared-storage for HA is short-lived, as it's not going to be
acceptable to most people, I imagine.
> }
> - return default_db_;
> + return default_nb_db_;
> +}
> +
> +static char *default_sb_db_;
> +
> +static const char *
> +default_sb_db(void)
> +{
> + if (!default_sb_db_) {
> + default_sb_db_ = xasprintf("unix:%s/ovn/run/db_sb.sock",
> ovs_rundir());
> + }
> + return default_sb_db_;
> }
>
> static void
> @@ -1856,11 +1868,11 @@ parse_options(int argc OVS_UNUSED, char *argv[]
> OVS_UNUSED)
> }
>
> if (!ovnsb_db) {
> - ovnsb_db = default_db();
> + ovnsb_db = default_sb_db();
> }
>
> if (!ovnnb_db) {
> - ovnnb_db = default_db();
> + ovnnb_db = default_nb_db();
> }
>
> free(short_options);
> @@ -1976,7 +1988,8 @@ main(int argc, char *argv[])
> ovsdb_idl_loop_destroy(&ovnsb_idl_loop);
> service_stop();
>
> - free(default_db_);
> + free(default_nb_db_);
> + free(default_sb_db_);
> exit(res);
> }
>
> diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
> index b171934..85c4199 100755
> --- a/ovn/utilities/ovn-ctl
> +++ b/ovn/utilities/ovn-ctl
> @@ -31,30 +31,38 @@ done
> ## ----- ##
>
> upgrade_ovn_dbs () {
> - ovn_dbs=$(ovs-appctl -t ovsdb-server ovsdb-server/list-dbs 2>/dev/null)
> - for db in $ovn_dbs; do
> - case $db in
> - OVN*)
> - action "Removing $db from ovsdb-server" \
> - ovs-appctl -t ovsdb-server ovsdb-server/remove-db $db
> - ;;
> - esac
> - done
> - upgrade_db "$DB_NB_FILE" "$DB_NB_SCHEMA"
> - upgrade_db "$DB_SB_FILE" "$DB_SB_SCHEMA"
> - for db in $DB_NB_FILE $DB_SB_FILE; do
> - action "Adding $db to ovsdb-server" \
> - ovs-appctl -t ovsdb-server ovsdb-server/add-db $db || exit 1
> - done
> + upgrade_db "$DB_NB_FILE" "$DB_NB_SCHEMA" 1>/dev/null 2>/dev/null
> + upgrade_db "$DB_SB_FILE" "$DB_SB_SCHEMA" 1>/dev/null 2>/dev/null
> +}
> +
> +stop_ovsdb_ovn () {
> + if [ -f $DB_NB_PID ]; then
> + kill -9 $(cat $DB_NB_PID) 1>/dev/null 2>/dev/null
> + rm -f $DB_NB_PID 1>/dev/null 2>/dev/null
> + fi
> + if [ -f $DB_SB_PID ]; then
> + kill -9 $(cat $DB_SB_PID) 1>/dev/null 2>/dev/null
> + rm -f $DB_SB_PID 1>/dev/null 2>/dev/null
> + fi
> +}
> +
> +start_ovsdb_ovn () {
> + mkdir -p $OVN_DB_DIR
> + mkdir -p $OVN_DB_DIR/run
> +
> + ovsdb-server --detach -vconsole:off --remote=punix:$DB_NB_SOCK
> --remote=ptcp:$DB_NB_PORT --pidfile=$DB_NB_PID $DB_NB_FILE
> + ovsdb-server --detach -vconsole:off --remote=punix:$DB_SB_SOCK
> --remote=ptcp:$DB_SB_PORT --pidfile=$DB_SB_PID $DB_SB_FILE
One of my comments on the last revision was about how we start
ovsdb-server. Take a look at how ovsdb-server is started in
utilities/ovs-ctl. Maybe some of that should be factored out into a
function that can be shared (via utilities/ovs-lib)? For example, the
version here doesn't provide the SSL options, so in theory SSL would
have worked before but not after this patch.
> }
>
> start_northd () {
> # We expect ovn-northd to be co-located with ovsdb-server handling both
> the
> # OVN_Northbound and OVN_Southbound dbs.
> + stop_ovsdb_ovn
> upgrade_ovn_dbs
> + start_ovsdb_ovn
>
> set ovn-northd
> - set "$@" -vconsole:emer -vsyslog:err -vfile:info
> + set "$@" -vconsole:emer -vsyslog:err -vfile:info
> --ovnnb-db=unix:$DB_NB_SOCK --ovnsb-db=unix:$DB_SB_SOCK
> OVS_RUNDIR=${OVN_RUNDIR} start_daemon "$OVN_NORTHD_PRIORITY"
> "$OVN_NORTHD_WRAPPER" "$@"
> }
>
> @@ -70,6 +78,7 @@ start_controller () {
>
> stop_northd () {
> OVS_RUNDIR=${OVN_RUNDIR} stop_daemon ovn-northd
> + stop_ovsdb_ovn
> }
>
> stop_controller () {
> @@ -95,12 +104,24 @@ restart_controller () {
> ## ---- ##
>
> set_defaults () {
> - DB_SOCK=$rundir/db.sock
> - DB_NB_FILE=$dbdir/ovnnb.db
> - DB_SB_FILE=$dbdir/ovnsb.db
> + OVN_DB_DIR=$rundir/ovn
> +
> + DB_NB_SOCK=$OVN_DB_DIR/run/db_nb.sock
> + DB_NB_PID=$OVN_DB_DIR/run/db_nb.pid
> + DB_NB_PORT=6651
If OVSDB is typically 6640, how about 6641 and 6642? Both of those
ports are currently unassigned. 6651 and 6652 are unassigned as well, I
just thought putting them together made sense.
> +
> + DB_SB_SOCK=$OVN_DB_DIR/run/db_sb.sock
> + DB_SB_PID=$OVN_DB_DIR/run/db_sb.pid
> + DB_SB_PORT=6652
> +
> + DB_NB_FILE=$OVN_DB_DIR/db/ovnnb.db
> + DB_SB_FILE=$OVN_DB_DIR/db/ovnsb.db
> +
> DB_NB_SCHEMA=$datadir/ovn-nb.ovsschema
> DB_SB_SCHEMA=$datadir/ovn-sb.ovsschema
>
> + DB_SOCK=$rundir/db.sock
> +
> OVN_NORTHD_PRIORITY=-10
> OVN_NORTHD_WRAPPER=
> OVN_CONTROLLER_PRIORITY=-10
> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
> index 324a0a4..62adfba 100644
> --- a/ovn/utilities/ovn-nbctl.c
> +++ b/ovn/utilities/ovn-nbctl.c
> @@ -142,7 +142,7 @@ nbctl_default_db(void)
> if (!def) {
> def = getenv("OVN_NB_DB");
> if (!def) {
> - def = ctl_default_db();
> + def = xasprintf("unix:%s/ovn/run/db_nb.sock", ovs_rundir());
> }
> }
> return def;
> diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
> index b428a35..15b3ea3 100644
> --- a/ovn/utilities/ovn-sbctl.c
> +++ b/ovn/utilities/ovn-sbctl.c
> @@ -28,6 +28,7 @@
> #include <unistd.h>
>
> #include "db-ctl-base.h"
> +#include "dirs.h"
>
> #include "command-line.h"
> #include "compiler.h"
> @@ -154,7 +155,7 @@ sbctl_default_db(void)
> if (!def) {
> def = getenv("OVN_SB_DB");
> if (!def) {
> - def = ctl_default_db();
> + def = xasprintf("unix:%s/ovn/run/db_sb.sock", ovs_rundir());
> }
> }
> return def;
>
In the review of v1, I also noted adding this to the NEWS file.
Thanks,
--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
