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 filter on task status in addition to desired status (Docker Provider - swarm) #1304

Merged
merged 3 commits into from Mar 20, 2017
Merged

Add filter on task status in addition to desired status (Docker Provider - swarm) #1304

merged 3 commits into from Mar 20, 2017

Conversation

Yshayy
Copy link
Contributor

@Yshayy Yshayy commented Mar 16, 2017

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

Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐸 👍

@emilevauge
Copy link
Member

@Yshayy Thanks for this :)
Could we have a unit test ?

@Yshayy
Copy link
Contributor Author

Yshayy commented Mar 16, 2017

Sure.
I'm not familiar with the docker test-suite, but it seems simple enough :)

@emilevauge
Copy link
Member

I added a unit test.
/cc @containous/traefik

Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐳

type fakeTasksClient struct {
dockerClient.APIClient
tasks []swarm.Task
err error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the test, you don't need that one 👼

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, indeed ^^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, as it is used in return statement of TaskList, we may leave it as is.

@@ -6,11 +6,15 @@ import (
"testing"

"github.com/containous/traefik/types"
"github.com/davecgh/go-spew/spew"
dockerClient "github.com/docker/engine-api/client"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be dockerclient. Go eschews camel case or even underscores in package names/aliases.

cases := []struct {
service swarm.Service
tasks []swarm.Task
isGlobalSvc bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be isGlobalSVC.

Signed-off-by: Emile Vauge <emile@vauge.com>
}

for _, e := range cases {
dockerData := parseService(e.service, e.networks)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 every t.{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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added sub tests in the whole file.


for i, taskID := range e.expectedTasks {
if taskDockerData[i].Name != taskID {
t.Fatalf("expect task id %v, got %v", taskID, taskDockerData[i].Name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this possibly be t.Errorf?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timoreimann On this, I suggest to make a global PR as a lot of test should be changed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timoreimann replaced in the whole file.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not ignore the error but check it and t.Fatal out if it turns out to be non-nil.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only thing I find import to change now; my other comments would be nice to have but don't seem critical.

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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

t.Fatalf("expected %q, got %q", e.expected, actual)
}
for containerID, e := range containers {
t.Run(strconv.Itoa(containerID), func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar with the other tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I forgot this awesome feature in go...

Thanks 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ROFLing so hard right now...

Signed-off-by: Emile Vauge <emile@vauge.com>
@emilevauge
Copy link
Member

@timoreimann fixes pushed :)

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 😊

@emilevauge emilevauge merged commit dd0a20a into traefik:v1.2 Mar 20, 2017
@Yshayy
Copy link
Contributor Author

Yshayy commented Mar 22, 2017

Thanks

@Yshayy Yshayy deleted the filter-non-running-tasks branch March 22, 2017 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants