Init global settings from object store #1043

tiewei · 2017-11-01T04:45:50Z

Currently netplugin is trying to init from default settings if global
setting is not ready or wrong, and when the settings don't match it
will re-init netplugin. This is very easy to go wrong when there are
resources created before global settings are ready.
This commit makes netplugin wait until the global settings ready, and
initiate itself according to it. If the global settings are mis-match,
it will return error.

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

kahou82 · 2017-11-01T05:14:49Z

Can you add unit test?

kahou82 · 2017-11-01T05:15:04Z

netplugin/plugin/netplugin.go

-	InitGlobalSettings(p.StateDriver, &pluginConfig.Instance)
+	err = InitGlobalSettings(p.StateDriver, &pluginConfig.Instance)
+	if err != nil {
+		return err


Also if global setting != instant setting, are we going to do anything?

it will be handled here

// Init the driver plugins.. err = netPlugin.Init(*pluginConfig) if err != nil { log.Fatalf("Failed to initialize the plugin. Error: %s", err) }

which will exit(1) after log fatal

kahou82 · 2017-11-01T05:18:05Z

netplugin/plugin/netplugin.go

+	} else {
+		if inst.FwdMode != gCfg.FwdMode {
+			logrus.Errorf("Global forwarding mode setting doesn't match local settings, global=%v , local=%v", gCfg.FwdMode, inst.FwdMode)
+			return core.Errorf("Global forwarding mode setting doesn't match local settings, global=%v , local=%v", gCfg.FwdMode, inst.FwdMode)


Can we use fmt.Errorf and assign it to a variable instead of having two duplicated string?

it's actually every odd we had core.Errorf and its logic .... yeah I will change to use fmt.Errorf , but codes with core.Errorf has been used in many places through the project

@dseevr ^ we need to pick one way or the other and keep it consistent

or use errors pkg

We should get rid of core.Error entirely IMO. We don't need the stack trace stuff in it.

Based on the people who wrote it (erik/madhav) and the dates, it looks like it was a precursor to errored.

I'll add an issue for this tomorrow

I'd say let's use fmt.Errorf() or errors.New() for now. As Kahou said, let's also avoid duplicating strings. There's a lot of those that need to be cleaned up in the codebase(s)...

kahou82 · 2017-11-01T05:18:45Z

netplugin/plugin/netplugin.go

+			} else {
+				logrus.Errorf("Error reading private subnet cluster store")
+			}
+


extra space

(blank line)

kahou82 · 2017-11-01T05:21:03Z

netplugin/plugin/netplugin.go

+				logrus.Errorf("Error reading forwarding mode from cluster store")
+			} else {
+				logrus.Errorf("Error reading private subnet cluster store")
+			}


Shouldn't it be:
if gCfg.FwdMode == "" { logrus.Warnf }
if gCfg.PvtSubnet == "" { logrus.Warnf}

kahou82 · 2017-11-01T05:22:03Z

netplugin/plugin/netplugin.go

-		inst.HostPvtNW = net
-		logrus.Infof("HostPvtNW: %v", net)
+		logrus.Errorf("Error convert private subnet %v from CIDR to mask, error %v", gCfg.PvtSubnet, err)
+		return core.Errorf("Error convert private subnet %v from CIDR to mask, error %v", gCfg.PvtSubnet, err)


Can we use fmt.Errorf and assign it to a variable instead of having two duplicated string?

tiewei · 2017-11-01T06:31:31Z

UT will be followed

dseevr · 2017-11-01T07:49:01Z

netplugin/plugin/netplugin.go

+
+	/*
+		Query global settings from state store
+		1. if forward mode or private net is empty, retry 1 sec


"retry after 1 second"

dseevr · 2017-11-01T07:52:12Z

netplugin/plugin/netplugin.go

+
+	// wait until able to get fwd mode and private subnet
+	for {
+		err := gCfg.Read("")


If the error is only checked immediately following the function call, it's Go convention to just check it inline in the if statement:

if err := gCfg.Read(""); err != nil { logrus.Errorf("Error reading global settings from cluster store") }

This style of assignment is also okay to use even if there is an existing error object in the current scope. You can always use := and the err var does not leak out into the function scope.

dseevr · 2017-11-01T07:52:25Z

netplugin/plugin/netplugin.go

+			} else {
+				logrus.Errorf("Error reading private subnet cluster store")
+			}
+


(blank line)

dseevr · 2017-11-01T07:52:40Z

netplugin/plugin/netplugin.go

+			}
+
+		} else {
+			logrus.Infof("Get global forwarding mode: %v", gCfg.FwdMode)


rchirakk · 2017-11-01T17:10:26Z

netplugin/plugin/netplugin.go

@@ -50,8 +51,6 @@ type NetPlugin struct {
 	PluginConfig  Config
 }

-const defaultPvtSubnet = 0xac130000


if you are removing the default svc net,
can you add it in netmaster/netctl so that it uses this svc subnet by default ?

i think it does, i'll add more stuff on the netmaster side after this PR, it currently still having default value to init global settings as bridge instead of wait until it set

rchirakk · 2017-11-01T17:11:37Z

netplugin/plugin/netplugin.go

+	for {
+		err := gCfg.Read("")
+		if err != nil {
+			logrus.Errorf("Error reading global settings from cluster store")


sleep & continue ?

improved in other way

rchirakk · 2017-11-01T17:16:20Z

netplugin/plugin/netplugin.go

+	if inst.FwdMode == "" {
+		inst.FwdMode = gCfg.FwdMode
+	} else {
+		if inst.FwdMode != gCfg.FwdMode {


this check should be in netmaster/netctl.
netplugin being a worker node, it should accept the value from the netmaster

technically local fwd mode will always be empty or the one get from kv store with this code, it doesn't hurt if anything wired happens here

currently inst.FwdMode is "" because during init there's no default value set

tiewei · 2017-11-03T00:47:04Z

test case added

unclejack · 2017-11-03T09:14:24Z

build PR

tiewei · 2017-11-03T17:51:05Z

build PR

chrisplo · 2017-11-03T17:26:14Z

netplugin/plugin/netplugin.go

-	} else {
-		inst.HostPvtNW = net
-		logrus.Infof("HostPvtNW: %v", net)
+		err := fmt.Errorf("Error convert private subnet %v from CIDR to mask, error %v", gCfg.PvtSubnet, err)


nit: convert->converting, net->netmask, colon at the end is nice to delineate the error message

chrisplo · 2017-11-03T17:30:04Z

netplugin/plugin/netplugin.go

 	}
+	inst.HostPvtNW = net
+	return nil
 }


we lost the message of what we initialized the Private network to

tiewei · 2017-11-03T23:29:04Z

build PR

tiewei · 2017-11-03T23:33:57Z

build PR

rchirakk · 2017-11-04T00:29:40Z

netplugin/plugin/netplugin.go

+	if inst.FwdMode == "" {
+		inst.FwdMode = gCfg.FwdMode
+	} else {
+		if inst.FwdMode != gCfg.FwdMode {


Can this happen ever here ?

it depends, if we moves the fwd mode into the CLI input for netplugin, it will, but not today

rchirakk · 2017-11-04T00:30:34Z

netmaster/objApi/apiController.go

@@ -116,7 +116,7 @@ func NewAPIController(router *mux.Router, objdbClient objdb.API, storeURL string
 			NetworkInfraType: "default",
 			Vlans:            "1-4094",
 			Vxlans:           "1-10000",
-			FwdMode:          "bridge",
+			FwdMode:          "", // set empty fwd mode by default


can be removed, variables are initialized by default

i would leave as empty string here and in next commit to make it accept from CLI, and I think defined explicitly is always better

tiewei · 2017-11-06T22:31:10Z

build PR

tiewei · 2017-11-07T00:00:20Z

build PR

tiewei · 2017-11-07T00:13:20Z

build PR

tiewei · 2017-11-07T00:54:08Z

build PR

chrisplo · 2017-11-07T05:10:37Z

utils/netutils/netutils.go

@@ -921,23 +921,16 @@ func HostIPToGateway(hostIP string) (string, error) {
 	return ip[0] + "." + ip[1] + ".255.254", nil
 }

-// CIDRToMask converts a mask to corresponding netmask
+// CIDRToMask converts a CIRD to corresponding network number


chrisplo · 2017-11-07T05:13:26Z

test/systemtests/util_test.go

+			ArpMode:          "proxy",
+			PvtSubnet:        "172.19.0.0/16",
+		}), IsNil)
+		time.Sleep(40 * time.Second)


since everything but the FwdMode string is the same, just make that a variable set via an if else and pass in the variable?

chrisplo · 2017-11-07T05:14:05Z

test/systemtests/util_test.go

+			ArpMode:          "proxy",
+			PvtSubnet:        "172.19.0.0/16",
+		}), IsNil)
+		time.Sleep(120 * time.Second)


same as above, and why 2 minute sleep vs 40s as above?

i guess it was because it's faster on metal server, the sleep time was in old code as well

chrisplo · 2017-11-07T05:15:52Z

test/integration/npcluster_test.go

@@ -86,20 +86,18 @@ func NewNPCluster(its *integTestSuite) (*NPCluster, error) {
 	time.Sleep(10 * time.Second)

 	// set forwarding mode if required


it's always required, no longer if :)

tiewei · 2017-11-07T06:10:48Z

build PR

kahou82 · 2017-11-07T15:19:05Z

netplugin/plugin/netplugin_test.go

+                       "state": "fakedriver",
+                       "container": "docker",
+                    },
+					"plugin-instance": {


kahou82 · 2017-11-07T15:19:34Z

netplugin/plugin/netplugin_test.go

 func TestNetPluginInit(t *testing.T) {
+	initFakeStateDriver(t)


Document what you are testing?

kahou82 · 2017-11-07T15:19:45Z

netplugin/plugin/netplugin_test.go

+}
+
+func TestNetPluginInitInvalidConfigInvalidPrivateSubnet(t *testing.T) {
+	initFakeStateDriver(t)


Document what you are testing?

kahou82 · 2017-11-07T15:20:28Z

test/integration/npcluster_test.go

+	})
+
+	if err != nil {
+		log.Fatalf("Error creating global state. Err: %v", err)


exit if it fails?

it was doing so, you want me to change the behavior ?

kahou82 · 2017-11-07T15:20:49Z

test/systemtests/util_test.go

-	}
+
+	s.setGlobalSettings(c, s.fwdMode)
+	time.Sleep(40 * time.Second)


why 40 seconds?

it was set to 40 sec on baremetal testing and 120 sec on vagrant case, this magic number comes from previous testing, i think

kahou82 · 2017-11-07T15:21:00Z

test/systemtests/util_test.go

@@ -1438,21 +1442,12 @@ func (s *systemtestSuite) SetUpTestVagrant(c *C) {
 		}
 	}

+	s.setGlobalSettings(c, s.fwdMode)
+	time.Sleep(120 * time.Second)


same here, why 120 seconds?

chrisplo · 2017-11-07T22:43:05Z

netplugin/plugin/netplugin.go

-			return err
-		}
-	}
+	inst.FwdMode = gCfg.FwdMode


since it looks like we are going all in with config from etcd, should we warn if inst.FwdMode is configured whever that is created?

there is no chance it would be configured though ... this function only got called when the netplugin starts, and we are not providing a way to set the fwdmode today

tiewei · 2017-11-07T23:32:16Z

build PR

tiewei · 2017-11-07T23:42:33Z

netplugin/plugin/netplugin_test.go

-					   "db-url": "etcd://127.0.0.1:4001"
-                    }
-                  }`
+					"drivers" : {


This is due to tabs before the chars, if we think spaces are better, i can switch it

tiewei · 2017-11-08T06:40:58Z

@dseevr @kahou82 @unclejack @chrisplo please let me know if it meets the comments

rchirakk · 2017-11-08T07:26:23Z

netplugin/plugin/netplugin.go

+		} else {
+			if gCfg.FwdMode == "" || gCfg.PvtSubnet == "" {
+				if gCfg.FwdMode == "" {
+					logrus.Warnf("Error reading forwarding mode from cluster store")


cosmetic: this is not read error but waiting for config.
to dump all elements in struct xxxf("%+v", gCfg)

rchirakk · 2017-11-08T07:28:13Z

netplugin/plugin/netplugin.go

+	// wait until able to get fwd mode and private subnet
+	for {
+		if err := gCfg.Read(""); err != nil {
+			logrus.Errorf("Error reading global settings from cluster store")


include err in log message

chrisplo · 2017-11-08T16:49:51Z

All my comments have been addressed, will defer to the reset of the comments

tiewei · 2017-11-08T17:44:37Z

build PR

rchirakk

LGTM

Currently netplugin is trying to init from default settings if global setting is not ready or wrong, and when the settings don't match it will re-init netplugin. This is very easy to go wrong when there are resources created before global settings are ready. This commit makes netplugin wait until the global settings ready, and initiate itself according to it. If the global settings are mis-match, it will return error. Also set default fwd mode to empty on netmaster and Refactor CIDRToMask Signed-off-by: Wei Tie <wtie@cisco.com>

tiewei · 2017-11-08T18:17:21Z

build PR

tiewei · 2017-11-08T21:32:25Z

build PR

tiewei · 2017-11-09T00:56:50Z

build PR

tiewei · 2017-11-09T04:19:07Z

build PR

tiewei · 2017-11-09T15:37:13Z

build PR

unclejack

LGTM

Docker expects the netplugin socket to be available within 10 seconds before it fails enabling (or installing) the v2plugin. Due to contiv#1043, netplugin is blocking waiting for the forward mode to be set, which is done by netctl calling netmaster, but netmaster is not started until the plugin is activating. Instead of backgrounding the plugin install/enabling then letting ansible set the forward mode, do it in the plugin script to avoid ansible's unpredictable round trip delays. Signed-off-by: Chris Plock <chrisplo@cisco.com>

* v2plugin set forward mode when netmaster up Docker expects the netplugin socket to be available within 10 seconds before it fails enabling (or installing) the v2plugin. Due to #1043, netplugin is blocking waiting for the forward mode to be set, which is done by netctl calling netmaster, but netmaster is not started until the plugin is activating. Instead of backgrounding the plugin install/enabling then letting ansible set the forward mode, do it in the plugin script to avoid ansible's unpredictable round trip delays. v2plugin’s startcontiv.sh errors when fwd_mode not set Signed-off-by: Chris Plock <chrisplo@cisco.com>

kahou82 reviewed Nov 1, 2017

View reviewed changes

dseevr reviewed Nov 1, 2017

View reviewed changes

rchirakk suggested changes Nov 1, 2017

View reviewed changes

chrisplo reviewed Nov 3, 2017

View reviewed changes

rchirakk reviewed Nov 4, 2017

View reviewed changes

chrisplo reviewed Nov 7, 2017

View reviewed changes

kahou82 reviewed Nov 7, 2017

View reviewed changes

netplugin/plugin/netplugin_test.go Outdated

"state": "fakedriver",

"container": "docker",

},

"plugin-instance": {

Copy link

Member

kahou82 Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra tab?

kahou82 reviewed Nov 7, 2017

View reviewed changes

chrisplo reviewed Nov 7, 2017

View reviewed changes

tiewei commented Nov 7, 2017

View reviewed changes

rchirakk suggested changes Nov 8, 2017

View reviewed changes

dseevr approved these changes Nov 8, 2017

View reviewed changes

rchirakk approved these changes Nov 8, 2017

View reviewed changes

unclejack mentioned this pull request Nov 9, 2017

global fwd and subnet error msgs fix & test cases #1051

Merged

unclejack approved these changes Nov 9, 2017

View reviewed changes

unclejack merged commit d05de09 into contiv:master Nov 9, 2017

chrisplo mentioned this pull request Nov 11, 2017

v2plugin set forward mode when netmaster up #1058

Merged

tiewei mentioned this pull request Nov 16, 2017

Make netplugin resilient against unconfigured data stores #915

Closed

		@@ -86,20 +86,18 @@ func NewNPCluster(its integTestSuite) (NPCluster, error) {
		time.Sleep(10 * time.Second)

		// set forwarding mode if required

		func TestNetPluginInit(t *testing.T) {
		initFakeStateDriver(t)

Init global settings from object store #1043

Init global settings from object store #1043

Conversation

tiewei commented Nov 1, 2017

kahou82 commented Nov 1, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kahou82 Nov 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiewei commented Nov 1, 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

tiewei Nov 1, 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

tiewei commented Nov 3, 2017

unclejack commented Nov 3, 2017

tiewei commented Nov 3, 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

tiewei commented Nov 3, 2017

tiewei commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiewei Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

tiewei commented Nov 6, 2017

tiewei commented Nov 7, 2017

tiewei commented Nov 7, 2017

tiewei commented Nov 7, 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 Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiewei commented Nov 7, 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

tiewei commented Nov 7, 2017

Choose a reason for hiding this comment

kahou82 Nov 1, 2017 •

edited

Loading

tiewei Nov 1, 2017 •

edited

Loading

tiewei Nov 6, 2017 •

edited

Loading

tiewei Nov 7, 2017 •

edited

Loading