NO-JIRA: Add E2E coverage reporting for secrets-store-csi-driver-operator#79231
NO-JIRA: Add E2E coverage reporting for secrets-store-csi-driver-operator#79231PillaiManish wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@PillaiManish: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: PillaiManish The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/pj-rehearse max |
|
@PillaiManish: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a coverage-enabled operator image and CI steps to the ChangesCoverage Instrumentation and Collection for FIPS E2E Tests
Sequence DiagramsequenceDiagram
participant CI as CI Job
participant API as OpenShift API
participant Pod as Operator Pod
participant Codecov as Codecov
CI->>API: setup-coverage: patch CSV -> use COVERAGE_IMAGE, add GOCOVERDIR
API->>Pod: rollout new operator deployment
Pod-->>API: readiness
CI->>API: run operator-e2e-fips tests
API->>Pod: tests exercise operator (writes coverage to /tmp/e2e-cover)
CI->>Pod: collect-coverage: kubectl exec SIGTERM (flush coverage)
Pod-->>API: restart/ready
CI->>Pod: kubectl cp /tmp/e2e-cover -> workspace
CI->>CI: go tool covdata convert -> text/percent
CI->>Codecov: verify uploader SHA256
alt CODECOV_TOKEN present
CI->>Codecov: upload coverage profile
else
CI->>CI: preserve local coverage files
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
`@ci-operator/config/openshift/secrets-store-csi-driver-operator/openshift-secrets-store-csi-driver-operator-main.yaml`:
- Around line 225-228: The JSON patch that appends GOCOVERDIR to the container
env can fail if the container has no env array; modify the patching logic around
the oc patch csv "${csv}" -n "${NAMESPACE}" call so you first ensure the target
container has an env array before using the add with path ".../env/-". Implement
this by either (a) issuing a preparatory patch (e.g., an oc patch --type=merge
or a JSON patch that creates
/spec/install/spec/deployments/0/spec/template/spec/containers/0/env as an empty
array if missing) and then run the existing add for the GOCOVERDIR entry, or (b)
stop using a single JSON-patch append and instead perform a
merge/strategic-merge patch that injects the env entry (including COVERAGE_IMAGE
and GOCOVERDIR_PATH) in one safe operation so it works whether env exists or
not.
- Around line 142-151: The current flow sends SIGTERM to PID 1 then waits for
the pod to restart and runs oc cp from ${GOCOVERDIR_PATH}, but that path is on
the container ephemeral filesystem so the coverage files are lost when the
container restarts; update the job to either (A) ensure ${GOCOVERDIR_PATH} is
backed by a persistent volume (e.g., mount an emptyDir or hostPath to the same
path in the pod spec) so files survive the restart, or (B) copy the coverage
files out of the running container before sending kill -TERM 1 (use oc cp from
${NAMESPACE}/${pod}:${GOCOVERDIR_PATH} to ${coverage_dir} prior to the oc exec
'kill -TERM 1'), referencing the variables GOCOVERDIR_PATH, coverage_dir, oc
exec, and oc cp in your changes.
🪄 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: dc595cc8-a074-4cb4-966d-d7c7cea09e3e
📒 Files selected for processing (1)
ci-operator/config/openshift/secrets-store-csi-driver-operator/openshift-secrets-store-csi-driver-operator-main.yaml
| echo "Sending SIGTERM to flush coverage data (container will restart)..." | ||
| oc exec -n "${NAMESPACE}" "${pod}" -- /bin/sh -c 'kill -TERM 1' 2>/dev/null || true | ||
|
|
||
| echo "Waiting for container to restart and become ready..." | ||
| sleep 5 | ||
| oc wait pod "${pod}" -n "${NAMESPACE}" --for=condition=Ready --timeout=120s | ||
|
|
||
| echo "Copying coverage data from restarted container..." | ||
| mkdir -p "${coverage_dir}" | ||
| oc cp "${NAMESPACE}/${pod}:${GOCOVERDIR_PATH}/." "${coverage_dir}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a volume mount defined for the coverage directory in the operator deployment
# or in related CSV/deployment manifests
# Search for volume definitions related to e2e-cover in the repo
rg -n "e2e-cover|GOCOVERDIR" --type yaml -C3
# Check if emptyDir or any volume is configured for /tmp
ast-grep --pattern 'volumes:
$$$
emptyDir
$$$'Repository: openshift/release
Length of output: 8693
🏁 Script executed:
# Look at the full deployment definition in the CI config file
grep -n "volumes:" ci-operator/config/openshift/secrets-store-csi-driver-operator/openshift-secrets-store-csi-driver-operator-main.yaml
# Also check if there's any volumeMount defined
grep -n "volumeMounts:" ci-operator/config/openshift/secrets-store-csi-driver-operator/openshift-secrets-store-csi-driver-operator-main.yaml
# Check the exact lines around the deployment spec
sed -n '200,250p' ci-operator/config/openshift/secrets-store-csi-driver-operator/openshift-secrets-store-csi-driver-operator-main.yamlRepository: openshift/release
Length of output: 2048
🏁 Script executed:
# Look at lines 140-160 to see the post-step context
sed -n '140,160p' ci-operator/config/openshift/secrets-store-csi-driver-operator/openshift-secrets-store-csi-driver-operator-main.yaml
# Also check for any volumes definition in the whole file
grep -n "volumes:" ci-operator/config/openshift/secrets-store-csi-driver-operator/openshift-secrets-store-csi-driver-operator-main.yaml
# Check if there are volumeMounts anywhere
grep -n "volumeMounts:" ci-operator/config/openshift/secrets-store-csi-driver-operator/openshift-secrets-store-csi-driver-operator-main.yamlRepository: openshift/release
Length of output: 1000
🏁 Script executed:
# Get the full test-e2e-post step definition to see complete context
grep -n "test-e2e-post" ci-operator/config/openshift/secrets-store-csi-driver-operator/openshift-secrets-store-csi-driver-operator-main.yaml -A 100
# Also look for any emptyDir or volume definitions in the entire file
cat ci-operator/config/openshift/secrets-store-csi-driver-operator/openshift-secrets-store-csi-driver-operator-main.yaml | grep -i "emptydir\|volumes\|volumemount"Repository: openshift/release
Length of output: 43
🏁 Script executed:
# Let's read the entire CI config file to understand the full structure
wc -l ci-operator/config/openshift/secrets-store-csi-driver-operator/openshift-secrets-store-csi-driver-operator-main.yaml
# Read the file
cat ci-operator/config/openshift/secrets-store-csi-driver-operator/openshift-secrets-store-csi-driver-operator-main.yamlRepository: openshift/release
Length of output: 14132
Coverage data will be lost after sending SIGTERM to the container.
When SIGTERM is sent to PID 1, the Go runtime flushes coverage to /tmp/e2e-cover in the current container instance. However, /tmp is part of the container's ephemeral filesystem with no persistent volume backing it. When the container restarts, Kubernetes creates a new container instance with a fresh /tmp, discarding the old container's filesystem entirely. The subsequent oc cp will copy from the new container's empty /tmp/e2e-cover directory.
To fix this, use a persistent volume (emptyDir or hostPath) mounted at the coverage directory path, or refactor to collect coverage data before triggering the container restart.
🤖 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/secrets-store-csi-driver-operator/openshift-secrets-store-csi-driver-operator-main.yaml`
around lines 142 - 151, The current flow sends SIGTERM to PID 1 then waits for
the pod to restart and runs oc cp from ${GOCOVERDIR_PATH}, but that path is on
the container ephemeral filesystem so the coverage files are lost when the
container restarts; update the job to either (A) ensure ${GOCOVERDIR_PATH} is
backed by a persistent volume (e.g., mount an emptyDir or hostPath to the same
path in the pod spec) so files survive the restart, or (B) copy the coverage
files out of the running container before sending kill -TERM 1 (use oc cp from
${NAMESPACE}/${pod}:${GOCOVERDIR_PATH} to ${coverage_dir} prior to the oc exec
'kill -TERM 1'), referencing the variables GOCOVERDIR_PATH, coverage_dir, oc
exec, and oc cp in your changes.
| oc patch csv "${csv}" -n "${NAMESPACE}" --type=json -p "[ | ||
| {\"op\": \"replace\", \"path\": \"/spec/install/spec/deployments/0/spec/template/spec/containers/0/image\", \"value\": \"${COVERAGE_IMAGE}\"}, | ||
| {\"op\": \"add\", \"path\": \"/spec/install/spec/deployments/0/spec/template/spec/containers/0/env/-\", \"value\": {\"name\": \"GOCOVERDIR\", \"value\": \"${GOCOVERDIR_PATH}\"}} | ||
| ]" |
There was a problem hiding this comment.
JSON patch may fail if env array doesn't exist.
The "add" operation with path ...env/- assumes the env array already exists in the container spec. If the container has no existing environment variables, this patch will fail with "path not found" error.
Consider using a test-then-add approach or ensure the operator container always has at least one env var defined:
Suggested defensive approach
- oc patch csv "${csv}" -n "${NAMESPACE}" --type=json -p "[
- {\"op\": \"replace\", \"path\": \"/spec/install/spec/deployments/0/spec/template/spec/containers/0/image\", \"value\": \"${COVERAGE_IMAGE}\"},
- {\"op\": \"add\", \"path\": \"/spec/install/spec/deployments/0/spec/template/spec/containers/0/env/-\", \"value\": {\"name\": \"GOCOVERDIR\", \"value\": \"${GOCOVERDIR_PATH}\"}}
- ]"
+ # Check if env array exists, create if needed
+ has_env=$(oc get csv "${csv}" -n "${NAMESPACE}" -o jsonpath='{.spec.install.spec.deployments[0].spec.template.spec.containers[0].env}' 2>/dev/null)
+ if [ -z "${has_env}" ] || [ "${has_env}" = "null" ]; then
+ env_patch='{"op": "add", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/0/env", "value": [{"name": "GOCOVERDIR", "value": "'"${GOCOVERDIR_PATH}"'"}]}'
+ else
+ env_patch='{"op": "add", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/0/env/-", "value": {"name": "GOCOVERDIR", "value": "'"${GOCOVERDIR_PATH}"'"}}'
+ fi
+ oc patch csv "${csv}" -n "${NAMESPACE}" --type=json -p "[
+ {\"op\": \"replace\", \"path\": \"/spec/install/spec/deployments/0/spec/template/spec/containers/0/image\", \"value\": \"${COVERAGE_IMAGE}\"},
+ ${env_patch}
+ ]"🤖 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/secrets-store-csi-driver-operator/openshift-secrets-store-csi-driver-operator-main.yaml`
around lines 225 - 228, The JSON patch that appends GOCOVERDIR to the container
env can fail if the container has no env array; modify the patching logic around
the oc patch csv "${csv}" -n "${NAMESPACE}" call so you first ensure the target
container has an env array before using the add with path ".../env/-". Implement
this by either (a) issuing a preparatory patch (e.g., an oc patch --type=merge
or a JSON patch that creates
/spec/install/spec/deployments/0/spec/template/spec/containers/0/env as an empty
array if missing) and then run the existing add for the GOCOVERDIR entry, or (b)
stop using a single JSON-patch append and instead perform a
merge/strategic-merge patch that injects the env entry (including COVERAGE_IMAGE
and GOCOVERDIR_PATH) in one safe operation so it works whether env exists or
not.
…ator Add coverage-instrumented image build and CI steps to collect E2E test coverage data and upload to Codecov. Co-authored-by: Cursor <cursoragent@cursor.com>
PillaiManish
force-pushed
the
sscsi-e2e-coverage-integration
branch
from
4a11003 to
81f4d0c
Compare
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/pj-rehearse max |
|
@PillaiManish: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-openshift-secrets-store-csi-driver-operator-main-operator-e2e-gcp |
|
@PillaiManish: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@PillaiManish: 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. |
|
/cc: @rausingh-rh, @siddhibhor-56 please refer job: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_release/79231/rehearse-79231-pull-ci-openshift-secrets-store-csi-driver-operator-main-operator-e2e-fips/2054546102635466752 Do let me know if we can proceed with this approach. Thanks 😄 |
2 participants
Summary
Coverage collection failures never block PR merge (best_effort: true).
References
E2E Coverage Reporting Infrastructure for secrets-store-csi-driver-operator
This PR updates the OpenShift CI configuration in openshift/release to add end-to-end Go coverage collection for the secrets-store-csi-driver-operator. It affects the ci-operator config for the operator (ci-operator/config/openshift/secrets-store-csi-driver-operator/openshift-secrets-store-csi-driver-operator-main.yaml) and the operator-e2e-fips job flow.
Practical changes:
Why this matters:
References: