Implemented batch processing for check capacity provisioning class #7283

Duke0404 · 2024-09-13T21:49:39Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implements batch processing such that user can configure CA to process multiple CheckCapacity ProvisioningRequests in a single autoscaling iteration.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Allows CheckCapacity ProvisioningRequests to be processed in batch mode with configurable max batch size and batch timebox enabled by feature flags.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

cc: @yaroslava-serdiuk @aleksandra-malinowska @kawych

k8s-ci-robot · 2024-09-13T21:50:16Z

Hi @Duke0404. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

yaroslava-serdiuk · 2024-09-16T11:50:33Z

/ok-to-test

yaroslava-serdiuk · 2024-09-18T12:13:43Z

please, fix tests

Duke0404 · 2024-09-18T14:14:20Z

please, fix tests

I think the tests were failing due to an issue with Github actions, they are passing now.

aleksandra-malinowska · 2024-09-19T11:42:43Z

FYI @MaciekPytel @mwielgus the change we've been discussing a couple weeks ago.

MaciekPytel · 2024-09-19T12:20:32Z

I don't like the way that checkcapacity is implemented and I'm not super happy about doubling down on it. Checking capacity is not really similar to scale-up, but it is conceptually pretty much the same as fitting existing pods on upcoming nodes in FilterOutSchedulable. Both cases involve just binpacking on existing nodes and don't require using Estimator, Expander, etc to make scale-up decisions or all the logic related to actuating such decisions.

PodListProcessor interface has been pretty much designed exactly for this use-case, while scale-up orchestrator is intended to do more complex operations that you don't actually need here. The architectural / maintenance downside is inconsistency with the rest of the codebase and related maintenance problems (anyone debugging CA must be aware that provreq works differently from other, similar extensions to CA - our steep learning curve is likely a sum total of small gotchas like that).
In more practical terms - having it in a scale-up orchestrator means you need to use up an entire CA loop to check provreq capacity. If you moved it to a PodListProcessor you could checkcapacity and continue the loop to still do any necessary scale-up. In the type of batch cluster that I expect to be using provreq, loosing CA loops like that is very likely to translate to significant scale-up latency / throughput problem.

I'm not going to block this PR, but I'd really like to look into aligning provisioning request implementation with the expected CA architecture - and migrating the logic to PLP would be an obvious first step here.

aleksandra-malinowska · 2024-09-19T13:30:35Z

The architectural / maintenance downside is inconsistency with the rest of the codebase and related maintenance problems (anyone debugging CA must be aware that provreq works differently from other, similar extensions to CA - our steep learning curve is likely a sum total of small gotchas like that).

I agree with the general point about CA architecture. I would go even further and say we should probably explicitly group "capacity booking" (CRs, PRs, and possible future overprovisioning API) to make the implementation more consistent - while it'll be slightly different for each case (ex. all-or-nothing for PRs) they're logically very similar and if there's ever a substantial change to how the capacity needs to be booked for CA to recognize it, it'll have to be re-implemented everywhere.

In more practical terms - having it in a scale-up orchestrator means you need to use up an entire CA loop to check provreq capacity. If you moved it to a PodListProcessor you could checkcapacity and continue the loop to still do any necessary scale-up. In the type of batch cluster that I expect to be using provreq, loosing CA loops like that is very likely to translate to significant scale-up latency / throughput problem.

I think this argument essentially boils down to "this could be optimized further". With improvements to frequent loops logic (meaning we'll skip the scan interval), this change will still significantly improve performance compared to what there's now, even in large clusters. Yes, it's possible to improve it even more by skipping refreshing cluster state, but it isn't necessarily an argument against doing partial optimization now.

I'm not going to block this PR, but I'd really like to look into aligning provisioning request implementation with the expected CA architecture - and migrating the logic to PLP would be an obvious first step here.

I expect batch processing to remain an experimental feature for 1.31 (meaning it's turned off by default). I agree we may want to solve this before it becomes enabled by default.

yaroslava-serdiuk · 2024-09-19T13:12:05Z

cluster-autoscaler/main.go

+	proactiveScaleupEnabled      = flag.Bool("enable-proactive-scaleup", false, "Whether to enable/disable proactive scale-ups, defaults to false")
+	podInjectionLimit            = flag.Int("pod-injection-limit", 5000, "Limits total number of pods while injecting fake pods. If unschedulable pods already exceeds the limit, pod injection is disabled but pods are not truncated.")
+	checkCapacityBatchProcessing = flag.Bool("check-capacity-batch-processing", false, "Whether to enable batch processing for check capacity requests.")
+	maxBatchSize                 = flag.Int("max-batch-size", 10, "Maximum number of provisioning requests to process in a single batch.")


I think initial value for maxBatchSize should be 1, which means no batching, and with that we don't need to have checkCapacityBatchProcessing flag.
If we remove checkCapacityBatchProcessing flag, then we should rename maxBatchSize and batchTimebox to have checkCapacityProvisioningRequest in the name.

TBH I'd rather have a one top-level flag controlling the feature. Also it allows us to have a reasonable defaults that have been tested for max batch size and processing time.

I agree with renaming the remaining flags to be clearly associated with check-capacity.

I agree that would be much better user experience to just enable the feature with predefined parameters, but I'm not sure we have enough data to recommend max batch size.
However I think we can drop max processing time flag.

I think having the maxBatchSize to a value other than 1 makes sense if we keep a top level flag for clarity.

If we keep checkCapacityBatchProcessing flag, i'd leave maxBatchSize default to 10. Or we can remove checkCapacityBatchProcessing and have maxBatchSize default to 1.

cluster-autoscaler/main.go

cluster-autoscaler/provisioningrequest/checkcapacity/provisioningclass.go

aleksandra-malinowska · 2024-09-23T08:23:55Z

@Duke0404 FYI #7182 just merged. I don't think it'll affect batch processing, but it might affect unit tests in this PR.

yaroslava-serdiuk · 2024-10-15T11:55:17Z

cluster-autoscaler/provisioningrequest/checkcapacity/provisioningclass.go

+	context                                      *context.AutoscalingContext
+	client                                       *provreqclient.ProvisioningRequestClient
+	schedulingSimulator                          *scheduling.HintingSimulator
+	checkCapacityBatchProcessing                 bool


Having a checkCapacityBatchProcessing bool flag makes sence to have some default for max batch size other than 1, but I would remove bool value here and specify maxBatchSize equal to 1 in case checkCapacityBatchProcessing flag is false.

yaroslava-serdiuk · 2024-10-15T12:08:40Z

cluster-autoscaler/main.go

+	proactiveScaleupEnabled      = flag.Bool("enable-proactive-scaleup", false, "Whether to enable/disable proactive scale-ups, defaults to false")
+	podInjectionLimit            = flag.Int("pod-injection-limit", 5000, "Limits total number of pods while injecting fake pods. If unschedulable pods already exceeds the limit, pod injection is disabled but pods are not truncated.")
+	checkCapacityBatchProcessing = flag.Bool("check-capacity-batch-processing", false, "Whether to enable batch processing for check capacity requests.")
+	maxBatchSize                 = flag.Int("max-batch-size", 10, "Maximum number of provisioning requests to process in a single batch.")


If we keep checkCapacityBatchProcessing flag, i'd leave maxBatchSize default to 10. Or we can remove checkCapacityBatchProcessing and have maxBatchSize default to 1.

yaroslava-serdiuk · 2024-10-15T12:27:09Z

cluster-autoscaler/provisioningrequest/checkcapacity/provisioningclass.go

+		if scaleUpIsSuccessful {
+			combinedStatus.Add(&status.ScaleUpStatus{Result: status.ScaleUpSuccessful})
+		} else {
+			combinedStatus.Add(&status.ScaleUpStatus{Result: status.ScaleUpNoOptionsAvailable})
+		}


I suppose else statement is missed, otherwise in case err != nil we append combinedStatus twice.

Could you also add a test case that will catch this?

Even without the else, it would not cause any problematic behavior, thus I do not think we can test for this either. Since, in combinedStatusSet we do not store the number of statuses that we recorded. And ScaleUpNoOptionsAvailable is overwritten by ScaleUpSuccessful in the result.

yaroslava-serdiuk · 2024-10-15T12:59:08Z

cluster-autoscaler/provisioningrequest/checkcapacity/provisioningclass.go

 	if len(st) < len(unschedulablePods) || err != nil {
 		conditions.AddOrUpdateCondition(provReq, v1.Provisioned, metav1.ConditionFalse, conditions.CapacityIsNotFoundReason, "Capacity is not found, CA will try to find it later.", metav1.Now())
 		capacityAvailable = false
 	} else {
+		commitErr := o.context.ClusterSnapshot.Commit()
+		if commitErr != nil {
+			commit = true


Shouldn't we revert snapshot in case of error during Commit? By the way, we can use a helper function WithForkedSnapshot for this case.

Fixed and updated.

cluster-autoscaler/provisioningrequest/checkcapacity/provisioningclass.go

yaroslava-serdiuk · 2024-10-15T19:00:31Z

cluster-autoscaler/provisioningrequest/checkcapacity/provisioningclass.go

+}
+
+// Export converts the combinedStatusSet into a ScaleUpStatus.
+func (c *combinedStatusSet) Export() (*status.ScaleUpStatus, errors.AutoscalerError) {


Let's add few tests for combinedStatusSet.

yaroslava-serdiuk · 2024-10-16T08:29:20Z

@Duke0404 could you address comments in separate commit, please? It will make review process easier and faster.
We can squash commits in the end or use /label tide/merge-method-squash

cluster-autoscaler/main.go

yaroslava-serdiuk · 2024-10-16T10:41:23Z

cluster-autoscaler/provisioningrequest/checkcapacity/provisioningclass.go

+		if scaleUpIsSuccessful {
+			combinedStatus.Add(&status.ScaleUpStatus{Result: status.ScaleUpSuccessful})
+		} else {
+			combinedStatus.Add(&status.ScaleUpStatus{Result: status.ScaleUpNoOptionsAvailable})
+		}


Could you also add a test case that will catch this?

yaroslava-serdiuk · 2024-10-16T11:55:42Z

cluster-autoscaler/provisioningrequest/orchestrator/orchestrator_test.go

+			scaleUpResult:      status.ScaleUpSuccessful,
+			batchProcessing:    true,
+			maxBatchSize:       5,
+			batchTimebox:       1 * time.Nanosecond,


Let's make it 0, to avoid flakiness in case fast execution (which is very unlikely, but is possible in theory).

yaroslava-serdiuk · 2024-10-16T12:25:52Z

cluster-autoscaler/provisioningrequest/orchestrator/orchestrator_test.go

 	}
 	for _, tc := range testCases {
 		tc := tc
-		allNodes := allNodes


you need it, because we modify cluster snapshot in each test

My bad, this should be removed. We need to have a deep copy of nodes in each case.

yaroslava-serdiuk · 2024-10-28T12:55:01Z

/lgtm

mwielgus

/lgtm
/approve

k8s-ci-robot · 2024-10-28T14:33:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Duke0404, mwielgus

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:

~~cluster-autoscaler/OWNERS~~ [mwielgus]

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

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 13, 2024

k8s-ci-robot requested a review from aleksandra-malinowska September 13, 2024 21:50

k8s-ci-robot added the area/cluster-autoscaler label Sep 13, 2024

k8s-ci-robot requested a review from vadasambar September 13, 2024 21:50

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 13, 2024

Duke0404 force-pushed the batchadditional branch 4 times, most recently from 5dddce4 to bca8b93 Compare September 16, 2024 07:35

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 16, 2024

Duke0404 force-pushed the batchadditional branch from bca8b93 to bbc758d Compare September 16, 2024 07:53

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 16, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 16, 2024

Duke0404 force-pushed the batchadditional branch from bbc758d to 11703e9 Compare September 16, 2024 21:13

Duke0404 force-pushed the batchadditional branch from 11703e9 to 28f4c57 Compare September 18, 2024 13:54

yaroslava-serdiuk reviewed Sep 20, 2024

View reviewed changes

Duke0404 force-pushed the batchadditional branch from 28f4c57 to a6f4fc2 Compare September 20, 2024 12:57

Duke0404 force-pushed the batchadditional branch from 1ff5348 to 6976fe9 Compare October 15, 2024 01:16

yaroslava-serdiuk suggested changes Oct 15, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2024

Duke0404 force-pushed the batchadditional branch from 6976fe9 to 93d5c1e Compare October 16, 2024 01:22

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2024

Duke0404 force-pushed the batchadditional branch from 93d5c1e to 68d3224 Compare October 16, 2024 03:05

yaroslava-serdiuk reviewed Oct 16, 2024

View reviewed changes

Duke0404 force-pushed the batchadditional branch 2 times, most recently from 2a6d391 to 4b19bbc Compare October 17, 2024 10:08

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2024

Duke0404 force-pushed the batchadditional branch from 0f6aa3a to 2e1135e Compare October 23, 2024 09:41

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2024

yaroslava-serdiuk mentioned this pull request Oct 24, 2024

Remove redundant copy in UpdateProvisioningRequest() method #7430

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2024

Duke0404 added 2 commits October 24, 2024 21:14

Implement batch processing for check capacity class with combined status

63d0275

Implement unit tests for batch processing of check capacity class

d73bdb1

Duke0404 force-pushed the batchadditional branch from 2e1135e to d73bdb1 Compare October 24, 2024 21:16

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2024

Duke0404 mentioned this pull request Oct 27, 2024

Add generic CombinedStatus type #7439

Open

k8s-ci-robot assigned yaroslava-serdiuk Oct 28, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 28, 2024

mwielgus approved these changes Oct 28, 2024

View reviewed changes

k8s-ci-robot assigned mwielgus Oct 28, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 28, 2024

k8s-ci-robot merged commit e6150cc into kubernetes:master Oct 28, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented batch processing for check capacity provisioning class #7283

Implemented batch processing for check capacity provisioning class #7283

Duke0404 commented Sep 13, 2024

k8s-ci-robot commented Sep 13, 2024

yaroslava-serdiuk commented Sep 16, 2024

yaroslava-serdiuk commented Sep 18, 2024

Duke0404 commented Sep 18, 2024

aleksandra-malinowska commented Sep 19, 2024

MaciekPytel commented Sep 19, 2024

aleksandra-malinowska commented Sep 19, 2024

yaroslava-serdiuk Sep 19, 2024

aleksandra-malinowska Sep 20, 2024

yaroslava-serdiuk Sep 20, 2024

Duke0404 Sep 20, 2024

yaroslava-serdiuk Oct 15, 2024

aleksandra-malinowska commented Sep 23, 2024

yaroslava-serdiuk Oct 15, 2024

Duke0404 Oct 16, 2024

yaroslava-serdiuk Oct 15, 2024

yaroslava-serdiuk Oct 15, 2024

Duke0404 Oct 16, 2024

yaroslava-serdiuk Oct 16, 2024

Duke0404 Oct 17, 2024

yaroslava-serdiuk Oct 15, 2024

Duke0404 Oct 16, 2024

yaroslava-serdiuk Oct 15, 2024

Duke0404 Oct 17, 2024

yaroslava-serdiuk commented Oct 16, 2024 •

edited

Loading

yaroslava-serdiuk Oct 16, 2024

yaroslava-serdiuk Oct 16, 2024

Duke0404 Oct 17, 2024

yaroslava-serdiuk Oct 16, 2024

yaroslava-serdiuk Oct 16, 2024

Duke0404 Oct 16, 2024

yaroslava-serdiuk commented Oct 28, 2024

mwielgus left a comment

k8s-ci-robot commented Oct 28, 2024

Implemented batch processing for check capacity provisioning class #7283

Implemented batch processing for check capacity provisioning class #7283

Conversation

Duke0404 commented Sep 13, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Sep 13, 2024

yaroslava-serdiuk commented Sep 16, 2024

yaroslava-serdiuk commented Sep 18, 2024

Duke0404 commented Sep 18, 2024

aleksandra-malinowska commented Sep 19, 2024

MaciekPytel commented Sep 19, 2024

aleksandra-malinowska commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksandra-malinowska commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaroslava-serdiuk commented Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaroslava-serdiuk commented Oct 28, 2024

mwielgus left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 28, 2024

yaroslava-serdiuk commented Oct 16, 2024 •

edited

Loading