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 Platforms field to TaskSpec Placement #33144

Merged
merged 1 commit into from May 16, 2017

Conversation

Projects
None yet
7 participants
@nishanttotla
Contributor

nishanttotla commented May 10, 2017

The Platforms field contains an array of all platforms supported by the Task. This PR is part of adding multi-arch support for Swarm Mode (#31348).

The platform filter was introduced in SwarmKit in docker/swarmkit#1981 and vendored into moby/moby with #33059.

// Placement represents orchestration parameters.
type Placement struct {
	Constraints []string              `json:",omitempty"`
	Preferences []PlacementPreference `json:",omitempty"`

	// Platforms stores all the platforms that the image can run on.
	// This field is used in the platform filter for scheduling. If empty,
	// then the platform filter is off, meaning there are no scheduling restrictions.
	Platforms []Platform `json:",omitempty"`
}

The follow-up for this PR will actually add the platform information to the Spec when a service is created. That part depends on #32388.

@nishanttotla nishanttotla changed the title from Adding Platforms field to TaskSpec to Adding Platforms field to TaskSpec Placement May 10, 2017

items:
type: "object"
properties:
Architecture:

This comment has been minimized.

@clnperez

clnperez May 11, 2017

Contributor

Thinking of the ARM folks here -- do you want to include a variant as well?

@clnperez

clnperez May 11, 2017

Contributor

Thinking of the ARM folks here -- do you want to include a variant as well?

This comment has been minimized.

@nishanttotla

nishanttotla May 11, 2017

Contributor

@clnperez I'm not familiar with what a variant is, could you clarify? Also, Swarm currently only has Architecture and OS fields, which is why this structure has only those (see docker/swarmkit#1981).

We could certainly add more in the future to make the comparison finer.

@nishanttotla

nishanttotla May 11, 2017

Contributor

@clnperez I'm not familiar with what a variant is, could you clarify? Also, Swarm currently only has Architecture and OS fields, which is why this structure has only those (see docker/swarmkit#1981).

We could certainly add more in the future to make the comparison finer.

This comment has been minimized.

@clnperez

clnperez May 11, 2017

Contributor

@nishanttotla Hopefully I'll get this all straight, but as far as I'm understanding, ARM has a specific need for architecture variants -- but they haven't completely ironed out what they'll call them. I've seen things like arm64_ilp32_variant8 and arm32_variant6, as examples. The real point is that the arch isn't always the whole story when choosing the image to pull. Right now the engine only looks at $GOARCH + $GOOS to decide which image to pull in the case of multi-arch, but I believe that in the future it will need to also consider the variant.

@stevvooe can correct anything I've gotten wrong here.

@clnperez

clnperez May 11, 2017

Contributor

@nishanttotla Hopefully I'll get this all straight, but as far as I'm understanding, ARM has a specific need for architecture variants -- but they haven't completely ironed out what they'll call them. I've seen things like arm64_ilp32_variant8 and arm32_variant6, as examples. The real point is that the arch isn't always the whole story when choosing the image to pull. Right now the engine only looks at $GOARCH + $GOOS to decide which image to pull in the case of multi-arch, but I believe that in the future it will need to also consider the variant.

@stevvooe can correct anything I've gotten wrong here.

This comment has been minimized.

@nishanttotla

nishanttotla May 11, 2017

Contributor

@clnperez thanks for the info. It sounds like we should consider this once the details are well-defined. Right now, the basic platform comparison should suffice IMO.

@nishanttotla

nishanttotla May 11, 2017

Contributor

@clnperez thanks for the info. It sounds like we should consider this once the details are well-defined. Right now, the basic platform comparison should suffice IMO.

This comment has been minimized.

@stevvooe

stevvooe May 12, 2017

Contributor

Yes, this should brought into line with the OCI fields.

@stevvooe

stevvooe May 12, 2017

Contributor

Yes, this should brought into line with the OCI fields.

This comment has been minimized.

@nishanttotla

nishanttotla May 12, 2017

Contributor

The OCI platform struct seems identical to manifestlist.PlatformSpec, which is what this is meant to consume.

We should do this for 17.07, given that the filter isn't going to change right now.

@nishanttotla

nishanttotla May 12, 2017

Contributor

The OCI platform struct seems identical to manifestlist.PlatformSpec, which is what this is meant to consume.

We should do this for 17.07, given that the filter isn't going to change right now.

This comment has been minimized.

@stevvooe

stevvooe May 15, 2017

Contributor

The OCI platform struct seems identical to manifestlist.PlatformSpec, which is what this is meant to consume.

It is slightly different. You should err on using the OCI version, where possible.

The only thing to keep in line here is that Architecture should always use a GOARCH value.

@stevvooe

stevvooe May 15, 2017

Contributor

The OCI platform struct seems identical to manifestlist.PlatformSpec, which is what this is meant to consume.

It is slightly different. You should err on using the OCI version, where possible.

The only thing to keep in line here is that Architecture should always use a GOARCH value.

This comment has been minimized.

@thaJeztah

thaJeztah May 16, 2017

Member

@stevvooe @nishanttotla we can make those changes in 17.07 without causing backward compatibility issues?

@thaJeztah

thaJeztah May 16, 2017

Member

@stevvooe @nishanttotla we can make those changes in 17.07 without causing backward compatibility issues?

This comment has been minimized.

@thaJeztah

thaJeztah May 16, 2017

Member

@nishanttotla just told me it would only be adding fields, so no issue there ^^

@thaJeztah

thaJeztah May 16, 2017

Member

@nishanttotla just told me it would only be adding fields, so no issue there ^^

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 11, 2017

Contributor

Why wouldn't this be read from the image manifest?

Contributor

cpuguy83 commented May 11, 2017

Why wouldn't this be read from the image manifest?

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 11, 2017

Contributor

@cpuguy83 this will be retrieved from the image manifest when the client creates the service spec. It will then be used by the Swarm scheduler.

Contributor

nishanttotla commented May 11, 2017

@cpuguy83 this will be retrieved from the image manifest when the client creates the service spec. It will then be used by the Swarm scheduler.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 11, 2017

Contributor

This is just bringing the engine's REST API in sync with a field that already exists in swarmkit's gRPC API.

LGTM

Contributor

aaronlehmann commented May 11, 2017

This is just bringing the engine's REST API in sync with a field that already exists in swarmkit's gRPC API.

LGTM

Adding Platforms field to TaskSpec
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 15, 2017

Contributor

Ping @thaJeztah addressed your concerns.

cc @aaronlehmann updated the relevant functions in daemon/convert.

Contributor

nishanttotla commented May 15, 2017

Ping @thaJeztah addressed your concerns.

cc @aaronlehmann updated the relevant functions in daemon/convert.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 16, 2017

Member

Looks like theres an issue with the swagger.yml. If I render the docs, and go to the "service create" endpoint http://localhost:9000/#operation/ServiceCreate, I can no longer expand the TaskTemplate node

Member

thaJeztah commented May 16, 2017

Looks like theres an issue with the swagger.yml. If I render the docs, and go to the "service create" endpoint http://localhost:9000/#operation/ServiceCreate, I can no longer expand the TaskTemplate node

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 16, 2017

Contributor

I confirmed that the swagger issues are unrelated, and are present in master. Filed #33231

Contributor

nishanttotla commented May 16, 2017

I confirmed that the swagger issues are unrelated, and are present in master. Filed #33231

@thaJeztah

LGTM

@nishanttotla if you have time, could you (as follow-up) also update the example-request for service create?

@thaJeztah thaJeztah added this to the 17.06.0 milestone May 16, 2017

@cpuguy83 cpuguy83 merged commit 7b7f9a4 into moby:master May 16, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 34182 has succeeded
Details
janky Jenkins build Docker-PRs 42747 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 3131 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 13979 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2847 has succeeded
Details

@nishanttotla nishanttotla deleted the nishanttotla:update-swarmkit-platform-structs branch May 16, 2017

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