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

[COPP]If copp group is configured using cfgdb, duplicate trapids will… #2509

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

Conversation

qyp0203
Copy link

@qyp0203 qyp0203 commented Nov 10, 2022

… appear in appdb.

Signed-off-by: qiuyupeng qiuyupeng@git.asterfusion.com

What I did
deleting some code about updating cache in coppmgr
Why I did it
It is repetitive and affects subsequent logical judgments
The same code in the next few lines,so i delete it。

1、creating a copp trap group in cfg_db
hset COPP_GROUP|queue7_group7 trap_action copy trap_priority 7 queue 7 "meter_type" "bytes" "mode" "tr_tcm" "cir" "4096000" "cbs" "64000" "pir" "8192000" "pbs" "128000" "red_action" "drop"

2、let arp bind the new group
HSET COPP_TRAP|arp trap_group queue7_group7

3、the old group not del the trap_id(arp) in app db.
127.0.0.1:6379> hgetall "COPP_TABLE:queue7_group7"

  1. "trap_action"
  2. "copy"
  3. "trap_priority"
  4. "7"
  5. "queue"
  6. "7"
  7. "meter_type"
  8. "bytes"
  9. "mode"
  10. "tr_tcm"
  11. "cir"
  12. "4096000"
  13. "cbs"
  14. "64000"
  15. "pir"
  16. "8192000"
  17. "pbs"
  18. "128000"
  19. "red_action"
  20. "drop"
  21. "trap_ids"
  22. "arp_req,arp_resp,neigh_discovery"
    127.0.0.1:6379> hgetall "COPP_TABLE:queue5_group1"
  23. "cbs"
  24. "64000"
  25. "cir"
  26. "512000"
  27. "meter_type"
  28. "bytes"
  29. "mode"
  30. "tr_tcm"
  31. "pbs"
  32. "128000"
  33. "pir"
  34. "1024000"
  35. "queue"
  36. "5"
  37. "red_action"
  38. "drop"
  39. "trap_action"
  40. "copy"
  41. "trap_priority"
  42. "5"
  43. "trap_ids"
  44. "arp_req,arp_resp,neigh_discovery"
    127.0.0.1:6379>

How I verified it
1、creating a copp trap group in cfg_db
hset COPP_GROUP|queue7_group7 trap_action copy trap_priority 7 queue 7 "meter_type" "bytes" "mode" "tr_tcm" "cir" "4096000" "cbs" "64000" "pir" "8192000" "pbs" "128000" "red_action" "drop"

2、let arp bind the new group
HSET COPP_TRAP|arp trap_group queue7_group7

3、arp only appears in queue7_group7
127.0.0.1:6379> hgetall "COPP_TABLE:queue5_group1"

  1. "cbs"
  2. "64000"
  3. "cir"
  4. "4096000"
  5. "meter_type"
  6. "bytes"
  7. "mode"
  8. "tr_tcm"
  9. "pbs"
  10. "128000"
  11. "pir"
  12. "8192000"
  13. "queue"
  14. "5"
  15. "red_action"
  16. "drop"
  17. "trap_action"
  18. "copy"
  19. "trap_priority"
  20. "5"
    127.0.0.1:6379> hgetall "COPP_TABLE:queue7_group7"
  21. "trap_action"
  22. "copy"
  23. "trap_priority"
  24. "7"
  25. "queue"
  26. "7"
  27. "meter_type"
  28. "bytes"
  29. "mode"
  30. "tr_tcm"
  31. "cir"
  32. "1024000"
  33. "cbs"
  34. "64000"
  35. "pir"
  36. "2048000"
  37. "pbs"
  38. "128000"
  39. "red_action"
  40. "drop"
  41. "trap_ids"
  42. "arp_req,arp_resp,neigh_discovery"

Details if related

… appear in appdb.

Signed-off-by: qiuyupeng <qiuyupeng@git.asterfusion.com>
@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@@ -654,9 +654,6 @@ void CoppMgr::doCoppTrapTask(Consumer &consumer)
m_coppTrapConfMap[key].trap_ids);
}

m_coppTrapConfMap[key].trap_group = trap_group;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing this will affect the logic inside addTrap since it is used to determine isTrapIdDisabled. I recommend instead to save the old trap group in a different variable and use it to compare below. You can remove the assignment at the end of the if loop.
Please also add UT to cover this case

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.

None yet

2 participants