STOR-2560: Add testcase for readonlyrootfilesystem#30930
STOR-2560: Add testcase for readonlyrootfilesystem#30930chao007 wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@chao007: This pull request references STOR-2560 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 epic to target either version "4.22." or "openshift-4.22.", but it targets "openshift-4.21" instead. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chao007 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 |
WalkthroughAdds a new Ginkgo e2e test file that validates Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ 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.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
![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 the current code and only fix it if needed.
Inline comments:
In `@test/extended/storage/csi_readonly_rootfs.go`:
- Around line 68-80: The test currently treats an empty discovery result as
success; after calling discovery (where results are stored in the
matches/driverMatches variable), add an explicit check for the zero-resource
case and fail the test with a clear message (e.g., t.Fatalf("no CSI drivers
discovered for prefixes %v", openshiftCSIDrivers)) so an empty discovery does
not produce a false positive; apply the same explicit check and failure to the
other similar places that use the hardcoded driver lists (the blocks using
openshiftCSIDrivers and the other driver-prefix slices at the locations
corresponding to lines ~138-143 and ~203-208).
- Around line 95-97: The test is unconditionally attempting to list resources in
ManilaCSINamespace via getCSIResourcesInCluster -> getNamespaceCheckResources
which will hard-fail on non-OpenStack clusters; modify getCSIResourcesInCluster
(or the caller) to first guard access to ManilaCSINamespace by checking the
cluster platform (e.g., an existing isOpenStack/platform check) or by attempting
a namespace GET and treating NotFound as a non-fatal case, and only call
getNamespaceCheckResources(ManilaCSINamespace) when the platform is OpenStack or
the namespace exists; ensure any namespace-list errors for ManilaCSINamespace
are handled as skip/continue rather than g.Fail so the test does not abort on
clusters without that namespace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdbbc538-a777-4c7b-9de4-05205cd7a22c
📒 Files selected for processing (1)
test/extended/storage/csi_readonly_rootfs.go
| openshiftCSIDrivers := []string{ | ||
| // "aws-ebs-csi-driver", | ||
| "aws-efs-csi-driver", | ||
| "azure-disk-csi-driver", | ||
| "azure-file-csi-driver", | ||
| "gcp-pd-csi-driver", | ||
| "gcp-filestore-csi-driver", | ||
| "vmware-vsphere-csi-driver", | ||
| "ibm-vpc-block-csi", | ||
| "openstack-cinder-csi-driver", | ||
| "openstack-manila-csi", | ||
| "smb-csi-driver", | ||
| } |
There was a problem hiding this comment.
Make the zero-resource case explicit.
If discovery returns no matches, the test currently falls through to the success path and validates nothing. With the hardcoded driver prefixes and the explicit EBS exclusion, that false-positive path is realistic.
Proposed fix
e2e.Logf("Found %d OpenShift CSI resources in cluster", len(resources))
+ if len(resources) == 0 {
+ g.Skip("no matching CSI resources discovered for readOnlyRootFilesystem validation")
+ }
for _, resource := range resources {
e2e.Logf("OpenShift CSI Resource: %s %s/%s", resource.ResourceType, resource.Namespace, resource.Name)
}Also applies to: 138-143, 203-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/storage/csi_readonly_rootfs.go` around lines 68 - 80, The test
currently treats an empty discovery result as success; after calling discovery
(where results are stored in the matches/driverMatches variable), add an
explicit check for the zero-resource case and fail the test with a clear message
(e.g., t.Fatalf("no CSI drivers discovered for prefixes %v",
openshiftCSIDrivers)) so an empty discovery does not produce a false positive;
apply the same explicit check and failure to the other similar places that use
the hardcoded driver lists (the blocks using openshiftCSIDrivers and the other
driver-prefix slices at the locations corresponding to lines ~138-143 and
~203-208).
| if err != nil { | ||
| g.Fail(fmt.Sprintf("Failed to list deployments in namespace %s: %v", namespace, err)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the file and check its size
find . -name "csi_readonly_rootfs.go" -type fRepository: openshift/origin
Length of output: 106
🏁 Script executed:
# Check the helpers.go file to understand the namespace definitions
find . -path "*/test/extended/storage/helpers.go" -type fRepository: openshift/origin
Length of output: 94
🏁 Script executed:
# Read the target file to understand the code structure
wc -l test/extended/storage/csi_readonly_rootfs.goRepository: openshift/origin
Length of output: 108
🏁 Script executed:
cat -n test/extended/storage/csi_readonly_rootfs.goRepository: openshift/origin
Length of output: 8903
🏁 Script executed:
cat -n test/extended/storage/helpers.go | head -50Repository: openshift/origin
Length of output: 2673
Don't unconditionally scan the Manila CSI namespace on non-OpenStack clusters.
The ManilaCSINamespace is documented as OpenStack-only in test/extended/storage/helpers.go:30, but getCSIResourcesInCluster calls getNamespaceCheckResources(ManilaCSINamespace) at line 136 regardless of platform. The namespace listing at lines 95–97 and 117–119 hard-fails on any error, so the test will abort on clusters where this namespace doesn't exist.
Proposed fix
import (
"context"
"fmt"
"strings"
g "github.com/onsi/ginkgo/v2"
o "github.com/onsi/gomega"
configv1 "github.com/openshift/api/config/v1"
exutil "github.com/openshift/origin/test/extended/util"
corev1 "k8s.io/api/core/v1"
+ apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
e2e "k8s.io/kubernetes/test/e2e/framework"
)
@@
// Check main CSI namespace
getNamespaceCheckResources(CSINamespace)
// Check Manila CSI namespace (OpenStack)
- getNamespaceCheckResources(ManilaCSINamespace)
+ if _, err := oc.AdminKubeClient().CoreV1().Namespaces().Get(ctx, ManilaCSINamespace, metav1.GetOptions{}); err == nil {
+ getNamespaceCheckResources(ManilaCSINamespace)
+ } else if !apierrors.IsNotFound(err) {
+ g.Fail(fmt.Sprintf("Failed to get namespace %s: %v", ManilaCSINamespace, 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.
| if err != nil { | |
| g.Fail(fmt.Sprintf("Failed to list deployments in namespace %s: %v", namespace, err)) | |
| } | |
| import ( | |
| "context" | |
| "fmt" | |
| "strings" | |
| g "github.com/onsi/ginkgo/v2" | |
| o "github.com/onsi/gomega" | |
| configv1 "github.com/openshift/api/config/v1" | |
| exutil "github.com/openshift/origin/test/extended/util" | |
| corev1 "k8s.io/api/core/v1" | |
| apierrors "k8s.io/apimachinery/pkg/api/errors" | |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| e2e "k8s.io/kubernetes/test/e2e/framework" | |
| ) | |
| // ... existing code ... | |
| // Check main CSI namespace | |
| getNamespaceCheckResources(CSINamespace) | |
| // Check Manila CSI namespace (OpenStack) | |
| if _, err := oc.AdminKubeClient().CoreV1().Namespaces().Get(ctx, ManilaCSINamespace, metav1.GetOptions{}); err == nil { | |
| getNamespaceCheckResources(ManilaCSINamespace) | |
| } else if !apierrors.IsNotFound(err) { | |
| g.Fail(fmt.Sprintf("Failed to get namespace %s: %v", ManilaCSINamespace, err)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/storage/csi_readonly_rootfs.go` around lines 95 - 97, The test
is unconditionally attempting to list resources in ManilaCSINamespace via
getCSIResourcesInCluster -> getNamespaceCheckResources which will hard-fail on
non-OpenStack clusters; modify getCSIResourcesInCluster (or the caller) to first
guard access to ManilaCSINamespace by checking the cluster platform (e.g., an
existing isOpenStack/platform check) or by attempting a namespace GET and
treating NotFound as a non-fatal case, and only call
getNamespaceCheckResources(ManilaCSINamespace) when the platform is OpenStack or
the namespace exists; ensure any namespace-list errors for ManilaCSINamespace
are handled as skip/continue rather than g.Fail so the test does not abort on
clusters without that namespace.
|
Scheduling required tests: |
|
@chao007: 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. |
2 participants
No description provided.