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

Topology-aware scheduling #30725

Merged
merged 1 commit into from Mar 3, 2017

Conversation

@aaronlehmann
Contributor

aaronlehmann commented Feb 3, 2017

This adds support for placement preferences in Swarm services (design doc: docker/swarmkit#1473, swarmkit PR: docker/swarmkit#1512). This feature allows services to be balanced over nodes based on a particular user-defined property, such as which datacenter or rack they are located in.

  • Convert PlacementPreferences between GRPC API and HTTP API
  • Add --placement-pref-add and --placement-pref-rm to CLI
  • Add support for placement preferences in service inspect --pretty
  • Add integration test

cc @aluzzardi @dongluochen @dnephin

@aaronlehmann aaronlehmann changed the title from [WIP] Topology-aware scheduling to Topology-aware scheduling Feb 7, 2017

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 7, 2017

Contributor

swarmkit PR was merged. No longer WIP.

Changed the command line syntax from spread,... to spread=... as suggested in docker/swarmkit#1512 (comment)

Contributor

aaronlehmann commented Feb 7, 2017

swarmkit PR was merged. No longer WIP.

Changed the command line syntax from spread,... to spread=... as suggested in docker/swarmkit#1512 (comment)

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Feb 8, 2017

Member

@aaronlehmann some test failures

22:47:48 # github.com/docker/docker/cli/command/service
22:47:48 cli/command/service/update_test.go:63: undefined: updatePlacement
Member

vdemeester commented Feb 8, 2017

@aaronlehmann some test failures

22:47:48 # github.com/docker/docker/cli/command/service
22:47:48 cli/command/service/update_test.go:63: undefined: updatePlacement
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 8, 2017

Contributor

Fixed, sorry about that.

Contributor

aaronlehmann commented Feb 8, 2017

Fixed, sorry about that.

yongtang added a commit to yongtang/docker that referenced this pull request Feb 8, 2017

Update SwarmKit to ed384f3b3957f65e3111bd020f9815f3d4296fa2
This fix updates SwarmKit to ed384f3b3957f65e3111bd020f9815f3d4296fa2.
Notable changes since last update (3ca4775ba4a5519e2225c3337c7db8901ec39d26):

1. Fix duplicated ports allocation with restarted swarm. (Docker issue moby#29247)
2. Topology-aware scheduling (Docker PR moby#30725)

Docker issue moby#29247 was labeled 1.13.1, though it is advised that
related SwarmKit changes only to be merged to master
(based on the feedback docker/swarmkit#1802 (comment))

This fix fixes moby#29247 (master only).
This fix is related to moby#30725.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@dnephin

dnephin approved these changes Feb 8, 2017

LGTM

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 11, 2017

Contributor

Rebased.

Contributor

aaronlehmann commented Feb 11, 2017

Rebased.

@vdemeester

LGTM 🐸
/cc @thaJeztah @vieux

Show outdated Hide outdated cli/command/service/opts.go Outdated
@cpuguy83

LGTM

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 13, 2017

Contributor

Needs some docs.

Contributor

cpuguy83 commented Feb 13, 2017

Needs some docs.

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Feb 14, 2017

Member

@aaronlehmann Ohh I didn't think of that (ordering). Hum I guess it makes sense (cc @thaJeztah)

Member

vdemeester commented Feb 14, 2017

@aaronlehmann Ohh I didn't think of that (ordering). Hum I guess it makes sense (cc @thaJeztah)

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 16, 2017

Member

Do we have other options that depend on ordering? I agree that -add is inconsistent from a UX perspective; almost tempted to say if ordering is important, it should be a comma-separated list.

If I understand correctly, these services have a very different placement preference;

docker service create \
  --name=serviceA \
  --placement-pref-add=one \
  --placement-pref-add=two \
  --placement-pref-add=three \
  foobar
docker service create \
  --name=serviceB \
  --placement-pref-add=three \
  --placement-pref-add=two \
  --placement-pref-add=one \
  foobar

How do I update my service from "serviceA" to "serviceB"? This would probably become;

docker service update \
  --placement-pref-rm=one \
  --placement-pref-rm=two \
  --placement-pref-rm=three \
  --placement-pref-add=three \
  --placement-pref-add=two \
  --placement-pref-add=one \
  serviceA

That's a bit awkward.

If it would be a comma-separated list, this would be;

docker service create \
  --name=serviceA \
  --placement-pref=one,two,three \
  foobar
docker service update \
  --placement-pref=three,two,one \
  serviceA

That's really just a quick thought, so putting it here for discussion

Member

thaJeztah commented Feb 16, 2017

Do we have other options that depend on ordering? I agree that -add is inconsistent from a UX perspective; almost tempted to say if ordering is important, it should be a comma-separated list.

If I understand correctly, these services have a very different placement preference;

docker service create \
  --name=serviceA \
  --placement-pref-add=one \
  --placement-pref-add=two \
  --placement-pref-add=three \
  foobar
docker service create \
  --name=serviceB \
  --placement-pref-add=three \
  --placement-pref-add=two \
  --placement-pref-add=one \
  foobar

How do I update my service from "serviceA" to "serviceB"? This would probably become;

docker service update \
  --placement-pref-rm=one \
  --placement-pref-rm=two \
  --placement-pref-rm=three \
  --placement-pref-add=three \
  --placement-pref-add=two \
  --placement-pref-add=one \
  serviceA

That's a bit awkward.

If it would be a comma-separated list, this would be;

docker service create \
  --name=serviceA \
  --placement-pref=one,two,three \
  foobar
docker service update \
  --placement-pref=three,two,one \
  serviceA

That's really just a quick thought, so putting it here for discussion

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 16, 2017

Contributor

Thanks for the thoughtful response. This was my initial instinct, but using a comma-separated list has some serious disadvantages as well. See the discussion thread starting at docker/swarmkit#1512 (comment) where this was discussed, and the alternative of using a repeated flag was encouraged.

Contributor

aaronlehmann commented Feb 16, 2017

Thanks for the thoughtful response. This was my initial instinct, but using a comma-separated list has some serious disadvantages as well. See the discussion thread starting at docker/swarmkit#1512 (comment) where this was discussed, and the alternative of using a repeated flag was encouraged.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Feb 21, 2017

Member

Looks like this needs a rebase.

Do we have other options that depend on ordering?

Yes we do, but Aaron noted that a change in order for most of those options is less significant than in this case.

Member

dnephin commented Feb 21, 2017

Looks like this needs a rebase.

Do we have other options that depend on ordering?

Yes we do, but Aaron noted that a change in order for most of those options is less significant than in this case.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 21, 2017

Contributor

Rebased

Contributor

aaronlehmann commented Feb 21, 2017

Rebased

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Feb 25, 2017

Member

I do agree with UX consistency as well...

--placement-pref would have the same behavior as --placement-pref-add during service create, it's just called differently...

Member

aluzzardi commented Feb 25, 2017

I do agree with UX consistency as well...

--placement-pref would have the same behavior as --placement-pref-add during service create, it's just called differently...

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 27, 2017

Contributor

Then is there a consensus around calling it --placement-pref in service create and --placement-pref-add in service update?

It's not my favorite outcome, but I don't see another way to move forward.

Contributor

aaronlehmann commented Feb 27, 2017

Then is there a consensus around calling it --placement-pref in service create and --placement-pref-add in service update?

It's not my favorite outcome, but I don't see another way to move forward.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Feb 27, 2017

Member

Yes, I think that is the most consistent naming

Member

dnephin commented Feb 27, 2017

Yes, I think that is the most consistent naming

Topology-aware scheduling
This adds support for placement preferences in Swarm services.

- Convert PlacementPreferences between GRPC API and HTTP API
- Add --placement-pref, --placement-pref-add and --placement-pref-rm to CLI
- Add support for placement preferences in service inspect --pretty
- Add integration test

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 27, 2017

Contributor

Okay, I've updated this to use --placement-pref for service create.

Contributor

aaronlehmann commented Feb 27, 2017

Okay, I've updated this to use --placement-pref for service create.

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Feb 27, 2017

Collaborator

that's very cool, LGTM!

Collaborator

vieux commented Feb 27, 2017

that's very cool, LGTM!

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Mar 1, 2017

Contributor

@thaJeztah: Are you happy with the latest change to the CLI flags?

Contributor

aaronlehmann commented Mar 1, 2017

@thaJeztah: Are you happy with the latest change to the CLI flags?

@thaJeztah

LGTM, new names for the flags work 👍

I see there's no man page yet for these commands, so I won't hold up this PR because of that (it's something to be addressed in general)

@thaJeztah thaJeztah merged commit 3a5a1c3 into moby:master Mar 3, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 31142 has succeeded
Details
janky Jenkins build Docker-PRs 39759 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 10821 has succeeded
Details

@aaronlehmann aaronlehmann deleted the aaronlehmann:topology branch Apr 5, 2017

@davidhiendl

This comment has been minimized.

Show comment
Hide comment
@davidhiendl

davidhiendl Apr 12, 2017

Is this supported when deploying services via docker-compose file yet? I can't seem to find any reference to it.

davidhiendl commented Apr 12, 2017

Is this supported when deploying services via docker-compose file yet? I can't seem to find any reference to it.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 12, 2017

Contributor

I don't believe it is supported in compose yet. I just filed #32584 to track this.

Contributor

aaronlehmann commented Apr 12, 2017

I don't believe it is supported in compose yet. I just filed #32584 to track this.

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

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