[Nexthop][fboss2-dev] Replace file-based config versioning with Git#825
Closed
benoit-nexthop wants to merge 1 commit intofacebook:mainfrom
Closed
[Nexthop][fboss2-dev] Replace file-based config versioning with Git#825benoit-nexthop wants to merge 1 commit intofacebook:mainfrom
benoit-nexthop wants to merge 1 commit intofacebook:mainfrom
Conversation
2 tasks
benoit-nexthop
force-pushed
the
fboss2-cli-prototype_part15
branch
5 times, most recently
from
c468783 to
81eaba1
Compare
2 tasks
benoit-nexthop
force-pushed
the
fboss2-cli-prototype_part15
branch
2 times, most recently
from
3c9d00a to
af5398a
Compare
meta-codesync bot
pushed a commit
that referenced
this pull request
Feb 20, 2026
Summary: **Pre-submission checklist** - [x] I've ran the linters locally and fixed lint errors related to the files I modified in this PR. You can install the linters by running `pip install -r requirements-dev.txt && pre-commit install` - [x] `pre-commit run` Now every config command is saved in the CLI session metadata so we can easily tell what commands were used in a given session. The metadata is now also saved along the config when we commit the session. A future commit will make rollback also rely on this metadata to decide whether or not to restart the agent. Note: this change is part of a series, the previous one is #805, the next one is #825. Pull Request resolved: #809 Test Plan: New unit tests added. ## Sample usage ``` [admin@fboss101 ~]$ ~/benoit/fboss2-dev config interface eth1/1/1 description this is a test. Successfully set description for interface(s) eth1/1/1 [admin@fboss101 ~]$ ~/benoit/fboss2-dev config interface eth1/1/1 mtu 1500 Successfully set MTU for interface(s) eth1/1/1 to 1500 [admin@fboss101 ~]$ ls -l ~/.fboss2/ total 216 -rw-r--r-- 1 admin admin 213174 Jan 13 13:00 agent.conf -rw-r--r-- 1 admin admin 141 Jan 13 13:00 conf_metadata.json [admin@fboss101 ~]$ cat ~/.fboss2/conf_metadata.json { "action": {}, "commands": [ "config interface eth1/1/1 description this is a test.", "config interface eth1/1/1 mtu 1500" ] } [admin@fboss101 ~]$ ~/benoit/fboss2-dev config session commit Config session committed successfully as r6 and config reloaded. [admin@fboss101 ~]$ ls -l /etc/coop/cli/ total 1292 -rw-r--r-- 1 root root 213185 Jan 13 12:52 agent-r1.conf -rw-r--r-- 1 admin admin 213194 Jan 13 12:56 agent-r2.conf -rw-r--r-- 1 admin admin 108 Jan 13 12:56 agent-r2.metadata.json -rw-r--r-- 1 admin admin 213185 Jan 13 12:56 agent-r3.conf -rw-r--r-- 1 admin admin 99 Jan 13 12:56 agent-r3.metadata.json -rw-r--r-- 1 admin admin 213185 Jan 13 12:58 agent-r4.conf -rw-r--r-- 1 admin admin 80 Jan 13 12:58 agent-r4.metadata.json -rw-r--r-- 1 admin admin 213185 Jan 13 12:58 agent-r5.conf -rw-r--r-- 1 admin admin 80 Jan 13 12:58 agent-r5.metadata.json -rw-r--r-- 1 admin admin 213174 Jan 13 13:00 agent-r6.conf -rw-r--r-- 1 admin admin 141 Jan 13 13:00 agent-r6.metadata.json [admin@fboss101 ~]$ cat /etc/coop/cli/agent-r6.metadata.json { "action": {}, "commands": [ "config interface eth1/1/1 description this is a test.", "config interface eth1/1/1 mtu 1500" ] } ``` Reviewed By: KevinYakar Differential Revision: D93669865 Pulled By: joseph5wu fbshipit-source-id: 9c37dfd88a59d8e76bdb09f30d0c55897c4e889f
benoit-nexthop
force-pushed
the
fboss2-cli-prototype_part15
branch
from
af5398a to
d4f6898
Compare
Replace the basic file-based configuration versioning mechanism with Git-based versioning for the CLI config session. Key changes: - Add new Git class (Git.h/cpp) providing a simple interface for Git operations: init, commit, log, show, resolveRef, getHead, hasCommits - Use folly::Subprocess with full path /usr/bin/git for all Git commands - Replace revision files (agent-rN.conf + symlink) with atomic writes to agent.conf tracked in a local Git repository - Use Git commit SHAs as revision identifiers instead of rN format - Update RevisionList validation to accept Git SHAs (7+ hex chars) Repository initialization: - Automatically initialize Git repo if it doesn't exist - Automatically create initial commit if repo has no commits but config file exists - Use --shared=group flag and umask 0002 to ensure .git directory is group-writable when /etc/coop is group-writable Commands updated: - config history: Shows Git commit log with SHA, author, timestamp, message - config session diff: Uses git show to compare commits - config session commit: Creates Git commits with username as author - config rollback: Reads config from Git history and creates new commit Test updates: - Update all CLI config tests to use Git-based setup - Initialize Git repo and create initial commit in test fixtures
benoit-nexthop
force-pushed
the
fboss2-cli-prototype_part15
branch
from
d4f6898 to
b8b37f2
Compare
|
@joseph5wu has imported this pull request. If you are a Meta employee, you can view this in D94975358. |
joseph5wu
reviewed
Mar 3, 2026
Contributor
joseph5wu
left a comment
joseph5wu
left a comment
There was a problem hiding this comment.
There're some improvement comments about how to properly manage the full sha vs short sha, to avoid mis-using them in your code. Please consider to improve the logic in the future PRs.
I'll land this PR for now to unblock your rebase effort
|
|
||
| // Build message based on what actions were taken | ||
| std::string message; | ||
| std::string shortSha = result.commitSha.substr(0, 7); |
Contributor
There was a problem hiding this comment.
Would like to see a common utility function for getShortSha(const std::string& commitSha).
Or a more organized way to getFullSha() or getShortSha() from the result.
This will avoid mis-using the wrong sha string.
Could be addressed in future diff
Contributor
Author
There was a problem hiding this comment.
Just to be clear, you can always use the long or the short sha1, as long as it's a unique prefix (just like when using git directly). But other than that, yes, we can add a little helper function to clip the string.
| throw std::runtime_error( | ||
| "Failed to read current config from " + cliConfigPath); | ||
| } | ||
| return {content, "current live config"}; |
Contributor
There was a problem hiding this comment.
Will it be a problem that you don't give a shortSha of the current revision here?
In line 42, you always return shortSha revision for non-current case.
Contributor
Author
There was a problem hiding this comment.
You can pass anything that git will accept: short sha1, full sha1, branch or tag name, any other ref...
Comment on lines
+41
to
+42
| std::string content = git.fileAtRevision(resolvedSha, "cli/agent.conf"); | ||
| return {content, revision.substr(0, 8)}; |
Contributor
There was a problem hiding this comment.
Now I think your Git can have to provide a getShortSha()
Contributor
Author
There was a problem hiding this comment.
Yes it's inconsistent with the other file, good catch. It doesn't matter in practice but I agree it would be better to be consistent. We can fix this in this PR if you want, it's such a small change it wouldn't hurt anything.
|
@joseph5wu merged this pull request in 307355c. |
meta-codesync bot
pushed a commit
that referenced
this pull request
Mar 5, 2026
…esolution (#832) Summary: **Pre-submission checklist** - [x] I've ran the linters locally and fixed lint errors related to the files I modified in this PR. You can install the linters by running `pip install -r requirements-dev.txt && pre-commit install` - [x] `pre-commit run` When multiple users have concurrent config sessions, the first user to commit succeeds, but subsequent users get an error because their session is based on a stale commit. Previously, whoever committed last would overwrite changes of other users that committed before them. This commit adds a `config session rebase` command that: 1. Takes the diff between the config as of the base commit and the session 2. Re-applies this diff on top of the current HEAD (similar to git rebase) 3. Uses a recursive 3-way merge algorithm for JSON objects 4. Detects and reports conflicts at specific JSON paths 5. Updates the session's base to the current HEAD on success The 3-way merge algorithm handles: - Objects: recursively merges each key - Arrays: element-by-element merge if sizes match - Scalars: conflict if both head and session changed differently from base Note: this change is part of a series, the previous one is #825, the next one is #837. Pull Request resolved: #832 Test Plan: Add unit tests covering: - Successful rebase with non-conflicting changes - Rebase failure when changes conflict - Rebase not needed when session is already up-to-date ## Sample usage TBD Reviewed By: srikrishnagopu Differential Revision: D95099578 Pulled By: joseph5wu fbshipit-source-id: b665afa35a43829981ce9dd8434abf5b3942882a
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
3 participants
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
Replace the basic file-based configuration versioning mechanism with Git-based versioning for the CLI config session.
Key changes:
Gitclass providing a simple interface for Git operations: init, commit, log, show, etcfolly::Subprocesswith full path/usr/bin/gitfor all Git commandsagent-rN.conf+ symlink) with atomic writes to agent.conf tracked in a local Git repositoryRepository initialization:
--shared=groupflag and umask 0002 to ensure.gitdirectory is group-writable when/etc/coopis group-writableCommands updated:
Note: this change is part of a series, the previous one is #809, the next one is #832.
Test Plan
Test updates:
Sample usage
Simple change and session commit:
Rollback flow: