OCPBUGS-74528: upkeep: HighlyAvailableArbiter has been GA for 2 releases#2759
OCPBUGS-74528: upkeep: HighlyAvailableArbiter has been GA for 2 releases#2759eggfoobar wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
Hello @eggfoobar! Some important instructions when contributing to openshift/api: |
|
@eggfoobar: This pull request references Jira Issue OCPBUGS-74528, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci
bot
added
the
size/XXL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (18)
💤 Files with no reviewable changes (17)
📝 WalkthroughWalkthroughThe PR removes the HighlyAvailableArbiter feature gate and its usages across the repo: deleted the exported feature gate entry, removed HighlyAvailableArbiter from multiple FeatureGate payload manifests and feature lists, and deleted related test files and generated feature-gated CRD entries. The InfrastructureStatus.ControlPlaneTopology validation annotations were simplified to drop per-featureGate/requiredFeatureGate constraints (default remains HighlyAvailable). Documentation row for HighlyAvailableArbiter was removed and an arbiter-specific short-circuit was deleted from the featuregate test analyzer. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.3)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoRemove HighlyAvailableArbiter feature gate - GA for 2 releases
WalkthroughsDescription• Remove HighlyAvailableArbiter feature gate after GA stabilization • Make HighlyAvailableArbiter topology mode always available • Update validation rules to allow arbiter mode unconditionally • Fix code formatting and indentation in feature gate definitions Diagramflowchart LR
A["HighlyAvailableArbiter<br/>Feature Gate"] -->|Remove| B["Always Available<br/>Topology Mode"]
C["Feature Gate<br/>Validation Rules"] -->|Simplify| D["Unconditional<br/>Enum Values"]
E["CRD Manifests"] -->|Update| F["Include Arbiter<br/>in All Profiles"]
File Changes1. features/features.go
|
Code Review by Qodo
1. controlPlaneTopology enum undocumented
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // +openshift:validation:FeatureGateAwareEnum:featureGate="",enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;External | ||
| // +openshift:validation:FeatureGateAwareEnum:featureGate=DualReplica,enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;External |
There was a problem hiding this comment.
1. controlplanetopology enum undocumented 📘 Rule violation ✓ Correctness
The controlPlaneTopology field comment does not document the HighlyAvailableArbiter and DualReplica enum values that are now allowed by validation markers. This leaves API consumers without an explanation of the behavior/meaning of these permitted values.
Agent Prompt
## Issue description
The `controlPlaneTopology` field's validation markers allow additional enum values (`HighlyAvailableArbiter` and `DualReplica`), but the field comment does not explain those values.
## Issue Context
Compliance requires that enum constraints implied by validation markers be documented in the field comment so users understand the API contract.
## Fix Focus Areas
- config/v1/types_infrastructure.go[99-109]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
@eggfoobar: 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. |
|
Looks like the integration tests still reference the gate, that will need to be removed What payload E2E can we run to check that this PR doesn't break any existing jobs? Is there a way to run an arbiter job? |
removing the HighlyAvailableArbiter featureGate, the feature has been GA for 2 releases Signed-off-by: ehila <ehila@redhat.com>
eggfoobar
force-pushed
the
upkeep-remove-arbiter-featuregate
branch
from
405f89c to
821a1ab
Compare
Ah yup, updated the integration tests! As it stands the installer should be the only one that checks for the feature gate, the other tooling relies on the control plane topology or compute count to execute flows. We should be able to execute some payloads on this PR, but I think the cluster config operator update will be the one to show any failures. I'll open a PR there with a temp update to go mod to validate |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-arbiter |
|
@eggfoobar: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/42c8af30-2237-11f1-9a8b-2e91671672d5-0 |
| featureGates: | ||
| - DualReplica | ||
| - HighlyAvailableArbiter |
There was a problem hiding this comment.
Are these tests no longer relevant? We can keep them if they still make sense, just gotta drop the gates
There was a problem hiding this comment.
Oh this was that sort of hack we had to do because DualReplica and HighlyAvailableArbiter were occupying the same field, this file was just a merging of the tests in DualReplica and HighlyAvailableArbiter files, now that HighlyAvailableArbiter is default, we don't need this file anymore.
The tests this was checking for are still present with their respective DualReplica and HighlyAvailableArbiter file.
.../controllerconfigs.machineconfiguration.openshift.io/HighlyAvailableArbiter+DualReplica.yaml
Show resolved
Hide resolved
3 participants
removing the HighlyAvailableArbiter featureGate, the feature has been GA for 2 releases