flashers: fix not overwriting mismatching hashes#383
flashers: fix not overwriting mismatching hashes#383NickCao merged 1 commit intojumpstarter-dev:mainfrom
Conversation
WalkthroughThe changes simplify the image transfer process in the Changes
Sequence Diagram(s)sequenceDiagram
participant T as _transfer_bg_thread
participant S as Storage
participant M as _create_metadata_and_json
T->>T: Check operator scheme
alt fs scheme
T->>T: Compute SHA256 of file
else Non-local
T->>T: Use provided known hash
end
T->>S: Compare computed/provided hash with storage hash
alt Hashes match
T->>T: Log "Image already exists" and skip upload
else Hashes differ
T->>S: Log "Overwriting image" and perform upload
end
T->>M: Call metadata creation (pass file_hash)
M-->>T: Return metadata JSON with file_hash
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an issue where an image stored in the filesystem may not be overwritten despite a checksum mismatch by basing the check solely on metadata.
- Removed metadata-based checks and switched to comparing actual file hashes.
- Updated the metadata creation function to store the file hash.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
291-297: Good metadata structure updateThe metadata dictionary now includes the file hash, which provides better tracking and verification capabilities for stored files.
Consider adding a compact comment explaining the significance of storing the hash in metadata for future maintainers.
metadata_dict = { "path": str(src_path), "content_length": metadata.content_length, "etag": metadata.etag, + # Store hash for verification during future operations "hash": file_hash, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.13)
🔇 Additional comments (5)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (5)
241-242: Simplified docstring is more maintainableThe simplified docstring is easier to maintain while still communicating the purpose of the method. Consider adding back minimal parameter documentation to maintain clarity without the verbosity.
247-251: Good implementation of file hash handlingThe code now properly handles both local files (by computing SHA256) and remote files (by using the provided hash). This approach ensures reliable hash verification regardless of the source.
253-262: Effective fix for the hash mismatch issueThis change directly addresses the PR objective by comparing file hashes instead of relying on metadata. Now files with mismatching hashes will be correctly overwritten even if metadata matches.
The logging is also helpful for debugging - it clearly indicates whether a file is being skipped or overwritten based on hash comparison.
266-268: Properly passing file hash to metadata creationThe file hash is now correctly passed to the metadata creation method, ensuring the hash is preserved with the file metadata.
288-288: Appropriate method signature updateThe method signature has been properly updated to include the optional file_hash parameter with a sensible default value of None.
bennyz
force-pushed
the
fix-storage-write
branch
from
18a962d to
178696e
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an issue where images are not overwritten when checksum mismatches occur by refactoring the transfer logic and enhancing metadata generation. Key changes include:
- Removal of redundant metadata checks and streamlining of the image transfer process.
- Incorporation of computed file hashes in both storage verification and metadata generation.
- Update of the _create_metadata_and_json method to accept an optional file_hash parameter.
Comments suppressed due to low confidence (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:288
- [nitpick] Consider updating the function docstring to include documentation for the new file_hash parameter to clearly reflect its purpose.
def _create_metadata_and_json(self, src_operator, src_path, file_hash=None) -> tuple[Metadata, str]:
Currently, despite mismatch in hash, the existing file will not be over-written because the metadata matches. However, file name and sizes are likely to be the same because of automotive-image-builder defaults. Since metadata check is unreliable, if there is a checksum mismatch overwrite the image. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
fix-storage-write
branch
from
178696e to
2b7f9b3
Compare
Currently, despite mismatch in hash, the existing file will not be over-written because the metadata matches. However, file name and sizes are likely to be the same because of automotive-image-builder defaults.
Since metadata check is unreliable, if there is a checksum mismatch overwrite the image.
Summary by CodeRabbit