CMP-4116: Fix platform scan pod stuck when RawResultStorage is disabled#1097
CMP-4116: Fix platform scan pod stuck when RawResultStorage is disabled#1097Vincent056 wants to merge 3 commits intoComplianceAsCode:masterfrom
Conversation
The addResultsCollectionPods function unconditionally added the TLS volume and mount referencing the result-client-cert secret, which is only created when RawResultStorage.Enabled=true. This caused the platform scan pod to get stuck in Init:0/2 when RawResultStorage was disabled. Reuse getLogCollectorVolumeMounts and conditionally append the TLS volume, matching the existing behavior in getNodeScannerPodVolumes. Made-with: Cursor
Made-with: Cursor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Vincent056 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
🤖 To deploy this PR, run the following command: |
rhmdnd
left a comment
rhmdnd
left a comment
There was a problem hiding this comment.
I have one recommendation about improving the test check by moving it to after the scan completes, which ensures we've exercised the conditional given the scan is done and must have gone through the aggregating phase successfully.
I'm concerned if we check PVCs too soon, we'll short-circuit the check. We take the same approach in TestScanSettingBindingNoStorage.
| defer f.Client.Delete(context.TODO(), testSuite) | ||
|
|
||
| pvcList := &corev1.PersistentVolumeClaimList{} | ||
| err = f.Client.List(context.TODO(), pvcList, client.InNamespace(f.OperatorNamespace), client.MatchingLabels(map[string]string{ |
There was a problem hiding this comment.
This is going to happen really fast after the scan is created, where the reconcilation loop might not have picked up the change yet and even had the opportunity to test the conditional.
Would it make sense to move this to after the scan completes?
tests/e2e/parallel/main_test.go
Outdated
| for _, pvc := range pvcList.Items { | ||
| t.Fatalf("Found unexpected PVC %s", pvc.Name) | ||
| } | ||
| t.Fatal("Expected not to find PVC associated with the scan.") |
There was a problem hiding this comment.
If the list is not empty (conditional on line 2356), we will always exit on line 2358, right? I don't think it's possible to reach this code.
rhmdnd
changed the title
|
@Vincent056: This pull request references CMP-4116 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 bug to target the "4.22.0" version, but no target version was set. 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. |
|
Verification pass. Really confused |
|
🤖 To deploy this PR, run the following command: |
|
@Vincent056: 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. |
4 participants
Summary
RawResultStorage.Enabled=false,addResultsCollectionPodsunconditionally added a TLS volume referencing theresult-client-cert-{scanName}secret, which is only created when raw result storage is enabled. This caused the platform scan pod to get stuck inInit:0/2.getLogCollectorVolumeMounts()and conditionally append the TLS volume only whenRawResultStorage.Enabled=true, matching the existing behavior ingetNodeScannerPodVolumes.TestScheduledSuitePlatformNoStoragecovering platform scans with disabled raw result storage.Made with Cursor