Introduce internal channels for scheduler

[hprateek43]

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR introduces internal channels for scheduler with the general idea of phasing out the scheduler util package. The error channels for scheduler must be internal and should not be accessible to other components.
Which issue(s) this PR fixes:

Fixes #84338

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

[draveness]

/remove-kind feature
/kind cleanup

/lgtm

[draveness]

/lgtm cancel

[hprateek43]

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

[ahg-g]

can you please rebase and resolve the conflicts?

[hprateek43]

can you please rebase and resolve the conflicts?

I just rebased. Had to upgrade go to 1.13 to do so. But now it seems some of the scripts in the hack folder are not working. Could not get hack/update-bazel.sh to run. Also make update changes a lot of go.mod files which is strange.

[ahg-g]

/retest
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, deads2k, hprateek43

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

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/controller/OWNERS [deads2k]
• pkg/scheduler/OWNERS [ahg-g]

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

[alculquicondor]

I just rebased. Had to upgrade go to 1.13 to do so. But now it seems some of the scripts in the hack folder are not working. Could not get hack/update-bazel.sh to run. Also make update changes a lot of go.mod files which is strange.

Any specific errors? This can't merge otherwise.

[hprateek43]

I just rebased. Had to upgrade go to 1.13 to do so. But now it seems some of the scripts in the hack folder are not working. Could not get hack/update-bazel.sh to run. Also make update changes a lot of go.mod files which is strange.

Any specific errors? This can't merge otherwise.

I had a discussion with @liggitt around this. I just setup my source code inside GOPATH, but am still struggling to get the bazel update running. Will update this in a day or two. Working on that.

[hprateek43]

@alculquicondor Seems sorted for now. Should be clear for merge?

[liggitt]

we don't want controllers depending on internal scheduler packages, right?

[hprateek43]

Eventually we dont. I plan to remove this coupling in the coming weeks, the code can be refactored to remove this dependency. This is added for now for the scheduler cleanup. Can be marked as a TODO.

[liggitt]

if this is intended to be a cleanup PR, it would be preferable not to introduce new debt in it. can that dependency be dropped before we mark this internal?

[alculquicondor]

@hprateek43, how do you plan to remove the dependency?

Other than that, I don't understand why import-restrictions apply to indirect imports. I feel like it shouldn't.

[hprateek43]

the idea is to first eliminate the utils package and then refactor predicates to remove dependencies on internal packages. But @alculquicondor is also right in his place that transient dependencies should not be considered.

[misterikkit]

Just as a reminder, .import-restrictions is a home-grown k8s utility that requires us to whitelist all transitive dependencies. The go compiler prevents pkg/controller from directly importing anything in pkg/scheduler/internal/...

[hprateek43]

@liggitt This point could be considered. Although now I am planning to remove the error channel altogether

[liggitt]

@liggitt This point could be considered

that's a good point, and would remove my objection

Although now I am planning to remove the error channel altogether

that sounds even better :)

[hprateek43]

/test pull-kubernetes-node-e2e-containerd

[hprateek43]

/test pull-kubernetes-node-e2e-containerd

[hprateek43]

@liggitt can you give a lgtm ?

[alculquicondor]

/hold
for @liggitt's comments

[hprateek43]

@misterikkit Can you share your input here? We had a discussion over transitive dependencies over https://github.com/kubernetes/kubernetes/pull/76990/files where you stated that this is more of a quirk of the tool? Do you think this change seems logically correct or we should probably refactor it in a better way ?

[hprateek43]

Closing this PR as consensus is not formed regarding internal channels

[misterikkit]

@misterikkit Can you share your input here?

Sorry I just noticed this. My opinion is that the ErrorChannel utility is providing close to zero value, and we could remove some of this dependency mess by just deleting it.

At its core, it is just a wrapper for non-blocking reads and writes on a channel. Adding some select statements to the codebase is not horrible, and if we don't like doing that, having much smaller helper functions just to wrap a select is fine to copy-paste into the packages that need it.

The part where we pass a cancel function into ErrorChannel just so it can call that for us is not doing anything. The caller needs to decide whether to pass that in, so the caller can decide to call cancel instead.