Introduce ability to specify strategies for target allocation #1068
Conversation
…nto pluggable-strategies
…nto pluggable-strategies
| } | ||
|
|
||
| // SetCollectors sets the set of collectors with key=collectorName, value=Collector object. | ||
| // SetCollectors sets the set of collectors with key=collectorName, value=CollectorName object. |
There was a problem hiding this comment.
I don't think we need this comment change - it makes it seem like key == value.
| collectors: make(map[string]*collector), | ||
| targetItems: make(map[string]*TargetItem), | ||
| log: log, | ||
| state: strategy.NewState(make(map[string]strategy.Collector), make(map[string]strategy.TargetItem)), |
There was a problem hiding this comment.
If we don't need/use the maps here, shouldn't the NewState() function be able to handle nil values and make the maps itself?
| for colName, _ := range allocator.Collectors() { | ||
| if colName == collector { |
There was a problem hiding this comment.
The colName can only be in the map once - can we try to access it directly rather than traversing the whole map?
| }, []string{"collector_name"}) | ||
| ) | ||
|
|
||
| func New(name string) (Allocator, error) { |
There was a problem hiding this comment.
Sorry if this is obvious or has already been discussed, but what is the benefit of the init() registration pattern? Couldn't we pass an interface into New() instead and not require global state? Any caller will always have to import their strategy (or strategies) and this package.
There was a problem hiding this comment.
Having a mapping of string identifiers to strategy implementations enables using the CRD or other configuration mechanisms to select from among multiple available strategies at runtime. This could be implemented by the consuming system, but would likely result in duplicate implementation.
There was a problem hiding this comment.
got it, thanks for explaining! 🙂
| NumTargets int | ||
| } | ||
|
|
||
| type State struct { |
There was a problem hiding this comment.
All of this state stuff seems like it belongs in its own file. I'm not sure if some of these other things should also be split up?
| collectors map[string]Collector | ||
| // targetItems is a map from a target item's hash to the target items allocated state | ||
| targetItems map[string]TargetItem |
There was a problem hiding this comment.
The lock for these maps are up in allocation.Allocator but then they are also modified in the strategy - it seems easy to change something later and mess it up.
There was a problem hiding this comment.
This is why I think it is important that State is immutable and that strategies return a new State instance. Then the locks held by Allocator are protecting the "active" State and anything modifying State doesn't need to worry about locking since it's immutable and always returning a new State.
There was a problem hiding this comment.
Yeah, I think only one object should be modifying the state if it's going to be modified - making it immutable would also solve the problem.
|
As discussed in slack, I'm going to close this PR in favor of a different approach where we instead make the allocator itself the interface instead of making the strategy a part of the allocator. See this conversation for more detail |
Goals:
Summary
This PR introduces the AllocationStrategy interface, implements it through a refactor of the current allocator, and changes the configuration of the TA to allow for a user to specify an option. The operator's CRDs are also updated to allow the strategy to be specified in the Collector CRD. New tests have been introduced to confirm the strategy works as expected. All previous tests should continue working.