Adding Platforms field to TaskSpec Placement#33144
Conversation
a44caad to
1b8e97d
Compare
api/swagger.yaml
Outdated
There was a problem hiding this comment.
Thinking of the ARM folks here -- do you want to include a variant as well?
There was a problem hiding this comment.
@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 moby/swarmkit#1981).
We could certainly add more in the future to make the comparison finer.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Yes, this should brought into line with the OCI fields.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@stevvooe @nishanttotla we can make those changes in 17.07 without causing backward compatibility issues?
There was a problem hiding this comment.
@nishanttotla just told me it would only be adding fields, so no issue there ^^
|
Why wouldn't this be read from the image manifest? |
|
@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. |
|
This is just bringing the engine's REST API in sync with a field that already exists in swarmkit's gRPC API. LGTM |
thaJeztah
left a comment
There was a problem hiding this comment.
@nishanttotla can you add a note to the API history? https://github.com/moby/moby/blob/master/docs/api/version-history.md
8376a47 to
64d5fc1
Compare
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
64d5fc1 to
1efbe6e
Compare
|
Ping @thaJeztah addressed your concerns. cc @aaronlehmann updated the relevant functions in |
|
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 |
|
I confirmed that the swagger issues are unrelated, and are present in master. Filed #33231 |
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
@nishanttotla if you have time, could you (as follow-up) also update the example-request for service create?
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 moby/swarmkit#1981 and vendored into
moby/mobywith #33059.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.