[Nexthop] [fboss2] don't stage a config session from read-only commands - #1474
Open
hillol-nexthop wants to merge 1 commit into
Open
[Nexthop] [fboss2] don't stage a config session from read-only commands#1474hillol-nexthop wants to merge 1 commit into
hillol-nexthop wants to merge 1 commit into
Conversation
Signed-off-by: vybhav-nexthop <vybhav@nexthop.ai>
Contributor
|
This pull request has been imported. If you are a Meta employee, you can view this in D115030926. (Because this pull request was imported automatically, there will not be any future comments.) |
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 runSlack thread
Summary
fboss2-dev config historyandfboss2-dev config session diffare purely informational, but both create a config session as a side effect.Both call
ConfigSession::getInstance(), which lazily default-constructs the singleton, whose constructor unconditionally runsinitializeSession(). With nothing staged, that takes the "starting a new session" branch:ensureDirectoryExists(~/.fboss2),copySystemConfigToSession()(seeding~/.fboss2/agent.conffrom the live/etc/coop/agent.conf),base_ = git HEAD, andsaveMetadata()(writing~/.fboss2/cli_metadata.json).Consequences:
config historyon a clean box silently starts a config session.config session diff/commitafterwards behave as if the user has an edit in flight.session diff's own"No config session exists. Make a config change first."message is checked aftergetInstance()has already created the session — the message is false by the time it is printed.Fix
Add a
ConfigSession::SessionInit { CreateIfAbsent, ReadOnly }mode, threaded through the constructors intoinitializeSession():ReadOnlyreturns before the "start a new session" branch, so nothing under~/.fboss2is written. The git handle, path getters,configDomains()andreadStagedContent()all still work, which is everything these two commands actually use.CreateIfAbsentremains the default, so every editing command is byte-for-byte unchanged.loadConfig()— the entry point of every editing command — now callsinitializeSession(CreateIfAbsent)explicitly, so even aReadOnly-constructed singleton materializes a session the moment someone genuinely reads the agent config.CmdConfigHistoryandCmdConfigSessionDiffpassSessionInit::ReadOnly.Two deliberate choices worth reviewing:
initializeGit()still runs inReadOnlymode. It only bootstraps the system config repo under/etc/coop(never~/.fboss2), it is idempotent once the repo exists, and both read commands need a usable repo to report anything at all. Skipping it would makehistoryfail with a raw git error on a fresh device instead of showing the baseline commit. Happy to tighten this further if reviewers prefer.~/.fboss2is now ensured insaveConfig()andsaveMetadata()rather than only at construction, since the directory is no longer guaranteed to have been created up front. This also removes a pre-existing latent failure in therollback()path, which callssaveMetadata()unconditionally.A "delete the files on exit" approach was considered and rejected: it races against a concurrent
config session start/edit in another terminal, and does a pointless copy-then-delete on every invocation. Not creating the file at all is strictly better.Test Plan
New unit tests (all use a new
CmdConfigTestBase::setupReadOnlyTestableConfigSession()helper that installs aSessionInit::ReadOnlysession, mirroring how the commands construct theirs):CmdConfigHistoryTest.cpphistoryDoesNotCreateSessionFiles— asserts~/.fboss2does not exist beforehand, runs the command, asserts the output still listsInitial commit, and thatagent.conf,cli_metadata.json,bgp_config.jsonandhasActiveSession()are all still absent/false.historyLeavesExistingSessionUntouched— with a session already staged, the command still works and the staged content is byte-identical afterwards.defaultSessionStillCreatesSessionFile— regression guard that the defaultCreateIfAbsentediting path still seeds~/.fboss2/agent.conf.CmdConfigSessionDiffTest.cppdiffNoSessionDoesNotCreateSessionFiles— the "No config session exists" message is now true when printed; no session files created.diffTwoRevisionsDoesNotCreateSessionFiles— the revision-vs-revision mode never consults the session, and no longer stages one.diffReadOnlyStillSeesStagedSession— a real staged session is still diffed correctly and left untouched.Existing
CmdConfigHistoryTest/CmdConfigSessionDiffTest/CmdConfigSessionTest/ConfigSessionSystemdTestcases exercise the unchangedCreateIfAbsentpath.