Add Capacity Buffer controller logic #8521

abdelrahman882 · 2025-09-11T11:07:59Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add Capacity Buffer controller loop along with the main needed skeleton for buffers with podTemplateRef.

Special notes for your reviewer:

This PR includes:

Filters: Capacity Buffers provisioning strategy and status filtering
Translators: podTemplateRef translator that updates buffer status accordingly
Updater: updates buffer status via capacity buffer client
Controller: Initiates the needed components and contains the reconciliation loop

This PR is not including:

Capacity buffers resources limits and scalable objects (those will be introduced as translators)
Buffers updating logic (use PodTemplateGeneration for updating buffer status)
These 2 points will be included in a following PR together
Running controller in CA and CA injection will be in separate PR

Proposal document: https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/proposals/buffers.md

Does this PR introduce a user-facing change?

no

Add Capacity Buffer controller logic

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

[AEP]:  https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/proposals/buffers.md

k8s-ci-robot · 2025-09-11T11:08:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: abdelrahman882
Once this PR has been reviewed and has the lgtm label, please assign feiskyer for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

cluster-autoscaler/OWNERS

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

BigDarkClown

A lot of duplicate comments, most important to address:

Naming should not include buffer infixes, it is redundant
Implementations should be private and not exposed outside of packages.
Cleanup methods for filters remove their configs, this is a bug.
The usage of filter interface should be consistent, eg. we should use the first return value in both cases. This will make it easier to maintain the implementations without caring about the use cases.

cluster-autoscaler/capacitybuffer/controller/buffer_controller.go

cluster-autoscaler/capacitybuffer/translators/buffers_translator.go

cluster-autoscaler/capacitybuffer/translators/pod_template_buffer_translator_test.go

cluster-autoscaler/capacitybuffer/updater/buffers_status_updater.go

cluster-autoscaler/capacitybuffer/controller/buffer_controller.go

jbtk

Reviewed the logic and structure. Did not look into tests as I have seen that @BigDarkClown did that already.

jbtk · 2025-09-11T14:57:00Z

cluster-autoscaler/capacitybuffer/controller/buffer_controller.go

+		select {
+		case <-stopCh:
+			return
+		case <-time.After(c.loopInterval):


Shouldn't we run the loop only if there are any buffers and have a watcher on creation of new so that it does not run without a good reason?

As mentioned in the reply below that will contradict with the periodic check over updated buffer objects and quota checking

I meant that we should not run the loop at all if there is no a single buffer object and for starting the loop we should rely on a watch that will inform us that a buffer was created. Once there are any buffers in the cluster we should run periodically and if all get deleted go back to relying on the watch. What would it contradict?

(Note that this can be implemented as optimization later)

jbtk · 2025-09-11T15:00:56Z

cluster-autoscaler/capacitybuffer/common/common.go

+
+// Constants to use in Capacity Buffers objects
+const (
+	ActiveProvisioningStrategy    = "active-capacity"


Should this be in common or should it be in some dedicated filter/translator.

The provisioning filter is generic, it filters the buffers with strategy defined when constructing, so we pass ActiveProvisioningStrategy to the generic provisioningFilter in the controller.

I would prefer keeping it in common because filter package is not only for strategy filtering so filter.ActiveProvisioningStrategy wouldn't give much more meaning imo and also in case it would be used by other filters/translators.

But It's not strong opinion, I would change it if you insist

I was thinking that we would keep the logic for a single buffer type in a single place and therefore keep the name together with translation logic and only make the buffer controller rely on the list of supported types. They would reuse the translation logic so that it is not implemented multiple times.

This is simple refactoring though so let's focus on having something working end to end first.

Also the plan was to go with "buffer.x-k8s.io/active-capacity" rather than just active-capacity as per discussion from https://docs.google.com/document/d/1bcct-luMPP51YAeUhVuFV7MIXud5wqHsVBDah9WuKeo/edit?disco=AAABj1R4vNs

(this will require update to validations)

cluster-autoscaler/capacitybuffer/filters/buffers_filter.go

jbtk · 2025-09-11T15:21:03Z

cluster-autoscaler/capacitybuffer/controller/buffer_controller.go

+	nodeBufferListener v1.CapacityBufferLister,
+	kubeClient kube_client.Clientset,
+) BufferController {
+	return &bufferController{


I thought we would split it more by buffer prov strategy or so in case different strategies need different handling, but for now I would say that it may be better to leave it as is and make sure that when adding second one we will introduce a structure that makes sense at that time.

In this structure it would be very hard to implement for example if translation of pods to pod template differs depending on the provisioning strategy. But as mentioned - not worth creating the whole structure up front. The logic seems well split to be reused and this is more important now

My understanding was that the controller will be operating on only one provisioningStrategy - So just to double check that I understood correctly, we need to have the provisioningStrategies mapped to set of translators and depending on the defined strategy we route the buffer to its corresponding translator, is that correct?

I am not sure whether we need this structure right away but I was assuming that we have a single controller to support all the provisioning strategies that have built in support. (If the user wants to have some custom ones they are free to implement their own controller and handle their translation and execution).

cluster-autoscaler/capacitybuffer/filters/buffers_strategy_filter.go

cluster-autoscaler/capacitybuffer/translators/buffers_translator.go

jbtk · 2025-09-11T15:39:08Z

cluster-autoscaler/capacitybuffer/translators/pod_template_buffer_translator.go

+	return errors
+}
+
+func (b *PodTemplateBufferTranslator) translate(buffer *api_v1.CapacityBuffer) (*api_v1.LocalObjectRef, int32, error) {


When do you plant to take the buffer.spec limit into account? Also the error is incorrect if you take into account the limits.

The limits will be applied by adding another translator for the limits, which will be executed last and will enforce the limits on the number of pods overriding replicas wrote by previous translators (podTemplateRef or scalableRef).

For your question when, It will be in separate PR right after merging this one, I have it already implemented but wrapping up its unit tests

In this case the error will be incorrect as we were planning to support an option: do not provide number of replicas and we will count how many replicas fit into the limit.

I would rather have a single place where we calculate the whole translation but provide libraries that handle limits or quotas so that you do not have to reimplement if there are many buffers that need the same logic. Chaining translators is like chaining processors - powerful but later to make any changes you need to understand the whole chain.

IMO it's better to keep it chained in this way, I mean to make a change you don't need to understand the whole chain at all but rather understand what you want you translator to do only.

One note, I will need to double check the API then because we have Xor=replicas,percentage which I am not sure if it will block having both set to nil

Okay so, my suggestion is to keep it as chained translators, and in the following PR -with introducing limits- I will be double checking the API validation to accept both replicas and percentage as nil along side with fixing this translator here to not return error, does that make sense?

Also I would appreciate if you took a look on the CRD validation and please let me know if you notice something odd.

We can do this in a follow up, but I do not like the idea of chaining in general.

If we provide libraries you will still have to understand only the part that you are modifying, but if you want to read what happens with a single buffer you will just read through a single place that has library calls rather than go through list of translators that are applied one on top of each other making modifying them more complex and error handling much harder. I know that we do a lot of this with processors in our code, but this is because these are extension points. Here I do not see a reason to do it as a list of translators.

jbtk · 2025-09-11T16:46:32Z

cluster-autoscaler/capacitybuffer/controller/controller.go

+	return &bufferController{
+		buffersLister:  nodeBufferListener,
+		strategyFilter: filters.NewStrategyFilter([]string{common.ActiveProvisioningStrategy}),
+		statusFilter: filters.NewBuffersStatusFilter(map[string]string{


Note that filtering on status isn't likely a good idea:

for pod template we will need to store the generation id. If the pod is update we have to process it once again (as there are limits)

for quotas we will need to process all the buffers over and over again to make sure that they fit into quotas

Got it,

For generation id: will update the status filter to include generation so we process those with higher generation id

For Quotas: will create another translator that applies on all buffers -including filtered out buffers- that updates status replicas

As mentioned Would add in following PR, this one is big already.

One note here we will need to run the loop periodically and not only if there are any buffers with a watcher on creation -from your previous comment- so we can check quotas and updates, does that make sense to you?

BigDarkClown

One micro nit. Overall LGTM, waiting for LGTM from @jbtk to add approval.

/lgtm

BigDarkClown · 2025-09-12T14:59:12Z

cluster-autoscaler/capacitybuffer/filters/strategy_filter.go

+}
+
+func (f *strategyFilter) isAllowedProvisioningStrategy(buffer *v1.CapacityBuffer) bool {
+	provisioingStrategy := ""


nit: typo provisioning

jackfrancis · 2025-09-12T16:07:17Z

/release-note-none

jackfrancis · 2025-09-12T16:07:44Z

/release-note-edit

Add Capacity Buffer controller logic

k8s-ci-robot · 2025-09-12T16:07:46Z

@jackfrancis: /release-note-edit must be used with a single release note block.

In response to this:

/release-note-edit
Add Capacity Buffer controller logic

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.

jackfrancis · 2025-09-12T16:08:35Z

/release-note-edit

Add Capacity Buffer controller logic

jbtk

Re validations (since the PR is already merged and we are discussing here some of the things):

xor on percentage and replicas is wrong - you can define neither (the replicas will be counted based on number of pod templates fitting the limits), you can also define both (this will mean that we will use number of replicas as minimum)
maximum for percentage - you could keep it higher than 100, for example 200 would mean that we keep twice the number of replicas compared to the current size of a workload. I guess this is not a common thing to do but I do not see why we should introduce this limitation

jbtk · 2025-09-12T20:15:34Z

cluster-autoscaler/capacitybuffer/controller/controller.go

+) BufferController {
+	return &bufferController{
+		buffersLister:  nodeBufferListener,
+		strategyFilter: filters.NewStrategyFilter([]string{common.ActiveProvisioningStrategy, ""}),


Please leave a comment for why the empty is accepted.

jbtk · 2025-09-12T20:25:53Z

cluster-autoscaler/capacitybuffer/translators/pod_template_buffer_translator.go

+	return errors
+}
+
+func (b *PodTemplateBufferTranslator) translate(buffer *api_v1.CapacityBuffer) (*api_v1.LocalObjectRef, int32, error) {


We can do this in a follow up, but I do not like the idea of chaining in general.

If we provide libraries you will still have to understand only the part that you are modifying, but if you want to read what happens with a single buffer you will just read through a single place that has library calls rather than go through list of translators that are applied one on top of each other making modifying them more complex and error handling much harder. I know that we do a lot of this with processors in our code, but this is because these are extension points. Here I do not see a reason to do it as a list of translators.

jbtk · 2025-09-12T20:27:23Z

cluster-autoscaler/capacitybuffer/common/common.go

+
+// Constants to use in Capacity Buffers objects
+const (
+	ActiveProvisioningStrategy    = "active-capacity"


Also the plan was to go with "buffer.x-k8s.io/active-capacity" rather than just active-capacity as per discussion from https://docs.google.com/document/d/1bcct-luMPP51YAeUhVuFV7MIXud5wqHsVBDah9WuKeo/edit?disco=AAABj1R4vNs

(this will require update to validations)

k8s-ci-robot requested review from feiskyer and vadasambar September 11, 2025 11:08

k8s-ci-robot removed the do-not-merge/needs-area label Sep 11, 2025

abdelrahman882 force-pushed the capacity-buffer-controller branch 4 times, most recently from 555355e to 829faae Compare September 11, 2025 12:40

BigDarkClown reviewed Sep 11, 2025

View reviewed changes

jbtk reviewed Sep 11, 2025

View reviewed changes

abdelrahman882 force-pushed the capacity-buffer-controller branch from 829faae to 312308a Compare September 11, 2025 16:44

jbtk reviewed Sep 11, 2025

View reviewed changes

abdelrahman882 force-pushed the capacity-buffer-controller branch 2 times, most recently from 3524194 to 5c1e317 Compare September 11, 2025 19:27

Add Capacity Buffer controller logic

1a7d949

abdelrahman882 force-pushed the capacity-buffer-controller branch from 5c1e317 to 1a7d949 Compare September 11, 2025 19:51

BigDarkClown reviewed Sep 12, 2025

View reviewed changes

k8s-ci-robot assigned BigDarkClown Sep 12, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2025

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 12, 2025

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 12, 2025

jbtk reviewed Sep 12, 2025

View reviewed changes

Add Capacity Buffer controller logic #8521

Are you sure you want to change the base?

Add Capacity Buffer controller logic #8521

Conversation

abdelrahman882 commented Sep 11, 2025 • edited by k8s-ci-robot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

Uh oh!

k8s-ci-robot commented Sep 11, 2025

Uh oh!

BigDarkClown left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbtk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abdelrahman882 Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BigDarkClown left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

abdelrahman882 commented Sep 11, 2025 •

edited by k8s-ci-robot

Loading

abdelrahman882 Sep 11, 2025 •

edited

Loading