Skip to content

[Nexthop][fboss2-dev] Add fboss2-dev config hostname CLI#1343

Closed
vybhav-nexthop wants to merge 1 commit into
facebook:mainfrom
nexthop-ai:nos-7154-hostname-config
Closed

[Nexthop][fboss2-dev] Add fboss2-dev config hostname CLI#1343
vybhav-nexthop wants to merge 1 commit into
facebook:mainfrom
nexthop-ai:nos-7154-hostname-config

Conversation

@vybhav-nexthop

@vybhav-nexthop vybhav-nexthop commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

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

Adds fboss2-dev config switch hostname <name>, which sets the optional
sw.hostname field in the agent switch config.

The command is nested under the config switch command group (introduced by
the admin-distance command) rather than at the top level, grouping
switch-level settings that do not belong to a specific network object.

  • Action level: HITLESS — applied via
    ThriftConfigApplier::updateSwitchSettings (newSwitchSettings->setHostname);
    no SAI ChangeProhibited guard.
  • Upsert: if hostname is already set to the requested value, the
    command is a no-op.
  • Validation: the hostname argument is validated against RFC 952/1123
    using RE2 (alphanumeric plus hyphens, max 63 chars per label, total max
    253 chars).
  • Field: switch_config.thrift:SwitchConfig.hostname (optional string) —
    "Overrides the system hostname, useful in ICMP responses." When unset, the
    agent falls back to the OS hostname at config-apply time
    (getLocalHostname()), so the value is always populated in switch state.

Test Plan

Unit tests (CmdConfigHostname{,Absent}TestFixture)

9 tests, all passing in CI: argument validation (valid, FQDN, empty args,
empty string, too-many args, invalid format), set-new hostname, already-set
no-op, and set-when-field-absent.

Integration test (ConfigHostnameTest) — on DUT gold407

The integration test reads the current hostname from switch state
(getSwitchSettingsField, always populated) inside a try/catch that returns
"" if absent, sets a test value, verifies it via the switch-state thrift
path, and restores the original hostname in TearDown. It does not read the
optional config field directly, so it does not need any getSwConfigField
default-value helper — Fboss2IntegrationTest.h is unchanged from upstream.

Results from the internal CLI Integration Test (DUT) run on 2026-07-03
(device gold407):

[----------] 2 tests from ConfigHostnameTest
[ RUN      ] ConfigHostnameTest.SetAndVerifyHostname
  Current hostname: 'gold407'
  [Step 2] Setting hostname to 'cli-e2e-test-hostname'...
  Running CLI command: config switch hostname cli-e2e-test-hostname
  Verified: hostname is 'cli-e2e-test-hostname'
  Running CLI command: config switch hostname gold407      (restore)
[       OK ] ConfigHostnameTest.SetAndVerifyHostname (173 ms)
[ RUN      ] ConfigHostnameTest.IdempotentSet
  Running CLI command: config switch hostname cli-e2e-idempotent-hostname
  Running CLI command: config switch hostname cli-e2e-idempotent-hostname
  Running CLI command: config switch hostname gold407      (restore)
[       OK ] ConfigHostnameTest.IdempotentSet (120 ms)
[----------] 2 tests from ConfigHostnameTest (294 ms total)

Ran as part of the 25-test fboss2 CLI integration suite; all 25 passed
(total 50.5 s).

Review Findings

Pre-publication review (single-pass automated reviewer). One item raised —
unchecked *config.sw() dereference in CmdConfigHostname.cpp — reviewed
and dismissed: it is the established pattern across all fboss2 config
commands (e.g. CmdConfigArp.cpp, CmdConfigCopp.cpp); sw is reliably
present for a loaded agent config. No issues above threshold.

Presubmission Checks

  • clang-format (--style=file --dry-run --Werror): pass on all added/modified C++ sources.
  • pre-commit: pass (no violations on changed files).

@vybhav-nexthop vybhav-nexthop requested review from a team as code owners June 29, 2026 07:44
@meta-cla meta-cla Bot added the CLA Signed label Jun 29, 2026
@vybhav-nexthop vybhav-nexthop force-pushed the nos-7154-hostname-config branch from 9fb36c7 to 1f79a62 Compare June 29, 2026 09:35

@joseph5wu joseph5wu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider to use CmdConfigSwitch to manage individual Switch Settings

Comment thread fboss/cli/fboss2/test/integration_test/ConfigHostnameTest.cpp Outdated
Comment thread fboss/cli/fboss2/test/integration_test/Fboss2IntegrationTest.h Outdated
vybhav-nexthop

This comment was marked as resolved.

@vybhav-nexthop vybhav-nexthop force-pushed the nos-7154-hostname-config branch 2 times, most recently from 7e359f0 to 2e00edd Compare July 3, 2026 06:37
Adds:

    fboss2-dev config switch hostname <name>

Sets SwitchConfig::hostname in the agent running config via a HITLESS
config session (applied by ThriftConfigApplier::updateSwitchSettings, no
agent restart required). Input validated as RFC 952/1123 compliant.

Nested under the existing `config switch` command group (introduced by the
admin-distance command) rather than at the top level, grouping
switch-level settings that do not belong to a specific network object.

Files:
- commands/config/switch/hostname/CmdConfigHostname.{h,cpp}  new handler
- test/config/CmdConfigHostnameTest.cpp                      unit tests
- test/integration_test/ConfigHostnameTest.cpp              integration test
- CmdListConfig.cpp: register hostname as a subcommand of config switch
- CmdConfigSwitch.cpp: list hostname in the subcommand help string
- BUCK / cmake wiring

Signed-off-by: Vybhav <vybhav@nexthop.ai>
@vybhav-nexthop vybhav-nexthop force-pushed the nos-7154-hostname-config branch from 2e00edd to d8c99af Compare July 3, 2026 10:36
@meta-codesync

meta-codesync Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

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

@meta-codesync meta-codesync Bot closed this in 3aed671 Jul 6, 2026
@meta-codesync meta-codesync Bot added the Merged label Jul 6, 2026
@meta-codesync

meta-codesync Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

@joseph5wu merged this pull request in 3aed671.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants