OSSM test: split Istio image build from test execution to fix WebSocket drops - #82416
OSSM test: split Istio image build from test execution to fix WebSocket drops#82416fjglira wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe CI workflow adds a dedicated Istio image-build step, runs integration tests directly in the test container, and updates master, retest, sail, and release jobs to use the new step with Istio CI pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CIJob
participant ImagesBuild
participant MaestraPod
participant Quay
participant IntTests
CIJob->>ImagesBuild: Start image-build step
ImagesBuild->>MaestraPod: Copy source and kubeconfig
MaestraPod->>Quay: Build and push Istio images
CIJob->>IntTests: Start integration tests
IntTests->>Quay: Pull images from TEST_HUB
IntTests->>IntTests: Run tests and copy artifacts to SHARED_DIR
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fjglira 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
|
@fjglira: |
![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)
ci-operator/step-registry/servicemesh/istio/images-build/servicemesh-istio-images-build-commands.sh (1)
8-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTAG derivation is duplicated across the images-build and int-tests scripts.
Both scripts independently re-implement the samePULL_PULL_SHA/BUILD_IDfallback logic to computeTAG. Since these are separate ci-operator steps within the same job, pass the tag explicitly via${SHARED_DIR}from the build step instead of recomputing it, removing the risk of the two copies drifting if one is updated without the other.
ci-operator/step-registry/servicemesh/istio/images-build/servicemesh-istio-images-build-commands.sh#L8-L18: after derivingTAG, write it to${SHARED_DIR}/istio-image-tagfor the downstream test step to consume.ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh#L63-L71: replace the re-derivation withTAG="$(cat "${SHARED_DIR}/istio-image-tag")".Based on coding guidelines for
ci-operator/step-registry/**/*-commands.sh: "use${SHARED_DIR}for passing data between steps instead of printing secrets to logs" — the same SHARED_DIR handoff pattern is the idiomatic way to pass this derived value between steps rather than duplicating the derivation logic.🤖 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/servicemesh/istio/images-build/servicemesh-istio-images-build-commands.sh` around lines 8 - 18, Centralize TAG derivation between the two steps: in ci-operator/step-registry/servicemesh/istio/images-build/servicemesh-istio-images-build-commands.sh lines 8-18, write the derived TAG to ${SHARED_DIR}/istio-image-tag; in ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh lines 63-71, remove the PULL_PULL_SHA/BUILD_ID fallback logic and load TAG from that shared file.Source: Coding guidelines
ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-ref.yaml (1)
39-42: 🚀 Performance & Scalability | 🔵 TrivialRe-evaluate
resourcessizing now that tests run directly in this container.Documentation now states tests execute directly in the step container rather than being delegated to the MAISTRA pod via
oc rsh. The step'sresources.requests(cpu: '1', memory: 500Mi) were presumably sized for a mostly-idle orchestration container; running the actual Go integration-test binaries locally (compiling, buffering test/verbose output, retry loop) may need higher limits to avoid CPU throttling or OOM under load.🤖 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/servicemesh/istio/int-tests/servicemesh-istio-int-tests-ref.yaml` around lines 39 - 42, Re-evaluate and increase the resources.requests CPU and memory values for the step now that it runs Istio integration-test binaries directly in the container. Update the resources configuration associated with the servicemesh-istio-int-tests step, preserving the existing test execution behavior and choosing capacity suitable for compilation, output buffering, and retries.
🤖 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/servicemesh/istio/images-build/servicemesh-istio-images-build-commands.sh`:
- Around line 3-6: Disable xtrace immediately around the docker login credential
injection in the command flow, including the oc rsh invocation that interpolates
QUAY_USERNAME and QUAY_PASSWORD, then restore tracing afterward if subsequent
commands require it. Keep the disabled scope minimal and add a clear comment
documenting that tracing is disabled to prevent credential leakage; update the
script’s default tracing behavior only as needed to ensure these credentials are
never emitted.
In
`@ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh`:
- Around line 56-58: Update the configuration parsing flow around
parse-test-config.sh to invoke it only once, capture its generated export
statements, and reuse that result instead of discarding it and running the
script again. Avoid directly evaluating downloaded script output with eval;
write the generated configuration to a controlled temporary or local file and
source it, preserving the SKIP_PARSER_SKIP_TESTS and SKIP_PARSER_SKIP_SUBSUITES
exports.
---
Nitpick comments:
In
`@ci-operator/step-registry/servicemesh/istio/images-build/servicemesh-istio-images-build-commands.sh`:
- Around line 8-18: Centralize TAG derivation between the two steps: in
ci-operator/step-registry/servicemesh/istio/images-build/servicemesh-istio-images-build-commands.sh
lines 8-18, write the derived TAG to ${SHARED_DIR}/istio-image-tag; in
ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh
lines 63-71, remove the PULL_PULL_SHA/BUILD_ID fallback logic and load TAG from
that shared file.
In
`@ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-ref.yaml`:
- Around line 39-42: Re-evaluate and increase the resources.requests CPU and
memory values for the step now that it runs Istio integration-test binaries
directly in the container. Update the resources configuration associated with
the servicemesh-istio-int-tests step, preserving the existing test execution
behavior and choosing capacity suitable for compilation, output buffering, and
retries.
🪄 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: 25de24c5-3588-43c9-b01a-1f689f944ae9
📒 Files selected for processing (12)
ci-operator/config/openshift-service-mesh/istio/openshift-service-mesh-istio-master.yamlci-operator/config/openshift-service-mesh/istio/openshift-service-mesh-istio-release-1.24.yamlci-operator/config/openshift-service-mesh/istio/openshift-service-mesh-istio-release-1.26.yamlci-operator/config/openshift-service-mesh/istio/openshift-service-mesh-istio-release-1.27.yamlci-operator/config/openshift-service-mesh/istio/openshift-service-mesh-istio-release-1.28.yamlci-operator/config/openshift-service-mesh/istio/openshift-service-mesh-istio-release-1.30.yamlci-operator/step-registry/servicemesh/istio/images-build/OWNERSci-operator/step-registry/servicemesh/istio/images-build/servicemesh-istio-images-build-commands.shci-operator/step-registry/servicemesh/istio/images-build/servicemesh-istio-images-build-ref.metadata.jsonci-operator/step-registry/servicemesh/istio/images-build/servicemesh-istio-images-build-ref.yamlci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.shci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-ref.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh`:
- Around line 42-44: Update the parse-test-config.sh download step in the
service-mesh Istio integration test commands to fetch it from a reviewed
immutable commit or version instead of mutable main. Verify the downloaded file
against a pinned expected checksum before chmod or any execution, and fail the
job on checksum mismatch.
🪄 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: 8a54d24f-874f-49b8-8b91-71d2e0687e88
📒 Files selected for processing (12)
ci-operator/config/openshift-service-mesh/istio/openshift-service-mesh-istio-master.yamlci-operator/config/openshift-service-mesh/istio/openshift-service-mesh-istio-release-1.24.yamlci-operator/config/openshift-service-mesh/istio/openshift-service-mesh-istio-release-1.26.yamlci-operator/config/openshift-service-mesh/istio/openshift-service-mesh-istio-release-1.27.yamlci-operator/config/openshift-service-mesh/istio/openshift-service-mesh-istio-release-1.28.yamlci-operator/config/openshift-service-mesh/istio/openshift-service-mesh-istio-release-1.30.yamlci-operator/step-registry/servicemesh/istio/images-build/OWNERSci-operator/step-registry/servicemesh/istio/images-build/servicemesh-istio-images-build-commands.shci-operator/step-registry/servicemesh/istio/images-build/servicemesh-istio-images-build-ref.metadata.jsonci-operator/step-registry/servicemesh/istio/images-build/servicemesh-istio-images-build-ref.yamlci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.shci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-ref.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh (1)
135-150: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTreat a missing completion marker as failure on the first attempt.
The first-attempt retry path only handles a non-zero exit together with a missing
/tmp/ISTIO_TESTS_DONEmarker. A zero exit without the marker can therefore bypass retry/failure handling, even though Lines 78-82 define marker absence as incomplete execution. Use the sameORcompletion invariant as the second attempt.Proposed fix
- if [ "${TEST_RC}" -ne 0 ] && ! are_tests_done; then + if [ "${TEST_RC}" -ne 0 ] || ! are_tests_done; then🤖 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/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh` around lines 135 - 150, Update the first-attempt retry condition in the Istio test execution flow to use OR semantics: retry whenever TEST_RC is non-zero or are_tests_done reports a missing completion marker. Keep the existing second-attempt condition and failure handling unchanged.
🤖 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/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh`:
- Around line 66-70: Move the KUBECONFIG export from the later setup block into
the beginning of run_tests, before its invocation of
./prow/check-cluster-ready.sh. Preserve the existing ${SHARED_DIR}/kubeconfig
value and remove the duplicate later export.
---
Outside diff comments:
In
`@ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh`:
- Around line 135-150: Update the first-attempt retry condition in the Istio
test execution flow to use OR semantics: retry whenever TEST_RC is non-zero or
are_tests_done reports a missing completion marker. Keep the existing
second-attempt condition and failure handling unchanged.
🪄 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: 240eb204-1016-4e74-9dc8-393c681bdb05
📒 Files selected for processing (3)
ci-operator/step-registry/servicemesh/istio/images-build/servicemesh-istio-images-build-commands.shci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.shci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- ci-operator/step-registry/servicemesh/istio/images-build/servicemesh-istio-images-build-commands.sh
|
/pj-rehearse pull-ci-openshift-service-mesh-istio-master-istio-integration-pilot-retest pull-ci-openshift-service-mesh-istio-master-istio-integration-sail-pilot |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-openshift-service-mesh-istio-master-istio-integration-pilot-retest pull-ci-openshift-service-mesh-istio-master-istio-integration-sail-pilot |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-openshift-service-mesh-istio-master-istio-integration-pilot-retest pull-ci-openshift-service-mesh-istio-master-istio-integration-sail-pilot |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
| export SKIP_TEST_RUN=\"true\" | ||
| export ARTIFACT_DIR=\"${ARTIFACT_DIR}\" | ||
| export INSTALL_SAIL_OPERATOR=\"${INSTALL_SAIL_OPERATOR:-false}\" | ||
| oc version |
There was a problem hiding this comment.
Do we need this oc version here ? Is that checking if the remote cluster pod has oc client there ?
There was a problem hiding this comment.
Nope, it's just to make sure which OCP cluster version we are running. I added long time ago and I didn't removed. When we have the doubt about the version it help to have no doubts about the version
| export INSTALL_SAIL_OPERATOR=\"${INSTALL_SAIL_OPERATOR:-false}\" | ||
| oc version | ||
| cd /work | ||
| entrypoint prow/integ-suite-ocp.sh |
There was a problem hiding this comment.
How does this call the build_images() function in the integ-suite-ocp.sh ? Are we passing that function name as an argument in job config or somewhere else ?
There was a problem hiding this comment.
Because in the integ-suite, there is a logic to specifically build and push the images. So we skip the test run just to help us to build and push there. Upstream way does not work for us, because it's more static. But maybe we can contribute something to make it more flexible and avoid this workaround
There was a problem hiding this comment.
Just wondering.
Why to not add a configurable flag to integ-suite-ocp.sh script that would configure the build of the images and push them to the defined HUB.
By default, it would be not used.
Here, we're setting the HUB and quay.io/sail-dev.
But in local development we could use our own quay.io account.
That might help during testing/debugging/development to not rrebuild the images every time the integ-suite-ocp.sh script is being executed.
|
/pj-rehearse pull-ci-openshift-service-mesh-istio-master-istio-integration-pilot-retest pull-ci-openshift-service-mesh-istio-master-istio-integration-sail-pilot |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-openshift-service-mesh-istio-master-istio-integration-pilot-retest pull-ci-openshift-service-mesh-istio-master-istio-integration-sail-pilot |
1 similar comment
|
/pj-rehearse pull-ci-openshift-service-mesh-istio-master-istio-integration-pilot-retest pull-ci-openshift-service-mesh-istio-master-istio-integration-sail-pilot |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-openshift-service-mesh-istio-master-istio-integration-pilot-retest pull-ci-openshift-service-mesh-istio-master-istio-integration-sail-pilot |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Split the monolithic step (build images + run tests inside a privileged pod) into three sequential steps: 1. servicemesh-istio-copy-src (new): copies source tree and kubeconfig into the builder pod via oc cp, following the same pattern as the existing servicemesh-sail-operator-copy-src step. 2. servicemesh-istio-images-build (new): oc rsh into the pod with SKIP_TEST_RUN=true to build and push Istio images to quay.io/sail-dev. Writes the derived image tag (PULL_PULL_SHA or BUILD_ID) to SHARED_DIR. 3. servicemesh-istio-int-tests (modified): runs prow/integ-suite-ocp.sh directly from the src step container (no pod). Reads the pre-built image tag and hub from SHARED_DIR/TEST_HUB. Security improvements: - Credentials are copied into the pod as files via oc cp instead of being interpolated into sh -c strings, preventing process table exposure. - docker login uses --password-stdin instead of -p flag. - KUBECONFIG is properly quoted in copy-src. Also renamed QUAY_HUB to TEST_HUB so push destination and pull source share a single variable, making it consistent with servicemesh-istio-int-tests and requiring only one env override in the CI config. Updated all affected test entries in openshift-service-mesh-istio-master.yaml (and release-1.24 through release-1.30 configs) to use the three-step sequence. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ac03ad0 to
407103b
Compare
|
/pj-rehearse pull-ci-openshift-service-mesh-istio-master-istio-integration-pilot-retest pull-ci-openshift-service-mesh-istio-master-istio-integration-sail-pilot |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Signed-off-by: Francisco Herrera <fjglira@gmail.com>
|
/pj-rehearse pull-ci-openshift-service-mesh-istio-master-istio-integration-pilot-retest |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Signed-off-by: Francisco Herrera <fjglira@gmail.com>
|
[REHEARSALNOTIFIER]
A total of 33 jobs have been affected by this change. The above listing is non-exhaustive and limited to 25 jobs. A full list of affected jobs can be found here Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/pj-rehearse pull-ci-openshift-service-mesh-istio-master-istio-integration-pilot-retest |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-openshift-service-mesh-istio-master-istio-integration-pilot-retest |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-openshift-service-mesh-istio-master-istio-integration-pilot-retest |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@fjglira: 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. |
3 participants
Long-lived oc rsh sessions fail with WebSocket close 1006 when the kube-apiserver rolls mid-run. This PR eliminates that failure mode by separating build and test into two short-lived steps.
Changes:
As a result, the test phase has no WebSocket session to drop; go test communicates with the cluster via the API server over KUBECONFIG, just like any other CI step.
Depends on this PR. We need to update first on istio midstream to be able to run rehearsal over these jobs
Summary by CodeRabbit
oc rshsessions during kube-apiserver rollouts.servicemesh-istio-images-buildstep in theci-operatorstep registry that builds and pushes the required Istio images toquay.io/sail-devusing a unique tag derived fromPULL_PULL_SHA(retries/re-triggers) orBUILD_ID(postsubmit), and writes that tag to${SHARED_DIR}/istio-image-tagfor downstream steps.servicemesh-istio-int-testsstep to runprow/integ-suite-ocp.shdirectly in the step container (usingKUBECONFIG=$SHARED_DIR/kubeconfigandSKIP_SETUP=true), removing the prior MAISTRA-basedoc rshexecution model./tmp/ISTIO_TESTS_DONE), debug/cleanup is simplified (nooc rshdebug), and JUnit results are copied from${ARTIFACT_DIR}/junit/*into${SHARED_DIR}.TEST_HUB(set toquay.io/sail-dev) so tests pull prebuilt images, and removes the previous execution dependency on MAISTRA runtime variables.servicemesh-istio-images-buildbefore their integration test steps and to setTEST_HUB=quay.io/sail-dev.openshift-service-mesh/istio#860being updated midstream before rehearsal jobs can run.