Repository navigation
CNTRLPLANE-3434: add ho-release-gate pipeline for nightly promotion - #8602
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds a Tekton HO release gate that extracts a Snapshot image, runs blocking and informing Gangway jobs, evaluates their results, conditionally creates a Konflux Release, and sends Slack notifications. Shared stdlib-only helpers provide HTTP, Prow, KubeArchive, Slack, orchestration, stale-history handling, mocks, documentation, and unit tests. A PipelineRun configures timeouts, workspace storage, service-account usage, and pipeline resolution. Sequence Diagram(s)sequenceDiagram
participant PipelineRun
participant TektonPipeline
participant Gangway
participant Konflux
participant Slack
PipelineRun->>TektonPipeline: provide Snapshot and gate parameters
TektonPipeline->>Gangway: trigger and poll E2E jobs
Gangway-->>TektonPipeline: return job outcomes
TektonPipeline->>TektonPipeline: evaluate gate
TektonPipeline->>Konflux: create Release when gate passes
TektonPipeline->>Slack: send gate or error notification
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
![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
🧹 Nitpick comments (2)
.tekton/pipelines/ho-release-gate.yaml (2)
91-102: ⚡ Quick winConsider adding a timeout to the polling loop.
The commented implementation polls indefinitely until success/failure. If the Prow job gets stuck or the API becomes unreachable, this could cause the pipeline to hang forever.
When implementing the actual gangway integration, add a maximum retry count or deadline:
♻️ Suggested pattern for timeout
+ MAX_ATTEMPTS=180 # 3 hours at 60s intervals + ATTEMPT=0 # Poll for completion: while true; do + ATTEMPT=$((ATTEMPT + 1)) + if [[ ${ATTEMPT} -gt ${MAX_ATTEMPTS} ]]; then + echo "ERROR: Timeout waiting for Prow job completion" + echo -n "failed" > $(results.result.path) + break + fi STATUS=$(curl -s "${GANGWAY_URL}/v1/executions/${JOB_URL}" \🤖 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 @.tekton/pipelines/ho-release-gate.yaml around lines 91 - 102, The commented polling loop for checking Gangway job status (using GANGWAY_URL, JOB_URL, GANGWAY_TOKEN and writing to results.result.path) can hang indefinitely; update the loop to enforce a timeout by adding either a max retry counter or a deadline variable (e.g., MAX_RETRIES or GANGWAY_POLL_DEADLINE_SECONDS) and break with a failure result when exceeded; ensure the loop increments the counter or checks the deadline each iteration, logs a clear timeout error, and writes "failed" to results.result.path if the timeout is reached.
29-29: 💤 Low valueConsider pinning container image versions for reproducibility.
Multiple tasks use
:latesttags (lines 29, 62, 124, 164). For a release gate pipeline, unexpected image updates could cause inconsistent behavior or breakages. Pin to specific digests or version tags before removing the draft status.♻️ Example with pinned versions
- image: registry.redhat.io/openshift4/ose-cli:latest + image: registry.redhat.io/openshift4/ose-cli:v4.15Or use digest for stronger guarantees:
image: registry.redhat.io/openshift4/ose-cli@sha256:<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 @.tekton/pipelines/ho-release-gate.yaml at line 29, Replace occurrences of the image field using the :latest tag (e.g., "registry.redhat.io/openshift4/ose-cli:latest") with explicit, pinned version tags or immutable digests (e.g., "`@sha256`:...") to ensure reproducible builds; update every task that references the same image (the other occurrences of the same "image: registry.redhat.io/openshift4/ose-cli:latest" in this pipeline) to the chosen tag/digest and verify compatibility before merging.
🤖 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 @.tekton/pipelines/ho-release-gate.yaml:
- Around line 112-153: The pipeline currently only handles explicit
$(tasks.run-e2e.results.result) values "passed" or "failed" so errors/skips
produce no notification; add a catch-all task (e.g., notify-error) or extend
notify-slack to inspect $(tasks.run-e2e.status) so non-Succeeded statuses
trigger a notification. Specifically, add a finally task (name: notify-error)
using when: input: $(tasks.run-e2e.status) operator: notin values: ["Succeeded"]
(and/or guard with $(tasks.run-e2e.results.result) notin ["passed","failed"])
that sends an alert, or update the existing notify-slack when clause to include
$(tasks.run-e2e.status) operator: in values: ["Failed"] so task
errors/timeouts/omissions are reported.
- Around line 165-171: The Slack JSON payload is built by interpolating params
directly in the shell script (the script block that posts to
"${SLACK_WEBHOOK_URL}"), which risks JSON injection if params like
$(params.ho-image), $(params.snapshot-name) or $(params.prow-job-url) contain
quotes or newlines; fix by constructing the JSON safely with a JSON tool (e.g.,
use jq -n --arg snapshot "$(params.snapshot-name)" --arg image
"$(params.ho-image)" --arg prow "$(params.prow-job-url)" '{text: "HyperShift
nightly promotion FAILED\nSnapshot: \($snapshot)\nImage: \($image)\nProw job:
\($prow)\nPipeline: $(context.pipelineRun.name)"}' ) so each param is properly
escaped and then pipe that output to curl instead of embedding parameters
directly in the here-doc.
---
Nitpick comments:
In @.tekton/pipelines/ho-release-gate.yaml:
- Around line 91-102: The commented polling loop for checking Gangway job status
(using GANGWAY_URL, JOB_URL, GANGWAY_TOKEN and writing to results.result.path)
can hang indefinitely; update the loop to enforce a timeout by adding either a
max retry counter or a deadline variable (e.g., MAX_RETRIES or
GANGWAY_POLL_DEADLINE_SECONDS) and break with a failure result when exceeded;
ensure the loop increments the counter or checks the deadline each iteration,
logs a clear timeout error, and writes "failed" to results.result.path if the
timeout is reached.
- Line 29: Replace occurrences of the image field using the :latest tag (e.g.,
"registry.redhat.io/openshift4/ose-cli:latest") with explicit, pinned version
tags or immutable digests (e.g., "`@sha256`:...") to ensure reproducible builds;
update every task that references the same image (the other occurrences of the
same "image: registry.redhat.io/openshift4/ose-cli:latest" in this pipeline) to
the chosen tag/digest and verify compatibility before merging.
🪄 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: bd039c9f-b76d-4301-b332-b5f6622f4d01
📒 Files selected for processing (1)
.tekton/pipelines/ho-release-gate.yaml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8602 +/- ##
==========================================
+ Coverage 43.60% 44.12% +0.51%
==========================================
Files 771 779 +8
Lines 95806 98065 +2259
==========================================
+ Hits 41778 43269 +1491
- Misses 51119 51761 +642
- Partials 2909 3035 +126 see 66 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.tekton/pipelines/ho-release-gate.yaml (1)
230-234:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden Slack webhook call with fail-fast and timeout controls.
The webhook POST can currently fail silently (non-2xx) or hang without bounds. Add curl failure/timeout/retry flags so pipeline outcome reflects notification delivery failures.
Proposed patch
- name: send-notification image: curlimages/curl:latest script: | #!/bin/sh - curl -X POST -H 'Content-type: application/json' \ + curl --fail --show-error --silent \ + --connect-timeout 10 --max-time 30 \ + --retry 3 --retry-delay 2 \ + -X POST -H 'Content-type: application/json' \ --data "{ \"text\": \"HyperShift nightly promotion FAILED\nSnapshot: $(params.snapshot-name)\nImage: $(params.ho-image)\nProw job: $(params.prow-job-url)\nPipeline: $(context.pipelineRun.name)\" }" \ "${SLACK_WEBHOOK_URL}"🤖 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 @.tekton/pipelines/ho-release-gate.yaml around lines 230 - 234, Update the curl invocation that posts to "${SLACK_WEBHOOK_URL}" (the Slack webhook step) to use robust failure, timeout, and retry flags so non-2xx responses and hangs produce a non-zero exit: add --fail --show-error --connect-timeout 5 --max-time 10 --retry 3 --retry-delay 2 --retry-connrefused to the existing curl command that posts the JSON payload (the block using params.snapshot-name, params.ho-image, params.prow-job-url and context.pipelineRun.name) so the pipeline reflects notification delivery failures.
🤖 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.
Outside diff comments:
In @.tekton/pipelines/ho-release-gate.yaml:
- Around line 230-234: Update the curl invocation that posts to
"${SLACK_WEBHOOK_URL}" (the Slack webhook step) to use robust failure, timeout,
and retry flags so non-2xx responses and hangs produce a non-zero exit: add
--fail --show-error --connect-timeout 5 --max-time 10 --retry 3 --retry-delay 2
--retry-connrefused to the existing curl command that posts the JSON payload
(the block using params.snapshot-name, params.ho-image, params.prow-job-url and
context.pipelineRun.name) so the pipeline reflects notification delivery
failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0389e56d-fe3e-4fce-a4a9-7608f8e44e1b
📒 Files selected for processing (1)
.tekton/pipelines/ho-release-gate.yaml
e79809d to
58a3243
Compare
Nirshal
changed the title
|
Actionable comments posted: 0 |
58a3243 to
bfb44b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.tekton/pipelines/ho-release-gate.yaml (1)
112-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a non-success notification path in
finally.Only the pass path is handled today. If
run-e2ereturnsfailedor errors before publishing results, there is no explicit failure notification task in this pipeline. Add a failure/error finalizer path keyed off task status/results.💡 Minimal fix pattern
finally: - name: create-release when: - input: $(tasks.run-e2e.results.result) operator: in values: ["passed"] @@ - name: snapshot-name value: $(tasks.extract-image.results.snapshot-name) + + - name: notify-failure + when: + - input: $(tasks.run-e2e.status) + operator: notin + values: ["Succeeded"] + taskSpec: + steps: + - name: notify + image: registry.redhat.io/openshift4/ose-cli:latest + script: | + #!/bin/bash + set -euo pipefail + echo "run-e2e did not complete successfully; add Slack notification here"🤖 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 @.tekton/pipelines/ho-release-gate.yaml around lines 112 - 118, The pipeline's finally currently only handles the success path for the create-release finalizer (referencing the create-release entry and the run-e2e task via $(tasks.run-e2e.results.result)); add a complementary finalizer (e.g., name: notify-failure) that triggers when run-e2e did not pass by using a when clause such as input: $(tasks.run-e2e.results.result) operator: notin values: ["passed"] and also add a guard on $(tasks.run-e2e.status) to catch missing results/errors (e.g., operator: in values: ["Failed","Error"] or similar), and implement the notification taskSpec for failure/error handling; ensure both create-release and notify-failure entries live under finally so failures are explicitly handled.
🤖 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 @.tekton/pipelines/ho-release-gate.yaml:
- Around line 36-39: The pipeline currently validates params.ho-image but does
not check params.snapshot-name, so add an early non-empty validation for
snapshot-name in the same extract-image validation block: detect if
"$(params.snapshot-name)" is empty, emit a clear error like "ERROR:
snapshot-name parameter is empty" and exit 1 to fail fast; locate the validation
near the existing ho-image check in the extract-image task/script and mirror the
same pattern to ensure bad input fails before e2e runs.
---
Duplicate comments:
In @.tekton/pipelines/ho-release-gate.yaml:
- Around line 112-118: The pipeline's finally currently only handles the success
path for the create-release finalizer (referencing the create-release entry and
the run-e2e task via $(tasks.run-e2e.results.result)); add a complementary
finalizer (e.g., name: notify-failure) that triggers when run-e2e did not pass
by using a when clause such as input: $(tasks.run-e2e.results.result) operator:
notin values: ["passed"] and also add a guard on $(tasks.run-e2e.status) to
catch missing results/errors (e.g., operator: in values: ["Failed","Error"] or
similar), and implement the notification taskSpec for failure/error handling;
ensure both create-release and notify-failure entries live under finally so
failures are explicitly handled.
🪄 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: abc02483-34a2-44b5-865b-99e67bdfd6e4
📒 Files selected for processing (1)
.tekton/pipelines/ho-release-gate.yaml
50b32c6 to
946978c
Compare
e036171 to
69fb685
Compare
d3d3429 to
dfc76bb
Compare
a5ea2d1 to
14f5a1a
Compare
|
Addressing CodeRabbit nitpick: pin container image versions All 7 occurrences of MintMaker (built on Renovate) will automatically open PRs to bump the digest when the |
Pin quay.io/konflux-ci/appstudio-utils to tag+digest format as required by Konflux policy. MintMaker (Renovate) will auto-bump the digest via weekly PRs when the :latest tag is updated. Add early validation for snapshot-name in extract-image to fail fast before e2e execution. Addresses CodeRabbit findings openshift#4 and openshift#6 on PR openshift#8602. CNTRLPLANE-3434 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add a Konflux-based release gating pipeline that validates nightly HyperShift Operator Snapshots before promoting them to downstream managed services (ARO HCP first, ROSA and GCP to follow). Pipeline flow: - CronJob labels the latest Snapshot, Integration Service creates a PipelineRun via git-resolved ITS - extract-image parses the Snapshot JSON for the HO container image - run-e2e triggers blocking and informing Prow periodic jobs via Gangway, polls until completion (45 min initial delay, 10 min interval, 4h timeout) - evaluate-results applies AND logic on blocking tests; informing failures are reported but do not block promotion - create-release creates a Release CR on pass, triggering the managed pipeline to push the verified image to Quay Stale promotion alerting (CNTRLPLANE-3451): on gate failure, both finally tasks query KubeArchive for consecutive failure streaks and send a dedicated Slack alert when the streak meets the configured threshold. Includes: - Python stdlib-only modules: http_utils, prow_utils, slack_utils, kubearchive_utils, ho_release_gate - Unit tests for all modules - Mock utilities for end-to-end pipeline integration testing JIRA: CNTRLPLANE-3434 OCPSTRAT: OCPSTRAT-3250 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
c2b1f20 to
31342d1
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Now I have all the data I need. Here is the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe root cause is a mismatch between the Codecov ignore pattern and Codecov's actual file-counting behavior for newly introduced Python files in a Go-only coverage project. Step-by-step failure chain:
The fundamental issue is that Codecov is including Python source files discovered via repository scanning in the project coverage denominator, despite the Recommendations
Option 3 ( Evidence
|
|
/area ci-tooling |
The clone-lib task and PipelineRun git resolver were pointing to the fork (Nirshal/hypershift) instead of the upstream repo (openshift/hypershift). Signed-off-by: Alessandro Rossi <alesross@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 @.tekton/lib/ho_release_gate.py:
- Around line 630-701: `check_and_build_stale_payload` can crash the finally
notification path if `datetime.fromisoformat` receives an invalid
`oldest_created` timestamp. Add error handling around the timestamp parsing in
this helper so a bad KubeArchive value is treated like a skipped stale check,
with a clear log message, and keep the existing `build_stale_notification` flow
unchanged for valid timestamps.
In @.tekton/lib/kubearchive_utils.py:
- Around line 16-84: fetch_pipelineruns currently only processes the first
KubeArchive list response, so it can miss older PipelineRuns when the API
paginates results. Update the fetch loop in fetch_pipelineruns to keep
requesting additional pages using metadata.continue (or an explicit limit plus
continue token) until no continue token is returned, then merge all items before
sorting and returning the runs. Keep the existing parsing and logging behavior
in fetch_pipelineruns, but make sure every page’s items are included so
streak_days and stale checks see the full history.
In @.tekton/lib/mock/test_util_mock.py:
- Around line 256-266: The docstring in the stale alert test helper is
inconsistent with the generation logic: the oldest synthetic PipelineRun is
actually `threshold_days + 1` days ago, not `threshold_days` ago. Update the
documentation in `test_util_mock.py` around the stale streak generator (and the
related copy in the other referenced block) to match the behavior of `num_runs`,
`days_ago`, and the stale promotion helper so engineers debugging the alert see
accurate timing semantics.
In @.tekton/lib/prow_utils.py:
- Around line 160-167: `get_prow_job_status()` currently treats `status == 0` as
a generic error, which makes `poll_until_complete()` fail fast on transient
transport issues. Update the status classification logic in
`get_prow_job_status()` so `0` is handled as retryable (either alongside the
`"rate_limited"` path or as a distinct connection-failure result), and ensure
the caller path in `poll_until_complete()` continues polling instead of marking
the job terminal for this case.
- Around line 58-69: Stop retrying 5xx responses for the trigger POST in
trigger_prow_job, since a lost response can duplicate Prow executions. Update
should_retry in prow_utils.py to only retry transport failures and 429 for this
path, or otherwise make the Gangway POST idempotent before allowing 5xx retries.
Use the existing http_request_with_retry call and its should_retry helper as the
place to change this behavior.
In @.tekton/lib/tests/test_prow_utils.py:
- Around line 95-106: The test_passes_correct_payload case in test_prow_utils
should actually verify the POST body built by trigger_prow_job, since it
currently only checks retries/backoff and never asserts the payload. Simplify
the mock call inspection around http_request_with_retry, capture the body passed
by trigger_prow_job, and add assertions that job_name and env_overrides are
present in that payload with the expected values.
🪄 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: 603cb9ef-4cac-41e9-bd66-91288cc84b52
📒 Files selected for processing (16)
.tekton/lib/README.md.tekton/lib/ho_release_gate.py.tekton/lib/http_utils.py.tekton/lib/kubearchive_utils.py.tekton/lib/mock/__init__.py.tekton/lib/mock/test_util_mock.py.tekton/lib/prow_utils.py.tekton/lib/slack_utils.py.tekton/lib/tests/__init__.py.tekton/lib/tests/test_ho_release_gate.py.tekton/lib/tests/test_http_utils.py.tekton/lib/tests/test_kubearchive_utils.py.tekton/lib/tests/test_prow_utils.py.tekton/lib/tests/test_slack_utils.py.tekton/pipelines/ho-release-gate-run.yaml.tekton/pipelines/ho-release-gate.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- .tekton/pipelines/ho-release-gate-run.yaml
- .tekton/lib/tests/test_slack_utils.py
- .tekton/pipelines/ho-release-gate.yaml
- Guard datetime.fromisoformat against invalid timestamps in stale check to prevent finally task crash (ho_release_gate.py) - Add time filter (creationTimestampAfter) and explicit limit to KubeArchive query to ensure all recent runs are fetched regardless of server-side sort order (kubearchive_utils.py) - Treat connection failures (status 0) as retryable in get_prow_job_status so polling continues on transient errors (prow_utils.py) - Fix test_passes_correct_payload to verify POST body content (test_prow_utils.py) - Fix docstring timing semantics in stale mock (test_util_mock.py) JIRA: CNTRLPLANE-3434 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
The mock/ directory contains test_util_mock.py, a utility module that generates synthetic test data for end-to-end pipeline integration testing (fake Snapshots, simulated Gangway/Prow responses, stale streak generators). The __init__.py makes it a Python package so the utilities can be imported. These are not unit tests (those are in tests/) but helpers for validating the full pipeline flow. I left them in the PR because I found them very useful to test some corner cases, and I thought they might have value for future maintenance. That said, working with these requires temporarily altering the Tekton pipeline, which is something possible only on an open PR (which still requires the ITS to point to the PR branch instead of main), so I am open to removing these and archiving them locally on my computer, just in case I need them in the future. Waiting for feedback from you on this.
There was a problem hiding this comment.
You're right — the mock package is a self-contained toolkit for manual integration testing, not a unit test suite. fetch_pipelineruns_mock_stale_long is documented with usage instructions in the module docstring and is part of the mock API surface. "Not called from tests/" doesn't make it dead code here. Withdrawing.
There was a problem hiding this comment.
You're right — the mock package is a self-contained toolkit for manual integration testing, not a unit test suite. fetch_pipelineruns_mock_stale_long is documented with usage instructions in the module docstring and is part of the mock API surface. "Not called from tests/" doesn't make it dead code here. Withdrawing.
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Review: feat(ci): add ho-release-gate pipeline for nightly promotion
Well-structured Tekton pipeline with solid Python library design. Two blocking issues (both the same class: feature branch references that need updating to main), several hardening suggestions, and a few questions. See inline comments.
| Category | Count |
|---|---|
| Blocking | 2 |
| Suggestions | 6 |
| Nits | 1 |
| Questions | 2 |
| Praise | 3 |
Praise:
- Excellent defensive design with default-first result initialization — prevents silent failures on OOMKill
- Comprehensive test suite with well-structured edge cases
- stdlib-only constraint is well-justified with clear upgrade path documented in README
Once the two branch references are corrected, this should be in good shape for a second pass.
| - name: url | ||
| value: https://github.com/openshift/hypershift | ||
| - name: revision | ||
| value: ho-release-gate-pipeline |
There was a problem hiding this comment.
[blocking] This references the feature branch ho-release-gate-pipeline. Once merged and the branch is deleted, the git resolver will fail to find the Pipeline definition.
| value: ho-release-gate-pipeline | |
| value: main |
If the ITS configuration overrides this value at runtime, please add a comment here stating that.
There was a problem hiding this comment.
Fixed in 6028dd6. Changed to openshift/hypershift + main.
| echo "=== clone-lib ===" | ||
| echo "Running as: $(oc whoami)" | ||
| REPO_URL="https://github.com/openshift/hypershift.git" | ||
| BRANCH="ho-release-gate-pipeline" |
There was a problem hiding this comment.
[blocking] Same issue as the PipelineRun template — this sparse checkout of .tekton/lib/ will break once the feature branch is deleted post-merge.
| BRANCH="ho-release-gate-pipeline" | |
| BRANCH="main" |
There was a problem hiding this comment.
Fixed in 6028dd6. Changed to openshift/hypershift + main.
| with open("$(results.results-json.path)", "w") as f: | ||
| f.write("[]") | ||
|
|
||
| gangway_url = "https://gangway-ci.apps.ci.l2s4.p1.openshiftapps.com/v1/executions" |
There was a problem hiding this comment.
[suggestion] This Gangway URL is hardcoded. The CI cluster has migrated before (app.ci → build0x). Consider making this a pipeline parameter with this as the default, so a cluster move doesn't require a code change.
There was a problem hiding this comment.
Agreed. The Gangway URL is hardcoded in a single place (the run-e2e inline script). I will extract it as a pipeline parameter with the current value as default, so a cluster migration only requires updating the ITS definition.
|
|
||
| env_overrides = { | ||
| "OVERRIDE_IMAGE_HYPERSHIFT_OPERATOR": ho_image, | ||
| "OVERRIDE_IMAGE_HYPERSHIFT_TESTS": "registry.ci.openshift.org/hypershift/hypershift-tests:latest" |
There was a problem hiding this comment.
[suggestion] Using :latest for the test image means the gate runs whatever test binary happens to be latest, which may not correspond to the HO image being validated. If the Snapshot contains a test image component, it would be more correct to extract and use that. If :latest is intentional (e.g., tests are always forward-compatible), a comment explaining why would help future readers.
There was a problem hiding this comment.
This is a known limitation that I already flagged to the team (see Slack thread). The Konflux Snapshot only contains the hypershift-operator component. The test image (hypershift-tests) is built by the OpenShift CI pipeline, not by Konflux, so it is not part of the Snapshot and there is no straightforward way to extract the matching version from another source. Using :latest is intentional given this constraint. I will add a comment in the code explaining why.
| pipeline_run = "$(context.pipelineRun.name)" | ||
| results_json = os.environ["RESULTS_JSON"] | ||
| webhook_url = os.environ["SLACK_WEBHOOK_URL"] | ||
| konflux_base = "https://konflux-ui.apps.stone-prd-rh01.pg1f.p1.openshiftapps.com/ns/crt-redhat-acm-tenant/applications/hypershift-operator" |
There was a problem hiding this comment.
[suggestion] Since ARO HCP is the pilot and ROSA/GCP HCP follow using the same template, consider extracting the namespace (crt-redhat-acm-tenant) and Konflux app URL as pipeline parameters. This avoids forking the pipeline when onboarding new platforms — their ITS definitions can supply different values.
There was a problem hiding this comment.
The namespace crt-redhat-acm-tenant is the Konflux tenant where all resources live (Snapshots, ReleasePlans, Release CRs) -- it is not specific to ARO HCP. When onboarding ROSA or GCP, each will have its own ITS definition and ReleasePlan, but they will all live in the same tenant namespace. The pipeline is already designed to be reusable across managed services through its parameters (gate-label, release-plan-name, e2e-blocking-job-names, etc.), so no forking is needed. The Konflux app URL is only used for building links in Slack notifications and is also tenant-level, not service-specific.
There was a problem hiding this comment.
Makes sense — the namespace and URL are tenant-level constants shared across all managed services, not service-specific values. The pipeline is already parameterized at the right level (gate-label, release-plan-name, e2e-blocking-job-names). Extracting these would add complexity for a scenario that won't happen. Withdrawing.
There was a problem hiding this comment.
Makes sense — the namespace and URL are tenant-level constants shared across all managed services, not service-specific values. The pipeline is already parameterized at the right level (gate-label, release-plan-name, e2e-blocking-job-names). Withdrawing.
| release_name, snapshot, pipeline_run, konflux_base) | ||
|
|
||
| success = send_slack_message(webhook_url, payload) | ||
| if not success: |
There was a problem hiding this comment.
[suggestion] When the Slack notification fails, this task still exits 0, making the failure invisible in PipelineRun status. Consider sys.exit(1) so the PipelineRun shows a partial failure, or write a result indicating notification status. I understand the rationale of not blocking pipeline completion from a finally task, but silent notification failure means operators won't know alerts are broken.
There was a problem hiding this comment.
I intentionally chose to exit 0 on notification failure. If notify-slack exits non-zero, the PipelineRun is marked as Failed in Tekton and KubeArchive. When an operator notices that notifications are missing and starts investigating, they would see all PipelineRuns marked as Failed and would have to open each one to determine whether the failure was a real gate failure or just a broken notification. By keeping the exit code tied to the gate verdict only, the PipelineRun status directly answers "did the release succeed?" without ambiguity. At that point the operator already knows notifications are broken and can check the task logs of the latest run to understand why.
There was a problem hiding this comment.
Good rationale. PipelineRun status = gate verdict is the right invariant for a finally task — mixing in notification health would make triage harder, not easier. Withdrawing.
There was a problem hiding this comment.
Good rationale. PipelineRun status = gate verdict is the right invariant for a finally task — mixing in notification health would make triage harder, not easier. Withdrawing.
| pipeline_run, konflux_base, gate_label) | ||
|
|
||
| success = send_slack_message(webhook_url, payload) | ||
| if not success: |
There was a problem hiding this comment.
[question] The PR description says notify-slack and notify-slack-error are "mutually exclusive." Is there a scenario where neither fires? A short comment in the YAML explaining the mutual exclusivity logic (which task fires under which condition) would help future maintainers.
There was a problem hiding this comment.
I agree that this is a bit counter-intuitive. The notify-slack-error is meant to be a "catch-all" before the release task is reached, so every error before reaching that point will be reported by that. The reason why we also have error handling in the other finally task is that after create-release, more information about the errors becomes available, so the message can be more helpful. The mutual exclusivity exists and relies on Tekton's task status propagation: (1) If the pipeline reaches evaluate-results, then create-release runs (Succeeded or Failed), notify-slack fires (it receives all resolved params), and notify-slack-error is skipped (create-release.status != None). (2) If the pipeline fails before evaluate-results (e.g. extract-image or run-e2e crash), create-release is skipped (status=None), its result params are unresolved so notify-slack cannot run, and notify-slack-error fires instead. There is no scenario where neither fires. I will add a YAML comment block above the finally section explaining this logic.
|
|
||
| The output format is a JSON array of objects with keys: | ||
| job (full name), result, url, type. Uses compact separators | ||
| to minimize size (Tekton results have a 4KB limit). |
There was a problem hiding this comment.
[suggestion] With multiple blocking + informing jobs, each carrying a full Prow URL (~150 chars), the result could exceed 4KB. Consider either:
- Adding a size check with truncation fallback
- Writing results to a workspace file instead of a Tekton result
- Truncating URLs in the JSON (full URLs are already printed to stdout)
There was a problem hiding this comment.
Good catch, this is a real risk. I will switch to your option 2: write the results JSON to a workspace file instead of a Tekton result. The shared workspace is already mounted on all tasks (we use it for the Python libraries), so both evaluate-results and notify-slack can read from it directly. I will also remove the Tekton result declaration to avoid confusion.
|
|
||
| __all__ = ["fetch_pipelineruns", "build_pipelinerun_url"] | ||
|
|
||
| KUBEARCHIVE_API_BASE = ( |
There was a problem hiding this comment.
[suggestion] This couples the library to a specific production cluster. Consider making it configurable via env var for staging/alternative environments:
KUBEARCHIVE_API_BASE = os.environ.get(
"KUBEARCHIVE_API_BASE",
"https://kubearchive-api-server-product-kubearchive"
".apps.stone-prd-rh01.pg1f.p1.openshiftapps.com"
)There was a problem hiding this comment.
Agreed. I will make it configurable via env var with the current value as default, as you suggested.
| threshold = int(os.environ["STALE_THRESHOLD_DAYS"]) | ||
| stale = check_and_build_stale_payload( | ||
| token, its_scenario, threshold, pipeline_run, | ||
| konflux_base, "crt-redhat-acm-tenant", gate_label) |
There was a problem hiding this comment.
[question] Since generateName is used with oc create in the create-release task, re-triggering the pipeline for the same Snapshot creates a duplicate Release CR. Is there deduplication upstream (ReleasePlanAdmission, managed pipeline), or should there be a check here?
There was a problem hiding this comment.
There is no deduplication at the Release CR creation level, so a duplicate CR will be created. However, the managed release pipeline is designed to be idempotent. Early in execution, the filter-already-released-advisory-images task checks if the container image digests from the Snapshot have already been pushed to the target repository. If so, it sets a skip_release flag that causes Tekton to skip all downstream tasks (push-snapshot, rh-sign-image, create-pyxis-image, create-advisory, etc.). Individual tasks also check their own domains separately as an additional safety net. In practice, a duplicate Release CR will start the pipeline but it will short-circuit after the initial setup tasks without altering the target registry or creating duplicate advisories. If you want, we could add a pre-creation check, but this will add complexity and could also not be straightforward, because we could have several Release CRs for the same Snapshot when we add more managed services (like ROSA) and in that case it would be further difficult and prone to errors to correctly check.
There was a problem hiding this comment.
I misread the failure mode — generateName + oc create produces a new CR each time, so there's no collision. And the managed release pipeline's filter-already-released-advisory-images task handles idempotency downstream. No dedup needed here. Withdrawing.
There was a problem hiding this comment.
I misread the failure mode — generateName + oc create produces a new CR each time, so there's no collision. The managed release pipeline's filter-already-released-advisory-images handles idempotency downstream. Withdrawing.
Switch from OVERRIDE_IMAGE_HYPERSHIFT_OPERATOR (ci-operator ImageStream mechanism, broken by race condition) to the MULTISTAGE_PARAM_OVERRIDE_ transport variable that passes the image directly to the hypershift-install step parameter. Requires openshift/release#81877 to be merged first. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Point the PipelineRun git resolver and clone-lib back to Nirshal/hypershift so the pipeline can run against the feature branch and validate the OVERRIDE_HYPERSHIFT_OPERATOR_IMAGE flow end-to-end. Will be switched to openshift/hypershift + main after validation. Signed-off-by: Alessandro Rossi <alesross@redhat.com>
The PipelineRun git resolver and clone-lib sparse checkout were referencing the fork branch (Nirshal/hypershift, ho-release-gate-pipeline). After merge, the feature branch will be deleted and both would fail. Switch to openshift/hypershift + main so the pipeline resolves from upstream permanently. Signed-off-by: Alessandro Rossi <alesross@redhat.com>
…ve docs - Extract Gangway URL as pipeline parameter with default value - Extract KubeArchive URL as pipeline parameter, pass through function arguments instead of module-level env var read - Add comment explaining intentional :latest for test image - Improve finally tasks comment explaining Tekton skip mechanism - Update tests for new function signatures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tekton task results have a 4KB limit. With many blocking and informing jobs, each carrying a full Prow URL, the results JSON can exceed that limit and crash the task at runtime. Write results to a file on the shared workspace instead, and have downstream tasks read from it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Second-round review. 6 inline comments — 4 carry forward from the first round (still open), 2 are new findings.
| with open("$(workspaces.shared.path)/results.json") as f: | ||
| results_json = f.read() | ||
| webhook_url = os.environ["SLACK_WEBHOOK_URL"] | ||
| konflux_base = "https://konflux-ui.apps.stone-prd-rh01.pg1f.p1.openshiftapps.com/ns/crt-redhat-acm-tenant/applications/hypershift-operator" |
There was a problem hiding this comment.
Still open from round 1 — hardcoded Konflux URL + namespace
konflux_base and the namespace crt-redhat-acm-tenant are baked into the pipeline YAML. If the tenant or Konflux instance changes, every pipeline that inlines this needs an update.
Consider lifting both into pipeline parameters (with the current values as defaults) so they can be overridden at the PipelineRun level without touching the Pipeline definition.
There was a problem hiding this comment.
Alessandro's response is correct — the namespace and URL are tenant-level constants shared across all managed services, not service-specific. The pipeline is already parameterized at the right level. Withdrawing.
|
|
||
| success = send_slack_message(webhook_url, payload) | ||
| if not success: | ||
| print("ERROR: Slack notification failed after all retries", flush=True) |
There was a problem hiding this comment.
Still open from round 1 — silent exit 0 on Slack failure
When Slack notification fails, the task prints the error but still exits 0. Downstream consumers (humans, dashboards) have no way to tell that notification was lost.
Consider writing a task result (e.g. $(results.slack-status.path)) with "failed" so callers can detect it, even if the task itself should not block the pipeline.
There was a problem hiding this comment.
Alessandro's rationale holds. PipelineRun status = gate verdict is the right invariant for a finally task — mixing in notification health would make triage harder, not easier. Withdrawing
| print(f"Release: {release_name}", flush=True) | ||
|
|
||
| if not gate_passed: | ||
| its_scenario = os.environ["ITS_SCENARIO"] |
There was a problem hiding this comment.
New — duplicated stale-check block
The ~15-line stale-check block (its_scenario = … through send_slack_message(stale_payload, …)) is copy-pasted between notify-slack and notify-slack-error. This is the same logic with the same env vars — a classic Duplicated Code smell.
Extract it into a shared shell function or a separate Task so changes only need to happen in one place.
There was a problem hiding this comment.
The duplicated block is the parameter preparation for check_and_build_stale_payload(), which is where the actual shared logic lives (already extracted in ho_release_gate.py). The env var reads and token acquisition are the glue that feeds values into that function.
Extracting this glue into a helper would create a script-like function that reads from os.environ and calls subprocess internally. This would be difficult to test in isolation (it's not a pure function, it's effectively a script), and it would break the design pattern we follow throughout the pipeline: testable, reusable functions in the Python library, called by thin inline scripts in the YAML. Adding a helper in between would introduce a layer of indirection (scripts calling scripts calling functions) without reducing complexity.
Since the two finally tasks run as separate Tekton pods with no shared execution context, each must independently prepare its own parameters. I think the current factoring is the right one, but happy to chat about it if you see it differently.
There was a problem hiding this comment.
Misread the failure mode — generateName + oc create produces a new CR each time, no collision. The managed release pipeline's filter-already-released-advisory-images handles idempotency downstream. Withdrawing.
| "OVERRIDE_IMAGE_HYPERSHIFT_TESTS": "registry.ci.openshift.org/hypershift/hypershift-tests:latest" | ||
| } | ||
|
|
||
| jobs = trigger_all_jobs(blocking, informing, gangway_url, token, env_overrides) |
There was a problem hiding this comment.
New — run-e2e can exit non-zero despite spec "always exits 0"
trigger_all_jobs() raises ValueError on missing env vars, and this script: block has no try/except around it. If GANGWAY_URL or a job-list var is unset, the task exits non-zero and the pipeline fails hard — violating the spec requirement that run-e2e always exits 0, deferring pass/fail to evaluate-results.
Wrap the call in a try/except that catches ValueError, logs it, and writes a sentinel to $(results.*) so evaluate-results can report the real cause.
There was a problem hiding this comment.
The pipeline has two distinct error categories by design, each with its own reporting path:
-
Test failures (normal operation): one or more Prow jobs fail.
run-e2eexits 0, writes structured results to the workspace,evaluate-resultsdetermines the gate verdict, andnotify-slacksends a detailed notification with per-job status, URLs, and stale promotion alerts. This is the path optimized for actionable detail. -
Infrastructure errors (exceptional): a task crashes due to misconfiguration, transient cluster issues, API outages, etc.
notify-slack-errorfires and alerts the team that the pipeline itself broke. Troubleshooting goes through PipelineRun logs, which are the only place that can provide full context for unexpected failures.
These two paths are intentionally separate. Adding error handling inside run-e2e to catch infrastructure errors and route them through notify-slack would blur this boundary: we would have two error reporting paths for infrastructure failures (one partial in run-e2e, one generic in notify-slack-error), and the partial one could never be exhaustive because it only covers one task out of four. A crash in clone-lib, extract-image, or evaluate-results would still go through notify-slack-error regardless. The result would be added complexity without added coverage.
On the specific scenario (missing env vars): GANGWAY_URL, HO_IMAGE, BLOCKING_JSON, and INFORMING_JSON are all injected via explicit Tekton param bindings in the task definition, and GANGWAY_TOKEN comes from a secretKeyRef. If any of these are missing, Tekton fails the pod scheduling before the script runs - the os.environ reads are never reached.
| echo "ReleasePlan: ${RELEASE_PLAN}" | ||
|
|
||
| set +e | ||
| RELEASE_NAME=$(oc create -f - -o jsonpath='{.metadata.name}' <<EOFRELEASE |
There was a problem hiding this comment.
Still open from round 1 — duplicate Release CR on re-trigger
If the pipeline is re-triggered for the same snapshot, oc create -f - will attempt to create a Release CR that may already exist, causing a failure. Is there deduplication logic elsewhere (e.g. a unique name derived from the snapshot SHA), or should this use oc apply / create-if-not-exists?
| return runs | ||
|
|
||
|
|
||
| def fetch_pipelineruns_mock_stale_long(token, namespace, label_selector): |
There was a problem hiding this comment.
nit: fetch_pipelineruns_mock_stale_long is exported in __all__ but never referenced in any test file. Dead code — either add test coverage that uses it or remove it.
There was a problem hiding this comment.
The mock/ package is a self-contained toolkit for manual integration testing, not a unit test suite. The function is documented with usage instructions in the module docstring. "Not called from tests/" doesn't make it dead code. Withdrawing.
|
Replying to Alessandro's four responses from the first round — he's right on all of them, withdrawing those items. Mock dead code ( Konflux URL + namespace: The namespace and URL are tenant-level constants shared across all managed services, not service-specific values. The pipeline is already parameterized at the right level ( Silent exit 0 on Slack failure: PipelineRun status = gate verdict is the right invariant for a Duplicate Release CR: I misread the failure mode — The two new findings from round 2 (duplicated stale-check block and |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, Nirshal 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
Nirshal
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@Nirshal: This pull request references CNTRLPLANE-3434 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 epic 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. |
|
/verified later @Nirshal |
openshift-ci-robot
added
verified-later
verified
|
@Nirshal: This PR has been marked to be verified later by 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. |
|
/retest ci/prow/okd-scos-images |
|
/test okd-scos-images |
|
@Nirshal: 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. |
3 participants
Summary
Adds the Tekton pipeline and PipelineRun template for the HyperShift Operator nightly release gating pipeline, triggered via IntegrationTestScenario (ITS).
OVERRIDE_HYPERSHIFT_OPERATOR_IMAGEstep parameter and Gangway transport variablePipeline flow
Stale promotion alerting (CNTRLPLANE-3451)
When the gate fails, both
notify-slackandnotify-slack-errorquery theKubeArchive REST API for historical PipelineRuns matching the ITS scenario label.
The consecutive failure streak is measured in days (difference between now and the
oldest consecutive failed run, inclusive of today).
If
streak_days >= stale-threshold-days, the normal notification is replaced witha stale promotion alert containing:
dates, and failure reasons (Failed, PipelineRunTimeout, CouldntGetPipeline, etc.)
The stale check is safe to skip: if KubeArchive is unreachable or returns no data,
the normal notification is sent instead.
Notification reliability
All tasks use default-first initialization: each task writes safe default values
to its result paths at the start of its script, before any logic. If a task starts
but crashes mid-execution (e.g., OOMKilled), results are still initialized.
Two mutually exclusive finally tasks ensure a notification is always sent:
notify-slack: detailed notification, depends on task results (skipped if results uninitialized)notify-slack-error: generic fallback, zero result dependencies, fires only whencreate-releasewas skipped by DAG failureBoth finally tasks perform the stale promotion check independently, so a stale alert
is sent regardless of which notification path fires.
Files
.tekton/pipelines/ho-release-gate.yaml.tekton/pipelines/ho-release-gate-run.yaml.tekton/lib/ho_release_gate.py.tekton/lib/kubearchive_utils.py.tekton/lib/prow_utils.py.tekton/lib/slack_utils.py.tekton/lib/http_utils.py.tekton/lib/README.md.tekton/lib/tests/.tekton/lib/mock/Pipeline parameters
SNAPSHOTe2e-blocking-job-namese2e-informing-job-namesgate-labelrelease-plan-namestale-threshold-daysResources
All resources live in namespace
crt-redhat-acm-tenantunless noted..tekton/pipelines/ho-release-gate*.yamlredhat-hypershift-operator-ho-release-gate-aro-hcp(inrhtap-releng-tenant)hypershift-operator-ho-release-gate-aro-hcphypershift-operator-nightly-promotionhypershift-ho-release-gate-aro-hcpnightly-promotion-sanightly-promotion-sa->konflux-tester-internalbot-actionsnightly-promotion-sa->konflux-releaser-bot-actionsvia taskRunSpecsgangway-token(Prow auth)slack-webhook(Slack notifications)Release mechanism
The pipeline uses explicit Release CR creation (not auto-release):
create-releasetask creates aReleaseobject referencing the ReleasePlan (name passed via ITS paramrelease-plan-name) and the tested Snapshotrhtap-releng-tenant(managed workspace)redhat-hypershift-operator-ho-release-gate-aro-hcp) picks up the Releaserh-push-to-external-registry) pushes the validated image to the verified Quay repo with platform-prefixed tags (aro-hcp-latest,aro-hcp-latest-{{ timestamp }},aro-hcp-{{ git_sha }}, etc.)Target repo:
quay.io/redhat-services-prod/crt-redhat-acm-tenant/hypershift/hypershift-operator-verifiedRemaining work
OVERRIDE_HYPERSHIFT_OPERATOR_IMAGEstep parameter + Gangway transport variable tohypershift-install)openshift/hypershift:mainonce this PR merges (!19913 - Draft)Related
OVERRIDE_HYPERSHIFT_OPERATOR_IMAGEparameter tohypershift-installstep (dependency)Summary by CodeRabbit