Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Using Finite State Machine library to expose state diagram for configuration applied transitions #210

Closed
wants to merge 14 commits into from

Conversation

edcdavid
Copy link
Collaborator

@edcdavid edcdavid commented Sep 19, 2024

Proposing to use https://github.com/qmuntal/stateless to expose the implicit state diagram used for The configuration applied status

  • With each calls to reconcile, the state machine is initialized with the current state saved in configurationAppliedCondition.Reason
  • The FsmHelper collects all the data required by the state machine to make its transition decisions. It is initialized during reconciliation with .InitFromCluster()
  • updateConfigurationAppliedStatus pushes updates to the status back to the cluster

The fsm_test.go tests the state machine transitions independently from kubenetes.
The ci-job process updates the state diagram in documentation (dev/config_fsm.mg link)

TODO:

  • Unit tests
  • add CGO in prow for creating SVG in docs
  • system test - retesting

internal/configfsm/fsm.go Outdated Show resolved Hide resolved
internal/configfsm/fsm.go Outdated Show resolved Hide resolved
internal/configfsm/fsm.go Outdated Show resolved Hide resolved
@edcdavid
Copy link
Collaborator Author

/cc @irinamihai @Missxiaoguo

@Missxiaoguo
Copy link
Collaborator

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2024
Copy link
Collaborator

@pixelsoccupied pixelsoccupied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it's a nice use of fsm, i dont know if the concept of finite statement machine is compatible with the notion of k8s controller? Meaning operators are meant to be "level based", i.e look at the state of the cluster at every requeue until we reach desired state vs "edge-based" where we are relying to history stored in status.

Api convention doc specifically says .status should not be used for state machines.

lots of great doc on this:

Since this is totally new pattern we are introducing (I couldnt find any reference of fsm lib in operators anywhere), at the very least this should be better documented for anyone coming to develop this in the future?

The State diagram of the configuration state machine is generated below in dot format.
A Link to the rendered diagram is at: [link](https://dreampuf.github.io/GraphvizOnline/?presentation#digraph%20%7B%0A%09compound=true%3B%0A%09node%20%5Bshape=Mrecord%5D%3B%0A%09rankdir=%22LR%22%3B%0A%0A%09ClusterNotReady%20%5Blabel=%22ClusterNotReady%7Centry%20%2F%20func2%22%5D%3B%0A%09Completed%20%5Blabel=%22Completed%7Centry%20%2F%20func5%22%5D%3B%0A%09InProgress%20%5Blabel=%22InProgress%7Centry%20%2F%20func3%22%5D%3B%0A%09Missing%20%5Blabel=%22Missing%7Centry%20%2F%20func1%22%5D%3B%0A%09OutOfDate%20%5Blabel=%22OutOfDate%7Centry%20%2F%20func4%22%5D%3B%0A%09Start%20%5Blabel=%22Start%22%5D%3B%0A%09TimedOut%20%5Blabel=%22TimedOut%7Centry%20%2F%20func6%22%5D%3B%0A%09ClusterNotReady%20-%3E%20ClusterNotReady%20%5Blabel=%22ClusterNotReady-&gt%3BClusterNotReady%22%5D%3B%0A%09ClusterNotReady%20-%3E%20InProgress%20%5Blabel=%22ClusterNotReady-&gt%3BInProgress%22%5D%3B%0A%09ClusterNotReady%20-%3E%20Missing%20%5Blabel=%22ClusterNotReady-&gt%3BMissing%22%5D%3B%0A%09Completed%20-%3E%20Completed%20%5Blabel=%22Completed-&gt%3BCompleted%22%5D%3B%0A%09Completed%20-%3E%20InProgress%20%5Blabel=%22Completed-&gt%3BInProgress%22%5D%3B%0A%09InProgress%20-%3E%20ClusterNotReady%20%5Blabel=%22InProgress-&gt%3BClusterNotReady%22%5D%3B%0A%09InProgress%20-%3E%20Completed%20%5Blabel=%22InProgress-&gt%3BCompleted%22%5D%3B%0A%09InProgress%20-%3E%20InProgress%20%5Blabel=%22InProgress-&gt%3BInProgress%22%5D%3B%0A%09InProgress%20-%3E%20OutOfDate%20%5Blabel=%22InProgress-&gt%3BOutOfDate%22%5D%3B%0A%09InProgress%20-%3E%20TimedOut%20%5Blabel=%22InProgress-&gt%3BTimedOut%22%5D%3B%0A%09Missing%20-%3E%20ClusterNotReady%20%5Blabel=%22Missing-&gt%3BClusterNotReady%22%5D%3B%0A%09Missing%20-%3E%20Missing%20%5Blabel=%22Missing-&gt%3BMissing%22%5D%3B%0A%09OutOfDate%20-%3E%20InProgress%20%5Blabel=%22OutOfDate-&gt%3BInProgress%22%5D%3B%0A%09OutOfDate%20-%3E%20OutOfDate%20%5Blabel=%22OutOfDate-&gt%3BOutOfDate%22%5D%3B%0A%09Start%20-%3E%20Missing%20%5Blabel=%22Start-&gt%3BMissing%22%5D%3B%0A%09TimedOut%20-%3E%20InProgress%20%5Blabel=%22TimedOut-&gt%3BInProgress%22%5D%3B%0A%09TimedOut%20-%3E%20TimedOut%20%5Blabel=%22TimedOut-&gt%3BTimedOut%22%5D%3B%0A%09init%20%5Blabel=%22%22%2C%20shape=point%5D%3B%0A%09init%20-%3E%20Start%0A%7D%0A)
```
digraph {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible generate this using make generate by sm.ToGraph() as mentioned here

Also since this generated...would it be nicer to move this under say docs/generated/clusterrequest-fsm to reflect the content better?

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2024
Copy link

openshift-ci bot commented Sep 30, 2024

New changes are detected. LGTM label has been removed.

Copy link

openshift-ci bot commented Sep 30, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from missxiaoguo. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2024
@openshift-merge-robot
Copy link
Collaborator

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.

@edcdavid
Copy link
Collaborator Author

Closing this PR for now, does not follow the no state/machine recommendation for operator. Should recompute the state from information available in the cluster. This follow up PR is attempting to do this: #246

@edcdavid edcdavid closed this Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants