Skip to content

Moving a dependency on pkg/api/v1/pod and switching to component-helper #90039

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
Closed
wants to merge 4 commits into from
Closed

Moving a dependency on pkg/api/v1/pod and switching to component-helper #90039

wants to merge 4 commits into from

Conversation

yuzhiquan
Copy link
Member

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
(cleanup): remove direct import k8s.io/kubernetes/pkg/api/v1/pod to pkg/scheduler/util
There is other refer usage, so does not remove older function.
Which issue(s) this PR fixes:

Ref #89930

Special notes for your reviewer:

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 release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 10, 2020
@yuzhiquan
Copy link
Member Author

/assign @Huang-Wei

@yuzhiquan yuzhiquan force-pushed the feature-remove-podutil branch from 5e7800b to 6cd820a Compare April 10, 2020 04:30
@yuzhiquan
Copy link
Member Author

/priority backlog

@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 10, 2020
@yuzhiquan yuzhiquan force-pushed the feature-remove-podutil branch from ba1954b to 3796cc4 Compare April 11, 2020 15:30
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Apr 11, 2020
@yuzhiquan yuzhiquan force-pushed the feature-remove-podutil branch from d61dbee to edc3239 Compare April 12, 2020 03:11
@yuzhiquan
Copy link
Member Author

yuzhiquan commented Apr 13, 2020 •
edited
Loading

/assign @damemi
PLZ have a reivew?

@alculquicondor
Copy link
Member

What is the other import that you are referring to?

Could we not overload the pkg/scheduler/util package and have some kind of podhelpers or apihelpers package?

@alculquicondor alculquicondor mentioned this pull request Apr 14, 2020
Closed
@damemi
Copy link
Member

damemi commented Apr 14, 2020

@alculquicondor from a quick look, outside of scheduler these helpers are used in:
GetPodPriority:

  • pkg/api/v1/pod/util_test.go
  • pkg/kubelet/eviction/helpers.go

UpdatePodCondition:

  • pkg/api/v1/pod/util_test.go
  • pkg/controller/daemon/daemon_controller_test.go
  • pkg/controller/statefulset/stateful_set_{control,utils}_test.go
  • pkg/controller/util/node/controller_utils.go

GetPodConditionFromList is called in a couple other places but for the scheduler it's only used within the other functions. GetPodCondition is only used in pkg/api/v1/pod and pkg/kubelet/status.

By enumerating these, would it be reasonable for some of these packages to call into the scheduler helpers, given that we ultimately might want to externalize those helpers anyway?

@alculquicondor
Copy link
Member

By enumerating these, would it be reasonable for some of these packages to call into the scheduler helpers, given that we ultimately might want to externalize those helpers anyway?

That sounds quite reasonable. But also that's another argument for making a package that is as narrow as possible. In this case, we are trying to expose helpers around core APIs.

@yuzhiquan
Copy link
Member Author

Function in pkg/scheduler/util/utils.go is refer to pod operation, so remove all of them to new pkg, like pkg/apis/scheduling/podhelper ?

@alculquicondor
Copy link
Member

alculquicondor commented Apr 15, 2020 •
edited
Loading

or perhaps podutil, so that functions can be something like: podutil.GetAffinityTerms

@alculquicondor
Copy link
Member

cc @Huang-Wei

@alculquicondor
Copy link
Member

/hold

I prefer that we fix #89092 first, so that we can carry out patches more easily to older releases.

@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 Apr 15, 2020
@k8s-ci-robot k8s-ci-robot reopened this Dec 2, 2021
@k8s-ci-robot
Copy link
Contributor

@yuzhiquan: Reopened this PR.

In response to this:

/reopen

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.

@yuzhiquan yuzhiquan force-pushed the feature-remove-podutil branch from d525c07 to 984aded Compare December 8, 2021 02:31
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 8, 2021
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2021
@yuzhiquan
Copy link
Member Author

ping @alculquicondor @damemi @ehashman @soltysh for review and approve, this pr last for many days

@yuzhiquan yuzhiquan force-pushed the feature-remove-podutil branch from 568eb11 to 77bc1bf Compare December 8, 2021 06:13
alculquicondor
alculquicondor reviewed Dec 8, 2021
View reviewed changes
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2021
@yuzhiquan yuzhiquan force-pushed the feature-remove-podutil branch from 77bc1bf to ae5da6e Compare December 21, 2021 07:59
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 21, 2021
@haosdent
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 24, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: damemi, soltysh, yuzhiquan
To complete the pull request process, please ask for approval from lavalamp after the PR has been reviewed.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@yuzhiquan
Copy link
Member Author

ping @lavalamp for approve

@yuzhiquan
Copy link
Member Author

ping @alculquicondor for approve

@alculquicondor
Copy link
Member

You already have @damemi's approval for sig scheduling

@yuzhiquan
Copy link
Member Author

ping @ehashman for kubelet part approve

@yuzhiquan
Copy link
Member Author

ping @liggitt for api part approve.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2022
@k8s-ci-robot
Copy link
Contributor

@yuzhiquan: PR needs rebase.

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-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alculquicondor alculquicondor alculquicondor left review comments

@damemi damemi damemi approved these changes

@ahg-g ahg-g ahg-g left review comments

@soltysh soltysh soltysh approved these changes

@liggitt liggitt Awaiting requested review from liggitt

@mrunalp mrunalp Awaiting requested review from mrunalp

Assignees

@soltysh soltysh

@lavalamp lavalamp

@Huang-Wei Huang-Wei

@damemi damemi

@derekwaynecarr derekwaynecarr

@deads2k deads2k

Labels
area/dependency Issues or PRs related to dependency changes area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node CI/Test Board
Archived in project
SIG Node: code and documentation PRs
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.