Remove stale records for short-lived containers

[inercia]

This PR removes stale records for short-lived containers by asking Docker about the container that corresponds to the new name:IP that has been introduced.

This can be tested by following the procedure described by @rade in #821: with (and without) docker kill $CONTAINER_ID in weave:put_dns_fqdn() and then running something like:

#!/bin/sh

make clean
make all

./weave launch
./weave launch-dns 10.2.1.1/24 --debug
./weave run 10.2.0.1/24 -h foo.weave.local -ti ubuntu
echo "Checking if 'foo.weave.local' is there"
./weave status | grep "foo.weave.local"
echo "Stopping everything"
./weave stop-dns
./weave stop

Fixes #821

[rade]

I think the abstraction isn't quite right here. There's too much machinery in weavedns and too little in updater. Bear in mind that solving #819 will likely require very similar machinery.

[inercia]

@rade If with "too much machinery in weavedns" you mean lines 72-86, IMO fourteen lines are not so much machinery, specially when they are pretty specific of WeaveDNS (eg, deleting records in the Zone).

Adding this mechanism in the "updater" would not be the right place either, as the "updater" is really a Docker listener, and this does not listen for anything: it just checks whether the container is running. I raised the question in HipChat whether we should have a general "docker client" in common...

So I would wait until #819 before trying to do an early generalization...

[rade]

fourteen lines is a lot of machinery when we could have something like if isContainerRunning(container) {...}.

Also, there's all the startup machinery in weavedns/main.go

[inercia]

Yes, it would be nice to have isContainerRunning(), but the thing is where to put that method. And, as you mentioned, "the startup machinery in weavedns/main.go" is the Docker client initialization, used both by the "updater" and by this code. So I would prefer not to transform the issue "stale records for short-lived containers" into "refactor all our Docker code into a common client"...

[rade]

weavedns/main.go has no business initialising a docker client. This should be well abstracted away. The new import of "github.com/fsouza/go-dockerclient" in main.go is a dead giveaway.

[inercia]

Ok, then we will have a common/docker/client.go file... :)

[rade]

The wrapping can be simpler:

diff --git a/common/docker/client.go b/common/docker/client.go
index ecd213d..73c5cfd 100644
--- a/common/docker/client.go
+++ b/common/docker/client.go
@@ -11,7 +11,7 @@ type ContainerObserver interface {
 }

 type Client struct {
-       client *docker.Client
+       *docker.Client
 }

 // NewClient creates a new Docker client
@@ -21,10 +21,7 @@ func NewClient(apiPath string) (*Client, error) {
                return nil, err
        }

-       c := Client{
-               client: dc,
-       }
-       return &c, nil
+       return &Client{dc}, nil
 }

 // Start starts the client
@@ -47,7 +44,7 @@ func (c *Client) Start(apiPath string) error {
 // AddObserver adds an observer for docker events
 func (c *Client) AddObserver(ob ContainerObserver) error {
        events := make(chan *docker.APIEvents)
-       if err := c.client.AddEventListener(events); err != nil {
+       if err := c.AddEventListener(events); err != nil {
                Error.Printf("[docker] Unable to add listener to Docker API: %s"
                return err
        }
@@ -69,7 +66,7 @@ func (c *Client) IsContainerRunning(idStr string) bool {
        running := true

        // check with Docker whether the container is really running
-       if container, err := c.client.InspectContainer(idStr); err != nil {
+       if container, err := c.InspectContainer(idStr); err != nil {
                if _, notThere := err.(*docker.NoSuchContainer); notThere {
                        running = false
                }

[inercia]

IMO that would not be appropriate here. Defining a struct like type Client struct { *docker.Client } does not make much sense, not only because it provides nothing on top of the original struct but also because we do not want to expose the whole docker.Client API, and that would do it... I think common/docker/client.go should remain as a facade for the docker API, not as an alias.

[rade]

it provides nothing on top of the original struct

But it does. Namely the methods defined here.

we do not want to expose the whole docker.Client API

I actually see no problem with that.

[rade]

what's happening with this?

[inercia]

@rade finishing #836, then I will resume this PR...

(anyway, I think you last comment is personal preference more than a real problem you found when reviewing the code, and I think this kind of comments slow down the review...)

[rade]

The code I posted is shorter and just as readable, if not more so, than the original.

[inercia]

I also considered that solution when writing the code, and I discarded it because I think type aliases are not a good idea, even when they lead to shorter code (eg, type i int is not a good idea, even when produces shorter code and you can also add your own methods). But I will change it because I don't like to engage in endless discussions like this... ;)

[rade]

Can it actually be done with a type alias? I tried and failed.

[inercia]

(facepalm)

[inercia]

You can do this:

type s string
func (something s) IsEmpty() bool { return len(something) == 0 }

so should we replace all our strings with s? 😕

[rade]

I know about type aliases, but I cannot get them to work in this situation. Can you? The problem I am encountering is that we get a pointer to the docker client, which I cannot figure out how to turn into the right type, if that is even possible.

[rade]

Found the answer; so the wrapper struct actually works better than an alias.

[inercia]

Changed the client...

[rade]

Does this handle the non-container ids case correctly, e.g. when we add records for weave:expose?

[rade]

and the linter is unhappy.

[inercia]

Good point about the weave:expose thing: I think I will need a if...

And I will make the linter happy again...

[rade]

To address the weave:expose problem, I suggest you exclude idents from the aliveness check that are not container IDs, e.g. ^[a-f0-9]+$.

[inercia]

Fixed the container id check.

[rade]

also, it looks like this needs rebasing.

[rade]

The outdent still needs doing. Rest looks fine.

[bboreham]

I'm going to take this over in @inercia's absence.