Skip to content
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

[Proof of concept] Add Windows fields #504

Closed
wants to merge 1 commit into from

Conversation

lowenna
Copy link
Contributor

@lowenna lowenna commented Jun 21, 2016

Signed-off-by: John Howard John.Howard@microsoft.com

Would like some feedback on this approach. This is the first step in removing the 'frankenstein' version of the spec used by Docker on Windows (https://github.com/docker/docker/blob/master/libcontainerd/windowsoci/oci_windows.go), which was based on a slightly earlier version of the OCI runtime spec (circa March 2016).

In particular, I'd like to get feedback on the best approach to have common structure names (eg Resources, CPU, Memory, Network etc) which are actually quite different in their contents between Windows and other platforms. I noticed that since I first took a cut of the OCI spec for use in Docker, the Solaris additions have been made, so I followed the same pattern for adding a Windows structure to the main Spec structure.

However, for those common structures, I modified it to be platform specific by prefixing the structure name with the platform. Hence Resources is renamed LinuxResources, and a new WindowsResources structure has been added, as an example.

For structures which are very similar, I just added Windows specific fields, which is a pattern which already exists in the spec.

Hopefully that's there is concurrence in the above approach 😄

@@ -25,6 +25,8 @@ type Spec struct {
Linux Linux `json:"linux" platform:"linux,omitempty"`
// Solaris is platform specific configuration for Solaris containers.
Solaris Solaris `json:"solaris" platform:"solaris,omitempty"`
// Windows is the platform specific configuration for Windows based containers.
Windows Windows `json:"windows" platform:"windows,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is matching a broken pattern ;). See #502 for what I think we should be using instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems sensible. Updated to match #502.

@wking
Copy link
Contributor

wking commented Jun 22, 2016

On Tue, Jun 21, 2016 at 04:47:42PM -0700, John Howard wrote:

Would like some feedback on this approach.

I left a few nits as inline comments, but the general approach you
have here looks pretty good to me (and it matches our current sketch
of a policy 1). My main concern is about future type-name churn
impacting downstream Go users, and I left suggestions on addressing
that here 2.

Signed-off-by: John Howard <John.Howard@microsoft.com>
}

// Networking contains the network configuration for Windows based containers.
type Networking struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably be renamed once we determine if this is necessary.

type User struct {
// UID is the user id. (this field is platform dependent)
UID uint32 `json:"uid" platform:"linux,solaris"`
// GID is the group id. (this field is platform dependent)
GID uint32 `json:"gid" platform:"linux,solaris"`
// AdditionalGids are additional group ids set for the container's process. (this field is platform dependent)
AdditionalGids []uint32 `json:"additionalGids,omitempty" platform:"linux,solaris"`
// User is the user name. (this field is platform dependent)
User string `json:"user" platform:"windows"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason why it's not username, and can we expect a groupname in the future ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a proof-of-concept PR. In the actual PR (now in master), it's username. https://github.com/opencontainers/runtime-spec/blob/master/specs-go/config.go#L65. There are no plans for groups in the future (it's not really something that makes sense on Windows).

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, was just curious. Thanks.

@lowenna
Copy link
Contributor Author

lowenna commented Sep 17, 2016

Closing this proof-of-concept PR now there are multiple PRs out to add support for Windows and address other comments in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants