refactor: stage consolidation for overwritable consolidationTTL #2312
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
the
do-not-merge/work-in-progress
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rschalo The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/L
rschalo
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/retest |
| recorder: recorder, | ||
| } | ||
| func NewDrift(c consolidation) *Drift { | ||
| return &Drift{consolidation: c} |
There was a problem hiding this comment.
Drift contains consolidation? That doesn't seem right to me. Perhaps we need to change the naming, but I'm also wondering what the need is to encapsulate all of the details of this for drift
There was a problem hiding this comment.
No need to encapsulate all of the details, we were passing in members of consolidation and this is looking to unify what is required for a new Method. If every Method takes in consolidation then it could just use what it needs - I think a change to accept a consolidation as a param but then change back to a struct that isn't just consolidation is reasonable.
| provisioner: provisioner, | ||
| cloudProvider: cloudProvider, | ||
| recorder: recorder, | ||
| consolidationTTL: consolidationTTL, |
There was a problem hiding this comment.
This is static, right? So is there a need to define this as a consolidation member variable
|
|
||
| type ConsolidationOption func(*consolidation) | ||
|
|
||
| func WithConsolidationTTL(ttl time.Duration) ConsolidationOption { |
There was a problem hiding this comment.
Do you need this or can this just be a property of the validator directly?
| @@ -74,7 +74,7 @@ func NewControllers( | |||
|
|
|||
| controllers := []controller.Controller{ | |||
| p, evictionQueue, disruptionQueue, | |||
| disruption.NewController(clock, kubeClient, p, cloudProvider, recorder, cluster, disruptionQueue), | |||
| disruption.NewController(disruption.MakeConsolidation(clock, cluster, kubeClient, p, cloudProvider, recorder, disruptionQueue)), | |||
There was a problem hiding this comment.
I'm curious why you want to encapsulate everything inside of consolidation and pass it to the constructor like this. If you are literally wrapping everything in the constructor, it sort of begs the question why you even need to wrap it in the first place
There was a problem hiding this comment.
It's because when we make the disruption controller in our int tests, there isn't an opportunity to customize validator behavior. We could make consolidationTTL a member of Validator and export the Validator on each Method and then in the tests where relevant overwrite it there.
There was a problem hiding this comment.
Discussed offline - going to export Methods on the controller and overwrite methods individually as needed.
k8s-ci-robot
added
the
needs-rebase
|
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-sigs/prow repository. |
Fixes #N/A
Description
Simplify code used in validation and set up consolidation to take in a customizable consolidationTTL which will eventually be used to remove
ExpectToWaitin consolidation tests.How was this change tested?
make presubmitBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.