Refactor: Move AutoscalingContext to Estimate func

[azylinski]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

The Estimator Builder should use (wide) AutoscalingContext instead of (specific sub-) params.
That would be consistent with other interfaces, e.g.: all processors, scaleup/scaledown.

Does this PR introduce a user-facing change?

"NONE"

/assign @x13n

[vadasambar]

The Estimator Builder should use (wide) AutoscalingContext instead of (specific sub-) params.
That would be consistent with other interfaces, e.g.: all processors, scaleup/scaledown.

Is there any other reason apart from consistency? I don't see why estimator should match processors or scaleup/scaledown when it comes to passing parameters. They are quite different constructs.

I think passing context to the estimator makes the function definition less prone to changes in the future because you can just use anything from the context (right now you have to pluck the value from the context and pass it to the estimator) but that also means estimator now knows a whole lot more than it should. Do we really need this?

[azylinski]

Is there any other reason apart from consistency? I don't see why estimator should match processors or scaleup/scaledown when it comes to passing parameters. They are quite different constructs.

I'd say that estimator (especially: estimationAnalyserFunc and binpackingLimiter) are pretty much processors.

estimator now knows a whole lot more than it should

I agree 100%. IMHO AutoscalerContext by itself is more than it should.
I see it as a collection of: options, clients and "global" state, that we pass around. It's not ideal (again, just my opinion), but it's a design decision that was made long time ago.
And again, this is currently the case for all the processors.

definition less prone to changes in the future because you can just use anything from the context

In essence, that's the direction I'm aiming for.
Current PR is 2nd part of refactor - and there's one more to come, when I'd like to move nodeInfos (or better: TemplateNodesForGroups) to context as well. That's not only a huge code simplification, but also pairs well wth ClusterStateRegistry "level".

[MaciekPytel]

estimator now knows a whole lot more than it should

I agree 100%. IMHO AutoscalerContext by itself is more than it should.

I think this is a decision we already took with introduction of processors and the definition of AutoscalingContext. The idea was that we want to allow cloud providers / advanced users to have broad power to customize CA to fit the specifics of their platform, including arbitrary changes to internal CA logic.

CA cloudprovider interface expects very little from provider, which makes it relatively easy to add integrations (as demonstrated by the number of integrations we have). The downside is that our interface and our generic logic is essentially a lowest common denominator which makes it very hard to use more advanced cloud provider features (e.g. how long it took for CA integration with MixedInstancesPolicy? How limiting that integration is to this day?).

The idea of introducing processors was to address this limitation, by giving providers tools to essentially completely change arbitrary parts of CA logic. To this end we've added programming hooks (processors) in most critical parts of CA and gave them full access to all the internal state (i.e. AutoscalingContext). The expectation was users forking CA and writing custom processors that can significantly change / override core logic in order to support their specific use-cases that cannot be satisfied purely via implementation of cloudprovider interface.

To sum up: we designed AutoscalingContext the way it is, specifically to allow people to inject arbitrary and unexpected logic in CA extension points (processors). It can be argued if this is the right approach, but I think pushing AutoscalingContext to any interface that we reasonably expect to be customized by users (e.g. EstimationAnalyserFunc or BinpackingLimiter) is at least consistent with what we've been doing elsewhere in CA codebase.

[vadasambar]

Is there any other reason apart from consistency? I don't see why estimator should match processors or scaleup/scaledown when it comes to passing parameters. They are quite different constructs.

I'd say that estimator (especially: estimationAnalyserFunc and binpackingLimiter) are pretty much processors.

estimator now knows a whole lot more than it should

I agree 100%. IMHO AutoscalerContext by itself is more than it should. I see it as a collection of: options, clients and "global" state, that we pass around. It's not ideal (again, just my opinion), but it's a design decision that was made long time ago. And again, this is currently the case for all the processors.

definition less prone to changes in the future because you can just use anything from the context

In essence, that's the direction I'm aiming for. Current PR is 2nd part of refactor - and there's one more to come, when I'd like to move nodeInfos (or better: TemplateNodesForGroups) to context as well. That's not only a huge code simplification, but also pairs well wth ClusterStateRegistry "level".

On a second look, I see why you say estimator and limiter are processors :)

[vadasambar]

estimator now knows a whole lot more than it should

I agree 100%. IMHO AutoscalerContext by itself is more than it should.

I think this is a decision we already took with introduction of processors and the definition of AutoscalingContext. The idea was that we want to allow cloud providers / advanced users to have broad power to customize CA to fit the specifics of their platform, including arbitrary changes to internal CA logic.

CA cloudprovider interface expects very little from provider, which makes it relatively easy to add integrations (as demonstrated by the number of integrations we have). The downside is that our interface and our generic logic is essentially a lowest common denominator which makes it very hard to use more advanced cloud provider features (e.g. how long it took for CA integration with MixedInstancesPolicy? How limiting that integration is to this day?).

The idea of introducing processors was to address this limitation, by giving providers tools to essentially completely change arbitrary parts of CA logic. To this end we've added programming hooks (processors) in most critical parts of CA and gave them full access to all the internal state (i.e. AutoscalingContext). The expectation was users forking CA and writing custom processors that can significantly change / override core logic in order to support their specific use-cases that cannot be satisfied purely via implementation of cloudprovider interface.

To sum up: we designed AutoscalingContext the way it is, specifically to allow people to inject arbitrary and unexpected logic in CA extension points (processors). It can be argued if this is the right approach, but I think pushing AutoscalingContext to any interface that we reasonably expect to be customized by users (e.g. EstimationAnalyserFunc or BinpackingLimiter) is at least consistent with what we've been doing elsewhere in CA codebase.

Appreciate the context. Might be a good idea to document this somewhere (feels like tribal knowledge).

[k8s-ci-robot]

PR needs rebase.

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.

[x13n]

I just realized I never replied here. Approving, based on the reasoning above. @azylinski can you rebase?

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: azylinski, x13n

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 [x13n]

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

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.