move Get*(v1.Pod) into a new podinfo package #82212

hprateek43 · 2019-09-01T06:46:17Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
This is in reference to PR #76990 referencing to the issue #71863. The previous PR was large so is being refactored into smaller PRs as per recommendation.
Which issue(s) this PR fixes:

Fixes #71863

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 · 2019-09-01T06:46:25Z

Hi @hprateek43. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

hprateek43 · 2019-09-01T06:49:39Z

@misterikkit @alculquicondor Can you review the changes in reference to PR #76990

draveness

/ok-to-test

draveness · 2019-09-01T07:00:48Z

pkg/api/pod/util.go

@@ -842,3 +844,14 @@ func multiplePodIPsInUse(podStatus *api.PodStatus) bool {
 	}
 	return false
 }
+
+// GetPodPriority returns priority of the given pod.
+func GetPodPriority(pod *v1.Pod) int32 {


Why are we moving this function here?

The podinfo package has exactly the same function.

Should I keep it here or in podinfo?

Should I keep it here or in podinfo?

We should keep it in podinfo if it works well in this PR.

wait, this is what the kubelet uses. But, this should actually go to pkg/api/v1/pod.

You could consider open a separate PR just for moving this function, as it involves a different set of reviewers.

Closing this PR and opening separate PR

draveness · 2019-09-01T07:12:34Z

pkg/kubelet/eviction/helpers.go

 	volumeutils "k8s.io/kubernetes/pkg/volume/util"
+	podinfo "k8s.io/kubernetes/pkg/api/pod"


this is weird and confusing, podutil could be a better same as other packages import this with podutil

draveness · 2019-09-01T07:12:53Z

pkg/scheduler/algorithm/predicates/predicates.go

 	"k8s.io/kubernetes/pkg/scheduler/volumebinder"
 	volumeutil "k8s.io/kubernetes/pkg/volume/util"
+	podinfo "k8s.io/kubernetes/pkg/scheduler/internal/podinfo"
+


draveness · 2019-09-01T07:13:01Z

pkg/scheduler/core/extender_test.go

@@ -37,6 +37,7 @@ import (
 	schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo"
 	schedulertesting "k8s.io/kubernetes/pkg/scheduler/testing"
 	"k8s.io/kubernetes/pkg/scheduler/util"
+	podinfo "k8s.io/kubernetes/pkg/api/pod"


draveness · 2019-09-01T07:13:07Z

pkg/scheduler/core/generic_scheduler.go

@@ -36,6 +36,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/errors"
 	corelisters "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/util/workqueue"
+	podinfo "k8s.io/kubernetes/pkg/api/pod"


draveness · 2019-09-01T07:15:19Z

pkg/scheduler/util/heap.go

@@ -240,12 +241,12 @@ func (h *Heap) Len() int {
 }

 // NewHeap returns a Heap which can be used to queue up items to process.
-func NewHeap(keyFn KeyFunc, lessFn LessFunc) *Heap {
+func NewHeap(keyFn KeyFunc, lessFn podinfo.LessFunc) *Heap {


We should move LessFunc to this file IMO.

@draveness Can you confirm the changes?

draveness · 2019-09-01T07:15:45Z

pkg/api/pod/util.go

@@ -842,3 +844,14 @@ func multiplePodIPsInUse(podStatus *api.PodStatus) bool {
 	}
 	return false
 }
+
+// GetPodPriority returns priority of the given pod.
+func GetPodPriority(pod *v1.Pod) int32 {


The podinfo package has exactly the same function.

k82cn · 2019-09-02T11:26:16Z

pkg/scheduler/internal/podinfo/podinfo_test.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package util
+package podinfo


why are we going to move this into internal util pkg? It maybe useful for customized scheduler by framework.

PR #76990 has discussion regarding moving this into an internal package

@k82cn Can you confirm if we can move it into an internal package?

k8s-ci-robot · 2019-09-03T12:58:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hprateek43
To complete the pull request process, please assign dims, thockin, wojtek-t
You can assign the PR to them by writing /assign @dims @thockin @wojtek-t in a comment when ready.

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

hprateek43 · 2019-09-04T13:27:48Z

/test pull-kubernetes-verify

hprateek43 · 2019-09-04T13:28:14Z

/test pull-kubernetes-kubemark-e2e-gce-big

k8s-ci-robot · 2019-09-04T14:23:39Z

@hprateek43: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-verify	`8fd998a`	link	`/test pull-kubernetes-verify`

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

Move GetPodPriority to /pkg/api/pod

7041722

k8s-ci-robot requested review from ahg-g and david-mcmahon September 1, 2019 06:47

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 1, 2019

draveness reviewed Sep 1, 2019

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 1, 2019

draveness reviewed Sep 1, 2019

View reviewed changes

draveness suggested changes Sep 1, 2019

View reviewed changes

k82cn reviewed Sep 2, 2019

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 3, 2019

hprateek43 force-pushed the Get(v1.Pod)Movement branch from 21d7703 to e05cdbb Compare September 3, 2019 12:58

k8s-ci-robot added area/conformance Issues or PRs related to kubernetes conformance tests sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Sep 3, 2019

k8s-ci-robot added the sig/release Categorizes an issue or PR as relevant to SIG Release. label Sep 3, 2019

hprateek43 force-pushed the Get(v1.Pod)Movement branch from e05cdbb to 495d109 Compare September 3, 2019 13:42

Test

8fd998a

hprateek43 force-pushed the Get(v1.Pod)Movement branch from 495d109 to 8fd998a Compare September 4, 2019 10:51

hprateek43 closed this Sep 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move Get*(v1.Pod) into a new podinfo package #82212

move Get*(v1.Pod) into a new podinfo package #82212

hprateek43 commented Sep 1, 2019 •

edited

k8s-ci-robot commented Sep 1, 2019

hprateek43 commented Sep 1, 2019 •

edited

draveness left a comment

draveness Sep 1, 2019

draveness Sep 1, 2019

hprateek43 Sep 3, 2019

draveness Sep 3, 2019 •

edited

alculquicondor Sep 4, 2019

alculquicondor Sep 4, 2019

hprateek43 Sep 8, 2019

draveness Sep 1, 2019

draveness Sep 1, 2019

draveness Sep 1, 2019

draveness Sep 1, 2019

draveness Sep 1, 2019

hprateek43 Sep 8, 2019

draveness Sep 1, 2019

k82cn Sep 2, 2019

hprateek43 Sep 3, 2019

hprateek43 Sep 8, 2019

k8s-ci-robot commented Sep 3, 2019

hprateek43 commented Sep 4, 2019

hprateek43 commented Sep 4, 2019

k8s-ci-robot commented Sep 4, 2019 •

edited

		volumeutils "k8s.io/kubernetes/pkg/volume/util"
		podinfo "k8s.io/kubernetes/pkg/api/pod"

move Get*(v1.Pod) into a new podinfo package #82212

move Get*(v1.Pod) into a new podinfo package #82212

Conversation

hprateek43 commented Sep 1, 2019 • edited

k8s-ci-robot commented Sep 1, 2019

hprateek43 commented Sep 1, 2019 • edited

draveness left a 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

draveness Sep 3, 2019 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 3, 2019

hprateek43 commented Sep 4, 2019

hprateek43 commented Sep 4, 2019

k8s-ci-robot commented Sep 4, 2019 • edited

hprateek43 commented Sep 1, 2019 •

edited

hprateek43 commented Sep 1, 2019 •

edited

draveness Sep 3, 2019 •

edited

k8s-ci-robot commented Sep 4, 2019 •

edited