Skip to content

[macsec] Add dirty-restart SAK regression test - #24903

Merged
abdosi merged 1 commit into
sonic-net:masterfrom
nexthop-ai:macsec-add-dirty-restart-sak-regression
Jul 22, 2026
Merged

[macsec] Add dirty-restart SAK regression test#24903
abdosi merged 1 commit into
sonic-net:masterfrom
nexthop-ai:macsec-add-dirty-restart-sak-regression

Conversation

@senthil-nexthop

Copy link
Copy Markdown
Contributor

Description of PR

Add a new dirty-restart regression test for MACsec that catches the stale-SAK class of bug (docker kill -s 9 macsec → MKA renegotiates → orchagent silently keeps the prior cycle's SAK in SAI).

The existing tests/macsec/test_docker_restart.py (graceful systemctl restart macsec smoke test) is untouched — its historical pass/fail data continues to flow into the test-results system unchanged.

Companion fix: stale-sak (recover from stale-SAK after dirty macsec docker restart).

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • New Test case
    • Skipped for non-supported platforms
  • Test case improvement

Back port request

  • 202311
  • 202405
  • 202411
  • 202505
  • 202511
  • 202512
  • 202605

Approach

What is the motivation for this PR?

The existing test_docker_restart is a graceful systemctl restart — macsecmgrd gets to disable MACsec on each port before the container goes down, so re-keying after restart works fine. The bug class fixed in stale-sak only triggers when the container dies without the graceful per-port teardown — e.g. docker kill -s 9, OOM-kill, host reset, panic. A separate "dirty" test exercises that path.

How did you do it?

Lives in a new tests/macsec/test_macsec_recovery.py, with shared disrupt / wait / snapshot / assert helpers in a new tests/common/macsec/recovery_helpers.py.

How did you verify/test it?

  • All 9 cipher_suite parametrizations of test_dirty_container_kill_preserves_sak_consistency PASSED: 128, 128_SCI, 128_XPN, 128_XPN_SCI, 256, 256_SCI, 256_XPN, 256_XPN_SCI, MACSEC_PROFILE.
  • The same case run against a swss build with the fix reverted produced the expected assert_appl_db_sak_matches_show_macsec failure (APPL_DB advertised the renegotiated key while show macsec still showed the prior cycle's SAK on at least one port).
  • Static checks: python3 -m ast parse on both new files.

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld

Copy link
Copy Markdown
Collaborator

This PR has backport request for branch(es): 202511.
Added label(s) for branch(es) 202511.

---Powered by SONiC BuildBot

@senthil-nexthop
senthil-nexthop force-pushed the macsec-add-dirty-restart-sak-regression branch from ea05a97 to 256d33d Compare June 2, 2026 20:11
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@senthil-nexthop
senthil-nexthop force-pushed the macsec-add-dirty-restart-sak-regression branch from 256d33d to d5bf043 Compare June 2, 2026 21:44
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@abdosi

abdosi commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

@rawal01 : can you help review this ?

@rlhui
rlhui requested a review from tjchadaga June 10, 2026 17:19
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@deepak-singhal0408

Copy link
Copy Markdown
Contributor

@saksarav-nokia to review/sign-off.

Comment thread tests/macsec/test_macsec_recovery.py Outdated
that an advance-timeout error mid-test can't leak a non-zero rekey_period
into the next profile's run.
"""
duthost = rand_selected_dut

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tests use rand_selected_dut, but ctrl_links is built from macsec_duthost.

test_docker_restart.py uses the explicit pattern:
duthost = duthosts[enum_rand_one_per_hwsku_macsec_frontend_hostname]
So it is better to use macsec_duthost (or the same enum fixture) in both tests and in force_dut_key_server / macsec_loganalyzer_ignore, so kills and DB checks run on the DUT that actually owns the MACsec ctrl links.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Pushed a commit to address this, please review.

Comment thread tests/macsec/conftest.py Outdated
@senthil-nexthop
senthil-nexthop force-pushed the macsec-add-dirty-restart-sak-regression branch from fa30417 to 242a523 Compare July 8, 2026 09:10
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

@senthil-nexthop
senthil-nexthop force-pushed the macsec-add-dirty-restart-sak-regression branch from 242a523 to 60d3b4f Compare July 14, 2026 17:59
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@saravanan-nexthop

Copy link
Copy Markdown
Contributor

@saksarav-nokia please review the latest diffs.
Thank you

saksarav-nokia
saksarav-nokia previously approved these changes Jul 15, 2026
@senthil-nexthop
senthil-nexthop force-pushed the macsec-add-dirty-restart-sak-regression branch from 60d3b4f to 09310f2 Compare July 16, 2026 06:10
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@senthil-nexthop
senthil-nexthop force-pushed the macsec-add-dirty-restart-sak-regression branch from 09310f2 to 4016614 Compare July 17, 2026 12:47
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@senthil-nexthop
senthil-nexthop force-pushed the macsec-add-dirty-restart-sak-regression branch from 4016614 to 6a4ff4a Compare July 17, 2026 15:45
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Signed-off-by: senthil-nexthop <senthil@nexthop.ai>
@senthil-nexthop
senthil-nexthop force-pushed the macsec-add-dirty-restart-sak-regression branch from 6a4ff4a to 0ce91d0 Compare July 19, 2026 05:42
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@mssonicbld

Copy link
Copy Markdown
Collaborator

This PR has backport request label(s) for branch(es): 202511, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202511: <test result>) in the Test result section as well in your PR description.

---Powered by SONiC BuildBot

@abdosi
abdosi merged commit f335de7 into sonic-net:master Jul 22, 2026
26 checks passed
@mssonicbld

Copy link
Copy Markdown
Collaborator

This PR has backport request label(s) for branch(es): 202605, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202605: <test result>) in the Test result section as well in your PR description.

---Powered by SONiC BuildBot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P0 Request for 202511 branch Request to backport a change to 202511 branch Request for 202605 branch

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants