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 14 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.
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.
Dev Preview carries the peer payload as serialized JSON on MCO's internal ControllerConfig (agreed in openshift/api#2923 review) so the schema can bake. Tech Preview replaces the ConfigMap and the JSON field with a cluster-scoped BGPVIPConfig CRD: one admission-validated schema for installer, CNO and MCO, passwordSecretRef instead of inline passwords, and a real day-2 flow (watches, status conditions, a NodeDisruptionPolicy so peer changes do not reboot nodes). GA hardens the same CRD and removes the ConfigMap path.
…review Option A: Infrastructure.spec.platformSpec.baremetal.bgp - precedented by the day-2 VIP editing fields, no new CRD, but fans every peer edit out to every Infrastructure watcher and offers no feature-scoped conditions. Option B: dedicated BGPVIPConfig CRD - two watchers, native status conditions, schema iteration room for the multi-rack hostOverrides shape. Preference: Option B; the decision is settled with API reviewers before Tech Preview implementation.
…ctured BGP API Option A (Infrastructure.spec.platformSpec.baremetal.bgp) is the primary choice: it extends the object that already owns the on-prem VIP surface and its day-2 editing flow. The dedicated BGPVIPConfig CRD remains the fallback if API review decides the Infrastructure CR should not grow this schema.
| - **ControllerConfigSpec.BGPVIPPeersJSON (openshift/api, | ||
| machineconfiguration/v1)**: a feature-gated, optional string field on | ||
| MCO's internal ControllerConfig API carrying the `bgp-vip-config` | ||
| ConfigMap's `config.json` payload (validated and compacted by the MCO | ||
| operator) so the template controller can render the node peer file. Not | ||
| a user-facing API; only populated when BGP VIP management is active. | ||
| **Dev Preview only**: the serialized-JSON form exists to let the payload | ||
| schema bake without API commitments. It is replaced for Tech Preview by | ||
| the structured `BGPVIPConfig` CRD described below. | ||
|
|
||
| - **Structured BGP configuration API (Tech Preview)**: the Dev Preview | ||
| ConfigMap and serialized-JSON ControllerConfig field are replaced by a | ||
| single admission-validated API. Two candidate placements are under | ||
| consideration; the decision is made with the API reviewers before Tech | ||
| Preview implementation starts. | ||
|
|
||
| **Option A - `Infrastructure.spec.platformSpec.baremetal.bgp`**: a typed | ||
| struct on the existing Infrastructure CR spec. | ||
|
|
||
| - Precedent: `BareMetalPlatformSpec` already serves day-2 editable | ||
| on-prem networking (`apiServerInternalIPs`/`ingressIPs` for VIP | ||
| changes), with the established spec-to-status propagation flow. | ||
| Secret references from config-group objects are also established | ||
| (APIServer `servingCerts`, Proxy `trustedCA`). | ||
| - Pros: no new CRD lifecycle; co-located with `vipManagement` and the | ||
| VIP fields; discoverable where operators already look for on-prem | ||
| networking; bootstrap consumes the Infrastructure manifest unchanged. | ||
| - Cons: Infrastructure is watched by nearly every operator and node | ||
| agent, so every peer edit fans a full-object update to the fleet - | ||
| `hostOverrides` scales with host count (multi-rack: per-host peer | ||
| lists), making the hottest object in the cluster hotter; there is no | ||
| feature-scoped status/conditions surface (only value mirroring into | ||
| `platformStatus`), so the day-2 feedback loop is limited; fields in | ||
| `config.openshift.io/v1` are permanent on arrival, while the peer | ||
| schema is still young (it changed twice during Dev Preview). | ||
| - The cons are mitigated by keeping the schema lean where possible and | ||
| accepting value mirroring into `platformStatus` as the feedback | ||
| mechanism; `hostOverrides` size at multi-rack scale is the one factor | ||
| that could tip the decision to Option B. | ||
|
|
||
| **Option B - dedicated `BGPVIPConfig` CRD | ||
| (machineconfiguration.openshift.io, cluster-scoped, feature-gated)**, | ||
| replacing both the `bgp-vip-config` ConfigMap and the serialized-JSON | ||
| ControllerConfig field as the single source of BGP peer configuration: | ||
|
|
||
| - The installer generates the `BGPVIPConfig` manifest from | ||
| `install-config.yaml` (the bootstrap MCO render consumes the manifest | ||
| file exactly as it consumes the ConfigMap manifest today, so the | ||
| bootstrap flow is unchanged). | ||
| - CNO and the MCO operator both watch the CR: CNO renders the | ||
| `FRRConfiguration` from it, MCO populates an operator-internal typed | ||
| copy on `ControllerConfigSpec` (replacing `BGPVIPPeersJSON`) for the | ||
| template render. One schema, admission-validated, no hand-mirrored | ||
| JSON contracts. | ||
| - Typed shape fixes the Dev Preview payload warts: `metav1.Duration` | ||
| for hold/keepalive times, booleans instead of `"true"` strings, | ||
| list-map `hostOverrides`, CEL validation for ASNs and peer addresses, | ||
| and `passwordSecretRef` (a `kubernetes.io/basic-auth` Secret | ||
| reference) instead of an inline plaintext password. The | ||
| `apiVIPs`/`ingressVIPs` duplication is dropped: templates read VIPs | ||
| from the Infrastructure CR as they do for keepalived. | ||
| - Day-2 reconfiguration becomes a first-class flow: `oc edit | ||
| bgpvipconfig cluster` is validated at admission, both consumers react | ||
| via watches, and the CR carries status conditions | ||
| (`observedGeneration`, rendered/applied) so changes have a feedback | ||
| loop. A NodeDisruptionPolicy ships alongside so peer-file updates do | ||
| not reboot nodes (after the bootstrap-to-CRD handover the on-disk | ||
| config only matters at early boot). | ||
| - GA hardens the same CRD (no shape change expected): status conditions | ||
| complete, day-2 flows covered by e2e, and the Dev Preview ConfigMap | ||
| path removed. | ||
| - Pros: watched by exactly two consumers (CNO, MCO operator); native | ||
| status conditions for the day-2 feedback loop; the feature-gated CRD | ||
| can iterate while the schema matures; room for the full multi-rack | ||
| `hostOverrides` shape, which the GA test criteria require. | ||
| - Cons: a new CRD lifecycle to own (shipped and reconciled via MCO's | ||
| payload manifests); one more object to discover, mitigated by | ||
| cross-references from the `vipManagement` field documentation. | ||
|
|
||
| The current preference is Option A: it keeps the BGP configuration on | ||
| the API object that already owns the on-prem VIP surface, reuses the | ||
| established day-2 editing and spec-to-status flow, and adds no new CRD | ||
| lifecycle. Option B is the fallback if API review concludes the | ||
| Infrastructure CR should not grow this schema (the watch fan-out of | ||
| `hostOverrides` at multi-rack scale being the main reason to make that | ||
| call). |
There was a problem hiding this comment.
@yuqi-zhang, that's what we discussed in the API PR - for Tech Preview we will move out of serialized JSON and either add fields to the Infrastructure CR or create a separate CRD
vipManagement (openshift/api#2923) is the API-level signal; session health is per-node in frr-k8s BGPSessionState/FRRNodeState. Echoing localASN/peers into Infrastructure status would restate configuration and require CNO as a second status writer.
ENABLE_BGP_TOR deploys an FRR container on the host network that accepts dynamic BGP sessions from cluster nodes on the external subnet and installs learned routes on the hypervisor. Primary consumer is BGP-based VIP management (openshift/enhancements#1982): the cluster advertises API and Ingress VIPs to this speaker and the host reaches them over the BGP paths. ASNs and the FRR image are configurable; teardown is wired into host_cleanup.
ENABLE_BGP_TOR deploys an FRR container on the host network that accepts dynamic BGP sessions from cluster nodes on the external subnet and installs learned routes on the hypervisor. Primary consumer is BGP-based VIP management (openshift/enhancements#1982): the cluster advertises API and Ingress VIPs to this speaker and the host reaches them over the BGP paths. ASNs and the FRR image are configurable; teardown is wired into host_cleanup.
fedepaol
left a comment
fedepaol
left a comment
There was a problem hiding this comment.
Nice! Sorry for the late review
| 3. Extend the OpenShift installer API (`install-config.yaml`) to accept BGP | ||
| peering configuration for API and Ingress VIPs at installation time. | ||
|
|
||
| 4. Ensure that OVN-Kubernetes operates correctly when frr-k8s runs as a static |
There was a problem hiding this comment.
please don't forget about metallb!
There was a problem hiding this comment.
ACK, will need to double check this. So that when we deploy MetalLB on a system with FRR already running as static pod, it will not try to create a duplicated deployment.
| unconditional `network` statements, which would advertise the VIPs | ||
| regardless of kube-vip's health gate in routing table 198 | ||
| (implementation experience: this steered ECMP traffic to nodes with | ||
| failed backends). Instead, the CR's `spec.raw.rawConfig` reproduces |
There was a problem hiding this comment.
please note that rawConfig is declared as unsupported and available for experimentation only - provided a transition to frrconfiguratoin
There was a problem hiding this comment.
AFAIK, the proposal https://github.com/metallb/frr-k8s/pull/470/changes covers exactly this. Or not? If that ever lands, then we will drop rawConfig.
It's fair to say that we should not GA this feature here until we solve this.
| CRs. During implementation, the VIP advertisement `FRRConfiguration` CRs | ||
| will use distinct naming conventions and labels (e.g., prefixed with | ||
| `bgp-vip-`) so that OVN-Kubernetes's reconciliation loop does not interfere | ||
| with them. |
There was a problem hiding this comment.
please take into account metallb too
There was a problem hiding this comment.
I hope the same applies to metallb and ovn-k8s -- they only touch CRs which belong to them.
| - **FRRConfiguration CR management**: When creating `FRRConfiguration` CRs for | ||
| route advertisements (as described in | ||
| [bgp-ovn-kubernetes.md](/enhancements/network/bgp-ovn-kubernetes.md)), | ||
| OVN-Kubernetes must account for the pre-existing BGP peering configuration |
There was a problem hiding this comment.
how? And also, the pre existing configuration during bootstrap is an frr.conf, so ovnk is probably unaware of that?
There was a problem hiding this comment.
ovn-k should only manage the CRs it created. It should not touch the ones belonging to someone else. That's why I am doing e.g. the bgp-vip- prefix
| outbound prefix-lists when `toAdvertise` is absent; a prefix-list | ||
| deny is a route-map no-match, so evaluation falls through to the | ||
| appended permits -- egress opens exactly for the VIP prefixes and | ||
| everything else remains implicitly denied. An upstream frr-k8s |
There was a problem hiding this comment.
I think the pr is already in, so this can be reworded
There was a problem hiding this comment.
Correct, PR has landed. That's the price of enhancements hanging for 6 months in the purgatory 🤷🏻
| - `route-advertisements-*` owned by OVN-Kubernetes | ||
| - `metallb-*` owned by MetalLB operator | ||
| frr-k8s merges all applicable CRs for the node into a single FRR | ||
| configuration. Consumers must not modify or delete CRs they do not own. |
There was a problem hiding this comment.
how does this work when a node is rebooted?
There was a problem hiding this comment.
Our static pod controller is doing the re-merge after reboot
| instance inside frr-k8s is configured to read routes from routing table | ||
| `198` (the table managed by kube-vip) and advertise them to configured BGP | ||
| peers. frr-k8s uses the `FRRConfiguration` CRD for configuration; however, | ||
| during bootstrap when the API server is not yet available, a static FRR |
There was a problem hiding this comment.
how about reading a file version of frrconfiguration and merging it with what comes from the apiserver once available? This would solve the frr.conf replacement and we'll have one single way to instruct frrk8s on how to advertise the api vip.
It will also make that configuration immutable and less prone to configuration mistakes.
There was a problem hiding this comment.
That sounds cool, so I would have a FRRConfiguration manifest as YAML on a filesystem and frr-k8s would be reading this and merging with any FRRConfiguration CR applied via kube-apiserver.
Way cleaner.
But requires a new RFE + implementation in frr-k8s. I am happy to propose that and do it before we GA this feature here. But also, given we already have one RFE in-flight, I would get one done and then move to the next one. Otherwise we will have two in-progress things hanging for years
| respective configs):** frr-k8s runs from CRD-based configuration. Each | ||
| consumer owns its own `FRRConfiguration` CRs, identified by distinct | ||
| name prefixes and `managed-by` labels: | ||
| - `bgp-vip-*` owned by CNO (VIP advertisement) |
There was a problem hiding this comment.
this is still the configuration described above right?
Incorporate the remaining implementation findings and address fedepaol's review (4853101609): - MetalLB: coexistence added as a goal, folded into the CR-ownership and garbage-collection contract, and backed by live validation (three FRRConfiguration producers on one cluster; day-2 MetalLB operator in frr-k8s-external mode merging into the same neighbor the VIP CR declares). - rawConfig: explicitly framed as an interim carrier (the frr-k8s API declares it unsupported); committed migration to the first-class redistribution API (metallb/frr-k8s#469 + design PR openshift#470). - Bootstrap/steady-state: steady state clarified as the same handover configuration; node-reboot sequencing documented; file-form FRRConfiguration source named as the pre-GA direction (one config language end to end, replacing the frr.conf handover seam). - OVN-K section rewritten: no awareness of the VIP configuration needed; the contract is additive CR ownership plus GC scoped to own CRs (applies to MetalLB symmetrically). - ip import-table dropped everywhere (implementation finding: table-direct reads the kernel table directly; import-table was unnecessary and copies routes into the main table). - kube-vip-ingress scope corrected: rendered for all nodes via MCO templates/common (keepalived parity), workers advertise via the CNO DaemonSet instance. - FRRConfiguration is a single cluster-wide bgp-vip CR (no node selector); OVN-K CR naming corrected to ovnk-generated-*. - Second zebra bug documented (FRRouting/frr#22654, fixed upstream via #22676) with the kube-vip re-assertion as optional hardening; FRR version/backport requirements consolidated. - kube-vip restart-on-settled-cluster gap added to Risks (kubeconfig points at a node IP the serving cert does not cover). - Test Plan updated with the implemented CI lanes (e2e-metal-ipi-bgp-vip and the three coexistence lanes); Implementation Experience refreshed (27 installs, merged upstream work). Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
…heck endpoint Findings from the run28 dual-stack validation: a second kube-vip instance per role manages the secondary family's VIP (single-address limitation, keepalived per-family parity), and kubernetes_addr points at the IPv4 loopback literal - the only certificate-valid local API endpoint - which kube-vip now also honors for the routing-table backend health check (closing the settled-cluster restart gap for newly started instances). Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
Rewrite the API Extensions Option A/B deliberation into the selected Option B: a v1alpha1 BGPVIPConfig CRD (machineconfiguration.openshift.io, cluster-scoped singleton), with the openshift/api conventions applied (enums not booleans, integer-second timers, no schema defaults, inline password with MetalLB precedent and a discriminated-union secretRef reserved pre-GA), Rendered and SessionsConfigured status conditions, a NodeDisruptionPolicy for the peers file, VIPs read from Infrastructure, and BGPVIPPeersJSON demoted to internal transport. Implemented in the reference implementation and validated live: a day-2 peer edit propagates without node disruption and the Dev Preview ConfigMap deleted with no effect. Amend the graduation criteria: reword the passwordSecretRef TP criterion to inline-now/union-secretRef-pre-GA and move the ConfigMap path removal from GA to Tech Preview. Record two findings from live validation: the bootstrap frr.conf needs per-neighbor disable-connected-check or IPv6 sessions fail nexthop tracking after reboot, and SessionsConfigured is currently render-level with post-apply semantics as a pre-graduation follow-up. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
Typed, admission-validated configuration API for BGP-based VIP management (enhancement openshift/enhancements#1982, OPNET-595), gated on BGPBasedVIPManagement: a cluster-scoped singleton carrying the local ASN, the default BGP peer set, optional communities and per-host peer overrides, replacing the Dev Preview bgp-vip-config ConfigMap and the serialized-JSON ControllerConfigSpec.BGPVIPPeersJSON user surface (the JSON field remains as machine-config-operator internal transport). API conventions applied: Enabled/Disabled enums instead of booleans, integer-second timer fields (BGP wire-format uint16 seconds), no schema defaults (consumers default and godoc documents omitted behavior), list-map peers/overrides, CEL validation for the singleton name, peer IPs, timer relation and community segment ranges. The peer password is deliberately inline (matching MetalLB's BGPPeer API); a secret-reference variant is reserved as a future discriminated union. Status carries observedGeneration and two conditions: Rendered (owned by machine-config-operator, set after the per-node peer configuration is applied to the ControllerConfig) and SessionsConfigured (owned by cluster-network-operator, set when the FRR session configuration has been rendered for application). Includes the integration test suite (validation matrix incl. dual-stack peers, host overrides, timer relation and community range). Consumers are implemented and were validated end to end on a live dual-stack baremetal cluster: byte-identical rendered peer configuration, day-2 peer edits propagating without node disruption (paired with a NodeDisruptionPolicy in MCO), and removal of the Dev Preview ConfigMap with no effect. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
Typed, admission-validated configuration API for BGP-based VIP management (enhancement openshift/enhancements#1982, OPNET-595), gated on BGPBasedVIPManagement: a cluster-scoped singleton carrying the local ASN, the default BGP peer set, optional communities and per-host peer overrides, replacing the Dev Preview bgp-vip-config ConfigMap and the serialized-JSON ControllerConfigSpec.BGPVIPPeersJSON user surface (the JSON field remains as machine-config-operator internal transport). API conventions applied: Enabled/Disabled enums instead of booleans, integer-second timer fields (BGP wire-format uint16 seconds), no schema defaults (consumers default and godoc documents omitted behavior), list-map peers/overrides, CEL validation for the singleton name, peer IPs, timer relation and community segment ranges. The peer password is deliberately inline (matching MetalLB's BGPPeer API); a secret-reference variant is reserved as a future discriminated union. Status carries observedGeneration and two conditions: Rendered (owned by machine-config-operator, set after the per-node peer configuration is applied to the ControllerConfig) and SessionsConfigured (owned by cluster-network-operator, set when the FRR session configuration has been rendered for application). Includes the integration test suite (validation matrix incl. dual-stack peers, host overrides, timer relation and community range). Consumers are implemented and were validated end to end on a live dual-stack baremetal cluster: byte-identical rendered peer configuration, day-2 peer edits propagating without node disruption (paired with a NodeDisruptionPolicy in MCO), and removal of the Dev Preview ConfigMap with no effect. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
Typed, admission-validated configuration API for BGP-based VIP management (enhancement openshift/enhancements#1982, OPNET-595), gated on BGPBasedVIPManagement: a cluster-scoped singleton carrying the local ASN, the default BGP peer set, optional communities and per-host peer overrides, replacing the Dev Preview bgp-vip-config ConfigMap and the serialized-JSON ControllerConfigSpec.BGPVIPPeersJSON user surface (the JSON field remains as machine-config-operator internal transport). API conventions applied: Enabled/Disabled enums instead of booleans, integer-second timer fields (BGP wire-format uint16 seconds), no schema defaults (consumers default and godoc documents omitted behavior), list-map peers/overrides, CEL validation for the singleton name, peer IPs, timer relation and community segment ranges. Peer authentication is secret-only: passwordSecret references a kubernetes.io/basic-auth Secret in the openshift-config namespace ('password' key, 80-byte TCP MD5 limit) - passwords are never stored in this API; the shape mirrors frr-k8s's FRRConfiguration neighbor passwordSecret, which the cluster-network-operator maps it onto. Status carries observedGeneration and two conditions: Rendered (owned by machine-config-operator, set after the per-node peer configuration is applied to the ControllerConfig) and SessionsConfigured (owned by cluster-network-operator, set when the FRR session configuration has been rendered for application). Includes the integration test suite (validation matrix incl. dual-stack peers, host overrides, timer relation, community range and passwordSecret name validation). The consumers (installer, MCO, CNO) are implemented against the inline-password revision and were validated end to end on a live dual-stack baremetal cluster; their passwordSecret rework follows this API. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
The structured BGPVIPConfig API carries no inline password: peer authentication is a name-only reference to a kubernetes.io/basic-auth Secret in openshift-config, mirroring the frr-k8s FRRConfiguration neighbor passwordSecret it is mapped onto. This satisfies the original passwordSecretRef Tech Preview criterion directly (previous revision deferred it as a pre-GA discriminated union). Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
|
@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. |
Typed, admission-validated configuration API for BGP-based VIP management (enhancement openshift/enhancements#1982, OPNET-595), gated on BGPBasedVIPManagement: a cluster-scoped singleton carrying the local ASN, the default BGP peer set, optional communities and per-host peer overrides, replacing the Dev Preview bgp-vip-config ConfigMap and the serialized-JSON ControllerConfigSpec.BGPVIPPeersJSON user surface (the JSON field remains as machine-config-operator internal transport). API conventions applied: Enabled/Disabled enums instead of booleans, integer-second timer fields (BGP wire-format uint16 seconds), no schema defaults (consumers default and godoc documents omitted behavior), list-map peers/overrides, CEL validation for the singleton name, peer IPs, timer relation and community segment ranges. Peer authentication is secret-only: passwordSecret references a kubernetes.io/basic-auth Secret in the openshift-config namespace ('password' key, 80-byte TCP MD5 limit) - passwords are never stored in this API; the shape mirrors frr-k8s's FRRConfiguration neighbor passwordSecret, which the cluster-network-operator maps it onto. Status carries observedGeneration and two conditions: Rendered (owned by machine-config-operator, set after the per-node peer configuration is applied to the ControllerConfig) and SessionsConfigured (owned by cluster-network-operator, set when the FRR session configuration has been rendered for application). Includes the integration test suite (validation matrix incl. dual-stack peers, host overrides, timer relation, community range and passwordSecret name validation). The consumers (installer, MCO, CNO) are implemented against the inline-password revision and were validated end to end on a live dual-stack baremetal cluster; their passwordSecret rework follows this API. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
5 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