fix(evpn-bridge): fix for frr to handle vrf setup and teardown #401

venkyvsp · 2024-09-25T07:20:27Z

fix: frr handle the duplicate vni and deleting of non-existence vrf and svi.

pkg/frr/frr.go

artek-koltun · 2024-09-27T13:44:51Z

pkg/frr/frr.go

-		if err != nil {
+		data, err = Frr.FrrZebraCmd(ctx, fmt.Sprintf("configure terminal\n %s\n exit\n", delCmd2))
+		if err != nil || checkFrrResult(data, false) {
+			log.Printf("FRR: Error  %s\n", data)


Does data contain error description?

Yes data contains the complete FRR command output.

would it be better in case of an error, in FrrBgpCmd, just add data content into err and return empty first argument?

artek-koltun · 2024-09-27T13:48:16Z

pkg/frr/frr.go

@@ -437,7 +430,7 @@ func setUpVrf(vrf *infradb.Vrf) (string, bool) {

 // checkFrrResult checks the vrf result
 func checkFrrResult(cp string, show bool) bool {
-	return ((show && reflect.ValueOf(cp).IsZero()) || strings.Contains(cp, "warning") || strings.Contains(cp, "unknown") || strings.Contains(cp, "Unknown") || strings.Contains(cp, "Warning") || strings.Contains(cp, "Ambiguous") || strings.Contains(cp, "specified does not exist"))
+	return ((show && reflect.ValueOf(cp).IsZero()) || strings.Contains(cp, "warning") || strings.Contains(cp, "unknown") || strings.Contains(cp, "Unknown") || strings.Contains(cp, "Warning") || strings.Contains(cp, "Ambiguous") || strings.Contains(cp, "specified does not exist") || strings.Contains(cp, "Error"))


reflect.ValueOf(cp).IsZero()) -> cp == ""

what if cp contains waRning? Will you add a new comparison then? Will it be better to lower case the string and only then check what itcontains?

all local vars should start from lowercase. I see Patterns in that function. Pls check the code you touched in the PR

do not use reflect.ValueOf(...).IsZero() frequently, you can use simple comparison. Pls check all cases

Changed all local vars names in frr.go and replaced all reflect.valueof IsZero with simple comparison.

artek-koltun · 2024-09-27T13:54:48Z

pkg/frr/frr.go

 		}
 		hname, _ := os.Hostname()
 		L2vpnCmd := strings.Split(cp, "json")
 		L2vpnCmd = strings.Split(L2vpnCmd[1], hname)
 		cp = L2vpnCmd[0]
-		// fmt.Printf("FRR_L2vpn[0]: %s\n",cp)
-		if len(cp) != 7 {
+		if len(cp) != 7 { // Checking CMD o/p


magic number. Rely on string length is very unreliable

Addressed this and changed to string compare instead of string length

is response should be in json format? Should we better try to Unmarshall it then using"\r\n{}\r\n{\r\n}\r\n"? If not, maybe we could check if it contains alphanumerical values instead?

Changed the comparing the string with alphanumerical.

codecov · 2024-09-30T19:48:19Z

Codecov Report

Attention: Patch coverage is 0% with 30 lines in your changes missing coverage. Please review.

Project coverage is 42.82%. Comparing base (5440fb9) to head (e6b6b4a).
Report is 48 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/utils/frr.go	0.00%	30 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (5440fb9) and HEAD (e6b6b4a). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (5440fb9) HEAD (e6b6b4a)

2 1

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #401      +/-   ##
==========================================
- Coverage   50.77%   42.82%   -7.95%     
==========================================
  Files          37       25      -12     
  Lines        2525     1721     -804     
==========================================
- Hits         1282      737     -545     
+ Misses       1114      896     -218     
+ Partials      129       88      -41

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Venkatesh, Vemula <[email protected]>

Signed-off-by: atulpatel261194 <[email protected]>

Signed-off-by: Vemula Venkatesh <[email protected]>

Signed-off-by: atulpatel261194 <[email protected]>

Signed-off-by: Vemula Venkatesh <[email protected]>

Signed-off-by: atulpatel261194 <[email protected]>

venkyvsp marked this pull request as ready for review September 25, 2024 07:50

venkyvsp requested a review from a team as a code owner September 25, 2024 07:50

venkyvsp marked this pull request as draft September 27, 2024 11:47

artek-koltun reviewed Sep 27, 2024

View reviewed changes

artek-koltun marked this pull request as ready for review September 27, 2024 13:55

atulpatel261194 force-pushed the opi_frr_bug_main_pr branch from 585cd47 to 138318f Compare September 30, 2024 11:34

venkyvsp force-pushed the opi_frr_bug_main_pr branch 2 times, most recently from a553e28 to 5ab2fc8 Compare October 1, 2024 07:15

atulpatel261194 force-pushed the opi_frr_bug_main_pr branch from 05da753 to a8da21f Compare October 25, 2024 08:50

venkyvsp and others added 5 commits October 28, 2024 06:16

fix(evpn-bridge): fix for frr to handle vrf setup and teardown

a074efe

Signed-off-by: Venkatesh, Vemula <[email protected]>

fix(evpn): report error in comp details

d2094b1

Signed-off-by: atulpatel261194 <[email protected]>

fix(evpn): changed the string length in if condition to string compare

5ae39df

Signed-off-by: Vemula Venkatesh <[email protected]>

fix(evpn): report error in comp details review fixes

1076f6a

Signed-off-by: atulpatel261194 <[email protected]>

fix(evpn): frr bug pr review fixes

3a22a16

Signed-off-by: Vemula Venkatesh <[email protected]>

atulpatel261194 force-pushed the opi_frr_bug_main_pr branch from 4ee351a to 165664c Compare October 28, 2024 07:13

fix(evpn): fix error handling in frr utils

3174273

Signed-off-by: atulpatel261194 <[email protected]>

venkyvsp force-pushed the opi_frr_bug_main_pr branch from 165664c to 3174273 Compare November 7, 2024 11:28

fix(evpn): fix ci docker failure

e6b6b4a

Signed-off-by: atulpatel261194 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(evpn-bridge): fix for frr to handle vrf setup and teardown #401

fix(evpn-bridge): fix for frr to handle vrf setup and teardown #401

venkyvsp commented Sep 25, 2024

artek-koltun Sep 27, 2024

venkyvsp Sep 30, 2024

artek-koltun Oct 22, 2024

atulpatel261194 Oct 28, 2024

artek-koltun Sep 27, 2024

venkyvsp Oct 3, 2024

artek-koltun Oct 22, 2024

venkyvsp Oct 25, 2024

artek-koltun Sep 27, 2024

venkyvsp Oct 1, 2024

artek-koltun Oct 22, 2024

venkyvsp Oct 25, 2024

codecov bot commented Sep 30, 2024 •

edited

Loading

fix(evpn-bridge): fix for frr to handle vrf setup and teardown #401

Are you sure you want to change the base?

fix(evpn-bridge): fix for frr to handle vrf setup and teardown #401

Conversation

venkyvsp commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 30, 2024 • edited Loading

Codecov Report

codecov bot commented Sep 30, 2024 •

edited

Loading