I will do this stuff tomorrow, I will take the way ovsdb is started using
ovs-ctl and refactor the logic in a shared library.
I was also thinking about the database issue for HA and I’m currently thinking
why not to leverage the current ecosystem of distributed dbs.
In specific why not to make the db interface pluggable?
What I will do for start is make some flags to have the option of running dbs
separately.
> On 08 Feb 2016, at 22:02, Russell Bryant <russ...@ovn.org> wrote:
>
> 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
Michael Arnaldi - Chief Technology Officer (CTO)
MyMoneyEx Limited
c/o Buzzacott LLP
130 Wood Street
London EC2V 6DL - United Kingdom
Mail: michael.arna...@mymoneyex.com
Web: https://www.mymoneyex.com
--------------------------------------------------------------------------------------------
This message is private and confidential.
If you have received this message in error, please notify us and remove it from
your system.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
