Revert "Surface more information about plugin scores in scheduler"

[Huang-Wei]

What type of PR is this?

/kind regression
/sig scheduling

What this PR does / why we need it:

This PR reverts #99411.

Tried locally that change v(4) to v(5) works, but that doesn't fix the root cause - users who customized logging level to 5 or greater can still encounter this issue. So I proposed to revert #99411 and dig out why it regresses.

Which issue(s) this PR fixes:

Fixes #99913

NONE

[k8s-ci-robot]

@Huang-Wei: 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.

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.

[Huang-Wei]

/priority critical-urgent

/assign @damemi
/cc @ahg-g @alculquicondor

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei

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/scheduler/OWNERS [Huang-Wei]

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

[ahg-g]

can you explain the regression?

[ahg-g]

but that doesn't fix the root cause - users who customized logging level to 5 or greater can still encounter this issue.

Which issue?

[ahg-g]

oh, you are referring to #99913

Well, the benchmark should be running with the default v=2 logging level. I am guessing that increasing the logging level to 3 or 4 is done sometimes temporarily.

That being said, I don't quite like the way #99411 was implemented, it seems a bit ad hoc.

[Huang-Wei]

oh, you are referring to #99913

Yes, I referred to #99913. Sorry, there was a typo.

Well, the benchmark should be running with the default v=2 logging level. I am guessing that increasing the logging level to 3 or 4 is done sometimes temporarily.

Here is the magic:

kubernetes/test/integration/framework/master_utils.go

Lines 131 to 136 in 90851a0

	// Ensure we log at least level 4
	v := flag.Lookup("v").Value
	level, _ := strconv.Atoi(v.String())
	if level < 4 {
	v.Set("4")
	}

But using which logging level is not the key point - #99411 is beneficial only when the desired logging level is enabled, but when that level is enabled, performance downgrades.

[damemi]

The performance drop is unexpected, though I would like to continue working on this to find a more efficient way to surface information like this.

I think we made some decisions in the original PR that have affected this, such as duplicating the calculation loop in order to separate out the logging information into its own function.

Due to that, we are essentially doing much of this work twice (which would explain the ~50% drop in throughput). I think that some reasonable level of efficiency drop is acceptable, because like @ahg-g said this is not a default logging level. This much is not acceptable.

Thanks for catching this and opening the reversion @Huang-Wei. I'll work to get a new PR with performance testing. Any other suggestions for how to improve this are welcome.
/lgtm

[Huang-Wei]

Thanks for catching this and opening the reversion @Huang-Wei. I'll work to get a new PR with performance testing. Any other suggestions for how to improve this are welcome.

Thanks @damemi ! As code freeze is around the corner, reverting may be the most viable option to unblock other PRs.

And it's easy to reproduce the perf drop, reverting can buy us more time to find out the root cause.

[Huang-Wei]

/retest

[alculquicondor]

I don't think the performance drop is unexpected. But I don't think it's related to the calculation. It's probably about IO. We have 5000 nodes in those tests. That's a lot of writes.

I just didn't think that tests would run at level 4 :(