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.

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

feat(idpool): idpool feature for generating id's #400

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

venkyvsp
Copy link
Contributor

idpool feature for generating id's for mod pointer, routing table id , trie pointer.

@venkyvsp venkyvsp marked this pull request as ready for review September 23, 2024 07:09
@venkyvsp venkyvsp requested a review from a team as a code owner September 23, 2024 07:09
func GenerateRouteTable() uint32 {
return uint32(rand.Intn(RoutingTableMax-RoutingTableMin+1) + RoutingTableMin) //nolint:gosec
// RoutingTableRange ModPointer structure of mod ptr definitions
var RoutingTableRange = struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

if not used in other modules, then the type and members can be lowercased

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

@@ -363,7 +363,8 @@ func Initialize() {
brTenant = "br-tenant"
ipMtu = config.GlobalConfig.LinuxFrr.IPMtu
ctx = context.Background()
nlink = utils.NewNetlinkWrapperWithArgs(config.GlobalConfig.Tracer)
RouteTableGen = utils.IDPoolInit("RTtable", RoutingTableRange.RoutingTableMin, RoutingTableRange.RoutingTableMax)
Copy link
Contributor

Choose a reason for hiding this comment

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

if RoutingTableRange is used only to put values here, then why not just use 2 constants routingTableMin and routingTableMax?

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

@@ -454,6 +455,7 @@ func setUpBridge(lb *infradb.LogicalBridge) bool {
//nolint:funlen,gocognit
func setUpVrf(vrf *infradb.Vrf) (string, bool) {
IPMtu := fmt.Sprintf("%+v", ipMtu)
var addKey int
Copy link
Contributor

Choose a reason for hiding this comment

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

why we need to declare the variable here, if you zero it on 471?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified

@@ -465,8 +467,12 @@ func setUpVrf(vrf *infradb.Vrf) (string, bool) {
vrf.Metadata.RoutingTable = make([]*uint32, 1)
vrf.Metadata.RoutingTable[0] = new(uint32)
var routingTable uint32
Name := vrf.Name
Copy link
Contributor

Choose a reason for hiding this comment

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

please align with Go style guide:
https://google.github.io/styleguide/go/decisions.html

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

for {
routingTable = GenerateRouteTable()
// var key interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

if code is not sued, it can be removed

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

pkg/utils/Idpool.go Outdated Show resolved Hide resolved
}

// ReleaseID get the reference id
func (ip *IDPool) ReleaseID(key interface{}, ref interface{}) (uint32, uint32) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see that ReleaseID is used anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its used in vendor plugin code dcgw.go for releasing the ecmp and trie id's


// IDPool structure
/* Helper class for uniquely assigning IDs from a specified integer set (e.g. a
# range) to keys. IDs are assigned (or read) with get_id(key) and returned back
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no get_id(key) function

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

// IDPool structure
/* Helper class for uniquely assigning IDs from a specified integer set (e.g. a
# range) to keys. IDs are assigned (or read) with get_id(key) and returned back
# into the pool with release_id(key). The IDPool remembers a once-assigned ID
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no release_id(key) function

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

_unusedIDs []uint32 // Yet unused IDs in pool Available ids
_idsInUse map[interface{}]uint32 // Mapping key: id for currently assigned ids
_idsForReuse map[interface{}]uint32 // Mapping key: id for previously assigned ids
_refs map[uint32][]interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

refs sounds like a counter tracking how many times we requested the same id, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, its reference to the same id

Signed-off-by: Venkatesh, Vemula <[email protected]>
Signed-off-by: Atul Patel <[email protected]>
Signed-off-by: Vemula Venkatesh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants