OCPNODE-3811: Migrate artifact implementation#9621
Conversation
|
Skipping CI for Draft Pull Request. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9621 +/- ##
==========================================
- Coverage 67.08% 66.84% -0.25%
==========================================
Files 208 208
Lines 29001 28879 -122
==========================================
- Hits 19455 19303 -152
- Misses 7889 7916 +27
- Partials 1657 1660 +3 🚀 New features to boost your workflow:
|
|
/test all |
|
@bitoku: This pull request references OCPNODE-3811 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThis pull request refactors the artifact handling subsystem to integrate with the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
323ea58 to
5563f9e
Compare
Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
internal/storage/image_test.go (1)
595-595: GraphRoot expectations correctly validate new dependency on a concrete graph rootUpdating the mocks to return
graphRootfromstoreMock.EXPECT().GraphRoot()in the variousPullImagefailure-path tests looks right and aligns the tests with the new implementation that relies onGraphRoot(). This also usefully encodes that these code paths must consult the graph root (while the “invalid policy path” case intentionally does not).If you want to reduce duplication later, you could factor a small helper inside this
Describeto register the expectation in one place:t.Describe("PullImage", func() { - var graphRoot string - BeforeEach(func() { - graphRoot = t.MustTempDir("ociartifact") - }) + var graphRoot string + + expectGraphRoot := func() { + mockutils.InOrder( + storeMock.EXPECT().GraphRoot().Return(graphRoot), + ) + } + + BeforeEach(func() { + graphRoot = t.MustTempDir("ociartifact") + }) @@ It("should fail on copy image", func() { // Given - mockutils.InOrder( - storeMock.EXPECT().GraphRoot().Return(graphRoot), - ) + expectGraphRoot() @@ It("should fail on canonical copy image", func() { // Given - mockutils.InOrder( - storeMock.EXPECT().GraphRoot().Return(graphRoot), - ) + expectGraphRoot() @@ It("should fail on cancelled context", func() { // Given - mockutils.InOrder( - storeMock.EXPECT().GraphRoot().Return(graphRoot), - ) + expectGraphRoot() @@ It("should fail on timed out context", func() { // Given - mockutils.InOrder( - storeMock.EXPECT().GraphRoot().Return(graphRoot), - ) + expectGraphRoot()Purely optional, as the current version is already clear.
Also applies to: 613-613, 631-631, 652-652
internal/storage/image.go (1)
927-936: Error variable shadowing may mask original errors.In the
elsebranch (lines 928, 933), you're using:=which creates newerrvariables that shadow the outererr. While this works, it's confusing because the outererrfromcopy.Image(line 895) is stillnilin this branch. Consider using explicit variable names:} else { - manifestDigest, err := manifest.Digest(manifestBytes) + manifestDigest, digestErr := manifest.Digest(manifestBytes) - if err != nil { - return RegistryImageReference{}, fmt.Errorf("digesting image: %w", err) + if digestErr != nil { + return RegistryImageReference{}, fmt.Errorf("digesting image: %w", digestErr) } - canonicalRef, err = reference.WithDigest(reference.TrimNamed(imageName.Raw()), manifestDigest) - if err != nil { - return RegistryImageReference{}, fmt.Errorf("create canonical reference: %w", err) + canonicalRef, refErr := reference.WithDigest(reference.TrimNamed(imageName.Raw()), manifestDigest) + if refErr != nil { + return RegistryImageReference{}, fmt.Errorf("create canonical reference: %w", refErr) } }Alternatively, use
=instead of:=for line 933 sincecanonicalRefis already declared.vendor/go.podman.io/common/pkg/libartifact/types/errors.go (1)
8-8: Typo in error variable name:ErrArtifactUnamedshould beErrArtifactUnnamed.This is vendored code, so the fix should be submitted upstream to
go.podman.io/common. If you maintain that dependency, consider fixing it there and re-vendoring.server/server.go (2)
461-464: Consider wrapping the error with additional context.Per coding guidelines, use
fmt.Errorfwith%wfor error wrapping to provide context about where the error occurred.artifactStore, err := ociartifact.NewStore(containerServer.Store().GraphRoot(), config.SystemContext) if err != nil { - return nil, err + return nil, fmt.Errorf("create artifact store: %w", err) }
1133-1136: Update stale comment.The comment says "returns a new artifact store instance" but the implementation now returns the pre-initialized
s.artifactStore. Update the comment to reflect the actual behavior.-// ArtifactStore returns a new artifact store instance. +// ArtifactStore returns the artifact store instance. func (s *Server) ArtifactStore() *ociartifact.Store { return s.artifactStore }internal/ociartifact/store.go (1)
162-171: Consider logging the removed items or documenting why they're discarded.
s.ArtifactStore.Removereturns([]string, error)but the string slice is discarded. If the removed items aren't needed, consider adding a brief comment explaining why, or log them at debug level for troubleshooting.- _, err = s.ArtifactStore.Remove(ctx, artifact.Reference()) - - return err + removed, err := s.ArtifactStore.Remove(ctx, artifact.Reference()) + if err != nil { + return err + } + log.Debugf(ctx, "Removed artifact: %v", removed) + return nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
internal/config/seccomp/seccomp.go(2 hunks)internal/config/seccomp/seccompociartifact/seccompociartifact.go(1 hunks)internal/config/seccomp/seccompociartifact/seccompociartifact_test.go(1 hunks)internal/ociartifact/artifact.go(2 hunks)internal/ociartifact/impl.go(0 hunks)internal/ociartifact/store.go(14 hunks)internal/ociartifact/store_test.go(1 hunks)internal/storage/image.go(1 hunks)internal/storage/image_test.go(5 hunks)server/artifacts_test.go(4 hunks)server/container_create_linux.go(3 hunks)server/container_restore_test.go(2 hunks)server/image_list_test.go(0 hunks)server/image_remove_test.go(0 hunks)server/image_status_test.go(0 hunks)server/server.go(4 hunks)server/server_test.go(1 hunks)server/suite_test.go(1 hunks)test/mocks/ociartifact/ociartifact.go(0 hunks)vendor/go.podman.io/common/pkg/libartifact/artifact.go(1 hunks)vendor/go.podman.io/common/pkg/libartifact/store/config.go(1 hunks)vendor/go.podman.io/common/pkg/libartifact/store/store.go(1 hunks)vendor/go.podman.io/common/pkg/libartifact/types/config.go(1 hunks)vendor/go.podman.io/common/pkg/libartifact/types/errors.go(1 hunks)vendor/modules.txt(1 hunks)
💤 Files with no reviewable changes (5)
- server/image_list_test.go
- server/image_status_test.go
- server/image_remove_test.go
- internal/ociartifact/impl.go
- test/mocks/ociartifact/ociartifact.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use interface-based design and dependency injection patterns in Go code
Propagate context.Context through function calls in Go code
Usefmt.Errorfwith%wfor error wrapping in Go code
Use logrus with structured fields for logging in Go code
Add comments explaining 'why' not 'what' in Go code
Use platform-specific file naming:*_{linux,freebsd}.gofor platform-dependent code
Files:
server/suite_test.gointernal/config/seccomp/seccompociartifact/seccompociartifact_test.gointernal/config/seccomp/seccomp.goserver/container_restore_test.goserver/artifacts_test.gointernal/config/seccomp/seccompociartifact/seccompociartifact.gointernal/ociartifact/store_test.gointernal/storage/image_test.goserver/server.goserver/server_test.govendor/go.podman.io/common/pkg/libartifact/artifact.govendor/go.podman.io/common/pkg/libartifact/store/config.govendor/go.podman.io/common/pkg/libartifact/types/errors.gointernal/storage/image.govendor/go.podman.io/common/pkg/libartifact/types/config.gointernal/ociartifact/artifact.govendor/go.podman.io/common/pkg/libartifact/store/store.goserver/container_create_linux.gointernal/ociartifact/store.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Use
*_test.gonaming convention for unit test files
Files:
server/suite_test.gointernal/config/seccomp/seccompociartifact/seccompociartifact_test.goserver/container_restore_test.goserver/artifacts_test.gointernal/ociartifact/store_test.gointernal/storage/image_test.goserver/server_test.go
🧬 Code graph analysis (14)
internal/config/seccomp/seccompociartifact/seccompociartifact_test.go (1)
internal/config/seccomp/seccompociartifact/seccompociartifact.go (1)
New(23-32)
internal/config/seccomp/seccomp.go (2)
internal/config/seccomp/seccompociartifact/seccompociartifact.go (1)
New(23-32)server/server.go (1)
New(397-615)
server/container_restore_test.go (1)
vendor/github.com/onsi/ginkgo/v2/core_dsl.go (1)
BeforeEach(758-760)
server/artifacts_test.go (1)
vendor/go.podman.io/common/pkg/libartifact/types/config.go (1)
BlobMountPath(45-50)
internal/ociartifact/store_test.go (2)
test/mocks/ociartifact/ociartifact.go (2)
MockImpl(24-28)NewMockImpl(36-40)internal/ociartifact/store.go (1)
NewStore(44-58)
internal/storage/image_test.go (1)
vendor/github.com/onsi/ginkgo/v2/core_dsl.go (1)
BeforeEach(758-760)
server/server.go (1)
internal/ociartifact/store.go (2)
Store(36-41)NewStore(44-58)
server/server_test.go (2)
internal/mockutils/mockutils.go (1)
InOrder(15-44)vendor/go.uber.org/mock/gomock/call.go (1)
InOrder(438-454)
vendor/go.podman.io/common/pkg/libartifact/artifact.go (2)
internal/ociartifact/artifact.go (1)
Artifact(13-18)vendor/go.podman.io/common/pkg/libartifact/types/errors.go (2)
ErrArtifactUnamed(8-8)ErrArtifactNotExist(9-9)
internal/storage/image.go (2)
internal/ociartifact/store.go (1)
NewStore(44-58)internal/storage/references/registry_reference.go (1)
RegistryImageReference(18-22)
vendor/go.podman.io/common/pkg/libartifact/types/config.go (1)
vendor/go.opentelemetry.io/otel/semconv/v1.37.0/attribute_group.go (1)
FileName(5581-5583)
internal/ociartifact/artifact.go (1)
vendor/go.podman.io/common/pkg/libartifact/artifact.go (1)
Artifact(13-18)
vendor/go.podman.io/common/pkg/libartifact/store/store.go (3)
vendor/go.podman.io/common/pkg/libartifact/artifact.go (2)
Artifact(13-18)ArtifactList(56-56)vendor/go.podman.io/common/pkg/libartifact/types/config.go (5)
ArtifactBlob(52-56)AddOptions(10-20)FilterBlobOptions(23-30)BlobMountPathOptions(40-42)BlobMountPath(45-50)vendor/go.podman.io/common/pkg/libartifact/types/errors.go (3)
ErrArtifactAlreadyExists(10-10)ErrArtifactFileExists(11-11)ErrArtifactBlobTitleInvalid(12-12)
server/container_create_linux.go (1)
vendor/go.podman.io/common/pkg/libartifact/types/config.go (1)
BlobMountPath(45-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: rpm-build:fedora-rawhide-x86_64:fedora-rawhide
- GitHub Check: rpm-build:fedora-43-x86_64:fedora-rawhide
- GitHub Check: rpm-build:fedora-rawhide-aarch64:fedora-rawhide
- GitHub Check: rpm-build:centos-stream-9-aarch64:fedora-rawhide
- GitHub Check: rpm-build:centos-stream-9-x86_64:fedora-rawhide
- GitHub Check: rpm-build:fedora-43-aarch64:fedora-rawhide
- GitHub Check: rpm-build:fedora-rawhide-aarch64:fedora-rawhide
- GitHub Check: rpm-build:centos-stream-9-aarch64:fedora-rawhide
- GitHub Check: rpm-build:fedora-rawhide-x86_64:fedora-rawhide
- GitHub Check: rpm-build:fedora-43-x86_64:fedora-rawhide
- GitHub Check: rpm-build:fedora-43-aarch64:fedora-rawhide
- GitHub Check: rpm-build:centos-stream-9-x86_64:fedora-rawhide
- GitHub Check: rpm-build:centos-stream-9-aarch64:fedora-rawhide
- GitHub Check: rpm-build:centos-stream-9-x86_64:fedora-rawhide
- GitHub Check: rpm-build:fedora-43-x86_64:fedora-rawhide
- GitHub Check: rpm-build:fedora-rawhide-x86_64:fedora-rawhide
- GitHub Check: rpm-build:fedora-rawhide-aarch64:fedora-rawhide
- GitHub Check: rpm-build:fedora-43-aarch64:fedora-rawhide
- GitHub Check: rpm-build:fedora-rawhide-aarch64:fedora-rawhide
- GitHub Check: rpm-build:fedora-43-x86_64:fedora-rawhide
- GitHub Check: rpm-build:fedora-rawhide-x86_64:fedora-rawhide
- GitHub Check: rpm-build:fedora-43-aarch64:fedora-rawhide
- GitHub Check: rpm-build:centos-stream-9-x86_64:fedora-rawhide
- GitHub Check: rpm-build:centos-stream-9-aarch64:fedora-rawhide
🔇 Additional comments (28)
server/container_restore_test.go (2)
353-356: LGTM! Proper test isolation with dynamic graph root.The BeforeEach setup correctly creates an isolated temporary directory for each test iteration, replacing the previous hard-coded empty string. This aligns with the libartifact migration and ensures proper test isolation.
483-483: LGTM! Mock updated to use isolated graph root.The mock expectation now correctly returns the dynamically created
graphRootinstead of a hard-coded empty string, ensuring the test verifies behavior with a real, isolated graph root path.internal/storage/image_test.go (1)
572-575: Per-testgraphRootsetup is correct and matches new PullImage behaviorInitializing
graphRootvia a per-testBeforeEachandt.MustTempDir("ociartifact")is a good way to ensurePullImageexercises a non-empty, isolated graph root without affecting other tests. No functional issues spotted here.internal/storage/image.go (2)
904-905: LGTM!Good approach declaring
canonicalRefbefore the branching logic to ensure it's accessible in both paths.
912-924: Code snippet does not match the cri-o repository.The provided code snippet showing
manifestDigestas a pointer todigest.Digestand the dereference at line 921 (*manifestDigest) does not exist in the cri-o codebase. The actualPullManifestfunction signature returns(manifestBytes []byte, err error), not(*digest.Digest, error). The suggested nil check is therefore not applicable to the actual implementation.Likely an incorrect or invalid review comment.
internal/config/seccomp/seccompociartifact/seccompociartifact_test.go (1)
30-41: LGTM!The test correctly adapts to the new constructor signature that returns
(*SeccompOCIArtifact, error). The temporary directory setup and error checking are properly implemented.server/artifacts_test.go (1)
8-8: LGTM!The test correctly migrates from the internal
ociartifact.BlobMountPathtolibartTypes.BlobMountPath. The type aliaslibartTypesis consistent with usage in other files, and the struct fields (Name,SourcePath) are compatible with the new type definition.Also applies to: 18-24, 46-49, 71-74
server/server_test.go (2)
85-85: LGTM! Proper test setup for artifact store initialization.The temporary directory creation follows the test framework's standard pattern and provides the necessary graph root path for the artifact store initialization introduced in this PR.
91-91: LGTM! Mock expectation aligns with new artifact store initialization.The GraphRoot() expectation is correctly positioned in the call sequence and returns the temporary directory created for testing.
vendor/modules.txt (1)
1056-1058: LGTM! Standard vendor module additions.The libartifact modules have been properly added to the vendor manifest, enabling the artifact store migration to the external library.
internal/config/seccomp/seccomp.go (2)
241-244: LGTM! Proper error handling for constructor.The error handling follows Go best practices:
- Uses
%wfor error wrapping to preserve the error chain- Provides meaningful context ("create OCI artifact seccomp profile store")
- Returns appropriate zero values on error
As per coding guidelines, this demonstrates proper error wrapping in Go code.
245-245: LGTM! Clean separation of store creation and usage.The change to use a separate store variable improves readability and aligns with the new constructor signature that returns an error.
server/suite_test.go (1)
230-236: LGTM! Consistent test setup pattern.The test scaffolding correctly creates a temporary graph root directory and configures the mock to return it, consistent with the pattern used in server/server_test.go.
internal/ociartifact/store_test.go (3)
18-18: LGTM! Standard test error pattern.Using a shared test error variable is a common and acceptable pattern in Go tests when the specific error content doesn't matter.
44-61: LGTM! Well-structured error path test.The test follows the Arrange-Act-Assert pattern and properly validates the error handling when ParseNormalizedNamed fails.
63-83: Test structure is sound, but test coverage reduction claims require manual verification.The test properly validates the error path when DockerNewReference fails and follows Go testing conventions. However, verification of the broader claim about removed test coverage (digest validity, blob reads, size limits, manifest handling) could not be completed due to sandbox limitations.
To properly assess regression risk, you should:
- Check git history to confirm which tests were removed from this file
- Verify whether libartifact tests now cover the removed scenarios
- Ensure integration tests provide adequate coverage for the artifact store functionality
vendor/go.podman.io/common/pkg/libartifact/store/config.go (1)
1-41: Vendor code - no review needed.This is vendored code from go.podman.io/common/pkg/libartifact. Vendor code is typically not reviewed as part of PRs unless there are specific integration concerns.
internal/config/seccomp/seccompociartifact/seccompociartifact.go (1)
23-31: LGTM! Proper error-returning constructor pattern.The constructor correctly:
- Propagates errors from ociartifact.NewStore
- Returns both pointer and error following Go conventions
- Uses direct error return (no wrapping needed as context is clear)
As per coding guidelines, this demonstrates proper error propagation in Go code.
server/container_create_linux.go (3)
18-18: LGTM! Clear import alias for libartifact types.The import alias
libartTypesis descriptive and follows Go conventions for managing long package paths.
488-503: LGTM! Consistent type usage throughout the function.The function body correctly uses
libartTypes.BlobMountPathconsistently:
- In the
slices.ContainsFunclambda parameter- In the struct literal construction with
NameandSourcePathfieldsThe field names match the type definition shown in the relevant code snippets.
481-481: Type migration to libartifact library is valid.The change from
ociartifact.BlobMountPathtolibartTypes.BlobMountPathis safe. Both types define the same fields:SourcePath(string) andName(string), which are used consistently throughout this function for mount path filtering.internal/ociartifact/artifact.go (2)
12-18: LGTM - Clean embedding pattern with local digest caching.The embedding of
*libartifact.Artifactproperly delegates size calculation viaTotalSizeBytes(). The localdigestfield appears intentional for caching the computed digest value rather than recalculating it from the manifest on each access.
40-53: LGTM - Size calculation properly delegated.The
CRIImage()method correctly uses the embeddedTotalSizeBytes()fromlibartifact.Artifactinstead of the previous local size calculation.internal/ociartifact/store.go (5)
43-58: LGTM - Constructor properly initializes embedded store.The
NewStorefunction correctly initializes the embeddedArtifactStoreand returns errors appropriately. The error fromlibartStore.NewArtifactStoreis already descriptive from the library.
115-128: LGTM - PullManifest properly delegates to embedded store.The refactored
PullManifestcorrectly delegates to the embeddedArtifactStore.Pulland returns the digest. The error wrapping follows the coding guidelines.
189-213: LGTM - buildArtifact correctly constructs internal Artifact from libartifact.The function properly handles the digest retrieval, empty name case, and parse failures with appropriate logging.
316-321: Acknowledge the TODO for future cleanup.The TODO to replace with
GetByNameOrDigestin libartifact is noted. This temporary implementation is reasonable for the migration phase.Please ensure this TODO is tracked for follow-up. Consider creating an issue to track the replacement once the libartifact version supports the needed functionality.
369-406: LGTM - BlobMountPaths maintains modelpack support.The comment at line 370 clearly explains why this method isn't delegated to c/common (modelpack support). The implementation properly uses
libartTypes.BlobMountPathand handles missing annotations gracefully with warning logs.
|
/retest |
|
@cri-o/cri-o-maintainers PTAL |
|
hm that one critest failure looks legit though i'm not sure why it's only failing there. |
|
@saschagrunert Can you PTAL? |
|
@cri-o/cri-o-maintainers PTAL |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bitoku, haircommander, saschagrunert The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.