Check image checksum when uploading or downloading images - #239
Check image checksum when uploading or downloading images#239weiiwang01 wants to merge 3 commits into
Conversation
yhaliaw
left a comment
yhaliaw
left a comment
There was a problem hiding this comment.
Major changes needed
🤝 Human review with AI assistance.
| @@ -96,6 +109,66 @@ def upload_image( | |||
| except openstack.exceptions.OpenStackCloudException as exc: | |||
| logger.exception("Error while uploading image.") | |||
| raise UploadImageError from exc | |||
| finally: | |||
| # The temporary image is renamed on success, meaning this only has an effect if the | |||
| # upload did not complete. | |||
| _delete_images_by_name_quietly(connection=connection, image_name=tmp_image_name) | |||
|
|
|||
|
|
|||
| def _delete_images_by_name(connection: openstack.connection.Connection, image_name: str) -> None: | |||
| """Delete every image matching the given name. | |||
|
|
|||
| Args: | |||
| connection: The connected openstack cloud instance. | |||
| image_name: The exact image name to delete. | |||
| """ | |||
| for image in connection.image.images(name=image_name): | |||
| logger.info("Deleting image %s %s.", image_name, image.id) | |||
| connection.delete_image(image.id, wait=True) | |||
There was a problem hiding this comment.
The new <image_name>-tmp name is deterministic and gets unconditionally deleted-by-name both at the start of upload_image (line 85) and again in the finally cleanup (line 115), with no locking.
image_name is derived only from resource_prefix (charm app name) + base + arch, not per-unit — every unit runs its own cron, and _on_run/_on_run_action call self._run() unconditionally with no "upload already in progress" guard (unlike _on_image_relation_changed, which checks has_any_images). If two units' builds overlap, or a manual run action overlaps a running cron build, process B's startup delete-by-name can remove process A's in-flight tmp image mid-upload (create_image(..., wait=True) can take minutes). A's create_image then fails and its own finally deletes-by-name again, potentially wiping B's freshly created tmp image — this can cascade into both builds repeatedly clobbering each other.
Consider making the delete-then-create sequence race-safe: track and delete only the image ID this process created rather than deleting by name, or make the tmp name unique per invocation (uuid/pid). At minimum, extending the existing "already in progress" guard from _on_image_relation_changed to _on_run/_on_run_action would close the concurrent-trigger path. Note the existing test_upload_image_deletes_leftover_tmp_image test only covers the stale-leftover case, not concurrency.
🤖 AI-assisted
3 participants
Applicable spec:
Overview
Image checksums are now verified during the entire process of image creation, to ensure that the image content is unaltered when an error happens.
Rationale
Juju Events Changes
Module Changes
Library Changes
Checklist
urgent,trivial,senior-review-required,documentation)app/pyproject.toml