369 name weave peers #407

awh · 2015-02-19T15:39:06Z

Implement -hostname flag to weave router for inclusion in weave status:

root@weave01:~# weave status
weave router git-170724472555
Encryption off
Our name is 7a:85:0d:3f:4b:8b (weave01)
Sniffing traffic on &{25 65535 ethwe 26:47:3f:b1:9f:35 up|broadcast|multicast}
MACs:
e6:eb:3d:c2:f2:c0 -> 7a:85:0d:3f:4b:8b (2015-02-19 14:36:43.698599704 +0000 UTC)
ea:9b:f6:9a:a8:94 -> 7a:ee:54:05:6d:19 (2015-02-19 14:36:45.467878022 +0000 UTC)
02:c5:dc:fa:4f:19 -> 7a:c4:7d:e3:0f:9b (2015-02-19 14:36:50.594692416 +0000 UTC)
Peers:
Peer 7a:c4:7d:e3:0f:9b (weave03) (v37) (UID 12377675045837118970)
   -> 7a:ee:54:05:6d:19 (weave02) [178.62.77.81:6783]
   -> 7a:85:0d:3f:4b:8b (weave01) [178.62.57.35:6783]
Peer 7a:85:0d:3f:4b:8b (weave01) (v8) (UID 8327482320931219319)
   -> 7a:c4:7d:e3:0f:9b (weave03) [178.62.106.102:59473]
   -> 7a:ee:54:05:6d:19 (weave02) [178.62.77.81:48078]
Peer 7a:ee:54:05:6d:19 (weave02) (v4) (UID 13020696009711836879)
   -> 7a:85:0d:3f:4b:8b (weave01) [178.62.57.35:6783]
   -> 7a:c4:7d:e3:0f:9b (weave03) [178.62.106.102:53570]

Notes:

weave launch does not accept user supplied -hostname as this would require introducing getopt or similar into the bash script and given #307 is in the offing it didn't seem prudent; script supplies kernel hostname directly to weave router for now. Please advise if you would like this addressed before merging
Protocol version was increased due to binary incompatibility introduced by inclusion of hostname in the gossip protocol messages

rade · 2015-02-19T15:46:43Z

Just for reference, this is meant to address #369.

rade · 2015-02-19T16:04:13Z

Referring to this new piece of data as 'hostname' is wrong; it could be anything, and in many cases will not be the hostname. Yes, I know #369 calls it 'hostname', but I didn't file that issue ;)

And yes, we really, really must be able to pass that thing into weave launch.

awh · 2015-02-19T16:06:42Z

Understood. Do you have a preference on what you'd like it to be called?

rade · 2015-02-19T16:07:36Z

Do you have a preference on what you'd like it to be called?

I am sure I do, but I don't know what it is :)

rade · 2015-02-19T16:09:47Z

'displayname'? Not great, so happy to consider alternatives.

weaver/main.go

+			log.Fatal(err)
+		}
+		if osHostName == "" {
+			hostName = "<unknown>"


awh · 2015-02-19T16:11:03Z

I was just about to suggest that or friendlyname, informalname....

weaver/main.go

-	router := weave.NewRouter(iface, ourName, []byte(password), connLimit, bufSz*1024*1024, logFrame)
-	log.Println("Our name is", router.Ourself.Name)
+	router := weave.NewRouter(iface, ourName, hostName, []byte(password), connLimit, bufSz*1024*1024, logFrame)
+	log.Println("Our name is", router.Ourself.Name, "(" + hostName + ")")


rade · 2015-02-19T16:16:19Z

'friendlyname' is quite nice. let's invite @bboreham to help paint this particular bike shed :)

dpw · 2015-02-19T16:20:33Z

If I may join the bike-shedding, what about "nickname"?

rade · 2015-02-19T16:22:45Z

I do like 'nickname'.

Accept -nickname parameter to weave script

awh · 2015-02-19T17:15:22Z

Addressed comments to date!

rade · 2015-02-19T17:32:25Z

The new option needs to added to the docs. Somewhere.

awh · 2015-02-20T18:43:23Z

Just thinking about the default value of the nickname - would $(hostname -f) be better than $(hostname)?

rade · 2015-02-20T19:54:03Z

Ouch, hostname isn't even in POSIX! I guess we don't care, but still surprising.

awh · 2015-02-23T09:29:21Z

That is surprising. Bash has a HOSTNAME environment variable, however the script depends on /bin/sh. Unfortunately we can't solely depend on os.Hostname() from Go because it obtains the hostname of the container and not of the underlying host kernel :(

With respect to the other issue (short hostname vs FQDN) I suspect the latter is going to be useful if folks have naming schemes like app01.dc01.example.com, app01.dc02.example.com...

rade · 2015-02-23T09:32:09Z

Can anybody think of a downside to using FQDNs? @bboreham

bboreham · 2015-02-23T09:36:24Z

When I thought of this originally, I meant the "hostname" of the container. So we didn't have to implement any flag-parsing, because --hostname is already a Docker option. I'm not clear why you went down this other route.

rade · 2015-02-23T09:37:49Z

The hostname of the weave container is not exactly meaningful.

bboreham · 2015-02-23T09:38:49Z

It defaults to the container ID, which I usually know when testing, and I can set to anything else I feel like. So it is meaningful to me.

rade · 2015-02-23T09:39:20Z

setting it to anything else is not exactly straightforward

bboreham · 2015-02-23T09:40:07Z

I seem to be missing this part. Extra args in weave launch are handed to Docker; I can add --hostname to those args. Where does it get difficult?

rade · 2015-02-23T09:40:37Z

extra args to weave launch are handed to weave.

bboreham · 2015-02-23T09:41:28Z

Aha. I think that belongs as a comment to #369.

rade · 2015-02-23T09:42:05Z

you'd have to use WEAVE_DOCKER_ARGS.

rade · 2015-02-23T10:15:16Z

Based on those two samples, I can tell what cloud the host is in from the short name.

Perhaps, but in case of EC2, the FQDN also tells you the region, which is useful.

What's your objection to using the FQDN by default? That it can be quite long and thus clutter the output? I can see how that could be annoying, but IMO the default should favour uniqueness & more info over compactness. As you say, it's easy to override by supplying -hostname $(hostname).

site/troubleshooting.md

@@ -86,7 +86,11 @@ The terms used here are explained further at
 [how it works](how-it-works.html).

 The 'Our name' line identifies the local weave router as a peer in the
-weave network.
+weave network. It displays both the peer name and the peer's nickname
+in parenthesis; the peer name defaults to a MAC address, whilst the


rade · 2015-02-23T22:12:04Z

IF we are going with hostname -f as the default, then the go-level default should also be the FQDN. How hard is that?

awh · 2015-02-24T17:03:38Z

Go question - which is preferred?
As the code is now:

    if nickName == "" {
        osHostname, err := os.Hostname()
        if err != nil {
            log.Fatal(err)
        }
        nickName = osHostname
    }

Assigning directly to nickName (NB using '=' instead of ':='); now we're depending upon and overwriting an earlier 'err' declaration:

    if nickName == "" {
        nickName, err = os.Hostname()
        if err != nil {
            log.Fatal(err)
        }
    }

Explicitly shadow err:

    if nickName == "" {
        var err error = nil
        nickName, err = os.Hostname()
        if err != nil {
            log.Fatal(err)
        }
    }

Unfortunately this doesn't work because the inner nickName shadows the outer:

    if nickName == "" {
        nickName, err := os.Hostname()
        if err != nil {
            log.Fatal(err)
        }
    }

awh · 2015-02-24T17:15:16Z

Getting the FQDN in pure Go is doable:

func main() {
    hostname, err := os.Hostname()
    if err == nil {
        addrs, err := net.LookupHost(hostname)

        if err == nil {
            names, err := net.LookupAddr(addrs[0])

            if err == nil {
                fmt.Printf("%v", names)
            }
        }
    }
}

(Obviously both LookupHost and LookupAddr can return zero to many items, which the above code doesn't handle). In my Vagrant VM the above code prints this:

vagrant@ubuntu-14:~$ ./main
[ubuntu-14.04-amd64-vbox ubuntu-14]

Which looks to me like the LookupAddr function isn't parsing out the alias from the hosts file:

vagrant@ubuntu-14:~$ cat /etc/hosts
127.0.0.1   localhost
127.0.1.1   ubuntu-14.04-amd64-vbox ubuntu-14

rade · 2015-02-24T17:45:58Z

Go question - which is preferred?

The 2nd one. Nothing wrong with re-using err.

Getting the FQDN in pure Go is doable.

That's horrific. going via reverse lookup. Yuck.

awh · 2015-02-24T17:52:31Z

:) As far as I am aware, this is exactly what hostname -f is doing underneath, albeit via libnsl to parse nsswitch.conf, resolv.conf and hosts...

awh · 2015-02-24T17:57:26Z

Shelling out to hostname -f from Go is an option I suppose :)

rade · 2015-02-24T18:11:57Z

Shelling out to hostname -f from Go is an option I suppose :)

Sure. If we want to undo all the work we've done to avoid using the native resolver libraries. Oh and to produce an image containing just a single executable.

I'm afraid hostname/os.Hostname() is doing to have to be the default.

awh · 2015-02-24T18:56:54Z

Sure. If we want to undo all the work we've done to avoid using the native resolver libraries. Oh and to producer an image containing just a single executable.

I was half joking, but learned something useful! Is the desire to avoid the resolver related to the warnings in #5?

I'm afraid hostname/os.Hostname() is doing to have to be the default.

In that case as far as I am aware the currently implemented behaviour matches the now finalised requirements...

rade · 2015-02-24T21:26:08Z

Is the desire to avoid the resolver related to the warnings in #5?

Yes. But off-topic.

router/gossip_test.go

@@ -19,7 +19,7 @@ type mockChannelConnection struct {
 // We need to create some dummy channels otherwise tests hang on nil
 // channels when Router.OnGossip() calls async methods.
 func NewTestRouter(name PeerName) *Router {
-	router := NewRouter(nil, name, nil, 10, 1024, nil)
+	router := NewRouter(nil, name, "hostname", nil, 10, 1024, nil)


awh · 2015-02-25T11:23:17Z

Above commits address comments to date.

rade · 2015-02-25T12:05:21Z

You missed a fair few places where "hostname" is used as a nickname in tests.

awh · 2015-02-25T12:12:39Z

Apologies, I neglected to look further afield than the specific file you commented on. Fixed.

nicknames for weave peers Closes #369.

awh added 3 commits February 19, 2015 12:29

Add hostname to peers

f517b94

Include remote hostname for connections in router status

1707244

Bump protocol version due to introduction of hostname field

8f6a525

rade reviewed Feb 19, 2015
View reviewed changes

weaver/main.go

log.Fatal(err)

}

if osHostName == "" {

hostName = "<unknown>"

This comment was marked as abuse.

Sign in to view

rade reviewed Feb 19, 2015
View reviewed changes

Rename hostname to nickname

244af11

Accept -nickname parameter to weave script

Add nickname feature to documentation

1b92f60

rade reviewed Feb 23, 2015
View reviewed changes

awh added 3 commits February 24, 2015 16:52

Remove reference to containers from argument help

5e54637

Fix formatting

3cf2179

Address documentation comments

34f7b5c

Simplify argument parsing

4e9e183

rade reviewed Feb 24, 2015
View reviewed changes

awh added 2 commits February 24, 2015 22:23

Transpose script arguments to match declaration order

326a5d1

Use empty string as test nickname

72c9ae7

Actually use empty string as test nickname

25c0cfd

rade added a commit that referenced this pull request Feb 25, 2015

Merge pull request #407 from awh/369_name_weave_peers

d1400bc

nicknames for weave peers Closes #369.

rade merged commit d1400bc into weaveworks:master Feb 25, 2015

awh deleted the 369_name_weave_peers branch February 25, 2015 20:36

rade modified the milestone: 0.10.0 Apr 18, 2015

369 name weave peers #407

369 name weave peers #407

Conversation

awh commented Feb 19, 2015

rade commented Feb 19, 2015

rade commented Feb 19, 2015

awh commented Feb 19, 2015

rade commented Feb 19, 2015

rade commented Feb 19, 2015

This comment was marked as abuse.

awh commented Feb 19, 2015

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

rade commented Feb 19, 2015

dpw commented Feb 19, 2015

rade commented Feb 19, 2015

awh commented Feb 19, 2015

rade commented Feb 19, 2015

awh commented Feb 20, 2015

rade commented Feb 20, 2015

awh commented Feb 23, 2015

rade commented Feb 23, 2015

bboreham commented Feb 23, 2015

rade commented Feb 23, 2015

bboreham commented Feb 23, 2015

rade commented Feb 23, 2015

bboreham commented Feb 23, 2015

rade commented Feb 23, 2015

bboreham commented Feb 23, 2015

rade commented Feb 23, 2015

rade commented Feb 23, 2015

This comment was marked as abuse.

rade commented Feb 23, 2015

awh commented Feb 24, 2015

awh commented Feb 24, 2015

rade commented Feb 24, 2015

awh commented Feb 24, 2015

awh commented Feb 24, 2015

rade commented Feb 24, 2015

awh commented Feb 24, 2015

rade commented Feb 24, 2015

This comment was marked as abuse.

awh commented Feb 25, 2015

rade commented Feb 25, 2015

awh commented Feb 25, 2015