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.

Sign up for GitHub

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

Jump to bottom

Add windows config to Kubelet CRI #57076

Merged
merged 2 commits into from
Feb 3, 2018
Merged

Add windows config to Kubelet CRI #57076

merged 2 commits into from
Feb 3, 2018

Conversation

feiskyer
Copy link
Member

@feiskyer feiskyer commented Dec 12, 2017 •
edited
Loading

What this PR does / why we need it:

Currently Container Runtime Interface (CRI) only supports LinuxContainerConfig and therefore LinuxContainerResources in ContainerConfig. Windows resource config is different from Linux's, although it shares some common properties.

This PR adds windows config to CRI. Add newly added WindowsContainerResources is original from OCI spec (see https://github.com/opencontainers/runtime-spec/blob/master/specs-go/config.go#L437).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

First part of #56734. A further PR is needed to fill the values after we have agreement on the spec.

Special notes for your reviewer:

Release note:

Add windows config to Kubelet CRI

/assign @yujuhong @brendandburns
/cc @taylorb-microsoft @JiangtianLi @dchen1107

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 12, 2017
@k8s-ci-robot
Copy link
Contributor

@feiskyer: GitHub didn't allow me to request PR reviews from the following users: taylorb-microsoft, JiangtianLi.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

What this PR does / why we need it:

Currently Container Runtime Interface (CRI) only supports LinuxContainerConfig and therefore LinuxContainerResources in ContainerConfig. Windows resource config is different from Linux's, although it shares some common properties.

This PR adds windows config to CRI. Add newly added WindowsContainerResources is original from OCI spec (see https://github.com/opencontainers/runtime-spec/blob/master/specs-go/config.go#L437).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

First part of #56734. A further PR is needed to fill the values after we have agreement on the spec.

Special notes for your reviewer:

Release note:

Add windows config to Kubelet CRI

/assign @yujuhong @brendandburns
/cc @taylorb-microsoft @JiangtianLi

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 12, 2017
@feiskyer feiskyer added sig/node Categorizes an issue or PR as relevant to SIG Node. sig/windows Categorizes an issue or PR as relevant to SIG Windows. labels Dec 12, 2017
@feiskyer
Copy link
Member Author

/retest

@brendandburns
Copy link
Contributor

This LGTM, but do we want SIG-Node to sign off on the API changes?

@kubernetes/sig-node-api-reviews

@k8s-ci-robot k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Dec 13, 2017
@feiskyer
Copy link
Member Author

This LGTM, but do we want SIG-Node to sign off on the API changes?

@brendandburns Yep, of course. @JiangtianLi is preparing a proposal and will present it on next sig-node meeting.

@JiangtianLi
Copy link
Contributor

I drafted the proposal here: kubernetes/community#1510.

@yujuhong
Copy link
Contributor

/hold
Let's discuss over the proposal now that there's one.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2017
@feiskyer feiskyer removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2018
JiangtianLi
JiangtianLi approved these changes Jan 18, 2018
View reviewed changes
Copy link
Contributor

@JiangtianLi JiangtianLi left a comment

Choose a reason for hiding this comment

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

lgtm

@feiskyer
Copy link
Member Author

Since windows pod-level resource management is not decided yet, I think we could add container resources first and address pod-level in another PRs. This PR only adds WindowsContainerResources to CRI.

@dchen1107 @yujuhong @brendandburns @PatrickLang PTAL

@feiskyer
Copy link
Member Author

ping @yujuhong @brendandburns Could you help to take a look at this?

@brendandburns
Copy link
Contributor

As I said, this looks fine to me, but I'd like an ack from @dchen1107 or @yujuhong

@yujuhong
Copy link
Contributor

Ack. Reviewing kubernetes/community#1510. Will review this PR once questions are resolved over there.

@dchen1107
Copy link
Member

ACK, and will review kubernetes/community#1510 first.

@dchen1107
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, feiskyer

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2018
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 59097, 57076, 59295). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit f02e37b into kubernetes:master Feb 3, 2018
@feiskyer feiskyer deleted the win-resources branch February 3, 2018 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JiangtianLi JiangtianLi JiangtianLi approved these changes

Assignees

@brendandburns brendandburns

@dchen1107 dchen1107

@yujuhong yujuhong

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@feiskyer @k8s-ci-robot @brendandburns @JiangtianLi @yujuhong @dchen1107 @k8s-github-robot