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

pass sandbox resource requirements over CRI #104886

Merged
merged 7 commits into from
Oct 20, 2021

Conversation

egernst
Copy link
Contributor

@egernst egernst commented Sep 9, 2021

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR updates the CRI-api, adding two additional fields to LinuxPodSandboxConfig: Resources and Overhead. This API change was approved as part of the pod overhead KEP, but was not yet implemented.

Kubelet is also updated to populate these fields when generating the sandbox config with the sum of container resources and pod overhead, respectively.

These fields are important to underlying runtimes, like Kata Containers, to facilitate appropriate sandbox sizing. More specifically to Kata, with this a VM can be created with multi-queue for IO sized to match the expected number of vCPUs. Similarly, the sandbox can be created appropriately without requiring CPU/memory hot plug.

Special notes for your reviewer:

Majority of the code changes are:

  • added, expanded unit tests
  • minor refactoring for better code reuse

This change is based on top of #105772 in order to verify that the refactoring doesn't impact existing functionality (which didn't have great unit tests in the first place).

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/kubelet labels Sep 9, 2021
@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 9, 2021
@egernst
Copy link
Contributor Author

egernst commented Sep 10, 2021

/test pull-kubernetes-verify


@ehashman ehashman added this to Waiting on Author in SIG Node PR Triage Sep 10, 2021
@jiangliu
Copy link

We have handled CPU/memory request and limit, how about storage and ephemeral storage? We hope to assigned an ephemeral storage device for confidential containers, so we need to sum containers' request and pod overhead for ephemeral storage too.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 5, 2021
@egernst
Copy link
Contributor Author

egernst commented Oct 5, 2021

/test pull-kubernetes-e2e-gce-ubuntu-containerd

@egernst
Copy link
Contributor Author

egernst commented Oct 5, 2021

@jiangliu -- understand your point re: ephemeral storage. Today this isn't accounted for in terms of resource quota/scheduler, etc, so I'm not sure it'd necessarily make sense to add to this part of the interface. It seems reasonable, but I think it'd make sense as a separate PR.

This sound ok?

@egernst
Copy link
Contributor Author

egernst commented Oct 5, 2021

/test pull-kubernetes-verify-govet-levee

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 5, 2021
@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 20, 2021
@wgahnagl wgahnagl moved this from Triage to Needs Approver in SIG Node PR Triage Oct 20, 2021
@egernst
Copy link
Contributor Author

egernst commented Oct 20, 2021

Pushed update to:

  • address feedback from Tim on the return error handling (Thanks!)
  • address Jordan's suggestion for the resource helper function rename (thanks - names are the hardest part, this is much better now!)

Signed-off-by: Eric Ernst <eric_ernst@apple.com>
Signed-off-by: Eric Ernst <eric_ernst@apple.com>
Signed-off-by: Eric Ernst <eric_ernst@apple.com>
Signed-off-by: Eric Ernst <eric_ernst@apple.com>
Currently we only care about the sum of sandbox resources, which
includes a pod overhead if defined. We have a need for also calculating
*just* the sum of container requests/limits for CPU / Memory, so let's
do a refactor and expose this new helper function.

Signed-off-by: Eric Ernst <eric_ernst@apple.com>
Seperate the CPU/Memory req/limit -> linux resource conversion into its
own function for better reuse.

Elsewhere in kuberuntime pkg, we will want to leverage this
requests/limits to Linux Resource type conversion.

Signed-off-by: Eric Ernst <eric_ernst@apple.com>
Populate Resources and Overhead fields which, are now part of
LinuxPodSandboxConfig.

Signed-off-by: Eric Ernst <eric_ernst@apple.com>
@egernst
Copy link
Contributor Author

egernst commented Oct 20, 2021

Pushed update to:

@mrunalp
Copy link
Contributor

mrunalp commented Oct 20, 2021

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2021
@liggitt
Copy link
Member

liggitt commented Oct 20, 2021

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bergwolf, egernst, liggitt, mrunalp

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet