Skip to content

[Nexthop][fboss2-dev] Add a simple fboss2 config reload command. #753

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.

Sign up for GitHub

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

Jump to bottom
Closed
benoit-nexthop wants to merge 1 commit into from
Closed

[Nexthop][fboss2-dev] Add a simple fboss2 config reload command. #753

benoit-nexthop wants to merge 1 commit into from
+275 −1

Conversation

@benoit-nexthop
Copy link
Contributor

@benoit-nexthop benoit-nexthop commented Dec 18, 2025 •
edited
Loading

Pre-submission checklist

  • 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
  • pre-commit run

Summary

This command just calls reloadConfig() on the wedge_agent.

Note: this change is part of a series, the next one is #754.

Test Plan

Unit tests.

@meta-cla meta-cla bot added the CLA Signed label Dec 18, 2025
@benoit-nexthop benoit-nexthop marked this pull request as ready for review December 18, 2025 00:33
@benoit-nexthop benoit-nexthop force-pushed the fboss2-cli-prototype_part01 branch from e2649b7 to fbb53a7 Compare December 19, 2025 23:31
@meta-codesync
Copy link

meta-codesync bot commented Dec 22, 2025

@joseph5wu has imported this pull request. If you are a Meta employee, you can view this in D89698241.

joseph5wu
joseph5wu approved these changes Dec 22, 2025
View reviewed changes
Copy link
Contributor

@joseph5wu joseph5wu left a comment

Choose a reason for hiding this comment

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

Overall this looks pretty clean to me and ready to land.
My suggestion about supporting reloading other config can be addressed in the future

fboss/cli/fboss2/commands/config/CmdConfigReload.cpp
Comment on lines +17 to +19
auto client =
utils::createClient<facebook::fboss::FbossCtrlAsyncClient>(hostInfo);

Copy link
Contributor

@joseph5wu joseph5wu Dec 22, 2025

Choose a reason for hiding this comment

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

Right now this config can only reload agent config.
But eventually if we need to reload qsfp/platform/bgp configs in the future,
we might have to support an option to reload specific service config.

For now, I'm okay to land this but please allow flexibility to load specific config in your future diff

Copy link
Contributor Author

@benoit-nexthop benoit-nexthop Dec 23, 2025

Choose a reason for hiding this comment

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

Yeah we can easily add a subcommand later that specifies what agent to reload specifically.

@joseph5wu
Copy link
Contributor

joseph5wu commented Dec 23, 2025

@benoit-nexthop : We've discussed before the config cli should only be used by fboss2-dev while fboss2 should not support any config related cli for now.
But this PR will leak the config reload function to fboss2, which will cause issue for our production fboss2.
Can you address this, please

❯❯❯ ~/fbsource/buck-out/v2/gen/fbcode/fboss/cli/fboss2/fboss2 config reload --help
Reload agent configuration
Usage: /home/joseph5wu/fbsource/buck-out/v2/gen/fbcode/fboss/cli/fboss2/fboss2 config reload [OPTIONS]

Options:
  -h,--help                   Print this help message and exit

/data/users/joseph5wu/fbsource/fbcode (947ac67dc4)
❯❯❯ ~/fbsource/buck-out/v2/gen/fbcode/fboss/cli/fboss2/fboss2 config reload
Failed to reload config: (CONNECT_UNKNOWN) Connect Error to [0000:0000:0000:0000:0000:0000:0000:0001]:5909 for tier ''. AsyncSocketException: recv() failed (peer=[::1]:5909, local=[::1]:45332), type = Internal error, errno = 111 (Connection refused): Connection refused: Connection refused -- For help debugging visit https://fburl.com/wiki/sr_connect_unknown

@benoit-nexthop
Copy link
Contributor Author

benoit-nexthop commented Dec 23, 2025

@benoit-nexthop : We've discussed before the config cli should only be used by fboss2-dev while fboss2 should not support any config related cli for now. But this PR will leak the config reload function to fboss2, which will cause issue for our production fboss2. Can you address this, please

As I mentioned last week, it's done in #762 but if you need it moved earlier in the stack, it's doable, it's just not gonna be easy. Let me know.

@benoit-nexthop
Add a simple fboss2 config reload command.
72a47d6 
This command just calls `reloadConfig()` on the wedge_agent.
@benoit-nexthop benoit-nexthop force-pushed the fboss2-cli-prototype_part01 branch from fbb53a7 to 72a47d6 Compare December 23, 2025 21:37
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Dec 23, 2025

@benoit-nexthop has updated the pull request. You must reimport the pull request before landing.

@benoit-nexthop
Copy link
Contributor Author

benoit-nexthop commented Dec 23, 2025

@benoit-nexthop : We've discussed before the config cli should only be used by fboss2-dev while fboss2 should not support any config related cli for now. But this PR will leak the config reload function to fboss2, which will cause issue for our production fboss2. Can you address this, please

As I mentioned last week, it's done in #762 but if you need it moved earlier in the stack, it's doable, it's just not gonna be easy. Let me know.

Done. PTAL.

This was referenced Dec 23, 2025
Closed
Closed
Closed
@meta-codesync meta-codesync bot closed this in 71d5072 Dec 24, 2025
@meta-codesync
Copy link

meta-codesync bot commented Dec 24, 2025

@joseph5wu merged this pull request in 71d5072.

@benoit-nexthop benoit-nexthop deleted the fboss2-cli-prototype_part01 branch December 25, 2025 21:55
meta-codesync bot pushed a commit that referenced this pull request Dec 29, 2025
@benoit-nexthop @meta-codesync
Add fboss2 config applied-info. (#754)
bacfe12 
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`

Sample output:
```
Config Applied Information:
===========================
Last Applied Time: 2025-10-11 09:29:36.589
Last Coldboot Applied Time: 2025-10-11 06:44:36.741
```

Note: this change is part of a series, the previous one is #753, the next one is #755.

Pull Request resolved: #754

Test Plan: Unit tests.

Reviewed By: KevinYakar

Differential Revision: D89894234

Pulled By: joseph5wu

fbshipit-source-id: 97c71bc011882b0cb19b5e9d947c50d8bc3edef2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joseph5wu joseph5wu joseph5wu approved these changes

Assignees

@joseph5wu joseph5wu

Labels

CLA Signed Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@benoit-nexthop @joseph5wu @facebook-github-bot