[Nexthop][fboss2-dev] Harden fboss2 CLI integration tests

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

fboss/cli/fboss2/test/integration_test/ConfigPfcTest.cpp

@@ -13,13 +13,17 @@
 
 #include <fmt/format.h>
 #include <folly/json/dynamic.h>
+#include <folly/json/json.h>
 #include <folly/logging/xlog.h>
 #include <gtest/gtest.h>
+#include <thrift/lib/cpp2/protocol/Serializer.h>
 #include <cstdint>
 #include <initializer_list>
 #include <optional>
 #include <string>
 #include <vector>
+#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<cfg::SwitchConfig>(
+            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<std::string> pfcIntfName_;
+  // Pre-test SwitchConfig captured in SetUp, restored in TearDown.
+  std::optional<cfg::SwitchConfig> 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...";

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 <folly/json/dynamic.h>
@@ -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")))