Refactored pkg/scheduler/utils to podinfo and heap packages. #76990

hprateek43 · 2019-04-24T06:57:22Z

What type of PR is this?
kind cleanup

What this PR does / why we need it:
#71863 is an active open request for scheduler cleanup. I have picked up that request and moved the generic util package into more meaningful internal packages podinfo and heap. Necessary changes in the scheduler and kubelet eviction packages have been made to support those changes.

Fixes #71863

Special notes for your reviewer:
The previous PR used schedulerinternalpodinfo as the package reference.The name was found to be too long by members in the previous thread and has been reduced to just podinfo. Let me know if we need any refactoring to rename this to something more meaningful. Also the package is in /pkg/scheduler/internal/podinfo and /pkg/scheduler/internal/heap, so the word internal in path can be a potential break. Comment if we can move them to more meaningful areas.

NONE

k8s-ci-robot · 2019-04-24T06:57:31Z

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-04-24T07:14:05Z

@misterikkit Can you review this PR and let me know the changes ?

hprateek43 · 2019-04-25T17:44:09Z

@lavalamp @derekwaynecarr Can you review this request?

lavalamp · 2019-04-25T18:11:00Z

You should get a scheduler-specific review, e.g. @bsalamat or @misterikkit.

I'm not sure why the go.sum files all changed; also, the commits seem arranged sub-optimally (consider using fetch & rebase rather than merge?).

liggitt · 2019-04-25T20:10:45Z

go.mod

@@ -157,6 +157,7 @@ require (
 	github.com/storageos/go-api v0.0.0-20180912212459-343b3eff91fc
 	github.com/stretchr/testify v1.2.2
 	github.com/syndtr/gocapability v0.0.0-20160928074757-e7cb7fa329f4 // indirect
+	github.com/tools/godep v0.0.0-20180126220526-ce0bfadeb516 // indirect


drop this commit

Yeah, this shouldn't be included in the PR. Is the change needed at all?

@misterikkit @liggitt Squashed and rebased. Dropped commits as per recommendation

liggitt · 2019-04-25T20:11:10Z

staging/src/k8s.io/api/go.sum

@@ -30,11 +30,14 @@ github.com/spf13/pflag v1.0.1 h1:aCvUg6QPl3ibpQUxyLkrEkCHtPqYJL4x9AuhqVqFis4=
 github.com/spf13/pflag v1.0.1/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4=
 github.com/stretchr/testify v1.2.2 h1:bSDNvY7ZPG5RlJ8otE/7V6gMiyenm9RtJ7IUVIAoJ1w=
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
+golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=


drop this commit as well

@liggitt Dropped

misterikkit

/lgtm

hprateek43 · 2019-08-06T17:45:26Z

@bowei @andrewsykim @bsalamat Can you review and approve this PR?

hprateek43 · 2019-08-07T11:36:59Z

@derekwaynecarr @vishh @dchen1107 @dashpole @sjenning Can one of you review this PR and give an approval ?

derekwaynecarr · 2019-08-07T20:18:05Z

kubelet changes lgtm.

/approve
/lgtm

k8s-ci-robot · 2019-08-07T20:18:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deads2k, derekwaynecarr, hprateek43
To complete the pull request process, please assign ravisantoshgudimetla
You can assign the PR to them by writing /assign @ravisantoshgudimetla 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:

~~pkg/controller/OWNERS~~ [deads2k,derekwaynecarr]
~~pkg/kubelet/eviction/OWNERS~~ [derekwaynecarr]
pkg/scheduler/OWNERS
~~test/OWNERS~~ [deads2k]

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

hprateek43 · 2019-08-08T03:42:35Z

/assign @ravisantoshgudimetla

draveness · 2019-08-09T15:10:58Z

pkg/scheduler/internal/channels/error_channel.go

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

-package util
+package channels


channels may not be a good name for package, channel would be better as a package name here.

I would be more generic and name it something like asyncwork or parallelwork

@draveness @alculquicondor What package name would your suggest to go with?

I prefer errchannel.New to channel.NewErrorChannel

being more specific, as @draveness suggested, reads well too.

draveness · 2019-08-09T15:11:34Z

pkg/scheduler/internal/heap/heap.go

@@ -25,6 +25,7 @@ import (
 	"fmt"

 	"k8s.io/client-go/tools/cache"
+	podinfo "k8s.io/kubernetes/pkg/scheduler/internal/podinfo"


does goimports add this automatically or it's a typo.

draveness · 2019-08-09T15:14:35Z

pkg/scheduler/framework/v1alpha1/framework.go

@@ -28,7 +28,7 @@ import (
 	"k8s.io/klog"
 	"k8s.io/kubernetes/pkg/scheduler/apis/config"
 	"k8s.io/kubernetes/pkg/scheduler/internal/cache"
-	schedutil "k8s.io/kubernetes/pkg/scheduler/util"
+	channels "k8s.io/kubernetes/pkg/scheduler/internal/channels"


I think goimports only add alias when the package name is v1, it's the first time I saw this and looks weird.

+1, that's also my understanding. Please remove this unnecessary alias.

draveness · 2019-08-09T15:15:34Z

pkg/scheduler/algorithm/priorities/interpod_affinity.go

 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/kubernetes/pkg/scheduler/algorithm"
 	"k8s.io/kubernetes/pkg/scheduler/algorithm/predicates"
 	priorityutil "k8s.io/kubernetes/pkg/scheduler/algorithm/priorities/util"
 	schedulerapi "k8s.io/kubernetes/pkg/scheduler/api"
+	schedutil "k8s.io/kubernetes/pkg/scheduler/internal/channels"


what do you think of keeping the original name of the package? rename it to schedutil does not sound like a good idea.

draveness · 2019-08-09T15:16:08Z

pkg/scheduler/algorithm/predicates/metadata.go

@@ -22,17 +22,19 @@ import (
 	"math"
 	"sync"

+	"k8s.io/kubernetes/pkg/scheduler/internal/channels"


This should be grouped with imports in L29-L37

alculquicondor

My general feeling is that this PR can be broken up into at least 2.

For example:
PR1: move Get*(v1.Pod) into a new podinfo package
PR2: move heap into its own package
PR3: move SortableList into its own package.

arguably, heap and sortable list can live in the same package named. Arguably, heap and sortable list could be in the same package named "datastructures" or "containers".

alculquicondor · 2019-08-09T15:43:52Z

pkg/kubelet/eviction/helpers.go

@@ -91,6 +90,16 @@ func init() {
 	signalToResource[evictionapi.SignalPIDAvailable] = resourcePids
 }

+func getPodPriority(pod *v1.Pod) int32 {


@hprateek43 I would suggest that you open a PR just for moving this function to pkg/api/pod/util.go prior to merging this. Another alternative could be pkg/apis/scheduling/helpers.go, depending on which one introduces less risk of circular dependencies.

alculquicondor · 2019-08-09T15:45:47Z

pkg/scheduler/algorithm/priorities/even_pods_spread.go

@@ -18,17 +18,16 @@ package priorities

 import (
 	"context"
+	"k8s.io/kubernetes/pkg/scheduler/internal/channels"


move to next import block

alculquicondor · 2019-08-09T15:56:26Z

pkg/scheduler/internal/channels/error_channel.go

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

-package util
+package channels


I would be more generic and name it something like asyncwork or parallelwork

alculquicondor · 2019-08-09T16:01:30Z

pkg/scheduler/core/extender_test.go

@@ -198,7 +198,7 @@ func (f *FakeExtender) selectVictimsOnNodeByExtender(
 	// and get cached node info by given node name.
 	nodeInfoCopy := f.cachedNodeNameToInfo[node.GetName()].Clone()

-	potentialVictims := util.SortableList{CompFunc: util.MoreImportantPod}
+	potentialVictims := podinfo.SortableList{CompFunc: podinfo.MoreImportantPod}


It feels like SortableList shouldn't belong to the podinfo package.

@alculquicondor Can you suggest a package to move it into?

what about internal/collections or more specific internal/sortablelist

hprateek43 · 2019-08-20T10:38:48Z

@alculquicondor @draveness Can you suggest the right PR titles to split this PR?

alculquicondor · 2019-08-20T12:35:17Z

Your call. If something is very off, I will request a change.

alejandrox1 · 2019-09-29T18:09:01Z

/remove-area e2e-test-framework
/remove-sig testing

neolit123 · 2019-10-09T15:37:23Z

/remove-area kubeadm

danwinship · 2019-12-12T12:41:11Z

/remove-sig network

k8s-ci-robot · 2019-12-12T12:41:20Z

@hprateek43: 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.

fejta-bot · 2020-03-11T12:47:31Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2020-04-10T13:30:39Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

alculquicondor · 2020-04-14T13:57:22Z

/close
This is quite stale now.
Perhaps the pod utils can be moved to a new package once #90039 lands

k8s-ci-robot · 2020-04-14T13:57:43Z

@alculquicondor: Closed this PR.

In response to this:

/close
This is quite stale now.
Perhaps the pod utils can be moved to a new package once #90039 lands

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-ci-robot requested review from andrewsykim, bowei and a team April 24, 2019 06:58

liggitt reviewed Apr 25, 2019

View reviewed changes

misterikkit reviewed Aug 6, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2019

draveness suggested changes Aug 9, 2019

View reviewed changes

alculquicondor reviewed Aug 9, 2019

View reviewed changes

hprateek43 mentioned this pull request Sep 1, 2019

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

Closed

hprateek43 mentioned this pull request Sep 27, 2019

Move heap into its own internal package #83233

Merged

k8s-ci-robot removed area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Sep 29, 2019

k8s-ci-robot removed the area/kubeadm label Oct 9, 2019

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed sig/network Categorizes an issue or PR as relevant to SIG Network. labels Dec 12, 2019

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 11, 2020

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 10, 2020

k8s-ci-robot closed this Apr 14, 2020

Refactored pkg/scheduler/utils to podinfo and heap packages. #76990

Refactored pkg/scheduler/utils to podinfo and heap packages. #76990

Conversation

hprateek43 commented Apr 24, 2019 • edited Loading

k8s-ci-robot commented Apr 24, 2019

hprateek43 commented Apr 24, 2019

hprateek43 commented Apr 25, 2019

lavalamp commented Apr 25, 2019

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

misterikkit left a comment

Choose a reason for hiding this comment

hprateek43 commented Aug 6, 2019

hprateek43 commented Aug 7, 2019

derekwaynecarr commented Aug 7, 2019

k8s-ci-robot commented Aug 7, 2019

hprateek43 commented Aug 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

draveness Aug 12, 2019 • edited Loading

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

alculquicondor 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hprateek43 commented Aug 20, 2019

alculquicondor commented Aug 20, 2019

alejandrox1 commented Sep 29, 2019

neolit123 commented Oct 9, 2019

danwinship commented Dec 12, 2019

k8s-ci-robot commented Dec 12, 2019

fejta-bot commented Mar 11, 2020

fejta-bot commented Apr 10, 2020

alculquicondor commented Apr 14, 2020

k8s-ci-robot commented Apr 14, 2020

hprateek43 commented Apr 24, 2019 •

edited

Loading

draveness Aug 12, 2019 •

edited

Loading