OCPBUGS-78576: Update the csi-driver-host-path version to 1.17.1#2641
OCPBUGS-78576: Update the csi-driver-host-path version to 1.17.1#2641jsafrane wants to merge 3 commits intoopenshift:masterfrom
Conversation
When testing in an air-gapped cluster, users of the e2e.test depend on -list-images and KUBE_TEST_REPO_LIST to pre-populate a local registry with required images. The hostpath image used by DRA was not part of that process (not getting patched with local registry) and the version fell behind what SIG Storage was using at least once. Now the registry and version are derived from what is being tested elsewhere, using the latest available image if there is more than one.
openshift-ci-robot
added
backports/validated-commits
|
@jsafrane: This pull request references Jira Issue OCPBUGS-78576, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
@jsafrane: the contents of this pull request could be automatically validated. The following commits are valid:
Comment |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughDeploy logic now discovers and patches the highest-version hostpathplugin image at runtime and injects the patched image into the ReplicaSet manifest. Several test manifests had container image tags updated (including one unpinned placeholder for runtime replacement). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/e2e/dra/utils/deploy.go`:
- Around line 473-491: The loop selecting hostPathVersion uses string
comparison; change it to parse image tags as semantic versions (e.g., use
k8s.io/apimachinery/pkg/util/version's ParseSemantic or a semver lib) and
compare versions semantically when updating hostPathVersion (use version.Compare
or GreaterThan/Compare methods on parsed versions), and add a guard after the
loop to fail the test if hostPathVersion is still empty before building
origImageURL (e.g., call framework.Failf or return an error) so
ReplaceRegistryInImageURL is not invoked with a missing tag; keep references to
GetOriginalImageConfigs, GetE2EImage, hostPathVersion, origImageURL, and
ReplaceRegistryInImageURL to locate the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ffd5fa4-fe6c-4fdc-be0b-4c619c00ba82
📒 Files selected for processing (6)
test/e2e/dra/utils/deploy.gotest/e2e/testing-manifests/dra/dra-test-driver-proxy.yamltest/e2e/testing-manifests/storage-csi/external-snapshotter/volume-group-snapshots/csi-hostpath-plugin.yamltest/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-plugin.yamltest/e2e/testing-manifests/storage-csi/mock/csi-mock-driver.yamltest/e2e/testing-manifests/storage-csi/mock/csi-mock-proxy.yaml
| for _, config := range image.GetOriginalImageConfigs() { | ||
| parts := strings.SplitN(config.GetE2EImage(), ":", 2) | ||
| if len(parts) < 2 { | ||
| continue | ||
| } | ||
| image, version := parts[0], parts[1] | ||
| if image != hostPathImage { | ||
| continue | ||
| } | ||
| // "Dumb" string comparison is good enough for e.g. v1.16.1 < v1.17.0. | ||
| // It seems unlikely that any major/patch will need more than one digit | ||
| // or that version grow beyond 99. | ||
| if hostPathVersion == "" || hostPathVersion < version { | ||
| hostPathVersion = version | ||
| } | ||
| } | ||
| origImageURL := hostPathImage + ":" + hostPathVersion | ||
| patchedImageURL, err := image.ReplaceRegistryInImageURL(origImageURL) | ||
| framework.ExpectNoError(err, "look up E2E image") |
There was a problem hiding this comment.
Use semantic version ordering when selecting the hostpath image.
The current lexical comparison can pick an older version as “latest” (for example, v1.9.0 sorting above v1.17.1). Also add a guard for the “no hostpath tag found” case before building origImageURL.
Suggested fix
hostPathImage := "registry.k8s.io/sig-storage/hostpathplugin"
hostPathVersion := ""
+ bestMajor, bestMinor, bestPatch := -1, -1, -1
for _, config := range image.GetOriginalImageConfigs() {
parts := strings.SplitN(config.GetE2EImage(), ":", 2)
if len(parts) < 2 {
continue
}
image, version := parts[0], parts[1]
if image != hostPathImage {
continue
}
- // "Dumb" string comparison is good enough for e.g. v1.16.1 < v1.17.0.
- // It seems unlikely that any major/patch will need more than one digit
- // or that version grow beyond 99.
- if hostPathVersion == "" || hostPathVersion < version {
+ var major, minor, patch int
+ if _, err := fmt.Sscanf(strings.TrimPrefix(version, "v"), "%d.%d.%d", &major, &minor, &patch); err != nil {
+ continue
+ }
+ if major > bestMajor ||
+ (major == bestMajor && minor > bestMinor) ||
+ (major == bestMajor && minor == bestMinor && patch > bestPatch) {
+ bestMajor, bestMinor, bestPatch = major, minor, patch
hostPathVersion = version
}
}
+ if hostPathVersion == "" {
+ framework.Failf("failed to find tagged %q image in E2E image configs", hostPathImage)
+ }
origImageURL := hostPathImage + ":" + hostPathVersion🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/dra/utils/deploy.go` around lines 473 - 491, The loop selecting
hostPathVersion uses string comparison; change it to parse image tags as
semantic versions (e.g., use k8s.io/apimachinery/pkg/util/version's
ParseSemantic or a semver lib) and compare versions semantically when updating
hostPathVersion (use version.Compare or GreaterThan/Compare methods on parsed
versions), and add a guard after the loop to fail the test if hostPathVersion is
still empty before building origImageURL (e.g., call framework.Failf or return
an error) so ReplaceRegistryInImageURL is not invoked with a missing tag; keep
references to GetOriginalImageConfigs, GetE2EImage, hostPathVersion,
origImageURL, and ReplaceRegistryInImageURL to locate the change.
|
/test unit |
|
e2e errors are real, investigating |
In air-gaped environment, the image must have `name:tag` to be parsed and updated with a private registry.
openshift-ci-robot
added
backports/unvalidated-commits
|
@jsafrane: the contents of this pull request could not be automatically validated. The following commits are valid:
The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/remove-label backports/unvalidated-commits |
openshift-ci
bot
removed
the
backports/unvalidated-commits
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto, jsafrane 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
|
/verified by jsafrane |
openshift-ci-robot
added
the
verified
|
@jsafrane: 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. |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/retest-required |
|
/retest-required https://redhat.atlassian.net/browse/OCPBUGS-77244 got fixed, so maybe... |
|
/retest-required |
3 similar comments
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
@jsafrane: The following tests failed, say
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. |
5 participants
This is a replacement for #2629, with two upstream PRs:
kubernetes#136913: Update the csi-driver-host-path version to 1.17.1 in the CSI driver manifests
kubernetes#136337: Update DRA tests to read csi-driver-host-path from the CSI driver manifests instead of hardcoding its own version.