netmaster cli refactor #1088

tiewei · 2017-11-28T23:17:01Z

Refactor netmaster cli to use "github.com/urfave/cli", it brings
benefit like setting forwarding mode, infra type and network mode,
allows setting multi etcd/consul endpoints and using SSL, enabling
syslog,json log and multi log levels.

Signed-off-by: Wei Tie wtie@cisco.com

tiewei · 2017-11-29T01:50:09Z

TODO: update container entrypoint scripts

dseevr · 2017-11-29T22:38:34Z

build pr

tiewei · 2017-11-29T23:10:59Z

build PR

dseevr

Nice work!

dseevr · 2017-11-29T23:00:58Z

netmaster/main.go

+			Name:   "infra, infra-type",
+			Value:  "default",
+			EnvVar: "CONTIV_NETMASTER_PLUGIN_HOST",
+			Usage:  "set netmaster infra tyoe, options [aci, default]",


typo: tyoe -> type

dseevr · 2017-11-29T23:12:08Z

netmaster/main.go


-	if err := parseOpts(&opts); err != nil {
-		log.Fatalf("Failed to parse cli options. Error: %s", err)
+	internalAddress := ctx.String("interal-address")


typo: interal -> internal

dseevr · 2017-11-29T23:12:16Z

netmaster/main.go

-		log.Fatalf("Failed to parse cli options. Error: %s", err)
+	internalAddress := ctx.String("interal-address")
+	if internalAddress == "" {
+		return nil, errors.New("netmaster interal-address is not set")


typo: interal -> internal

dseevr · 2017-11-29T23:12:24Z

netmaster/main.go

 	}

-	// execute options
-	execOpts(&opts)
+	logrus.Infof("Using netmaster interal-address: %s", internalAddress)


typo: interal -> internal

dseevr · 2017-11-29T23:14:30Z

netmaster/main.go

+}
+
+func main() {
+	localIP, _ := netutils.GetDefaultAddr()


Check error here

dseevr · 2017-11-29T23:19:31Z

netplugin/netd.go

-	if networkMode == "vxlan" && vtepIP == "" {
-		return plugin.Config{}, fmt.Errorf("vtep-ip should be set when using VXLAN mode")
+	if netConfigs.NetworkMode == "vxlan" && vtepIP == "" {
+		return nil, fmt.Errorf("vtep-ip should be set when using VXLAN mode")


should -> must

dseevr · 2017-11-29T23:19:38Z

netplugin/netd.go

-	if networkMode == "vlan" && len(vlanUpLinks) == 0 {
-		return plugin.Config{}, fmt.Errorf("vlan-uplinks should be set when using VLAN mode")
+	if netConfigs.NetworkMode == "vlan" && len(vlanUpLinks) == 0 {
+		return nil, fmt.Errorf("vlan-uplinks should be set when using VLAN mode")


should -> must

dseevr · 2017-11-29T23:25:28Z

test/integration/integ_test.go

@@ -53,7 +54,8 @@ func TestMain(m *M) {
 	flag.StringVar(&integ.arpMode, "arp-mode", "proxy", "ARP mode [ proxy | flood ]")
 	flag.StringVar(&integ.encap, "encap", "vlan", "Encap [ vlan | vxlan ]")
 	flag.StringVar(&integ.fabricMode, "fabric-mode", "default", "fabric-mode [ aci | default ]")
-	flag.StringVar(&integ.clusterStore, "cluster-store", "etcd://127.0.0.1:2379", "Cluster store URL")
+	flag.StringVar(&integ.clusterStoreDriver, "cluster-store-driver", "etcd", "Cluster store dirver")


typo: dirver -> driver

dseevr · 2017-11-29T23:31:28Z

utils/configs.go

+
+	hook, err = logrus_syslog.NewSyslogHook(syslogURL.Scheme, syslogURL.Host, priority, binary)
+	if err != nil {
+		return fmt.Errorf("Failed connectting to syslog %q: %v", syslogRawURL, err.Error())


typo: connectting -> connecting

dseevr · 2017-11-29T23:35:38Z

utils/netutils/netutils.go

+	port, err := strconv.Atoi(addr[1])
+	if err != nil {
+		return fmt.Errorf("bind port is a integer, got %v", port)
+	}


Maybe validate that the port is >= 1 and <= 65535 here?

(I don't think we need to support listening on port 0 lol)

dseevr · 2017-11-29T23:38:22Z

@tiewei How do you feel about raising an error if etcd and consul endpoints are set? Right now it says "consul endpoints are ignored if etcd endpoints are set"... if both of them are set, we can't really determine what the user intent was IMO. I think we should just explode in that case.

tiewei · 2017-11-30T00:02:41Z

@chrisplo mentioned that as well in last commit ... i'll error it out then

dseevr · 2017-11-30T05:58:26Z

netmaster/main.go

@@ -161,7 +160,9 @@ func main() {
 		if err != nil {
 			errmsg := err.Error()
 			logrus.Error(errmsg)
-			return cli.NewExitError(errmsg, (len(errmsg)%254 + 1))
+			// use 22 Invalid argument as error return code
+			// http://www-numi.fnal.gov/offline_software/srt_public_context/WebDocs/Errors/unix_system_errors.html


Refactor netmaster cli to use "github.com/urfave/cli", it brings benefit like setting forarding mode, infra type and network mode, allows setting multi etcd/consul endpoints and using SSL, enabling syslog,json log and multi log levels. Signed-off-by: Wei Tie <wtie@cisco.com>

tiewei · 2017-12-02T01:20:20Z

build PR

unclejack · 2017-12-02T09:09:33Z

What release is this meant to be included in? It'd be nice to deprecate the old flags instead of removing them. This should make it easier if we make a 1.2 release which is still backwards compatible and some users upgrade an existing setup. What do you think?

tiewei · 2017-12-04T17:53:08Z

Most old flag was kept except --cluster-store due to the schema of url change, yes 1.2 would be ideal for this type of changes

tiewei · 2017-12-05T02:10:39Z

netmaster/main.go

+		cli.StringFlag{
+			Name:   "infra, infra-type",
+			Value:  "default",
+			EnvVar: "CONTIV_NETMASTER_PLUGIN_HOST",


should be CONTIV_NETMASTER_INFRA

tiewei · 2017-12-05T02:11:09Z

netmaster/main.go

+		cli.StringFlag{
+			Name:   "name, plugin-name",
+			Value:  "netplugin",
+			EnvVar: "CONTIV_NETMASTER_PLUGIN_HOST",


should be CONTIV_NETMASTER_PLUGIN_NAME

chrisplo

3/4 done, some of the below I'd strongly suggest but they probably all slide under non-blocking

chrisplo · 2017-12-04T22:06:10Z

netmaster/daemon/daemon.go

+	apiConfig := &objApi.APIControllerConfig{
+		NetForwardMode: d.NetForwardMode,
+		NetInfraType:   d.NetInfraType,
+	}


AFACT this isn't supposed to be modifiable, why not have NewAPIController accept APIControllerConfig by value?

this object only got used here (and integ testings), I used object was because of intending to add more parameters from configs .

have NewAPIController accept APIControllerConfig by value

this is troublesome in golang, especially when having the parameters in different type

i think the struct is fine, just dunno why you need a pointer to it

chrisplo · 2017-12-04T22:37:44Z

netmaster/daemon/daemon.go

 	if err != nil {
-		log.Fatalf("Error connecting to state store: %v. Err: %v", d.ClusterStore, err)
+		log.Fatalf("Error connecting to state store: driver %q, URLs %q. Err: %v", d.ClusterStoreDriver, d.ClusterStoreURL, err)
 	}


i think we should check for ':' in ControlURL and ListenURL since those are parsed later in startListeners

it actually got checked in the CLI part

ah I see it now

chrisplo · 2017-12-04T22:57:34Z

netmaster/daemon/daemon.go

+		internalAddr := strings.Split(d.ControlURL, ":")
+		if externalAddr[0] == "0.0.0.0" && externalAddr[1] == internalAddr[1] {
+			// ignore internal bind if external and internal are on the same port and external bind on 0.0.0.0
+			log.Infof("Ignore creating API listener on %q because %q covers it", d.ControlURL, d.ListenURL)


seems we could have external and internal on same IP as well?
externalAddr[1] == internalAddr[1] && (externalAddr[0] == "0.0.0.0" || externalAddr[0] == internalAddr[0])

if they are on the same ip but different port will go to else where create a listener on different port with same ip. Only got ignored when they have overlapping

oh nvm, I wasn't looking at the outer if restriction

chrisplo · 2017-12-05T00:20:54Z

utils/configs.go

+
+// FlattenFlags concatenate slices of flags into one slice
+func FlattenFlags(flagSlices ...[]cli.Flag) []cli.Flag {
+	flags := flagSlices[0]


start with flags := [] then range starting at 0

or, I think you can just do instead of this function:
append(flags, flagsA..., flagsB...) where the ... is a literal
https://stackoverflow.com/a/16248257/1322463
https://golang.org/ref/spec#Passing_arguments_to_..._parameters

i don't think golang allows more than one slice parameter ( it only works for the last parameter)

and here, i thought i was on to something . . ok fine, though now I wonder if this could be ...[]interface{} arg and be a general purpose

Anyway . . . I'm back to first comment, start range at 0 so that you can handle 0 length input?

chrisplo · 2017-12-05T00:24:18Z

utils/configs.go

+		cli.BoolFlag{
+			Name:   "use-syslog, syslog",
+			EnvVar: fmt.Sprintf("CONTIV_%s_USE_SYSLOG", binUpper),
+			Usage:  fmt.Sprintf("set %s send log to syslog or not", binLower),


maybe say what true value does to be more explicit?
true value sends logs to syslog

i think the parameter is pretty self-explained ... --use-syslog is actually not used as --use-syslog=True , it is if flag --use-syslog is provided, this var is treated as True.
I'll update the Usage to make it more clear

chrisplo · 2017-12-05T02:01:53Z

scripts/python/api/tnode.py

-        command = "sudo " + self.binpath + "/netplugin --vlan-if eth2 --vlan-if eth3 " + fwd_arg + fwd_arg + store_args + args + "> /tmp/netplugin.log 2>&1"
+        # NOTE: this testing only used in mesos-docker
+        mode_args = " --fwdmode bridge --netmode vlan --mode docker "
+        command = "sudo " + self.binpath + "/netplugin --vlan-if eth2 --vlan-if eth3 " + mode_args + gen_cluster_store_args() + args + "> /tmp/netplugin.log 2>&1"


i'd make it in different PR, codes under systest are pretty sick

chrisplo · 2017-12-05T02:13:30Z

test/systemtests/docker_test.go

 	} else {
-		storeArgs = " --consul-endpoints " + strings.Replace(cStore, "consul", "http", 1) + " "
+		storeArgs = " --consul-endpoints " + d.node.suite.basicInfo.ClusterStoreURLs + " "


d.node.suite.basicInfo.ClusterStoreURLs + " " could be added to storeArgs outside the if block, not asking for change, just sayin

chrisplo · 2017-12-05T02:16:50Z

test/systemtests/k8setup_test.go

+		storeArgs = " --consul-endpoints " + k.node.suite.basicInfo.ClusterStoreURLs + " "
+	}
+	return " --netmode " + netMode + " --fwdmode " + fwdMode + " --mode " + mode + storeArgs
+}


is the only diff with docker setup tset is the mode? reuse?

it's due to the way how our sys testing was done, k8setpup_test.go and docker_test.go are separated scheduler and codes between them are not able to share, to fix this we need to introduce a new util and generates shared codes ... i'd leave it in different PR

chrisplo · 2017-12-05T02:17:25Z

test/systemtests/kubeadm_test.go

+	}
+	return " --netmode " + netMode + " --fwdmode " + fwdMode + " --mode " + mode + storeArgs
+}
+


chrisplo · 2017-12-05T02:27:37Z

test/systemtests/kubeadm_test.go

+	var podName string
+	for retry := 60; retry > 0; retry-- {
+		// only get running pods name
+		podNameCmd := `kubectl -n kube-system get pods -o wide | grep Running | grep ` + podRegex + ` | grep ` + nodeName + ` | cut -d " " -f 1`


maybe there is a better kubectl query for this? dunno if this helps, there is also all that json path stuff

kubectl get pods --field-selector=status.phase=Running

yeah this is better, what i did this this line is just added grep Running piece ... I'd add it into the systest improvement PR

I think this is not supported until 1.9
kubernetes/kubernetes#50140

reviewing own comments, none are truly blockers

Signed-off-by: Wei Tie <nuaafe@gmail.com>

tiewei · 2017-12-05T06:48:03Z

Build PR

chrisplo · 2017-12-05T16:39:14Z

test/systemtests/swarm_test.go

 	logrus.Infof("Starting netplugin on %s", w.node.Name())
-
-	cmd := "sudo " + w.node.suite.basicInfo.BinPath + "/netplugin --netmode " + netMode + " --fwdmode " + fwdMode + " --plugin-mode docker --vlan-if " + w.node.suite.hostInfo.HostDataInterfaces + storeArgs + args + "&> /tmp/netplugin.log"
+	cmd := "sudo " + w.node.suite.basicInfo.BinPath + "/netplugin --vlan-if " + w.node.suite.hostInfo.HostDataInterfaces + w.commonArgs() + args + "&> /tmp/netplugin.log"


chrisplo · 2017-12-05T16:39:19Z

test/systemtests/swarm_test.go

+	if w.node.suite.basicInfo.AciMode == "on" {
+		infraType = " --infra aci "
+	}
+	return w.node.tbnode.RunCommandBackground("sudo " + w.node.suite.basicInfo.BinPath + "/netmaster" + infraType + w.commonArgs() + " &> /tmp/netmaster.log")


chrisplo · 2017-12-05T16:56:12Z

utils/configs.go

+
+// FlattenFlags concatenate slices of flags into one slice
+func FlattenFlags(flagSlices ...[]cli.Flag) []cli.Flag {
+	flags := flagSlices[0]


and here, i thought i was on to something . . ok fine, though now I wonder if this could be ...[]interface{} arg and be a general purpose

Anyway . . . I'm back to first comment, start range at 0 so that you can handle 0 length input?

chrisplo · 2017-12-05T16:59:26Z

vagrant/mesos-cni/Vagrantfile

-if [[ $# -gt 9 ]] && [[ $10 != "" ]]; then
-    shift; shift; shift; shift; shift; shift; shift; shift; shift
+if [[ $# -gt 10 ]] && [[ ${11} != "" ]]; then
+    shift; shift; shift; shift; shift; shift; shift; shift; shift; shift


please use shift 10 instead

chrisplo · 2017-12-05T17:01:04Z

vagrant/mesos-cni/Vagrantfile

@@ -166,7 +167,9 @@ Vagrant.configure(VAGRANTFILE_API_VERSION) do |config|
            end
            node.vm.provision "shell" do |s|
                s.inline = provision_common_once
-                s.args = [node_name, node_addr, cluster_ip_nodes, ENV["http_proxy"] || "", ENV["https_proxy"] || "", ENV["NIGHTLY_RELEASE"] || "", ENV["CONTIV_CLUSTER_STORE"] || "etcd://localhost:2379", ENV["CONTIV_DOCKER_VERSION"] || "", ENV['CONTIV_NODE_OS'] || "", *ENV['CONTIV_ENV']]
+                s.args = [node_name, node_addr, cluster_ip_nodes, ENV["http_proxy"] || "", ENV["https_proxy"] || "", ENV["NIGHTLY_RELEASE"] || "", \


more than 80 here?

chrisplo · 2017-12-05T17:02:29Z

vagrant/nomad-docker/Vagrantfile

-if [[ $# -gt 9 ]] && [[ $10 != "" ]]; then
-    shift; shift; shift; shift; shift; shift; shift; shift; shift
+if [[ $# -gt 10 ]] && [[ ${11} != "" ]]; then
+    shift; shift; shift; shift; shift; shift; shift; shift; shift; shift


chrisplo · 2017-12-05T17:11:47Z

netmaster/daemon/daemon.go

+		internalAddr := strings.Split(d.ControlURL, ":")
+		if externalAddr[0] == "0.0.0.0" && externalAddr[1] == internalAddr[1] {
+			// ignore internal bind if external and internal are on the same port and external bind on 0.0.0.0
+			log.Infof("Ignore creating API listener on %q because %q covers it", d.ControlURL, d.ListenURL)


oh nvm, I wasn't looking at the outer if restriction

chrisplo · 2017-12-05T17:15:15Z

utils/configs.go

+		cli.BoolFlag{
+			Name:   "use-json-log, json-log",
+			EnvVar: fmt.Sprintf("CONTIV_%s_USE_JSON_LOG", binUpper),
+			Usage:  fmt.Sprintf("set %s log format to json if this flag is provided", binLower),


this is OK, but might be easier to read "format logs with json"

chrisplo · 2017-12-05T17:15:42Z

utils/configs.go

+		cli.BoolFlag{
+			Name:   "use-syslog, syslog",
+			EnvVar: fmt.Sprintf("CONTIV_%s_USE_SYSLOG", binUpper),
+			Usage:  fmt.Sprintf("set %s send log to syslog if this flag is provided", binLower),


this is OK, but might be easier to read "send logs to syslog"

chrisplo · 2017-12-05T17:15:47Z

utils/configs.go

+		cli.BoolFlag{
+			Name:   "use-syslog, syslog",
+			EnvVar: fmt.Sprintf("CONTIV_%s_USE_SYSLOG", binUpper),
+			Usage:  fmt.Sprintf("set %s send log to syslog or not", binLower),


chrisplo · 2017-12-05T17:20:18Z

overall substance looks good, just some style nits

dseevr requested changes Nov 29, 2017

View reviewed changes

dseevr reviewed Nov 30, 2017

View reviewed changes

dseevr approved these changes Dec 1, 2017

View reviewed changes

Wei Tie and others added 4 commits November 30, 2017 21:58

Address review comments

7cd63b4

Update README message

d851259

Wait up to 5 mins on getting pod name of running pods

23636b2

tiewei changed the title ~~WIP: netmaster cli refactor~~ netmaster cli refactor Dec 2, 2017

dseevr requested review from kahou82 and chrisplo December 2, 2017 06:39

tiewei commented Dec 5, 2017

View reviewed changes

chrisplo previously requested changes Dec 5, 2017

View reviewed changes

Polish CLI helpers

eef09ad

Signed-off-by: Wei Tie <nuaafe@gmail.com>

chrisplo reviewed Dec 5, 2017

View reviewed changes

chrisplo approved these changes Dec 5, 2017

View reviewed changes

tiewei merged commit 43d7b3f into contiv:master Dec 5, 2017

netmaster cli refactor #1088

netmaster cli refactor #1088

Conversation

tiewei commented Nov 28, 2017 • edited Loading

tiewei commented Nov 29, 2017

dseevr commented Nov 29, 2017

tiewei commented Nov 29, 2017

dseevr left a 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

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

dseevr commented Nov 29, 2017

tiewei commented Nov 30, 2017

Choose a reason for hiding this comment

tiewei commented Dec 2, 2017

unclejack commented Dec 2, 2017

tiewei commented Dec 4, 2017

Choose a reason for hiding this comment

tiewei Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

chrisplo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiewei Dec 5, 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

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 5, 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

chrisplo commented Dec 5, 2017

tiewei commented Nov 28, 2017 •

edited

Loading

tiewei Dec 5, 2017 •

edited

Loading

chrisplo left a comment •

edited

Loading

tiewei Dec 5, 2017 •

edited

Loading