WIP: feat: allow deploying sno - #394
Conversation
Signed-off-by: ehila <ehila@redhat.com>
WalkthroughThe PR adds topology awareness to keepalived configuration validation. The ChangesKeepalived topology validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/monitor/dynkeepalived.go (1)
106-106: UseSingleReplicaTopologyModeconstant instead of hard-coding"SingleReplica"In
pkg/monitor/dynkeepalived.go(doesConfigChanged), the unicast backend-count validation currently checkscontrolPlaneTopology != "SingleReplica". Switch to the OpenShift API constant (configv1.SingleReplicaTopologyMode) to avoid drift/typos.Suggested patch
import ( "context" "fmt" "net" "net/http" "os" "os/signal" "strings" "syscall" "time" "github.com/google/go-cmp/cmp" + configv1 "github.com/openshift/api/config/v1" "github.com/sirupsen/logrus" "gopkg.in/yaml.v2" @@ - if controlPlaneTopology != "SingleReplica" { + if controlPlaneTopology != string(configv1.SingleReplicaTopologyMode) { log.Warnf("Invalid keepalived.conf - number of backends must be at least 2 got %d", len(curConfig.LBConfig.Backends)) validConfig = false }🤖 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/monitor/dynkeepalived.go` at line 106, In doesConfigChanged replace the hard-coded string comparison controlPlaneTopology != "SingleReplica" with the OpenShift constant configv1.SingleReplicaTopologyMode: import the configv1 package (k8s.io/.../config/v1) if not already imported, and change the condition to controlPlaneTopology != configv1.SingleReplicaTopologyMode so the unicast backend-count validation uses the official constant.
🤖 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/monitor/dynkeepalived.go`:
- Line 106: In doesConfigChanged replace the hard-coded string comparison
controlPlaneTopology != "SingleReplica" with the OpenShift constant
configv1.SingleReplicaTopologyMode: import the configv1 package
(k8s.io/.../config/v1) if not already imported, and change the condition to
controlPlaneTopology != configv1.SingleReplicaTopologyMode so the unicast
backend-count validation uses the official constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0a64976c-99b6-4a4a-9a5c-f56b314d8145
📒 Files selected for processing (1)
pkg/monitor/dynkeepalived.go
|
@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. |
Summary by CodeRabbit