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

[zhangzujian]

Pull Request

• Make sure you have followed Kube-OVN Code Style.

What type of this PR

Examples of user facing changes:

• Features
• Bug fixes
• Docs
• Tests

Which issue(s) this PR fixes

Fixes #(issue-number)

[gemini-code-assist]

Summary of Changes

Hello @zhangzujian, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the internal mechanism for executing ovs-appctl and ovn-appctl commands. The primary goal is to eliminate the redundant and error-prone practice of reading PID files to determine the correct control socket for these utilities. By introducing new helper functions, the codebase now provides a more consistent and simplified API for interacting with OVS/OVN components, enhancing maintainability and reducing potential issues related to file system operations.

Highlights

• Refactored OVS/OVN appctl command execution: The logic for invoking ovs-appctl and ovn-appctl commands has been centralized and simplified, removing the need to manually read PID files to construct control socket paths.
• Introduced OvnDatabaseControl helper: A new utility function OvnDatabaseControl was added to pkg/ovs/ovs-appctl_linux.go to streamline interactions with OVN Northbound and Southbound database control sockets.
• Simplified OVN status and database operations: Existing code in pkg/ovn_leader_checker/ovn.go and pkg/ovnmonitor/util.go has been updated to leverage the new ovs.Appctl and ovs.OvnDatabaseControl functions, reducing boilerplate and improving clarity.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request refactors the way ovs-appctl and ovn-appctl commands are executed by introducing helper functions, removing the need to read PID files. This is a good simplification of the codebase. However, I've identified a critical compilation error in the new code, along with a couple of medium-severity issues related to code clarity and robustness. Please review the detailed comments.

[gemini-code-assist]

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.

[zhangzujian]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors how ovs/ovn appctl commands are executed by removing the need to read PID files. This is an excellent improvement that increases robustness and simplifies the code. The introduction of helper functions like ovs.OvnDatabaseControl also enhances code clarity at the call sites. I've identified one high-severity issue in the new logic for determining which appctl command to use, where any absolute path would incorrectly default to ovn-appctl. A code suggestion to rectify this logic has been provided.

[gemini-code-assist]

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