OPNET-773: network: add BGP-based VIP management enhancement proposal#1982
OPNET-773: network: add BGP-based VIP management enhancement proposal#1982mkowalski wants to merge 6 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@mkowalski: This pull request references OPNET-773 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
|
[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 |
cff5714 to
4d7317d
Compare
Introduce an enhancement proposal for replacing keepalived/VRRP-based VIP management with a BGP-based approach using kube-vip (Routing Table Mode) and frr-k8s deployed as static pods on bare metal clusters. Closes: https://redhat.atlassian.net/browse/OPNET-773 Epic: https://redhat.atlassian.net/browse/OPNET-595
4d7317d to
a4a5e10
Compare
|
/cc @cybertron |
|
|
||
| #### Single-node Deployments or MicroShift | ||
|
|
||
| For single-node OpenShift (SNO), BGP-based VIP management is applicable but has |
There was a problem hiding this comment.
Does SNO use keepalived today? We prevent keepalived from starting with less than two nodes: https://github.com/openshift/baremetal-runtimecfg/blob/dca8cb1be0cce1ce404dfd407ead1326a3c8bb40/pkg/monitor/dynkeepalived.go#L105
I suppose SNO could disable unicast though, which would at least allow it to run.
There was a problem hiding this comment.
SNO does not use keepalived today. But I would like SNO to be able to use BGP tomorrow.
|
|
||
| ## Summary | ||
|
|
||
| This enhancement proposes replacing the current keepalived/VRRP-based Virtual IP |
| - **Bootstrap phase:** The installer generates a per-node `frr.conf` for each | ||
| host. If `host.bgpPeers` is set, that host's `frr.conf` uses the | ||
| host-specific peers; otherwise, it uses the global `bgpVIPConfig.peers`. | ||
| MCO renders the correct `frr.conf` for each node via per-node MachineConfig. |
There was a problem hiding this comment.
per-node MachineConfig
This is not a thing today. How do you intend to apply per-node configs?
There was a problem hiding this comment.
Very good point. As we are vibe-coding here, what we got is to follow the pattern we have in keepalived, i.e. sidecar/init container which will render the configuration for the specific node (equivalent of keepalived-monitor).
The good part is that we will only need it for bootstrap because at runtime we already have the FRRConfiguration CRD which has its own nodeSelector support.
Will update the enhancement doc soon.
| address: "ipmi://192.168.111.101" | ||
| username: admin | ||
| password: password | ||
| bgpPeers: |
There was a problem hiding this comment.
Because of where this field is, it will be baremetal-specific. Do you anticipate needing per-host configs on other platforms?
There was a problem hiding this comment.
Yeah, sadly yes. There is nothing so much baremetal-specific here. Whoever has the BGP feature, needs the ability to configure this. Probably will make more sense to move this section somewhere else (otherwise the API will be full of copy-pasta)
There was a problem hiding this comment.
I have checked and I am not sure we should improve the current state. Not all the platforms even contain the hosts[] array, e.g. vSphere does not seem to have it. Same for platform: none.
I feel at this point it should stay in platform.baremetal.hosts[].bgpPeers and once the per-host configuration is solved in general in our API, we can piggyback.
| partitioned nodes. Since the API server runs on all control plane nodes, API | ||
| requests will succeed regardless of which node receives them -- the client | ||
| reaches a valid kube-apiserver instance either way. This is a transient | ||
| condition, not a data-loss scenario. |
There was a problem hiding this comment.
This is true for the API VIP, but not for the ingress VIP. We'll probably want something like the fault mode for keepalived that will prevent it from ever taking the VIP if there is no ingress service running on the node.
There was a problem hiding this comment.
Correct. For Ingress VIP we want to have a health check. I guess that would be haproxy port? From the kubevip docs (https://kube-vip.io/docs/usage/bgp-health-check/) it seems to me this is doable. In case the check fails, we would not be announcing the route.
This needs a bit of additional tooling because kubevip on its own supports one VIP per instance. So we'd need one process for API and one process for Ingress. The one for Ingress would also need a sidecar that would be probing the haproxy port because kubevip's check works for BGP mode, but our desired mode is Routing Table Mode (so that frr-k8s does the actual BGP).
It slightly increases complexity.
There was a problem hiding this comment.
+ we will need a patch in kubevip (but this can be downstream-only)
There was a problem hiding this comment.
Based on the POC I ran, this is not an issue any more. Kube-vip has health-check mode (feature already merged upstream) and will only announce Ingress VIP from the node running the router.
|
|
||
| ## Graduation Criteria | ||
|
|
||
| ### Dev Preview -> Tech Preview |
There was a problem hiding this comment.
Are we actually planning a dev preview for this feature?
There was a problem hiding this comment.
Nah, the whole DP TP GA is just a corporate lingo but we probably go our usual TP GA unless something changes and DP is enforceable at the time when this feature comes to its existence.
There was a problem hiding this comment.
I retract the comment above. I actually want to do DevPreview first. Maybe we will graduate to GA directly, but the first stage will be DP and not TP
…ate rendering Replace the incorrect claim of 'per-node MachineConfig' for delivering per-node FRR configuration with a runtime template rendering approach using an FRR config renderer sidecar in the frr-k8s static pod. This follows the established keepalived/baremetal-runtimecfg pattern: MCO delivers a single, identical MachineConfig to all master nodes containing a Go template and a JSON peer mapping file keyed by hostname. The sidecar discovers the local node's hostname and primary IP at startup, resolves the correct BGP peer list from the mapping, and renders the final frr.conf. Addresses review feedback from @cybertron regarding per-node MachineConfig not being a thing today.
… BGP peers Document that hosts[].bgpPeers is inherently baremetal-specific since only baremetal has a hosts[] array. Other on-prem platforms will use post-bootstrap FRRConfiguration CRs with nodeSelector for per-node peer overrides. Define BGPPeerConfig and BGPVIPConfig as reusable Go types in a shared package, with each platform struct referencing them independently -- following the established apiVIPs/ingressVIPs/PlatformLoadBalancerType precedent. Add open question about the long-term cross-platform per-node peer override strategy for non-baremetal platforms. Addresses review feedback from @cybertron regarding baremetal-specific API placement.
…istinction Add a service readiness check for the ingress VIP in the kube-vip section: kube-vip periodically probes the local haproxy stats port (29445) and relinquishes leadership if the ingress controller is not running, equivalent to keepalived's vrrp_script mechanism. Update the split-brain analysis to distinguish API VIP (benign ECMP, kube-apiserver runs on all control plane nodes) from ingress VIP (mitigated by the readiness check withdrawing the BGP route on nodes without a healthy ingress controller). Addresses review feedback from @cybertron regarding ingress VIP fault mode.
…lth check Restructure kube-vip deployment as two separate static pods: - kube-vip-api.yaml: manages the API VIP from bootstrap, uses the built-in Kubernetes API backend health check to gate route presence in table 198. - kube-vip-ingress.yaml: manages the Ingress VIP post-bootstrap, deployed by CNO via MCO MachineConfig update. Uses a downstream HTTP backend health check enhancement to probe the local haproxy stats port (29445) instead of the Kubernetes API. Two instances are required because kube-vip's address env var accepts only a single IP -- there is no upstream support for multiple control plane VIPs in one instance. Document the downstream kube-vip enhancement: a ~50-line change that adds a backend_health_check_url env var. When set, the Routing Table Mode reconciliation loop performs an HTTP GET instead of Kubernetes API discovery. This is needed because upstream Entry.Check() in pkg/backend/backend.go is hardcoded to client.DiscoveryClient. ServerVersion(), which only works against a real Kubernetes API server. Update the installation workflow, static pod startup ordering, split-brain analysis, alternatives section, and cross-team dependencies table to reflect the two-instance model.
The design was implemented across all affected repositories and validated end to end (github.com/mkowalski/bgp-vip-demo). Corrections: health-gated ECMP is the actual routing-table mode advertisement model; the CRD handover carries sessions while advertisement stays on gated table-direct redistribution; ingress health is the router endpoint on 1936; the node kubeconfig identity is the MCO node-bootstrapper ServiceAccount; DaemonSet avoidance is role-based (NodeRestriction forbids the label approach); the bootstrap static pod is FRR-only; FRR needs the 10.7 zebra import-table fix backported; the peer file schema matches runtimecfg's FRRPeerMapping verbatim.
|
@mkowalski: 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. |
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
Quite a lot - not too familiar with the networking context, so left some questions from MCO/API POV
|
|
||
| - **Static pod manifest generation**: The installer will generate the initial | ||
| static pod manifest for frr-k8s and place it in | ||
| `/etc/kubernetes/manifests/` on the bootstrap node and initial control plane |
There was a problem hiding this comment.
To clarify, this static pod needs to run on the bootstrap node itself?
For the initial control plane nodes I think it would make more sense to have the bootstrap MCO path render if, it's already doing it for the in-cluster version. I don't think we would need to explicitly have the installer generate anything for them
There was a problem hiding this comment.
We need it on bootstrap node. Today bootstrap node runs keepalived and holds the API VIP. Tomorrow bootstrap node needs to run FRR to hold the API VIP.
Running bootstrap node without API VIP coming from FRR or keepalived would be a monster redesign.
| `/etc/kubernetes/manifests/` on the bootstrap node and initial control plane | ||
| nodes. Post-bootstrap, the Machine Config Operator (MCO) owns these | ||
| manifests. MCO will render updated MachineConfig resources containing the | ||
| frr-k8s static pod manifest for control plane nodes, ensuring the manifests |
There was a problem hiding this comment.
One thing to note is that every update, the MCO will re-write the static pod definition before it does the other parts of the update (including pulling the OS). This has caused issues with the coredns static pod since it will immediately restart and cause some network unavailability. Do you foresee similar issues with this static pod restarting mid update?
This also means that each node will be updating this pod independently when it's their turn to update, but I assume that's fine and the old and new pods are compatible with each other.
There was a problem hiding this comment.
Yeah, no incompatibility on that front. There are reboots expected during upgrade. There are network instabilities expected during upgrade. But we do the same reboots/restarts with keepalived now which runs as static pod.
| provisioning path); metrics delivery will be revisited for the Tech | ||
| Preview observability criteria. Peer data reaches MCO from the | ||
| installer-generated `bgp-vip-config` ConfigMap: the bootstrap render | ||
| reads the ConfigMap manifest from the installer asset directory, and |
There was a problem hiding this comment.
I'm not sure I understand this sequence. Why is the bootstrap workflow different? Could the bootstrap-time MCO not consume the install config object and render the necessary configuration?
Also, even though we think of controllerconfig as an internal-use API, technically all OpenShift API is customer facing, so we don't really have an "internal API field"
There was a problem hiding this comment.
FRR-k8s is a fully-fledge operator, not just one container/process. It owns CRDs and bunch of stuff which require k8s API.
Bootstrap FRR on the other hand needs to run before we have k8s API (because it will give node the IP address which will be used by k8s API). So it needs to be stripped-down version of FRR-k8s.
Also, even though we think of controllerconfig as an internal-use API, technically all OpenShift API is customer facing, so we don't really have an "internal API field"
Agreed with that 10/10
| - CNO waits for the `FRRNodeState` CR on each node to report that the | ||
| CRD-based configuration has been applied and BGP sessions are | ||
| established. | ||
| - Once verified, CNO updates the MCO MachineConfig to remove the static |
There was a problem hiding this comment.
Hmm, ok I think this partially answers some of my earlier questions. This workflow is technically not a supported operation (we don't allow drifts between install time and in-cluster configs). Even if there is no race condition (i.e. we're sure the MCO has done a render and completed everything before the CNO does this update) we would prefer not to have to do this operation for an immediate update.
One possible mechanism is for the bootstrap generated configs to be populated via the bootstrap ignition path instead (not in MachineConfigs, not tracked) and then have some daemon logic (?) to remove it once the MCD runs and applies the new configs. Not sure about that though - we try to ensure that the bootstrap-time MCO has all the necessary information to generate the final configs on the firstboot of the control plane nodes. Is that not possible here?
There was a problem hiding this comment.
I need to digest this comment and get back to it later. I don't really understand the "one possible mechanism [...]" way.
| - "64512:100" | ||
| ``` | ||
|
|
||
| Proposed API additions to the platform-specific bare metal configuration: |
There was a problem hiding this comment.
Are these only install time configurations? If I try to switch to BGP in a running cluster is there any equivalent implementation for this? Or I guess this is only a install-time feature?
There was a problem hiding this comment.
Now install-time feature. In the future, maybe changeable
|
|
||
| The per-host peer override works as follows: | ||
|
|
||
| - **Bootstrap phase:** MCO delivers a single, identical MachineConfig to all |
There was a problem hiding this comment.
As mentioned above, I'm not sure this should be a machineconfig
| // This field is only populated when vipManagement is "BGP". | ||
| // +openshift:enable:FeatureGate=BGPBasedVIPManagement | ||
| // +optional | ||
| BGPVIPStatus *BGPVIPStatus `json:"bgpVIPStatus,omitempty"` |
There was a problem hiding this comment.
This field doesn't seem to be there in openshift/api#2923 ?
There was a problem hiding this comment.
This is actually outdated.
During the POC implementation I realized that we have frr-k8s CRDs BGPSessionState/FRRNodeState which hold the status.
That would mean I need CNO to continuously copy-paste status between those CRDs and BareMetalPlatformStatus.
| } | ||
| ``` | ||
|
|
||
| **Phase 2 -- Sidecar renders per-node config at runtime:** |
There was a problem hiding this comment.
Still not entirely clear, is it that:
- MCO bootstrap renders a general config and static pod manifests
- MCO in-cluster renders no config and the same static pod manifests
Or something different?
There was a problem hiding this comment.
Installer contains full config for every node. MCO gets this full config and places it on every node (because we don't have per-node MachineConfig). Sidecar reads the full config and renders per-node config.
This is a workaround for not being able to place different files on different nodes at install-time.
4 participants
Introduce an enhancement proposal for replacing keepalived/VRRP-based VIP management with a BGP-based approach using kube-vip (Routing Table Mode) and frr-k8s deployed as static pods on bare metal clusters.
Closes: https://redhat.atlassian.net/browse/OPNET-773
Epic: https://redhat.atlassian.net/browse/OPNET-595