NO-JIRA: MCO jobs: re-add cloud jobs to standard, default to candidate#3400
NO-JIRA: MCO jobs: re-add cloud jobs to standard, default to candidate#3400yuqi-zhang wants to merge 1 commit intoopenshift:mainfrom
Conversation
This changes the MCO logic a bit to always start in candidate, and explicitly set AWS/GCP/Azure disruptive and disruptive-techpreview jobs to standard. With this, - e2e-aws-mco-fips-proxy-longduration - e2e-aws-ovn-day1-ocl - e2e-vsphere-mco-tp-longduration - update-amis Has been moved to candidate instead since I'm not sure they should have been standard by default. The cloud jobs should be stable enough, and is intended to serve as our component readiness for MCO specific tests.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@yuqi-zhang: 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. |
WalkthroughThe pull request updates job-tier configurations for machine-config-operator (MCO) jobs in the variant registry. The Go code refactors hardcoded tier mappings to use broader defaults with explicit cloud MCO disruptive job overrides, while the YAML snapshot reflects corresponding tier reassignments across multiple periodic job entries. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yuqi-zhang 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/variantregistry/ocp.go (1)
758-759: Align matcher scope with the “periodic jobs” intent.Line 759 matches any job containing
machine-config-operator, while the comment on Line 758 says periodic jobs only. Consider narrowing this pattern to periodic job names to avoid accidental tiering of non-periodic jobs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/variantregistry/ocp.go` around lines 758 - 759, The matcher entry {[]string{"machine-config-operator"}, "candidate"} is too broad and will match non-periodic jobs; narrow the pattern to only periodic job names by replacing that token with a periodic-specific matcher (for example change to the periodic suffix or an anchored regex such as matching "-periodic" jobs) in the pkg/variantregistry/ocp.go entry so only jobs intended as "periodic" (e.g., names like machine-config-operator-*-periodic) are tiered to "candidate".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/variantregistry/ocp.go`:
- Around line 748-756: The candidate match for
machine-config-operator-release-4.19/4.20 is shadowing the more specific cloud
disruptive overrides; move the cloud disruptive entries
({[]string{"e2e-aws-mco-disruptive"}, "standard"},
{[]string{"e2e-azure-mco-disruptive"}, "standard"},
{[]string{"e2e-gcp-mco-disruptive"}, "standard"}) so they appear before the
generic machine-config-operator-release-4.19 and -4.20 candidate entries in the
variant matching list, ensuring the specific "standard" overrides are evaluated
first.
---
Nitpick comments:
In `@pkg/variantregistry/ocp.go`:
- Around line 758-759: The matcher entry {[]string{"machine-config-operator"},
"candidate"} is too broad and will match non-periodic jobs; narrow the pattern
to only periodic job names by replacing that token with a periodic-specific
matcher (for example change to the periodic suffix or an anchored regex such as
matching "-periodic" jobs) in the pkg/variantregistry/ocp.go entry so only jobs
intended as "periodic" (e.g., names like machine-config-operator-*-periodic) are
tiered to "candidate".
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 38f91db3-36cb-4fba-bdf0-61d72c900209
📒 Files selected for processing (2)
pkg/variantregistry/ocp.gopkg/variantregistry/snapshot.yaml
| // All 4.19/4.20 MCO jobs default to candidate | ||
| {[]string{"machine-config-operator-release-4.19"}, "candidate"}, | ||
| {[]string{"machine-config-operator-release-4.20"}, "candidate"}, | ||
|
|
||
| // Set remaining periodic MCO jobs to standard for component readiness | ||
| {[]string{"-mco-disruptive"}, "candidate"}, | ||
| // Cloud MCO disruptive jobs set to standard for component readiness | ||
| // This also includes techpreview variants | ||
| {[]string{"e2e-aws-mco-disruptive"}, "standard"}, | ||
| {[]string{"e2e-azure-mco-disruptive"}, "standard"}, | ||
| {[]string{"e2e-gcp-mco-disruptive"}, "standard"}, |
There was a problem hiding this comment.
Cloud standard overrides are currently shadowed by earlier candidate matches.
On Line 749 and Line 750, the function returns candidate before Line 754–756 can match, so 4.19/4.20 cloud disruptive MCO jobs won’t be classified as standard.
Proposed fix (reorder patterns so specific cloud overrides run first)
- // All 4.19/4.20 MCO jobs default to candidate
- {[]string{"machine-config-operator-release-4.19"}, "candidate"},
- {[]string{"machine-config-operator-release-4.20"}, "candidate"},
-
// Cloud MCO disruptive jobs set to standard for component readiness
// This also includes techpreview variants
{[]string{"e2e-aws-mco-disruptive"}, "standard"},
{[]string{"e2e-azure-mco-disruptive"}, "standard"},
{[]string{"e2e-gcp-mco-disruptive"}, "standard"},
+
+ // All 4.19/4.20 MCO jobs default to candidate
+ {[]string{"machine-config-operator-release-4.19"}, "candidate"},
+ {[]string{"machine-config-operator-release-4.20"}, "candidate"},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // All 4.19/4.20 MCO jobs default to candidate | |
| {[]string{"machine-config-operator-release-4.19"}, "candidate"}, | |
| {[]string{"machine-config-operator-release-4.20"}, "candidate"}, | |
| // Set remaining periodic MCO jobs to standard for component readiness | |
| {[]string{"-mco-disruptive"}, "candidate"}, | |
| // Cloud MCO disruptive jobs set to standard for component readiness | |
| // This also includes techpreview variants | |
| {[]string{"e2e-aws-mco-disruptive"}, "standard"}, | |
| {[]string{"e2e-azure-mco-disruptive"}, "standard"}, | |
| {[]string{"e2e-gcp-mco-disruptive"}, "standard"}, | |
| // Cloud MCO disruptive jobs set to standard for component readiness | |
| // This also includes techpreview variants | |
| {[]string{"e2e-aws-mco-disruptive"}, "standard"}, | |
| {[]string{"e2e-azure-mco-disruptive"}, "standard"}, | |
| {[]string{"e2e-gcp-mco-disruptive"}, "standard"}, | |
| // All 4.19/4.20 MCO jobs default to candidate | |
| {[]string{"machine-config-operator-release-4.19"}, "candidate"}, | |
| {[]string{"machine-config-operator-release-4.20"}, "candidate"}, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/variantregistry/ocp.go` around lines 748 - 756, The candidate match for
machine-config-operator-release-4.19/4.20 is shadowing the more specific cloud
disruptive overrides; move the cloud disruptive entries
({[]string{"e2e-aws-mco-disruptive"}, "standard"},
{[]string{"e2e-azure-mco-disruptive"}, "standard"},
{[]string{"e2e-gcp-mco-disruptive"}, "standard"}) so they appear before the
generic machine-config-operator-release-4.19 and -4.20 candidate entries in the
variant matching list, ensuring the specific "standard" overrides are evaluated
first.
There was a problem hiding this comment.
This is the behaviour we want. 4.19/4.20 cloud test are not stable enough for standard.
There was a problem hiding this comment.
@yuqi-zhang, understood — the ordering is intentional so that 4.19/4.20 cloud MCO jobs are caught first and kept as candidate, while the cloud standard entries only apply to non-4.19/4.20 releases. Thanks for the clarification!
📝 Learnings were identified and not saved (knowledge base disabled). Enable
|
@yuqi-zhang: The following test 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. |
2 participants
This changes the MCO logic a bit to always start in candidate, and explicitly set AWS/GCP/Azure disruptive and disruptive-techpreview jobs to standard. With this,
Has been moved to candidate instead since I'm not sure they should have been standard by default. The cloud jobs should be stable enough, and is intended to serve as our component readiness for MCO specific tests.
For older context see #3393 and #3094
Summary by CodeRabbit