-
Notifications
You must be signed in to change notification settings - Fork 231
USHIFT-7432: Fix OVN multinode networking regression from removal of --sb-address flag #7364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
9393c44
1a3e135
c8eef6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,17 +110,24 @@ func startCNIPlugin(ctx context.Context, cfg *config.Config, kubeconfigPath stri | |
| return err | ||
| } | ||
|
|
||
| // Multinode only params: OVN_NB_PORT, OVN_SB_PORT | ||
| // Multinode only params: OVN_NB_PORT, OVN_SB_PORT, MultiNodeEnabled | ||
| extraParams := assets.RenderParams{ | ||
| "OVNConfig": ovnConfig, | ||
| "KubeconfigPath": kubeconfigPath, | ||
| "KubeconfigDir": filepath.Join(config.DataDir, "/resources/kubeadmin"), | ||
| "OVN_NB_PORT": ovn.OVN_NB_PORT, | ||
| "OVN_SB_PORT": ovn.OVN_SB_PORT, | ||
| } | ||
| if err := assets.ApplyConfigMaps(ctx, cm, renderTemplate, renderParamsFromConfig(cfg, extraParams), kubeconfigPath); err != nil { | ||
| klog.Warningf("Failed to apply configMap %v %v", cm, err) | ||
| return err | ||
| "OVNConfig": ovnConfig, | ||
| "KubeconfigPath": kubeconfigPath, | ||
| "KubeconfigDir": filepath.Join(config.DataDir, "/resources/kubeadmin"), | ||
| "OVN_NB_PORT": ovn.OVN_NB_PORT, | ||
| "OVN_SB_PORT": ovn.OVN_SB_PORT, | ||
|
Comment on lines
+118
to
+119
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check whether any TLS/pssl configuration exists for OVN DB remotes, and how ports are firewalled.
rg -nP -C3 'pssl|ptcp|ssl:|OVN_NB_PORT|OVN_SB_PORT' --glob '!deps/**'
rg -nP -C3 '9641|9642' --glob '!deps/**' -g '!vendor/**'Repository: openshift/microshift Length of output: 17255 Security Misconfiguration Reachability: External Protect the OVN database remotes. In multinode mode, the configmap publishes Use authenticated TLS remotes ( 🤖 Prompt for AI Agents |
||
| "MultiNodeEnabled": cfg.MultiNode.Enabled, | ||
| } | ||
| // In multinode mode the configmap contains [OvnNorth]/[OvnSouth] stanzas | ||
| // with the primary's IP. Only the primary may write it; a worker applying | ||
| // the configmap would overwrite the primary IP with its own, breaking SBDB | ||
| // connectivity for every node that reads the configmap afterwards. | ||
| if !cfg.MultiNode.Enabled || !cfg.BootstrapKubeConfigExists() { | ||
| if err := assets.ApplyConfigMaps(ctx, cm, renderTemplate, renderParamsFromConfig(cfg, extraParams), kubeconfigPath); err != nil { | ||
| klog.Warningf("Failed to apply configMap %v %v", cm, err) | ||
| return err | ||
| } | ||
| } | ||
| if err := assets.ApplyDaemonSets(ctx, apps, renderTemplate, renderParamsFromConfig(cfg, extraParams), kubeconfigPath); err != nil { | ||
| klog.Warningf("Failed to apply apps %v %v", apps, err) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,6 +89,9 @@ func (s *KubeletServer) configure(cfg *config.Config) { | |
| kubeletFlags.NodeLabels["node-role.kubernetes.io/worker"] = "" | ||
| kubeletFlags.NodeLabels["node.openshift.io/os_id"] = osID | ||
| kubeletFlags.NodeLabels["node.kubernetes.io/instance-type"] = "rhde" | ||
| if !cfg.BootstrapKubeConfigExists() { | ||
| kubeletFlags.NodeLabels["node.microshift.io/role"] = "primary" | ||
| } | ||
|
Comment on lines
+92
to
+94
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
# Look for any code that patches/ensures node labels outside kubelet flags.
rg -nP --type=go -C4 'node\.microshift\.io/role|NodeLabels\['
rg -nP --type=go -C4 'Nodes\(\)\.(Patch|Update)\(' pkg/Repository: openshift/microshift Length of output: 4193 🤖 get_repo_knowledge executed:
Length of output: 649 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- pkg/node/kubelet.go ---'
sed -n '1,180p' pkg/node/kubelet.go
printf '%s\n' '--- role label and selector references ---'
rg -n -C5 'node\.microshift\.io/role|ovnkube-master|BootstrapKubeConfigExists' --glob '!vendor/**' --glob '!deps/**' .
printf '%s\n' '--- node patch/update callers ---'
rg -n -C4 'CoreV1\(\)\.Nodes\(\)|Nodes\(\)\.(Patch|Update)' pkg --glob '*.go'Repository: openshift/microshift Length of output: 50378 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- kubelet label application path ---'
rg -n -C6 'NodeLabels|registerWithApiserver|registration.*node|RegisterWithApiserver' vendor/k8s.io/kubernetes/pkg/kubelet vendor/k8s.io/kubernetes/cmd/kubelet --glob '*.go' | head -220
printf '%s\n' '--- exact OVN selector ---'
sed -n '462,480p' assets/components/ovn/multi-node/master/daemonset.yaml
printf '%s\n' '--- existing node reconciliation implementation ---'
sed -n '1,190p' pkg/controllers/c2cc/annotation.goRepository: openshift/microshift Length of output: 17180 🏁 Script executed: #!/bin/bash
set -eu
rg -n 'func .*register|registerWithApiserver|tryRegisterWithApiserver|registrationCompleted|NodeLabels' vendor/k8s.io/kubernetes/pkg/kubelet vendor/k8s.io/kubernetes/cmd/kubelet --glob '*.go' | head -120Repository: openshift/microshift Length of output: 5215 🏁 Script executed: #!/bin/bash
set -eu
sed -n '1,125p' vendor/k8s.io/kubernetes/pkg/kubelet/kubelet_node_status.goRepository: openshift/microshift Length of output: 4244 🏁 Script executed: #!/bin/bash
set -eu
sed -n '120,235p' vendor/k8s.io/kubernetes/pkg/kubelet/kubelet_node_status.goRepository: openshift/microshift Length of output: 4970 🏁 Script executed: #!/bin/bash
set -eu
rg -n -C8 'func \(kl \*Kubelet\) updateDefaultLabels|updateDefaultLabels\(' vendor/k8s.io/kubernetes/pkg/kubelet --glob '*.go'Repository: openshift/microshift Length of output: 3793 🏁 Script executed: #!/bin/bash
set -eu
sed -n '276,325p' vendor/k8s.io/kubernetes/pkg/kubelet/kubelet_node_status.goRepository: openshift/microshift Length of output: 1763 Reconcile the primary label on existing Nodes.
🤖 Prompt for AI Agents |
||
|
|
||
| kubeletConfig, err := loadConfigFile(filepath.Join(config.DataDir, "/resources/kubelet/config/config.yaml")) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🌐 Web query:
ovn-kubernetes ovnkube.conf config file OvnNorth OvnSouth section address field format💡 Result:
In the ovn-kubernetes configuration file (ovnkube.conf), the address field within the [ovnnorth] and [ovnsouth] sections uses a format that specifies the connection protocol followed by the IP address and port [1][2]. The standard format is: address=:<ip_address>: Common examples include: - For SSL/TLS connections: address=ssl:1.2.3.4:6641 [1][2] - For plain TCP connections: address=tcp:1.2.3.4:6641 [3] In these sections, the address defines the connection point for the OVN Northbound and Southbound database APIs, respectively [1][2]. When SSL is used, additional fields in the same section are required to specify client and server private keys, certificates, and CA certificates [1][2].
Citations:
🤖 get_repo_knowledge executed:
get_repo_knowledge openshift/microshift /tmp/coderabbit-repo-knowledge/openshift-microshift-dfe3d15bLength of output: 658
🏁 Script executed:
Repository: openshift/microshift
Length of output: 21019
🏁 Script executed:
Repository: openshift/microshift
Length of output: 50376
🏁 Script executed:
Repository: openshift/microshift
Length of output: 21476
🌐 Web query:
site:github.com/openshift/ovn-kubernetes ovnkube config parser ovnnorth ovnsouth OvnNorth OvnSouth config.go💡 Result:
In the ovn-kubernetes project, the configuration for OVN Northbound and Southbound databases is handled primarily within the go-controller/pkg/config/config.go file [1][2][3]. The structures OvnNorth and OvnSouth define the parameters required to connect to these databases, including database locations, run directories, and SSL/TLS authentication details [1]. Key configuration details include: Structure Definitions: Both OvnNorth and OvnSouth are instances of the OvnAuthConfig struct [1]. This struct contains fields such as Address, ClientPrivKey, ClientCert, ClientCACert, ServerPrivKey, ServerCert, ServerCACert, RunDir, and DbLocation [1][4]. Configuration Sources: - Configuration File: Options are parsed from a configuration file (defaulting to /etc/openvswitch/ovn_k8s.conf) under the [ovnnorth] and [ovnsouth] sections [4][5][6]. - Command-Line Arguments: The project allows command-line arguments to override settings defined in the configuration file [4][5]. Usage: - The [ovnnorth] and [ovnsouth] sections specify the database connection addresses and, if SSL is used, the necessary certificate paths [4]. - The DbLocation field specifies the path to the database files, while RunDir specifies the directory for OVN runtime files [1]. These structures and parsing logic allow the ovnkube executable to properly initialize connections to the OVN databases, which is essential for managing OVN resources within the Kubernetes cluster [4][7].
Citations:
🌐 Web query:
github openshift ovn-kubernetes go-controller/pkg/config/config.go config.ReadConfig ovnnorth section case sensitive💡 Result:
In the ovn-kubernetes configuration, the section header [ovnnorth] (and [ovnsouth]) is case-sensitive when defined in the configuration file [1][2][3][4]. The ovn-kubernetes project uses the gcfg library to parse these configuration files [5][6], which adheres to standard INI-style section naming conventions where identifiers are typically case-sensitive. In the official configuration documentation and example files, the section is consistently represented in lowercase as [ovnnorth] [1][7][8][4]. Using incorrect casing, such as [OvnNorth], will result in the configuration parser failing to recognize the section, causing the options contained within it (such as address, client-cert, etc.) to be ignored or remain unset, as they will not be correctly mapped to the OvnNorth struct defined in go-controller/pkg/config/config.go [5][9]. If you are encountering issues where settings in the [ovnnorth] section are not taking effect, verify that your configuration file uses the exact lowercase [ovnnorth] header [1][4].
Citations:
Rename the OVN database sections and remove the relay workaround.
ovn-kubernetesrequires case-sensitive[ovnnorth]and[ovnsouth]sections. The current mixed-case names leave theaddress=values unset. Rename both sections, update the matchingawkselectors in the node DaemonSet, and letovnkubeuse the TCP addresses directly.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 39-39: syntax error: could not find expected ':'
(syntax)
🤖 Prompt for AI Agents