diff --git a/fboss/cli/fboss2/test/integration_test/ConfigInterfaceMtuTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigInterfaceMtuTest.cpp index af1f37b915542..bdb827cee4141 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigInterfaceMtuTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigInterfaceMtuTest.cpp @@ -32,6 +32,7 @@ class ConfigInterfaceMtuTest : public Fboss2IntegrationTest { {"config", "interface", interfaceName, "mtu", std::to_string(mtu)}); ASSERT_EQ(result.exitCode, 0) << "Failed to set MTU: " << result.stderr; commitConfig(); + waitForAgentReady(); } }; @@ -77,16 +78,20 @@ TEST_F(ConfigInterfaceMtuTest, SetAndVerifyMtu) { // Step 5: Poll kernel interface MTU until it propagates XLOG(INFO) << "[Step 5] Waiting for kernel interface MTU to propagate..."; - ASSERT_TRUE(interface.vlan.has_value()); - int kernelMtu = getKernelInterfaceMtu(*interface.vlan); - if (kernelMtu > 0) { - EXPECT_EQ(kernelMtu, newMtu) - << "Kernel MTU is " << kernelMtu << ", expected " << newMtu; - XLOG(INFO) << " Verified: Kernel interface fboss" << *interface.vlan - << " has MTU " << kernelMtu; + if (!interface.vlan.has_value()) { + XLOG(INFO) << " Skipped: interface has no VLAN (routed-only platform)"; } else { - XLOG(INFO) << " Skipped: Kernel interface fboss" << *interface.vlan - << " not found"; + int kernelMtu = waitForKernelMtu( + *interface.vlan, [newMtu](int mtu) { return mtu == newMtu; }); + if (kernelMtu > 0) { + EXPECT_EQ(kernelMtu, newMtu) + << "Kernel MTU is " << kernelMtu << ", expected " << newMtu; + XLOG(INFO) << " Verified: Kernel interface fboss" << *interface.vlan + << " has MTU " << kernelMtu; + } else { + XLOG(INFO) << " Skipped: Kernel interface fboss" << *interface.vlan + << " not found"; + } } // Step 6: Restore original MTU diff --git a/fboss/cli/fboss2/test/integration_test/ConfigPfcTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigPfcTest.cpp index 37e2120142c76..c7d97b7735641 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigPfcTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigPfcTest.cpp @@ -13,13 +13,17 @@ #include #include +#include #include #include +#include #include #include #include #include #include +#include "fboss/agent/gen-cpp2/switch_config_types.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/test/integration_test/Fboss2IntegrationTest.h" using namespace facebook::fboss; @@ -58,47 +62,49 @@ class ConfigPfcTest : public Fboss2IntegrationTest { Fboss2IntegrationTest::SetUp(); XLOG(INFO) << "Using buffer-pool: " << bufferPoolName_; XLOG(INFO) << "Using pg-policy: " << policyName_; + // Snapshot the full running SwitchConfig BEFORE this test mutates it. The + // buffer-pool / priority-group-policy / port-PFC objects this test creates + // have no `delete` CLI, so a per-field revert is impossible. The only clean + // way to undo is to restore the entire SwitchConfig in TearDown. Leaving + // the PFC state behind makes a later test's hitless config apply crash the + // agent (EventBase re-entrancy on gracefulExit). Deserializing here (not + // in TearDown) fails the test before anything is mutated if the running + // config can't round-trip. + auto running = getRunningConfig(); + ASSERT_TRUE(running.count("sw")) + << "Running config has no 'sw' section; cannot snapshot for restore"; + configSnapshot_ = + apache::thrift::SimpleJSONSerializer::deserialize( + folly::toJson(running["sw"])); } void TearDown() override { - if (pfcIntfName_.has_value()) { - // Disable PFC so subsequent tests (e.g. flow-control/PAUSE tests) can - // use the same port without hitting the "PAUSE and PFC cannot be enabled - // on the same port" rejection from the agent. - XLOG(INFO) << "TearDown: disabling PFC on " << *pfcIntfName_ - << " (port used by this test)"; - bool anyFailed = false; - for (const auto* dir : {"tx", "rx"}) { - auto result = runCli( - {"config", - "interface", - *pfcIntfName_, - "pfc-config", - dir, - "disabled"}); - if (result.exitCode != 0) { - XLOG(WARN) << "TearDown: failed to disable pfc-config " << dir - << " on " << *pfcIntfName_ << ": " << result.stderr; - anyFailed = true; - } else { - XLOG(INFO) << "TearDown: pfc-config " << dir << " disabled on " - << *pfcIntfName_; - } - } - auto commitResult = runCli({"config", "session", "commit"}); - if (commitResult.exitCode != 0) { - XLOG(WARN) << "TearDown: commit after PFC disable failed: " - << commitResult.stderr; - } else if (!anyFailed) { - XLOG(INFO) << "TearDown: PFC cleanup committed successfully on " - << *pfcIntfName_; + if (configSnapshot_.has_value()) { + XLOG(INFO) << "TearDown: restoring pre-test config snapshot"; + try { + auto& session = ConfigSession::getInstance(); + session.getAgentConfig().sw() = *configSnapshot_; + // Force WARMBOOT (not the default HITLESS): committing a restart + // re-applies the snapshot cleanly at boot. A *hitless* apply of this + // revert is exactly the path that aborts the agent, so it must be + // avoided. + session.saveConfig( + cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + commitConfig(); + waitForAgentReady(); + XLOG(INFO) << "TearDown: config snapshot restored"; + } catch (const std::exception& e) { + // Surface the failure: leftover PFC state crashes a later suite's + // hitless config apply, and that crash would be misattributed. + ADD_FAILURE() << "TearDown: failed to restore config snapshot: " + << e.what(); } } Fboss2IntegrationTest::TearDown(); } - // Recorded by the test body so TearDown knows which interface to clean up. - std::optional pfcIntfName_; + // Pre-test SwitchConfig captured in SetUp, restored in TearDown. + std::optional configSnapshot_; void configureBufferPool(int sharedBytes, int headroomBytes) { ASSERT_EQ( @@ -187,7 +193,6 @@ class ConfigPfcTest : public Fboss2IntegrationTest { TEST_F(ConfigPfcTest, BufferPoolPriorityGroupAndPortPfc) { Interface intf = findFirstEthInterface(); - pfcIntfName_ = intf.name; XLOG(INFO) << "Using test interface " << intf.name; XLOG(INFO) << "[Step 1] Configuring buffer pool..."; diff --git a/fboss/cli/fboss2/test/integration_test/ConfigVlanCreateTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigVlanCreateTest.cpp index df3959239295c..808df1f0ebe85 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigVlanCreateTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigVlanCreateTest.cpp @@ -6,7 +6,7 @@ * Tests: * * 1. StaticMacAddDeleteOnExistingVlan - * - Add a static MAC entry to an existing VLAN (2001) + * - Add a static MAC entry to an existing VLAN (discovered dynamically) * - Commit and verify * - Delete the entry and commit * This is the primary use case for static-mac add/delete. @@ -30,7 +30,7 @@ * Requirements: * - FBOSS agent must be running with a valid configuration * - The test must be run as root (or with appropriate permissions) - * - VLAN 2001 must exist in the config (standard fboss103 setup) + * - Tests that need an existing L2 VLAN skip on pure routed-mode DUTs */ #include @@ -50,35 +50,26 @@ namespace fs = std::filesystem; using namespace facebook::fboss; class ConfigVlanCreateTest : public Fboss2IntegrationTest { - public: - // Find and cache an eth interface name once before any test in this suite - // runs. This avoids calling show interface during the agent reload window - // that follows test 1's commit. - static void SetUpTestSuite() { - // Concrete subclass to access protected Fboss2IntegrationTest methods. - struct Helper : public Fboss2IntegrationTest { - void TestBody() override {} - Interface getFirstEthInterface() { - return findFirstEthInterface(); - } - }; - Helper h; - auto iface = h.getFirstEthInterface(); - s_testInterfaceName_ = iface.name; - XLOG(INFO) << "SetUpTestSuite: cached test interface = " - << s_testInterfaceName_; + protected: + // Discover an existing VLAN + port before each test. existingVlanId_ stays + // -1 on pure routed-mode DUTs (no L2 VLANs). + void SetUp() override { + Fboss2IntegrationTest::SetUp(); + if (auto vp = findConfiguredVlanPort()) { + existingVlanId_ = vp->first; + existingVlanPort_ = vp->second; + } + XLOG(INFO) << "SetUp: existing VLAN=" << existingVlanId_ + << " port=" << existingVlanPort_; } - protected: - // An existing VLAN with a proper L3 interface — safe to add static MACs to - static constexpr int kExistingVlanId = 2001; // A VLAN ID used to test auto-create; cleaned up before each run static constexpr int kNewVlanId = 3999; static constexpr const char* kTestMac = "02:00:00:00:27:01"; - // Interface name cached by SetUpTestSuite — valid for the lifetime of - // the test suite, even if the agent is reloading between individual tests. - static std::string s_testInterfaceName_; + // Discovered in SetUp(). -1/"" means no L2 VLAN on this DUT. + int existingVlanId_ = -1; + std::string existingVlanPort_; void addStaticMac(int vlanId, const std::string& mac, const std::string& port) { @@ -113,7 +104,7 @@ class ConfigVlanCreateTest : public Fboss2IntegrationTest { * test is idempotent across multiple runs. * * Strategy: - * 1. Run a benign config command on kExistingVlanId to initialize the + * 1. Run a benign config command on existingVlanId_ to initialize the * session file (~/.fboss2/agent.conf) from the current running config. * 2. Read the JSON session file and filter out any vlans, interfaces, and * staticMacAddrs entries that reference vlanId. @@ -128,7 +119,7 @@ class ConfigVlanCreateTest : public Fboss2IntegrationTest { runCli( {"config", "vlan", - std::to_string(kExistingVlanId), + std::to_string(existingVlanId_), "static-mac", "delete", "00:00:00:00:00:00"}); @@ -209,23 +200,25 @@ class ConfigVlanCreateTest : public Fboss2IntegrationTest { } }; -std::string ConfigVlanCreateTest::s_testInterfaceName_; - TEST_F(ConfigVlanCreateTest, StaticMacAddDeleteOnExistingVlan) { - XLOG(INFO) << "[Step 1] Using pre-cached interface: " << s_testInterfaceName_ - << " (VLAN: " << kExistingVlanId << ")"; + if (existingVlanId_ < 0) { + GTEST_SKIP() + << "No L2 VLAN with port members on this DUT (pure routed mode)"; + } + XLOG(INFO) << "[Step 1] Using VLAN=" << existingVlanId_ + << " port=" << existingVlanPort_; // Step 2: Add a static MAC to an existing VLAN - no creation message expected XLOG(INFO) << "[Step 2] Adding static MAC to existing VLAN " - << kExistingVlanId << "..."; + << existingVlanId_ << "..."; auto addResult = runCli( {"config", "vlan", - std::to_string(kExistingVlanId), + std::to_string(existingVlanId_), "static-mac", "add", kTestMac, - s_testInterfaceName_}); + existingVlanPort_}); ASSERT_EQ(addResult.exitCode, 0) << "Failed to add static MAC: " << addResult.stderr; EXPECT_THAT( @@ -240,12 +233,16 @@ TEST_F(ConfigVlanCreateTest, StaticMacAddDeleteOnExistingVlan) { // Step 3: Clean up - delete the static MAC entry XLOG(INFO) << "[Step 3] Cleaning up static MAC entry..."; - deleteStaticMac(kExistingVlanId, kTestMac); + deleteStaticMac(existingVlanId_, kTestMac); XLOG(INFO) << " Cleanup complete. TEST PASSED"; } TEST_F(ConfigVlanCreateTest, AutoCreateVlanInSession) { - XLOG(INFO) << "[Step 1] Using pre-cached interface: " << s_testInterfaceName_; + if (existingVlanId_ < 0) { + GTEST_SKIP() + << "No L2 VLAN with port members on this DUT (pure routed mode)"; + } + XLOG(INFO) << "[Step 1] Using port=" << existingVlanPort_; // Step 0: Ensure VLAN kNewVlanId is absent (idempotency across test runs). XLOG(INFO) << "[Step 0] Ensuring VLAN " << kNewVlanId @@ -263,7 +260,7 @@ TEST_F(ConfigVlanCreateTest, AutoCreateVlanInSession) { "static-mac", "add", kTestMac, - s_testInterfaceName_}); + existingVlanPort_}); ASSERT_EQ(result.exitCode, 0) << "Command failed: " << result.stderr; EXPECT_THAT(result.stdout, ::testing::HasSubstr("Created VLAN")) << "Expected VLAN creation message for new VLAN"; @@ -280,7 +277,7 @@ TEST_F(ConfigVlanCreateTest, AutoCreateVlanInSession) { "static-mac", "add", kTestMac, - s_testInterfaceName_}); + existingVlanPort_}); ASSERT_EQ(result2.exitCode, 0) << "Command failed: " << result2.stderr; EXPECT_THAT( result2.stdout, ::testing::Not(::testing::HasSubstr("Created VLAN"))) diff --git a/fboss/cli/fboss2/test/integration_test/ConfigVlanDefaultTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigVlanDefaultTest.cpp index c6cf30f07d086..c5e0d5d8715e5 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigVlanDefaultTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigVlanDefaultTest.cpp @@ -2,9 +2,14 @@ * End-to-end tests for 'fboss2-dev config vlan default '. * * Covers the happy-path workflow (move ports, set default, commit, verify via - * thrift), the no-op case when the target equals the current default, a safety - * guard that refuses the command when the precondition is unsafe, and the case - * where ports have already been moved off the current default VLAN. + * thrift), the no-op case when the target equals the current default, and a + * safety guard that refuses the command when the precondition is unsafe. + * + * Branch-level coverage of CmdConfigVlanDefault (target VLAN exists vs. + * created, old VLAN removal, missing defaultVlan field) lives in the fast unit + * test CmdConfigVlanDefaultTest; the e2e tests here only verify that a + * committed change survives the agent warmboot, so a single happy-path e2e + * suffices rather than one per branch. */ #include @@ -23,9 +28,9 @@ class ConfigVlanDefaultTest : public Fboss2IntegrationTest {}; * Sets a new default VLAN, commits, verifies via thrift, and restores. * * CmdConfigVlanDefault::queryClient auto-creates the target VLAN if it - * doesn't exist, so no manual VLAN creation is needed. We just need to - * ensure ports on the old default VLAN are moved off first (the command - * refuses to change when ports use the old default but it has no interface). + * doesn't exist, so no manual VLAN creation is needed. Ports on the old + * default VLAN must be moved off first (the command refuses to change when + * ports use the old default but it has no interface). */ TEST_F(ConfigVlanDefaultTest, SetDefaultVlanTo300) { waitForAgentReady(); @@ -37,40 +42,70 @@ TEST_F(ConfigVlanDefaultTest, SetDefaultVlanTo300) { XLOG(INFO) << "[Test] currentDefault=" << currentDefault << " targetVlan=" << targetVlan; - // Step 1: Move all eth ports whose ingressVlan matches the current default - // VLAN to an existing non-default VLAN. This is required because the - // command refuses to change when ports use the old default but it has no - // interface (safety guard against agent crash). - int64_t sideVlan = -1; - for (const auto& vlan : initialConfig["sw"]["vlans"]) { - int64_t vid = vlan.getDefault("id", -1).asInt(); - if (vid > 0 && vid != currentDefault) { - sideVlan = vid; - break; + // Step 0: Create the target VLAN if it doesn't already exist. + // VlanManager auto-creates the VLAN entry and a barebone cfg::Interface so + // the agent accepts the commit. The dummy MAC is deleted right away; only + // the VLAN and its interface are kept. + const std::string kDummyMac = "02:00:00:00:01:2c"; + { + bool exists = false; + if (initialConfig["sw"].count("vlans")) { + for (const auto& vlan : initialConfig["sw"]["vlans"]) { + if (vlan.getDefault("id", -1).asInt() == std::stoi(targetVlan)) { + exists = true; + break; + } + } + } + if (!exists) { + auto firstEthPort = findFirstEthInterface().name; + ASSERT_FALSE(firstEthPort.empty()) + << "No eth port found for VLAN creation"; + auto createResult = runCli( + {"config", + "vlan", + targetVlan, + "static-mac", + "add", + kDummyMac, + firstEthPort}); + ASSERT_EQ(createResult.exitCode, 0) + << "Failed to create VLAN " << targetVlan << ": " + << createResult.stderr; + auto macCleanup = runCli( + {"config", "vlan", targetVlan, "static-mac", "delete", kDummyMac}); + EXPECT_EQ(macCleanup.exitCode, 0) + << "Failed to delete dummy MAC from VLAN " << targetVlan << ": " + << macCleanup.stderr; + XLOG(INFO) << "[Step 0] " << createResult.stdout; } } + // Step 1: Move every eth port whose ingressVlan is the current default VLAN + // onto targetVlan (created in Step 0). targetVlan is intentionally the move + // target — it always exists by now, so no other side VLAN has to be found. + // This is required because the command refuses to change the default while + // ports still use the old default VLAN and it has no interface (safety + // guard against an agent crash), so the old default is vacated first. std::vector movedPorts; - if (sideVlan > 0) { - for (const auto& port : initialConfig["sw"]["ports"]) { - auto name = port.getDefault("name", "").asString(); - if (name.rfind("eth", 0) != 0) { - continue; - } - if (port.getDefault("ingressVlan", -1).asInt() != currentDefault) { - continue; - } - auto result = runCli( - {"config", - "interface", - name, - "switchport", - "access", - "vlan", - std::to_string(sideVlan)}); - if (result.exitCode == 0) { - movedPorts.push_back(name); - } + for (const auto& port : initialConfig["sw"]["ports"]) { + auto name = port.getDefault("name", "").asString(); + if (name.rfind("eth", 0) != 0) { + continue; + } + if (port.getDefault("ingressVlan", -1).asInt() != currentDefault) { + continue; + } + auto result = runCli( + {"config", + "interface", + name, + "switchport", + "access", + "vlan", + targetVlan}); + if (result.exitCode == 0) { + movedPorts.push_back(name); } } XLOG(INFO) << "[Step 1] Moved " << movedPorts.size() @@ -91,9 +126,22 @@ TEST_F(ConfigVlanDefaultTest, SetDefaultVlanTo300) { XLOG(INFO) << "[Step 3] Verified defaultVlan=" << getSwConfigField("defaultVlan"); - // Restore: move ports back, reset defaultVlan - XLOG(INFO) << "[Restore] Reverting ports and defaultVlan to " + // Restore: reset defaultVlan first, then move ports back. + XLOG(INFO) << "[Restore] Reverting defaultVlan and ports to " << currentDefault; + + // 'config vlan default' recreates currentDefault as a non-routable + // placeholder (no interface) when the earlier default change erased it, so + // it has to run before the ports are moved back — 'switchport access vlan' + // throws on a VLAN that is absent from the config. Recreating the VLAN via + // a static-mac add would instead attach a cfg::Interface to it, silently + // turning the interface-less default VLAN state this suite's refusal test + // depends on into an unreachable one. + auto restoreDefault = + runCli({"config", "vlan", "default", std::to_string(currentDefault)}); + ASSERT_EQ(restoreDefault.exitCode, 0) + << "[Restore] Failed to reset default VLAN: " << restoreDefault.stderr; + for (const auto& name : movedPorts) { runCli( {"config", @@ -105,11 +153,6 @@ TEST_F(ConfigVlanDefaultTest, SetDefaultVlanTo300) { std::to_string(currentDefault)}); } - auto restoreDefault = - runCli({"config", "vlan", "default", std::to_string(currentDefault)}); - ASSERT_EQ(restoreDefault.exitCode, 0) - << "[Restore] Failed to reset default VLAN: " << restoreDefault.stderr; - commitConfig(); waitForAgentReady(); discardSession();