Skip to content

Conversation

@jmguzik
Copy link
Contributor

@jmguzik jmguzik commented Dec 23, 2025

When sanitize-prow-jobs runs during validation, it now preserves existing cluster assignments if the cluster is a valid build farm member and not blocked. This prevents validation failures caused by minor cluster field inconsistencies between dispatcher assignments and the deterministic algorithm, while still recalculating when the assignment is invalid.

…ation

When sanitize-prow-jobs runs during validation, it now preserves existing
cluster assignments if the cluster is a valid build farm member and not
blocked. This prevents validation failures caused by minor cluster field
inconsistencies between dispatcher assignments and the deterministic
algorithm, while still recalculating when the assignment is invalid.

Signed-off-by: Jakub Guzik <jguzik@redhat.com>
@openshift-ci-robot
Copy link
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: automatic mode

@coderabbitai
Copy link

coderabbitai bot commented Dec 23, 2025

Walkthrough

Adds early cluster preservation logic in determineCluster() to return existing valid BuildFarm clusters instead of computing new ones when pjs is nil. Includes corresponding test coverage for cluster preservation behavior across multiple scenarios.

Changes

Cohort / File(s) Summary
Production Logic
pkg/sanitizer/determinize.go
Added guard clause in determineCluster() that returns existing non-blocked cluster if already assigned and recognized by build-farm (via config.IsInBuildFarm), short-circuiting subsequent relocation logic.
Test Coverage
pkg/sanitizer/determinize_test.go
Added TestDetermineClusterPreservesValidBuildFarmCluster() function with new api package import to validate cluster selection across BuildFarm clusters, blocked clusters, and JobGroup scenarios.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 4a066ea and cddfe66.

📒 Files selected for processing (2)
  • pkg/sanitizer/determinize.go
  • pkg/sanitizer/determinize_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

  • pkg/sanitizer/determinize_test.go
  • pkg/sanitizer/determinize.go
🧬 Code graph analysis (1)
pkg/sanitizer/determinize_test.go (2)
pkg/api/types.go (3)
  • Cloud (904-904)
  • Cluster (749-749)
  • CloudAWS (907-907)
pkg/dispatcher/config.go (3)
  • BuildFarmConfig (54-57)
  • JobGroups (60-60)
  • Config (32-52)
🔇 Additional comments (3)
pkg/sanitizer/determinize_test.go (2)

9-9: LGTM!

The import is necessary for referencing api.Cloud and api.Cluster types in the new test cases.


52-139: Remove this comment. The test assumptions are correct—the DetermineClusterForJob algorithm does respect the Groups configuration. Lines 230–244 in pkg/dispatcher/config.go explicitly match job names to clusters via config.Groups, so test cases 2, 3, and 4 will correctly resolve to "special-cluster" when the job name "test-job" matches the Groups entry.

pkg/sanitizer/determinize.go (1)

144-150: Code is correct as written.

The IsInBuildFarm method (defined in pkg/dispatcher/config.go:295) returns api.Cloud and yields either the cloud provider name or an empty string. The check config.IsInBuildFarm(api.Cluster(jb.Cluster)) != "" correctly verifies whether a cluster belongs to the build farm. The early preservation logic at lines 144-150 properly guards valid cluster assignments and aligns with the PR objective.

Note: While the "Is..." naming convention typically suggests a boolean return in Go, the API returns a string; this is the existing method contract and the reviewed code uses it correctly.


Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 23, 2025
@danilo-gemoli
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 23, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 23, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danilo-gemoli, jmguzik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [danilo-gemoli,jmguzik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 4a066ea and 2 for PR HEAD cddfe66 in total

@jmguzik
Copy link
Contributor Author

jmguzik commented Dec 23, 2025

/retest

@jmguzik
Copy link
Contributor Author

jmguzik commented Dec 23, 2025

/test breaking-changes

@jmguzik
Copy link
Contributor Author

jmguzik commented Dec 23, 2025

/test checkconfig frontend-checks images integration

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 23, 2025

@jmguzik: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/images cddfe66 link true /test images
ci/prow/frontend-checks cddfe66 link true /test frontend-checks
ci/prow/checkconfig cddfe66 link true /test checkconfig
ci/prow/integration cddfe66 link true /test integration
ci/prow/breaking-changes cddfe66 link false /test breaking-changes

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants