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
Move some helper functions from api/v1 to component-helpers #124609
base: master
Are you sure you want to change the base?
Conversation
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
33c6589
to
b9d1030
Compare
|
||
// AllFeatureEnabledContainers returns a ContainerType mask which includes all container | ||
// types except for the ones guarded by feature gate. | ||
func AllFeatureEnabledContainers() ContainerType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove this function. It's probably legacy from the time when ephemeral containers were still alpha/beta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SergeyKanzhelev I'm not sure who should own this.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking if this must be considered a public API and api-approvers needs to be notified for changes here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here're the diff links:
helpers.go
helpers_test.go
Also, I split the commits by file. But git diff seems not very useful :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
/remove-sig api-machinery |
} | ||
|
||
for _, container := range pod.Spec.Containers { | ||
containerReqs := container.Resources.Requests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but... the input to this function is Pod, not PodTemplate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but one can easily convert one to the other in order to get a resource calculation.
/remove-sig api-machinery |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AxeZhan The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@AxeZhan: The following test failed, say
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. |
/remove-sig api-machinery |
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
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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: