Skip to content

Comments

Adding compatible platforms to service spec#33239

Merged
mavenugo merged 2 commits intomoby:masterfrom
nishanttotla:add-platform-information-service-spec
May 18, 2017
Merged

Adding compatible platforms to service spec#33239
mavenugo merged 2 commits intomoby:masterfrom
nishanttotla:add-platform-information-service-spec

Conversation

@nishanttotla
Copy link
Contributor

@nishanttotla nishanttotla commented May 17, 2017

Signed-off-by: Nishant Totla [email protected]

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.

Choose a reason for hiding this comment

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

This is changing the pointer, not the value

Choose a reason for hiding this comment

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

?

@aaronlehmann
Copy link

Tests?

@nishanttotla
Copy link
Contributor Author

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

Where should the tests ideally be put for this?

@aaronlehmann
Copy link

Where should the tests ideally be put for this?

client/service_create_test.go

@nishanttotla nishanttotla force-pushed the add-platform-information-service-spec branch from b338a13 to 6ff0852 Compare May 17, 2017 06:00
@thaJeztah thaJeztah added this to the 17.06.0 milestone May 17, 2017
@nishanttotla nishanttotla force-pushed the add-platform-information-service-spec branch 6 times, most recently from 5a1236e to 624bd2e Compare May 17, 2017 21:34
@nishanttotla
Copy link
Contributor Author

@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
Copy link

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

@nishanttotla nishanttotla force-pushed the add-platform-information-service-spec branch from 624bd2e to 587d07c Compare May 17, 2017 23:20
@nishanttotla
Copy link
Contributor Author

@aaronlehmann done.

@aaronlehmann
Copy link

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

Choose a reason for hiding this comment

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

This comparison won't work. They are both slices.

@nishanttotla nishanttotla force-pushed the add-platform-information-service-spec branch 2 times, most recently from 92e56c7 to c2c891a Compare May 18, 2017 00:06
@nishanttotla nishanttotla force-pushed the add-platform-information-service-spec branch from c2c891a to 7d4b8fb Compare May 18, 2017 00:12
@aaronlehmann
Copy link

LGTM

@tiborvass
Copy link
Contributor

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

@tiborvass
Copy link
Contributor

Should be fine like this, sorting is usually for display

@nishanttotla
Copy link
Contributor Author

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

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