fix: reorder variable merging logic in GetAllVariable for clarity and…#3094
Conversation
… priority Signed-off-by: redscholar <blacktiledhouse@gmail.com>
kubesphere-prow
Bot
added
release-note-none
kind/bug
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: redscholar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubesphere-prow
Bot
added
approved
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request reorders the variable merging logic within the getHostsVariable function to establish a specific priority hierarchy, moving remote and runtime variables to the lowest priority. Feedback focuses on performance optimizations, specifically recommending that constant inventory and configuration variables be pre-calculated outside the host loop to avoid redundant processing. Additionally, the reviewer noted a potential non-determinism issue when merging group variables due to Go's map iteration behavior, suggesting that group names should be sorted first.
| hostVars = CombineVariables(hostVars, v.value.Hosts[hostname].RemoteVars) | ||
| // 2. Merge runtime variables | ||
| hostVars = CombineVariables(hostVars, v.value.Hosts[hostname].RuntimeVars) | ||
| // 3. Merge inventory-level variables (vars) |
There was a problem hiding this comment.
The variable merging logic in the following line is inefficient because Extension2Variables (which performs JSON unmarshaling) is called for every host even though the inventory variables are constant. To improve performance, especially with large inventories, pre-calculate this variable map once outside the loop.
| // 3. Merge inventory-level variables (vars) | ||
| hostVars = CombineVariables(hostVars, Extension2Variables(v.value.Inventory.Spec.Vars)) | ||
| // Set group variables for hosts that belong to groups (override spec.vars) | ||
| // 4. Merge group variables (groups > vars) |
There was a problem hiding this comment.
The group merging logic that follows has two issues:
- Non-determinism: Iterating over the groups map is non-deterministic in Go. If a host belongs to multiple groups with conflicting variables, the result will be inconsistent across different executions. Consider sorting the group names before merging.
- Efficiency:
Extension2Variablesis called repeatedly for each host. Pre-calculating these maps outside the loop would significantly improve performance.
| // 5. Merge host-specific variables from inventory (host > groups) | ||
| hostVars = CombineVariables(hostVars, Extension2Variables(v.value.Inventory.Spec.Hosts[hostname])) | ||
| // Merge configuration variables | ||
| // 6. Merge configuration variables (highest priority) |
There was a problem hiding this comment.
Similar to the inventory variables, the configuration variables in the following line should be pre-calculated outside the loop to avoid redundant JSON unmarshaling for every host.
1 participant
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fix variable priority: config > inventory(host > groups > vars) > runtime > remote
Which issue(s) this PR fixes:
Fixes #
Special notes for reviewers:
Does this PR introduced a user-facing change?
Additional documentation, usage docs, etc.: