Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add options to set ports on weaver command line #534

Merged
merged 1 commit into from Apr 10, 2015

Conversation

abligh
Copy link
Contributor

@abligh abligh commented Apr 9, 2015

This commit adds two command line options, intended to allow multiple
instances of weave to run without container separation between them.

  • -port=PORT makes weave bind to port PORT (an integer). Without the
    option, the default value (6783) is used.
  • -bindjsonto=BINDSTRING makes weave bind its control interface to
    a different port. For instance "127.0.0.1:6784" can be used to
    bind to localhost only. ":6784" is the default. An empty string
    can be used to disable the control interface.

Signed-off-by: Alex Bligh alex@alex.org.uk

@abligh
Copy link
Contributor Author

abligh commented Apr 9, 2015

Hmm... travis failed here, just like a build does locally, because weaver/main.go has:

import (
       ....
       weave "github.com/zettio/weave/router"

and of course a build will fail against the github version of weave/router, as this hasn't been merged yet.

Changing this to:

weave "../router"

makes it work fine, but presumably you don't want to merge that.

@rade
Copy link
Member

rade commented Apr 9, 2015

Hmm. Looks like something isn't quite right in our (relatively new) travis setup here. It appears to be pulling down dependencies from git when it should just use the repo it has already checked out. @tomwilkie any idea?

@bboreham
Copy link
Contributor

It's because the name is different in the repo. Fix is in #528

@rade
Copy link
Member

rade commented Apr 10, 2015

I reckon we should move NormalisePeerAddr to Router.NormaliseAddr.

@@ -31,6 +31,7 @@ type Router struct {
ConnLimit int
BufSz int
LogFrame func(string, []byte, *layers.Ethernet)
ListenPort int

This comment was marked as abuse.

@abligh abligh force-pushed the select-ports-on-cli branch 2 times, most recently from c6bd35f to b278a15 Compare April 10, 2015 09:26
@abligh
Copy link
Contributor Author

abligh commented Apr 10, 2015

Rade: I fixed all your comments bar this one:

In router/router.go:

@@ -86,8 +88,8 @@ func (router *Router) Start() {
router.Macs.Start()
router.Routes.Start()
router.ConnectionMaker.Start()

  • router.UDPListener = router.listenUDP(Port, po)
  • router.listenTCP(Port)
  • router.UDPListener = router.listenUDP(router.ListenPort, po)
  • router.listenTCP(router.ListenPort)

Just use port here, i.e. the method param.

I didn't understand that one. This is in Start(), which takes no parameters. The parameters are passed to NewRouter() (immediately above it).

Travis appears to like the commit now.

@rade
Copy link
Member

rade commented Apr 10, 2015

I didn't understand that one.

Ah sorry, github showed me a diff with a function boundary collapsed, and I didn't realise that.

@rade
Copy link
Member

rade commented Apr 10, 2015

One more thing. The introduction of a dependency of ConnectionMaker on Router (via cm.ourself.Router) is undesirable. So I suggest we pass a function to NewConnectionMaker, stash that in a new ConnectionMaker.normalisePeerAddr field, and invoke that wherever you currently have cm.ourself.Router.NormalisePeerAddr.

@abligh
Copy link
Contributor Author

abligh commented Apr 10, 2015

I think I've done that, and it works here, but it's now failing the coverage test. I am afraid the reasons it's failing (but wasn't before) are a bit opaque to me.

@rade
Copy link
Member

rade commented Apr 10, 2015

don't worry about the coverage "failure".

@@ -14,12 +14,15 @@ const (
MaxInterval = 10 * time.Minute
)

type PeerNormaliser func(string) string

This comment was marked as abuse.

This commit adds two command line options, intended to allow multiple
instances of weave to run without container separation between them.

* -port=PORT makes weave bind to port PORT (an integer). Without the
  option, the default value (6783) is used.

* -httpaddr=ADDRESS makes weave bind its control interface to
  a different address. For instance "127.0.0.1:6784" can be used to
  bind to localhost only. An absent address element makes it bind to
  all interfaces. ":6784" is the default. An empty string
  can be used to disable the control interface.

Signed-off-by: Alex Bligh <alex@alex.org.uk>
@rade
Copy link
Member

rade commented Apr 10, 2015

LGTM!

@abligh
Copy link
Contributor Author

abligh commented Apr 10, 2015

Fixed

rade added a commit that referenced this pull request Apr 10, 2015
Add options to set ports on weaver command line
@rade rade merged commit bd8cb02 into weaveworks:master Apr 10, 2015
@rade rade modified the milestone: 0.10.0 Apr 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants