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

[macsec]: test macsec counters over rekey, and clear command #15161

Conversation

liamkearney-msft
Copy link
Contributor

@liamkearney-msft liamkearney-msft commented Oct 25, 2024

  • test macsec counters over rekey
  • use show command for counters
  • test sonic-clear macsec

Summary:
Fixes #13347

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • [ x] Test case(new/improvement)

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • [x ] 202405

Approach

What is the motivation for this PR?

Fix test gap by adding sonic-clear macsec tests and testing over rekeys

How did you verify/test it?

Ran on macsec t2 chassis, using profiles with rekeys and without
https://elastictest.org/scheduler/testplan/6720229a42c9736d8d478924

@liamkearney-msft liamkearney-msft self-assigned this Oct 25, 2024
@liamkearney-msft liamkearney-msft force-pushed the pub-dev/macsec-counter-clear-rekey branch from f7b6067 to 80412a0 Compare October 25, 2024 03:25
@liamkearney-msft
Copy link
Contributor Author

Note that the test across rekey is a known failure - see : sonic-net/sonic-buildimage#19311


def get_macsec_counters(sonic_asic, port):
cmd = f"show macsec {port}"
output = sonic_asic.sonichost.command(cmd)["stdout"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This command is support at the Linecard level. We can get the output from duthost?

Copy link
Contributor

@arlakshm arlakshm left a comment

Choose a reason for hiding this comment

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

changes lgtm. couple of minor comments

ingress_counters = Counter()
for up_port in up_ports:

asic = duthost.get_port_asic_instance(up_port)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do not have get te asic instance here the. show macsec works on the linecard level

@liamkearney-msft liamkearney-msft force-pushed the pub-dev/macsec-counter-clear-rekey branch from 80412a0 to 8958759 Compare October 28, 2024 00:06
@liamkearney-msft
Copy link
Contributor Author

@arlakshm I've addressed your comments, and also updated macsec setup to use config commands rather than poking the configdb directly. Would appreciate a re-review

* test macsec counters over rekey
* use show command for counters
* use config instead of hitting configdb directly
* test macsec counters clear

Signed-off-by: Liam Kearney <[email protected]>
@liamkearney-msft liamkearney-msft force-pushed the pub-dev/macsec-counter-clear-rekey branch from 8958759 to 887d7d2 Compare October 28, 2024 02:11
@liamkearney-msft
Copy link
Contributor Author

liamkearney-msft commented Oct 28, 2024

pipeline is failing due to a bug in config macsec command - If a port has macsec config deleted without it existing, the command crashes with the following backtrace:

Traceback (most recent call last):
  File "/usr/local/bin/config", line 8, in <module>
    sys.exit(config())
  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 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_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/config/plugins/macsec.py", line 87, in del_port
    del port_entry['macsec']
KeyError: 'macsec'

Ticket in buildimage for this : sonic-net/sonic-buildimage#20631

Signed-off-by: Liam Kearney <[email protected]>
(it is bad practice to assert in fixtures)

Signed-off-by: Liam Kearney <[email protected]>
@liamkearney-msft
Copy link
Contributor Author

Ive reverted the change to use config - will address in future PR

Signed-off-by: Liam Kearney <[email protected]>
@judyjoseph
Copy link
Contributor

@liamkearney-msft I feel it is good to have a separate test : test_clear_counters : than modifying the existing counter test.

Additionally, as a first step let us add the test to do clear counters functionality without rekey. We have a Brcm CSP still open (sonic-net/sonic-buildimage#19311) to fix the clear counters after rekey. Will enhance the test_clear_counters for rekey once this fix is merged.

@liamkearney-msft
Copy link
Contributor Author

@liamkearney-msft I feel it is good to have a separate test : test_clear_counters : than modifying the existing counter test.

Additionally, as a first step let us add the test to do clear counters functionality without rekey. We have a Brcm CSP still open (sonic-net/sonic-buildimage#19311) to fix the clear counters after rekey. Will enhance the test_clear_counters for rekey once this fix is merged.

Sure. Will close this and reopen with "just counters", and another with "test across rekey"

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.

[Test Gap] Test to check sonic-clear macsec
3 participants