Put ScaleUp logic behind an interface #5597
Conversation
* Move resource manager to a dedicated package * Move pod equivalence groups to dedicated package
* Add ScaleUpManager interface, which is copy of existing stand-alone functions * Add a wrapper which contains the current scale up logic code
* Simplify the ScaleUp* functions parameter list * Introduce the ScaleUpManagerFactory to allow greater expandability * Simplify helper functions in scale up wrapper * Make the SkippedReasons public and move those to a dedicated file
* Introduced ExecuteScaleUp function, which runs chosen expansion options. * Lowered intendation of ScaleUp function, by reverting bestOption check. * Unified error and Autoscaling error variable names to 'err' and 'aErr'.
k8s-ci-robot
added
the
kind/cleanup
k8s-ci-robot
added
area/cluster-autoscaler
cncf-cla: yes
* Make the structures public, as well as some helper functions * manager.go to scaleup.go file rename * Minor code simplifications * Minor comment fixes/style consolidations
kisieland
force-pushed
the
scale-up-changes-v2
branch
from
1169d55 to
26a3604
Compare
* Add field names to struct literals * Slice iteration/population standardized * Add boilerplate and fix comment lint errors
kisieland
force-pushed
the
scale-up-changes-v2
branch
from
26a3604 to
9cd44fd
Compare
| ) | ||
|
|
||
| // ManagerFactory is a component that creates a new instance of the scale up manager. | ||
| type ManagerFactory interface { |
There was a problem hiding this comment.
What is the benefit of having a factory? Assuming there is one - what is the benefit of putting the factory object behind an interface? I think interfaces are useful if you have multiple possible implementations (e.g. mock for testing), but I don't think this is the case here.
There was a problem hiding this comment.
The idea was to allow users to specify a component with a custom scale up logic, unfortunately some of the required objects (autoscalingContext, clusterStateRegistry) are created in NewStaticAutoscaler function.
Another option I considered was to add an Initialize function to the Manager interface, if you think it makes more sense I can rewrite it.
There was a problem hiding this comment.
Oh, right, I forgot that function is quite a dependency mess. I think a factory is a bit more natural than the Initialize method, but I don't have a strong opinion.
There was a problem hiding this comment.
I think Initialize looks better so leaving it for now.
|
@x13n thanks for the review, Uploaded a new version which:
PTAL |
* Rename scaleup.Manager to scaleup.Orchestrator * Remove factory and add Initialize function * Rename the wrpapper package to orchestrator * Rename NewOrchestrator func to just New
kisieland
force-pushed
the
scale-up-changes-v2
branch
from
6957887 to
5b6c50e
Compare
|
@x13n responded to last comment, PTAL :) |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kisieland, 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Which component does this PR applies to?
cluster-autoscaler
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This refactoring is similar to the one done for scale down logic in: #4806
The scale up is put behind interface and can be overridden by users.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This is a huge change and can be hard to review. I divided it into 6 commits that are self-contained (i.e. tests were passing on each commit). Looking at the changes may be simpler one commit at a time.
Note: any comments on how to modify the interface and make it more user expendable are welcome.
Does this PR introduce a user-facing change?
/assign @x13n
/cc @towca