feat(e2e test): add e2e test to verify udp recovery - #3346
Conversation
|
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:
📝 WalkthroughWalkthroughAdds namespace-aware syslog receiver helpers, shared syslog utilities, log-generator manifests and builders, UDP forwarding configuration, recovery E2E coverage, and controlled E2E logging. ChangesSyslog recovery and test infrastructure
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcantrill 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 |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
hack/manifests/log-generator/base/log_generator_deployment.yaml (2)
16-17: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the log-generator image to an immutable version.
:latestwithimagePullPolicy: Alwaysmakes recovery-test results depend on unreviewed registry changes. Use a reviewed release tag or, preferably, an image digest.🤖 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 `@hack/manifests/log-generator/base/log_generator_deployment.yaml` around lines 16 - 17, Update the image reference in the log-generator deployment manifest from the mutable latest tag to a reviewed release tag or immutable digest, while preserving the existing imagePullPolicy behavior.
8-17: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd the same hardening pattern and pin the image tag
Mirrortest/runtime/log_generator.go’s pod/container securityContext here, and replacequay.io/openshift-logging/cluster-logging-load-client:latestwith a fixed tag so this deployment stays reproducible.🤖 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 `@hack/manifests/log-generator/base/log_generator_deployment.yaml` around lines 8 - 17, The log-generator container lacks the security hardening used by test/runtime/log_generator.go and uses a mutable image tag. Update the container under name log-generator to mirror that pod/container securityContext, and replace the :latest image reference with a fixed version tag.Source: Linters/SAST tools
🤖 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 `@hack/manifests/log-generator/base/servicemonitor.yaml`:
- Around line 10-12: Update the ServiceMonitor configuration around jobLabel to
remove it or set it to an existing app.kubernetes.io/* Service label key defined
by the log-generator kustomization. Do not use the literal monitor-log-generator
value, and preserve the existing endpoint configuration.
- Around line 6-9: Update the ServiceMonitor spec.selector structure so the
existing log-generator labels are nested under selector.matchLabels, preserving
all label keys and values unchanged.
In `@test/e2e/logforwarding/syslog/recovery_test.go`:
- Around line 129-134: The NodePort recovery scenario must target the NodePort
endpoint rather than the service DNS address. In the “when using a NodePort
service” test, after CreateSyslogService returns, obtain the allocated nodePort
and a reachable node IP, update the sink/forwarder URL to use that IP and port,
then run podDeleteTest().
- Around line 109-118: Update the repeated deletion loop in the syslog recovery
test to track the current receiver pod UID, delete the receiver pod, and wait
for a newly recreated ready pod with a different UID before the next iteration.
Replace the fixed 5-second sleep with the existing pod readiness/wait mechanism,
while preserving the deployment replica availability assertion.
In `@test/framework/e2e/framework.go`:
- Around line 106-120: Update E2ETestFramework.Delete to return an
unsupported-kind error when the core-group switch does not assign kind, before
invoking kind.Delete. Preserve the existing Service handling and non-core error
behavior, and ensure no method is called on a nil kind.
In `@test/framework/functional/outputs/syslog/sender.go`:
- Line 30: Update the error returned by the input lookup in the relevant sender
function to identify the missing syslog input rather than an HTTP input, while
preserving the inputName value and existing error behavior.
In `@test/helpers/syslog/syslog.go`:
- Around line 24-34: Update the combined-RFC case in MustParseRFC to match the
uppercased input produced by strings.ToUpper, while preserving the existing
RFC3164 and RFC5424 mappings.
---
Nitpick comments:
In `@hack/manifests/log-generator/base/log_generator_deployment.yaml`:
- Around line 16-17: Update the image reference in the log-generator deployment
manifest from the mutable latest tag to a reviewed release tag or immutable
digest, while preserving the existing imagePullPolicy behavior.
- Around line 8-17: The log-generator container lacks the security hardening
used by test/runtime/log_generator.go and uses a mutable image tag. Update the
container under name log-generator to mirror that pod/container securityContext,
and replace the :latest image reference with a fixed version tag.
🪄 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: Enterprise
Run ID: 1d449ea3-f28d-417e-bf57-33c902d714b0
📒 Files selected for processing (21)
hack/manifests/clusterlogforwarder/base/clusterlogforwarder.yamlhack/manifests/clusterlogforwarder/overlays/syslog/udp/kustomization.yamlhack/manifests/log-generator/base/kustomization.yamlhack/manifests/log-generator/base/log_generator_deployment.yamlhack/manifests/log-generator/base/service.yamlhack/manifests/log-generator/base/servicemonitor.yamlhack/manifests/log-generator/overlays/by_payload_size/1024b_0001lps/kustomization.yamlhack/manifests/log-generator/overlays/logtype/application/kustomization.yamlinternal/factory/deployment.gotest/e2e/logforwarding/syslog/recovery_test.gotest/e2e/logforwarding/syslog/syslog_suite_test.gotest/framework/e2e/framework.gotest/framework/e2e/syslog.gotest/framework/e2e/wait.gotest/framework/functional/output_syslog.gotest/framework/functional/outputs/syslog/sender.gotest/functional/inputs/syslog/syslog_input_test.gotest/helpers/syslog/syslog.gotest/helpers/syslog/syslog_conf.gotest/runtime/log_generator.gotest/runtime/observability/cluster_log_forwarder.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: 7
🧹 Nitpick comments (2)
hack/manifests/log-generator/base/log_generator_deployment.yaml (2)
16-17: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the log-generator image to an immutable version.
:latestwithimagePullPolicy: Alwaysmakes recovery-test results depend on unreviewed registry changes. Use a reviewed release tag or, preferably, an image digest.🤖 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 `@hack/manifests/log-generator/base/log_generator_deployment.yaml` around lines 16 - 17, Update the image reference in the log-generator deployment manifest from the mutable latest tag to a reviewed release tag or immutable digest, while preserving the existing imagePullPolicy behavior.
8-17: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd the same hardening pattern and pin the image tag
Mirrortest/runtime/log_generator.go’s pod/container securityContext here, and replacequay.io/openshift-logging/cluster-logging-load-client:latestwith a fixed tag so this deployment stays reproducible.🤖 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 `@hack/manifests/log-generator/base/log_generator_deployment.yaml` around lines 8 - 17, The log-generator container lacks the security hardening used by test/runtime/log_generator.go and uses a mutable image tag. Update the container under name log-generator to mirror that pod/container securityContext, and replace the :latest image reference with a fixed version tag.Source: Linters/SAST tools
🤖 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 `@hack/manifests/log-generator/base/servicemonitor.yaml`:
- Around line 10-12: Update the ServiceMonitor configuration around jobLabel to
remove it or set it to an existing app.kubernetes.io/* Service label key defined
by the log-generator kustomization. Do not use the literal monitor-log-generator
value, and preserve the existing endpoint configuration.
- Around line 6-9: Update the ServiceMonitor spec.selector structure so the
existing log-generator labels are nested under selector.matchLabels, preserving
all label keys and values unchanged.
In `@test/e2e/logforwarding/syslog/recovery_test.go`:
- Around line 129-134: The NodePort recovery scenario must target the NodePort
endpoint rather than the service DNS address. In the “when using a NodePort
service” test, after CreateSyslogService returns, obtain the allocated nodePort
and a reachable node IP, update the sink/forwarder URL to use that IP and port,
then run podDeleteTest().
- Around line 109-118: Update the repeated deletion loop in the syslog recovery
test to track the current receiver pod UID, delete the receiver pod, and wait
for a newly recreated ready pod with a different UID before the next iteration.
Replace the fixed 5-second sleep with the existing pod readiness/wait mechanism,
while preserving the deployment replica availability assertion.
In `@test/framework/e2e/framework.go`:
- Around line 106-120: Update E2ETestFramework.Delete to return an
unsupported-kind error when the core-group switch does not assign kind, before
invoking kind.Delete. Preserve the existing Service handling and non-core error
behavior, and ensure no method is called on a nil kind.
In `@test/framework/functional/outputs/syslog/sender.go`:
- Line 30: Update the error returned by the input lookup in the relevant sender
function to identify the missing syslog input rather than an HTTP input, while
preserving the inputName value and existing error behavior.
In `@test/helpers/syslog/syslog.go`:
- Around line 24-34: Update the combined-RFC case in MustParseRFC to match the
uppercased input produced by strings.ToUpper, while preserving the existing
RFC3164 and RFC5424 mappings.
---
Nitpick comments:
In `@hack/manifests/log-generator/base/log_generator_deployment.yaml`:
- Around line 16-17: Update the image reference in the log-generator deployment
manifest from the mutable latest tag to a reviewed release tag or immutable
digest, while preserving the existing imagePullPolicy behavior.
- Around line 8-17: The log-generator container lacks the security hardening
used by test/runtime/log_generator.go and uses a mutable image tag. Update the
container under name log-generator to mirror that pod/container securityContext,
and replace the :latest image reference with a fixed version tag.
🪄 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: Enterprise
Run ID: 1d449ea3-f28d-417e-bf57-33c902d714b0
📒 Files selected for processing (21)
hack/manifests/clusterlogforwarder/base/clusterlogforwarder.yamlhack/manifests/clusterlogforwarder/overlays/syslog/udp/kustomization.yamlhack/manifests/log-generator/base/kustomization.yamlhack/manifests/log-generator/base/log_generator_deployment.yamlhack/manifests/log-generator/base/service.yamlhack/manifests/log-generator/base/servicemonitor.yamlhack/manifests/log-generator/overlays/by_payload_size/1024b_0001lps/kustomization.yamlhack/manifests/log-generator/overlays/logtype/application/kustomization.yamlinternal/factory/deployment.gotest/e2e/logforwarding/syslog/recovery_test.gotest/e2e/logforwarding/syslog/syslog_suite_test.gotest/framework/e2e/framework.gotest/framework/e2e/syslog.gotest/framework/e2e/wait.gotest/framework/functional/output_syslog.gotest/framework/functional/outputs/syslog/sender.gotest/functional/inputs/syslog/syslog_input_test.gotest/helpers/syslog/syslog.gotest/helpers/syslog/syslog_conf.gotest/runtime/log_generator.gotest/runtime/observability/cluster_log_forwarder.go
🛑 Comments failed to post (7)
hack/manifests/log-generator/base/servicemonitor.yaml (2)
6-9: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail git ls-files 'hack/manifests/log-generator/base/*' 'hack/manifests/log-generator/**' | sed -n '1,200p' echo echo '--- servicemonitor.yaml ---' cat -n hack/manifests/log-generator/base/servicemonitor.yaml echo echo '--- service.yaml ---' if [ -f hack/manifests/log-generator/base/service.yaml ]; then cat -n hack/manifests/log-generator/base/service.yaml fi echo echo '--- kustomization.yaml ---' if [ -f hack/manifests/log-generator/base/kustomization.yaml ]; then cat -n hack/manifests/log-generator/base/kustomization.yaml fiRepository: openshift/cluster-logging-operator
Length of output: 1797
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- ServiceMonitor usages ---' rg -n --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' 'kind:\s*ServiceMonitor|selector:' hack/manifests -n echo echo '--- CRD/schema references for ServiceMonitor ---' rg -n 'ServiceMonitor|matchLabels|jobLabel|podTargetLabels' -g '*.yaml' -g '*.yml' -g '*.json' -g '*.md' . echo echo '--- any other servicemonitor.yaml files ---' fd -a 'servicemonitor.*' .Repository: openshift/cluster-logging-operator
Length of output: 8560
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- jobLabel references in repo ---' rg -n 'jobLabel:' hack config bundle internal docs -g '*.yaml' -g '*.yml' -g '*.md' echo echo '--- prometheus operator servicemonitor examples in repo ---' sed -n '1,120p' config/prometheus/servicemonitor.yaml echo sed -n '1,120p' bundle/manifests/cluster-logging-operator-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yaml echo echo '--- Service label usage in log-generator base ---' cat -n hack/manifests/log-generator/base/service.yamlRepository: openshift/cluster-logging-operator
Length of output: 2027
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- ServiceMonitor schema snippets ---' rg -n -A4 -B4 'jobLabel|selector:' bundle/manifests/cluster-logging-operator-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yaml config/prometheus/servicemonitor.yaml echo echo '--- any ServiceMonitor docs in repo ---' rg -n -A3 -B3 'jobLabel|matchLabels|LabelSelector' docs config bundle hack -g '*.yaml' -g '*.yml' -g '*.md'Repository: openshift/cluster-logging-operator
Length of output: 50390
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- bundled ServiceMonitor schema around jobLabel ---' rg -n -A8 -B8 'jobLabel:' bundle/manifests/cluster-logging-operator-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yaml echo echo '--- bundled ServiceMonitor schema around selector ---' rg -n -A8 -B8 'selector:' bundle/manifests/cluster-logging-operator-metrics-monitor_monitoring.coreos.com_v1_servicemonitor.yamlRepository: openshift/cluster-logging-operator
Length of output: 226
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- log-generator kustomization ---' cat -n hack/manifests/log-generator/base/kustomization.yaml echo echo '--- any kustomize configurations in log-generator tree ---' rg -n 'configurations:|FieldSpec|ServiceMonitor|selector' hack/manifests/log-generator -g '*.yaml' -g '*.yml' echo echo '--- any repository kustomize configs mentioning ServiceMonitor selectors ---' rg -n 'ServiceMonitor|spec\.selector|matchLabels' config hack bundle -g '*kustomization*.yaml' -g '*.yaml' -g '*.yml'Repository: openshift/cluster-logging-operator
Length of output: 8157
🌐 Web query:
Prometheus Operator ServiceMonitor jobLabel missing service label fallback semantics💡 Result:
In the Prometheus Operator, the jobLabel field in a ServiceMonitor specifies which label from the associated Kubernetes Service object should be used as the value for the job label on the ingested metrics [1][2][3]. If the jobLabel field is left empty, or if the specified label does not exist on the associated Kubernetes Service, the Prometheus Operator defaults the job label to the name of that Kubernetes Service [1][4][2]. This fallback behavior ensures that every set of metrics scraped via a ServiceMonitor consistently receives a job label, even in the absence of explicit label mapping [1][3].
Citations:
- 1: https://github.com/prometheus-operator/prometheus-operator/blob/e4c727291acc543dab531bc4aaf16637067c1b86/pkg/apis/monitoring/v1/servicemonitor_types.go
- 2: https://github.com/prometheus-operator/prometheus-operator/blob/main/example/prometheus-operator-crd/monitoring.coreos.com%5Fservicemonitors.yaml
- 3: https://github.com/prometheus-operator/prometheus-operator/blob/main/pkg/apis/monitoring/v1/servicemonitor_types.go
- 4: https://docs.redhat.com/en/documentation/openshift_container_platform/4.20/html/monitoring_apis/servicemonitor-monitoring-coreos-com-v1
🌐 Web query:
kustomize labels includeSelectors ServiceMonitor selector matchLabels raw labels💡 Result:
In Kustomize, applying labels to resources while managing their inclusion in selectors (such as
spec.selector.matchLabelsin a ServiceMonitor or Deployment) is handled primarily through thelabelsfield in yourkustomization.yaml[1][2]. Thelabelsfield provides finer control than the deprecatedcommonLabelsfield [1][2]. By using thelabelsblock, you can explicitly control whether labels are propagated to selectors using theincludeSelectorsboolean flag [3][4]. Key Behavior and Configuration: 1. Labels Configuration: You can specify a list of label pairs and define their behavior usingincludeSelectors[3][1]. When set totrue, the labels are added to both themetadata.labelsand relevant selectors [3][1]. Example: labels: - pairs: app: my-app includeSelectors: false 2. Impact on Selectors (e.g., ServiceMonitor): WhenincludeSelectorsis set tofalse, Kustomize will apply the labels to the resource'smetadata.labelsbut will refrain from injecting them into thespec.selector.matchLabelssection [3]. This is critical for resources like ServiceMonitors or Deployments where you might have immutable or specific selectors that you do not want to be modified by a global label transformer [5][6]. 3. Regarding commonLabels: Note thatcommonLabelsis considered deprecated in favor of the more flexiblelabelsfield [2].commonLabelsdoes not provide anincludeSelectorsoption and will consistently apply labels to all selectors it identifies, which often causes issues with existing matchLabels [5][7]. If you need to avoid modifying selectors, migrating to thelabelsconfiguration is the recommended approach [3][1]. IfincludeSelectors: falsedoes not provide sufficient granularity (for instance, if you still want labels on pod templates but not on service selectors), you may need to use a custompatchor areplacementsconfiguration to precisely target the fields you wish to modify, bypassing the blanket behavior of the built-in label transformers [6][8][7].Citations:
- 1: kubernetes-sigs/kustomize#5436
- 2: kubernetes-sigs/kustomize#5464
- 3: kubernetes-sigs/kustomize#3937
- 4: https://pkg.go.dev/sigs.k8s.io/kustomize/api@v0.21.1
- 5: https://wi1dcard.dev/posts/integrate-servicemonitor-matchlabels-with-kustomize-commonlabels/
- 6: kubernetes-sigs/kustomize#5912
- 7: kubernetes-sigs/kustomize#249
- 8: https://github.com/kubernetes-sigs/cli-experimental/blob/master/site/content/en/references/kustomize/kustomization/replacements/_index.md
Nest the selector under
matchLabels.
spec.selectorexpects aLabelSelector, so this needs:selector: matchLabels: app.kubernetes.io/name: log-generator app.kubernetes.io/component: log-generator app.kubernetes.io/part-of: cluster-logging🤖 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 `@hack/manifests/log-generator/base/servicemonitor.yaml` around lines 6 - 9, Update the ServiceMonitor spec.selector structure so the existing log-generator labels are nested under selector.matchLabels, preserving all label keys and values unchanged.
10-12: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== service monitor ==\n' cat -n hack/manifests/log-generator/base/servicemonitor.yaml printf '\n== service ==\n' cat -n hack/manifests/log-generator/base/service.yaml printf '\n== kustomization ==\n' cat -n hack/manifests/log-generator/base/kustomization.yaml printf '\n== search for monitor-log-generator ==\n' rg -n "monitor-log-generator|jobLabel|labels:" hack/manifests/log-generator/base -SRepository: openshift/cluster-logging-operator
Length of output: 1589
🌐 Web query:
Prometheus Operator jobLabel missing label fallback Service name documentation💡 Result:
In the Prometheus Operator, the jobLabel field in a ServiceMonitor specifies which label from the associated Kubernetes Service object should be used as the source for the job label on the ingested metrics [1][2][3]. If the jobLabel field is left empty, or if the specified label does not exist on the associated Kubernetes Service, the job label on the metrics defaults to the name of the Kubernetes Service itself [1][2][3][4]. Official documentation clarifies that this mechanism is designed to provide a predictable job name even when custom labels are not explicitly defined or present on the service [1][2][3]. While older or unofficial documentation occasionally referenced Endpoints in relation to this fallback behavior, the authoritative specification confirms that the logic relies on the properties of the associated Kubernetes Service object [1][2][3][4].
Citations:
- 1: https://github.com/prometheus-operator/prometheus-operator/blob/e4c727291acc543dab531bc4aaf16637067c1b86/pkg/apis/monitoring/v1/servicemonitor_types.go
- 2: https://github.com/prometheus-operator/prometheus-operator/blob/main/pkg/apis/monitoring/v1/servicemonitor_types.go
- 3: https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/v0.82.0/example/prometheus-operator-crd/monitoring.coreos.com_servicemonitors.yaml
- 4: https://docs.redhat.com/en/documentation/openshift_container_platform/4.20/html/monitoring_apis/servicemonitor-monitoring-coreos-com-v1
🌐 Web query:
prometheus-operator ServiceMonitor jobLabel missing label key falls back to service name💡 Result:
Yes, that is correct. In the Prometheus Operator, the jobLabel field in a ServiceMonitor resource allows you to specify a label on the Kubernetes Service object to be used as the value for the job label in the ingested metrics [1][2][3]. If the jobLabel field is left empty, or if the specified label key does not exist on the associated Kubernetes Service object, the Prometheus Operator defaults the job label to the name of that Kubernetes Service [1][4][3]. This fallback behavior ensures that every scraped target receives a job label even when no custom mapping is provided [1][2].
Citations:
- 1: https://github.com/prometheus-operator/prometheus-operator/blob/main/example/prometheus-operator-crd/monitoring.coreos.com%5Fservicemonitors.yaml
- 2: https://docs.redhat.com/en/documentation/OpenShift_container_platform/4.18/html/monitoring_apis/servicemonitor-monitoring-coreos-com-v1
- 3: https://github.com/prometheus-operator/prometheus-operator/blob/main/pkg/apis/monitoring/v1/servicemonitor_types.go
- 4: https://github.com/prometheus-operator/kube-prometheus/blob/main/manifests/setup/0servicemonitorCustomResourceDefinition.yaml
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "monitor-log-generator" -S .Repository: openshift/cluster-logging-operator
Length of output: 265
Make
jobLabeluse an existing Service label key.hack/manifests/log-generator/base/servicemonitor.yamlsetsjobLabel: monitor-log-generator, but the Service only gets theapp.kubernetes.io/*labels fromkustomization.yaml. RemovejobLabelor point it at one of those labels; otherwise Prometheus uses the Service name.🤖 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 `@hack/manifests/log-generator/base/servicemonitor.yaml` around lines 10 - 12, Update the ServiceMonitor configuration around jobLabel to remove it or set it to an existing app.kubernetes.io/* Service label key defined by the log-generator kustomization. Do not use the literal monitor-log-generator value, and preserve the existing endpoint configuration.test/e2e/logforwarding/syslog/recovery_test.go (2)
109-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' test/e2e/logforwarding/syslog/recovery_test.goRepository: openshift/cluster-logging-operator
Length of output: 6097
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,220p' test/e2e/logforwarding/syslog/recovery_test.goRepository: openshift/cluster-logging-operator
Length of output: 6097
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "syslogDeployment|DeleteCollection|WaitForDeployment|labelSelector|replicas" test/e2e/logforwarding/syslog/recovery_test.goRepository: openshift/cluster-logging-operator
Length of output: 879
Wait for a new ready pod before the next delete. A fixed 5-second sleep doesn’t guarantee the receiver was recreated, so some iterations may delete nothing. Track the current pod UID and wait for a different ready UID before continuing.
🤖 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/logforwarding/syslog/recovery_test.go` around lines 109 - 118, Update the repeated deletion loop in the syslog recovery test to track the current receiver pod UID, delete the receiver pod, and wait for a newly recreated ready pod with a different UID before the next iteration. Replace the fixed 5-second sleep with the existing pod readiness/wait mechanism, while preserving the deployment replica availability assertion.
129-134: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the target test file around the cited lines. sed -n '1,240p' test/e2e/logforwarding/syslog/recovery_test.go printf '\n--- SEARCH CreateSyslogService / podDeleteTest / destination config ---\n' rg -n "CreateSyslogService|podDeleteTest|udp://|nodePort|ServiceTypeNodePort|SyslogReceiverName|syslogDeployment" test/e2e/logforwarding/syslog -S printf '\n--- Outline any nearby helpers if present ---\n' fd -a '.*\.go$' test/e2e/logforwarding/syslog | sed -n '1,120p'Repository: openshift/cluster-logging-operator
Length of output: 7757
Replace the sink URL with the NodePort endpoint. Creating a
NodePortService here still leaves the forwarder targetingudp://<service>.<namespace>.svc:514, so traffic continues through the Service/ClusterIP path. Point it at a reachable node IP and the allocated nodePort beforepodDeleteTest().🤖 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/logforwarding/syslog/recovery_test.go` around lines 129 - 134, The NodePort recovery scenario must target the NodePort endpoint rather than the service DNS address. In the “when using a NodePort service” test, after CreateSyslogService returns, obtain the allocated nodePort and a reachable node IP, update the sink/forwarder URL to use that IP and port, then run podDeleteTest().test/framework/e2e/framework.go (1)
106-120: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return an error for unsupported core kinds instead of panicking.
For a core object other than
Service,kindremains nil and Line 120 invokes a method on it.Proposed fix
switch gvk.Group { case "": switch gvk.Kind { case "Service": kind = tc.KubeClient.CoreV1().Services(meta.GetNamespace()) - break + default: + return fmt.Errorf("not impl for kind: %s", gvk.Kind) } default: return fmt.Errorf("not impl for kind: %s", gvk.Kind) }📝 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.func (tc *E2ETestFramework) Delete(o runtime.Object) error { meta := runtime.Meta(o) gvk := runtime.GroupVersionKind(o) var kind kindInterface switch gvk.Group { case "": switch gvk.Kind { case "Service": kind = tc.KubeClient.CoreV1().Services(meta.GetNamespace()) default: return fmt.Errorf("not impl for kind: %s", gvk.Kind) } default: return fmt.Errorf("not impl for kind: %s", gvk.Kind) } return kind.Delete(context.TODO(), meta.GetName(), metav1.DeleteOptions{}) }🤖 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/framework/e2e/framework.go` around lines 106 - 120, Update E2ETestFramework.Delete to return an unsupported-kind error when the core-group switch does not assign kind, before invoking kind.Delete. Preserve the existing Service handling and non-core error behavior, and ensure no method is called on a nil kind.test/framework/functional/outputs/syslog/sender.go (1)
30-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Report the missing syslog input accurately.
The current HTTP-specific message is misleading when this lookup fails.
Proposed fix
- return fmt.Errorf("WriteToHttpInput: no HTTP input named %s", inputName) + return fmt.Errorf("WriteToSyslogInputWithNetcat: no syslog input named %q", inputName)📝 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.return fmt.Errorf("WriteToSyslogInputWithNetcat: no syslog input named %q", inputName)🤖 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/framework/functional/outputs/syslog/sender.go` at line 30, Update the error returned by the input lookup in the relevant sender function to identify the missing syslog input rather than an HTTP input, while preserving the inputName value and existing error behavior.test/helpers/syslog/syslog.go (1)
24-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match the uppercased combined-RFC value.
strings.ToUpperconverts this input toRFC3164 OR RFC5424, so the current case never matches andMustParseRFCterminates the test process.Proposed fix
- case "RFC3164 or RFC5424": + case "RFC3164 OR RFC5424":📝 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.func MustParseRFC(rfc string) SyslogRfc { switch strings.ToUpper(rfc) { case "RFC3164": return RFC3164 case "RFC5424": return RFC5424 case "RFC3164 OR RFC5424": return RFC3164RFC5424 } log.Fatal("Unable to parse RFC", "rfc", rfc) return 0 }🤖 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/helpers/syslog/syslog.go` around lines 24 - 34, Update the combined-RFC case in MustParseRFC to match the uppercased input produced by strings.ToUpper, while preserving the existing RFC3164 and RFC5424 mappings.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
hack/manifests/log-generator/base/log_generator_deployment.yaml (1)
16-17: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the load-client image to an immutable version.
Using
latestwithAlwaysmeans each E2E run can execute different image contents, making failures and recovery behavior non-reproducible. Prefer a reviewed version tag or digest and update it intentionally.🤖 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 `@hack/manifests/log-generator/base/log_generator_deployment.yaml` around lines 16 - 17, Update the load-client image reference in the deployment manifest from the mutable latest tag to a reviewed immutable version tag or digest, while preserving the existing imagePullPolicy unless required by the chosen reference. Ensure future image changes are made intentionally.
🤖 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 `@hack/manifests/log-generator/base/log_generator_deployment.yaml`:
- Around line 9-17: Add an explicit restricted securityContext to the
log-generator container, including allowPrivilegeEscalation false,
readOnlyRootFilesystem true, runAsNonRoot true, a RuntimeDefault seccompProfile,
and dropping all unnecessary capabilities. Verify the
cluster-logging-load-client image supports non-root execution before setting
runAsNonRoot.
- Around line 5-8: Add a spec.selector.matchLabels entry to the Deployment and
add the identical label key/value under spec.template.metadata.labels, reusing
the label expected by the corresponding Service so the selector matches the pod
labels.
In `@test/framework/e2e/framework.go`:
- Around line 285-292: Update RunCleanupScript to surface failures from
cmd.CombinedOutput at default verbosity: log the command error unconditionally,
or propagate it to Cleanup for handling. Keep diagnostic output logging at its
existing verbosity unless needed to support the chosen error path.
---
Nitpick comments:
In `@hack/manifests/log-generator/base/log_generator_deployment.yaml`:
- Around line 16-17: Update the load-client image reference in the deployment
manifest from the mutable latest tag to a reviewed immutable version tag or
digest, while preserving the existing imagePullPolicy unless required by the
chosen reference. Ensure future image changes are made intentionally.
🪄 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: Enterprise
Run ID: a4c15531-e13a-452b-958d-1b6a947164ae
📒 Files selected for processing (21)
hack/manifests/clusterlogforwarder/base/clusterlogforwarder.yamlhack/manifests/clusterlogforwarder/overlays/syslog/udp/kustomization.yamlhack/manifests/log-generator/base/kustomization.yamlhack/manifests/log-generator/base/log_generator_deployment.yamlhack/manifests/log-generator/base/service.yamlhack/manifests/log-generator/base/servicemonitor.yamlhack/manifests/log-generator/overlays/by_payload_size/1024b_0001lps/kustomization.yamlhack/manifests/log-generator/overlays/logtype/application/kustomization.yamlinternal/factory/deployment.gotest/e2e/logforwarding/syslog/recovery_test.gotest/e2e/logforwarding/syslog/syslog_suite_test.gotest/framework/e2e/framework.gotest/framework/e2e/syslog.gotest/framework/e2e/wait.gotest/framework/functional/output_syslog.gotest/framework/functional/outputs/syslog/sender.gotest/functional/inputs/syslog/syslog_input_test.gotest/helpers/syslog/syslog.gotest/helpers/syslog/syslog_conf.gotest/runtime/log_generator.gotest/runtime/observability/cluster_log_forwarder.go
🚧 Files skipped from review as they are similar to previous changes (17)
- internal/factory/deployment.go
- hack/manifests/clusterlogforwarder/base/clusterlogforwarder.yaml
- hack/manifests/log-generator/overlays/by_payload_size/1024b_0001lps/kustomization.yaml
- hack/manifests/clusterlogforwarder/overlays/syslog/udp/kustomization.yaml
- test/framework/functional/outputs/syslog/sender.go
- test/framework/e2e/wait.go
- hack/manifests/log-generator/overlays/logtype/application/kustomization.yaml
- test/e2e/logforwarding/syslog/syslog_suite_test.go
- hack/manifests/log-generator/base/service.yaml
- hack/manifests/log-generator/base/kustomization.yaml
- test/e2e/logforwarding/syslog/recovery_test.go
- test/framework/functional/output_syslog.go
- test/helpers/syslog/syslog_conf.go
- test/helpers/syslog/syslog.go
- test/runtime/observability/cluster_log_forwarder.go
- test/runtime/log_generator.go
- test/framework/e2e/syslog.go
|
/test e2e-using-bundle |
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
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 `@hack/test-e2e-from-ci-bundle.sh`:
- Line 6: Quote the E2E package path in the launcher command by changing the
current_dir-based path to the quoted form "$current_dir/../test/e2e/...";
preserve the existing command and package pattern.
- Around line 5-6: Fix the shell continuation in the Ginkgo argument list by
ensuring the backslash after the poll-progress-after flag is the final character
on its line, so the two Ginkgo flags remain separate arguments and the test/e2e
path is continued correctly.
🪄 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: Enterprise
Run ID: 2b017e7b-b185-4818-bab9-e069f5452aeb
📒 Files selected for processing (2)
Makefilehack/test-e2e-from-ci-bundle.sh
Ref: LOG-9560 feat(manifests): add manifests for manual testing
|
/retest |
|
/test functional-target |
|
/test e2e-using-bundle |
|
PR-Agent: could not find a component named |
|
PR-Agent: could not find a component named |
|
/test functional-target |
|
PR-Agent: could not find a component named |
|
@jcantrill: 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. |
2 participants
Description
This PR:
Links
Summary by CodeRabbit
New Features
log-generatordeployment with configurable rate/payload size, Prometheus metrics, and kustomize overlays.test-e2e-from-ci-bundleexecution target.Bug Fixes
Tests