[Nexthop] Fix getting SDK state dump in CLI - #1611
Open
vvasavada-nexthop wants to merge 1 commit into
Open
vvasavada-nexthop wants to merge 1 commit into
vvasavada-nexthop wants to merge 1 commit into
Conversation
Signed-off-by: Vishrant Vasavada <vvasavada@nexthop.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pre-submission checklist
pip install -r requirements-dev.txt && pre-commit installpre-commit runSummary
fboss2 show sdk dumpwas broken byqsfp_serviceSDK-dump path-traversal guard added inDifferential Revision: D109336251: the CLI sent an absolute temp-file path which the guard rejected withgetSdkState: absolute fileName is not allowed.This PR fixes the CLI to work with the guard, ensures the service-owned dump directory exists, stops the agent from leaking a scratch directory per debug-dump, and surfaces empty agent SDK dumps to the user.Changes
show sdk dumpnow sends a plain basename instead of absolute temp path, and reads the result back from the service-owned dir. This clears theabsolute fileName is not allowederror.show sdk agentnow warns when the returned SDK state is empty instead of silently printing nothing.qsfp_service:getSdkStatenow callscreateDir(kSdkDumpDir)so the confined dump directory exists before SDK writes into it.getDebugDump()now usesfolly::test::TemporaryDirectory::Scope::DELETE_ON_DESTRUCTION, so thefboss_sdk_dump.<token>scratch dir is removed after the dump is read back - previously one dir leaked per invocation.CredoMacsecUtil.cpp: updated thegetSdkStatecaller to send a basename.Test Plan
SdkDumpPathTest(path sanitizer) andCmdShowSdkDumpTest(empty-dump warnings)Thrift.getHwDebugDumpextended to assert the scratch dir does not leak.The unit tests failed without the fixes, passed with the fixes.
DUT verification
Before
After
Note that residual
Getting SDK state failedis because of no xphy present on the box. The path error is gone and it can be verified from qsfp side.BEFORE (unpatched
fboss_hw_agent-sai_impl):AFTER (patched
fboss_hw_agent-sai_impl):