OCPBUGS-85060: feat: use 2 replicas for console on tnf#1151
OCPBUGS-85060: feat: use 2 replicas for console on tnf#1151eggfoobar wants to merge 1 commit intoopenshift:mainfrom
Conversation
Signed-off-by: ehila <ehila@redhat.com>
|
@eggfoobar: This pull request references Jira Issue OCPBUGS-85060, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
WalkthroughThe ChangesHA Mode Detection
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing-upgrade periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing-upgrade |
|
@eggfoobar: trigger 2 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/75b4bc10-4a3f-11f1-926d-63d4ede17a0e-0 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: eggfoobar 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 |
|
@eggfoobar: This pull request references Jira Issue OCPBUGS-85060, which is valid. 3 validation(s) were run on this bug
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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/console/subresource/deployment/deployment.go (1)
141-146: ⚡ Quick winAdd table-driven tests for
ShouldDeployHAtopology permutations.This helper now controls replicas, anti-affinity, and rollout strategy for
DualReplicaTopologyMode; please add/extend tests forHighlyAvailable,HighlyAvailableArbiter,DualReplica,SingleReplica, andExternal + InfrastructureTopologycombinations to lock behavior and avoid regressions.🤖 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 `@pkg/console/subresource/deployment/deployment.go` around lines 141 - 146, Add a table-driven unit test for the ShouldDeployHA function that enumerates topology permutations: HighlyAvailableTopologyMode, HighlyAvailableArbiterMode, DualReplicaTopologyMode, SingleReplica (or non-HA) mode, and the ExternalTopologyMode combined with InfrastructureTopology == HighlyAvailableTopologyMode; for each case assert the expected boolean result. Put the test in the same package (pkg/console/subresource/deployment) and reference ShouldDeployHA and configv1.Infrastructure.Status.ControlPlaneTopology / InfrastructureTopology in the table entries so each permutation is clearly named and validated to prevent regressions.
🤖 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.
Nitpick comments:
In `@pkg/console/subresource/deployment/deployment.go`:
- Around line 141-146: Add a table-driven unit test for the ShouldDeployHA
function that enumerates topology permutations: HighlyAvailableTopologyMode,
HighlyAvailableArbiterMode, DualReplicaTopologyMode, SingleReplica (or non-HA)
mode, and the ExternalTopologyMode combined with InfrastructureTopology ==
HighlyAvailableTopologyMode; for each case assert the expected boolean result.
Put the test in the same package (pkg/console/subresource/deployment) and
reference ShouldDeployHA and configv1.Infrastructure.Status.ControlPlaneTopology
/ InfrastructureTopology in the table entries so each permutation is clearly
named and validated to prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d382e74d-0d42-4bbe-9162-f2422a7ac076
📒 Files selected for processing (1)
pkg/console/subresource/deployment/deployment.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
pkg/**/*.go
⚙️ CodeRabbit configuration file
pkg/**/*.go: Review Go code following OpenShift operator patterns.
See CONVENTIONS.md for coding standards and patterns.
Controllers should use the library-go factory pattern.
Status conditions should use status.Handle* functions.
Files:
pkg/console/subresource/deployment/deployment.go
|
@eggfoobar: 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. |
Analysis / Root cause:
In TNF (Two Node Fencing) clusters we are currently only deploying a single replica console, this is causing issues during upgrade as one pod moves over to the other node and causes disruption. For all intents and purposes TNF should be considered like Arbiter and thus will require 2 replicas.
Solution description:
Updating the function to create the deployment with 2 replicas instead of 1.
Test setup:
Executing two node fencing upgrade payload job should not trigger
[Monitor:legacy-cvo-invariants][bz-Management Console] clusteroperator/console should not change condition/AvailablefailureTest cases:
Browser conformance:
N/A
Additional info:
Reviewers and assignees:
Summary by CodeRabbit