OCPNODE-2994: Add ArtifactVolume tests and register artifact image#31017
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@bitoku: This pull request references OCPNODE-2994 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 "5.0.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. |
WalkthroughReplaced a single hardcoded image-volume test suite with a parameterized generator ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
@bitoku: This pull request references OCPNODE-2994 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 "5.0.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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/node/image_volume.go (1)
194-194: Consider usingimage.ShellImage()for consistency.The container image is hardcoded, but the same file imports the
imagepackage and usesimage.LocationFor()elsewhere. Usingimage.ShellImage()would be more consistent with the rest of the codebase and more maintainable.♻️ Suggested change
- Image: "image-registry.openshift-image-registry.svc:5000/openshift/tools:latest", + Image: image.ShellImage(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/node/image_volume.go` at line 194, Replace the hardcoded container image string in the pod/container spec with the shared helper: change the Image field that currently uses "image-registry.openshift-image-registry.svc:5000/openshift/tools:latest" to call image.ShellImage() so the test uses the centralized image helper; update any imports/usages in test/extended/node/image_volume.go to reference image.ShellImage() where that Image value is constructed to keep consistency with image.LocationFor().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/extended/node/image_volume.go`:
- Line 194: Replace the hardcoded container image string in the pod/container
spec with the shared helper: change the Image field that currently uses
"image-registry.openshift-image-registry.svc:5000/openshift/tools:latest" to
call image.ShellImage() so the test uses the centralized image helper; update
any imports/usages in test/extended/node/image_volume.go to reference
image.ShellImage() where that Image value is constructed to keep consistency
with image.LocationFor().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bf5911d6-d5af-4e46-863e-7125d54321aa
📒 Files selected for processing (2)
test/extended/node/image_volume.gotest/extended/util/image/image.go
bitoku
force-pushed
the
atokubi/oci-artifact-volume-tests
branch
from
d0c1937 to
19c8537
Compare
|
@bitoku: This pull request references OCPNODE-2994 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 "5.0.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. |
|
/retest |
|
/payload-job periodic-ci-openshift-multiarch-main-nightly-4.22-ocp-e2e-aws-ovn-techpreview-multi-a-a periodic-ci-openshift-microshift-release-4.22-periodics-e2e-aws-ovn-ocp-conformance-arm64 periodic-ci-openshift-multiarch-main-nightly-4.22-ocp-e2e-ovn-remote-libvirt-s390x periodic-ci-openshift-multiarch-main-nightly-4.22-ocp-e2e-ovn-remote-s2s-libvirt-ppc64le |
|
@bitoku: trigger 4 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/7cc282c0-3cb7-11f1-9123-471f87fcec9e-0 |
|
/test e2e-gcp-ovn |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 19c8537
|
bitoku
force-pushed
the
atokubi/oci-artifact-volume-tests
branch
from
19c8537 to
f5a26f5
Compare
|
/payload-job periodic-ci-openshift-multiarch-main-nightly-4.22-ocp-e2e-aws-ovn-techpreview-multi-a-a periodic-ci-openshift-microshift-release-4.22-periodics-e2e-aws-ovn-ocp-conformance-arm64 periodic-ci-openshift-multiarch-main-nightly-4.22-ocp-e2e-ovn-remote-libvirt-s390x periodic-ci-openshift-multiarch-main-nightly-4.22-ocp-e2e-ovn-remote-s2s-libvirt-ppc64le |
|
@bitoku: trigger 4 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/f45ae210-3d79-11f1-92df-c83edea7ea62-0 |
|
/payload-job periodic-ci-openshift-multiarch-main-nightly-4.22-ocp-e2e-aws-ovn-arm64 |
|
@bitoku: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/af63be10-3d7a-11f1-8063-8728b0ec5335-0 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/node/image_volume.go (1)
87-158: Make the generated spec text neutral or config-driven.In the
ArtifactVolumebranch, theseIt(...)/By(...)strings still say"image volume", so the reported spec names and logs become contradictory. Using neutral wording like"configured volume"or deriving the noun fromconfig.describeLabelwould make failures much easier to read.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/node/image_volume.go` around lines 87 - 158, The test strings still hardcode "image volume" making logs/spec names misleading in the ArtifactVolume branch; update all human-facing test descriptions and g.By messages in this file (including the It(...) invocations around buildPodWithImageVolume, buildPodWithMultipleImageVolumes, buildPodWithImageVolumeSubPath and the g.By(...) calls that mention "image volume") to use a neutral phrase like "configured volume" or derive the noun from config.describeLabel (e.g., fmt.Sprintf("%s volume", config.describeLabel)) so the spec names and logs reflect the current config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/extended/node/image_volume.go`:
- Around line 87-158: The test strings still hardcode "image volume" making
logs/spec names misleading in the ArtifactVolume branch; update all human-facing
test descriptions and g.By messages in this file (including the It(...)
invocations around buildPodWithImageVolume, buildPodWithMultipleImageVolumes,
buildPodWithImageVolumeSubPath and the g.By(...) calls that mention "image
volume") to use a neutral phrase like "configured volume" or derive the noun
from config.describeLabel (e.g., fmt.Sprintf("%s volume", config.describeLabel))
so the spec names and logs reflect the current config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f89904e2-7b4a-4109-9464-5328ef77e270
⛔ Files ignored due to path filters (1)
test/extended/util/image/zz_generated.txtis excluded by!**/zz_generated*
📒 Files selected for processing (3)
test/extended/node/image_volume.gotest/extended/util/image/README.mdtest/extended/util/image/image.go
✅ Files skipped from review due to trivial changes (2)
- test/extended/util/image/README.md
- test/extended/util/image/image.go
| verifyCmd []string | ||
| // verifyCmd2 verifies the second volume in multi-volume tests | ||
| verifyCmd2 []string |
There was a problem hiding this comment.
also it looks like every verify cmd is just ls with the path. maybe pathsToVerify?
There was a problem hiding this comment.
I separate them because they are called for different purpose.
verifyCmd is for some test cases.
verifyCmd2 is for multiple volumes test.
verifySubPathCmd is for subpath test.
though I'm fine with either way.
No specific reason for having commands. I think it's more obvious that how we verify it, but I don't have opinion about it either.
Having said that, since the field names don't look good and explanatory, I'll change it as you suggested.
Refactor ImageVolume tests into a parameterized test suite (imageVolumeTestConfig) and add ArtifactVolume tests using quay.io/crio/artifact:subpath. Register the artifact image in the allowed images list. Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
bitoku
force-pushed
the
atokubi/oci-artifact-volume-tests
branch
from
f5a26f5 to
d6472e1
Compare
|
/test e2e-gcp-ovn |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/extended/node/image_volume.go (1)
132-138: Fail fast ifpathsToVerifyis empty.Line 135 assumes every suite provides at least one verification path. That is true today, but the shared helper will panic if a future config forgets to set it. A small guard here makes misconfiguration fail clearly.
♻️ Suggested guard
g.It("should handle image volume with subPath", func(ctx context.Context) { + o.Expect(config.pathsToVerify).NotTo(o.BeEmpty(), "pathsToVerify must be set for subPath tests") ref := config.getVolumeRef(ctx, oc, f.Namespace.Name) // Use the top-level directory of the first path as the subPath subPath := strings.Split(config.pathsToVerify[0], "/")[0] pod := buildPodWithImageVolumeSubPath(f.Namespace.Name, "", podName, ref, subPath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/node/image_volume.go` around lines 132 - 138, The test assumes config.pathsToVerify is non-empty and will panic if it's empty; add a fast-fail guard at the start of the It block to check len(config.pathsToVerify) > 0 and produce a clear test failure (e.g., call the test framework's fail/skip helper or t.Fatalf with a message) instead of proceeding; place the guard before computing subPath and before calling buildPodWithImageVolumeSubPath so failures point to the missing configuration rather than a runtime panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/node/image_volume.go`:
- Around line 170-177: In verifyPathsExist, the code uses plain "ls" which lists
directory contents and can return empty stdout for an existing empty directory;
change the command built in args to use "ls -d" (or include the "-d" flag in the
args slice) so each path is listed as an entry; update the invocation that
constructs args in verifyPathsExist (where args := []string{"ls"} and args are
appended with path.Join(mountPoint, p)) to instead start with "ls" and "-d" (or
insert "-d" before the paths) so e2epod.ExecCommandInContainer receives the
directory-checking flag and the stdout emptiness check remains valid.
---
Nitpick comments:
In `@test/extended/node/image_volume.go`:
- Around line 132-138: The test assumes config.pathsToVerify is non-empty and
will panic if it's empty; add a fast-fail guard at the start of the It block to
check len(config.pathsToVerify) > 0 and produce a clear test failure (e.g., call
the test framework's fail/skip helper or t.Fatalf with a message) instead of
proceeding; place the guard before computing subPath and before calling
buildPodWithImageVolumeSubPath so failures point to the missing configuration
rather than a runtime panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 45759ca7-fafc-4ea2-9e4a-980182f35807
⛔ Files ignored due to path filters (1)
test/extended/util/image/zz_generated.txtis excluded by!**/zz_generated*
📒 Files selected for processing (3)
test/extended/node/image_volume.gotest/extended/util/image/README.mdtest/extended/util/image/image.go
✅ Files skipped from review due to trivial changes (2)
- test/extended/util/image/README.md
- test/extended/util/image/image.go
| func verifyPathsExist(f *framework.Framework, pod *v1.Pod, mountPoint string, paths []string) { | ||
| args := []string{"ls"} | ||
| for _, p := range paths { | ||
| args = append(args, path.Join(mountPoint, p)) | ||
| } | ||
| g.By("Verifying paths exist in pod") | ||
| stdout := e2epod.ExecCommandInContainer(f, pod.Name, pod.Spec.Containers[0].Name, args...) | ||
| o.Expect(stdout).NotTo(o.BeEmpty()) |
There was a problem hiding this comment.
Use ls -d for path-existence checks.
verifyPathsExist is checking for path existence, but bare ls <dir> lists directory contents instead of the directory itself. That makes an empty directory look like a failure because stdout stays empty even though the path exists.
🐛 Minimal fix
func verifyPathsExist(f *framework.Framework, pod *v1.Pod, mountPoint string, paths []string) {
- args := []string{"ls"}
+ args := []string{"ls", "-d"}
for _, p := range paths {
args = append(args, path.Join(mountPoint, p))
}
g.By("Verifying paths exist in pod")
stdout := e2epod.ExecCommandInContainer(f, pod.Name, pod.Spec.Containers[0].Name, args...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/node/image_volume.go` around lines 170 - 177, In
verifyPathsExist, the code uses plain "ls" which lists directory contents and
can return empty stdout for an existing empty directory; change the command
built in args to use "ls -d" (or include the "-d" flag in the args slice) so
each path is listed as an entry; update the invocation that constructs args in
verifyPathsExist (where args := []string{"ls"} and args are appended with
path.Join(mountPoint, p)) to instead start with "ls" and "-d" (or insert "-d"
before the paths) so e2epod.ExecCommandInContainer receives the
directory-checking flag and the stdout emptiness check remains valid.
There was a problem hiding this comment.
this looks like a good fix.
There was a problem hiding this comment.
@rphillips, glad you agree! Here's the fix for clarity — @bitoku, please consider applying this change to test/extended/node/image_volume.go:
func verifyPathsExist(f *framework.Framework, pod *v1.Pod, mountPoint string, paths []string) {
args := []string{"ls", "-d"}
for _, p := range paths {
args = append(args, path.Join(mountPoint, p))
}
g.By("Verifying paths exist in pod")
stdout := e2epod.ExecCommandInContainer(f, pod.Name, pod.Spec.Containers[0].Name, args...)
o.Expect(stdout).NotTo(o.BeEmpty())
}Adding -d ensures ls reports each path as an entry rather than listing its contents, so an existing empty directory no longer produces empty stdout and causes a spurious failure.
There was a problem hiding this comment.
hm but in some cases it's not a directory right?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@haircommander ArtifactVolume tests succeeded in multi platform jobs. |
|
/lgtm |
|
/retest |
|
/pipeline required |
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: d6472e1
|
|
/retest |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@bitoku: This PR has been marked as verified by 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. |
|
/approve |
|
/test ? |
|
@bertinatto: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use 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 kubernetes-sigs/prow repository. |
bertinatto
left a comment
bertinatto
left a comment
There was a problem hiding this comment.
/approve
From image perspective
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto, bitoku, haircommander, jacobsee 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 |
openshift-ci
Bot
added
the
approved
|
@bitoku: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/cherry-pick release-4.22 |
|
@bitoku: new pull request created: #31108 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 kubernetes-sigs/prow repository. |
7 participants
Refactor ImageVolume tests into a parameterized test suite (imageVolumeTestConfig) and add ArtifactVolume tests using quay.io/crio/artifact:subpath. Register the artifact image in the allowed images list.
Summary by CodeRabbit