feat: add Slurm image registry foundation - #891
Open
nabinchha wants to merge 7 commits into
Open
Conversation
Add digest-bound client and serving inspection, atomic workspace-derived registry persistence, and verified alias/path resolution.\n\nPart of #867 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Exercise the production environment reader and verify restrictive atomic registry persistence discovered during review.\n\nPart of #867 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Persist the package-owned image registry as deterministic safe YAML at the migration plan's canonical path and declare the runtime YAML dependency explicitly.\n\nPart of #867. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Reject symlinked registry and lock files, use explicit UTF-8 persistence, and normalize malformed byte content into the package image error hierarchy.\n\nPart of #867. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Exercise the public image service from an isolated installed wheel and assert that the wheel metadata carries the runtime PyYAML dependency.\n\nPart of #867. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Treat only a genuinely missing registry as empty and reject non-regular registry or lock nodes before reading, locking, or changing permissions.\n\nPart of #867. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Contributor
Greptile SummaryAdds the foundational Slurm image registry, including digest-bound inspection, immutable records, atomic workspace persistence, and verified alias or path resolution.
|
| Filename | Overview |
|---|---|
| packages/data-designer-slurm/src/data_designer/slurm/images/service.py | Implements digest-bound registration and resolution, including final verification that addresses the previously reported stale-facts race. |
| packages/data-designer-slurm/src/data_designer/slurm/images/registry.py | Adds serialized, atomic registry mutation with rollback while both alias and global mutation locks remain held. |
| packages/data-designer-slurm/src/data_designer/slurm/images/inspection.py | Adds injectable client and serving environment inspectors that produce normalized inspection records. |
| packages/data-designer-slurm/src/data_designer/slurm/images/records.py | Defines immutable registry records and validates alias ordering, uniqueness, and consistent path-bound facts. |
| packages/data-designer-slurm/tests/images/test_registry.py | Covers registration, replacement rollback, resolution verification, concurrency, and filesystem boundaries. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Service as SlurmImageService
participant Registry as ImageRegistry
participant Disk as Registry/SQSH filesystem
Caller->>Service: register_existing(request, inspection)
Service->>Disk: Hash SQSH
Service->>Service: Verify digest and image kind
Service->>Registry: register(image)
Registry->>Registry: Acquire alias and registry locks
Registry->>Disk: Atomically persist updated snapshot
Registry->>Disk: Re-hash persisted image path
alt Verification succeeds
Registry-->>Service: RegisteredImage
else Verification fails
Registry->>Disk: Restore prior snapshot
Registry-->>Service: ImageVerificationError
end
Reviews (2): Last reviewed commit: "fix: rollback stale image registrations" | Re-trigger Greptile
| path = Path(request.source) | ||
| sqsh_sha256 = compute_file_sha256(path) | ||
| _validate_inspection(inspection, expected_kind=ImageKind(request.kind), expected_sha256=sqsh_sha256) | ||
| image = RegisteredImage( |
Contributor
There was a problem hiding this comment.
Registration Persists Stale Image Facts
If another process replaces or modifies the SQSH after compute_file_sha256 returns, registration persists the old digest and inspection without rechecking the file, causing the newly registered image to fail resolution immediately.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-slurm/src/data_designer/slurm/images/service.py
Line: 47
Comment:
**Registration Persists Stale Image Facts**
If another process replaces or modifies the SQSH after `compute_file_sha256` returns, registration persists the old digest and inspection without rechecking the file, causing the newly registered image to fail resolution immediately.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reverify image bytes while registry mutation locks remain held and restore the previous snapshot if final verification fails. Part of #867 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
📋 Summary
Adds the first independently reviewable slice of #867: package-owned digest-bound client and serving inspection, immutable registry records, atomic workspace-derived YAML persistence, and verified alias/path resolution. OCI import, artifact publication, and compatibility hardening remain in the later scoped slices.
🔗 Related Issue
Part of #867
🔄 Changes
registry.yamlpersistence under the selected workspace.🧪 Testing
✅ Checklist