@ingvagabund: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

@ingvagabund: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

/hold
DO-NOT-MERGE yet

/test pull-kubernetes-integration

can you squash the commits apart from the ones coming from other PRs?

/retest

can you squash the commits apart from the ones coming from other PRs?

I created separate commits to make easier to understand the changes. Though, if it's not needed, I can squash.

I am not sure I like this approach, creating an interface for logic that is static and injecting it around is an overkill.

Why can't we move those helper functions as is to component helpers pkg?

Why can't we move those helper functions as is to component helpers pkg?

I was not able to address @liggitt 's comment in #95553 (comment):

If we expose these externally, they will start getting used by out of tree components using versions
of the component-helpers library which will lag the API server. Those components will think they are
using identical logic to the API server, but will be missing additions made to the helper functions.
That will cause those components to consider some data invalid which the API server considers valid.
That seems like a problem we don't have today that this move would introduce.

/retest

So an external component that imports the scheduler code would need to define its own implementation of ResourceNameQualifier, right? If so, that doesn't sound like a good tradeoff either.

But in any case, even if we want to be able to inject this behavior just so we can cut the dependency, I think plumbing it this way throughout the code base is too much for a functionality that is static and not actually supposed to be swappable. I suggest we define a global variable under the scheduler's component-helpers pkg that is set/registered before scheduler startup, just like metrics registration.

So an external component that imports the scheduler code would need to define its own implementation of ResourceNameQualifier, right? If so, that doesn't sound like a good tradeoff either.

Any external component can still import the k/k resource name helpers. Though yeah, it's still an ugly hack.

But in any case, even if we want to be able to inject this behavior just so we can cut the dependency, I think plumbing it this way throughout the code base is too much for a functionality that is static and not actually supposed to be swappable. I suggest we define a global variable under the scheduler's component-helpers pkg that is set/registered before scheduler startup, just like metrics registration.

With a global variable one has no guarantee the variable will always get set. If the metrics do not get registered, no metrics get collected. If the global variable for the resource name helpers does not get registered, the scheduler will either panic or will not correctly classify scalar resources. Meaning the validation might produce different conclusions. Wiring the code this way is not so much about allowing to swap the implementation as about making the compiler complain and make anyone importing the scheduler framework bits to see what's missing and initialize the resource name helper.

Any external component can still import the k/k resource name helpers. Though yeah, it's still an ugly hack.

But that defeats the purpose of this refactoring.

With a global variable one has no guarantee the variable will always get set. If the metrics do not get registered, no metrics get collected. If the global variable for the resource name helpers does not get registered, the scheduler will either panic or will not correctly classify scalar resources. Meaning the validation might produce different conclusions.

Failing silently is bad, crashing is probably better. But I don't understand how we "will not correctly classify scalar resources." if the global variable is not set?

Wiring the code this way is not so much about allowing to swap the implementation as about making the compiler complain and make anyone importing the scheduler framework bits to see what's missing and initialize the resource name helper.

I get that it is not about swapping implementation, which in my opinion is the problematic part: abstracting something that doesn't really need to be abstracted.

I suggested setting up a global variable, I admit it may not necessarily be a better alternative, but the current proposal caused significant changes, the interface is being passed in many places with negative net benefit for the scheduler code base. It would be a lot less of an issue if it is limited to few places (like the framework and have it accessible via the framework handle).

Is the goal to cut dependencies for the whole scheduler pkg or mainly the framework plugins?

Failing silently is bad, crashing is probably better. But I don't understand how we "will not correctly classify scalar resources." if the global variable is not set?

That depends on the default behavior. If the global variable is nil, scalar resources might just get ignored. Though, I still prefer crashing.

Is the goal to cut dependencies for the whole scheduler pkg or mainly the framework plugins?

I'd like to do it for the whole scheduler pkg. So third party repositories can import more than just the plugins. One might take the entire pkg/scheduler and just replace the queue implementation.

I suggested setting up a global variable

I don't have a better solution right now. I will take this path for the moment and update the PR accordingly.

@ingvagabund: PR needs rebase.

The Kubernetes project currently lacks enough 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:

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

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

/lifecycle stale

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:

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

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

/lifecycle rotten

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-triage-robot: Closed this PR.