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

Fix docker issues with global and dead tasks #1167

Merged
merged 2 commits into from
Feb 23, 2017

Conversation

christopherobin
Copy link
Contributor

A couple small issues with Docker fixed in this PR:

  • Global tasks (docker service create --mode global) do not have a Slot ID in the task list, so each entry will end up having the same name in the backend configuration making the docker provider crash. This fallbacks to using the TaskID instead if the service is global.
  • Dead tasks would be also listed in traefik, added the desired-state: running filter to the task list query so that we only use alive tasks as backends.

Fixes #1138

@vdemeester vdemeester added this to the 1.2 milestone Feb 17, 2017
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

@vdemeester we could get this one in v1.2 right ?

@vdemeester
Copy link
Contributor

@emilevauge I think so

@emilevauge emilevauge changed the base branch from master to v1.2 February 21, 2017 10:30
@emilevauge emilevauge force-pushed the bugfix-docker branch 2 times, most recently from b84f030 to 69f939d Compare February 21, 2017 10:32
@emilevauge
Copy link
Member

Rebased on branch v1.2.
@christopherobin Do you think you could add a unit test on this ?

@christopherobin
Copy link
Contributor Author

@emilevauge I'll look into doing that asap.

I've also been using this branch on a local swarm cluster for about a week now and didn't run into any issue yet with both replicated and global services.

Copy link
Contributor

@Russell-IO Russell-IO left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@errm errm left a comment

Choose a reason for hiding this comment

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

LGTM, good work on the test, very clear and easy to understand...

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks @christopherobin !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants