Stor 2974: Support running e2e tests against a pre-deployed driver - #154
Stor 2974: Support running e2e tests against a pre-deployed driver#154rvagner78 wants to merge 2 commits into
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughAdds an OpenShift test-extension CLI and packages it in the RHEL image. The E2E suite adds pre-provisioned volume tests, environment-gated setup and teardown, shared helpers, and configurable driver diagnostic namespaces. ChangesOpenShift E2E integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant OpenShiftCLI
participant AzureDiskExtension
participant AzureDiskE2ETests
participant RHELImage
OpenShiftCLI->>AzureDiskExtension: Register and run extension commands
AzureDiskExtension->>AzureDiskE2ETests: Select Azure test specifications
RHELImage->>OpenShiftCLI: Provide compressed test binary
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 3 warnings)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rvagner78 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 |
|
/payload-job-with-prs periodic-ci-openshift-azure-disk-csi-driver-master-e2e-azure-csi openshift/origin#31455 |
|
@rvagner78: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@cmd/openshift-tests-extension/go.mod`:
- Around line 111-124: Update the cmd/openshift-tests-extension module graph so
google.golang.org/grpc and golang.org/x/crypto resolve to patched versions
beyond the reported CVEs, then regenerate the corresponding go.mod, go.sum, and
vendor metadata. Preserve dependency consistency across the CSI imports and the
rest of the module graph.
In `@cmd/openshift-tests-extension/main.go`:
- Line 18: Handle both ignored environment-mutation errors: in
cmd/openshift-tests-extension/main.go at line 18, check the error from setting
SKIP_DRIVER_INSTALL and fail the command if it fails; in test/e2e/suite_test.go
at lines 48-51, assert that updating KUBECONFIG succeeds before continuing suite
setup.
In `@test/e2e/pre_provisioning.go`:
- Around line 260-263: Mark the “should succeed when reattaching a disk to a new
node on DanglingAttachError [disk.csi.azure.com]” spec as unsupported on
single-replica topologies. Add the [Skipped:SingleReplicaTopology] label to the
Ginkgo test title or invoke the established single-node skip helper alongside
skipIfUsingInTreeVolumePlugin and skipIfOnAzureStackCloud.
- Around line 147-153: Update both CreateVolume specs in
test/e2e/pre_provisioning.go at lines 147-153 and 242-248 to derive the Azure
location from cluster node topology labels or the existing environment-variable
mechanism instead of hardcoding regions; at the first site, also reconsider the
cost of the 512 GiB Premium disk, and at the second, gate the PremiumV2 scenario
on actual PremiumV2 availability rather than a region literal.
- Around line 170-178: Update the shared-disk setup in makeCreateVolumeReq to
use a Premium_LRS disk size of at least 256Gi while retaining maxshares set to
"2", and ensure diskSize reflects the same sharedDiskSize value.
- Around line 306-314: Update the skip message in the inline-volume spec guarded
by isUsingInTreeVolumePlugin to state that the test is available only for the
in-tree volume plugin. Add a concise comment next to skipVolumeDeletion = true
explaining which external cleanup process reclaims the intentionally retained 10
GiB disk.
- Around line 58-68: Reset volumeID to an empty value in the BeforeEach setup
alongside skipVolumeDeletion, and update the AfterEach cleanup to delete only
when cleanup is enabled and volumeID is non-empty. Correct the DeleteVolume
failure message to describe deletion rather than creation.
In `@test/e2e/suite.go`:
- Around line 76-81: Update execTestCmd and its setup/teardown callers to accept
and propagate Ginkgo’s SpecContext, then replace exec.Command with
exec.CommandContext using that context. Add a bounded timeout and ensure
cancellation terminates the command and any stalled child processes while
preserving existing command output and error assertions.
- Around line 78-79: Update the command stream assignments in the suite-hook
helper so child stdout is directed to os.Stderr instead of os.Stdout, preventing
non-JSON output from corrupting the OTE protocol; keep stderr forwarding to
os.Stderr unchanged.
In `@test/utils/check_driver_pods_restart.sh`:
- Around line 39-42: Update the previous-container log command in the restart
diagnostic path to use the configurable DRIVER_CONTAINER variable, defaulting to
azuredisk when unset, while preserving the existing namespace configuration and
kubectl logs behavior.
🪄 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: Pro Plus
Run ID: 87ae8c97-724c-4b9f-b346-7891251b9fc4
⛔ Files ignored due to path filters (1)
cmd/openshift-tests-extension/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
Dockerfile.openshift.rhel7cmd/openshift-tests-extension/go.modcmd/openshift-tests-extension/main.gotest/e2e/dynamic_provisioning.gotest/e2e/pre_provisioning.gotest/e2e/suite.gotest/e2e/suite_test.gotest/utils/check_driver_pods_restart.sh
| golang.org/x/crypto v0.49.0 // indirect | ||
| golang.org/x/exp v0.0.0-20250911091902-df9299821621 // indirect | ||
| golang.org/x/mod v0.33.0 // indirect | ||
| golang.org/x/net v0.52.0 // indirect | ||
| golang.org/x/oauth2 v0.34.0 // indirect | ||
| golang.org/x/sync v0.20.0 // indirect | ||
| golang.org/x/sys v0.42.0 // indirect | ||
| golang.org/x/term v0.41.0 // indirect | ||
| golang.org/x/text v0.35.0 // indirect | ||
| golang.org/x/time v0.14.0 // indirect | ||
| golang.org/x/tools v0.42.0 // indirect | ||
| google.golang.org/genproto/googleapis/api v0.0.0-20251202230838-ff82c1b0f217 // indirect | ||
| google.golang.org/genproto/googleapis/rpc v0.0.0-20260316180232-0b37fe3546d5 // indirect | ||
| google.golang.org/grpc v1.79.3 // indirect |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
curl -fsSL \
-H 'Content-Type: application/json' \
https://api.osv.dev/v1/querybatch \
--data '{
"queries": [
{"package": {"ecosystem": "Go", "name": "golang.org/x/crypto"}, "version": "0.49.0"},
{"package": {"ecosystem": "Go", "name": "google.golang.org/grpc"}, "version": "1.79.3"}
]
}' | jq .Repository: openshift/azure-disk-csi-driver
Length of output: 3408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '::group::Relevant go.mod lines'
sed -n '105,130p' cmd/openshift-tests-extension/go.mod || true
echo '::endgroup::'
echo '::group::Dependency path references'
rg -n 'golang\.org/x/crypto|google\.golang\.org/grpc|grpc|ssh|crypto/ssh' -S .Repository: openshift/azure-disk-csi-driver
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '::group::Go module file lines'
sed -n '105,130p' cmd/openshift-tests-extension/go.mod
echo '::endgroup::'
echo '::group::CVE severity details for first crypto and gRPC vuln'
python3 - <<'PY'
import json, urllib.request
packages = [
("golang.org/x/crypto", "0.49.0", "GHSA-5cgq-3rg8-m6cv"),
("google.golang.org/grpc", "1.79.3", "GHSA-hrxh-6v49-42gf"),
]
for pkg, ver, vuln_id in packages:
req = urllib.request.Request("https://api.osv.dev/v1/vulns/" + vuln_id)
try:
with urllib.request.urlopen(req, timeout=20) as resp:
data = json.load(resp)
print(f"{pkg}@{ver} references {vuln_id}: severity={data.get('severity')} summary={data.get('details','')[:220]}")
except Exception as e:
print(f"{pkg}@{ver} references {vuln_id}: ERROR {e}")
PY
echo '::endgroup::'
echo '::group::Search for direct crypto/ssh or grpc imports in non-vendor code'
rg -n '"golang\.org/x/crypto/ssh"|"golang\.org/x/crypto"|google\.golang\.org/grpc|grpc\.' --glob '!vendor/**' --glob '!cmd/openshift-tests-extension/go.mod' --glob '!go.sum' .Repository: openshift/azure-disk-csi-driver
Length of output: 5142
Upgrade the dependency graph past the reported CVEs.
google.golang.org/grpc v1.79.3 resolves to public advisories and is directly imported by the CSI code path; golang.org/x/crypto v0.49.0 also resolves to public advisories. Update the module graph to patched versions and regenerate module/vendor metadata.
🤖 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 `@cmd/openshift-tests-extension/go.mod` around lines 111 - 124, Update the
cmd/openshift-tests-extension module graph so google.golang.org/grpc and
golang.org/x/crypto resolve to patched versions beyond the reported CVEs, then
regenerate the corresponding go.mod, go.sum, and vendor metadata. Preserve
dependency consistency across the CSI imports and the rest of the module graph.
Sources: Path instructions, Linters/SAST tools
| ) | ||
|
|
||
| func main() { | ||
| os.Setenv("SKIP_DRIVER_INSTALL", "true") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle both environment-mutation errors.
cmd/openshift-tests-extension/main.go#L18-L18: fail the command if settingSKIP_DRIVER_INSTALLfails.test/e2e/suite_test.go#L48-L51: assert theKUBECONFIGenvironment update succeeds before continuing suite setup.
As per path instructions, “Never ignore error returns.”
📍 Affects 2 files
cmd/openshift-tests-extension/main.go#L18-L18(this comment)test/e2e/suite_test.go#L48-L51
🤖 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 `@cmd/openshift-tests-extension/main.go` at line 18, Handle both ignored
environment-mutation errors: in cmd/openshift-tests-extension/main.go at line
18, check the error from setting SKIP_DRIVER_INSTALL and fail the command if it
fails; in test/e2e/suite_test.go at lines 48-51, assert that updating KUBECONFIG
succeeds before continuing suite setup.
Source: Path instructions
| cmdSh := exec.Command(cmd.command, cmd.args...) | ||
| cmdSh.Dir = projectRoot | ||
| cmdSh.Stdout = os.Stdout | ||
| cmdSh.Stderr = os.Stderr | ||
| err = cmdSh.Run() | ||
| gomega.Expect(err).NotTo(gomega.HaveOccurred()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 '\bexec\.Command\(' test/e2e
rg -n -C2 '\bexecTestCmd\s*\(' test/e2eRepository: openshift/azure-disk-csi-driver
Length of output: 3346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== suite.go excerpt =="
sed -n '1,140p' test/e2e/suite.go
echo
echo "== suite_test.go relevant hooks =="
sed -n '70,125p' test/e2e/suite_test.go
sed -n '140,205p' test/e2e/suite_test.goRepository: openshift/azure-disk-csi-driver
Length of output: 9648
Make suite teardown/setup commands cancellable and bounded.
execTestCmd() runs make and e2e scripts with exec.Command().Run() inside both setup and teardown paths, so a stalled shell/background process can block suite setup/teardown indefinitely. Thread Ginkgo’s SpecContext through this helper and use exec.CommandContext(context) with a timeout/kill mechanism for these commands.
🤖 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/suite.go` around lines 76 - 81, Update execTestCmd and its
setup/teardown callers to accept and propagate Ginkgo’s SpecContext, then
replace exec.Command with exec.CommandContext using that context. Add a bounded
timeout and ensure cancellation terminates the command and any stalled child
processes while preserving existing command output and error assertions.
Source: Path instructions
| kubectl describe pod "$POD_WITH_RESTART" --namespace "${DRIVER_NAMESPACE}" | ||
| echo "======================================================================================" | ||
| echo "print previous azuredisk container logs since there is a restart" | ||
| kubectl logs "$POD_WITH_RESTART" --container azuredisk --previous --namespace kube-system | ||
| kubectl logs "$POD_WITH_RESTART" --container azuredisk --previous --namespace "${DRIVER_NAMESPACE}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'check_driver_pods_restart.sh' --exec sed -n '1,45p'
rg -nP -C2 'name:\s*azuredisk\b' --iglob '*.yaml' | head -40Repository: openshift/azure-disk-csi-driver
Length of output: 1950
🏁 Script executed:
#!/bin/bash
set -u
echo "== script file and relevant usage =="
fd -t f 'check_driver_pods_restart.sh' -x sh -c 'wc -l "$1"; sed -n "1,90p" "$1"' sh {}
echo
echo "== call sites =="
rg -n -C3 'check_driver_pods_restart|get_env_variable|DRIVER_NAMESPACE|azuredisk' test --glob '*.{go,sh,yaml,yml}' || true
echo
echo "== container names in manifests and helm/template candidates =="
fd -t f . | rg -i '(^|/)(.*yaml$|.*yml$|values.*yaml$|templates/.*yaml$)' 2>/dev/null | head -200
rg -n -C2 'containerName|name:\s*azuredisk|azuredisk' manifest manifests helm charts config --glob '*.yaml' --glob '*.yml' --glob '*.tpl' --glob '*.tmpl' --glob 'values*.yaml' 2>/dev/null || true
echo
echo "== dockerfile/csi driver container references =="
rg -n 'containerName|azuredisk' .dockerfile Dockerfile . --glob 'Dockerfile*' --glob 'Dockerfile' --glob '*Dockerfile*' --glob '*.go' | head -120Repository: openshift/azure-disk-csi-driver
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -u
echo "== exact test file bytes and line count =="
wc -l test/utils/check_driver_pods_restart.sh test/e2e/suite_test.go
sed -n '1,90p' test/utils/check_driver_pods_restart.sh
echo
echo "== error handling / suite test call site =="
rg -n -C4 'check_driver_pods_restart|DriverRestart|DRIVER_NAMESPACE|kubectl|exec' test/e2e suite_test.go test --glob '*.go' 2>/dev/null || true
echo
echo "== container names exact templates only (non-snippet) =="
python3 - <<'PY'
import subprocess, re
files = subprocess.check_output(['git','ls-files'], text=True).splitlines()
matches = []
path_re = re.compile(r'(charts|manifests).*\.ya?ml$')
name_re = re.compile(r'^\s*-\s*name\s*:\s*(.+)$', re.M)
for f in files:
if not path_re.search(f):
continue
txt = subprocess.check_output(['sed','-n','1,360p', f], text=True)
if 'azuredisk' in txt:
for i, line in enumerate(txt.splitlines(), 1):
m = re.match(r'\s*-\s*name\s*:\s*(.+)', line)
if m:
matches.append((f,i,m.group(1).strip()))
seen = {}
for fm,i, name in sorted(set(matches), key=lambda x:(x[0],x[1])):
if name != 'azuredisk':
seen.setdefault(fm,[]).append((i,name))
print("non-azuredisk azuredisk-related manifests:")
for f, items in sorted(seen.items()):
print(f)
for i,val in items[:10]:
print(f" {i}: {val}")
print("total non-azuredisk matches:", len(seen))
PY
echo
echo "== all hardcoded container names in driver node/controller manifests =="
python3 - <<'PY'
import subprocess
files = subprocess.check_output(['git','ls-files'], text=True).splitlines()
for f in files:
if not ('charts' in f or 'manifests' in f) and not (f.endswith('.yaml') or f.endswith('.yml')):
continue
txt = subprocess.check_output(['sed','-n','1,360p', f], text=True)
if 'containers:' not in txt:
continue
lines = txt.splitlines()
for idx,line in enumerate(lines):
if line.strip() == 'containers:':
start=max(0,idx-3); end=min(len(lines),idx+38)
block='\n'.join(lines[start:end])
if any(x in block for x in ['azuredisk','azuredisk-csi','csi-azuredisk']):
print(f"--- {f} around line {idx+1} ---")
for n,line in enumerate(lines[start:end], start+1):
if n<=end:
print(f"{n}: {line}")
PYRepository: openshift/azure-disk-csi-driver
Length of output: 50388
🌐 Web query:
openshift azure-disk-csi-driver manifest container names name: azuredisk
💡 Result:
In the OpenShift Azure Disk CSI driver, the container within the CSI driver pods (such as the controller and node agent) is commonly referred to as azuredisk [1]. Technical documentation and debugging guides for the driver use this name when interacting with the pods via the command line, for example when retrieving logs or executing commands inside the container [1]: kubectl logs -c azuredisk -n kubectl exec -it -c azuredisk -n -- While the OpenShift component itself may be referred to in build metadata or internal product manifests by names such as ose-azure-disk-csi-driver [2][3], the specific container runtime name defined within the deployed CSI driver manifests for the application logic remains azuredisk [1].
Citations:
- 1: https://github.com/openshift/azure-disk-csi-driver/blob/master/docs/csi-debug.md
- 2: OCPBUGS-69754: Updating ose-azure-disk-csi-driver-container image to be consistent with ART for 4.22 #119
- 3: OCPBUGS-87284: Updating ose-azure-disk-csi-driver-container image to be consistent with ART for 5.0 #152
Make the log container configurable when the namespace is configurable.
This diagnostic path is already invoked by the e2e suite via execTestCmd, so a non-zero kubectl logs -c azuredisk exit aborts the test. The current charts/templates use azuredisk, but pre-deployed installs can redefine the container name, so using ${DRIVER_CONTAINER:-azuredisk} would prevent this from failing when logs are most needed.
🤖 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/utils/check_driver_pods_restart.sh` around lines 39 - 42, Update the
previous-container log command in the restart diagnostic path to use the
configurable DRIVER_CONTAINER variable, defaulting to azuredisk when unset,
while preserving the existing namespace configuration and kubectl logs behavior.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
🤖 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 `@cmd/openshift-tests-extension/go.mod`:
- Around line 111-124: Update the cmd/openshift-tests-extension module graph so
google.golang.org/grpc and golang.org/x/crypto resolve to patched versions
beyond the reported CVEs, then regenerate the corresponding go.mod, go.sum, and
vendor metadata. Preserve dependency consistency across the CSI imports and the
rest of the module graph.
In `@cmd/openshift-tests-extension/main.go`:
- Line 18: Handle both ignored environment-mutation errors: in
cmd/openshift-tests-extension/main.go at line 18, check the error from setting
SKIP_DRIVER_INSTALL and fail the command if it fails; in test/e2e/suite_test.go
at lines 48-51, assert that updating KUBECONFIG succeeds before continuing suite
setup.
In `@test/e2e/pre_provisioning.go`:
- Around line 260-263: Mark the “should succeed when reattaching a disk to a new
node on DanglingAttachError [disk.csi.azure.com]” spec as unsupported on
single-replica topologies. Add the [Skipped:SingleReplicaTopology] label to the
Ginkgo test title or invoke the established single-node skip helper alongside
skipIfUsingInTreeVolumePlugin and skipIfOnAzureStackCloud.
- Around line 147-153: Update both CreateVolume specs in
test/e2e/pre_provisioning.go at lines 147-153 and 242-248 to derive the Azure
location from cluster node topology labels or the existing environment-variable
mechanism instead of hardcoding regions; at the first site, also reconsider the
cost of the 512 GiB Premium disk, and at the second, gate the PremiumV2 scenario
on actual PremiumV2 availability rather than a region literal.
- Around line 170-178: Update the shared-disk setup in makeCreateVolumeReq to
use a Premium_LRS disk size of at least 256Gi while retaining maxshares set to
"2", and ensure diskSize reflects the same sharedDiskSize value.
- Around line 306-314: Update the skip message in the inline-volume spec guarded
by isUsingInTreeVolumePlugin to state that the test is available only for the
in-tree volume plugin. Add a concise comment next to skipVolumeDeletion = true
explaining which external cleanup process reclaims the intentionally retained 10
GiB disk.
- Around line 58-68: Reset volumeID to an empty value in the BeforeEach setup
alongside skipVolumeDeletion, and update the AfterEach cleanup to delete only
when cleanup is enabled and volumeID is non-empty. Correct the DeleteVolume
failure message to describe deletion rather than creation.
In `@test/e2e/suite.go`:
- Around line 76-81: Update execTestCmd and its setup/teardown callers to accept
and propagate Ginkgo’s SpecContext, then replace exec.Command with
exec.CommandContext using that context. Add a bounded timeout and ensure
cancellation terminates the command and any stalled child processes while
preserving existing command output and error assertions.
- Around line 78-79: Update the command stream assignments in the suite-hook
helper so child stdout is directed to os.Stderr instead of os.Stdout, preventing
non-JSON output from corrupting the OTE protocol; keep stderr forwarding to
os.Stderr unchanged.
In `@test/utils/check_driver_pods_restart.sh`:
- Around line 39-42: Update the previous-container log command in the restart
diagnostic path to use the configurable DRIVER_CONTAINER variable, defaulting to
azuredisk when unset, while preserving the existing namespace configuration and
kubectl logs behavior.
🪄 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: Pro Plus
Run ID: 87ae8c97-724c-4b9f-b346-7891251b9fc4
⛔ Files ignored due to path filters (1)
cmd/openshift-tests-extension/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
Dockerfile.openshift.rhel7cmd/openshift-tests-extension/go.modcmd/openshift-tests-extension/main.gotest/e2e/dynamic_provisioning.gotest/e2e/pre_provisioning.gotest/e2e/suite.gotest/e2e/suite_test.gotest/utils/check_driver_pods_restart.sh
🛑 Comments failed to post (5)
test/e2e/pre_provisioning.go (5)
58-68: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
volumeIDis never reset between specs, soAfterEachcan delete a stale volume or fail on an empty ID.
skipVolumeDeletionis reset inBeforeEach(Line 55) butvolumeIDis not. Any spec that skips (e.g. Lines 144-146, 239-241) or fails beforeCreateVolumeleaves the previous spec'svolumeIDin scope, soAfterEachissuesDeleteVolumeagainst an already-deleted ID; for the first such spec the ID is""and the driver error turns a skipped spec into a failure. The failure message also says "create volume" while deleting.🐛 Reset the ID at setup and guard the teardown
ginkgo.BeforeEach(func(_ ginkgo.SpecContext) { cs = f.ClientSet ns = f.Namespace testDriver = driver.InitAzureDiskDriver() + volumeID = "" // reset value to false to default to volume clean up after test unless specified otherwise skipVolumeDeletion = false }) ginkgo.AfterEach(func(ctx ginkgo.SpecContext) { - if !skipVolumeDeletion { + if !skipVolumeDeletion && volumeID != "" { req := &csi.DeleteVolumeRequest{ VolumeId: volumeID, } _, err := azurediskDriver.DeleteVolume(ctx, req) if err != nil { - ginkgo.Fail(fmt.Sprintf("create volume %q error: %v", volumeID, err)) + ginkgo.Fail(fmt.Sprintf("delete volume %q error: %v", volumeID, err)) } } })🤖 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/pre_provisioning.go` around lines 58 - 68, Reset volumeID to an empty value in the BeforeEach setup alongside skipVolumeDeletion, and update the AfterEach cleanup to delete only when cleanup is enabled and volumeID is non-empty. Correct the DeleteVolume failure message to describe deletion rather than creation.Source: Coding guidelines
147-153: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Azure region is pinned in
CreateVolumeparameters instead of derived from the cluster. Both specs hardcode alocation, so they fail or provision cross-region disks on any cluster outside that region; the shared fix is to resolve the region from node topology labels or an env var.
test/e2e/pre_provisioning.go#L147-L153: replace"location": "eastus2"with the cluster's region, and reconsider the 512 GiB Premium disk cost.test/e2e/pre_provisioning.go#L242-L248: replace"location": "eastus"with the cluster's region and gate on PremiumV2 availability rather than a region literal.📍 Affects 1 file
test/e2e/pre_provisioning.go#L147-L153(this comment)test/e2e/pre_provisioning.go#L242-L248🤖 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/pre_provisioning.go` around lines 147 - 153, Update both CreateVolume specs in test/e2e/pre_provisioning.go at lines 147-153 and 242-248 to derive the Azure location from cluster node topology labels or the existing environment-variable mechanism instead of hardcoding regions; at the first site, also reconsider the cost of the 512 GiB Premium disk, and at the second, gate the PremiumV2 scenario on actual PremiumV2 availability rather than a region literal.
170-178: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # How does the driver parse maxShares / enforce shared-disk size limits? rg -nP -i --type=go -C3 '\bmaxshares\b' -g '!vendor/**' rg -nP --type=go -C3 'maxShares' pkg/ | head -60Repository: openshift/azure-disk-csi-driver
Length of output: 220
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Relevant e2e file lines:" sed -n '135,190p' test/e2e/pre_provisioning.go 2>/dev/null || true sed -n '190,235p' test/e2e/pre_provisioning.go 2>/dev/null || true echo echo "Search all files for maxshares/maxShares/enforce size constraints:" rg -n -i -C2 'maxshares|maxShares|max_shares|max.*share|DiskSize|1024|iops|provision' . -g '!vendor/**' -g '!node_modules/**' | head -240 echo echo "Directory listing for e2e and pkg related files:" git ls-files | rg '(^test/e2e/|pkg/|go.mod|README)' | sed -n '1,200p'Repository: openshift/azure-disk-csi-driver
Length of output: 35316
Use a valid Premium SSD shared-disk size for this test.
10GiPremium SSD disks are below Azure’s supported size for shared disks; use at least256GiforPremium_LRSwithmaxShares: "2"so the CreateVolume path matches the intended shared-disk scenario.🤖 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/pre_provisioning.go` around lines 170 - 178, Update the shared-disk setup in makeCreateVolumeReq to use a Premium_LRS disk size of at least 256Gi while retaining maxshares set to "2", and ensure diskSize reflects the same sharedDiskSize value.
260-263: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Multi-node-only spec has no Single Node OpenShift guard.
Reattaching a disk "to a new node" requires ≥2 schedulable nodes, so this will fail on SNO/single-replica topologies once the suite ships as an OTE extension. Add a
[Skipped:SingleReplicaTopology]label to the title or a single-node skip helper alongside the existingskipIf*calls.As per coding guidelines: "Flag tests expecting multiple control-plane nodes, multiple worker nodes, ... pod rescheduling to different nodes".
🤖 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/pre_provisioning.go` around lines 260 - 263, Mark the “should succeed when reattaching a disk to a new node on DanglingAttachError [disk.csi.azure.com]” spec as unsupported on single-replica topologies. Add the [Skipped:SingleReplicaTopology] label to the Ginkgo test title or invoke the established single-node skip helper alongside skipIfUsingInTreeVolumePlugin and skipIfOnAzureStackCloud.Source: Coding guidelines
306-314: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Skip message contradicts its condition, and the disk is intentionally left behind with no owner.
Line 307 skips when the in-tree plugin is not in use, so the spec is in-tree-only — the message says the opposite. Also,
skipVolumeDeletion = true(Line 314) disables teardown; add a comment explaining who reclaims the disk, otherwise every run leaks a 10 GiB disk.🧹 Fix the skip message
if !isUsingInTreeVolumePlugin { - ginkgo.Skip("test case is only available for csi driver") + ginkgo.Skip("test case is only available for in-tree volume plugin") }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.ginkgo.It("should create an inline volume by in-tree driver [kubernetes.io/azure-disk]", func(ctx ginkgo.SpecContext) { if !isUsingInTreeVolumePlugin { ginkgo.Skip("test case is only available for in-tree volume plugin") } if !isTestingMigration { ginkgo.Skip("test case is only available for migration test") } skipVolumeDeletion = true🤖 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/pre_provisioning.go` around lines 306 - 314, Update the skip message in the inline-volume spec guarded by isUsingInTreeVolumePlugin to state that the test is available only for the in-tree volume plugin. Add a concise comment next to skipVolumeDeletion = true explaining which external cleanup process reclaims the intentionally retained 10 GiB disk.
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-azure-ovn openshift/origin#31455 |
|
@rvagner78: 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/d66c44c0-8c2e-11f1-8da9-1bd31b2da99f-0 |
|
@rvagner78: 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. |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-azure-ovn openshift/origin#31455 |
|
@rvagner78: 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/6593e400-8cbb-11f1-99c7-9b2fe0c48d0b-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-azure-ovn openshift/origin#31455 |
|
@rvagner78: 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/8b8e5880-8cbf-11f1-956e-1b4687d7a361-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-azure-ovn openshift/origin#31455 |
|
@rvagner78: 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/fd2dedb0-8cde-11f1-8d3f-b712a6b01f12-0 |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/openshift-tests-extension/main.go (1)
23-23: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle both
os.Setenverrors.If either environment update fails, stop before the extension runs with an unintended driver-install or kubeconfig setting. The Line 23 instance repeats a previous finding.
Proposed fix
func main() { - os.Setenv("SKIP_DRIVER_INSTALL", "true") + if err := os.Setenv("SKIP_DRIVER_INSTALL", "true"); err != nil { + panic(fmt.Sprintf("failed to set SKIP_DRIVER_INSTALL: %v", err)) + } @@ specs.AddBeforeAll(func() { if os.Getenv("KUBECONFIG") == "" { - os.Setenv("KUBECONFIG", filepath.Join(os.Getenv("HOME"), ".kube", "config")) + if err := os.Setenv("KUBECONFIG", filepath.Join(os.Getenv("HOME"), ".kube", "config")); err != nil { + panic(fmt.Sprintf("failed to set KUBECONFIG: %v", err)) + } }As per path instructions, “Never ignore error returns.”
#!/usr/bin/env bash set -euo pipefail if rg -n -P '^\s*os\.Setenv\s*\(' cmd/openshift-tests-extension/main.go; then echo "Found unchecked direct os.Setenv calls." >&2 exit 1 fiAlso applies to: 50-53
🤖 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 `@cmd/openshift-tests-extension/main.go` at line 23, Handle the errors returned by both os.Setenv calls in main, including the SKIP_DRIVER_INSTALL update and the environment update around the referenced later block. Stop and return the error before running the extension if either operation fails; do not leave any direct unchecked os.Setenv calls.Source: Path instructions
🤖 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.
Duplicate comments:
In `@cmd/openshift-tests-extension/main.go`:
- Line 23: Handle the errors returned by both os.Setenv calls in main, including
the SKIP_DRIVER_INSTALL update and the environment update around the referenced
later block. Stop and return the error before running the extension if either
operation fails; do not leave any direct unchecked os.Setenv calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b90210da-6f14-49a9-85ac-ae38ac24de09
⛔ Files ignored due to path filters (1)
cmd/openshift-tests-extension/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
Dockerfile.openshift.rhel7cmd/openshift-tests-extension/go.modcmd/openshift-tests-extension/main.gotest/e2e/dynamic_provisioning.gotest/e2e/pre_provisioning.gotest/e2e/suite.gotest/e2e/suite_test.gotest/utils/check_driver_pods_restart.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- test/utils/check_driver_pods_restart.sh
- Dockerfile.openshift.rhel7
- test/e2e/dynamic_provisioning.go
- test/e2e/suite.go
- test/e2e/pre_provisioning.go
- test/e2e/suite_test.go
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-azure-ovn openshift/origin#31455 |
|
@rvagner78: 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/53cfd1f0-8cf9-11f1-8b66-506f40aeb130-0 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/pre_provisioning.go (1)
71-350: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated create-volume-or-fail pattern.
The sequence
req := makeCreateVolumeReq(...),resp, err := azurediskDriver.CreateVolume(ctx, req),if err != nil { ginkgo.Fail(...) },volumeID = resp.Volume.VolumeIdrepeats almost verbatim across all sixItblocks (Lines 76-83, 117-124, 157-163, 185-190, 252-257, 271-276, 316-321). Extracting a small helper, for exampleprovisionVolume(ctx, name, sizeGiB, params), would reduce duplication and keep future changes to this pattern in one place.♻️ Example helper extraction
func provisionVolume(ctx ginkgo.SpecContext, name string, sizeGiB int64, mutate func(*csi.CreateVolumeRequest)) *csi.CreateVolumeResponse { req := makeCreateVolumeReq(name, sizeGiB) if mutate != nil { mutate(req) } resp, err := azurediskDriver.CreateVolume(ctx, req) if err != nil { ginkgo.Fail(fmt.Sprintf("create volume error: %v", err)) } return resp }🤖 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/pre_provisioning.go` around lines 71 - 350, Extract the repeated volume creation and failure-handling sequence into a shared provisionVolume helper, using makeCreateVolumeReq and azurediskDriver.CreateVolume. Have it accept the request name, size, and optional request mutator so custom parameters and capabilities remain configurable, fail through the existing Ginkgo error path, and return the CreateVolume response. Update each affected It block to use the helper and assign volumeID from its response.
🤖 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/pre_provisioning.go`:
- Around line 58-68: Update the error message in the AfterEach cleanup block
after azurediskDriver.DeleteVolume to describe a volume deletion failure instead
of volume creation, while preserving the existing volumeID and err details.
---
Nitpick comments:
In `@test/e2e/pre_provisioning.go`:
- Around line 71-350: Extract the repeated volume creation and failure-handling
sequence into a shared provisionVolume helper, using makeCreateVolumeReq and
azurediskDriver.CreateVolume. Have it accept the request name, size, and
optional request mutator so custom parameters and capabilities remain
configurable, fail through the existing Ginkgo error path, and return the
CreateVolume response. Update each affected It block to use the helper and
assign volumeID from its response.
🪄 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: Pro Plus
Run ID: 8f9e7fcd-1a7a-46b1-b3b6-e2ec10b0e36b
⛔ Files ignored due to path filters (1)
cmd/openshift-tests-extension/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
Dockerfile.openshift.rhel7cmd/openshift-tests-extension/go.modcmd/openshift-tests-extension/main.gotest/e2e/dynamic_provisioning.gotest/e2e/pre_provisioning.gotest/e2e/suite.gotest/e2e/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- Dockerfile.openshift.rhel7
- test/e2e/dynamic_provisioning.go
- cmd/openshift-tests-extension/main.go
- test/e2e/suite_test.go
- test/e2e/suite.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/pre_provisioning.go (1)
71-350: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated create-volume-or-fail pattern.
The sequence
req := makeCreateVolumeReq(...),resp, err := azurediskDriver.CreateVolume(ctx, req),if err != nil { ginkgo.Fail(...) },volumeID = resp.Volume.VolumeIdrepeats almost verbatim across all sixItblocks (Lines 76-83, 117-124, 157-163, 185-190, 252-257, 271-276, 316-321). Extracting a small helper, for exampleprovisionVolume(ctx, name, sizeGiB, params), would reduce duplication and keep future changes to this pattern in one place.♻️ Example helper extraction
func provisionVolume(ctx ginkgo.SpecContext, name string, sizeGiB int64, mutate func(*csi.CreateVolumeRequest)) *csi.CreateVolumeResponse { req := makeCreateVolumeReq(name, sizeGiB) if mutate != nil { mutate(req) } resp, err := azurediskDriver.CreateVolume(ctx, req) if err != nil { ginkgo.Fail(fmt.Sprintf("create volume error: %v", err)) } return resp }🤖 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/pre_provisioning.go` around lines 71 - 350, Extract the repeated volume creation and failure-handling sequence into a shared provisionVolume helper, using makeCreateVolumeReq and azurediskDriver.CreateVolume. Have it accept the request name, size, and optional request mutator so custom parameters and capabilities remain configurable, fail through the existing Ginkgo error path, and return the CreateVolume response. Update each affected It block to use the helper and assign volumeID from its response.
🤖 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/pre_provisioning.go`:
- Around line 58-68: Update the error message in the AfterEach cleanup block
after azurediskDriver.DeleteVolume to describe a volume deletion failure instead
of volume creation, while preserving the existing volumeID and err details.
---
Nitpick comments:
In `@test/e2e/pre_provisioning.go`:
- Around line 71-350: Extract the repeated volume creation and failure-handling
sequence into a shared provisionVolume helper, using makeCreateVolumeReq and
azurediskDriver.CreateVolume. Have it accept the request name, size, and
optional request mutator so custom parameters and capabilities remain
configurable, fail through the existing Ginkgo error path, and return the
CreateVolume response. Update each affected It block to use the helper and
assign volumeID from its response.
🪄 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: Pro Plus
Run ID: 8f9e7fcd-1a7a-46b1-b3b6-e2ec10b0e36b
⛔ Files ignored due to path filters (1)
cmd/openshift-tests-extension/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
Dockerfile.openshift.rhel7cmd/openshift-tests-extension/go.modcmd/openshift-tests-extension/main.gotest/e2e/dynamic_provisioning.gotest/e2e/pre_provisioning.gotest/e2e/suite.gotest/e2e/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- Dockerfile.openshift.rhel7
- test/e2e/dynamic_provisioning.go
- cmd/openshift-tests-extension/main.go
- test/e2e/suite_test.go
- test/e2e/suite.go
🛑 Comments failed to post (1)
test/e2e/pre_provisioning.go (1)
58-68: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the AfterEach failure message.
The
AfterEachblock performsazurediskDriver.DeleteVolume, but the failure message at Line 65 says"create volume %q error: %v". This text is wrong for a delete failure and misleads anyone reading CI failure logs when cleanup fails.🐛 Proposed fix for the failure message
_, err := azurediskDriver.DeleteVolume(ctx, req) if err != nil { - ginkgo.Fail(fmt.Sprintf("create volume %q error: %v", volumeID, err)) + ginkgo.Fail(fmt.Sprintf("delete volume %q error: %v", volumeID, err)) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.ginkgo.AfterEach(func(ctx ginkgo.SpecContext) { if !skipVolumeDeletion { req := &csi.DeleteVolumeRequest{ VolumeId: volumeID, } _, err := azurediskDriver.DeleteVolume(ctx, req) if err != nil { ginkgo.Fail(fmt.Sprintf("delete volume %q error: %v", volumeID, err)) } } })🤖 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/pre_provisioning.go` around lines 58 - 68, Update the error message in the AfterEach cleanup block after azurediskDriver.DeleteVolume to describe a volume deletion failure instead of volume creation, while preserving the existing volumeID and err details.
1 participant
Summary by CodeRabbit
New Features
Improvements