Add option to build Kolla container images by service groups instead of pattern matching#2206
Add option to build Kolla container images by service groups instead of pattern matching#2206seunghun1ee wants to merge 5 commits intostackhpc/2025.1from
Conversation
This command print list of images used by Kolla services
There was a problem hiding this comment.
Code Review
This pull request introduces a new get-service-images command to the kolla-images.py script, allowing users to retrieve a list of Kolla container images based on service groups. The implementation involves refactoring the existing check_hierarchy function for better code reuse. My review focuses on improving type safety and enhancing the readability of the new logic. I've suggested using more precise type hints and refactoring the main logic to be more concise and easier to understand.
| def get_hierarchy(kolla_ansible_path: str) -> yaml: | ||
| """Return the tag variable hierarchy against Kolla Ansible variables""" |
There was a problem hiding this comment.
The return type hint yaml is incorrect as yaml is a module, not a type. This function returns a dictionary, so Dict[str, str] would be the correct type hint. Also, I've added a period at the end of the docstring for consistency with others in the file.
| def get_hierarchy(kolla_ansible_path: str) -> yaml: | |
| """Return the tag variable hierarchy against Kolla Ansible variables""" | |
| def get_hierarchy(kolla_ansible_path: str) -> Dict[str, str]: | |
| """Return the tag variable hierarchy against Kolla Ansible variables.""" |
| def get_service_images(kolla_ansible_path: str, services: str): | ||
| """Get space separated list of images used by selected services in Kolla Ansible""" | ||
| hierarchy = get_hierarchy(kolla_ansible_path) | ||
| services_list = [] | ||
| is_filtered = False | ||
| if services: | ||
| services_list = services.split(" ") | ||
| is_filtered = True | ||
| images_list = [] | ||
| child_re = re.compile(r"^([a-z0-9_]+)_tag$") | ||
| parent_re = re.compile(r"{{[\s]*([a-z0-9_]+)_tag[\s]*}}") | ||
| parents_no_child_set = set() | ||
| for child, parent in hierarchy.items(): | ||
| child_name = child_re.match(child).group(1) | ||
| parent_name = parent_re.match(parent).group(1) | ||
| # This is parent | ||
| if parent_name == "openstack": | ||
| # And part of the query or no services specified | ||
| if is_filtered and child_name in services_list or not is_filtered: | ||
| parents_no_child_set.add(child_name) # Add to parent list | ||
| continue # Then move on | ||
| # This service is not part of the query | ||
| if is_filtered and parent_name not in services_list: | ||
| continue # ignore | ||
| # Child found | ||
| if parent_name in parents_no_child_set: | ||
| parents_no_child_set.discard(parent_name) # Remove parent that has child | ||
| images_list.append(child_name) # Add the child to the list | ||
| # Add parent with no child | ||
| images_list += list(parents_no_child_set) | ||
| # NOTE(seunghun1ee): Currently K-A has inconsistency on mariadb tag on 2025.1 release | ||
| # Adding manually | ||
| if is_filtered and "mariadb" in services_list or not is_filtered: | ||
| images_list.append("mariadb") | ||
| images_str = " ".join(images_list).replace("_", "-") | ||
| print(images_str) |
There was a problem hiding this comment.
This function can be simplified for better readability and maintainability:
- The
servicesargument can beNone, so its type hint should beOptional[str]. - The
is_filteredboolean flag can be removed by checkingservices_listdirectly, which simplifies the conditional logic.
Here is a suggested refactoring that applies these improvements:
def get_service_images(kolla_ansible_path: str, services: Optional[str]):
"""Get space separated list of images used by selected services in Kolla Ansible"""
hierarchy = get_hierarchy(kolla_ansible_path)
services_list = services.split(" ") if services else []
images_list = []
child_re = re.compile(r"^([a-z0-9_]+)_tag$")
parent_re = re.compile(r"{{[\s]*([a-z0-9_]+)_tag[\s]*}}")
parents_no_child_set = set()
for child, parent in hierarchy.items():
child_name = child_re.match(child).group(1)
parent_name = parent_re.match(parent).group(1)
# This is parent
if parent_name == "openstack":
# And part of the query or no services specified
if not services_list or child_name in services_list:
parents_no_child_set.add(child_name) # Add to parent list
continue # Then move on
# This service is not part of the query
if services_list and parent_name not in services_list:
continue # ignore
# Child found
if parent_name in parents_no_child_set:
parents_no_child_set.discard(parent_name) # Remove parent that has child
images_list.append(child_name) # Add the child to the list
# Add parent with no child
images_list.extend(parents_no_child_set)
# NOTE(seunghun1ee): Currently K-A has inconsistency on mariadb tag on 2025.1 release
# Adding manually
if not services_list or "mariadb" in services_list:
images_list.append("mariadb")
images_str = " ".join(images_list).replace("_", "-")
print(images_str)
Currently we build Kolla container images with regex filter which will limit the image getting built with pattern matching.
This brings the problem of Kolla building images from unrelated service or not building related images because of image naming.