WIP: test csi snapshot metadata in openshift - #2742
Conversation
When CSI_PROW_ENABLE_SNAPSHOT_METADATA is true, PrepareTest deploys the csi-hostpath StatefulSet with the csi-snapshot-metadata sidecar that mounts a TLS secret volume. Previously, the secret, Service, and SnapshotMetadataService CR were only created in the snapshot metadata test suite's BeforeEach. Non-snapshot-metadata tests never created these resources, causing FailedMount errors on the secret volume. Move resource creation to PrepareTest so all tests that deploy the sidecar also have the backing resources. This fixes mixed test suites (e.g. OpenShift CI) that run snapshot-metadata and non-snapshot-metadata tests together. Upstream CI didn't catch this because it runs snapshot metadata tests in isolation with FOCUS=[Feature:snapshotmetadata].
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
backports/unvalidated-commits
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
@dobsonj: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dobsonj The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 11 remain after this review. WalkthroughThe HostPath CSI driver setup enables snapshot metadata detection and manages snapshot metadata resources. The snapshot metadata test suite removes duplicate resource management and updates its verifier installer image. ChangesSnapshot metadata setup
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR temporarily enables snapshot metadata handling for pre-merge testing; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
![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
🧹 Nitpick comments (1)
test/e2e/storage/drivers/csi.go (1)
393-396: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn cleanup errors from the helper or remove this branch.
CleanupSnapshotMetadataResourceslogs each deletion error and returnsnilintest/e2e/storage/utils/snapshot-metadata.go, Line 248 through Line 274. Therefore, theerr != nilbranch at Line 394 through Line 396 cannot execute.Either aggregate and return cleanup errors from the helper, or keep the warning in the helper and remove this dead check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/storage/drivers/csi.go` around lines 393 - 396, Remove the unreachable err != nil warning branch after CleanupSnapshotMetadataResources in the surrounding cleanup flow, since that helper currently logs deletion failures and returns nil. Keep the helper’s existing warning behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/storage/drivers/csi.go`:
- Around line 384-391: Reorder snapshot-metadata setup in the driver
initialization flow: register DeferCleanup before
CreateSnapshotMetadataResources, create the TLS Secret before
utils.CreateFromManifests, and retain cleanup for any partial failure. Ensure
CleanupSnapshotMetadataResources is invoked for resources created before
subsequent setup errors.
---
Nitpick comments:
In `@test/e2e/storage/drivers/csi.go`:
- Around line 393-396: Remove the unreachable err != nil warning branch after
CleanupSnapshotMetadataResources in the surrounding cleanup flow, since that
helper currently logs deletion failures and returns nil. Keep the helper’s
existing warning behavior unchanged.
🪄 Autofix
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: a307d51b-2da9-4f81-847b-9629e751eca2
📒 Files selected for processing (2)
test/e2e/storage/drivers/csi.gotest/e2e/storage/testsuites/snapshot-metadata.go
💤 Files with no reviewable changes (1)
- test/e2e/storage/testsuites/snapshot-metadata.go
| if h.driverInfo.Capabilities[storageframework.CapSnapshotMetadata] { | ||
| // Create snapshot metadata resources (CRD is already created by test runner script) | ||
| ginkgo.By("Creating snapshot metadata resources") | ||
| err = utils.CreateSnapshotMetadataResources(ctx, f, config.Driver.GetDriverInfo().Name, driverns) | ||
| if err != nil { | ||
| framework.Failf("failed to create snapshot metadata resources: %v", err) | ||
| } | ||
| ginkgo.DeferCleanup(func(ctx context.Context) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'func CreateFromManifests|csi-snapshot-metadata-server-certs|CreateSnapshotMetadataResources|CleanupSnapshotMetadataResources' \
test/e2e/storage test/e2e/testing-manifestsRepository: openshift/kubernetes
Length of output: 20683
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- csi.go ---'
sed -n '310,415p' test/e2e/storage/drivers/csi.go
printf '%s\n' '--- create.go ---'
sed -n '130,235p' test/e2e/storage/utils/create.go
printf '%s\n' '--- snapshot-metadata.go ---'
sed -n '219,290p' test/e2e/storage/utils/snapshot-metadata.go
printf '%s\n' '--- manifest workload and secret references ---'
sed -n '300,465p' test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-plugin.yamlRepository: openshift/kubernetes
Length of output: 15313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
csi = Path("test/e2e/storage/drivers/csi.go").read_text()
create = Path("test/e2e/storage/utils/create.go").read_text()
snapshot = Path("test/e2e/storage/utils/snapshot-metadata.go").read_text()
manifest = Path("test/e2e/testing-manifests/storage-csi/hostpath/hostpath/csi-hostpath-plugin.yaml").read_text()
def body(text, signature):
start = text.index(signature)
brace = text.index("{", start)
depth = 0
for i in range(brace, len(text)):
if text[i] == "{":
depth += 1
elif text[i] == "}":
depth -= 1
if depth == 0:
return text[start:i+1]
raise RuntimeError(signature)
create_body = body(create, "func CreateFromManifestsTCtx")
snapshot_body = body(snapshot, "func CreateSnapshotMetadataResources")
cleanup_body = body(snapshot, "func CleanupSnapshotMetadataResources")
print("CreateFromManifestsTCtx creates items after loading/patching:")
for token in ("LoadFromManifests", "PatchItemsTCtx", "CreateItem", "Wait"):
print(f" {token}: {token in create_body}")
print("CreateSnapshotMetadataResources write order:")
for token in ("createTLSSecret", "createSnapshotMetadataSVC", "createSnapshotMetdataServiceCR"):
print(f" {token}: offset {snapshot_body.find(token)}")
print("Cleanup registration relative to helper call:")
print(" helper call offset:", csi.find("utils.CreateSnapshotMetadataResources"))
print(" DeferCleanup offset:", csi.find("ginkgo.DeferCleanup", csi.find("utils.CreateSnapshotMetadataResources")))
print("Manifest Secret reference count:")
print(" volume secretName:", len(re.findall(r"secretName:\s*csi-snapshot-metadata-server-certs", manifest)))
print(" volume mount name:", len(re.findall(r"name:\s*csi-snapshot-metadata-server-certs", manifest)))
print("CleanupSnapshotMetadataResources returns an error:")
print(" return statements:", re.findall(r"(?m)^\s*return\b.*$", cleanup_body))
PYRepository: openshift/kubernetes
Length of output: 738
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- snapshot resource creators ---'
sed -n '80,220p' test/e2e/storage/utils/snapshot-metadata.go
printf '%s\n' '--- corrected CreateFromManifests verifier ---'
python3 - <<'PY'
from pathlib import Path
text = Path("test/e2e/storage/utils/create.go").read_text()
start = text.index("func CreateFromManifestsTCtx")
end = text.index("// What is a subset", start)
body = text[start:end]
for token in ("LoadFromManifests", "PatchItemsTCtx", "patch(item)", "createItems"):
print(f"{token}: {token in body}")
print("wait-like calls in CreateFromManifestsTCtx section:")
for line in body.splitlines():
if any(token in line.lower() for token in ("wait", "poll", "observe")):
print(line.strip())
PYRepository: openshift/kubernetes
Length of output: 5245
Create snapshot-metadata resources before starting the driver and register cleanup early.
- Create the TLS Secret before
utils.CreateFromManifests. The StatefulSet references this Secret and reportsFailedMountuntil it exists. - Register
ginkgo.DeferCleanupbeforeCreateSnapshotMetadataResources, or add rollback. A later creation failure leaves earlier resources behind.CleanupSnapshotMetadataResourcesalways returnsnil, so cleanup errors cannot reach the caller.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/storage/drivers/csi.go` around lines 384 - 391, Reorder
snapshot-metadata setup in the driver initialization flow: register DeferCleanup
before CreateSnapshotMetadataResources, create the TLS Secret before
utils.CreateFromManifests, and retain cleanup for any partial failure. Ensure
CleanupSnapshotMetadataResources is invoked for resources created before
subsequent setup errors.
Source: MCP tools
|
/test ? |
|
/test e2e-aws-csi |
|
/test e2e-aws-ovn-techpreview |
|
@dobsonj: 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. |
|
@dobsonj: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/test e2e-aws-ovn-techpreview |
3 participants
This tests #2739 by temporarily hard coding
CSI_PROW_ENABLE_SNAPSHOT_METADATA=trueso we can see test results when the environment variable is true./cc @RomanBednar
/hold
Summary by CodeRabbit