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

[pbh]: Fix show PBH counters when cache is partial #3356

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

nazariig
Copy link
Collaborator

@nazariig nazariig commented Jun 6, 2024

Signed-off-by: Nazarii Hnydyn [email protected]

During system init the initial snapshot of PBH rule counters doesn't contain all the necessary information, which makes a dump invalid. Thus, it fails CLI to do a comparison logic which prints a traceback. After some time, when counters are getting updated in the DB, the snapshot can be taken correctly and the issue is gone.

What I did

  • Fixed PBH CLI output when counter's cache is partial

How I did it

  • Added additional constraint

How to verify it

  1. Reboot the switch
  2. Run sonic-clear pbh statistics
  3. Run show pbh statistics

Previous command output (if the output of a command-line utility has changed)

root@sonic:/home/admin# show pbh statistics
Traceback (most recent call last):
  File "/usr/local/bin/show", line 8, in <module>
    sys.exit(cli())
  File "/usr/local/lib/python3.9/dist-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python3.9/dist-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/usr/local/lib/python3.9/dist-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.9/dist-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.9/dist-packages/click/core.py", line 1114, in invoke
    return Command.invoke(self, ctx)
  File "/usr/local/lib/python3.9/dist-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/local/lib/python3.9/dist-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/usr/local/lib/python3.9/dist-packages/click/decorators.py", line 64, in new_func
    return ctx.invoke(f, obj, *args, **kwargs)
  File "/usr/local/lib/python3.9/dist-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/usr/local/lib/python3.9/dist-packages/show/plugins/pbh.py", line 386, in PBH_STATISTICS
    get_counter_value(pbh_counters, saved_pbh_counters, key, COUNTER_PACKETS_ATTR),
  File "/usr/local/lib/python3.9/dist-packages/show/plugins/pbh.py", line 401, in get_counter_value
    new_value = int(pbh_counters[key][type]) - int(saved_pbh_counters[key][type])
KeyError: 'SAI_ACL_COUNTER_ATTR_PACKETS'

New command output (if the output of a command-line utility has changed)

root@sonic:/home/admin# show pbh statistics
TABLE      RULE             RX PACKETS COUNT    RX BYTES COUNT
---------  ---------------  ------------------  ----------------
pbh_table  nvgre_ipv4_ipv4  0                   0
pbh_table  nvgre_ipv4_ipv6  0                   0
pbh_table  nvgre_ipv6_ipv4  0                   0
pbh_table  nvgre_ipv6_ipv6  0                   0
pbh_table  vxlan_ipv4_ipv4  0                   0
pbh_table  vxlan_ipv4_ipv6  0                   0
pbh_table  vxlan_ipv6_ipv4  0                   0
pbh_table  vxlan_ipv6_ipv6  0                   0

@nazariig
Copy link
Collaborator Author

@qiluo-msft / @yxieca please help to review & merge

@nazariig nazariig requested review from qiluo-msft and yxieca June 12, 2024 13:49
@liat-grozovik
Copy link
Collaborator

@nazariig you mean that while the cache is not fully available the printed value will be 0?

@nazariig
Copy link
Collaborator Author

nazariig commented Jun 17, 2024

@nazariig you mean that while the cache is not fully available the printed value will be 0?

@liat-grozovik no. If no cache - print the actual counter value. This is a simple fix

yxieca
yxieca previously approved these changes Jun 20, 2024
@yxieca yxieca self-requested a review June 20, 2024 17:31
@yxieca
Copy link
Contributor

yxieca commented Jun 20, 2024

@nazariig the change looks in the good direction. However, from the description, I am not 100% sure if there could be a race condition where the record is not none and still only have partial information? Your current change only protected against the record is none scenario.

Can you add a unit test?

I am okay with moving forward with unit test since it is protecting a valid scenario.

@nazariig
Copy link
Collaborator Author

nazariig commented Jun 27, 2024

@nazariig the change looks in the good direction. However, from the description, I am not 100% sure if there could be a race condition where the record is not none and still only have partial information? Your current change only protected against the record is none scenario.

Can you add a unit test?

I am okay with moving forward with unit test since it is protecting a valid scenario.

@yxieca done. Internal key partial information should be considered as a bug - traceback is expected

Signed-off-by: Nazarii Hnydyn <[email protected]>
@yxieca yxieca merged commit 667a150 into sonic-net:master Jun 27, 2024
7 checks passed
mssonicbld pushed a commit to mssonicbld/sonic-utilities that referenced this pull request Jun 27, 2024
* [pbh]: Fix show PBH counters when cache is partial.

Signed-off-by: Nazarii Hnydyn <[email protected]>
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202311: #3388

mssonicbld pushed a commit that referenced this pull request Jun 27, 2024
* [pbh]: Fix show PBH counters when cache is partial.

Signed-off-by: Nazarii Hnydyn <[email protected]>
mssonicbld pushed a commit to mssonicbld/sonic-utilities that referenced this pull request Aug 2, 2024
* [pbh]: Fix show PBH counters when cache is partial.

Signed-off-by: Nazarii Hnydyn <[email protected]>
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202405: #3466

mssonicbld pushed a commit that referenced this pull request Aug 2, 2024
* [pbh]: Fix show PBH counters when cache is partial.

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

Successfully merging this pull request may close these issues.

6 participants