feat(evpn-bridge): netlink watcher Module

[atulpatel261194]

No description provided.

[artek-koltun]

If you write to a closed channel, your program will panic, and it is possible since someone can still be writing on the line 56 to a chan.
Usually it is a responsibility of the send side to close the chan.

[atulpatel261194]

chan will be closed only when we deinitialize from vendor module. there are some missing pieces in unsubscriber function . will be added in vendor module PR

[artek-koltun]

hmmm... it might work with mutexes, but can we do better? Could WaitGroup help here?

[artek-koltun]

we have many global vars scattered here and there in this file. Could we group them within a struct, maybe even multiple structs, separated by their purpose? It is sometimes hard to follow what function can change. With usage of global vars and functions, they can change any of them. Functions could operate not on global vars, but on members of a specific struct instance

[atulpatel261194]

done

[artek-koltun]

some more notes

[artek-koltun]

how do you know that you have 2 elements in event array?

[Inbanoth]

done

[artek-koltun]

not done. I see

if rv.Vrf.Status.VrfOperStatus == infradb.VrfOperStatusToBeDeleted {
   notifyAddDel(rv, event.Operation[2])

What does index 2 mean here? Why was it 1? Would it make sense to create a dedicated struct where we ahve 3 members and their name clearly explain what those events mean?

[Inbanoth]

each event will have 3 operations add, update and delete, accordingly we are filling operations for each event type, so that's where we have length 3 and index 2 for delete, which already defined as constants

[artek-koltun]

For me, it is just a magic number. You might have context how indexing in this array correspond to operations, but I do not have such a context and people who come and try to understand this code, does not have this context either.
Why we cannot use?

notifyAddDel(rv, event.operations.to_delete)

Plus

The code chunk at the moment look like

if event.EventType == ROUTE || event.EventType == NEXTHOP {
var rv *RouteStruct
var nv *NexthopStruct
if event.EventType == ROUTE {
	rv, ok = v1.(*RouteStruct)
	if !ok {
		log.Printf("Netlink: Type mismatch")
		continue
	}
	if rv.Vrf.Status.VrfOperStatus == infradb.VrfOperStatusToBeDeleted {
		notifyAddDel(rv, event.Operation[2])
		delete(new_db, k1)
		delete(old_db, k1)
		continue
	}
} else if event.EventType == NEXTHOP {
	nv, ok = v1.(*NexthopStruct)
	if !ok {
		log.Printf("Netlink: Type mismatch")
		continue
	}
	if nv.Vrf.Status.VrfOperStatus == infradb.VrfOperStatusToBeDeleted {
		notifyAddDel(nv, event.Operation[2])
		delete(new_db, k1)
		delete(old_db, k1)
		continue
	}
}

Don't you have a feeling that the code in first if is very similar in the code in the second if?
Can we have an interface which would return the status, type assert to that interface once, get the status and do your actions without these duplications? Type Assert v1 to one type or another, get status and work with that? Any other means?

[Inbanoth]

Done

[artek-koltun]

This

switch event.EventType {
case ROUTE:
	status, ok = v1.(VrfStatusGetter)
case NEXTHOP:
	status, ok = v1.(VrfStatusGetter)
default:
}

to

status, ok := v1.(VrfStatusGetter)

[Inbanoth]

Done

[artek-koltun]

so if we have v1 and v2 of different types,t hen we return true
looks suspicious

[Inbanoth]

fixed

[artek-koltun]

but what if v1 and v2 of the same type but not RouteStruct or NexthopStruct. It is still true

[Inbanoth]

Done, we now check for 4 possible structs

[artek-koltun]

For me, it is just a magic number. You might have context how indexing in this array correspond to operations, but I do not have such a context and people who come and try to understand this code, does not have this context either.
Why we cannot use?

notifyAddDel(rv, event.operations.to_delete)

Plus

The code chunk at the moment look like

if event.EventType == ROUTE || event.EventType == NEXTHOP {
var rv *RouteStruct
var nv *NexthopStruct
if event.EventType == ROUTE {
	rv, ok = v1.(*RouteStruct)
	if !ok {
		log.Printf("Netlink: Type mismatch")
		continue
	}
	if rv.Vrf.Status.VrfOperStatus == infradb.VrfOperStatusToBeDeleted {
		notifyAddDel(rv, event.Operation[2])
		delete(new_db, k1)
		delete(old_db, k1)
		continue
	}
} else if event.EventType == NEXTHOP {
	nv, ok = v1.(*NexthopStruct)
	if !ok {
		log.Printf("Netlink: Type mismatch")
		continue
	}
	if nv.Vrf.Status.VrfOperStatus == infradb.VrfOperStatusToBeDeleted {
		notifyAddDel(nv, event.Operation[2])
		delete(new_db, k1)
		delete(old_db, k1)
		continue
	}
}

Don't you have a feeling that the code in first if is very similar in the code in the second if?
Can we have an interface which would return the status, type assert to that interface once, get the status and do your actions without these duplications? Type Assert v1 to one type or another, get status and work with that? Any other means?

[mardim91]

Hello @artek-koltun

I have created a dedicated PR for the docker-compose fix: #390

[artek-koltun]

Are you sure that you want to return true, if v1 is none of the listed types?

[Inbanoth]

yes, we deal with only the 4 structures defined.

[artek-koltun]

and if you add a new type, it will be always true if by mistake this code is not updated?

[Inbanoth]

when a new type is declared, one need to implement the difference check function for it, best thing is we can log in default

[artek-koltun]

when a new type is declared, one need to implement the difference check function for it, best thing is we can log in default

best we can do here is to log an error and report that they are not equal if an unknown types are coming

[Inbanoth]

Done

[artek-koltun]

true is still returned

[Inbanoth]

yes, we log and return true because don't want to handle such cases,
In case the values are equal we return true to deepcheck and we don't process it simply delete it,
same when we get unknown structs we log and return true and delete them

[artek-koltun]

why some vars are capitilized, some not? https://google.github.io/styleguide/go/decisions.html

[Inbanoth]

Done

[artek-koltun]

still see some like CPs

[Inbanoth]

Done

[artek-koltun]

Please check other places

[Inbanoth]

Done

[artek-koltun]

#385 (comment)

[artek-koltun]

Despite the current quality disadvantages, we agreed to merge the code as is due to the time constraints. According to the agreement, the tech debt will be re-designed and addressed by the team with the highest priority.