WIP: UPSTREAM: 141218: test: create snapshot metadata resources in PrepareTest when CapSnapshotMetadata is enabled - #2739
Conversation
openshift-ci-robot
added
the
backports/unvalidated-commits
|
@RomanBednar: 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 |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughThe CSI driver setup now creates snapshot metadata resources when supported and defers cleanup. The snapshot metadata test suite removes its duplicate cleanup logic. ChangesSnapshot metadata lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: RomanBednar 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 388-390: Capture the return value of
utils.CleanupSnapshotMetadataResources in a scoped error variable within the
cleanup flow, then check that cleanup error when deciding whether to call
framework.Logf. Do not reuse the earlier setup err from resource creation, and
ensure the cleanup function’s error return is handled rather than ignored.
- Around line 382-383: Remove the duplicate
utils.CreateSnapshotMetadataResources call from snapshotMetadataTestSuite.init
because PrepareTest already owns creation of the TLS Secret, Service, and
SnapshotMetadataService. Update CleanupSnapshotMetadataResources to return
deletion errors, then have the deferred cleanup callback capture and handle that
returned error rather than checking the earlier setup error.
🪄 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: f6ef5468-65a5-4d7e-be41-e02a10b5d5fa
📒 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
| err = utils.CreateSnapshotMetadataResources(ctx, f, config.Driver.GetDriverInfo().Name, driverns) | ||
| if err != nil { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if rg -n '\bCreateSnapshotMetadataResources\s*\(' \
test/e2e/storage/testsuites/snapshot-metadata.go; then
echo "duplicate snapshot metadata resource creation remains"
exit 1
fi
rg -n -C 3 '\bCreateSnapshotMetadataResources\s*\(' \
test/e2e/storage/drivers/csi.go \
test/e2e/storage/utils/snapshot-metadata.goRepository: openshift/kubernetes
Length of output: 342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- snapshot metadata suite ---'
sed -n '270,335p' test/e2e/storage/testsuites/snapshot-metadata.go
printf '%s\n' '--- CSI driver setup and cleanup ---'
sed -n '350,410p' test/e2e/storage/drivers/csi.go
printf '%s\n' '--- resource helper ---'
sed -n '1,220p' test/e2e/storage/utils/snapshot-metadata.go
printf '%s\n' '--- all relevant call sites and cleanup references ---'
rg -n -C 4 '\b(CreateSnapshotMetadataResources|DeleteSnapshotMetadataResources|snapshotMetadataTestSuite\.init|PrepareTest)\b' \
test/e2e/storageRepository: openshift/kubernetes
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- helper implementation ---'
sed -n '219,305p' test/e2e/storage/utils/snapshot-metadata.go
printf '%s\n' '--- PrepareTest declaration and resource setup ---'
sed -n '240,270p' test/e2e/storage/drivers/csi.go
sed -n '370,402p' test/e2e/storage/drivers/csi.go
printf '%s\n' '--- exact call-site counts ---'
rg -n '\bCreateSnapshotMetadataResources\s*\(' \
test/e2e/storage/drivers/csi.go \
test/e2e/storage/testsuites/snapshot-metadata.go \
test/e2e/storage/utils/snapshot-metadata.go
printf '%s\n' '--- cleanup function declarations and calls ---'
rg -n -C 5 '\bCleanupSnapshotMetadataResources\b' \
test/e2e/storage/utils/snapshot-metadata.go \
test/e2e/storage/drivers/csi.goRepository: openshift/kubernetes
Length of output: 7211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
suite = Path("test/e2e/storage/testsuites/snapshot-metadata.go").read_text()
driver = Path("test/e2e/storage/drivers/csi.go").read_text()
helper = Path("test/e2e/storage/utils/snapshot-metadata.go").read_text()
suite_calls = re.findall(r"\b(?:storageutils\.)?CreateSnapshotMetadataResources\s*\(", suite)
driver_calls = re.findall(r"\butils\.CreateSnapshotMetadataResources\s*\(", driver)
helper_creates = [
name for name in ("createTLSSecret", "createSnapshotMetadataSVC", "createSnapshotMetdataServiceCR")
if re.search(r"\b" + name + r"\s*\(", helper)
]
cleanup_block = re.search(
r"ginkgo\.DeferCleanup\(func\(ctx context\.Context\) \{(?P<body>.*?)\n\t\t\}\)",
driver,
re.S,
)
body = cleanup_block.group("body") if cleanup_block else ""
print({
"suite_create_call_count": len(suite_calls),
"prepare_test_create_call_count": len(driver_calls),
"resources_created_by_helper": helper_creates,
"cleanup_result_assigned": bool(re.search(
r"\b(?:cleanupErr|err)\s*=\s*utils\.CleanupSnapshotMetadataResources\s*\(",
body,
)),
"cleanup_condition_uses_setup_err": "if err != nil" in body,
})
PYRepository: openshift/kubernetes
Length of output: 417
Keep snapshot metadata resource creation in one owner.
PrepareTest creates the TLS Secret, Service, and SnapshotMetadataService. Remove the duplicate call from snapshotMetadataTestSuite.init.
Make CleanupSnapshotMetadataResources return deletion errors. Capture and handle that result in the deferred cleanup callback instead of checking the earlier setup error.
🤖 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 382 - 383, Remove the duplicate
utils.CreateSnapshotMetadataResources call from snapshotMetadataTestSuite.init
because PrepareTest already owns creation of the TLS Secret, Service, and
SnapshotMetadataService. Update CleanupSnapshotMetadataResources to return
deletion errors, then have the deferred cleanup callback capture and handle that
returned error rather than checking the earlier setup error.
RomanBednar
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
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].
7f22945 to
0caf91a
Compare
|
@RomanBednar: 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 |
RomanBednar
changed the title
|
@RomanBednar: The following test 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. |
|
@RomanBednar: 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 |
|
Caution CodeRabbit couldn't update its existing comment. The review summary may be out of date. Error details |
|
/payload-job-with-prs periodic-ci-openshift-origin-main-e2e-aws-ovn-techpreview openshift/release#81061 |
|
@RomanBednar: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
2 participants
Summary
Cherry-pick of upstream PR kubernetes#141218
When
CSI_PROW_ENABLE_SNAPSHOT_METADATA=true,PrepareTest()deploys the csi-hostpathStatefulSet with the
csi-snapshot-metadatasidecar 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 nevercreated these resources, causing
FailedMounterrors on the secret volume.This moves resource creation into
PrepareTest()so all tests that deploy the sidecaralso have the backing resources. Cleanup is handled via
DeferCleanup.Why this is needed downstream
OpenShift CI runs the full CSI suite with snapshot metadata enabled, unlike upstream
which isolates snapshot metadata tests with
FOCUS=[Feature:snapshotmetadata]. Thebug causes every non-snapshot-metadata CSI hostpath test to fail with:
Changes
test/e2e/storage/drivers/csi.go: CallCreateSnapshotMetadataResourcesinPrepareTest()whenCapSnapshotMetadatais enabled, withDeferCleanupfor teardowntest/e2e/storage/testsuites/snapshot-metadata.go: Remove duplicate resource creation and cleanup from the snapshot metadata test suite (now handled byPrepareTest)/cc @dobsonj
Summary by CodeRabbit