feat: Support this role in container builds #245
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
richm
requested review from
baude,
ikke-t,
rhatdan and
vrothberg
as code owners
Reviewer's GuideThis PR enables the role to distinguish between build-time (container) and runtime contexts by detecting if systemd is booted, and then branches tasks accordingly. It extracts image pull and cache logic into standalone tasks and a new Ansible module, refactors cleanup operations to defer during build-time, and updates the test suite to validate both scenarios. Sequence diagram for image handling during build-time vs runtimesequenceDiagram
participant Role
participant System
participant "Podman Image Module"
participant "Image Cache Module"
participant "Systemd"
Note over Role,System: Detect if systemd is booted
Role->>System: Run systemctl is-system-running
alt Systemd is booted (runtime)
Role->>"Podman Image Module": Pull images with podman_image
"Podman Image Module"->>System: Store images in containers-storage
else Systemd is not booted (build-time)
Role->>"Image Cache Module": Cache images with manage_image_cache
"Image Cache Module"->>System: Store images in cache directory
Role->>System: Create script and systemd unit for later import
end
Class diagram for new manage_image_cache Ansible moduleclassDiagram
class manage_image_cache {
+images: list~str~
+username: str
+password: str
+validate_certs: bool
+cache_dir: str
+preserve_digests: bool
+state: str
+run_skopeo_copy()
+get_image_cache_path()
+is_image_cached()
+update_mapping_file()
+remove_from_mapping_file()
+remove_cached_image()
+run_module()
}
manage_image_cache --> AnsibleModule
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `library/manage_image_cache.py:166-171` </location>
<code_context>
+from ansible.module_utils.basic import AnsibleModule
+
+
+def run_skopeo_copy(
+ module,
+ src_image,
+ dest_dir,
+ username=None,
+ password=None,
+ validate_certs=True,
+ preserve_digests=True,
+):
+ """Run skopeo copy command to cache an image"""
+
+ cmd = ["skopeo", "copy"]
+
+ if preserve_digests:
+ cmd.append("--preserve-digests")
+
+ # Add credentials if provided
+ if username:
+ if password:
+ cmd.extend(["--src-creds", f"{username}:{password}"])
+ else:
+ cmd.extend(["--src-creds", username])
+
+ # Add certificate validation option
+ if validate_certs is not None:
+ if validate_certs:
+ cmd.append("--src-tls-verify")
</code_context>
<issue_to_address>
**suggestion:** TLS verification flag handling may be inconsistent with skopeo expectations.
Always pass '--src-tls-verify' with an explicit boolean value (true/false) to match skopeo's expected usage and prevent ambiguous behavior.
```suggestion
# Always pass --src-tls-verify with explicit boolean value
cmd.append(f"--src-tls-verify={'true' if validate_certs else 'false'}")
```
</issue_to_address>
### Comment 2
<location> `library/manage_image_cache.py:200-202` </location>
<code_context>
+ )
+
+
+def has_mapping_entry(mapping_file, mapping_entry):
+ with open(mapping_file, "r") as ff:
+ return mapping_entry.strip() in ff.readlines()
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Mapping entry check may fail due to newline handling.
Since ff.readlines() includes newlines in each line, comparing with a stripped mapping_entry may not work as intended. Normalize both mapping_entry and file lines by stripping whitespace before comparison.
```suggestion
def has_mapping_entry(mapping_file, mapping_entry):
with open(mapping_file, "r") as ff:
file_lines = [line.strip() for line in ff.readlines()]
return mapping_entry.strip() in file_lines
```
</issue_to_address>
### Comment 3
<location> `library/manage_image_cache.py:191-197` </location>
<code_context>
+ return os.path.join(cache_dir, image_hash)
+
+
+def is_image_cached(cache_path):
+ """Check if an image is already cached"""
+ return (
+ os.path.exists(cache_path)
+ and os.path.isdir(cache_path)
</code_context>
<issue_to_address>
**suggestion:** Image cache existence check may be too permissive.
Consider verifying the presence of expected files or a manifest in the cache directory to ensure image integrity and completeness.
```suggestion
def is_image_cached(cache_path):
"""Check if an image is already cached and complete (manifest present)"""
manifest_path = os.path.join(cache_path, "manifest.json")
return (
os.path.exists(cache_path)
and os.path.isdir(cache_path)
and os.path.exists(manifest_path)
)
```
</issue_to_address>
### Comment 4
<location> `tasks/handle_image_cache.yml:34-36` </location>
<code_context>
- ternary(omit, __podman_validate_certs) }}"
- register: __podman_image_updated
- when: __podman_pull_image | bool
- until: __podman_image_updated is success
- retries: "{{ podman_pull_retry | ternary(3, 0) }}"
- failed_when:
- - __podman_image_updated is failed
- - not __podman_continue_if_pull_fails
</code_context>
<issue_to_address>
**suggestion:** Retry logic may not handle partial failures in image list.
Currently, the logic treats the batch as a single success or failure, which may mask individual image errors. Please update the handling to check each image's result and retry or report failures accordingly.
Suggested implementation:
```
- name: Cache container images for bootc (physically bound images) - per image
vars:
podman_image_pull_results: []
podman_image_pull_failures: []
block:
- name: Pull each image and retry on failure
vars:
image_pull_retries: "{{ podman_pull_retry | ternary(3, 0) }}"
loop: "{{ __podman_images | unique | list }}"
loop_control:
loop_var: podman_image
until: podman_image_pull_result is success or image_pull_retries == 0
retries: "{{ podman_pull_retry | ternary(3, 0) }}"
register: podman_image_pull_result
no_log: true
manage_image_cache:
images: "{{ [podman_image] }}"
username: "{{ __podman_registry_username
if __podman_registry_username | length > 0 else omit }}"
password: "{{ __podman_registry_password
if __podman_registry_password | length > 0 else omit }}"
validate_certs: "{{ __podman_validate_certs
if __podman_validate_certs is not none else omit }}"
cache_dir: "{{ __podman_image_cache_dir }}"
- name: Collect failed image pulls
set_fact:
podman_image_pull_failures: >-
{{
podman_image_pull_failures +
[item.item] if (item.failed or item is failed) else podman_image_pull_failures
}}
loop: "{{ podman_image_pull_result.results | default([]) }}"
loop_control:
label: "{{ item.item }}"
- name: Fail if any image pulls failed and not allowed to continue
fail:
msg: "Failed to cache images: {{ podman_image_pull_failures }}"
when:
- podman_image_pull_failures | length > 0
- not __podman_continue_if_pull_fails
=======
```
- If `manage_image_cache` is a custom module, ensure it supports single-image operation.
- You may need to adjust variable names and result parsing to match your module's output format.
- Remove or update any references to `__podman_image_updated` elsewhere in the playbook.
</issue_to_address>
### Comment 5
<location> `templates/lsr_podman_copy_images.sh.j2:16` </location>
<code_context>
+ skopeo copy --preserve-digests "dir:$cache_dir/$sha" "containers-storage:$image"
+done < "$mapping_file"
+podman images --all --digests --no-trunc || :
+rm -rf "$cache_dir"
</code_context>
<issue_to_address>
**issue (bug_risk):** Unconditional cache directory removal may cause race conditions or data loss.
Removing the cache directory without checks may disrupt other processes or cause data loss if the script fails. Please verify the cache is unused before deletion, or make removal optional.
</issue_to_address>
### Comment 6
<location> `tasks/handle_images.yml:10-11` </location>
<code_context>
- if __podman_registry_username | length > 0 else omit }}"
- password: "{{ __podman_registry_password
- if __podman_registry_password | length > 0 else omit }}"
- validate_certs: "{{ (__podman_validate_certs in ['', none]) |
- ternary(omit, __podman_validate_certs) }}"
- register: __podman_image_updated
</code_context>
<issue_to_address>
**suggestion:** validate_certs handling may not cover all falsy values.
The current check only accounts for '' and none; consider updating it to handle all possible falsy values, such as False and 0, to avoid unexpected behavior.
```suggestion
validate_certs: "{{ (__podman_validate_certs | bool) | ternary(__podman_validate_certs, omit) }}"
```
</issue_to_address>
### Comment 7
<location> `library/manage_image_cache.py:213-215` </location>
<code_context>
if os.path.exists(mapping_file):
if has_mapping_entry(mapping_file, mapping_entry):
return True, None
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))
```suggestion
if os.path.exists(mapping_file) and has_mapping_entry(mapping_file, mapping_entry):
return True, None
```
<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 8
<location> `library/manage_image_cache.py:178-181` </location>
<code_context>
def run_skopeo_copy(
module,
src_image,
dest_dir,
username=None,
password=None,
validate_certs=True,
preserve_digests=True,
):
"""Run skopeo copy command to cache an image"""
cmd = ["skopeo", "copy"]
if preserve_digests:
cmd.append("--preserve-digests")
# Add credentials if provided
if username:
if password:
cmd.extend(["--src-creds", f"{username}:{password}"])
else:
cmd.extend(["--src-creds", username])
# Add certificate validation option
if validate_certs is not None:
if validate_certs:
cmd.append("--src-tls-verify")
else:
cmd.append("--src-tls-verify=false")
# Add source and destination
cmd.extend([f"docker://{src_image}", f"dir:{dest_dir}"])
rc, stdout, stderr = module.run_command(cmd)
if rc == 0:
return True, stdout, None
else:
return False, stdout, stderr
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
```suggestion
return (True, stdout, None) if rc == 0 else (False, stdout, stderr)
```
</issue_to_address>
### Comment 9
<location> `library/manage_image_cache.py:264` </location>
<code_context>
def run_module():
module_args = dict(
images=dict(type="list", elements="str", required=True),
username=dict(type="str", required=False),
password=dict(type="str", required=False, no_log=True),
validate_certs=dict(type="bool", required=False, default=True),
cache_dir=dict(
type="str", required=False, default="/usr/lib/containers-image-cache"
),
preserve_digests=dict(type="bool", required=False, default=True),
state=dict(
type="str", required=False, default="present", choices=["present", "absent"]
),
)
result = dict(changed=False, results=[])
module = AnsibleModule(argument_spec=module_args, supports_check_mode=True)
images = module.params["images"]
username = module.params["username"]
password = module.params["password"]
validate_certs = module.params["validate_certs"]
cache_dir = module.params["cache_dir"]
preserve_digests = module.params["preserve_digests"]
state = module.params["state"]
overall_changed = False
overall_failed = False
for image in images:
image_result = {
"image": image,
"changed": False,
"skipped": False,
"failed": False,
"msg": "",
}
try:
cache_path = get_image_cache_path(cache_dir, image)
image_sha = os.path.basename(cache_path) # Extract SHA from cache path
if state == "present":
# Check if image is already cached
if is_image_cached(cache_path):
image_result["skipped"] = True
image_result["msg"] = (
f"Image {image} already cached at {cache_path}"
)
else:
if module.check_mode:
image_result["changed"] = True
image_result["msg"] = (
f"Would cache image {image} to {cache_path}"
)
else:
# Copy image to cache
success, stdout, stderr = run_skopeo_copy(
module,
image,
cache_path,
username,
password,
validate_certs,
preserve_digests,
)
image_result["stdout"] = stdout
if success:
image_result["changed"] = True
image_result["cache_path"] = cache_path
image_result["msg"] = (
f"Successfully cached image {image} to {cache_path}"
)
overall_changed = True
# Update mapping file with image name and SHA
mapping_success, mapping_error = update_mapping_file(
cache_dir, image, image_sha
)
if not mapping_success:
module.warn(
f"Failed to update mapping file: {mapping_error}"
)
else:
image_result["failed"] = True
error_msg = f"Failed to cache image {image}"
if stderr:
error_msg += f": {stderr}"
image_result["msg"] = error_msg
overall_failed = True
elif state == "absent":
# Check if image is cached and remove it
if is_image_cached(cache_path):
if module.check_mode:
image_result["changed"] = True
image_result["msg"] = (
f"Would remove cached image {image} from {cache_path}"
)
else:
# Remove cached image
remove_success, remove_error = remove_cached_image(cache_path)
if remove_success:
image_result["changed"] = True
image_result["msg"] = (
f"Successfully removed cached image {image} from {cache_path}"
)
overall_changed = True
# Remove from mapping file using image name and SHA
mapping_success, mapping_error = remove_from_mapping_file(
cache_dir, image, image_sha
)
if not mapping_success:
module.warn(
f"Failed to update mapping file: {mapping_error}"
)
else:
image_result["failed"] = True
image_result["msg"] = (
f"Failed to remove cached image {image}: {remove_error}"
)
overall_failed = True
else:
image_result["skipped"] = True
image_result["msg"] = f"Image {image} not cached at {cache_path}"
except Exception as e:
image_result["failed"] = True
image_result["msg"] = f"Error processing image {image}: {str(e)}"
overall_failed = True
result["results"].append(image_result)
result["changed"] = overall_changed
result["failed"] = overall_failed
module.exit_json(**result)
</code_context>
<issue_to_address>
**issue (code-quality):** Low code quality found in run\_module - 7% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
<br/><details><summary>Explanation</summary>The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+34
to
+36
| until: __podman_image_updated is success | ||
| retries: "{{ podman_pull_retry | ternary(3, 0) }}" | ||
| failed_when: |
There was a problem hiding this comment.
suggestion: Retry logic may not handle partial failures in image list.
Currently, the logic treats the batch as a single success or failure, which may mask individual image errors. Please update the handling to check each image's result and retry or report failures accordingly.
Suggested implementation:
- name: Cache container images for bootc (physically bound images) - per image
vars:
podman_image_pull_results: []
podman_image_pull_failures: []
block:
- name: Pull each image and retry on failure
vars:
image_pull_retries: "{{ podman_pull_retry | ternary(3, 0) }}"
loop: "{{ __podman_images | unique | list }}"
loop_control:
loop_var: podman_image
until: podman_image_pull_result is success or image_pull_retries == 0
retries: "{{ podman_pull_retry | ternary(3, 0) }}"
register: podman_image_pull_result
no_log: true
manage_image_cache:
images: "{{ [podman_image] }}"
username: "{{ __podman_registry_username
if __podman_registry_username | length > 0 else omit }}"
password: "{{ __podman_registry_password
if __podman_registry_password | length > 0 else omit }}"
validate_certs: "{{ __podman_validate_certs
if __podman_validate_certs is not none else omit }}"
cache_dir: "{{ __podman_image_cache_dir }}"
- name: Collect failed image pulls
set_fact:
podman_image_pull_failures: >-
{{
podman_image_pull_failures +
[item.item] if (item.failed or item is failed) else podman_image_pull_failures
}}
loop: "{{ podman_image_pull_result.results | default([]) }}"
loop_control:
label: "{{ item.item }}"
- name: Fail if any image pulls failed and not allowed to continue
fail:
msg: "Failed to cache images: {{ podman_image_pull_failures }}"
when:
- podman_image_pull_failures | length > 0
- not __podman_continue_if_pull_fails
=======
- If
manage_image_cacheis a custom module, ensure it supports single-image operation. - You may need to adjust variable names and result parsing to match your module's output format.
- Remove or update any references to
__podman_image_updatedelsewhere in the playbook.
Comment on lines
+178
to
+180
| if rc == 0: | ||
| return True, stdout, None | ||
| else: | ||
| return False, stdout, stderr |
There was a problem hiding this comment.
suggestion (code-quality): Replace if statement with if expression (assign-if-exp)
Suggested change
| if rc == 0: | |
| return True, stdout, None | |
| else: | |
| return False, stdout, stderr | |
| return (True, stdout, None) if rc == 0 else (False, stdout, stderr) |
| return False, str(e) | ||
|
|
||
|
|
||
| def run_module(): |
There was a problem hiding this comment.
issue (code-quality): Low code quality found in run_module - 7% (low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
richm
force-pushed
the
bootc
branch
9 times, most recently
from
fdea749 to
69e1fbe
Compare
richm
added a commit
to richm/lsr_.github
that referenced
this pull request
Sep 19, 2025
and fix ansible-lint for podman needed for podman bootc support PR linux-system-roles/podman#245
richm
added a commit
to linux-system-roles/.github
that referenced
this pull request
Sep 19, 2025
and fix ansible-lint for podman needed for podman bootc support PR linux-system-roles/podman#245
Member
|
I don't have the cycles (or competences) to review this PR but I want to share my excitement about this feature. Thanks a ton for enabling this use case 🙏 |
Contributor
Author
|
[citest] |
Feature: Support running this role during container builds. Reason: This is particularly useful for building bootc derivative OSes. Result: User can configure the image at build time. Signed-off-by: Rich Megginson <rmeggins@redhat.com>
Contributor
Author
|
[citest] |
Contributor
Author
|
The issue with centos-10-bootc looks like some sort of issue with bootc on centos-10 - all other bootc platforms work fine |
spetrosi
reviewed
Sep 24, 2025
| @@ -0,0 +1,412 @@ | |||
| #!/usr/bin/python | |||
Contributor
There was a problem hiding this comment.
You add this module only here or it will be used somewhere else? Do we need to cover it with unit tests?
Contributor
Author
There was a problem hiding this comment.
Only here. This is really just a small helper to do the skopeo stuff. I could have done it with command or shell - but doing it in python makes formatting the output much easier. I don't think we need unit tests for this.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Feature: Support running this role during container builds.
Reason: This is particularly useful for building bootc derivative OSes.
Result: User can configure the image at build time.
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by Sourcery
Enable the role to run during container build contexts by detecting whether systemd is available and deferring runtime operations accordingly. Extract image pull and caching logic into dedicated tasks and a new custom module, and update existing tasks to only run when the system is booted. Extend tests to cover buildah-based bootc scenarios and validate caching and secret injection.
New Features:
Enhancements:
Tests: