State Machine Design #1 #427
Conversation
|
Looks Nice, I had one high-level question (before digging into the code), is there any mechanism present to (check &)avoid a loop among the different states. |
|
@irajdeep there nothing to prevent cycles within states (a cycle is used to handle scaling) UpdateSts -> UpdateAutomationConfig -> UpdateStatus -> UpdateSts etc. as only a single member can be updated at a time. However I think it makes sense to detect undesired cycles in some way and to essentially reset back to the first state if that is the case. |
Exactly, I am a bit wary about ending in this situation unknowingly and perform an indefinite reconciliation. |
|
@irajdeep maybe we can discuss this and create a follow up PR to address potential undesired cycles (we won't refactor any of the logic to use this new type until that is addressed) |
SGTM |
irajdeep
left a comment
irajdeep
left a comment
There was a problem hiding this comment.
Overall it looks pretty good to me. Added some comments -- let me know your thoughts.
| return retry(0, true) | ||
| } | ||
|
|
||
| func RetryState(after int) (reconcile.Result, error, bool) { |
There was a problem hiding this comment.
Can we add a comment explaining this method?
| // transition represents a transition between two states. | ||
| type transition struct { | ||
| from, to State | ||
| predicate TransitionPredicate |
There was a problem hiding this comment.
q: do we need an additional field predicate? Can we form `transition for only valid froms and tos?
There was a problem hiding this comment.
The predicate allows us to define what a valid transition is, for example, we want to transition from "CreateServce" -> "EnsureTLSResources" only when tls is enabled on the resource. We can do this with something like
sm.AddTransition(createServce, ensureTlsResources, func() bool {
return mdb.Spec.Security.Tls.Enabled
})We build up the StateMachine with each reconciliation, but the transitions that are valid are dependant on the current state of the resource.
pkg/util/state/statemachine.go
Outdated
| type State struct { | ||
| Name string | ||
| Reconcile func() (reconcile.Result, error, bool) | ||
| OnEnter func() error |
There was a problem hiding this comment.
Can we add a description for the usage of OnEnter ?
There was a problem hiding this comment.
I'll add some additional docstrings!
| } | ||
|
|
||
| // SaveLoader can both load and save the name of a state. | ||
| type SaveLoader interface { |
pkg/util/state/statemachine.go
Outdated
| // State completed successfully. A value of true will move onto the next State, | ||
| // a value of false will repeat this State until true is returned. | ||
| type State struct { | ||
| Name string |
There was a problem hiding this comment.
nit: State.Name has a finite (relatively low) number of values, wondering if Name can be made enum ?
There was a problem hiding this comment.
I chose to make it a string as this struct can potentially be used with enterprise as well.
| type Machine struct { | ||
| allTransitions map[string][]transition | ||
| currentState *State | ||
| logger *zap.SugaredLogger |
|
@irajdeep I've added some additional docstrings, please take another look when you get some time! We can add the prevention of cycles as discussed in a future PR! |
All Submissions:
This PR introduces the State Machine struct that will be used in a future PR to refactor our current reconciliation loop design.
Individual states are defined with a Reconcile function and they are then linked together using the
AddTransitionfunction.A boolean value is returned from these states to indicate if they have been completed, if not, they will be re-run in the next reconciliation. If they are compete, the next state will be executed in the next reconciliation.
closes #XXXXin your comment to auto-close the issue that your PR fixes (if such).