controller: use slice instead of map to conserve consistent order #1052
Conversation
openshift-ci
bot
added
the
approved
| func syncNodeGroupsStatus(instance *nropv1.NUMAResourcesOperator, dsPerMCP []MCPDaemonSetPair) []nropv1.NodeGroupStatus { | ||
| ngStatuses := []nropv1.NodeGroupStatus{} | ||
| for _, mcp := range instance.Status.MachineConfigPools { | ||
| status := nropv1.NodeGroupStatus{ | ||
| PoolName: mcp.Name, | ||
| Config: *mcp.Config, | ||
| DaemonSet: dsPerMCP[mcp.Name], | ||
| for _, info := range dsPerMCP { | ||
| if mcp.Name != info.MCPName { | ||
| continue | ||
| } | ||
|
|
||
| status := nropv1.NodeGroupStatus{ | ||
| PoolName: mcp.Name, | ||
| Config: *mcp.Config, | ||
| DaemonSet: info.Daemonset, | ||
| } | ||
| ngStatuses = append(ngStatuses, status) |
There was a problem hiding this comment.
thanks for the fix. We can use another fix later: this function should create the NodeGroupStatus using dsPerMCP as master source (iterating over there) and not depend on other status fields like instance.Status.MachineConfigPools.
There was a problem hiding this comment.
we need to be aligned with the status update on MCP, currently the config won't be updated there if MCP is not completely ready. I'll look into this anyhow, we can use an improvement here indeed
There was a problem hiding this comment.
fine, material for another PR anyway.
shajmakh
force-pushed
the
replace-map-in-slice
branch
from
4e62f4d to
83e0ba7
Compare
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
shajmakh
force-pushed
the
replace-map-in-slice
branch
from
83e0ba7 to
181a8f6
Compare
Using map to store the daemonset of the targeted MCPs turned out to be a not the best data structure especially when we would like to conserve a consistent order of the elements that aligns with the rest of the slices in the operator status. The problem with map is that it cannot be guaranteed that the elements will be ordered in the same order they were placed. To fix that, define a new struct of an MCP and its respective created RTE daemonset and replace the map with a slice of this struct which ensures that same data will produce same output every time. ref: https://groups.google.com/g/golang-nuts/c/YfDxpkI34hY/m/4pktJI2ytusJ Signed-off-by: Shereen Haj <[email protected]>
shajmakh
force-pushed
the
replace-map-in-slice
branch
from
181a8f6 to
8c847f8
Compare
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ffromani, shajmakh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Using map to store the daemonset of the targeted MCPs turned out to be a not the best data structure especially when we would like to conserve a consistent order of the elements that aligns with the rest of the slices in the operator status.
The problem with map is that it cannot be guaranteed that the elements will be ordered in the same order they were placed. To fix that, define a new struct of an MCP and its respective created RTE daemonset and replace the map with a slice of this struct which ensures that same data will produce same output every time.
ref: https://groups.google.com/g/golang-nuts/c/YfDxpkI34hY/m/4pktJI2ytusJ