OCPBUGS-64681: fix: add arbiter to control plane mcp sync #5436
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@eggfoobar: This pull request references Jira Issue OCPBUGS-64681, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: eggfoobar 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 |
|
/test e2e-metal-ovn-two-node-arbiter |
|
@eggfoobar: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/6f953230-c699-11f0-960e-472a28fcb2d9-0 |
openshift-ci-robot
added
jira/valid-bug
|
@eggfoobar: This pull request references Jira Issue OCPBUGS-64681, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
during upgrades we need the arbiter mcp nodes to be counted in the same calculation for upgrading control plane nodes so an arbiter nodes does not update at the same time as a control plane node causing a quorum loss in etcd Signed-off-by: ehila <[email protected]>
eggfoobar
force-pushed
the
ocpbugs-64681-arbiter-update
branch
from
300a354 to
301ec41
Compare
|
/test e2e-metal-ovn-two-node-arbiter |
|
@eggfoobar: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ffac67c0-c708-11f0-9349-9d312453556e-0 |
|
/retest-required |
|
@eggfoobar: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
| } | ||
| combinedNodes := append([]*corev1.Node{}, nodes...) | ||
| combinedNodes = append(combinedNodes, arbiterNodes...) | ||
| combinedMax, err := maxUnavailable(pool, combinedNodes) |
There was a problem hiding this comment.
So, this basically implies that the arbiter pool's maxUnavailable no longer has an effect. Maybe we should not allow setting that field at all.
(in practice nobody should be fiddling with maxUnavailable for either masters or arbiters, so it probably doesn't matter, just wanted to note that just in case)
| arbiterUnavailable := len(getUnavailableMachines(arbiterNodes, arbiterPool)) | ||
| // Adjust maxunavail to account for arbiter unavailable nodes | ||
| // This ensures we don't exceed the combined maxUnavailable across both pools | ||
| maxunavail = combinedMax - arbiterUnavailable |
There was a problem hiding this comment.
the previously set maxunavail should just be a value set in the pool, and the candidate selection below filters for any in progress or not ready nodes. Given that we have some complex logic below, is it needed to subtract arbiter here?
| } | ||
|
|
||
| // If coordinating with arbiter pool, also handle arbiter node updates | ||
| if arbiterPool != nil && len(arbiterNodes) > 0 { |
There was a problem hiding this comment.
Should this whole section also be gated via if pool.Name == ctrlcommon.MachineConfigPoolMaster && controlPlaneTopology == configv1.HighlyAvailableArbiterMode similar to above? It seems possible that we'd be syncing the arbiter pool during a worker sync which can happen in parallel with masters, which might be unsafe
| // If coordinating with arbiter pool, also handle arbiter node updates | ||
| if arbiterPool != nil && len(arbiterNodes) > 0 { | ||
| // Set cluster config annotation for arbiter nodes | ||
| if err := ctrl.setClusterConfigAnnotation(arbiterNodes, controlPlaneTopology); err != nil { |
There was a problem hiding this comment.
Wondering if we could combine this with the same function call earlier somehow, or move it closer (although I guess functionally speaking it shouldn't matter)
There's quite a bit of duplicated code in general. I guess the main reason we can't just merge into the above functions is that arbiter has a different desiredConfig annotation it would need to set? Would it be easier if we modified the updateCandidateMachines function to account for that and treat the arbiter node as a master node in this function?
| combinedNodes = append(combinedNodes, arbiterNodes...) | ||
| combinedMax, err := maxUnavailable(pool, combinedNodes) | ||
| if err == nil { | ||
| remainingCapacity := combinedMax - masterUnavailable - masterTargeted - arbiterUnavailable |
There was a problem hiding this comment.
I'm having trouble following this set of logic we used for calculation. Given that we calculated the capacity (including arbiter) earlier, and tracked the amount of masters being updated in this round, wouldn't it be just be capacity - masterTargeted? Would it be possible to simplify this logic somehow? It feels like a lot of potentially unnecessary calculations on top of all the duplication, and would be hard to maintain in the future. I think I still prefer somehow merging arbiter into master calculation and keep most of the function unduplicated, just set the desired annotation differently if arbiter gets selected. Ultimately this function is designed to update some nodes' annotations to the new desired config, and I'm hoping we can keep the core functionality the same without having to special case too much.
3 participants
During upgrades we need the arbiter mcp nodes to be counted in the same calculation for upgrading control plane nodes so an arbiter nodes does not update at the same time as a control plane node causing a quorum loss in etcd
- What I did
- How to verify it
- Description for the changelog