Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 6 commits into from
+271 −249

Conversation

venkyvsp
Copy link
Contributor

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

@venkyvsp venkyvsp marked this pull request as ready for review September 25, 2024 07:50
@venkyvsp venkyvsp requested a review from a team as a code owner September 25, 2024 07:50
@venkyvsp venkyvsp marked this pull request as draft September 27, 2024 11:47
artek-koltun
artek-koltun reviewed Sep 27, 2024
View reviewed changes
pkg/frr/frr.go Outdated Show resolved Hide resolved
pkg/frr/frr.go Outdated
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does data contain error description?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes data contains the complete FRR command output.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

pkg/frr/frr.go Outdated
@@ -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"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. all local vars should start from lowercase. I see Patterns in that function. Pls check the code you touched in the PR
  2. do not use reflect.ValueOf(...).IsZero() frequently, you can use simple comparison. Pls check all cases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pkg/frr/frr.go Outdated
}
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic number. Rely on string length is very unreliable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed this and changed to string compare instead of string length

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the comparing the string with alphanumerical.

@artek-koltun artek-koltun marked this pull request as ready for review September 27, 2024 13:55
@codecov Codecov
Copy link

codecov bot commented Sep 30, 2024

Codecov Report

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

Project coverage is 20.23%. Comparing base (5440fb9) to head (e8efd4f).
Report is 36 commits behind head on main.

Files with missing lines Patch % Lines
pkg/LinuxGeneralModule/lgm.go 0.00% 80 Missing ⚠️
pkg/frr/frr.go 0.00% 56 Missing ⚠️
pkg/LinuxCIModule/lci.go 0.00% 25 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##             main     #401       +/-   ##
===========================================
- Coverage   50.77%   20.23%   -30.54%     
===========================================
  Files          37       51       +14     
  Lines        2525     6863     +4338     
===========================================
+ Hits         1282     1389      +107     
- Misses       1114     5306     +4192     
- Partials      129      168       +39
Flag Coverage Δ
20.23% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@venkyvsp venkyvsp force-pushed the opi_frr_bug_main_pr branch 2 times, most recently from a553e28 to 5ab2fc8 Compare October 1, 2024 07:15
venkyvsp and others added 6 commits October 28, 2024 06:16
@venkyvsp @vjambeka
fix(evpn-bridge): fix for frr to handle vrf setup and teardown
a074efe 
Signed-off-by: Venkatesh, Vemula <venkatesh.vemula@intel.com>
@atulpatel261194 @vjambeka
fix(evpn): report error in comp details
d2094b1 
Signed-off-by: atulpatel261194 <Atul.Patel@intel.com>
@venkyvsp @vjambeka
fix(evpn): changed the string length in if condition to string compare
5ae39df 
Signed-off-by: Vemula Venkatesh <venkatesh.vemula@intel.com>
@atulpatel261194 @vjambeka
fix(evpn): report error in comp details review fixes
1076f6a 
Signed-off-by: atulpatel261194 <Atul.Patel@intel.com>
@venkyvsp @vjambeka
fix(evpn): frr bug pr review fixes
3a22a16 
Signed-off-by: Vemula Venkatesh <venkatesh.vemula@intel.com>
@atulpatel261194
fix(evpn): fix error handling in frr utils
165664c 
Signed-off-by: atulpatel261194 <Atul.Patel@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atulpatel261194 atulpatel261194 atulpatel261194 left review comments

@artek-koltun artek-koltun artek-koltun left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@venkyvsp @atulpatel261194 @artek-koltun