[k8s] Fix list merging in merge_k8s_configs
#4762
Conversation
sky/utils/config_utils.py
Outdated
| # For other list values, merge lists and maintain uniqueness. | ||
| # Preserve the order - first list is the base_config list, and | ||
| # the second list is the override list. This is required for | ||
| # order sensitive lists like allowed_contexts. | ||
| seen = set(base_config[key]) | ||
| # Append new items from override in-place while preserving order | ||
| for item in value: | ||
| if item not in seen: | ||
| seen.add(item) | ||
| base_config[key].append(item) |
There was a problem hiding this comment.
Would there a case that same values are expected in the list?
There was a problem hiding this comment.
No such cases that I could think of - most list fields in k8s are designed to hold unique items (e.g.,, container definitions, imagePullSecrets, volume names, etc.)
There was a problem hiding this comment.
IIRC, some list like commands and args should be considered as atomic value
sky/utils/config_utils.py
Outdated
| # Preserve the order - first list is the base_config list, and | ||
| # the second list is the override list. This is required for | ||
| # order sensitive lists like allowed_contexts. | ||
| seen = set(base_config[key]) |
There was a problem hiding this comment.
Is it possible that the list items are dicts (unhashable)?
There was a problem hiding this comment.
oh yeah good point - I was thinking they'd be covered in the special casing above (e.g., volumes), but realized there may be other keys and it's good to be general. Simplified the logic here to also work with dicts.
There was a problem hiding this comment.
Also added relevant test cases.
| # new item not already present. | ||
| result = list(base_config[key]) | ||
| for item in value: | ||
| if item not in result: |
There was a problem hiding this comment.
A list might be treated as map in kubernetes (https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy), in this case the equality check should be based on the listMapKey.
Repro: a config.yaml with a single allowed_context would be read as multiple contexts:
Our k8s dict merge logic would extend lists instead of adding only unique elements. This PR fixes it.