Update entrypoint scripts and document to align with new CLI #1097

tiewei · 2017-12-05T20:41:24Z

Update entrypoint scripts for k8s, docker v2plugin to align with
new CLI, update documents, k8s config yaml.
Drive by update vagrantfile, kubeadm_test, utils/configs.go to make
codes more readable.

Signed-off-by: Wei Tie [email protected]

Update entrypoint scripts for k8s, docker v2plugin to align with new CLI, update documents, k8s config yaml. Drive by update vagrantfile, kubeadm_test, utils/configs.go to make codes more readable. Signed-off-by: Wei Tie <[email protected]>

tiewei · 2017-12-06T05:47:18Z

build PR

dseevr · 2017-12-06T07:09:49Z

install/v2plugin/README.md

 ```
 ### docker hub
 Developer release of v2plugin from contiv repo is also pushed to docker hub
+Please update mode, forward modemode, net mode according to your deployment.


modemode -> mode

dseevr · 2017-12-06T07:10:01Z

install/v2plugin/README.md

@@ -49,8 +73,11 @@ make demo-v2plugin

 ## Contiv plugin-roles
 Contiv plugin runs both netplugin and netmaster by default. Contiv v2plugin can be run with only netplugin by setting the plugin_role to worker.
+Please update mode, forward modemode, net mode according to your deployment.


modemode -> mode

dseevr · 2017-12-06T07:11:20Z

install/v2plugin/startcontiv.sh

-if [ "$log_dir" == "" ]; then
-    log_dir="/var/log/contiv"
+# setting up logs
+if [ ! -z "$CONTIV_LOG_DIR" ]; then


isn't ! -z "" == -n "" ? is it a concern ?

just a minor nitpick... you can avoid negation by using the opposing operator

[chrisplo:~] $ $ CONTIV_LOG_DIR=foo [chrisplo:~] $ $ if [ ! -z "$CONTIV_LOG_DIR" ]; then echo setting log dir; fi setting log dir [chrisplo:~] $ $ unset CONTIV_LOG_DIR [chrisplo:~] $ $ if [ ! -z "$CONTIV_LOG_DIR" ]; then echo setting log dir; fi [chrisplo:~] $

the logic is wrong I think, drop the !

dseevr · 2017-12-06T07:12:23Z

scripts/netContain/scripts/contivNet.sh


-if [ $cleanup == true ]; then
-	exit 0
+if [ ! -z "$CONTIV_MODE" ]; then


-n ?

(and the others here)

dseevr · 2017-12-06T07:14:12Z

scripts/netContain/scripts/contivNet.sh

-   mkdir -p /var/contiv/config
-   echo ${CONTIV_CONFIG} >/var/contiv/config/contiv.json
-   cp /var/contiv/config/contiv.json /opt/contiv/config/contiv.json
+if [ -d /var/contiv/log ]; then


Could both of these dirs even even exist at the same time? I think that implies both bindmounts are active... and in that case, the mv here is moving a bindmount onto another bindmount (gonna fail)

I think moving the contents will work, but moving the dir (possibly mountpoint) is not safe

Ah yeah I see these are both bindmounts... you'll need to mv the contents (or cp -r), not the directory (which is actually a mountpoint)

changed to use cp -a

unclejack · 2017-12-06T15:08:35Z

build PR

chrisplo · 2017-12-06T16:54:12Z

install/k8s/contiv/contiv.yaml

  # The location of your cluster store. This is set to the
  # avdertise-client value below from the contiv-etcd service.
  # Change it to an external etcd/consul instance if required.
-  cluster_store: "etcd://__NETMASTER_IP__:6666"
+  contiv_etcd: "http://__NETMASTER_IP__:6666"


were changing these three to have contiv_ necessary since we're in a contiv.yaml file? for example I see that calico doesn't namespace:
https://github.com/projectcalico/calico/blob/master/v2.6/getting-started/kubernetes/installation/hosted/calico.yaml#L19

same q for other deployments like contiv_devtest.yaml

it's just to make it more readable, it's useful when we split the yaml file to several yamls

chrisplo · 2017-12-06T17:01:57Z

install/v2plugin/README.md

-
+ARG                               : DESCRIPTION                                                               : DEFAULT VALUE
+----------------------------------:---------------------------------------------------------------------------:----------------------
+CONTIV_ROLE                       : contiv net service net, options: [netmaster, netplugin]                   : ""


why not default to netplugin? that should line up when we eventually pull out netmaster from the container . .

default to netplugin will not bring up netmaster, is it what we wanted ?

my thinking was that all nodes will be running netplugin (now and after we split)
the netmaster are special, as there are only 3 (or so) of them, and it seems to make more sense to tag the special ones?

chrisplo · 2017-12-06T17:06:58Z

install/v2plugin/README.md

+CONTIV_ROLE                       : contiv net service net, options: [netmaster, netplugin]                   : ""
+CONTIV_LOG_DIR                    : contiv log file directory                                                 : "/var/log/contiv"
+CONTIV_NETPLUGIN_CONSUL_ENDPOINTS : a comma-delimited list of netplugin consul endpoints                      : ""
+CONTIV_NETPLUGIN_ETCD_ENDPOINTS   : a comma-delimited list of netplugin etcd endpoints                        : ""


I'm sad we lost the etcd default, what if we:

default etcd to http://localhost:2379, consul to ""

if consul is set and etcd is default value, then use consul

if both explicitly set, error

that introduce extra layer in the startcontiv.sh and i'm trying to avoid it ...
we can default to use etcd http://127.0.0.1:2379 if none of them are set, but it needs to be in configs.go instead of startcontiv.sh. thought ?

if you meant "none" instead of "one" . . seems good, we can still document the default as etcd even if it's not done in startcontiv?

chrisplo · 2017-12-06T17:50:59Z

install/v2plugin/startcontiv.sh

-if [ "$log_dir" == "" ]; then
-    log_dir="/var/log/contiv"
+# setting up logs
+if [ ! -z "$CONTIV_LOG_DIR" ]; then


chrisplo · 2017-12-06T17:52:05Z

install/v2plugin/startcontiv.sh

-if [[ "$cluster_store" =~ ^etcd://.+ ]]; then
-    store_arg="--etcd-endpoints $(echo $cluster_store | sed s/etcd/http/)"
+mkdir -p /var/run/openvswitch
+mkdir -p /var/log/contiv/


not blocking, mkdir can take multiple args I think

chrisplo · 2017-12-06T17:58:21Z

install/v2plugin/startcontiv.sh

+             --private-key=db:Open_vSwitch,SSL,private_key \
+             --certificate=db:Open_vSwitch,SSL,certificate \
+             --bootstrap-ca-cert=db:Open_vSwitch,SSL,ca_cert \
+             --log-file=/var/log/contiv/ovs-db.log -vsyslog:info -vfile:info \


shouldn't vsyslog and vfile be based on the CLI setting?

not for ovs log, there was a PR to set to use info level specifically, this is just to make it align between docker and k8s

chrisplo · 2017-12-06T18:01:48Z

install/v2plugin/startcontiv.sh

+        set -x
+        /contiv/bin/netmaster "$@" &> "$CONTIV_LOG_DIR/netmaster.log"
+        set +x
+        echo "ERROR: Contiv netmaster has exited, restarting in 5s"


did log rotation happen elsewhere now?

no, there's no log rotation today

you removed it:
https://github.com/contiv/netplugin/pull/1097/files/e4106dff4e2aa6b0d32b3db7a8481e239c153ffe#diff-c076b59cd170dc6557cfc0f83d1e5528L84

chrisplo · 2017-12-06T18:19:03Z

install/v2plugin/startcontiv.sh

-    if [ "$i" -ge "10" ]; then
-        echo "netmaster port not open (needed to set forwarding mode), plugin failed"
-        exit 1
-    fi


why are we getting rid of the wait for the listen port?

b/c the fwd config are in passed CLI, don't have to wait for it's ready

chrisplo · 2017-12-06T18:31:59Z

scripts/netContain/scripts/ovsInit.sh

@@ -1,14 +1,15 @@
 #!/bin/bash
 #Start OVS in the Contiv container

-set -euo pipefail


the modprobe will not trigger pipefail, leave here?

#!/bin/bash set -eo pipefail (exit 1) || echo i am fine

chrisplo · 2017-12-06T18:35:15Z

test/systemtests/kubeadm_test.go

@@ -554,7 +554,8 @@ func (k *kubePod) startNetmaster(args string) error {
 	if k.node.suite.basicInfo.AciMode == "on" {
 		infraType = " --infra aci "
 	}
-	netmasterStartCmd := k.node.suite.basicInfo.BinPath + `/netmaster` + infraType + k.commonArgs() + args + ` > ` + netmasterLogLocation + ` 2>&1`
+	// unset CONTIV_NETMASTER_ETCD_ENDPOINTS to be able to use other endpoint
+	netmasterStartCmd := "unset CONTIV_NETMASTER_ETCD_ENDPOINTS && " + k.node.suite.basicInfo.BinPath + `/netmaster` + infraType + k.commonArgs() + args + ` > ` + netmasterLogLocation + ` 2>&1`


wrapping on these 2 plz?

tiewei · 2017-12-06T20:07:08Z

build PR

unclejack · 2017-12-06T20:21:34Z

build PR

This commit make netplugin and netmaster to use etcd at http://127.0.0.1:2379 if neither etcd or consul endpoints are provided. Also make v2plugin by default as netplugin role. Signed-off-by: Wei Tie <[email protected]>

tiewei · 2017-12-07T00:03:32Z

build PR

chrisplo · 2017-12-07T18:17:55Z

install/v2plugin/startcontiv.sh

-if [ "$log_dir" == "" ]; then
-    log_dir="/var/log/contiv"
+# setting up logs
+if [ ! -z "$CONTIV_LOG_DIR" ]; then


[chrisplo:~] $ $ CONTIV_LOG_DIR=foo [chrisplo:~] $ $ if [ ! -z "$CONTIV_LOG_DIR" ]; then echo setting log dir; fi setting log dir [chrisplo:~] $ $ unset CONTIV_LOG_DIR [chrisplo:~] $ $ if [ ! -z "$CONTIV_LOG_DIR" ]; then echo setting log dir; fi [chrisplo:~] $

the logic is wrong I think, drop the !

tiewei · 2017-12-07T18:27:15Z

build PR

chrisplo · 2017-12-07T18:33:05Z

install/k8s/contiv/contiv_devtest.yaml

@@ -14,7 +14,9 @@ data:
  # The location of your cluster store. This is set to the
  # avdertise-client value below from the contiv-etcd service.
  # Change it to an external etcd/consul instance if required.
-  contiv_etcd: "http://__NETMASTER_IP__:6666"
+  # this is not required for dev test, etcd or consul endpoints


i read this like A, B, or C . . . maybe dev test: etcd or ...

chrisplo

~~i think there is a README or two that need the updated parameter for netmaster~~ i found you did that already

problem was fixed

unclejack · 2017-12-07T20:53:19Z

build PR

tiewei · 2017-12-07T23:01:51Z

build PR

tiewei · 2017-12-08T05:09:58Z

build PR

dseevr reviewed Dec 6, 2017

View reviewed changes

don't mount contiv scripts dir

17c06d2

chrisplo reviewed Dec 6, 2017

View reviewed changes

Wei Tie added 2 commits December 6, 2017 15:39

Update on review

6f997d9

Set default to use etcd at http://127.0.0.1:2379

6ba7a7e

This commit make netplugin and netmaster to use etcd at http://127.0.0.1:2379 if neither etcd or consul endpoints are provided. Also make v2plugin by default as netplugin role. Signed-off-by: Wei Tie <[email protected]>

chrisplo previously requested changes Dec 7, 2017

View reviewed changes

Fix CONTIV_LOG_DIR condition

773186c

chrisplo reviewed Dec 7, 2017

View reviewed changes

chrisplo approved these changes Dec 7, 2017

View reviewed changes

dseevr approved these changes Dec 8, 2017

View reviewed changes

tiewei merged commit 38d2d9f into contiv:master Dec 8, 2017

Update entrypoint scripts and document to align with new CLI #1097

Update entrypoint scripts and document to align with new CLI #1097

Conversation

tiewei commented Dec 5, 2017

tiewei commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unclejack commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiewei Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiewei commented Dec 6, 2017

unclejack commented Dec 6, 2017

tiewei commented Dec 7, 2017

Choose a reason for hiding this comment

tiewei commented Dec 7, 2017

Choose a reason for hiding this comment

chrisplo left a comment • edited Loading

Choose a reason for hiding this comment

unclejack commented Dec 7, 2017

tiewei commented Dec 7, 2017

tiewei commented Dec 8, 2017

tiewei Dec 6, 2017 •

edited

Loading

chrisplo left a comment •

edited

Loading