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

Adding compatible platforms to service spec #33239

Merged
merged 2 commits into from May 18, 2017

Conversation

Projects
None yet
6 participants
@nishanttotla
Contributor

nishanttotla commented May 17, 2017

Signed-off-by: Nishant Totla nishanttotla@gmail.com

This is a follow-up to #32388 (Related to: #31348).

It extracts platform information that is already present after the call to the /distribution/json endpoint (#32061), and adds it to the service spec. This platform information is used as a Swarm scheduling filter, to prevent tasks for running on nodes where they shouldn't run.

CLI changes in docker/cli#30 need to be merged before this actually is enabled end to end. We'll also have to merge #33228, which includes a SwarmKit bug fix that is required for smooth functioning of this feature.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 17, 2017

Contributor

Tests?

Contributor

aaronlehmann commented May 17, 2017

Tests?

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 17, 2017

Contributor

@aaronlehmann I think my commit was overwritten, so this is stale. Updating it.

Where should the tests ideally be put for this?

Contributor

nishanttotla commented May 17, 2017

@aaronlehmann I think my commit was overwritten, so this is stale. Updating it.

Where should the tests ideally be put for this?

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 17, 2017

Contributor

Where should the tests ideally be put for this?

client/service_create_test.go

Contributor

aaronlehmann commented May 17, 2017

Where should the tests ideally be put for this?

client/service_create_test.go

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 17, 2017

Contributor

@aaronlehmann updated PR and added a unit test (the unit test is somewhat of a hack to test the platform response at the API level, but hopefully it is okay).

Contributor

nishanttotla commented May 17, 2017

@aaronlehmann updated PR and added a unit test (the unit test is somewhat of a hack to test the platform response at the API level, but hopefully it is okay).

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 17, 2017

Contributor

Thanks. Could you please extend the unit test to check that the correct platform information is being passed to the /services/create endpoint?

Contributor

aaronlehmann commented May 17, 2017

Thanks. Could you please extend the unit test to check that the correct platform information is being passed to the /services/create endpoint?

Adding compatible platforms to service spec
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla
Contributor

nishanttotla commented May 17, 2017

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 17, 2017

Contributor

I'm not seeing it. The request body to /services/create is never unmarshalled.

Contributor

aaronlehmann commented May 17, 2017

I'm not seeing it. The request body to /services/create is never unmarshalled.

Updating test for compatible platforms to test unmarshal body
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 18, 2017

Contributor

LGTM

Contributor

aaronlehmann commented May 18, 2017

LGTM

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass May 18, 2017

Collaborator

LGTM, not sure if we should sort the platforms array but shouldn't be a blocker

Collaborator

tiborvass commented May 18, 2017

LGTM, not sure if we should sort the platforms array but shouldn't be a blocker

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass May 18, 2017

Collaborator

Should be fine like this, sorting is usually for display

Collaborator

tiborvass commented May 18, 2017

Should be fine like this, sorting is usually for display

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 18, 2017

Contributor

@tiborvass we could do it as a follow-up, it's not critical right now.

Contributor

nishanttotla commented May 18, 2017

@tiborvass we could do it as a follow-up, it's not critical right now.

@mavenugo mavenugo merged commit 4874e05 into moby:master May 18, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 34287 has succeeded
Details
janky Jenkins build Docker-PRs 42888 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 3273 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 14128 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2991 has succeeded
Details

@nishanttotla nishanttotla deleted the nishanttotla:add-platform-information-service-spec branch May 18, 2017

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