ROSAENG-63202: Add e2e code coverage instrumentation for SRE operators - #82773
ROSAENG-63202: Add e2e code coverage instrumentation for SRE operators#82773dustman9000 wants to merge 1 commit into
Conversation
|
@dustman9000: This pull request references ROSAENG-63202 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 story to target the "5.0.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. |
openshift-ci-robot
added
the
jira/valid-reference
|
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:
WalkthroughThe change adds a coverage-instrumented operator image, setup and collection steps, and a weekly ROSA STS E2E coverage workflow. The workflow patches the operator deployment, runs tests, collects Go coverage data, and generates reports. ChangesROSA operator coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ScheduledWorkflow
participant CoverageSetup
participant OperatorDeployment
participant E2ETests
participant CoverageCollect
participant OperatorPod
ScheduledWorkflow->>CoverageSetup: configure coverage
CoverageSetup->>OperatorDeployment: patch image and coverage volume
OperatorDeployment-->>CoverageSetup: report rollout completion
ScheduledWorkflow->>E2ETests: run Route Monitor E2E tests
ScheduledWorkflow->>CoverageCollect: collect coverage
CoverageCollect->>OperatorPod: send SIGTERM and copy coverage files
CoverageCollect-->>ScheduledWorkflow: generate coverage reports
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dustman9000 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: 1
🧹 Nitpick comments (4)
ci-operator/config/openshift/route-monitor-operator/openshift-route-monitor-operator-master.yaml (1)
21-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the UBI Minimal base tag instead of
:latest.The coverage image stage uses
registry.access.redhat.com/ubi9/ubi-minimal:latest. A floating tag can change unexpectedly and break or alter this CI image's behavior without a corresponding code change in this repository.♻️ Proposed fix to pin the base image tag
- FROM registry.access.redhat.com/ubi9/ubi-minimal:latest + FROM registry.access.redhat.com/ubi9/ubi-minimal:9.4🤖 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 `@ci-operator/config/openshift/route-monitor-operator/openshift-route-monitor-operator-master.yaml` around lines 21 - 31, Update the coverage image stage in the dockerfile_literal for operator-coverage to replace the floating ubi-minimal:latest base image with a specific immutable UBI9 minimal version tag. Keep the builder stage and remaining image setup unchanged.ci-operator/step-registry/rosa/operator/coverage-setup/rosa-operator-coverage-setup-commands.sh (2)
17-18: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
OPERATOR_NAMEbefore deriving namespace and deployment name.This step does not check that
OPERATOR_NAMEis non-empty before derivingOPERATOR_NAMESPACEandOPERATOR_DEPLOYMENT_NAME.rosa-operator-install-commands.shvalidatesOPERATOR_NAMEbefore use. IfOPERATOR_NAMEis empty, this step silently targets namespaceopenshift-and an empty deployment name, producing an unclearoc patchfailure instead of a clear error message.🛡️ Proposed fix to validate OPERATOR_NAME
+if [[ -z "${OPERATOR_NAME:-}" ]]; then + log "ERROR: OPERATOR_NAME is required" + exit 1 +fi + OPERATOR_NAMESPACE="${OPERATOR_NAMESPACE:-openshift-${OPERATOR_NAME}}" OPERATOR_DEPLOYMENT_NAME="${OPERATOR_DEPLOYMENT_NAME:-${OPERATOR_NAME}}"Based on learnings, this mirrors the upstream contract in
ci-operator/step-registry/rosa/operator/install/rosa-operator-install-commands.sh:30-47, which validatesOPERATOR_NAME,OPERATOR_PKO_IMAGE, andOPERATOR_IMAGEbefore deriving namespace/deployment defaults.🤖 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 `@ci-operator/step-registry/rosa/operator/coverage-setup/rosa-operator-coverage-setup-commands.sh` around lines 17 - 18, Validate that OPERATOR_NAME is non-empty before deriving OPERATOR_NAMESPACE and OPERATOR_DEPLOYMENT_NAME, matching the validation behavior in rosa-operator-install-commands.sh. Fail immediately with a clear error message when it is missing, and preserve the existing default derivation for valid values.
30-33: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider distinguishing "PKO absent" from "scale failed" for the PKO scale-down.
oc scale ... || trueswallows every failure equally, including RBAC errors or a transient API failure, not just "PKO is not installed." If scaling silently fails for a real reason, PKO's reconciler can revert the coverage patch applied afterward, causing intermittent E2E flakiness with no visible signal in the logs.🤖 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 `@ci-operator/step-registry/rosa/operator/coverage-setup/rosa-operator-coverage-setup-commands.sh` around lines 30 - 33, Update the PKO scale-down block around the oc scale command to distinguish a missing package-operator-manager deployment from genuine scaling failures. Ignore only the explicit absent-resource case; for RBAC, API, or other failures, emit a visible log signal and preserve the failure rather than unconditionally swallowing it, while keeping the subsequent rollout handling aligned with whether scaling succeeded.ci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-commands.sh (1)
39-39: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the fixed
sleep 15with a poll loop.A fixed 15-second sleep after
SIGTERMis a race: it can be too short under load (partial coverage data copied) or unnecessarily slow otherwise. Poll for pod readiness/exit or for the presence of coverage files instead of a fixed delay.♻️ Proposed fix using a bounded poll loop
-log "Waiting for coverage data to flush" -sleep 15 +log "Waiting for coverage data to flush" +for _ in $(seq 1 15); do + if oc exec -n "${OPERATOR_NAMESPACE}" "${POD}" -- test -f /tmp/e2e-cover/*.done 2>/dev/null; then + break + fi + sleep 1 +done🤖 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 `@ci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-commands.sh` at line 39, Replace the fixed sleep after SIGTERM with a bounded polling loop that waits for the target pod to exit/become ready or for the expected coverage files to appear. Preserve the subsequent coverage collection flow, and include a timeout so the script cannot wait indefinitely.
🤖 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
`@ci-operator/step-registry/rosa/operator/coverage-setup/rosa-operator-coverage-setup-commands.sh`:
- Around line 37-42: Update the JSON patch in the coverage setup command to
create spec.template.spec.volumes and the target container’s volumeMounts arrays
when they are absent before appending entries, while preserving existing arrays
when present. Resolve the target container by name rather than assuming
containers/0, and apply the GOCOVERDIR environment update to that same
container.
---
Nitpick comments:
In
`@ci-operator/config/openshift/route-monitor-operator/openshift-route-monitor-operator-master.yaml`:
- Around line 21-31: Update the coverage image stage in the dockerfile_literal
for operator-coverage to replace the floating ubi-minimal:latest base image with
a specific immutable UBI9 minimal version tag. Keep the builder stage and
remaining image setup unchanged.
In
`@ci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-commands.sh`:
- Line 39: Replace the fixed sleep after SIGTERM with a bounded polling loop
that waits for the target pod to exit/become ready or for the expected coverage
files to appear. Preserve the subsequent coverage collection flow, and include a
timeout so the script cannot wait indefinitely.
In
`@ci-operator/step-registry/rosa/operator/coverage-setup/rosa-operator-coverage-setup-commands.sh`:
- Around line 17-18: Validate that OPERATOR_NAME is non-empty before deriving
OPERATOR_NAMESPACE and OPERATOR_DEPLOYMENT_NAME, matching the validation
behavior in rosa-operator-install-commands.sh. Fail immediately with a clear
error message when it is missing, and preserve the existing default derivation
for valid values.
- Around line 30-33: Update the PKO scale-down block around the oc scale command
to distinguish a missing package-operator-manager deployment from genuine
scaling failures. Ignore only the explicit absent-resource case; for RBAC, API,
or other failures, emit a visible log signal and preserve the failure rather
than unconditionally swallowing it, while keeping the subsequent rollout
handling aligned with whether scaling succeeded.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 58bc250e-dd51-4a45-8700-bdfc6a4fd06d
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/route-monitor-operator/openshift-route-monitor-operator-master-periodics.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (7)
ci-operator/config/openshift/route-monitor-operator/openshift-route-monitor-operator-master.yamlci-operator/step-registry/rosa/operator/coverage-collect/OWNERSci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-commands.shci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-ref.yamlci-operator/step-registry/rosa/operator/coverage-setup/OWNERSci-operator/step-registry/rosa/operator/coverage-setup/rosa-operator-coverage-setup-commands.shci-operator/step-registry/rosa/operator/coverage-setup/rosa-operator-coverage-setup-ref.yaml
| oc patch deployment "${OPERATOR_DEPLOYMENT_NAME}" -n "${OPERATOR_NAMESPACE}" --type=json -p "[ | ||
| {\"op\": \"replace\", \"path\": \"/spec/template/spec/containers/0/image\", \"value\": \"${COVERAGE_IMAGE}\"}, | ||
| {\"op\": \"add\", \"path\": \"/spec/template/spec/containers/0/env/-\", \"value\": {\"name\": \"GOCOVERDIR\", \"value\": \"/tmp/e2e-cover\"}}, | ||
| {\"op\": \"add\", \"path\": \"/spec/template/spec/volumes/-\", \"value\": {\"name\": \"coverage-data\", \"emptyDir\": {}}}, | ||
| {\"op\": \"add\", \"path\": \"/spec/template/spec/containers/0/volumeMounts/-\", \"value\": {\"name\": \"coverage-data\", \"mountPath\": \"/tmp/e2e-cover\"}} | ||
| ]" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect operator deployment manifests for container count and env presence.
fd -e yaml -e yml . | xargs rg -l 'kind:\s*Deployment' 2>/dev/null | xargs rg -n -A5 -B2 'containers:|env:' 2>/dev/null | head -100Repository: openshift/release
Length of output: 12571
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target step script ---'
target=$(fd -t f 'rosa-operator-coverage-setup-commands\.sh$' . | head -1)
if [[ -n "${target}" ]]; then
cat -n "${target}"
fi
printf '%s\n' '--- related coverage and route-monitor references ---'
rg -n -i -C 3 'route-monitor|coverage-setup|OPERATOR_DEPLOYMENT_NAME|COVERAGE_IMAGE' ci-operator 2>/dev/null | head -240
printf '%s\n' '--- deployment manifests mentioning route-monitor ---'
rg -l -i 'route-monitor' --glob '*.yaml' --glob '*.yml' . 2>/dev/null | head -100Repository: openshift/release
Length of output: 42799
🌐 Web query:
openshift route-monitor-operator Deployment containers env manifest
💡 Result:
The Route Monitor Operator uses specific environment variables and command-line flags to configure its container, primarily defined within the Deployment manifest at config/manager/manager.yaml [1]. The controller-manager container runs with the following command-line arguments, which leverage environment variable expansion [1]: - --zap-log-level=debug - --blackbox-image=$(BLACKBOX_IMAGE) - --blackbox-namespace=$(BLACKBOX_NAMESPACE) - --probe-api-url=$(PROBE_API_URL) - --oidc-client-id=$(OIDC_CLIENT_ID) - --oidc-client-secret=$(OIDC_CLIENT_SECRET) - --oidc-issuer-url=$(OIDC_ISSUER_URL) - --only-public-clusters=$(ONLY_PUBLIC_CLUSTERS) - --skip-infrastructure-health-check=$(SKIP_INFRASTRUCTURE_HEALTH_CHECK) The env section in the manifest specifies the following primary environment variables [1]: - LOG_LEVEL: Set to 1 (debug level). - BLACKBOX_IMAGE: Defaults to a specific quay.io/prometheus/blackbox-exporter image digest. - BLACKBOX_NAMESPACE: Uses the Kubernetes downward API (fieldRef) to dynamically set the value to the pod's namespace. While these variables are defined in the Deployment, the operator also supports dynamic configuration through a route-monitor-operator-config ConfigMap in the openshift-route-monitor-operator namespace [2][3][4]. Values defined in this ConfigMap take precedence over the command-line flags [4]. Supported fields in the ConfigMap include probe-api-url, probe-tenant, oidc-client-id, oidc-client-secret, oidc-issuer-url, only-public-clusters, and dynatrace-enabled [3][4].
Citations:
- 1: https://github.com/openshift/route-monitor-operator/blob/master/config/manager/manager.yaml
- 2: https://github.com/openshift/route-monitor-operator
- 3: https://pkg.go.dev/github.com/openshift/route-monitor-operator
- 4: https://github.com/openshift/route-monitor-operator/blob/master/README.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- route-monitor coverage-step usage ---'
rg -n -C 6 'rosa-operator-coverage-setup|route-monitor-operator' \
ci-operator/config/openshift/route-monitor-operator \
ci-operator/jobs/openshift/route-monitor-operator \
ci-operator/step-registry/rosa/operator 2>/dev/null | head -240
printf '%s\n' '--- route-monitor Deployment manifest ---'
curl -fsSL \
https://raw.githubusercontent.com/openshift/route-monitor-operator/master/config/manager/manager.yaml \
| nl -ba | sed -n '1,220p'Repository: openshift/release
Length of output: 31749
🏁 Script executed:
#!/bin/bash
set -euo pipefail
curl -fsSL \
https://raw.githubusercontent.com/openshift/route-monitor-operator/master/config/manager/manager.yaml \
| sed -n '1,220p'Repository: openshift/release
Length of output: 3861
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import urllib.request
try:
import yaml
except ImportError:
raise SystemExit("PyYAML is unavailable")
url = "https://raw.githubusercontent.com/openshift/route-monitor-operator/master/config/manager/manager.yaml"
with urllib.request.urlopen(url) as response:
deployment = yaml.safe_load(response.read())
pod_spec = deployment["spec"]["template"]["spec"]
containers = pod_spec["containers"]
manager = containers[0]
print(f"container_count={len(containers)}")
print(f"manager_name={manager.get('name')}")
print(f"manager_has_env={'env' in manager}")
print(f"pod_has_volumes={'volumes' in pod_spec}")
print(f"manager_has_volumeMounts={'volumeMounts' in manager}")
for path, parent in (
("/spec/template/spec/containers/0/env/-", manager),
("/spec/template/spec/volumes/-", pod_spec),
("/spec/template/spec/containers/0/volumeMounts/-", manager),
):
key = path.rsplit("/", 2)[-2]
value = parent.get(key)
print(f"{path}: parent_exists={key in parent}, parent_is_array={isinstance(value, list)}")
PYRepository: openshift/release
Length of output: 177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
import urllib.request
url = "https://raw.githubusercontent.com/openshift/route-monitor-operator/master/config/manager/manager.yaml"
manifest = urllib.request.urlopen(url).read().decode().splitlines()
container_items = [
line for line in manifest
if re.match(r"^ - ", line)
]
env_present = any(re.match(r"^ env:\s*$", line) for line in manifest)
pod_volumes_present = any(re.match(r"^ volumes:\s*$", line) for line in manifest)
volume_mounts_present = any(re.match(r"^ volumeMounts:\s*$", line) for line in manifest)
print(f"container_items={len(container_items)}")
print(f"container_0_env_present={env_present}")
print(f"pod_volumes_present={pod_volumes_present}")
print(f"container_0_volumeMounts_present={volume_mounts_present}")
assert len(container_items) == 1
assert env_present
assert not pod_volumes_present
assert not volume_mounts_present
PYRepository: openshift/release
Length of output: 2441
Make the JSON patch handle missing arrays. The route-monitor Deployment has one container and an env array, but it has no volumes or volumeMounts arrays. The add operations using /- therefore fail. Create these arrays when absent, and avoid relying on container index 0 for reusable deployments.
🤖 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
`@ci-operator/step-registry/rosa/operator/coverage-setup/rosa-operator-coverage-setup-commands.sh`
around lines 37 - 42, Update the JSON patch in the coverage setup command to
create spec.template.spec.volumes and the target container’s volumeMounts arrays
when they are absent before appending entries, while preserving existing arrays
when present. Resolve the target container by name rather than assuming
containers/0, and apply the GOCOVERDIR environment update to that same
container.
There was a problem hiding this comment.
Good catch. The Deployment may not have existing volumes/volumeMounts arrays. Will use strategic merge patch instead of JSON patch to handle missing arrays gracefully.
There was a problem hiding this comment.
@dustman9000, acknowledged. A strategic merge patch is appropriate here. It can add the missing volumes and volumeMounts fields without failing when the arrays do not exist. Please also target the operator container by name so the reusable step does not depend on containers[0].
You are interacting with an AI system.
7442c87 to
310592e
Compare
|
/pj-rehearse periodic-ci-openshift-route-monitor-operator-master-rosa-sts-e2e-coverage |
|
@dustman9000: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-route-monitor-operator-master-rosa-sts-e2e-coverage |
|
@dustman9000: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-route-monitor-operator-master-rosa-sts-e2e-coverage |
|
@dustman9000: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
310592e to
337f36f
Compare
|
/pj-rehearse periodic-ci-openshift-route-monitor-operator-master-rosa-sts-e2e-coverage |
|
@dustman9000: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
337f36f to
0e5f71c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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
`@ci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-commands.sh`:
- Line 97: Update the covered-function count in the Python print statement to
iterate over funcs and sum each function’s coverage predicate, replacing the
undefined f reference while preserving the existing output format.
- Around line 77-85: Update the coverage parser’s file extraction in the loop
building funcs so locations like file.go:42: are split at the final line-number
suffix before assigning the JSON file field. Preserve the function name and
coverage parsing, and ensure file contains only the source path.
- Around line 3-4: Update the coverage collection script’s shell options to use
set -euo pipefail, and guard only the fallback grep pipeline so expected
no-match behavior does not terminate the script. Remove || true from Go and
Python report generation, validate that covmeta* and covcounters* inputs exist,
and verify all four declared report files before logging successful generation.
Remove best_effort: true from the associated step configuration so these
failures fail the job.
- Line 6: Update the TERM trap in the coverage collection script so cleanup
tolerates child processes that have already exited: ensure a failed kill does
not prevent waiting on the captured child PIDs, then explicitly exit with status
143 to prevent continuing into oc cp and report generation.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6dffa174-eaea-4d6f-b652-c2e654fb63ba
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/route-monitor-operator/openshift-route-monitor-operator-master-periodics.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (9)
ci-operator/config/openshift/route-monitor-operator/openshift-route-monitor-operator-master.yamlci-operator/step-registry/rosa/operator/coverage-collect/OWNERSci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-commands.shci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-ref.metadata.jsonci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-ref.yamlci-operator/step-registry/rosa/operator/coverage-setup/OWNERSci-operator/step-registry/rosa/operator/coverage-setup/rosa-operator-coverage-setup-commands.shci-operator/step-registry/rosa/operator/coverage-setup/rosa-operator-coverage-setup-ref.metadata.jsonci-operator/step-registry/rosa/operator/coverage-setup/rosa-operator-coverage-setup-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- ci-operator/step-registry/rosa/operator/coverage-setup/rosa-operator-coverage-setup-ref.metadata.json
- ci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-ref.metadata.json
- ci-operator/step-registry/rosa/operator/coverage-collect/OWNERS
- ci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-ref.yaml
- ci-operator/step-registry/rosa/operator/coverage-setup/rosa-operator-coverage-setup-ref.yaml
- ci-operator/config/openshift/route-monitor-operator/openshift-route-monitor-operator-master.yaml
- ci-operator/step-registry/rosa/operator/coverage-setup/OWNERS
- ci-operator/step-registry/rosa/operator/coverage-setup/rosa-operator-coverage-setup-commands.sh
| set -o nounset | ||
| set -o pipefail |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd -t f -g 'rosa-operator-coverage-collect-commands.sh' | head -n 1)"
printf '%s\n' "FILE=$file"
wc -l "$file"
cat -n "$file"
printf '%s\n' '--- related coverage step files ---'
fd -t f . ci-operator/step-registry/rosa/operator/coverage-collect ci-operator/step-registry/rosa/operator 2>/dev/null | sort
printf '%s\n' '--- references to the command script and coverage outputs ---'
rg -n -S 'rosa-operator-coverage-collect|covmeta|covcounters|Coverage reports generated|coverage' ci-operator/step-registry/rosa ci-operator/step-registry 2>/dev/null | head -n 300Repository: openshift/release
Length of output: 25307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- step contract ---'
cat -n ci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-ref.yaml
printf '%s\n' '--- coverage setup producer ---'
cat -n ci-operator/step-registry/rosa/operator/coverage-setup/rosa-operator-coverage-setup-commands.sh
printf '%s\n' '--- workflow/job references ---'
rg -n -S 'rosa-operator-coverage-(setup|collect)|coverage-collect|coverage-setup' . --glob '!vendor/**' --glob '!node_modules/**' | head -n 250
printf '%s\n' '--- shell semantics: assignment, pipelines, and guarded conditions ---'
bash -c 'set -euo pipefail; x=$(printf x | grep z | head -1); printf "unreached: %s\\n" "$x"' >/tmp/strict-test.out 2>&1; status=$?; printf 'unguarded_assignment_status=%s output=%s\n' "$status" "$(cat /tmp/strict-test.out)"
bash -c 'set -euo pipefail; if ! x=$(printf x | grep z | head -1); then :; fi; printf "reached\\n"' >/tmp/guarded-test.out 2>&1; status=$?; printf 'guarded_assignment_status=%s output=%s\n' "$status" "$(cat /tmp/guarded-test.out)"
printf '%s\n' '--- required output names and producer commands ---'
python3 - <<'PY'
from pathlib import Path
p = Path("ci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-commands.sh")
s = p.read_text()
for needle in ("covmeta", "covcounters", "coverage.out", "coverage-func.txt", "coverage-report.json", "coverage.html"):
print(f"{needle}: {s.count(needle)} occurrence(s)")
PYRepository: openshift/release
Length of output: 5926
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- workflow context ---'
cat -n ci-operator/config/openshift/route-monitor-operator/openshift-route-monitor-operator-master.yaml | sed -n '180,230p'
printf '%s\n' '--- resilient shell semantics probes ---'
probe() {
name="$1"
shift
set +e
output="$("$@" 2>&1)"
status=$?
set -e
printf '%s_status=%s output=%q\n' "$name" "$status" "$output"
}
probe unguarded_assignment bash -c 'set -euo pipefail; x=$(printf x | grep z | head -1); printf "reached:%s\n" "$x"'
probe guarded_assignment bash -c 'set -euo pipefail; if ! x=$(printf x | grep z | head -1); then :; fi; printf "reached\n"'
probe assignment_without_pipefail bash -c 'set -eu; x=$(printf x | grep z | head -1); printf "reached:%s\n" "$x"'
printf '%s\n' '--- coverage file naming in repository ---'
rg -n -S 'covmeta|covcounters|GOCOVERDIR|go tool covdata textfmt' . --glob '!vendor/**' --glob '!node_modules/**' | head -n 300
printf '%s\n' '--- best-effort step documentation and examples ---'
rg -n -S 'best_effort:' ci-operator/config ci-operator/step-registry | head -n 120
rg -n -S 'best effort|best_effort' ci-operator/step-registry/README.md ci-operator/config/README.md docs 2>/dev/null | head -n 120Repository: openshift/release
Length of output: 17118
🌐 Web query:
OpenShift ci-operator best_effort step semantics Prow job
💡 Result:
In OpenShift CI (ci-operator), the best_effort semantic is used to define steps that should not cause the overall job to fail even if the step itself fails [1][2]. This mechanism is primarily utilized for non-critical tasks such as gathering debugging artifacts (e.g., must-gather logs), cleaning up resources, or releasing shared infrastructure (e.g., ephemeral namespaces) [3][4][2]. Key characteristics of best_effort steps include: - Failure propagation: If a step marked with best_effort: true fails, the CI job phase proceeds as if the step had succeeded, preventing the overall job result from being impacted by that step's failure [1][2]. - Configuration: To use best_effort for a post step, the best_effort field must be set to true within the individual step configuration [1]. Additionally, for workflows or jobs, the setting allow_best_effort_post_steps must be enabled at the workflow or job level [1]. - Prerequisites: Steps configured as best_effort must typically include a defined timeout [1]. - Typical use cases: They are frequently used in post-execution phases to ensure cleanup or data collection occurs reliably regardless of the success or failure of preceding test steps [3][4][5]. For example, when releasing ephemeral namespaces, the release step is marked as best_effort to ensure the namespace is returned to the pool even if the preceding tests failed [4][5]. Similarly, gathering network or console logs is often handled via best_effort steps to ensure that diagnostic data is collected without marking the entire CI job as failed should the collection tool itself encounter an issue [6][7].
Citations:
- 1: https://github.com/openshift/ci-docs/blob/main/content/en/architecture/step-registry.md
- 2: https://github.com/openshift-eng/ai-helpers/blob/main/plugins/ci/skills/prow-job-analysis/references/ci-infrastructure-changes.md
- 3: https://docs.ci.openshift.org/architecture/timeouts/
- 4: https://docs.ci.openshift.org/how-tos/ephemeral-namespace-workflow/
- 5: https://github.com/openshift/ci-docs/blob/main/content/en/how-tos/ephemeral-namespace-workflow.md
- 6: https://github.com/openshift/release/blob/main/ci-operator/step-registry/gather/network/gather-network-ref.yaml
- 7: https://github.com/openshift/release/blob/main/ci-operator/step-registry/gather/aws-console/gather-aws-console-ref.yaml
Fail when coverage collection or report generation fails.
The script exits 0 when no pod or coverage data exists. It also masks failures from Go and Python report generation with || true. It can log Coverage reports generated while declared artifacts are missing or incomplete. Use set -euo pipefail, guard the fallback grep pipeline, validate covmeta* and covcounters*, and check all four report files. Because this ref is best_effort: true, remove that setting if coverage must fail the job.
🤖 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
`@ci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-commands.sh`
around lines 3 - 4, Update the coverage collection script’s shell options to use
set -euo pipefail, and guard only the fallback grep pipeline so expected
no-match behavior does not terminate the script. Remove || true from Go and
Python report generation, validate that covmeta* and covcounters* inputs exist,
and verify all four declared report files before logging successful generation.
Remove best_effort: true from the associated step configuration so these
failures fail the job.
Source: Coding guidelines
| set -o nounset | ||
| set -o pipefail | ||
|
|
||
| trap 'CHILDREN=$(jobs -p); if test -n "${CHILDREN}"; then kill ${CHILDREN} && wait; fi' TERM |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd -i -t f 'rosa-operator-coverage-collect-commands\.sh$' . | head -n 1)"
printf '%s\n' "FILE=$file"
cat -n "$file"
printf '%s\n' '--- related references ---'
rg -n -C 4 'coverage-collect|CHILDREN|trap .*TERM|sleep|oc cp' ci-operator/step-registryRepository: openshift/release
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='ci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-commands.sh'
printf '%s\n' '--- target script ---'
cat -n "$file"
printf '%s\n' '--- target references only ---'
rg -n -C 3 'rosa-operator-coverage-collect-commands|coverage-collect|CHILDREN|trap' \
ci-operator/step-registry/rosa ci-operator/step-registry/upi/conf/vsphere ci-operator/step-registry/upi/conf/gcp
printf '%s\n' '--- shell availability ---'
bash --version | head -n 1Repository: openshift/release
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os
import signal
import subprocess
import sys
import tempfile
import textwrap
import time
script = r'''
set -euo pipefail
trap 'CHILDREN=$(jobs -p); if test -n "${CHILDREN}"; then kill ${CHILDREN} && wait; fi' TERM
printf 'before\n'
sleep 30 &
child=$!
printf 'child=%s\n' "$child"
sleep 30
printf 'after-sleep\n'
exit 0
'''
with tempfile.NamedTemporaryFile("w", delete=False) as f:
f.write(script)
path = f.name
p = subprocess.Popen(["bash", path], stdout=subprocess.PIPE,
stderr=subprocess.STDOUT, text=True)
try:
time.sleep(0.3)
os.kill(p.pid, signal.SIGTERM)
output, _ = p.communicate(timeout=3)
print(f"exit={p.returncode}")
print(output, end="")
except subprocess.TimeoutExpired:
p.kill()
output, _ = p.communicate()
print("timeout: shell continued after SIGTERM")
print(output, end="")
finally:
os.unlink(path)
PYRepository: openshift/release
Length of output: 209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os
import signal
import subprocess
import tempfile
import textwrap
import time
def run(body, signal_group):
with tempfile.NamedTemporaryFile("w", delete=False) as f:
f.write(body)
path = f.name
p = subprocess.Popen(["bash", path], stdout=subprocess.PIPE,
stderr=subprocess.STDOUT, text=True,
start_new_session=True)
try:
time.sleep(0.2)
if signal_group:
os.killpg(p.pid, signal.SIGTERM)
else:
os.kill(p.pid, signal.SIGTERM)
try:
out, _ = p.communicate(timeout=2)
return f"exit={p.returncode}\n{out}"
except subprocess.TimeoutExpired:
p.kill()
out, _ = p.communicate()
return f"timeout\n{out}"
finally:
os.unlink(path)
trap = "trap 'CHILDREN=$(jobs -p); if test -n \"${CHILDREN}\"; then kill ${CHILDREN} && wait; fi' TERM"
body = f"""set -o nounset
set -o pipefail
{trap}
printf 'before\\n'
sleep 10 &
printf 'background-started\\n'
sleep 10
printf 'after-sleep\\n'
"""
print("--- signal shell only ---")
print(run(body, False))
print("--- signal process group ---")
print(run(body, True))
jobs_probe = r'''set -o nounset
set -o pipefail
trap 'CHILDREN=$(jobs -p); printf "trap-children=%s\n" "$CHILDREN"; if test -n "${CHILDREN}"; then kill ${CHILDREN} && wait; fi; printf "trap-end\n"' TERM
sleep 0.1 &
child=$!
sleep 0.3
printf 'before-term child=%s jobs=%s\n' "$child" "$(jobs -p)"
kill -TERM "$BASHPID"
printf 'after-term\n'
'''
print("--- completed-child behavior ---")
print(run(jobs_probe, False))
PYRepository: openshift/release
Length of output: 545
Exit with status 143 from the TERM trap.
After TERM interrupts sleep, the trap can return and the script continues with oc cp and report generation. Make cleanup tolerate already-exited child PIDs so a failed kill does not skip wait, then call exit 143.
🤖 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
`@ci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-commands.sh`
at line 6, Update the TERM trap in the coverage collection script so cleanup
tolerates child processes that have already exited: ensure a failed kill does
not prevent waiting on the captured child PIDs, then explicitly exit with status
143 to prevent continuing into oc cp and report generation.
| for line in lines[:-1]: | ||
| parts = line.strip().rsplit(None, 1) | ||
| if len(parts) == 2: | ||
| file_func = parts[0].rsplit(None, 1) | ||
| if len(file_func) == 2: | ||
| funcs.append({ | ||
| 'file': file_func[0].strip(), | ||
| 'function': file_func[1].strip().rstrip(':'), | ||
| 'coverage': parts[1] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='ci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-commands.sh'
printf '%s\n' '--- target file ---'
cat -n "$file" | sed -n '1,130p'
printf '%s\n' '--- related coverage JSON references ---'
rg -n --glob '*.sh' --glob '*.py' --glob '*.json' --glob '*.yaml' --glob '*.yml' \
'rosa-operator-coverage|coverage.*json|funcs|function.*coverage|\"file\"' \
ci-operator 2>/dev/null | head -250Repository: openshift/release
Length of output: 11551
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- coverage step metadata ---'
cat -n ci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-ref.yaml
printf '%s\n' '--- Go tooling availability and output-format references ---'
if command -v go >/dev/null 2>&1; then
go version
goroot="$(go env GOROOT)"
rg -n -m 20 'total:|func.*coverage|cover -func|percentage' \
"$goroot/src/cmd/vendor/golang.org/x/tools/cover" \
"$goroot/src/cmd/cover" 2>/dev/null || true
else
printf '%s\n' 'go is unavailable'
fi
printf '%s\n' '--- deterministic parser probe ---'
python3 - <<'PY'
import json
sample = """example.com/operator/pkg/foo.go:42:\tReconcile\t75.0%
example.com/operator/pkg/foo.go:58:\thelper\t0.0%
total:\t\t(statements)\t60.0%
"""
lines = sample.splitlines(keepends=True)
funcs = []
for line in lines[:-1]:
parts = line.strip().rsplit(None, 1)
if len(parts) == 2:
file_func = parts[0].rsplit(None, 1)
if len(file_func) == 2:
funcs.append({
"file": file_func[0].strip(),
"function": file_func[1].strip().rstrip(":"),
"coverage": parts[1],
})
print(json.dumps(funcs, indent=2))
assert funcs[0]["file"] == "example.com/operator/pkg/foo.go:42:"
assert funcs[0]["file"] != "example.com/operator/pkg/foo.go"
print("The current parser retains the line number and trailing colon.")
PYRepository: openshift/release
Length of output: 3467
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Go cover function-output implementation ---'
sed -n '30,95p' "$(go env GOROOT)/src/cmd/cover/func.go"
printf '%s\n' '--- current and proposed field extraction ---'
python3 - <<'PY'
sample = "example.com/operator/pkg/foo.go:42:\tReconcile\t75.0%\n"
line = sample.strip()
parts = line.rsplit(None, 1)
file_func = parts[0].rsplit(None, 1)
current = file_func[0].strip()
location = file_func[0].strip().rstrip(":")
file_path, _ = location.rsplit(":", 1)
print(f"current file field: {current!r}")
print(f"proposed file field: {file_path!r}")
assert current == "example.com/operator/pkg/foo.go:42:"
assert file_path == "example.com/operator/pkg/foo.go"
PYRepository: openshift/release
Length of output: 2077
Strip the line number from the JSON file field.
go tool cover -func emits locations such as file.go:42:. The current parser stores this location as the file value. Split the final line number before writing the JSON field.
🤖 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
`@ci-operator/step-registry/rosa/operator/coverage-collect/rosa-operator-coverage-collect-commands.sh`
around lines 77 - 85, Update the coverage parser’s file extraction in the loop
building funcs so locations like file.go:42: are split at the final line-number
suffix before assigning the JSON file field. Preserve the function name and
coverage parsing, and ensure file contains only the source path.
|
/pj-rehearse periodic-ci-openshift-route-monitor-operator-master-rosa-sts-e2e-coverage |
|
@dustman9000: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
0e5f71c to
b8a73c3
Compare
|
/pj-rehearse periodic-ci-openshift-route-monitor-operator-master-rosa-sts-e2e-coverage |
|
@dustman9000: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
b8a73c3 to
b41485f
Compare
|
/pj-rehearse periodic-ci-openshift-route-monitor-operator-master-rosa-sts-e2e-coverage |
|
@dustman9000: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
b41485f to
3bb630b
Compare
|
/pj-rehearse periodic-ci-openshift-route-monitor-operator-master-rosa-sts-e2e-coverage |
|
@dustman9000: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Add reusable step-registry refs for coverage setup and collection: - rosa-operator-coverage-setup: patches Deployment with coverage image, GOCOVERDIR, emptyDir volume; scales down PKO to prevent revert - rosa-operator-coverage-collect: SIGTERMs operator to flush coverage, copies data via oc cp, generates Go profile, JSON, and HTML reports Add operator-coverage dockerfile_literal and rosa-sts-e2e-coverage weekly periodic for RMO as the reference implementation. Coverage artifacts: - coverage.out (Go profile) - coverage-report.json (JSON for dashboard) - coverage.html (HTML for Prow viewer) - coverage-func.txt (per-function text)
3bb630b to
32ebe5d
Compare
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/pj-rehearse periodic-ci-openshift-route-monitor-operator-master-rosa-sts-e2e-coverage |
|
@dustman9000: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@dustman9000: 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. |
|
/pj-rehearse ack |
|
@dustman9000: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
2 participants
Summary
Add Go binary coverage instrumentation to SRE operator Prow e2e tests to establish baseline coverage and identify gaps.
New step-registry refs:
rosa-operator-coverage-setup: Patches operator Deployment with coverage-instrumented image, GOCOVERDIR env var, and emptyDir volume. Scales down PKO to prevent reconciliation.rosa-operator-coverage-collect: SIGTERMs the operator to flush coverage data, copies via oc cp, generates reports in Go profile, JSON (for dashboard), and HTML (for Prow viewer) formats.RMO reference implementation:
operator-coveragedockerfile_literal builds withgo build -coverrosa-sts-e2e-coverageweekly periodic (Sunday 6am UTC) runs the full e2e with coverage collectionCoverage artifacts in Prow:
coverage.out- standard Go coverage profilecoverage-report.json- JSON for rosa-eng-dashboardcoverage.html- HTML reportcoverage-func.txt- per-function textAfter RMO validates, will add coverage jobs for CAMO, AVO, MUO, OAO, SFO.
Jira: https://redhat.atlassian.net/browse/ROSAENG-63202
Test plan
Summary by CodeRabbit
GOCOVERDIR, patch the operator Deployment, collect coverage data, and generate Go reports.operator-coverageimage and publishescoverage.out, JSON, HTML, and per-function coverage reports.