-
Notifications
You must be signed in to change notification settings - Fork 270
Bug 1822296: Expose raft (nb-db/sb-db) election-timer and ovn-controller inactivit… #615
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,12 @@ spec: | |
| value: "quay.io/openshift/origin-multus-route-override-cni:4.4" | ||
| - name: OVN_IMAGE | ||
| value: "quay.io/openshift/origin-ovn-kubernetes:4.3" | ||
| - name: OVN_NB_RAFT_ELECTION_TIMER | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be added the operator configuration CRD, which is defined here: https://github.com/openshift/api/blob/master/operator/v1/types_network.go#L37
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However, that is a long process, and won't be in in time for 4.5. The problem is that these fields will be instantly overwritten by the CVO.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally i don't like this PR as well. Mainly because the method that OVN provides us to set the election-timer is not very operation friendly. I already raised a RFE for OVN team to improve this and take election-timer as a parameter to ovn-ctl. That will make the entire logic pretty simple and we can write more stable logic to set the timer. So i am hoping that we will get rid of this logic, once OVN provides us a better way to set this timer.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense. I'm arguing that this value should either be hard-coded in the code, or available as a configuration knob.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am more in the favor of having it as a configuration knob, because for raft based implementation, choosing approximately right election-timer depends on the workload.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @squeed that it should be hard-coded for now, until we have a better idea whether it acutally does need to be changed. It should not be user-visible configuration at any rate, especially not in the API.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dcbw In general we want to keep these numbers as minimal as possible. Currently these numbers are set for 200 nodes (for 50 pods per node workload), so the moment number of nodes increases, you will have to increase it, there is no question about it. We can hardcode it for 500 nodes as of now (60 seconds-- frankly it's very high), and live with it until and unless something else comes up that require it to change it. |
||
| value: "5000" | ||
| - name: OVN_SB_RAFT_ELECTION_TIMER | ||
| value: "5000" | ||
| - name: OVN_CONTROLLER_INACTIVITY_PROBE | ||
| value: "30000" | ||
| - name: KURYR_DAEMON_IMAGE | ||
| value: "quay.io/openshift/origin-kuryr-cni:4.3" | ||
| - name: KURYR_CONTROLLER_IMAGE | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this all block ovn-kube master process? Would it make sense for it to be a separate container? It could just apply changes as necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think setting this timer while the master pods startup will give us bit more deterministic behavior. If user start cluster with high number of worker nodes, we can get into the death loop of raft partition.
Also i believe the general guideline is that we don't want to allow user to change this bahavior dynamically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, what I'm saying is that this should be a separate container in the master pod, rather than gating the master container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we think of allowing user to change this value dynamically, i think having separate container makes sense to me. But if it's one time startup config, there is nothing much for this container to do, it will come up, set the value and die, and not sure CNO will be happy with that.