BR: Refactor socket setup

[oncilla]

Add rctx.SocketType to indicate the type of socket.
The default is posix.

Add SocketConf to configure the socket type for the
local and all interface sockets individually.

Replace hook system for local and external socket setup
with a SocketOps dictionary. SocketOps may be added
by different network stacks.
This is done in preperation for a future PR that adds
proper transactional context reloads with rollback and
teardown capabilites.

Move posix specific socket setup to setup-posix.go.

Add basic unit tests for setupNet. These will be
extended in the future PR that adds rollbaack and
teardown.

---

This change is 

[oncilla]

---

go/border/setup.go, line 63 at r1 (raw file):

var DefaultSockType = PosixSock

type SocketConf struct {

Not sure what the appropriate place is for SocketConf.

[kormat]

Reviewed 1 of 5 files at r2.
Reviewable status: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @oncilla, @sgmonroy, and @kormat)

---

go/border/setup.go, line 39 at r1 (raw file):

type locSockOps interface {
	Setup(*Router, *rctx.Ctx, prometheus.Labels, *rctx.Ctx) error

When it's not clear what the parameters are for in an interface definition, give them names. In this case i only happen to know why there are 2x rctx.Ctx from previous reviews.

[kormat]

Reviewed 1 of 5 files at r1.
Reviewable status: 2 of 6 files reviewed, 2 unresolved discussions (waiting on @oncilla, @sgmonroy, and @kormat)

---

go/border/testdata/topology.json, line 57 at r2 (raw file):

    "ISD_AS": "1-ff00:0:111",
    "MTU": 1472
  }

Trailing newline.

[kormat]

Reviewed 1 of 5 files at r2.
Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @oncilla, @sgmonroy, and @kormat)

[kormat]

Reviewed 1 of 5 files at r2.
Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @oncilla, @sgmonroy, and @kormat)

---

go/border/setup-posix.go, line 70 at r2 (raw file):

	labels prometheus.Labels) error {

	log.Debug("Setting up new local socket.")

I'm not sure this is needed, but if it is please include the address it's binding to.

---

go/border/setup-posix.go, line 113 at r2 (raw file):

		// An existing interface has changed.
		// Stop old input goroutine.
		stopSock(oldCtx.ExtSockIn[intf.Id])

Q: why is stopSock needed? When would the rctx.Sock be nil?

[kormat]

Reviewed 1 of 5 files at r2.
Reviewable status: 5 of 6 files reviewed, 6 unresolved discussions (waiting on @oncilla, @sgmonroy, and @kormat)

---

go/border/setup.go, line 63 at r1 (raw file):

Previously, Oncilla wrote…

Not sure what the appropriate place is for SocketConf.

brconf seems alright.

---

go/border/setup.go, line 125 at r2 (raw file):

	oldCtx := rctx.Get()
	ctx := rctx.New(config)
	if err := r.setupNet(ctx, oldCtx, brconf.SockConf{Default: PosixSock}); err != nil {

brconf.SockConf{Default: PosixSock} - this is intended to be loaded from br.toml at some later point, right? In that case i'd assign it to a var here, with a TODO, and then pass that into setupNet.

---

go/border/setup.go, line 229 at r2 (raw file):

	}
	// Validate local sock of same type.
	if oldCtx.LocSockIn != nil && oldCtx.LocSockIn.Type != sockType {

Is there a case where oldCtx isn't nil, but has a nil LockSockIn?

[oncilla]

Reviewable status: 2 of 6 files reviewed, 6 unresolved discussions (waiting on @kormat and @sgmonroy)

---

go/border/setup.go, line 39 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

When it's not clear what the parameters are for in an interface definition, give them names. In this case i only happen to know why there are 2x rctx.Ctx from previous reviews.

Done.

---

go/border/setup.go, line 125 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

brconf.SockConf{Default: PosixSock} - this is intended to be loaded from br.toml at some later point, right? In that case i'd assign it to a var here, with a TODO, and then pass that into setupNet.

Done.

---

go/border/setup.go, line 229 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Is there a case where oldCtx isn't nil, but has a nil LockSockIn?

Nope, and if there were it would be broken anyway.
Done.

---

go/border/setup-posix.go, line 70 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I'm not sure this is needed, but if it is please include the address it's binding to.

Done.

---

go/border/setup-posix.go, line 113 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Q: why is stopSock needed? When would the rctx.Sock be nil?

Ah, good question.
With stopSock we can get rid of some nil checks in the rollback code.
Somehow I managed to wrap all sockets :)
As it is not needed here, I removed it (and will re-evaluate the usage in rollback)
Done.

---

go/border/testdata/topology.json, line 57 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Trailing newline.

Done.

[kormat]

Reviewed 3 of 4 files at r3.
Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @sgmonroy)

[kormat]

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @oncilla and @sgmonroy)

---

go/border/setup_test.go, line 107 at r3 (raw file):

	}
	sockConf := brconf.SockConf{Default: PosixSock}
	oldCtx := rctx.New(loadConfig(t))

This is a bit brain-twisting. The ctx that's created first is called newCtx, the one that's created second is oldCtx. Y u do dis?

[oncilla]

---

go/border/setup_test.go, line 107 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This is a bit brain-twisting. The ctx that's created first is called newCtx, the one that's created second is oldCtx. Y u do dis?

Refactored into 2 steps.

[kormat]

Reviewed 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @oncilla and @sgmonroy)

---

go/border/setup_test.go, line 45 at r4 (raw file):

		defer updateTestRouter(r, ctx, oldCtx)()
		// Check that the sockets are reused if nothing changes.
		checkLocSocksUnchanged("", ctx, oldCtx)

Passing an empty string here will make a message that starts with : .

---

go/border/setup_test.go, line 73 at r4 (raw file):

// checkLocSocksUnchanged compares that both contexts point to the same local socket.
func checkLocSocksUnchanged(key string, ctx, oldCtx *rctx.Ctx) {
	SoMsg(fmt.Sprintf("%s: LocSockIn unchanged", key), ctx.LocSockIn, ShouldEqual, oldCtx.LocSockIn)

Just to check - the intention is to compare the pointers, right?

---

go/border/setup_test.go, line 120 at r4 (raw file):

	// Initialize router with the topology.
	r := &Router{
		freePkts: ringbuf.New(1024, func() interface{} {

It's probably overkill to allocate 9MB of memory that won't be used.

[oncilla]

---

go/border/setup_test.go, line 73 at r4 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Just to check - the intention is to compare the pointers, right?

Yes.

[oncilla]

Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @kormat and @sgmonroy)

---

go/border/setup_test.go, line 45 at r4 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Passing an empty string here will make a message that starts with : .

Done.

---

go/border/setup_test.go, line 120 at r4 (raw file):

Previously, kormat (Stephen Shirley) wrote…

It's probably overkill to allocate 9MB of memory that won't be used.

Done.

[kormat]

Reviewed 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sgmonroy)

[kormat]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sgmonroy)

[sgmonroy]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sgmonroy)

[sgmonroy]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sgmonroy)

[sgmonroy]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sgmonroy)

[kormat]

Reviewed 1 of 1 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla and @sgmonroy)

---

go/border/setup_test.go, line 128 at r7 (raw file):

	r := &Router{
		// The number of free packets has to be at least the number of
		// posixInput routines times

This sentence is missing an end, but is it also a (partial) dupe of the comment just above?

[oncilla]

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @kormat and @sgmonroy)

---

go/border/setup_test.go, line 128 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This sentence is missing an end, but is it also a (partial) dupe of the comment just above?

Done.

[kormat]

Reviewed 1 of 1 files at r8.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sgmonroy)