Add Loki validation tests - #1359
Open
csalomon1 wants to merge 10 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request adds a comprehensive Loki system-test suite to validate cluster log forwarding. It introduces seven Loki-specific verification functions in ChangesLoki Verification and Cleanup Integration
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.go`:
- Around line 372-383: The LokiStack readiness check is currently listing
resources cluster-wide via APIClient.Client.List and can be tripped by unrelated
namespaces; change the List call to scope to the target namespace by passing a
client.InNamespace(optionsNamespace) (or equivalent) option when calling
APIClient.Client.List with lokiStackList so only the intended namespace is
queried, and add the import "sigs.k8s.io/controller-runtime/pkg/client" to
support client.InNamespace; ensure subsequent checks against lokiStackList.Items
only consider that namespace.
- Around line 421-452: The test currently validates every PVC returned by
storage.ListPVC in rdscoreparams.CLONamespace (pvcList) and can fail on non-Loki
PVCs; filter pvcList first to only include Loki-owned claims (e.g., check
pvcObj.Definition.Labels/Annotations for Loki-specific keys or
pvcObj.Definition.OwnerReferences pointing to the Loki StatefulSet/Pods) and
then iterate over that filtered list when checking pvcObj.Object.Status.Phase
and when logging/returning the count; update the klog message and the len()
usage to reflect the filtered list so only Loki PVCs are validated.
- Around line 505-516: The status condition assertions (using
clusterLogForwarder.Object.Status and helpers hasTrueCondition /
hasTrueConditionSuffix) are brittle because reconciliation is asynchronous; wrap
each Expect(...) assertion in a Ginkgo Eventually poll (e.g., Eventually with a
sensible timeout and interval) that repeatedly reads clusterLogForwarder status
and checks hasTrueCondition / hasTrueConditionSuffix for
observabilityv1.ConditionTypeReady, the audit/infrastructure inputs, the
fmt.Sprintf("ValidOutput/%s", lokiOutputName) output, and
fmt.Sprintf("ValidPipeline/%s", pipelineName) pipeline conditions until they
return true to avoid flakes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4284f512-92f1-48e5-9e8c-8b265799401d
📒 Files selected for processing (2)
tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.gotests/system-tests/rdscore/tests/00_validate_top_level.go
Comment on lines
+372
to
+383
| err := APIClient.Client.List(context.TODO(), lokiStackList) | ||
| if err != nil { | ||
| klog.V(rdscoreparams.RDSCoreLogLevel).Infof("Failed to list LokiStack resources: %v", err) | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| if len(lokiStackList.Items) == 0 { | ||
| klog.V(rdscoreparams.RDSCoreLogLevel).Info("No LokiStack resources found across namespaces") | ||
|
|
||
| return false | ||
| } |
Contributor
There was a problem hiding this comment.
Scope LokiStack readiness check to the target namespace.
Line 372 currently lists LokiStack resources cluster-wide and then requires all to be Ready. That can fail this test because of unrelated stacks in other namespaces.
Suggested fix
- err := APIClient.Client.List(context.TODO(), lokiStackList)
+ err := APIClient.Client.List(
+ context.TODO(),
+ lokiStackList,
+ client.InNamespace(rdscoreparams.CLONamespace),
+ )// import needed:
import "sigs.k8s.io/controller-runtime/pkg/client"📝 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.
Suggested change
| err := APIClient.Client.List(context.TODO(), lokiStackList) | |
| if err != nil { | |
| klog.V(rdscoreparams.RDSCoreLogLevel).Infof("Failed to list LokiStack resources: %v", err) | |
| return false | |
| } | |
| if len(lokiStackList.Items) == 0 { | |
| klog.V(rdscoreparams.RDSCoreLogLevel).Info("No LokiStack resources found across namespaces") | |
| return false | |
| } | |
| err := APIClient.Client.List( | |
| context.TODO(), | |
| lokiStackList, | |
| client.InNamespace(rdscoreparams.CLONamespace), | |
| ) | |
| if err != nil { | |
| klog.V(rdscoreparams.RDSCoreLogLevel).Infof("Failed to list LokiStack resources: %v", err) | |
| return false | |
| } | |
| if len(lokiStackList.Items) == 0 { | |
| klog.V(rdscoreparams.RDSCoreLogLevel).Info("No LokiStack resources found across namespaces") | |
| return false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.go` around
lines 372 - 383, The LokiStack readiness check is currently listing resources
cluster-wide via APIClient.Client.List and can be tripped by unrelated
namespaces; change the List call to scope to the target namespace by passing a
client.InNamespace(optionsNamespace) (or equivalent) option when calling
APIClient.Client.List with lokiStackList so only the intended namespace is
queried, and add the import "sigs.k8s.io/controller-runtime/pkg/client" to
support client.InNamespace; ensure subsequent checks against lokiStackList.Items
only consider that namespace.
Comment on lines
+421
to
+452
| pvcList, err := storage.ListPVC(APIClient, rdscoreparams.CLONamespace, metav1.ListOptions{}) | ||
| if err != nil { | ||
| klog.V(rdscoreparams.RDSCoreLogLevel).Infof( | ||
| "Failed to list PVCs in namespace %q: %v", rdscoreparams.CLONamespace, err) | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| if len(pvcList) == 0 { | ||
| klog.V(rdscoreparams.RDSCoreLogLevel).Infof( | ||
| "No PVC resources found in namespace %q", rdscoreparams.CLONamespace) | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| for _, pvcObj := range pvcList { | ||
| if pvcObj.Object.Status.Phase != corev1.ClaimBound { | ||
| klog.V(rdscoreparams.RDSCoreLogLevel).Infof( | ||
| "PVC %q in namespace %q is in %q phase", | ||
| pvcObj.Definition.Name, pvcObj.Definition.Namespace, pvcObj.Object.Status.Phase) | ||
|
|
||
| return false | ||
| } | ||
| } | ||
|
|
||
| klog.V(rdscoreparams.RDSCoreLogLevel).Infof( | ||
| "Detected %d PVC(s) in namespace %q; all are Bound", | ||
| len(pvcList), rdscoreparams.CLONamespace) | ||
|
|
||
| return true | ||
| }).WithContext(ctx).WithPolling(10*time.Second).WithTimeout(5*time.Minute).Should(BeTrue(), | ||
| "failed to verify Loki PVC resources are Bound") |
Contributor
There was a problem hiding this comment.
Filter PVC validation to Loki-owned claims only.
Line 421 validates every PVC in openshift-logging. That can produce false failures from non-Loki PVCs, while this test is explicitly Loki-focused.
Suggested fix
- for _, pvcObj := range pvcList {
+ lokiPVCCount := 0
+ for _, pvcObj := range pvcList {
+ if !strings.Contains(pvcObj.Definition.Name, "logging-loki") {
+ continue
+ }
+ lokiPVCCount++
if pvcObj.Object.Status.Phase != corev1.ClaimBound {
klog.V(rdscoreparams.RDSCoreLogLevel).Infof(
"PVC %q in namespace %q is in %q phase",
pvcObj.Definition.Name, pvcObj.Definition.Namespace, pvcObj.Object.Status.Phase)
return false
}
}
+
+ if lokiPVCCount == 0 {
+ klog.V(rdscoreparams.RDSCoreLogLevel).Infof(
+ "No Loki PVCs found in namespace %q", rdscoreparams.CLONamespace)
+ return false
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.go` around
lines 421 - 452, The test currently validates every PVC returned by
storage.ListPVC in rdscoreparams.CLONamespace (pvcList) and can fail on non-Loki
PVCs; filter pvcList first to only include Loki-owned claims (e.g., check
pvcObj.Definition.Labels/Annotations for Loki-specific keys or
pvcObj.Definition.OwnerReferences pointing to the Loki StatefulSet/Pods) and
then iterate over that filtered list when checking pvcObj.Object.Status.Phase
and when logging/returning the count; update the klog message and the len()
usage to reflect the filtered list so only Loki PVCs are validated.
Comment on lines
+505
to
+516
| By("Verify ClusterLogForwarder status conditions indicate valid and ready configuration") | ||
|
|
||
| Expect(hasTrueCondition(clusterLogForwarder.Object.Status.Conditions, observabilityv1.ConditionTypeReady)). | ||
| To(BeTrue(), "ClusterLogForwarder Ready condition is not True") | ||
| Expect(hasTrueConditionSuffix(clusterLogForwarder.Object.Status.InputConditions, "ValidInput/audit")). | ||
| To(BeTrue(), "ClusterLogForwarder audit input validation condition is not True") | ||
| Expect(hasTrueConditionSuffix(clusterLogForwarder.Object.Status.InputConditions, "ValidInput/infrastructure")). | ||
| To(BeTrue(), "ClusterLogForwarder infrastructure input validation condition is not True") | ||
| Expect(hasTrueConditionSuffix(clusterLogForwarder.Object.Status.OutputConditions, fmt.Sprintf("ValidOutput/%s", lokiOutputName))). | ||
| To(BeTrue(), fmt.Sprintf("ClusterLogForwarder output validation condition is not True for %q", lokiOutputName)) | ||
| Expect(hasTrueConditionSuffix(clusterLogForwarder.Object.Status.PipelineConditions, fmt.Sprintf("ValidPipeline/%s", pipelineName))). | ||
| To(BeTrue(), fmt.Sprintf("ClusterLogForwarder pipeline validation condition is not True for %q", pipelineName)) |
Contributor
There was a problem hiding this comment.
Make CLF status checks eventually consistent.
Lines 505-516 assert status conditions once. These conditions are reconciler-driven and can lag spec updates, causing avoidable flakes. Poll until conditions converge.
Suggested fix
- Expect(hasTrueCondition(clusterLogForwarder.Object.Status.Conditions, observabilityv1.ConditionTypeReady)).
- To(BeTrue(), "ClusterLogForwarder Ready condition is not True")
- Expect(hasTrueConditionSuffix(clusterLogForwarder.Object.Status.InputConditions, "ValidInput/audit")).
- To(BeTrue(), "ClusterLogForwarder audit input validation condition is not True")
- Expect(hasTrueConditionSuffix(clusterLogForwarder.Object.Status.InputConditions, "ValidInput/infrastructure")).
- To(BeTrue(), "ClusterLogForwarder infrastructure input validation condition is not True")
- Expect(hasTrueConditionSuffix(clusterLogForwarder.Object.Status.OutputConditions, fmt.Sprintf("ValidOutput/%s", lokiOutputName))).
- To(BeTrue(), fmt.Sprintf("ClusterLogForwarder output validation condition is not True for %q", lokiOutputName))
- Expect(hasTrueConditionSuffix(clusterLogForwarder.Object.Status.PipelineConditions, fmt.Sprintf("ValidPipeline/%s", pipelineName))).
- To(BeTrue(), fmt.Sprintf("ClusterLogForwarder pipeline validation condition is not True for %q", pipelineName))
+ Eventually(func() bool {
+ refreshed, err := clusterlogging.PullClusterLogForwarder(
+ APIClient, rdscoreparams.CLOInstanceName, rdscoreparams.CLONamespace)
+ if err != nil {
+ return false
+ }
+
+ return hasTrueCondition(refreshed.Object.Status.Conditions, observabilityv1.ConditionTypeReady) &&
+ hasTrueConditionSuffix(refreshed.Object.Status.InputConditions, "ValidInput/audit") &&
+ hasTrueConditionSuffix(refreshed.Object.Status.InputConditions, "ValidInput/infrastructure") &&
+ hasTrueConditionSuffix(refreshed.Object.Status.OutputConditions, fmt.Sprintf("ValidOutput/%s", lokiOutputName)) &&
+ hasTrueConditionSuffix(refreshed.Object.Status.PipelineConditions, fmt.Sprintf("ValidPipeline/%s", pipelineName))
+ }).WithContext(ctx).WithPolling(10*time.Second).WithTimeout(5*time.Minute).Should(BeTrue(),
+ "ClusterLogForwarder status conditions did not converge to Ready/Valid")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.go` around
lines 505 - 516, The status condition assertions (using
clusterLogForwarder.Object.Status and helpers hasTrueCondition /
hasTrueConditionSuffix) are brittle because reconciliation is asynchronous; wrap
each Expect(...) assertion in a Ginkgo Eventually poll (e.g., Eventually with a
sensible timeout and interval) that repeatedly reads clusterLogForwarder status
and checks hasTrueCondition / hasTrueConditionSuffix for
observabilityv1.ConditionTypeReady, the audit/infrastructure inputs, the
fmt.Sprintf("ValidOutput/%s", lokiOutputName) output, and
fmt.Sprintf("ValidPipeline/%s", pipelineName) pipeline conditions until they
return true to avoid flakes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/system-tests/rdscore/internal/rdscorecommon/sriov-rootless-dpdk.go (1)
142-155: 💤 Low valueLGTM — guard is correct and handles the unconfigured-namespace case cleanly.
One optional cosmetic improvement:
By("Ensuring rootless DPDK server deployment was deleted")fires unconditionally before the guard, so the Ginkgo step appears in test output even when the function returns early without doing anything. Moving it past the guard keeps the reported steps faithful to actual work performed.🔧 Optional: move
Byinside the active pathfunc CleanupRootlessDPDKServerDeployment() { - By("Ensuring rootless DPDK server deployment was deleted") - if deploymentNamespace == "" { klog.V(100).Info("Skipping rootless DPDK server cleanup: RootlessDPDKNamespace is not set in RDS config") return } + By("Ensuring rootless DPDK server deployment was deleted") + err := cleanUpRootlessDPDKDeployment(APIClient, serverDPDKDeploymentName, deploymentNamespace, serverPodLabel)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/system-tests/rdscore/internal/rdscorecommon/sriov-rootless-dpdk.go` around lines 142 - 155, In CleanupRootlessDPDKServerDeployment the By("Ensuring rootless DPDK server deployment was deleted") step is emitted even when deploymentNamespace is empty; move the By call below the deploymentNamespace guard so it only runs when actually cleaning up, i.e., keep the initial if deploymentNamespace == "" check with its log/return, then call By(...) and proceed to call cleanUpRootlessDPDKDeployment(APIClient, serverDPDKDeploymentName, deploymentNamespace, serverPodLabel) and the Expect on its err.tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.go (1)
630-635: 💤 Low valueAdd TLS MinVersion to the HTTP client configuration.
While
InsecureSkipVerifyis intentionally used for internal cluster communication, settingMinVersionis still a best practice to ensure modern TLS versions are negotiated.🔒 Suggested fix
httpClient := &http.Client{ Timeout: 30 * time.Second, Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, //nolint:gosec + MinVersion: tls.VersionTLS12, + }, }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.go` around lines 630 - 635, The TLS config for the http.Transport (created as tls.Config in the httpClient variable) lacks a MinVersion; update the tls.Config used in the http.Transport to explicitly set MinVersion (e.g., tls.VersionTLS12 or tls.VersionTLS13) while keeping InsecureSkipVerify, so the client still allows internal verification skip but only negotiates modern TLS versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.go`:
- Around line 537-552: The current errorMarkers slice used when scanning
distributor pod logs (errorMarkers) is too broad and causes false positives;
update the markers in the block that builds errorMarkers (and the subsequent
loop that checks lowerLogOutput) to use more specific patterns such as
"level=error", "err=", "failed to", or other structured-log fields your service
emits (or parse structured logs if available) so that checks against
lowerLogOutput only flag real errors instead of substrings like "error_count" or
metric names; keep the rest of the logic (containerName, distributorPod.GetLog,
loop over distributorPods and Expect checks) unchanged.
In `@tests/system-tests/rdscore/tests/00_validate_top_level.go`:
- Around line 27-30: The test references a non-existent function
rdscorecommon.VerifyDevSmokeSanity causing compile errors; fix by either
removing the Dev smoke Context block or replacing the call with an
existing/implemented function in the rdscorecommon package (e.g., implement
VerifyDevSmokeSanity with the expected signature func() or func() error or
update the It(...) call to use an existing helper like
rdscorecommon.SomeExistingSanityFunc); ensure the chosen fix compiles and the
test uses the correct function name/signature.
---
Nitpick comments:
In `@tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.go`:
- Around line 630-635: The TLS config for the http.Transport (created as
tls.Config in the httpClient variable) lacks a MinVersion; update the tls.Config
used in the http.Transport to explicitly set MinVersion (e.g., tls.VersionTLS12
or tls.VersionTLS13) while keeping InsecureSkipVerify, so the client still
allows internal verification skip but only negotiates modern TLS versions.
In `@tests/system-tests/rdscore/internal/rdscorecommon/sriov-rootless-dpdk.go`:
- Around line 142-155: In CleanupRootlessDPDKServerDeployment the By("Ensuring
rootless DPDK server deployment was deleted") step is emitted even when
deploymentNamespace is empty; move the By call below the deploymentNamespace
guard so it only runs when actually cleaning up, i.e., keep the initial if
deploymentNamespace == "" check with its log/return, then call By(...) and
proceed to call cleanUpRootlessDPDKDeployment(APIClient,
serverDPDKDeploymentName, deploymentNamespace, serverPodLabel) and the Expect on
its err.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a49a4ac3-55b5-4b4d-a235-4f8adc3d5a88
📒 Files selected for processing (3)
tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.gotests/system-tests/rdscore/internal/rdscorecommon/sriov-rootless-dpdk.gotests/system-tests/rdscore/tests/00_validate_top_level.go
Author
yprokule
requested changes
May 5, 2026
|
|
||
| // Use Eventually to handle transient errors during cleanup (etcd timeouts, connection issues) | ||
| // Retries every 5 seconds for up to 5 minutes with fresh timeout starting from cleanup phase | ||
| if deploymentNamespace == "" { |
Collaborator
There was a problem hiding this comment.
how is this related to the logging tests? please create a dedicated change for rootless DPDK tests
| Ordered, | ||
| ContinueOnFailure, | ||
| Label("rds-core-workflow"), func() { | ||
| Context("Dev smoke", Label("dev-smoke"), func() { |
Collaborator
There was a problem hiding this comment.
what is Dev smoke and why it's added for system tests?
Comment on lines
+293
to
+303
| It("Verify local Loki pods are running", | ||
| Label("log-forwarding", "loki"), | ||
| rdscorecommon.VerifyLokiPodsRunning) | ||
|
|
||
| It("Verify LokiStack resources are Ready", | ||
| Label("log-forwarding", "loki"), | ||
| rdscorecommon.VerifyLokiStackReady) | ||
|
|
||
| It("Verify Loki PVC resources are Bound", | ||
| Label("log-forwarding", "loki"), | ||
| rdscorecommon.VerifyLokiPVCsBound) |
Collaborator
There was a problem hiding this comment.
how are those system tests? that's some validation that should be used by other tests but not as standalone tests
Author
There was a problem hiding this comment.
I updated this section to a single Loki system test
Comment on lines
+304
to
+319
|
|
||
| It("Verify ClusterLogForwarder configuration for LokiStack forwarding", | ||
| Label("log-forwarding", "loki"), | ||
| rdscorecommon.VerifyClusterLogForwarderLokiConfiguration) | ||
|
|
||
| It("Verify Loki distributor logs do not contain ingestion errors", | ||
| Label("log-forwarding", "loki"), | ||
| rdscorecommon.VerifyLokiDistributorLogsNoErrors) | ||
|
|
||
| It("Verify Loki API query via ServiceAccount token", | ||
| Label("log-forwarding", "loki"), | ||
| rdscorecommon.VerifyLokiQueryWithServiceAccountToken) | ||
|
|
||
| It("Verify Loki workloads have no topology spread constraints", | ||
| Label("log-forwarding", "loki"), | ||
| rdscorecommon.VerifyLokiTopologySpreadConstraintsNotDefined) |
Collaborator
There was a problem hiding this comment.
Every test must have and ID
Author
There was a problem hiding this comment.
I updated this section to satisfy the ID requirement.
Collaborator
|
please check https://github.com/rh-ecosystem-edge/eco-gotests#commit-message-guidelines and adjust commit message accordingly, thanks |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/system-tests/rdscore/tests/00_validate_top_level.go (1)
27-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
reportxml.ID(...)to each newly addedItspec.The new
Dev smokeand Loki test cases currently lack IDs, which breaks the suite’s test-case traceability/reporting convention already called out in earlier review feedback.Also applies to: 293-320
🤖 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 `@tests/system-tests/rdscore/tests/00_validate_top_level.go` around lines 27 - 30, The new It specs (e.g., the "Dev smoke" It that calls rdscorecommon.VerifyDevSmokeSanity) are missing reportxml IDs; update each It invocation to wrap the spec with reportxml.ID("some-unique-id") so the test runner records the case ID for traceability (apply the same change to the Loki tests referenced around lines 293-320), ensuring each unique ID string is descriptive and stable per test.
🤖 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 `@tests/system-tests/rdscore/tests/00_validate_top_level.go`:
- Around line 27-30: The new It specs (e.g., the "Dev smoke" It that calls
rdscorecommon.VerifyDevSmokeSanity) are missing reportxml IDs; update each It
invocation to wrap the spec with reportxml.ID("some-unique-id") so the test
runner records the case ID for traceability (apply the same change to the Loki
tests referenced around lines 293-320), ensuring each unique ID string is
descriptive and stable per test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1e085ee3-5313-4997-8ba8-60c18fa59abe
📒 Files selected for processing (3)
tests/system-tests/rdscore/internal/rdscorecommon/dev-smoke.gotests/system-tests/rdscore/internal/rdscorecommon/sriov-rootless-dpdk.gotests/system-tests/rdscore/tests/00_validate_top_level.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/system-tests/rdscore/internal/rdscorecommon/sriov-rootless-dpdk.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.go (3)
507-516:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPoll CLF status until reconciliation converges.
These status conditions are reconciler-driven, so asserting them once from the initially fetched object is flaky. Re-read the
ClusterLogForwarderin anEventually(...)block and wait for all Ready/Valid conditions to become true.Suggested fix
- Expect(hasTrueCondition(clusterLogForwarder.Object.Status.Conditions, observabilityv1.ConditionTypeReady)). - To(BeTrue(), "ClusterLogForwarder Ready condition is not True") - Expect(hasTrueConditionSuffix(clusterLogForwarder.Object.Status.InputConditions, "ValidInput/audit")). - To(BeTrue(), "ClusterLogForwarder audit input validation condition is not True") - Expect(hasTrueConditionSuffix(clusterLogForwarder.Object.Status.InputConditions, "ValidInput/infrastructure")). - To(BeTrue(), "ClusterLogForwarder infrastructure input validation condition is not True") - Expect(hasTrueConditionSuffix(clusterLogForwarder.Object.Status.OutputConditions, fmt.Sprintf("ValidOutput/%s", lokiOutputName))). - To(BeTrue(), fmt.Sprintf("ClusterLogForwarder output validation condition is not True for %q", lokiOutputName)) - Expect(hasTrueConditionSuffix(clusterLogForwarder.Object.Status.PipelineConditions, fmt.Sprintf("ValidPipeline/%s", pipelineName))). - To(BeTrue(), fmt.Sprintf("ClusterLogForwarder pipeline validation condition is not True for %q", pipelineName)) + Eventually(func() bool { + refreshed, err := clusterlogging.PullClusterLogForwarder( + APIClient, rdscoreparams.CLOInstanceName, rdscoreparams.CLONamespace) + if err != nil { + return false + } + + return hasTrueCondition(refreshed.Object.Status.Conditions, observabilityv1.ConditionTypeReady) && + hasTrueConditionSuffix(refreshed.Object.Status.InputConditions, "ValidInput/audit") && + hasTrueConditionSuffix(refreshed.Object.Status.InputConditions, "ValidInput/infrastructure") && + hasTrueConditionSuffix(refreshed.Object.Status.OutputConditions, fmt.Sprintf("ValidOutput/%s", lokiOutputName)) && + hasTrueConditionSuffix(refreshed.Object.Status.PipelineConditions, fmt.Sprintf("ValidPipeline/%s", pipelineName)) + }).WithContext(ctx).WithPolling(10*time.Second).WithTimeout(5*time.Minute).Should(BeTrue(), + "ClusterLogForwarder status conditions did not converge to Ready/Valid")🤖 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 `@tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.go` around lines 507 - 516, The test asserts ClusterLogForwarder status conditions once which is flaky because reconciliation is asynchronous; update the assertions around ClusterLogForwarder (the variable clusterLogForwarder and the helper predicates hasTrueCondition / hasTrueConditionSuffix) to re-read the resource inside an Eventually block and wait until all the Ready/Input/Output/Pipeline conditions (the checks calling hasTrueCondition and hasTrueConditionSuffix with observabilityv1.ConditionTypeReady, "ValidInput/audit", "ValidInput/infrastructure", fmt.Sprintf("ValidOutput/%s", lokiOutputName), and fmt.Sprintf("ValidPipeline/%s", pipelineName)) become true; poll by fetching the ClusterLogForwarder object inside the Eventually closure and assert the same boolean checks there with a suitable timeout and interval.
372-383:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the LokiStack readiness check to
openshift-logging.This still lists LokiStacks cluster-wide, so an unrelated non-ready stack in another namespace can fail this Loki-specific test. Limit the list to
rdscoreparams.CLONamespaceand evaluate only those objects.Suggested fix
+import "sigs.k8s.io/controller-runtime/pkg/client" ... - err := APIClient.Client.List(context.TODO(), lokiStackList) + err := APIClient.Client.List( + ctx, + lokiStackList, + client.InNamespace(rdscoreparams.CLONamespace), + ) ... - klog.V(rdscoreparams.RDSCoreLogLevel).Info("No LokiStack resources found across namespaces") + klog.V(rdscoreparams.RDSCoreLogLevel).Infof( + "No LokiStack resources found in namespace %q", rdscoreparams.CLONamespace)🤖 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 `@tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.go` around lines 372 - 383, The LokiStack check currently lists cluster-wide via APIClient.Client.List and can be affected by unrelated namespaces; change the List call to restrict to rdscoreparams.CLONamespace by passing the namespace ListOption (e.g., use APIClient.Client.List(context.TODO(), lokiStackList, client.InNamespace(rdscoreparams.CLONamespace))) so only lokiStackList.Items from rdscoreparams.CLONamespace are returned, then keep the existing readiness/empty checks (using rdscoreparams.RDSCoreLogLevel for logging) against that scoped list.
421-448:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFilter the PVC check to Loki-owned claims only.
This loop still validates every PVC in
openshift-logging, so unrelated claims can fail a Loki-only test. Filter to Loki-owned PVCs first, then assert that at least one matching claim exists and all of those areBound.🤖 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 `@tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.go` around lines 421 - 448, Filter the PVCs to only Loki-owned claims before asserting phases: after calling storage.ListPVC (producing pvcList), build a filtered slice (e.g., lokiPVCs) by selecting only entries whose ownerReferences or identifying labels indicate they belong to Loki (use a helper predicate that checks pvcObj.Definition.OwnerReferences and known Loki labels like app/component if present). Then assert that len(lokiPVCs) > 0 and iterate over lokiPVCs (not pvcList) to ensure each pvcObj.Object.Status.Phase == corev1.ClaimBound; keep the existing log messages but reference the filtered count and names (use symbols pvcList, lokiPVCs, pvcObj, storage.ListPVC, corev1.ClaimBound).
🤖 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 `@tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.go`:
- Around line 507-516: The test asserts ClusterLogForwarder status conditions
once which is flaky because reconciliation is asynchronous; update the
assertions around ClusterLogForwarder (the variable clusterLogForwarder and the
helper predicates hasTrueCondition / hasTrueConditionSuffix) to re-read the
resource inside an Eventually block and wait until all the
Ready/Input/Output/Pipeline conditions (the checks calling hasTrueCondition and
hasTrueConditionSuffix with observabilityv1.ConditionTypeReady,
"ValidInput/audit", "ValidInput/infrastructure", fmt.Sprintf("ValidOutput/%s",
lokiOutputName), and fmt.Sprintf("ValidPipeline/%s", pipelineName)) become true;
poll by fetching the ClusterLogForwarder object inside the Eventually closure
and assert the same boolean checks there with a suitable timeout and interval.
- Around line 372-383: The LokiStack check currently lists cluster-wide via
APIClient.Client.List and can be affected by unrelated namespaces; change the
List call to restrict to rdscoreparams.CLONamespace by passing the namespace
ListOption (e.g., use APIClient.Client.List(context.TODO(), lokiStackList,
client.InNamespace(rdscoreparams.CLONamespace))) so only lokiStackList.Items
from rdscoreparams.CLONamespace are returned, then keep the existing
readiness/empty checks (using rdscoreparams.RDSCoreLogLevel for logging) against
that scoped list.
- Around line 421-448: Filter the PVCs to only Loki-owned claims before
asserting phases: after calling storage.ListPVC (producing pvcList), build a
filtered slice (e.g., lokiPVCs) by selecting only entries whose ownerReferences
or identifying labels indicate they belong to Loki (use a helper predicate that
checks pvcObj.Definition.OwnerReferences and known Loki labels like
app/component if present). Then assert that len(lokiPVCs) > 0 and iterate over
lokiPVCs (not pvcList) to ensure each pvcObj.Object.Status.Phase ==
corev1.ClaimBound; keep the existing log messages but reference the filtered
count and names (use symbols pvcList, lokiPVCs, pvcObj, storage.ListPVC,
corev1.ClaimBound).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e3de6bd-8ef0-46eb-82e6-3b583b01f445
📒 Files selected for processing (2)
tests/system-tests/rdscore/internal/rdscorecommon/log-forwarding.gotests/system-tests/rdscore/tests/00_validate_top_level.go
Author
yprokule
reviewed
May 7, 2026
Comment on lines
+385
to
+390
|
|
||
| By("Ensure rootless DPDK server deployment was deleted") | ||
| rdscorecommon.CleanupRootlessDPDKServerDeployment(ctx) | ||
|
|
||
| By("Ensure all nodes are Ready and scheduling enabled") | ||
| rdscorecommon.EnsureInNodeReadiness(ctx) |
yprokule
reviewed
May 7, 2026
Comment on lines
+744
to
+746
|
|
||
| By("Ensure rootless DPDK server deployment was deleted") | ||
| rdscorecommon.CleanupRootlessDPDKServerDeployment(ctx) |
yprokule
reviewed
May 7, 2026
Comment on lines
+1067
to
+1069
|
|
||
| By("Ensure rootless DPDK server deployment was deleted") | ||
| rdscorecommon.CleanupRootlessDPDKServerDeployment(ctx) |
…ps per review Removed cleanup steps for rootless DPDK server deployment from multiple test cases.
Author
|
The merge in the block by- openshift-kni/telco-reference#756. |
|
How dare you lay your dirty hands on newborn babies while having an affair with a married woman/your colleague on the same team? Squatting the apartment with the woman you're having affair? Having affairs under the disguise of attending a conference? Your appearance truly reflects your needs, disgusting and revolting. You even have the courage to show your dick face anywhere? In the office, in front of your colleagues? Did you pray to your God and ask for forgiveness? A rat has more value in life than you do. You should never have been born into this world. The animals that reproduced you should have left you in a toilet 44 years ago. Do you think becoming a Canadian would whitewash your sins? Your true colors cannot be hidden. Your disgusting appearance, your lousy accent, and, above all, your lack of morality reveal who you truly are. For the rest of your numbered days, may your God show you some mercy, so that you can sleep peacefully at night or look people in the eyes, hoping that they do not know about your disgusting deeds. |
1 similar comment
|
How dare you lay your dirty hands on newborn babies while having an affair with a married woman/your colleague on the same team? Squatting the apartment with the woman you're having affair? Having affairs under the disguise of attending a conference? Your appearance truly reflects your needs, disgusting and revolting. You even have the courage to show your dick face anywhere? In the office, in front of your colleagues? Did you pray to your God and ask for forgiveness? A rat has more value in life than you do. You should never have been born into this world. The animals that reproduced you should have left you in a toilet 44 years ago. Do you think becoming a Canadian would whitewash your sins? Your true colors cannot be hidden. Your disgusting appearance, your lousy accent, and, above all, your lack of morality reveal who you truly are. For the rest of your numbered days, may your God show you some mercy, so that you can sleep peacefully at night or look people in the eyes, hoping that they do not know about your disgusting deeds. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit