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.

/sig scheduling

Before #119779, there was this line:
https://github.com/sanposhiho/kubernetes/blob/5ab23179473b621e911511b03693234b60a38033/pkg/scheduler/schedule_one.go#L489

which also had a map access. So we can quite confidently say that the culprit is just the map insert of

@sanposhiho, @AxeZhan I have two alternatives:

1. The obvious one is to do a preallocation for the size of the map. I suspect this might be enough, but still somewhat wasteful.
2. Change the preemption logic to assume that if a Node is not in the status map, it's UnschedulableAndUnresolvable.

   kubernetes/pkg/scheduler/framework/preemption/preemption.go

   Line 422 in 44bd04c

   if m[name].Code() == framework.UnschedulableAndUnresolvable {

Option 2 also solves #124705. However, external PostFilter plugins need to be aware of the change. But I'm not aware of any in the wild that implements a behavior similar to preemption. We can leave an ACTION REQUIRED. But was this path working in 1.28 and older?

Regardless of the decision, I think we should accompany the fix with an integration test for preemption by daemonset pods.

Can we try (1) first, ask Google for a scalability test again, and then consider (2) if (1) doesn't improve the throughput enough?

(2) could break existing PostFilter plugins. Given PostFilter is not only for preemption, in the world, there may be custom PostFilter plugins which have to rely on the status map with UnschedulableAndUnresolvable nodes. So, it'd be best if we could avoid this breaking.

Since (1) is a really small fix, I'm also thinking if we can run tests after #124714 , and then consider (2) base on the test results.

Regardless of the decision, I think we should accompany the fix with an integration test for preemption by daemonset pods.

Are these "integration test" for performance or correctness?

I was asking for a correctness test

I wrote a benchmark test called Benchmark_findNodesThatFitPod

The performance without case(1) is:

4927125125 ~ 5118241875 ns/op

The performance of case (1) is:

3865283834 ~ 3972690125 ns/op

(1) may improve throughput

I guess we can have a different tracking issue for an integration benchmark.

This issue itself is fixed.

/close

@alculquicondor: Closing this issue.

/reopen
/assign

Still observing poor performance in large clusters post #124714. I'm investigating further improvements.

@gabesaba: You can't reopen an issue/PR unless you authored it or you are a collaborator.

/reopen

@alculquicondor: Reopened this issue.

@gabesaba @chengjoey

As I asked here, can we add a test case to scheduler_perf to reproduce this degradation?

Also, more generally, would it be possible to copy Google’s internal scalability tests to k/k somehow? (already talked to Aldo before) There’ve been multiple times that Google found a scalability issue with their internal tests.
Integrating them into scheduler_perf would be the best; we may need to improve scheduler_perf if its capability isn't enough to incorporate, though.

@gabesaba @chengjoey

As I asked here, can we add a test case to scheduler_perf to reproduce this degradation?

Also, more generally, would it be possible to copy Google’s internal scalability tests to k/k somehow? (already talked to Aldo before) There’ve been multiple times that Google found a scalability issue with their internal tests. Integrating them into scheduler_perf would be the best; we may need to improve scheduler_perf if its capability isn't enough to incorporate, though.

Currently, scheduler_perf does not seem to be able to reproduce the current scenario. Should we first improve scheduler_perf to make it similar to daemonset, such as labeling each node with a specific label or specifying a dedicated node for the pod to achieve the k/k effect?

As I asked #124728 (comment), can we add a test case to scheduler_perf to reproduce this degradation?

We plan to do it as a follow up. For now, we have the urgent need to get this back to baseline.

Yep, I didn't mean to block the patch PR for scheduler_perf improvement. Doing this as a follow up is totally fine.

Currently, scheduler_perf does not seem to be able to reproduce the current scenario. Should we first improve scheduler_perf to make it similar to daemonset, such as labeling each node with a specific label or specifying a dedicated node for the pod to achieve the k/k effect?

We can improve scheduler_perf engine if needed. But, I'm wondering if we could reproduce the degradation just with a simple test case in which creating 5k Pods that have NodeAffinity matching with only one Node among many Nodes (<- @alculquicondor how many Nodes did you have in the scalable test?). I believe the current scheduler_perf is expressive enough to define such test case.

We had 15k nodes with 6 Daemonsets, so 90k Pods.

I could reproduce the degradation with a new test case (15k nodes / 30k pods) in scheduler_perf #125293 (WIP) and confirmed #125197 solves the degradation.

Master

    {
      "data": {
        "Average": 120.80130209885886,
        "Perc50": 128.88163639395216,
        "Perc90": 155.10790546767575,
        "Perc95": 161.00505378763333,
        "Perc99": 169.00350513269643
      },
      "unit": "pods/s",
      "labels": {
        "Metric": "SchedulingThroughput",
        "Name": "SchedulingNodeAffinity/15000Nodes/namespace-2",
        "extension_point": "not applicable",
        "plugin": "not applicable",
        "result": "not applicable"
      }
    },

After #125197

    {
      "data": {
        "Average": 552.6016212924262,
        "Perc50": 474.34585036045274,
        "Perc90": 877.1160813824991,
        "Perc95": 1121.327881099065,
        "Perc99": 1457.5364518479603
      },
      "unit": "pods/s",
      "labels": {
        "Metric": "SchedulingThroughput",
        "Name": "SchedulingNodeAffinity/15000Nodes/namespace-2",
        "extension_point": "not applicable",
        "plugin": "not applicable",
        "result": "not applicable"
      }
    },

/reopen

I think we still need to implement option2 and some follow-upsof #125197.

@gabesaba Will you continue working on this?

@AxeZhan: Reopened this issue.

#125197 is option 2

But yes, there are some follow ups.

Can we close it? Any remaining task here?

Followup is tracked in #125345, so I think it's fine to close it.
/close

@AxeZhan: Closing this issue.