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

Signed-off-by: zhangzujian <zhangzujian.7@gmail.com>

Author: zhangzujian

pkg/ovn_leader_checker/ovn.go

@@ -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
 	}
 
@@ -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")
 }
 
@@ -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)
 	}
 }
 

pkg/ovnmonitor/exporter.go

@@ -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

pkg/ovnmonitor/util.go

@@ -2,7 +2,6 @@ package ovnmonitor
 
 import (
 	"fmt"
-	"os"
 	"os/exec"
 	"strconv"
 	"strings"
@@ -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"
 )
@@ -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
 		}
 	}
 
@@ -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
 	}
 
@@ -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
@@ -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() {

pkg/ovs/ovs-appctl_linux.go

@@ -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)
 	}
-	target := filepath.Join(runDir, fmt.Sprintf("%s.%s.ctl", component, pidFields[0]))
 
-	return appctlByTarget(target, args...)
+	return appctlByTarget(cmd, target, command, args...)
 }