STOR-3013: Add e2e test for SELinuxMount upgrade readiness#715
STOR-3013: Add e2e test for SELinuxMount upgrade readiness#715rvagner78 wants to merge 10 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@rvagner78: This pull request references STOR-3013 which is a valid jira issue. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
WalkthroughAdds a serial e2e suite that creates SELinux-labeled pods on a shared RWO PVC, checks upgrade readiness conditions, and polls Thanos and Prometheus signals across conflicting, compatible, and remediation scenarios. ChangesSELinux mount readiness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant KubernetesAPI
participant StorageOperator
participant ThanosQuerier
Test->>KubernetesAPI: create SELinux-labeled shared-RWO pods
KubernetesAPI->>StorageOperator: report workload readiness
Test->>StorageOperator: poll Upgradeable condition
Test->>ThanosQuerier: query conflict metric and alert state
Test->>KubernetesAPI: delete or recreate pods
Test->>StorageOperator: poll recovery to Upgradeable=True
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (11 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/e2e/selinux_mount_readiness.go (1)
84-89: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCleanup uses the timeout-bound
testContext.All deferred cleanups (namespace, pod1, pod2) use
testContext, which is derived fromctxwithtestTimeout. If the test body times out,testContextis already canceled/expired and the cleanup deletes will fail immediately, leaking the namespace/PVC/pods. Consider a freshcontext.Background()(with its own short timeout) for teardown.Also applies to: 119-123, 132-136
🤖 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/selinux_mount_readiness.go` around lines 84 - 89, The deferred cleanup paths in selinux_mount_readiness.go are using the timeout-bound testContext, so teardown can be canceled before namespace/pod deletion runs. Update the deferred cleanup blocks for the namespace and the pod cleanup helpers to use a fresh background context, ideally with a short dedicated timeout for teardown instead of reusing testContext. Make the change in the cleanup closures that call cleanupNamespace and the pod cleanup functions so they can still run after the main test context expires.
🤖 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/selinux_mount_readiness.go`:
- Around line 132-136: The deferred cleanup for pod2 in the
selinux_mount_readiness test is using pod2.Name after it has been cleared, so it
may try to delete an empty pod name. Update the cleanup logic around the
deferred deletePodWithWait closure and the later pod2 deletion path to track
“already deleted” with a separate boolean or guard instead of mutating
pod2.Name. Keep the deferred cleanup using the real pod2.Name only when the pod
still exists, and ensure the explicit delete path sets the flag rather than
blanking the name.
- Around line 65-72: The platform check in the selinux mount readiness test can
panic because infra.Status.PlatformStatus may be nil before accessing Type.
Update the infrastructure lookup/skip logic in the test function to guard for a
nil PlatformStatus before the switch, and only read PlatformStatus.Type after
confirming the pointer exists; if it is nil, skip or fail the test with a clear
message using the existing infra.Status.PlatformStatus access path.
- Around line 330-340: The value parsing in the query response aggregation
silently ignores malformed samples because the error from fmt.Sscanf is dropped.
In the helper that loops over queryResp.Data.Result, replace the Sscanf-based
conversion with explicit parsing in that block, handle the parse error
immediately, and propagate it instead of adding a default 0 to total. Keep the
fix localized to the result-processing logic so the calling path can surface bad
sample values clearly.
---
Nitpick comments:
In `@test/e2e/selinux_mount_readiness.go`:
- Around line 84-89: The deferred cleanup paths in selinux_mount_readiness.go
are using the timeout-bound testContext, so teardown can be canceled before
namespace/pod deletion runs. Update the deferred cleanup blocks for the
namespace and the pod cleanup helpers to use a fresh background context, ideally
with a short dedicated timeout for teardown instead of reusing testContext. Make
the change in the cleanup closures that call cleanupNamespace and the pod
cleanup functions so they can still run after the main test context expires.
🪄 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: Enterprise
Run ID: 9b3f00f5-6920-4de8-983c-302066099c87
📒 Files selected for processing (1)
test/e2e/selinux_mount_readiness.go
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-gcp-ovn-techpreview openshift/api#2882 openshift/kubernetes#2671 #714 #715 |
|
@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/7b96e9d0-7156-11f1-9c62-ad103feffb0c-0 |
|
@rvagner78: This PR was included in a payload test run from #715
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/7b96e9d0-7156-11f1-9c62-ad103feffb0c-0 |
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview Running the test without implementation to see how it fails. It should fail nicely. |
|
@jsafrane: 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/943f7110-7164-11f1-93a6-16440f096f18-0 |
Ginkgo cancels the BeforeEach context when the hook returns, so derive the shared test context from context.Background() instead. Co-authored-by: Cursor <cursoragent@cursor.com>
The thanos-querier route rejects unauthenticated requests with 401. Use a bearer token from kubeconfig or the prometheus-k8s service account. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-gcp-ovn-techpreview openshift/kubernetes#2671 #714 #715 |
|
@rvagner78: it appears that you have attempted to use some version of the payload command, but your comment was incorrectly formatted and cannot be acted upon. See the docs for usage info. |
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/selinux_mount_readiness.go`:
- Around line 71-73: The standalone timeout context creation in the BeforeEach
hook is triggering the fatcontext lint warning. Move the context setup out of
the BeforeEach function literal into a small helper (for example, a dedicated
function used by selinux_mount_readiness.go) that returns the context and cancel
pair, then call that helper from BeforeEach so the lifetime stays standalone
while satisfying golangci-lint.
- Around line 109-111: The shared-PVC SELinux test currently creates pod2
without ensuring it lands on the same node as pod1, so it can fail for
Multi-Attach instead of validating the SELinux-label behavior. In the SELinux
readiness test around newSelinuxTestPod and the pod2 creation flow, wait for
pod1 to reach Running, read pod1.Spec.NodeName, and force pod2 onto that node
(or use equivalent node affinity) before calling CoreV1().Pods(ns.Name).Create
for each shared-PVC case.
🪄 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: Enterprise
Run ID: 5dafda6e-c1eb-4a8a-b4b7-1540707759e1
📒 Files selected for processing (1)
test/e2e/selinux_mount_readiness.go
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-gcp-ovn-techpreview openshift/kubernetes#2671 #714 #715 |
|
@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/ecea6200-73be-11f1-80a5-d953eec5d313-0 |
|
@rvagner78: This PR was included in a payload test run from #715
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ecea6200-73be-11f1-80a5-d953eec5d313-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-gcp-ovn-techpreview openshift/kubernetes#2671 #714 #715 |
|
@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/ff343440-7490-11f1-893c-a6a79a083dc4-0 |
|
@rvagner78: This PR was included in a payload test run from #715
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ff343440-7490-11f1-893c-a6a79a083dc4-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial openshift/kubernetes#2671 #714 #715 this should succeed |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/selinux_mount_readiness.go`:
- Around line 251-354: Add deferred cleanup for the initial pods in the SELinux
e2e scenarios so they don’t leak if the test fails before the explicit delete
calls. In the scenario implementations that use createSharedPVCPods, add defers
for both pod1 and pod2 right after creation (using deleteSELinuxTestPod with the
current pod names), matching the cleanup pattern already used elsewhere; this is
especially needed in the scenario with pod1 already deferred and in the scenario
that currently has no initial defers. Keep the later reassignments to pod1/pod2
in createSELinuxTestPod unchanged, since deferred argument evaluation will
capture the original names safely.
🪄 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: Enterprise
Run ID: 205b0080-e5bc-4657-ad08-d998dac91178
📒 Files selected for processing (1)
test/e2e/selinux_mount_readiness.go
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/retest |
|
/test e2e-gcp-csi |
|
/test all |
2 similar comments
|
/test all |
|
/test all |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial openshift/kubernetes#2671 #715 |
|
@rvagner78: trigger 3 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/b0f91d60-7f60-11f1-95cd-1dbf0c0da4a3-0 |
|
@rvagner78: This PR was included in a payload test run from #715
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/b0f91d60-7f60-11f1-95cd-1dbf0c0da4a3-0 |
|
/test e2e-aws-ovn-upgrade |
|
/test e2e-azure-csi |
|
/test hypershift-e2e-aks |
|
@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-aws-ovn-techpreview-serial openshift/kubernetes#2671 #715 |
|
@rvagner78: trigger 3 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/14d6ea70-8064-11f1-93bb-5015ab4b9f49-0 |
|
@rvagner78: This PR was included in a payload test run from #715
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/14d6ea70-8064-11f1-93bb-5015ab4b9f49-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial openshift/kubernetes#2671 #715 |
|
@rvagner78: trigger 3 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/756dc720-80e9-11f1-84c7-c0f45ceb7642-0 |
|
@rvagner78: This PR was included in a payload test run from #715
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/756dc720-80e9-11f1-84c7-c0f45ceb7642-0 |
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview |
3 participants
Summary by CodeRabbit