Skip to content

refactor: do not read pid file when calling ovs/ovn appctl command #6230

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.

Sign up for GitHub

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

Merged
zhangzujian merged 1 commit into from
Jan 30, 2026
Merged

refactor: do not read pid file when calling ovs/ovn appctl command #6230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 8 additions & 24 deletions pkg/ovn_leader_checker/ovn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,20 +219,9 @@ func isDBLeader(address, database string) bool {
}

func checkNorthdActive() bool {
pid, err := os.ReadFile(OvnNorthdPid)
output, err := ovs.Appctl("ovn-northd", "status")
if err != nil {
klog.Errorf("failed to read file %q: %v", OvnNorthdPid, err)
return false
}

command := []string{
"-t",
fmt.Sprintf("/var/run/ovn/ovn-northd.%s.ctl", strings.TrimSpace(string(pid))),
"status",
}
output, err := exec.Command("ovn-appctl", command...).CombinedOutput() // #nosec G204
if err != nil {
klog.Errorf("checkNorthdActive execute err %v error msg %v", err, string(output))
klog.Errorf("checkNorthdActive execute err %v error msg %v", err, output)
return false
}

Expand All @@ -241,8 +230,8 @@ func checkNorthdActive() bool {
return false
}

klog.V(5).Infof("checkNorthdActive: output %s", string(output))
result := strings.TrimSpace(string(output))
klog.V(5).Infof("checkNorthdActive: output %s", output)
result := strings.TrimSpace(output)
return strings.Contains(result, "active")
}

Expand Down Expand Up @@ -317,21 +306,16 @@ func checkNorthdEpAlive(cfg *Configuration, namespace, service string) bool {
}

func compactOvnDatabase(db string) {
args := []string{
"-t",
fmt.Sprintf("/var/run/ovn/ovn%s_db.ctl", db),
"ovsdb-server/compact",
}
output, err := exec.Command("ovn-appctl", args...).CombinedOutput() // #nosec G204
output, err := ovs.OvnDatabaseControl(db, "ovsdb-server/compact")
if err != nil {
if !strings.Contains(string(output), "not storing a duplicate snapshot") {
klog.Errorf("failed to compact ovn%s database: %s", db, string(output))
if !strings.Contains(output, "not storing a duplicate snapshot") {
klog.Errorf("failed to compact ovn%s database: %s", db, output)
}
return
}

if len(output) != 0 {
klog.V(5).Infof("compact ovn%s database: %s", db, string(output))
klog.V(5).Infof("compact ovn%s database: %s", db, output)
}
}

Expand Down
9 changes: 3 additions & 6 deletions pkg/ovnmonitor/exporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,12 +261,9 @@ func (e *Exporter) exportOvnClusterEnableGauge() {

func (e *Exporter) exportOvnClusterInfoGauge() {
resetOvnClusterMetrics()
dirDbMap := map[string]string{
"nb": ovnnb.DatabaseName,
"sb": ovnsb.DatabaseName,
}
for direction, database := range dirDbMap {
clusterStatus, err := getClusterInfo(direction, database)
dbList := []string{ovnnb.DatabaseName, ovnsb.DatabaseName}
for _, database := range dbList {
clusterStatus, err := getClusterInfo(database)
if err != nil {
klog.Errorf("Failed to get Cluster Info for database %s: %v", database, err)
return
Expand Down
87 changes: 29 additions & 58 deletions pkg/ovnmonitor/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package ovnmonitor

import (
"fmt"
"os"
"os/exec"
"strconv"
"strings"
Expand All @@ -11,6 +10,7 @@ import (
"github.com/kubeovn/ovsdb"
"k8s.io/klog/v2"

"github.com/kubeovn/kube-ovn/pkg/ovs"
"github.com/kubeovn/kube-ovn/pkg/ovsdb/ovnnb"
"github.com/kubeovn/kube-ovn/pkg/ovsdb/ovnsb"
)
Expand All @@ -26,46 +26,34 @@ func (e *Exporter) getOvnStatus() map[string]int {
result := make(map[string]int)

// get ovn-northbound status
cmd := exec.Command("ovn-appctl", "-t", "/var/run/ovn/ovnnb_db.ctl", "cluster/status", ovnnb.DatabaseName) // #nosec G204
output, err := cmd.CombinedOutput()
output, err := ovs.OvnDatabaseControl(ovnnb.DatabaseName, "cluster/status", ovnnb.DatabaseName)
if err != nil {
klog.Errorf("get ovn-northbound status failed, err %v", err)
result["ovsdb-server-northbound"] = 0
}
result["ovsdb-server-northbound"] = parseDbStatus(string(output))
result["ovsdb-server-northbound"] = parseDbStatus(output)

// get ovn-southbound status
cmd = exec.Command("ovn-appctl", "-t", "/var/run/ovn/ovnsb_db.ctl", "cluster/status", ovnsb.DatabaseName) // #nosec G204
output, err = cmd.CombinedOutput()
output, err = ovs.OvnDatabaseControl(ovnsb.DatabaseName, "cluster/status", ovnsb.DatabaseName)
if err != nil {
klog.Errorf("get ovn-southbound status failed, err %v", err)
result["ovsdb-server-southbound"] = 0
}
result["ovsdb-server-southbound"] = parseDbStatus(string(output))
result["ovsdb-server-southbound"] = parseDbStatus(output)

// get ovn-northd status
pid, err := os.ReadFile(e.Client.Service.Northd.File.Pid.Path)
if err != nil {
klog.Errorf("read ovn-northd pid failed, err %v", err)
if output, err = ovs.Appctl("ovn-northd", "status"); err != nil {
klog.Errorf("get ovn-northd status failed, err %v", err)
result["ovn-northd"] = 0
} else if len(strings.Split(output, ":")) != 2 {
result["ovn-northd"] = 0
} else {
// #nosec G204
cmd := exec.Command("ovn-appctl", "-t", fmt.Sprintf("/var/run/ovn/ovn-northd.%s.ctl", strings.Trim(string(pid), "\n")), "status")
output, err := cmd.CombinedOutput()
if err != nil {
klog.Errorf("get ovn-northd status failed, err %v", err)
result["ovn-northd"] = 0
}
if len(strings.Split(string(output), ":")) != 2 {
result["ovn-northd"] = 0
} else {
status := strings.TrimSpace(strings.Split(string(output), ":")[1])
switch status {
case "standby":
result["ovn-northd"] = 2
case "active":
result["ovn-northd"] = 1
}
status := strings.TrimSpace(strings.Split(output, ":")[1])
switch status {
case "standby":
result["ovn-northd"] = 2
case "active":
result["ovn-northd"] = 1
}
}

Expand All @@ -76,24 +64,22 @@ func (e *Exporter) getOvnStatusContent() map[string]string {
result := map[string]string{"ovsdb-server-northbound": "", "ovsdb-server-southbound": ""}

// get ovn-northbound status
cmd := exec.Command("ovn-appctl", "-t", "/var/run/ovn/ovnnb_db.ctl", "cluster/status", ovnnb.DatabaseName) // #nosec G204
output, err := cmd.CombinedOutput()
output, err := ovs.OvnDatabaseControl(ovnnb.DatabaseName, "cluster/status", ovnnb.DatabaseName)
if err != nil {
klog.Errorf("get ovn-northbound status failed, err %v", err)
}
if strings.Contains(string(output), "Servers:") {
servers := strings.Split(string(output), "Servers:")[1]
if strings.Contains(output, "Servers:") {
servers := strings.Split(output, "Servers:")[1]
result["ovsdb-server-northbound"] = servers
}

// get ovn-southbound status
cmd = exec.Command("ovn-appctl", "-t", "/var/run/ovn/ovnsb_db.ctl", "cluster/status", ovnsb.DatabaseName) // #nosec G204
output, err = cmd.CombinedOutput()
output, err = ovs.OvnDatabaseControl(ovnsb.DatabaseName, "cluster/status", ovnsb.DatabaseName)
if err != nil {
klog.Errorf("get ovn-southbound status failed, err %v", err)
}
if strings.Contains(string(output), "Servers:") {
servers := strings.Split(string(output), "Servers:")[1]
if strings.Contains(output, "Servers:") {
servers := strings.Split(output, "Servers:")[1]
result["ovsdb-server-southbound"] = servers
}

Expand Down Expand Up @@ -174,15 +160,14 @@ func (e *Exporter) setLogicalSwitchPortInfoMetric() {
}
}

func getClusterInfo(direction, dbName string) (*OVNDBClusterStatus, error) {
cmd := exec.Command("ovn-appctl", "-t", fmt.Sprintf("/var/run/ovn/ovn%s_db.ctl", direction), "cluster/status", dbName) // #nosec G204
output, err := cmd.CombinedOutput()
func getClusterInfo(dbName string) (*OVNDBClusterStatus, error) {
output, err := ovs.OvnDatabaseControl(dbName, "cluster/status", dbName)
if err != nil {
return nil, fmt.Errorf("failed to retrieve cluster/status info for database %s: %w", dbName, err)
}

clusterStatus := &OVNDBClusterStatus{}
for line := range strings.SplitSeq(string(output), "\n") {
for line := range strings.SplitSeq(output, "\n") {
idx := strings.Index(line, ":")
if idx == -1 {
continue
Expand Down Expand Up @@ -305,35 +290,21 @@ func parseDbStatus(output string) int {
}

func getDBStatus(dbName string) (bool, error) {
var cmd *exec.Cmd
var result bool
switch dbName {
case ovnnb.DatabaseName:
cmd = exec.Command("ovn-appctl", "-t", "/var/run/ovn/ovnnb_db.ctl", "ovsdb-server/get-db-storage-status", dbName) // #nosec G204
case ovnsb.DatabaseName:
cmd = exec.Command("ovn-appctl", "-t", "/var/run/ovn/ovnsb_db.ctl", "ovsdb-server/get-db-storage-status", dbName) // #nosec G204
default:
return false, fmt.Errorf("unknown db name %s", dbName)
}

output, err := cmd.CombinedOutput()
output, err := ovs.OvnDatabaseControl(dbName, "ovsdb-server/get-db-storage-status", dbName)
if err != nil {
klog.Errorf("get %s status failed, err %v", dbName, err)
return false, err
}
lines := strings.SplitSeq(string(output), "\n")
for line := range lines {
for line := range strings.SplitSeq(output, "\n") {
if strings.Contains(line, "status: ok") {
result = true
break
return true, nil
}
if strings.Contains(line, "ovsdb error") {
result = false
break
return false, nil
}
}

return result, nil
return false, nil
}

func resetLogicalSwitchMetrics() {
Expand Down
59 changes: 34 additions & 25 deletions pkg/ovs/ovs-appctl_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,49 +4,58 @@ import (
"fmt"
"os"
"os/exec"
"path/filepath"
"slices"
"strings"

"github.com/kubeovn/kube-ovn/pkg/ovsdb/ovnnb"
"github.com/kubeovn/kube-ovn/pkg/ovsdb/ovnsb"
)

const (
ovsRunDir = "/var/run/openvswitch"
ovnRunDir = "/var/run/ovn"

cmdOvsAppctl = "ovs-appctl"
cmdOvnAppctl = "ovn-appctl"

ovnNBCtlSocket = "/var/run/ovn/ovnnb_db.ctl"
ovnSBCtlSocket = "/var/run/ovn/ovnsb_db.ctl"
)

func appctlByTarget(target string, args ...string) (string, error) {
args = slices.Insert(args, 0, "-t", target)
cmd := exec.Command(cmdOvsAppctl, args...)
func appctlByTarget(appctlCmd, target, command string, args ...string) (string, error) {
args = slices.Insert(args, 0, "-t", target, command)
cmd := exec.Command(appctlCmd, args...)
output, err := cmd.CombinedOutput()
if err != nil {
return "", fmt.Errorf("failed to run command %q: %w", cmd.String(), err)
}
return string(output), nil
}

func Appctl(component string, args ...string) (string, error) {
var runDir string
switch {
case strings.HasPrefix(component, "ovs"):
runDir = ovsRunDir
case strings.HasPrefix(component, "ovn"):
runDir = ovnRunDir
// OvnDatabaseControl sends a command to the specified OVN database control socket
// and returns the output or an error if the command fails.
func OvnDatabaseControl(db, command string, args ...string) (string, error) {
var socket string
switch db {
case "nb", ovnnb.DatabaseName:
socket = ovnNBCtlSocket
case "sb", ovnsb.DatabaseName:
socket = ovnSBCtlSocket
default:
return "", fmt.Errorf("unknown component %q", component)
return "", fmt.Errorf("unknown db %q", db)
}
return Appctl(socket, command, args...)
}

pidFile := filepath.Join(runDir, component+".pid")
pidBytes, err := os.ReadFile(pidFile)
if err != nil {
return "", fmt.Errorf("failed to read pid file %q: %w", pidFile, err)
}
pidFields := strings.Fields(string(pidBytes))
if len(pidFields) == 0 {
return "", fmt.Errorf("pid file %q is empty or contains only whitespace", pidFile)
func Appctl(target, command string, args ...string) (string, error) {
var cmd string
switch {
case strings.IndexRune(target, os.PathSeparator) == 0:
fallthrough
case strings.HasPrefix(target, "ovn"):
cmd = cmdOvnAppctl
case strings.HasPrefix(target, "ovs"):
cmd = cmdOvsAppctl
default:
return "", fmt.Errorf("unknown target %q", target)
}
Comment on lines +49 to 58
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of fallthrough here makes the logic for choosing between ovn-appctl and ovs-appctl a bit subtle. It effectively defaults to using ovn-appctl for any absolute path. While this may work since ovn-appctl is a wrapper for ovs-appctl, it would be more robust and clearer to explicitly handle path-based targets. For example, you could check if the path contains 'ovn' or 'openvswitch' to select the appropriate tool. This would make the code more resilient, especially if it's ever used with OVS sockets in an environment where ovn-appctl might not be available.

Comment on lines +49 to 58
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The logic to determine whether to use ovs-appctl or ovn-appctl is flawed due to the use of fallthrough. Currently, any absolute path provided as a target will cause ovn-appctl to be used, which is incorrect for OVS control sockets (e.g., /var/run/openvswitch/ovs-vswitchd.ctl). This can lead to command failures for valid OVS targets specified with an absolute path. I suggest restructuring the logic to be more explicit and correct.

switch {
	case strings.HasPrefix(target, "ovs"):
		cmd = cmdOvsAppctl
	case strings.HasPrefix(target, "ovn"):
		cmd = cmdOvnAppctl
	case strings.IndexRune(target, os.PathSeparator) == 0:
		if strings.Contains(target, "openvswitch") {
			cmd = cmdOvsAppctl
		} else {
			cmd = cmdOvnAppctl
		}
	default:
		return "", fmt.Errorf("unknown target %q", target)
	}

target := filepath.Join(runDir, fmt.Sprintf("%s.%s.ctl", component, pidFields[0]))

return appctlByTarget(target, args...)
return appctlByTarget(cmd, target, command, args...)
}
Loading