diff --git a/pkg/agent/cniserver/sriov.go b/pkg/agent/cniserver/sriov.go index 1a37dfc8b43..905a99b318b 100644 --- a/pkg/agent/cniserver/sriov.go +++ b/pkg/agent/cniserver/sriov.go @@ -63,9 +63,6 @@ func GetPodContainerDeviceIDs(podName string, podNamespace string) ([]string, er defer conn.Close() client := podresourcesv1alpha1.NewPodResourcesListerClient(conn) - if client == nil { - return []string{}, fmt.Errorf("error getting the lister client for Pod resources") - } podResources, err := client.List(ctx, &podresourcesv1alpha1.ListPodResourcesRequest{}) if err != nil { diff --git a/pkg/ovs/ovsctl/appctl.go b/pkg/ovs/ovsctl/appctl.go index 37435433719..3b0232ee03d 100644 --- a/pkg/ovs/ovsctl/appctl.go +++ b/pkg/ovs/ovsctl/appctl.go @@ -15,28 +15,31 @@ package ovsctl import ( + "context" "fmt" - "strings" + "os/exec" ) type ovsAppctlRunner struct { bridge string } -func (r *ovsAppctlRunner) RunAppctlCmd(cmd string, needsBridge bool, args ...string) ([]byte, *ExecError) { +func (r *ovsAppctlRunner) RunAppctlCmd(cmd string, needsBridge bool, args ...string) ([]byte, error) { // Use the control UNIX domain socket to connect to ovs-vswitchd, as Agent can // run in a different PID namespace from ovs-vswitchd. Relying on ovs-appctl to // determine the control socket based on the pidfile will then give a "stale // pidfile" error, as it tries to validate that the PID read from the pidfile // corresponds to a valid process in the current PID namespace. - var cmdStr string + uds, err := ovsVSwitchdUDS(context.TODO()) + if err != nil { + return nil, fmt.Errorf("failed to get UDS for OVS: %w", err) + } + cmdArgs := []string{"-t", uds, cmd} if needsBridge { - cmdStr = fmt.Sprintf("ovs-appctl -t %s %s %s", ovsVSwitchdUDS(), cmd, r.bridge) - } else { - cmdStr = fmt.Sprintf("ovs-appctl -t %s %s", ovsVSwitchdUDS(), cmd) + cmdArgs = append(cmdArgs, r.bridge) } - cmdStr = cmdStr + " " + strings.Join(args, " ") - out, err := getOVSCommand(cmdStr).CombinedOutput() + ovsCmd := exec.CommandContext(context.TODO(), "ovs-appctl", cmdArgs...) + out, err := ovsCmd.CombinedOutput() if err != nil { return nil, newExecError(err, string(out)) } diff --git a/pkg/ovs/ovsctl/interface.go b/pkg/ovs/ovsctl/interface.go index e022697f8d8..e39b2d20a9c 100644 --- a/pkg/ovs/ovsctl/interface.go +++ b/pkg/ovs/ovsctl/interface.go @@ -19,7 +19,7 @@ import ( ) type OVSAppctlRunner interface { - RunAppctlCmd(cmd string, needsBridge bool, args ...string) ([]byte, *ExecError) + RunAppctlCmd(cmd string, needsBridge bool, args ...string) ([]byte, error) } type OVSOfctlRunner interface { diff --git a/pkg/ovs/ovsctl/mock_ovsctl_test.go b/pkg/ovs/ovsctl/mock_ovsctl_test.go index b000ec116a8..929132973d7 100644 --- a/pkg/ovs/ovsctl/mock_ovsctl_test.go +++ b/pkg/ovs/ovsctl/mock_ovsctl_test.go @@ -91,7 +91,7 @@ func (m *MockOVSAppctlRunner) EXPECT() *MockOVSAppctlRunnerMockRecorder { } // RunAppctlCmd mocks base method -func (m *MockOVSAppctlRunner) RunAppctlCmd(arg0 string, arg1 bool, arg2 ...string) ([]byte, *ExecError) { +func (m *MockOVSAppctlRunner) RunAppctlCmd(arg0 string, arg1 bool, arg2 ...string) ([]byte, error) { m.ctrl.T.Helper() varargs := []interface{}{arg0, arg1} for _, a := range arg2 { @@ -99,7 +99,7 @@ func (m *MockOVSAppctlRunner) RunAppctlCmd(arg0 string, arg1 bool, arg2 ...strin } ret := m.ctrl.Call(m, "RunAppctlCmd", varargs...) ret0, _ := ret[0].([]byte) - ret1, _ := ret[1].(*ExecError) + ret1, _ := ret[1].(error) return ret0, ret1 } diff --git a/pkg/ovs/ovsctl/ofctl.go b/pkg/ovs/ovsctl/ofctl.go index eec207836dd..0d9b8240e70 100644 --- a/pkg/ovs/ovsctl/ofctl.go +++ b/pkg/ovs/ovsctl/ofctl.go @@ -15,8 +15,8 @@ package ovsctl import ( - "fmt" - "strings" + "context" + "os/exec" ) type ovsOfctlRunner struct { @@ -24,18 +24,14 @@ type ovsOfctlRunner struct { } func (r *ovsOfctlRunner) RunOfctlCmd(cmd string, args ...string) ([]byte, error) { - return runOfctlCmd(true, cmd, r.bridge, args...) + return runOfctlCmd(context.TODO(), true, cmd, r.bridge, args...) } -func runOfctlCmd(openflow15 bool, cmd string, bridge string, args ...string) ([]byte, error) { - cmdStr := fmt.Sprintf("ovs-ofctl %s %s", cmd, bridge) - cmdStr = cmdStr + " " + strings.Join(args, " ") +func runOfctlCmd(ctx context.Context, openflow15 bool, cmd string, bridge string, args ...string) ([]byte, error) { + cmdArgs := append([]string{cmd, bridge}, args...) if openflow15 { - cmdStr += " -O Openflow15" + cmdArgs = append(cmdArgs, "-O", "Openflow15") } - out, err := getOVSCommand(cmdStr).Output() - if err != nil { - return nil, err - } - return out, nil + ovsCmd := exec.CommandContext(ctx, "ovs-ofctl", cmdArgs...) + return ovsCmd.Output() } diff --git a/pkg/ovs/ovsctl/ovsctl.go b/pkg/ovs/ovsctl/ovsctl.go index 52df32e8a13..247cc8e6a15 100644 --- a/pkg/ovs/ovsctl/ovsctl.go +++ b/pkg/ovs/ovsctl/ovsctl.go @@ -17,6 +17,7 @@ package ovsctl import ( "bufio" "bytes" + "context" "fmt" "net" "strconv" @@ -181,7 +182,7 @@ func (c *ovsCtlClient) runTracing(flow string) (string, error) { return string(out), nil } -func (c *ovsCtlClient) RunAppctlCmd(cmd string, needsBridge bool, args ...string) ([]byte, *ExecError) { +func (c *ovsCtlClient) RunAppctlCmd(cmd string, needsBridge bool, args ...string) ([]byte, error) { return c.ovsAppctlRunner.RunAppctlCmd(cmd, needsBridge, args...) } @@ -399,7 +400,7 @@ func (c *ovsCtlClient) DumpPortsDesc() ([][]string, error) { func (c *ovsCtlClient) SetPortNoFlood(ofport int) error { // This command does not have standard output, and only has standard err when running with error. // NOTE THAT, THIS CONFIGURATION MUST WORK WITH OpenFlow10. - _, err := runOfctlCmd(false, "mod-port", c.bridge, strconv.FormatUint(uint64(ofport), 10), "no-flood") + _, err := runOfctlCmd(context.TODO(), false, "mod-port", c.bridge, strconv.FormatUint(uint64(ofport), 10), "no-flood") if err != nil { return fmt.Errorf("fail to set no-food config for port %d on bridge %s: %v", ofport, c.bridge, err) } diff --git a/pkg/ovs/ovsctl/ovsctl_others.go b/pkg/ovs/ovsctl/ovsctl_others.go index ada7936d151..c4a58ff2a39 100644 --- a/pkg/ovs/ovsctl/ovsctl_others.go +++ b/pkg/ovs/ovsctl/ovsctl_others.go @@ -18,9 +18,9 @@ package ovsctl import ( + "context" "fmt" "os" - "os/exec" "time" "k8s.io/apimachinery/pkg/util/wait" @@ -44,7 +44,7 @@ func readOVSVSwitchdPID() (int, error) { } // ovsVSwitchdUDS returns the file path of the ovs-vswitchd control UNIX domain socket. -func ovsVSwitchdUDS() string { +func ovsVSwitchdUDS(ctx context.Context) (string, error) { // It is a bit sub-optimal to read the PID every time we need it, but ovs-vswitchd restarts // are possible. Besides, this value is only used when invoking ovs-appctl (as a new // process) at the moment, so the overhead of reading the PID from file should not be a @@ -53,7 +53,7 @@ func ovsVSwitchdUDS() string { var readErr error startTime := time.Now() hasFailure := false - pollErr := wait.PollImmediate(50*time.Millisecond, 5*time.Second, func() (bool, error) { + err := wait.PollImmediateWithContext(ctx, 50*time.Millisecond, 5*time.Second, func(ctx context.Context) (bool, error) { pid, readErr = readOVSVSwitchdPID() if readErr != nil { hasFailure = true @@ -61,17 +61,11 @@ func ovsVSwitchdUDS() string { } return true, nil }) - if pollErr != nil { - klog.ErrorS(readErr, "Failed to read ovs-vswitchd PID") - // that seems like a reasonable value to return if we cannot read the PID - return "/var/run/openvswitch/ovs-vswitchd.*.ctl" + if err != nil { + return "", fmt.Errorf("failed to read ovs-vswitchd PID: %w", readErr) } if hasFailure { klog.V(2).InfoS("Waited for ovs-vswitchd PID to be ready", "duration", time.Since(startTime)) } - return fmt.Sprintf("/var/run/openvswitch/ovs-vswitchd.%d.ctl", pid) -} - -func getOVSCommand(cmdStr string) *exec.Cmd { - return exec.Command("/bin/sh", "-c", cmdStr) + return fmt.Sprintf("/var/run/openvswitch/ovs-vswitchd.%d.ctl", pid), nil } diff --git a/pkg/ovs/ovsctl/ovsctl_windows.go b/pkg/ovs/ovsctl/ovsctl_windows.go index fef17bf110f..3a05b478004 100644 --- a/pkg/ovs/ovsctl/ovsctl_windows.go +++ b/pkg/ovs/ovsctl/ovsctl_windows.go @@ -17,13 +17,11 @@ package ovsctl -import "os/exec" +import ( + "context" +) // ovsVSwitchdUDS returns the file path of the ovs-vswitchd control named pipe. -func ovsVSwitchdUDS() string { - return "c:/openvswitch/var/run/openvswitch/ovs-vswitchd.ctl" -} - -func getOVSCommand(cmdStr string) *exec.Cmd { - return exec.Command("cmd.exe", "/c", cmdStr) +func ovsVSwitchdUDS(ctx context.Context) (string, error) { + return "c:/openvswitch/var/run/openvswitch/ovs-vswitchd.ctl", nil } diff --git a/pkg/ovs/ovsctl/testing/mock_ovsctl.go b/pkg/ovs/ovsctl/testing/mock_ovsctl.go index ded0c0ea56d..656b09e60d6 100644 --- a/pkg/ovs/ovsctl/testing/mock_ovsctl.go +++ b/pkg/ovs/ovsctl/testing/mock_ovsctl.go @@ -191,7 +191,7 @@ func (mr *MockOVSCtlClientMockRecorder) GetDPFeatures() *gomock.Call { } // RunAppctlCmd mocks base method -func (m *MockOVSCtlClient) RunAppctlCmd(arg0 string, arg1 bool, arg2 ...string) ([]byte, *ovsctl.ExecError) { +func (m *MockOVSCtlClient) RunAppctlCmd(arg0 string, arg1 bool, arg2 ...string) ([]byte, error) { m.ctrl.T.Helper() varargs := []interface{}{arg0, arg1} for _, a := range arg2 { @@ -199,7 +199,7 @@ func (m *MockOVSCtlClient) RunAppctlCmd(arg0 string, arg1 bool, arg2 ...string) } ret := m.ctrl.Call(m, "RunAppctlCmd", varargs...) ret0, _ := ret[0].([]byte) - ret1, _ := ret[1].(*ovsctl.ExecError) + ret1, _ := ret[1].(error) return ret0, ret1 }