refactor: optimize ovn command when error exists

kubeovn · Mar 30, 2021 · acade01 · acade01
1 parent 7d1004e
commit acade01
Show file tree

Hide file tree

Showing 7 changed files with 38 additions and 37 deletions.
diff --git a/cmd/cni/cni.go b/cmd/cni/cni.go
@@ -172,9 +172,9 @@ func parseValueFromArgs(key, argString string) (string, error) {
 	args := strings.Split(argString, ";")
 	for _, arg := range args {
 		if strings.HasPrefix(arg, fmt.Sprintf("%s=", key)) {
-			podName := strings.TrimPrefix(arg, fmt.Sprintf("%s=", key))
-			if len(podName) > 0 {
-				return podName, nil
+			value := strings.TrimPrefix(arg, fmt.Sprintf("%s=", key))
+			if len(value) > 0 {
+				return value, nil
 			}
 		}
 	}

diff --git a/pkg/daemon/handler.go b/pkg/daemon/handler.go
@@ -218,7 +218,7 @@ func (csh cniServerHandler) handleDel(req *restful.Request, resp *restful.Respon
 	ipCrName := ovs.PodNameToPortName(podRequest.PodName, podRequest.PodNamespace, podRequest.Provider)
 	err = csh.KubeOvnClient.KubeovnV1().IPs().Delete(context.Background(), ipCrName, metav1.DeleteOptions{})
 	if err != nil && !k8serrors.IsNotFound(err) {
-		errMsg := fmt.Errorf("del ipcrd for %s failed %v", ipCrName, err)
+		errMsg := fmt.Errorf("del ip crd for %s failed %v", ipCrName, err)
 		klog.Error(errMsg)
 		if err := resp.WriteHeaderAndEntity(http.StatusInternalServerError, request.CniResponse{Err: errMsg.Error()}); err != nil {
 			klog.Errorf("failed to write response, %v", err)

diff --git a/pkg/ovs/ovn-ic-nbctl.go b/pkg/ovs/ovn-ic-nbctl.go
@@ -13,11 +13,7 @@ func (c Client) ovnIcNbCommand(cmdArgs ...string) (string, error) {
 	cmdArgs = append([]string{fmt.Sprintf("--timeout=%d", c.OvnTimeout), fmt.Sprintf("--db=tcp:%s", c.OVNIcNBAddress)}, cmdArgs...)
 	raw, err := exec.Command(OVNIcNbCtl, cmdArgs...).CombinedOutput()
 	elapsed := float64((time.Since(start)) / time.Millisecond)
-	klog.V(4).Infof("%s command %s in %vms", OVNIcNbCtl, strings.Join(cmdArgs, " "), elapsed)
-	if err != nil || elapsed > 500 {
-		klog.Warning("ovn-ic-nbctl command error or took too long")
-		klog.Warningf("%s %s in %vms", OVNIcNbCtl, strings.Join(cmdArgs, " "), elapsed)
-	}
+	klog.V(4).Infof("command %s %s in %vms", OVNIcNbCtl, strings.Join(cmdArgs, " "), elapsed)
 	method := ""
 	for _, arg := range cmdArgs {
 		if !strings.HasPrefix(arg, "--") {
@@ -26,12 +22,16 @@ func (c Client) ovnIcNbCommand(cmdArgs ...string) (string, error) {
 		}
 	}
 	code := "0"
+	defer func() {
+		ovsClientRequestLatency.WithLabelValues("ovn-ic-nb", method, code).Observe(elapsed)
+	}()
+
 	if err != nil {
 		code = "1"
-	}
-	ovsClientRequestLatency.WithLabelValues("ovn-ic-nb", method, code).Observe(elapsed)
-	if err != nil {
+		klog.Warningf("ovn-ic-nbctl command error: %s %s in %vms", OVNIcNbCtl, strings.Join(cmdArgs, " "), elapsed)
 		return "", fmt.Errorf("%s, %q", raw, err)
+	} else if elapsed > 500 {
+		klog.Warningf("ovn-ic-nbctl command took too long: %s %s in %vms", OVNIcNbCtl, strings.Join(cmdArgs, " "), elapsed)
 	}
 	return trimCommandOutput(raw), nil
 }

diff --git a/pkg/ovs/ovn-nbctl.go b/pkg/ovs/ovn-nbctl.go
@@ -22,10 +22,6 @@ func (c Client) ovnNbCommand(cmdArgs ...string) (string, error) {
 	raw, err := exec.Command(OvnNbCtl, cmdArgs...).CombinedOutput()
 	elapsed := float64((time.Since(start)) / time.Millisecond)
 	klog.V(4).Infof("command %s %s in %vms", OvnNbCtl, strings.Join(cmdArgs, " "), elapsed)
-	if err != nil || elapsed > 500 {
-		klog.Warning("ovn-nbctl command error or took too long")
-		klog.Warningf("%s %s in %vms", OvnNbCtl, strings.Join(cmdArgs, " "), elapsed)
-	}
 	method := ""
 	for _, arg := range cmdArgs {
 		if !strings.HasPrefix(arg, "--") {
@@ -34,12 +30,16 @@ func (c Client) ovnNbCommand(cmdArgs ...string) (string, error) {
 		}
 	}
 	code := "0"
+	defer func() {
+		ovsClientRequestLatency.WithLabelValues("ovn-nb", method, code).Observe(elapsed)
+	}()
+
 	if err != nil {
 		code = "1"
-	}
-	ovsClientRequestLatency.WithLabelValues("ovn-nb", method, code).Observe(elapsed)
-	if err != nil {
+		klog.Warningf("ovn-nbctl command error: %s %s in %vms", OvnNbCtl, strings.Join(cmdArgs, " "), elapsed)
 		return "", fmt.Errorf("%s, %q", raw, err)
+	} else if elapsed > 500 {
+		klog.Warningf("ovn-nbctl command took too long: %s %s in %vms", OvnNbCtl, strings.Join(cmdArgs, " "), elapsed)
 	}
 	return trimCommandOutput(raw), nil
 }

diff --git a/pkg/ovs/ovn-sbctl.go b/pkg/ovs/ovn-sbctl.go
@@ -26,11 +26,7 @@ func (c Client) ovnSbCommand(cmdArgs ...string) (string, error) {
 	}
 	raw, err := exec.Command(OvnSbCtl, cmdArgs...).CombinedOutput()
 	elapsed := float64((time.Since(start)) / time.Millisecond)
-	klog.V(4).Infof("%s command %s in %vms", OvnSbCtl, strings.Join(cmdArgs, " "), elapsed)
-	if err != nil || elapsed > 500 {
-		klog.Warning("ovn-sbctl command error or took too long")
-		klog.Warningf("%s %s in %vms", OvnSbCtl, strings.Join(cmdArgs, " "), elapsed)
-	}
+	klog.V(4).Infof("command %s %s in %vms", OvnSbCtl, strings.Join(cmdArgs, " "), elapsed)
 	method := ""
 	for _, arg := range cmdArgs {
 		if !strings.HasPrefix(arg, "--") {
@@ -39,12 +35,16 @@ func (c Client) ovnSbCommand(cmdArgs ...string) (string, error) {
 		}
 	}
 	code := "0"
+	defer func() {
+		ovsClientRequestLatency.WithLabelValues("ovn-sb", method, code).Observe(elapsed)
+	}()
+
 	if err != nil {
 		code = "1"
-	}
-	ovsClientRequestLatency.WithLabelValues("ovn-sb", method, code).Observe(elapsed)
-	if err != nil {
+		klog.Warningf("ovn-sbctl command error: %s %s in %vms", OvnSbCtl, strings.Join(cmdArgs, " "), elapsed)
 		return "", fmt.Errorf("%s, %q", raw, err)
+	} else if elapsed > 500 {
+		klog.Warningf("ovn-sbctl command took too long: %s %s in %vms", OvnSbCtl, strings.Join(cmdArgs, " "), elapsed)
 	}
 	return trimCommandOutput(raw), nil
 }

diff --git a/pkg/ovs/ovn.go b/pkg/ovs/ovn.go
@@ -29,6 +29,7 @@ const (
 	OvnNbCtl    = "ovn-nbctl"
 	OvnSbCtl    = "ovn-sbctl"
 	OVNIcNbCtl  = "ovn-ic-nbctl"
+	OvsVsCtl    = "ovs-vsctl"
 	MayExist    = "--may-exist"
 	IfExists    = "--if-exists"
 	Policy      = "--policy"

diff --git a/pkg/ovs/ovs-vsctl.go b/pkg/ovs/ovs-vsctl.go
@@ -16,13 +16,9 @@ import (
 func Exec(args ...string) (string, error) {
 	start := time.Now()
 	args = append([]string{"--timeout=30"}, args...)
-	output, err := exec.Command("ovs-vsctl", args...).CombinedOutput()
+	output, err := exec.Command(OvsVsCtl, args...).CombinedOutput()
 	elapsed := float64((time.Since(start)) / time.Millisecond)
-	klog.V(4).Infof("command ovs-vsctl %s in %vms", strings.Join(args, " "), elapsed)
-	if err != nil || elapsed > 500 {
-		klog.Warning("ovs-vsctl command error or took too long")
-		klog.Warningf("ovs-vsctl %s in %vms", strings.Join(args, " "), elapsed)
-	}
+	klog.V(4).Infof("command %s %s in %vms", OvsVsCtl, strings.Join(args, " "), elapsed)
 	method := ""
 	for _, arg := range args {
 		if !strings.HasPrefix(arg, "--") {
@@ -31,12 +27,16 @@ func Exec(args ...string) (string, error) {
 		}
 	}
 	code := "0"
+	defer func() {
+		ovsClientRequestLatency.WithLabelValues("ovsdb", method, code).Observe(elapsed)
+	}()
+
 	if err != nil {
 		code = "1"
-	}
-	ovsClientRequestLatency.WithLabelValues("ovsdb", method, code).Observe(elapsed)
-	if err != nil {
-		return "", fmt.Errorf("failed to run 'ovs-vsctl %s': %v\n  %q", strings.Join(args, " "), err, output)
+		klog.Warningf("ovs-vsctl command error: %s %s in %vms", OvsVsCtl, strings.Join(args, " "), elapsed)
+		return "", fmt.Errorf("failed to run '%s %s': %v\n  %q", OvsVsCtl, strings.Join(args, " "), err, output)
+	} else if elapsed > 500 {
+		klog.Warningf("ovs-vsctl command took too long: %s %s in %vms", OvsVsCtl, strings.Join(args, " "), elapsed)
 	}
 	return trimCommandOutput(output), nil
 }