plugin: Transaction-based speculative reserve and logic unification #936
Conversation
sharnoff
force-pushed
the
sharnoff/plugin-replace-readClusterState
branch
from
b1e53de
to
6555a77
Compare
sharnoff
force-pushed
the
sharnoff/plugin-replace-readClusterState
branch
from
c8e250e
to
ceceb07
Compare
sharnoff
force-pushed
the
sharnoff/plugin-transactions
branch
3 times, most recently
from
921c0d4
to
51f1d82
Compare
| ) | ||
| } | ||
|
|
||
| overbudget = r.node.Reserved > r.node.Total |
There was a problem hiding this comment.
Shouldn't it be r.node.Reserved + r.node.Buffer > r.node.Total?
There was a problem hiding this comment.
Buffer is included in Reserved, so it should be ok as is
| // New returns a new transaction object (called Xact) operating on the given pointer | ||
| func New[T any](ptr *T) *Xact[T] { | ||
| return &Xact[T]{ | ||
| tmp: *ptr, |
There was a problem hiding this comment.
The thing I am a little bit afraid of is T might have pointers, which will not be part of the transaction state.
We may consider pointer members not proper members, therefore, not part of a transaction, and maybe clarify this thing in comments.
Or, we can change this to reflect.DeepCopy.
WDYT?
There was a problem hiding this comment.
That's fair. Added a note about this in 0972c4c -- the types I intended to use for this don't have pointers (and at least IMO, that's a reasonable restriction for now).
|
|
||
| r.node.Reserved += r.pod.Reserved | ||
| r.node.Buffer += r.pod.Buffer | ||
| callback := func(oldState, newState resourceState[T]) string { |
There was a problem hiding this comment.
As discussed, perhaps all verdict formatting should be gathered at a single place, like a structure verdictGenerator, which will be used by handleRequestedGeneric.
There was a problem hiding this comment.
I like that idea -- I think it may end up somewhat complex; wdyt about leaving it as a follow-up?
| @@ -150,26 +134,130 @@ func (r resourceTransitioner[T]) handleReserve() (overbudget bool, verdict strin | |||
| // Any permitted increases are required to be a multiple of factor. | |||
| // | |||
| // A pretty-formatted summary of the outcome is returned as the verdict, for logging. | |||
There was a problem hiding this comment.
It would be easier to understand, if those comments would more clearly reflect the difference between handleRequested and handleReserve.
There was a problem hiding this comment.
Fair enough - added some comments in bbfca61, although not sure if there's more I should elaborate on.
sharnoff
force-pushed
the
sharnoff/plugin-transactions
branch
from
51f1d82
to
7d911b4
Compare
This commit adds 'pkg/util/xact' with usage in scheduler plugin such that all places that previously implemented checks for "can this pod fit on that node" are now done by speculatively reserving the pod, checking if it left the node over-full, and then deciding whether to accept it. This is unified through a new method speculativeReserve(). Internally, speculativeReserve() calls a new method on resourceTransitioner, which makes follow-up work easier.
This introduces a new method on resourceTransitioner[T],
handleRequestedGeneric(), which implements a superset of the
functionality needed by both handleReserve and handleRequested.
The two latter methods now internally "just" call the new method, albeit
with different parameterizations.
---
Other changes:
- The new method has a notion of "forced approval minimum", which sets
the minimum value it must approve, even if the node would be
over-budget.
- This is used to retain the infallible behavior of handleReserve.
- This is *also* used to move the lastPermit logic into
handleRequested.
- Because of the new forced approval minimum, handleLastPermit is no
longer needed.
---
In general, the motivation behind this change is to reduce the number of
distinct implementations of resource logic, so there's fewer changes
involved in implementing overcommit.
sharnoff
force-pushed
the
sharnoff/plugin-transactions
branch
from
bbfca61
to
148a85d
Compare
This commit adds 'pkg/util/xact' with usage in scheduler plugin such that all places that previously implemented checks for "can this pod fit on that node" are now done by speculatively reserving the pod, checking if it left the node over-full, and then deciding whether to accept it. This is unified through a new method speculativeReserve(). Internally, speculativeReserve() calls a new method on resourceTransitioner, which makes follow-up work easier.
Trying out this approach as a potential way to make implementing overcommit (#517) easier, to avoid dealing with rounding errors when responding to the autoscaler-agents.
I think this "speculative reserve" may be a reasonable change in isolation, and reducing the number of separate implementations of the same logic should also help with overcommit (because of how it affects every single resource calculation).
Not 100% sure we want to go this route, but it's my best guess so far.
Tested this locally and it seemed to be ok; already fixed a couple bugs based on suspicious logs. There might be more.