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

[Chassis][LAG_ID] Address the same lagid been used in two different LCs issue #3303

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+42 −17
Open

[Chassis][LAG_ID] Address the same lagid been used in two different LCs issue #3303

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2c4f2f9
[Chassis][LAG_ID] Address the same lagid been used in two different L…
mlok-nokia Sep 28, 2024
509c5d0
Fixed test case
mlok-nokia Sep 30, 2024
e9938d5
Fixed vs test failure
mlok-nokia Oct 1, 2024
c64dbf1
Merge branch 'master' into lag_id_conflict_issue
mlok-nokia Oct 8, 2024
4a0e6f8
Merge branch 'master' into lag_id_conflict_issue
judyjoseph Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 40 additions & 17 deletions orchagent/lagids.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,28 +37,48 @@ if op == "add" then
end
-- proposed lag id is different than that in database OR
-- the portchannel does not exist in the database
-- If proposed lagid is available, return the same proposed lag id
if redis.call("sismember", "SYSTEM_LAG_ID_SET", tostring(plagid)) == 0 then
redis.call("sadd", "SYSTEM_LAG_ID_SET", tostring(plagid))
redis.call("srem", "SYSTEM_LAG_ID_SET", tostring(dblagid))
-- If proposed lagid is not available, lpop the first availabe ID
local index = redis.call("lpos", "SYSTEM_LAG_IDS_FREE_LIST", tostring(plagid))
if index == false then
if redis.call("llen", "SYSTEM_LAG_IDS_FREE_LIST") <= 0 then
return -1
end
local lagid = redis.call("lpop", "SYSTEM_LAG_IDS_FREE_LIST")
redis.call("hset", "SYSTEM_LAG_ID_TABLE", pcname, lagid)
redis.call("sadd", "SYSTEM_LAG_ID_SET", lagid)
if dblagid then
Copy link
Contributor

Choose a reason for hiding this comment

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

this code under "if dblagid then" is duplicated in both if and else

Copy link
Author

@mlok-nokia mlok-nokia Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

They are the same in the if and else. Not able to simplify them in one because of "if" case returns "tonumber(lagid)" while the "else" case returns "plagid"

redis.call("srem", "SYSTEM_LAG_ID_SET", tostring(dblagid))
if redis.call("lpos", "SYSTEM_LAG_IDS_FREE_LIST", tostring(dblagid)) == false then
redis.call("rpush", "SYSTEM_LAG_IDS_FREE_LIST", tostring(dblagid))
end
end
return tonumber(lagid)
else
redis.call("lrem", "SYSTEM_LAG_IDS_FREE_LIST", 1, tostring(plagid))
redis.call("hset", "SYSTEM_LAG_ID_TABLE", pcname, tostring(plagid))
redis.call("sadd", "SYSTEM_LAG_ID_SET", tostring(plagid))
if dblagid then
redis.call("srem", "SYSTEM_LAG_ID_SET", tostring(dblagid))
if redis.call("lpos", "SYSTEM_LAG_IDS_FREE_LIST", tostring(dblagid)) == false then
redis.call("rpush", "SYSTEM_LAG_IDS_FREE_LIST", tostring(dblagid))
end
end
return plagid
end
end

local lagid = lagid_start
while lagid <= lagid_end do
if redis.call("sismember", "SYSTEM_LAG_ID_SET", tostring(lagid)) == 0 then
redis.call("sadd", "SYSTEM_LAG_ID_SET", tostring(lagid))
redis.call("srem", "SYSTEM_LAG_ID_SET", tostring(dblagid))
redis.call("hset", "SYSTEM_LAG_ID_TABLE", pcname, tostring(lagid))
return lagid
else
if redis.call("llen", "SYSTEM_LAG_IDS_FREE_LIST") <= 0 then
return -1
end
lagid = lagid + 1
local lagid = redis.call("lpop", "SYSTEM_LAG_IDS_FREE_LIST")
redis.call("hset", "SYSTEM_LAG_ID_TABLE", pcname, lagid)
redis.call("sadd", "SYSTEM_LAG_ID_SET", lagid)
if dblagid then
Copy link
Contributor

Choose a reason for hiding this comment

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

why use dblag here?

Copy link
Author

@mlok-nokia mlok-nokia Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

why use dblag here?

This is just for handling the out of sync cases. As normal operation, this should not happen.
In the old method, when there is a valid dblagid number (it is from a broken entry in SYSTEM_LAG_ID_TABLE), if a new lagId is assigned, the dblagid needs to be removed from the SYSTEM_LAG_ID_SET.
With the new modification, we need to handle it as the same -- if old lagId (dblagId) is valid lag ID, we need to it to the free_list.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should do

redis.call("srem", "SYSTEM_LAG_ID_SET", tostring(dblagid))

here too ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. In case of the old lagId (dblagid) still in the SYSTEM_LAG_ID_SET in a broken scenario, we just remove it. Anyway, this SYSTEM_LAG_ID_SET is just keeped for debugging purposes. We don't use it for any functionality. SYSTEM_LAG_ID_SET and SYSTM_LAG_IDS_FREE_LIST are mutual exclusive.

if redis.call("lpos", "SYSTEM_LAG_IDS_FREE_LIST", tostring(dblagid)) == false then
redis.call("rpush", "SYSTEM_LAG_IDS_FREE_LIST", tostring(dblagid))
end
end
return tonumber(lagid)
end

return -1

end

if op == "del" then
Expand All @@ -67,6 +87,9 @@ if op == "del" then
local lagid = redis.call("hget", "SYSTEM_LAG_ID_TABLE", pcname)
redis.call("srem", "SYSTEM_LAG_ID_SET", lagid)
redis.call("hdel", "SYSTEM_LAG_ID_TABLE", pcname)
if redis.call("lpos", "SYSTEM_LAG_IDS_FREE_LIST", lagid) == false then
redis.call("rpush", "SYSTEM_LAG_IDS_FREE_LIST", lagid)
end
return tonumber(lagid)
end

Expand Down
2 changes: 2 additions & 0 deletions tests/test_virtual_chassis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ def set_lag_id_boundaries(self, vct):
chassis_app_db = DVSDatabase(swsscommon.CHASSIS_APP_DB, dvs.redis_chassis_sock)
chassis_app_db.db_connection.set("SYSTEM_LAG_ID_START", "1")
chassis_app_db.db_connection.set("SYSTEM_LAG_ID_END", "2")
chassis_app_db.db_connection.rpush("SYSTEM_LAG_IDS_FREE_LIST", "1")
chassis_app_db.db_connection.rpush("SYSTEM_LAG_IDS_FREE_LIST", "2")
break

def config_inbandif_port(self, vct, ibport):
Expand Down
Loading