Scheduler: LastOrdinal based on replicas instead of FreeCap #8388

pierDipi · 2024-12-16T14:40:43Z

When scaling down and compacting, basing the last ordinal on the free capacity structure leads to have a lastOrdinal off by one since FreeCap might contain the free capacity for unschedulable pods.

We will have to continue including unschduelable pods in FreeCap because it might happen that a pod becomes unscheduleble for external reasons like node gets shutdown for pods with lower ordinals and the pod need to be rescheduled and during that time period we want to consider those when compacting; once all vpods that were on that pod that is gone get rescheduled, FreeCap will only include scheduleable pods.

Fixes #

Proposed Changes

Scheduler: LastOrdinal based on replicas instead of FreeCap

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Release Note

Docs

pierDipi · 2024-12-16T15:07:41Z

/cherry-pick release-1.15

pierDipi · 2024-12-16T15:07:44Z

/cherry-pick release-1.16

knative-prow-robot · 2024-12-16T15:07:44Z

@pierDipi: once the present PR merges, I will cherry-pick it on top of release-1.15 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.15

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-sigs/prow repository.

knative-prow-robot · 2024-12-16T15:07:47Z

@pierDipi: once the present PR merges, I will cherry-pick it on top of release-1.16 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.16

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-sigs/prow repository.

pierDipi · 2024-12-16T15:28:23Z

/cherry-pick release-1.15

knative-prow-robot · 2024-12-16T15:28:26Z

@pierDipi: once the present PR merges, I will cherry-pick it on top of release-1.15 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.15

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-sigs/prow repository.

matzew · 2024-12-17T06:42:04Z

pkg/scheduler/state/state.go

@@ -219,23 +217,19 @@ func pendingFromVPod(vpod scheduler.VPod) int32 {
 	return int32(math.Max(float64(0), float64(expected-scheduled)))
 }

-func (s *stateBuilder) updateFreeCapacity(logger *zap.SugaredLogger, free []int32, last int32, podName string, vreplicas int32) ([]int32, int32) {
+func (s *stateBuilder) updateFreeCapacity(logger *zap.SugaredLogger, free []int32, podName string, vreplicas int32) []int32 {


should we name the second arg freeCap ?

matzew · 2024-12-17T06:43:59Z

pkg/scheduler/statefulset/autoscaler.go

@@ -280,12 +282,14 @@ func (a *autoscaler) compact(s *st.State) error {
 		return err
 	}

+	lastOrdinal := s.Replicas - 1


why the replica-1 ?

this changed slightly now

When scaling down and compacting, basing the last ordinal on the free capacity structure leads to have a lastOrdinal off by one since `FreeCap` might contain the free capacity for unschedulable pods. We will have to continue including unschduelable pods in FreeCap because it might happen that a pod becomes unscheduleble for external reasons like node gets shutdown for pods with lower ordinals and the pod need to be rescheduled and during that time period we want to consider those when compacting; once all vpods that were on that pod that is gone get rescheduled, FreeCap will only include scheduleable pods. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

codecov · 2024-12-17T11:33:54Z

Codecov Report

Attention: Patch coverage is 87.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 64.19%. Comparing base (4087c3a) to head (4fb34df).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/scheduler/state/state.go	90.00%	1 Missing and 1 partial ⚠️
pkg/scheduler/statefulset/autoscaler.go	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8388      +/-   ##
==========================================
- Coverage   64.22%   64.19%   -0.03%     
==========================================
  Files         388      388              
  Lines       23324    23310      -14     
==========================================
- Hits        14979    14965      -14     
  Misses       7550     7550              
  Partials      795      795

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi · 2024-12-17T14:41:04Z

/test upgrade-tests

matzew

/lgtm
/approve

knative-prow · 2024-12-19T06:09:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, pierDipi

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~~ [matzew,pierDipi]

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

knative-prow-robot · 2024-12-19T07:12:51Z

@pierDipi: new pull request created: #8393

In response to this:

/cherry-pick release-1.15

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-sigs/prow repository.

knative-prow-robot · 2024-12-19T07:13:27Z

@pierDipi: new pull request created: #8394

In response to this:

/cherry-pick release-1.16

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-sigs/prow repository.

knative-prow bot requested review from Cali0707 and Leo6Leo December 16, 2024 14:40

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 16, 2024

pierDipi force-pushed the scheduler-last-ordinal-wrong branch from 1aeabdb to 3f807e9 Compare December 16, 2024 14:43

matzew reviewed Dec 17, 2024

View reviewed changes

pierDipi force-pushed the scheduler-last-ordinal-wrong branch from 3f807e9 to 7bad45b Compare December 17, 2024 11:26

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 17, 2024

pierDipi force-pushed the scheduler-last-ordinal-wrong branch from 7bad45b to 4fb34df Compare December 17, 2024 11:27

pierDipi added a commit to pierDipi/eventing-kafka-broker that referenced this pull request Dec 17, 2024

Test knative/eventing#8388

b8b2af6

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi mentioned this pull request Dec 17, 2024

[WIP][DNM] Test 8388 knative-extensions/eventing-kafka-broker#4189

Open

pierDipi added a commit to pierDipi/eventing-kafka-broker that referenced this pull request Dec 17, 2024

Test knative/eventing#8388

a245bf4

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

matzew approved these changes Dec 19, 2024

View reviewed changes

knative-prow bot assigned matzew Dec 19, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2024

knative-prow bot merged commit 4dbc2ba into knative:main Dec 19, 2024
34 of 36 checks passed

knative-prow-robot mentioned this pull request Dec 19, 2024

[release-1.15] Scheduler: LastOrdinal based on replicas instead of FreeCap #8393

Merged

knative-prow-robot mentioned this pull request Dec 19, 2024

[release-1.16] Scheduler: LastOrdinal based on replicas instead of FreeCap #8394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheduler: LastOrdinal based on replicas instead of FreeCap #8388

Scheduler: LastOrdinal based on replicas instead of FreeCap #8388

pierDipi commented Dec 16, 2024

pierDipi commented Dec 16, 2024

pierDipi commented Dec 16, 2024

knative-prow-robot commented Dec 16, 2024

knative-prow-robot commented Dec 16, 2024

pierDipi commented Dec 16, 2024

knative-prow-robot commented Dec 16, 2024

matzew Dec 17, 2024 •

edited

Loading

matzew Dec 17, 2024

pierDipi Dec 17, 2024

codecov bot commented Dec 17, 2024 •

edited

Loading

pierDipi commented Dec 17, 2024

matzew left a comment

knative-prow bot commented Dec 19, 2024

knative-prow-robot commented Dec 19, 2024

knative-prow-robot commented Dec 19, 2024

Scheduler: LastOrdinal based on replicas instead of FreeCap #8388

Scheduler: LastOrdinal based on replicas instead of FreeCap #8388

Conversation

pierDipi commented Dec 16, 2024

Proposed Changes

Pre-review Checklist

pierDipi commented Dec 16, 2024

pierDipi commented Dec 16, 2024

knative-prow-robot commented Dec 16, 2024

knative-prow-robot commented Dec 16, 2024

pierDipi commented Dec 16, 2024

knative-prow-robot commented Dec 16, 2024

matzew Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

matzew Dec 17, 2024

Choose a reason for hiding this comment

pierDipi Dec 17, 2024

Choose a reason for hiding this comment

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

pierDipi commented Dec 17, 2024

matzew left a comment

Choose a reason for hiding this comment

knative-prow bot commented Dec 19, 2024

knative-prow-robot commented Dec 19, 2024

knative-prow-robot commented Dec 19, 2024

matzew Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading