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

[crmorch]: neighbor used counter increased twice #472

Merged
merged 2 commits into from
Apr 17, 2018

Conversation

tylerlinp
Copy link
Contributor

What I did
Modified crmorch stats for neighbor used counter. Increase it just after create neighbor entry success. Decrease it when remove neighbor entry success in case of add nexthop failure.

Why I did it
Test found bug.

How I verified it
By code walkthrough I found neighbor used counter increased both before and after add nexthop.

Details if related
before add ipv4 neighbor,the value of crm_stats_ipv4_neighbor_used is 8
3) "crm_stats_ipv4_neighbor_used"
4) "8"
after add one ipv4 neighbor,the value of crm_stats_ipv4_neighbor_used is 10
3) "crm_stats_ipv4_neighbor_used"
4) "10"

@msftclas
Copy link

msftclas commented Apr 16, 2018

CLA assistant check
All CLA requirements met.

@lguohan
Copy link
Contributor

lguohan commented Apr 16, 2018

can you sign the cla?


if (neighbor_entry.ip_address.addr_family == SAI_IP_ADDR_FAMILY_IPV4)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like a previous review comment on this got missed half-way.
Suggest to keep this here and remove the increment counter above addNextHop. You need not decrement the counter if addNextHop failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought same as you said at the beginning, but then found when failed both addNextHop and remove_neighbor_entry we should add increment there, that is less readability.

@prsunny
Copy link
Collaborator

prsunny commented Apr 17, 2018

We have a testcase to verify the used counter but since the check was ">=", this bug was not caught.
sonic-mgmt/ansible/roles/test/tasks/crm/crm_test_ipv4_neighbor.yml

    - name: Verify "crm_stats_ipv4_neighbor_used" counter was incremented
      assert: {that: "{{new_crm_stats_ipv4_neighbor_used|int - crm_stats_ipv4_neighbor_used|int >= 1}}"}

@lguohan
Copy link
Contributor

lguohan commented Apr 17, 2018

why is this bug not caught by the virtual switch test? Is it possible to add virtual switch to catch this issue?

@prsunny
Copy link
Collaborator

prsunny commented Apr 17, 2018

@lguohan, VS test also checks >=. I'll fix the test cases.

@prsunny prsunny merged commit 57c9425 into sonic-net:master Apr 17, 2018
MaxPolovyi pushed a commit to MaxPolovyi/sonic-swss that referenced this pull request May 10, 2018
* [crmorch]: neighbor used counter increased twice
prsunny pushed a commit that referenced this pull request May 14, 2018
* [crmorch]: neighbor used counter increased twice
praveen-li pushed a commit to praveen-li/sonic-swss that referenced this pull request Aug 24, 2020
* [crmorch]: neighbor used counter increased twice
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
* [sonic-utilities] managementVRF cli support(l3mdev)

This commit adds CLI support for management VRF using l3dev. mVRF can
be enabled using config vrf add mgmt and deleted using config vrf del mgmt.
Show commands for management VRF are added which displays the linux command
output, will update show command display after concluding what would be the
output for the show commands.
Added cli to configure management interface(eth0), config interface ip eth0
add can be used to configure eth0 ip and config ip eth0 remove is used to
remove eth0 ip.

New cli config/show commands:

config vrf add mgmt
config vrf del mgmt
config interface eth0 ip add ip/mask gatewayIP
config interface eth0 ip remove ip/mask

show mgmt-vrf
show mgmt-vrf route
show mgmt-vrf addresses
show mgmt-vrf interfaces

Signed-off-by: Harish Venkatraman <harish_venkatraman@dell.com>
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants