Move some helper functions from api/v1 to component-helpers #124609

AxeZhan · 2024-04-29T12:14:15Z

What type of PR is this?

/kind cleanup
/kind feature

What this PR does / why we need it:

We currently have some functions which are used by many components,
for example:

kubernetes/pkg/api/v1/resource/helpers.go

Line 50 in ae02f87

func PodRequests(pod *v1.Pod, opts PodResourcesOptions) v1.ResourceList {

It would be useful to have them in a library package.

Which issue(s) this PR fixes:

Part of #124537

Special notes for your reviewer:

First commit moves the file.
Second commit only do the rename.

Does this PR introduce a user-facing change?

NONE

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

k8s-ci-robot · 2024-04-29T12:14:24Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

staging/src/k8s.io/component-helpers/pod/util.go

pkg/controller/controller_utils.go

staging/src/k8s.io/component-helpers/pod/util.go

alculquicondor · 2024-04-30T13:11:26Z

staging/src/k8s.io/component-helpers/resource/helpers.go

+
+// AllFeatureEnabledContainers returns a ContainerType mask which includes all container
+// types except for the ones guarded by feature gate.
+func AllFeatureEnabledContainers() ContainerType {


Let's remove this function. It's probably legacy from the time when ephemeral containers were still alpha/beta.

alculquicondor · 2024-04-30T13:12:01Z

staging/src/k8s.io/component-helpers/resource/OWNERS

@SergeyKanzhelev I'm not sure who should own this.

WDYT?

sig node is a good here. InPlace and Sidecar are both driven by SIG Node and making changes to this. But we can share with the sig scheduling.

I am thinking if this must be considered a public API and api-approvers needs to be notified for changes here

@kubernetes/api-reviewers should we add api review label for changes in these files? Or need to add it manually?

(we added the note The computation is part of the API and must be reviewed as an API change. to the function, but not sure if this is enough)

Yeah, not just notified, but approval constrained to API review. We've intentionally avoided relocating API validation functions and functions used by validation out of API packages for that reason in the past before… it's too easy to accidentally tighten or loosen validation in ways that cause issues without realizing it when the functions are in packages with more broad approval.

That's not to say that relocation can't happen here (I haven't looked through it yet to know what the exposure is), we just need to make sure changes are done as carefully as needed

is there a diff of the content in the old location and new location to make sure no functionality is changing in the move for pkg/api/v1/resource/helpers.go and pkg/api/v1/resource/helpers_test.go?

here're the diff links:
helpers.go
helpers_test.go

Also, I split the commits by file. But git diff seems not very useful :(

@liggitt Can you review again? Diff files seem fine.
But notice that I move ContainerType from pkg/api/pod/util.go to component-helpers/resource/helpers.go too. Becuase PodRequests function uses ContainerType, and import pkg v1 in staging is not allowed.

cici37 · 2024-04-30T20:16:29Z

/remove-sig api-machinery

staging/src/k8s.io/component-helpers/resource/OWNERS

pkg/kubelet/container/helpers.go

alculquicondor · 2024-05-01T13:35:03Z

staging/src/k8s.io/component-helpers/resource/helpers.go

+	}
+
+	for _, container := range pod.Spec.Containers {
+		containerReqs := container.Resources.Requests


Question for @liggitt: would it be fine to also incorporate the logic of "requests = limits, when not defined".

kubernetes/pkg/apis/core/v1/defaults.go

Line 158 in b041e54

func SetDefaults_Pod(obj *v1.Pod) {

two thoughts:

don't mix relocation and modification into the same PR

why would we need to duplicate API defaulting logic here? admission / controllers / clients reading from the API will already see pods with the defaults applied

I agree.

Because users of this helper might be using a Pod template that didn't pass through the apiserver yet. For example, when using an external CRD such as a kubeflow TFJob.

but... the input to this function is Pod, not PodTemplate

Yeah, but one can easily convert one to the other in order to get a resource calculation.

cici37 · 2024-05-02T16:37:54Z

/remove-sig api-machinery

pkg/kubelet/container/helpers.go

liggitt · 2024-05-09T14:34:02Z

staging/src/k8s.io/component-helpers/resource/OWNERS

is there a diff of the content in the old location and new location to make sure no functionality is changing in the move for pkg/api/v1/resource/helpers.go and pkg/api/v1/resource/helpers_test.go?

staging/src/k8s.io/component-helpers/resource/OWNERS

pkg/api/pod/util.go

k8s-ci-robot · 2024-05-09T16:15:42Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AxeZhan
Once this PR has been reviewed and has the lgtm label, please assign deads2k, kerthcet, msau42, sjenning for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

pkg/api/OWNERS
pkg/kubelet/OWNERS
pkg/quota/v1/OWNERS
pkg/scheduler/OWNERS
pkg/security/apparmor/OWNERS
pkg/volume/OWNERS
staging/src/k8s.io/component-helpers/OWNERS

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

k8s-ci-robot · 2024-05-09T17:07:35Z

@AxeZhan: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-autoscaling-hpa-cpu	`1ef33db`	link	false	`/test pull-kubernetes-e2e-autoscaling-hpa-cpu`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

fedebongio · 2024-05-16T16:33:50Z

/remove-sig api-machinery

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Apr 29, 2024

k8s-ci-robot requested review from alculquicondor and andrewsykim April 29, 2024 12:14

AxeZhan mentioned this pull request Apr 29, 2024

A library function to calculate Pod resource utilization #124537

Open

neolit123 reviewed Apr 29, 2024

View reviewed changes

staging/src/k8s.io/component-helpers/pod/util.go Outdated Show resolved Hide resolved

alculquicondor reviewed Apr 29, 2024

View reviewed changes

pkg/controller/controller_utils.go Outdated Show resolved Hide resolved

staging/src/k8s.io/component-helpers/pod/util.go Outdated Show resolved Hide resolved

AxeZhan force-pushed the refac branch from 1ef33db to 707a930 Compare April 30, 2024 05:43

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 30, 2024

AxeZhan force-pushed the refac branch 2 times, most recently from 33c6589 to b9d1030 Compare April 30, 2024 05:53

alculquicondor reviewed Apr 30, 2024

View reviewed changes

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Apr 30, 2024

alculquicondor reviewed May 1, 2024

View reviewed changes

staging/src/k8s.io/component-helpers/resource/OWNERS Outdated Show resolved Hide resolved

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 1, 2024

alculquicondor reviewed May 1, 2024

View reviewed changes

AxeZhan force-pushed the refac branch from fe4128f to 487e291 Compare May 1, 2024 14:01

bart0sh added this to not-sig-node in SIG Node PR Triage May 1, 2024

SergeyKanzhelev added this to Triage in SIG Node CI/Test Board May 1, 2024

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 2, 2024

haircommander moved this from Triage to Archive-it in SIG Node CI/Test Board May 8, 2024

liggitt reviewed May 9, 2024

View reviewed changes

pkg/kubelet/container/helpers.go Show resolved Hide resolved

liggitt reviewed May 9, 2024

View reviewed changes

create owners

c8de6b8

AxeZhan force-pushed the refac branch from 487e291 to 2e0b370 Compare May 9, 2024 15:12

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 9, 2024

AxeZhan force-pushed the refac branch from 2e0b370 to 5ac4d42 Compare May 9, 2024 15:22

AxeZhan added 3 commits May 9, 2024 23:56

move helpers.go

2d04866

rename

bf4f783

move helpers_test.go

2a5239a

AxeZhan force-pushed the refac branch from 5ac4d42 to 2a5239a Compare May 9, 2024 16:15

format

3474481

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move some helper functions from api/v1 to component-helpers #124609

Move some helper functions from api/v1 to component-helpers #124609

AxeZhan commented Apr 29, 2024

k8s-ci-robot commented Apr 29, 2024

alculquicondor Apr 30, 2024

SergeyKanzhelev Apr 30, 2024

alculquicondor Apr 30, 2024

SergeyKanzhelev Apr 30, 2024

SergeyKanzhelev Apr 30, 2024

SergeyKanzhelev Apr 30, 2024

liggitt Apr 30, 2024 •

edited

liggitt May 9, 2024

AxeZhan May 9, 2024 •

edited

AxeZhan May 10, 2024

cici37 commented Apr 30, 2024

alculquicondor May 1, 2024

liggitt May 9, 2024

alculquicondor May 9, 2024

liggitt May 9, 2024

alculquicondor May 10, 2024

cici37 commented May 2, 2024

liggitt May 9, 2024

k8s-ci-robot commented May 9, 2024

k8s-ci-robot commented May 9, 2024 •

edited

fedebongio commented May 16, 2024

Move some helper functions from api/v1 to component-helpers #124609

Are you sure you want to change the base?

Move some helper functions from api/v1 to component-helpers #124609

Conversation

AxeZhan commented Apr 29, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Apr 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AxeZhan May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cici37 commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cici37 commented May 2, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented May 9, 2024

k8s-ci-robot commented May 9, 2024 • edited

fedebongio commented May 16, 2024

liggitt Apr 30, 2024 •

edited

AxeZhan May 9, 2024 •

edited

k8s-ci-robot commented May 9, 2024 •

edited