Sort cell-names in nova_controller #903
Conversation
|
Hi @ratailor. Thanks for your PR. I'm waiting for a openstack-k8s-operators member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
How does the code on main leads to unstable condition messages? Could you please add that description to the PR's message? |
controllers/nova_controller.go
Outdated
| // iteration order is undefined we need to make sure that cell0 is the | ||
| // first to allow dependency handling during ensureCell calls. | ||
| orderedCellNames := []string{novav1.Cell0Name} | ||
| sortedCellNames := []string{} |
There was a problem hiding this comment.
we defintlly do not need to have 2 ordered lists of the same thing
if we were to do this, we shoudl not be keeping both the order cell name and sorted ones.
the entire point of orderedCellNames is to enforce a stable order.
As of now, the orderedCellNames variable contains 'cell0' as first element and then appends cell-names coming from CellTemplates map, which doesn't provide stable ordering. Then the CR status condition message could change for each reconcile with any change in the actual status of the CR. And a changing status message could trigger unnecessary reconcile loops. This change sorts the cell-names and also make sure that the cell0 is the first element in the orderedCellNames slice to maintain stable ordering. Closes: #OSPRH-9980
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ratailor 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 |
Done |
| // We need to sort the list of cellNames to iterate on to avoid | ||
| // unnecessary reconcile loop runs, as the map iteration order is undefined. |
There was a problem hiding this comment.
This is not for avoiding reconcile loops. It is for avoiding that different reconcile loops sees a different cell order while the cell creation is in the same state but generate a different condition message due to the different order.
|
I still would like to get a commit message / PR description that describes why we are doing this instead of / top of what the patch does. |
|
IMO it will be good to have some functional tests |
5 participants
This change sorts the cell-names and also make sure that the cell0 is the first element in the orderedCellNames slice.
Closes: #OSPRH-9980