Add filter on task status in addition to desired status (Docker Provider - swarm) #1304
Conversation
|
@Yshayy Thanks for this :) |
|
Sure. |
|
I added a unit test. |
provider/docker_test.go
Outdated
| type fakeTasksClient struct { | ||
| dockerClient.APIClient | ||
| tasks []swarm.Task | ||
| err error |
There was a problem hiding this comment.
Looking at the test, you don't need that one 👼
There was a problem hiding this comment.
In fact, as it is used in return statement of TaskList, we may leave it as is.
provider/docker_test.go
Outdated
| @@ -6,11 +6,15 @@ import ( | |||
| "testing" | |||
|
|
|||
| "github.com/containous/traefik/types" | |||
| "github.com/davecgh/go-spew/spew" | |||
| dockerClient "github.com/docker/engine-api/client" | |||
There was a problem hiding this comment.
Should be dockerclient. Go eschews camel case or even underscores in package names/aliases.
provider/docker_test.go
Outdated
| cases := []struct { | ||
| service swarm.Service | ||
| tasks []swarm.Task | ||
| isGlobalSvc bool |
There was a problem hiding this comment.
Should be isGlobalSVC.
Signed-off-by: Emile Vauge <emile@vauge.com>
provider/docker_test.go
Outdated
| } | ||
|
|
||
| for _, e := range cases { | ||
| dockerData := parseService(e.service, e.networks) |
There was a problem hiding this comment.
I'd really like us to use more sub-tests. Benefits:
- We can run sub-tests individually (
go test -run <sub test specifier>) - We are encouraged to pass a meaningful context into the
t.Run()method (and, at the same time, don't need to add it at everyt.{Log,Error,Fatal}*call). - We can parallelize tests easily.
- If we use
t.Fatal*, we only fatal out of the current sub-test and not the entire one.
Setting up sub-tests is really easy: Example.
There was a problem hiding this comment.
Added sub tests in the whole file.
provider/docker_test.go
Outdated
|
|
||
| for i, taskID := range e.expectedTasks { | ||
| if taskDockerData[i].Name != taskID { | ||
| t.Fatalf("expect task id %v, got %v", taskID, taskDockerData[i].Name) |
There was a problem hiding this comment.
Could this possibly be t.Errorf?
There was a problem hiding this comment.
@timoreimann On this, I suggest to make a global PR as a lot of test should be changed.
There was a problem hiding this comment.
I agree for pre-existing tests (although we probably can't do this practically in a single PR only), but disagree for new ones. IMHO, there's no reason to create new test code with improper t.Error/t.Fatal usage.
There was a problem hiding this comment.
@timoreimann replaced in the whole file.
provider/docker_test.go
Outdated
| for _, e := range cases { | ||
| dockerData := parseService(e.service, e.networks) | ||
| dockerClient := &fakeTasksClient{tasks: e.tasks} | ||
| taskDockerData, _ := listTasks(context.Background(), dockerClient, e.service.ID, dockerData, map[string]*docker.NetworkResource{}, e.isGlobalSvc) |
There was a problem hiding this comment.
We should not ignore the error but check it and t.Fatal out if it turns out to be non-nil.
There was a problem hiding this comment.
This is the only thing I find import to change now; my other comments would be nice to have but don't seem critical.
provider/docker_test.go
Outdated
| taskDockerData, _ := listTasks(context.Background(), dockerClient, e.service.ID, dockerData, map[string]*docker.NetworkResource{}, e.isGlobalSvc) | ||
|
|
||
| if len(e.expectedTasks) != len(taskDockerData) { | ||
| t.Fatalf("expected tasks %v, got %v", spew.Sprint(e.expectedTasks), spew.Sprint(taskDockerData)) |
There was a problem hiding this comment.
I'd prefer spew.Sdump because
To dump a variable with full newlines, indentation, type, and pointer information use Dump [...].
Sprint is a compacted form.
provider/docker_test.go
Outdated
| t.Fatalf("expected %q, got %q", e.expected, actual) | ||
| } | ||
| for containerID, e := range containers { | ||
| t.Run(strconv.Itoa(containerID), func(t *testing.T) { |
There was a problem hiding this comment.
You need to make an assignment like e := e before calling t.Run() in order to capture the range variable. Otherwise, your variable won't get bound to the correct instance.
It's basically about this particular Go gotcha.
There was a problem hiding this comment.
Similar with the other tests.
There was a problem hiding this comment.
Indeed, I forgot this awesome feature in go...
Thanks 👍
There was a problem hiding this comment.
ROFLing so hard right now...
Signed-off-by: Emile Vauge <emile@vauge.com>
|
@timoreimann fixes pushed :) |
|
Thanks |
ldez
added
kind/bug/fix
This PR solves the issue of passing traffic to a container before it's ready when using docker provider with swarm-mode and Traefik load-balancer (traefik.backend.loadbalancer.swarm=false), which is currently (V1.2-rc2) the default. (See issue #1238)
Current Tasks filter is based only on checking desired state, this PR adds an additional check on the current task state.
For reference, check lifecycle model in swarmkit: https://github.com/docker/swarmkit/blob/master/design/task_model.md#task-lifecycle
Fixes #1238