patchpanel: arc: simplify Device and Config management

This patch keeps the same patchpanel::Device::Config instances inside
ArcService for the ARC management device (arc0 on ARC++) and Device
instances corresponding to physical devices managed by shill.

This allows to instantiate the Device::Configs only once and simplify
Device::Config allocation into ArcService constructor. Similarly, the
ARC management Device is created only once in ArcService constructor.

Another simplification is merging together AddDevice with StartDevice,
and RemoveDevice with StopDevice. Physical devices are now "added" or
"removed" in the sense of patchpanel::Device only when ArcService is
running. When the service starts, all existing shill devices are added
at once. The main benefit is a unique codepath for setting up a virtual
device datapath and tearing it down.

These changes make ArcService effectively stateless when ARC is not
running. It would be now possible to create the ArcService object when
ARC starts and completely destroy it when ARC stops.

BUG=b:159866048
TEST=Unit test passes. Also verified ARC++ and ARCVM multinet
connectivity manually.

Change-Id: If1441ef94106d6e54bfc7f94ebf5f6cb3efe4587
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2291415
Tested-by: Hugo Benichi <hugobenichi@google.com>
Commit-Queue: Hugo Benichi <hugobenichi@google.com>
Reviewed-by: Garrick Evans <garrick@chromium.org>
diff --git a/patchpanel/arc_service.cc b/patchpanel/arc_service.cc
index 6e66a0f..a9fe586 100644
--- a/patchpanel/arc_service.cc
+++ b/patchpanel/arc_service.cc
@@ -56,7 +56,7 @@
   return true;
 }
 
-void OneTimeSetup(const Datapath& datapath) {
+void OneTimeContainerSetup(const Datapath& datapath) {
   static bool done = false;
   if (done)
     return;
@@ -166,35 +166,41 @@
   return (ifr.ifr_flags & IFF_MULTICAST);
 }
 
-// Returns the configuration for the ARC management interface used for VPN
-// forwarding, ADB-over-TCP and single-networked ARCVM.
-std::unique_ptr<Device::Config> MakeArcConfig(
-    AddressManager* addr_mgr,
-    AddressManager::Guest guest,
-    GuestMessage::GuestType guest_type) {
-  auto ipv4_subnet = addr_mgr->AllocateIPv4Subnet(guest);
+// Returns the ARC management device used for VPN forwarding, ADB-over-TCP.
+std::unique_ptr<Device> MakeArcDevice(AddressManager* addr_mgr,
+                                      GuestMessage::GuestType guest) {
+  auto ipv4_subnet = addr_mgr->AllocateIPv4Subnet(AddressManager::Guest::ARC);
   if (!ipv4_subnet) {
     LOG(ERROR) << "Subnet already in use or unavailable";
     return nullptr;
   }
+
   auto host_ipv4_addr = ipv4_subnet->AllocateAtOffset(0);
   if (!host_ipv4_addr) {
     LOG(ERROR) << "Bridge address already in use or unavailable";
     return nullptr;
   }
+
   auto guest_ipv4_addr = ipv4_subnet->AllocateAtOffset(1);
   if (!guest_ipv4_addr) {
     LOG(ERROR) << "ARC address already in use or unavailable";
     return nullptr;
   }
 
-  int subnet_index = (guest_type == GuestMessage::ARC_VM) ? 1 : kAnySubnetIndex;
+  int subnet_index = (guest == GuestMessage::ARC_VM) ? 1 : kAnySubnetIndex;
 
-  return std::make_unique<Device::Config>(
+  auto config = std::make_unique<Device::Config>(
       addr_mgr->GenerateMacAddress(subnet_index), std::move(ipv4_subnet),
       std::move(host_ipv4_addr), std::move(guest_ipv4_addr));
-}
 
+  Device::Options opts{
+      .fwd_multicast = false,
+      .ipv6_enabled = false,
+  };
+
+  return std::make_unique<Device>(kArcIfname, kArcBridge, kArcIfname,
+                                  std::move(config), opts);
+}
 }  // namespace
 
 ArcService::ArcService(ShillClient* shill_client,
@@ -208,6 +214,7 @@
       forwarder_(forwarder),
       guest_(guest),
       id_(kInvalidId) {
+  arc_device_ = MakeArcDevice(addr_mgr, guest_);
   AllocateAddressConfigs();
   shill_client_->RegisterDevicesChangedHandler(
       base::Bind(&ArcService::OnDevicesChanged, weak_factory_.GetWeakPtr()));
@@ -225,7 +232,6 @@
 }
 
 void ArcService::AllocateAddressConfigs() {
-  available_configs_.clear();
   // The first usable subnet is the "other" ARC device subnet.
   // As a temporary workaround, for ARCVM, allocate fixed MAC addresses.
   uint8_t mac_addr_index = 2;
@@ -258,24 +264,12 @@
         mac_addr, std::move(ipv4_subnet), std::move(host_ipv4_addr),
         std::move(guest_ipv4_addr)));
   }
-}
 
-void ArcService::ReallocateAddressConfigs() {
-  std::vector<std::string> existing_devices;
-  for (const auto& d : devices_) {
-    existing_devices.emplace_back(d.first);
-  }
-  for (const auto& d : existing_devices) {
-    RemoveDevice(d);
-  }
-  AllocateAddressConfigs();
-  all_configs_.clear();
   for (const auto& kv : available_configs_)
     for (const auto& c : kv.second)
       all_configs_.emplace_back(c.get());
-  for (const auto& d : existing_devices) {
-    AddDevice(d);
-  }
+  // Append arc0 config so that the necessary tap device gets created.
+  all_configs_.insert(all_configs_.begin(), &arc_device_->config());
 }
 
 std::unique_ptr<Device::Config> ArcService::AcquireConfig(
@@ -316,13 +310,8 @@
     Stop(id_);
   }
 
-  ReallocateAddressConfigs();
-  std::unique_ptr<Device::Config> arc_device_config =
-      MakeArcConfig(addr_mgr_, AddressManager::Guest::ARC, guest_);
+  std::string arc_device_ifname;
   if (guest_ == GuestMessage::ARC_VM) {
-    // Append arc0 config separately from ArcService::ReallocateAddressConfigs()
-    // so that VmImpl::Start() can create the necessary tap device.
-    all_configs_.insert(all_configs_.begin(), arc_device_config.get());
     // Allocate TAP devices for all configs.
     for (auto* config : all_configs_) {
       auto mac = config->mac_addr();
@@ -336,38 +325,14 @@
 
       config->set_tap_ifname(tap);
     }
+    arc_device_ifname = arc_device_->config().tap_ifname();
   } else {
-    OneTimeSetup(*datapath_);
+    OneTimeContainerSetup(*datapath_);
     if (!datapath_->NetnsAttachName(kArcNetnsName, id)) {
       LOG(ERROR) << "Failed to attach name " << kArcNetnsName << " to pid "
                  << id;
       return false;
     }
-  }
-  id_ = id;
-
-  // Create the bridge for the management device arc0.
-  // Per crbug/1008686 this device cannot be deleted and then re-added.
-  // So instead of removing the bridge when the service stops, bring down the
-  // device instead and re-up it on restart.
-  if (!datapath_->AddBridge(kArcBridge, arc_device_config->host_ipv4_addr(),
-                            30) &&
-      !datapath_->MaskInterfaceFlags(kArcBridge, IFF_UP)) {
-    LOG(ERROR) << "Failed to bring up arc bridge " << kArcBridge;
-    return false;
-  }
-
-  Device::Options opts{
-      .fwd_multicast = false,
-      .ipv6_enabled = false,
-  };
-  arc_device_ = std::make_unique<Device>(kArcIfname, kArcBridge, kArcIfname,
-                                         std::move(arc_device_config), opts);
-
-  std::string arc_device_ifname;
-  if (guest_ == GuestMessage::ARC_VM) {
-    arc_device_ifname = arc_device_->config().tap_ifname();
-  } else {
     arc_device_ifname = ArcVethHostName(arc_device_->guest_ifname());
     if (!datapath_->ConnectVethPair(id, kArcNetnsName, arc_device_ifname,
                                     arc_device_->guest_ifname(),
@@ -379,6 +344,18 @@
       return false;
     }
   }
+  id_ = id;
+
+  // Create the bridge for the management device arc0.
+  // Per crbug/1008686 this device cannot be deleted and then re-added.
+  // So instead of removing the bridge when the service stops, bring down the
+  // device instead and re-up it on restart.
+  if (!datapath_->AddBridge(kArcBridge, arc_device_->config().host_ipv4_addr(),
+                            30) &&
+      !datapath_->MaskInterfaceFlags(kArcBridge, IFF_UP)) {
+    LOG(ERROR) << "Failed to bring up arc bridge " << kArcBridge;
+    return false;
+  }
 
   if (!datapath_->AddToBridge(kArcBridge, arc_device_ifname)) {
     LOG(ERROR) << "Failed to bridge arc device " << arc_device_ifname << " to "
@@ -388,8 +365,8 @@
   LOG(INFO) << "Started ARC management device " << *arc_device_.get();
 
   // Start already known Shill <-> ARC mapped devices.
-  for (const auto& d : devices_)
-    StartDevice(d.second.get());
+  for (const auto& ifname : shill_devices_)
+    AddDevice(ifname);
 
   return true;
 }
@@ -407,8 +384,8 @@
   }
 
   // Stop Shill <-> ARC mapped devices.
-  for (const auto& d : devices_)
-    StopDevice(d.second.get());
+  for (const auto& ifname : shill_devices_)
+    RemoveDevice(ifname);
 
   // Per crbug/1008686 this device cannot be deleted and then re-added.
   // So instead of removing the bridge, bring it down and mark it. This will
@@ -422,28 +399,37 @@
     datapath_->RemoveInterface(ArcVethHostName(arc_device_->phys_ifname()));
     if (!datapath_->NetnsDeleteName(kArcNetnsName))
       LOG(WARNING) << "Failed to delete netns name " << kArcNetnsName;
-  } else {
-    // Destroy pre allocated TAP devices, including the ARC management device.
-    for (const auto* config : all_configs_)
-      if (!config->tap_ifname().empty())
-        datapath_->RemoveInterface(config->tap_ifname());
+  }
+
+  // Destroy allocated TAP devices if any, including the ARC management device.
+  for (auto* config : all_configs_) {
+    if (config->tap_ifname().empty())
+      continue;
+    datapath_->RemoveInterface(config->tap_ifname());
+    config->set_tap_ifname("");
   }
 
   LOG(INFO) << "Stopped ARC management device " << *arc_device_.get();
-  arc_device_.reset();
   id_ = kInvalidId;
 }
 
 void ArcService::OnDevicesChanged(const std::set<std::string>& added,
                                   const std::set<std::string>& removed) {
-  for (const std::string& name : removed)
-    RemoveDevice(name);
+  for (const std::string& ifname : removed) {
+    shill_devices_.erase(ifname);
+    RemoveDevice(ifname);
+  }
 
-  for (const std::string& name : added)
-    AddDevice(name);
+  for (const std::string& ifname : added) {
+    shill_devices_.insert(ifname);
+    AddDevice(ifname);
+  }
 }
 
 void ArcService::AddDevice(const std::string& ifname) {
+  if (!IsStarted())
+    return;
+
   if (ifname.empty())
     return;
 
@@ -456,42 +442,32 @@
   Device::Options opts{
       .fwd_multicast = IsMulticastInterface(ifname),
       // TODO(crbug/726815) Also enable |ipv6_enabled| for cellular networks
-      // once IPv6 is enabled on cellular networks in shill.
+      // once IPv6 is enabled on cellular networks in Shill.
       .ipv6_enabled =
           (itype == InterfaceType::ETHERNET || itype == InterfaceType::WIFI),
   };
 
   auto config = AcquireConfig(ifname);
   if (!config) {
-    LOG(ERROR) << "Cannot add device for " << ifname;
+    LOG(ERROR) << "Cannot acquire a Config for " << ifname;
     return;
   }
 
   auto device = std::make_unique<Device>(ifname, ArcBridgeName(ifname), ifname,
                                          std::move(config), opts);
-
-  StartDevice(device.get());
-  devices_.emplace(ifname, std::move(device));
-}
-
-void ArcService::StartDevice(Device* device) {
-  if (!IsStarted())
-    return;
-
-  const auto& config = device->config();
-
   LOG(INFO) << "Starting device " << *device;
 
   // Create the bridge.
-  if (!datapath_->AddBridge(device->host_ifname(), config.host_ipv4_addr(),
-                            30)) {
-    LOG(ERROR) << "Failed to setup arc bridge: " << device->host_ifname();
+  if (!datapath_->AddBridge(device->host_ifname(),
+                            device->config().host_ipv4_addr(), 30)) {
+    LOG(ERROR) << "Failed to setup bridge " << device->host_ifname();
     return;
   }
 
   // Set up iptables.
   if (!datapath_->AddInboundIPv4DNAT(
-          device->phys_ifname(), IPv4AddressToString(config.guest_ipv4_addr())))
+          device->phys_ifname(),
+          IPv4AddressToString(device->config().guest_ipv4_addr())))
     LOG(ERROR) << "Failed to configure ingress traffic rules for "
                << device->phys_ifname();
 
@@ -512,7 +488,7 @@
             id_, kArcNetnsName, virtual_device_ifname, device->guest_ifname(),
             device->config().mac_addr(), device->config().guest_ipv4_addr(), 30,
             device->options().fwd_multicast)) {
-      LOG(ERROR) << "Cannot create virtual link for device " << *device;
+      LOG(ERROR) << "Cannot create veth link for device " << *device;
       return;
     }
   }
@@ -528,32 +504,27 @@
   forwarder_->StartForwarding(device->phys_ifname(), device->host_ifname(),
                               device->options().ipv6_enabled,
                               device->options().fwd_multicast);
+  devices_.emplace(ifname, std::move(device));
 }
 
 void ArcService::RemoveDevice(const std::string& ifname) {
+  if (!IsStarted())
+    return;
+
   const auto it = devices_.find(ifname);
   if (it == devices_.end()) {
     LOG(WARNING) << "Unknown device: " << ifname;
     return;
   }
 
-  // If the container is down, this call does nothing.
-  StopDevice(it->second.get());
-
-  ReleaseConfig(ifname, it->second->release_config());
-  devices_.erase(it);
-}
-
-void ArcService::StopDevice(Device* device) {
-  if (!IsStarted())
-    return;
-
+  const auto* device = it->second.get();
   LOG(INFO) << "Removing device " << *device;
 
   forwarder_->StopForwarding(device->phys_ifname(), device->host_ifname(),
                              device->options().ipv6_enabled,
                              device->options().fwd_multicast);
-  // TAP devices are removed in VmImpl::Stop().
+
+  // ARCVM TAP devices are removed in VmImpl::Stop() when the service stops
   if (guest_ == GuestMessage::ARC)
     datapath_->RemoveInterface(ArcVethHostName(device->phys_ifname()));
 
@@ -562,6 +533,9 @@
   datapath_->RemoveInboundIPv4DNAT(
       device->phys_ifname(), IPv4AddressToString(config.guest_ipv4_addr()));
   datapath_->RemoveBridge(device->host_ifname());
+
+  ReleaseConfig(ifname, it->second->release_config());
+  devices_.erase(it);
 }
 
 std::vector<const Device::Config*> ArcService::GetDeviceConfigs() const {
diff --git a/patchpanel/arc_service.h b/patchpanel/arc_service.h
index 4b93218..2001fec 100644
--- a/patchpanel/arc_service.h
+++ b/patchpanel/arc_service.h
@@ -49,42 +49,28 @@
   // configurations, if any, are currently associated to TAP devices.
   std::vector<const Device::Config*> GetDeviceConfigs() const;
 
- private:
-  // Returns true if the service has been started for ARC container or ARCVM.
-  bool IsStarted() const;
-
   // Callback from ShillClient, invoked whenever the device list changes.
-  // |devices_| will contain all devices currently connected to shill
+  // |shill_devices_| will contain all devices currently connected to shill
   // (e.g. "eth0", "wlan0", etc).
   void OnDevicesChanged(const std::set<std::string>& added,
                         const std::set<std::string>& removed);
 
-  // Build and configure an ARC device for the interface |name| provided by
-  // Shill. The new device will be added to |devices_|. If ArcService is
-  // already running, the device will be started.
+ private:
+  // Returns true if the service has been started for ARC container or ARCVM.
+  bool IsStarted() const;
+
+  // Build and configure the ARC datapath for the physical device |ifname|
+  // provided by Shill.
   void AddDevice(const std::string& ifname);
 
-  // Deletes the ARC device; if ArcService is running, the device will be
-  // stopped first.
+  // Teardown the ARC datapath associated with the physical device |ifname| and
+  // stops forwarding services.
   void RemoveDevice(const std::string& ifname);
 
-  // Starts a device by setting up the bridge and configuring some NAT rules,
-  // then invoking the container or VM specific start routine.
-  void StartDevice(Device* device);
-
-  // Stops and cleans up any virtual interfaces and associated datapath.
-  void StopDevice(Device* device);
-
   // Creates device configurations for all available IPv4 subnets which will be
   // assigned to devices as they are added.
   void AllocateAddressConfigs();
 
-  // This function will temporarily remove existing devices, reallocate
-  // address configurations and re-add existing devices. This is necessary to
-  // properly handle the IPv4 addressing binding difference between ARC++ and
-  // ARCVM.
-  void ReallocateAddressConfigs();
-
   // Reserve a configuration for an interface.
   std::unique_ptr<Device::Config> AcquireConfig(const std::string& ifname);
 
@@ -112,24 +98,11 @@
   std::unique_ptr<Device> arc_device_;
   // The PID of the ARC container instance or the CID of ARCVM instance.
   uint32_t id_;
+  // All devices currently managed by shill.
+  std::set<std::string> shill_devices_;
 
-  FRIEND_TEST(ArcServiceTest, ContainerImpl_OnStartDevice);
-  FRIEND_TEST(ArcServiceTest, ContainerImpl_FailsToConfigureInterface);
-  FRIEND_TEST(ArcServiceTest, ContainerImpl_OnStopDevice);
-  FRIEND_TEST(ArcServiceTest, ContainerImpl_Start);
-  FRIEND_TEST(ArcServiceTest, ContainerImpl_FailsToCreateInterface);
-  FRIEND_TEST(ArcServiceTest, ContainerImpl_Stop);
   FRIEND_TEST(ArcServiceTest, NotStarted_AddDevice);
   FRIEND_TEST(ArcServiceTest, NotStarted_AddRemoveDevice);
-  FRIEND_TEST(ArcServiceTest, StableArcVmMacAddrs);
-  FRIEND_TEST(ArcServiceTest, StopDevice);
-  FRIEND_TEST(ArcServiceTest, VerifyAddrConfigs);
-  FRIEND_TEST(ArcServiceTest, VerifyAddrOrder);
-  FRIEND_TEST(ArcServiceTest, VmImpl_Start);
-  FRIEND_TEST(ArcServiceTest, VmImpl_StartDevice);
-  FRIEND_TEST(ArcServiceTest, VmImpl_StartMultipleDevices);
-  FRIEND_TEST(ArcServiceTest, VmImpl_Stop);
-  FRIEND_TEST(ArcServiceTest, VmImpl_StopDevice);
 
   base::WeakPtrFactory<ArcService> weak_factory_{this};
   DISALLOW_COPY_AND_ASSIGN(ArcService);
diff --git a/patchpanel/arc_service_test.cc b/patchpanel/arc_service_test.cc
index 01aad32..094319f 100644
--- a/patchpanel/arc_service_test.cc
+++ b/patchpanel/arc_service_test.cc
@@ -94,8 +94,9 @@
   EXPECT_CALL(*datapath_, AddOutboundIPv4(StrEq("arc_eth0"))).Times(0);
 
   auto svc = NewService(GuestMessage::ARC);
-  svc->AddDevice("eth0");
-  EXPECT_TRUE(svc->devices_.find("eth0") != svc->devices_.end());
+  svc->OnDevicesChanged({"eth0"}, {});
+  EXPECT_TRUE(svc->devices_.find("eth0") == svc->devices_.end());
+  EXPECT_FALSE(svc->shill_devices_.find("eth0") == svc->shill_devices_.end());
 }
 
 TEST_F(ArcServiceTest, NotStarted_AddRemoveDevice) {
@@ -107,9 +108,10 @@
   EXPECT_CALL(*datapath_, RemoveBridge(StrEq("arc_eth0"))).Times(0);
 
   auto svc = NewService(GuestMessage::ARC);
-  svc->AddDevice("eth0");
-  svc->RemoveDevice("eth0");
+  svc->OnDevicesChanged({"eth0"}, {});
+  svc->OnDevicesChanged({}, {"eth0"});
   EXPECT_TRUE(svc->devices_.find("eth0") == svc->devices_.end());
+  EXPECT_TRUE(svc->shill_devices_.find("eth0") == svc->shill_devices_.end());
 }
 
 TEST_F(ArcServiceTest, VerifyAddrConfigs) {
@@ -129,14 +131,18 @@
       .WillOnce(Return(true));
   EXPECT_CALL(*datapath_, AddInboundIPv4DNAT(_, _))
       .WillRepeatedly(Return(true));
+  EXPECT_CALL(*datapath_, AddOutboundIPv4(_)).WillRepeatedly(Return(true));
+  EXPECT_CALL(*datapath_, AddVirtualInterfacePair(StrEq("arc_netns"), _, _))
+      .WillRepeatedly(Return(true));
+  EXPECT_CALL(*datapath_, ToggleInterface(_, true))
+      .WillRepeatedly(Return(true));
+  EXPECT_CALL(*datapath_, ConfigureInterface(_, _, _, _, _, _))
+      .WillRepeatedly(Return(true));
+  EXPECT_CALL(*datapath_, AddToBridge(_, _)).WillRepeatedly(Return(true));
 
   auto svc = NewService(GuestMessage::ARC);
   svc->Start(kTestPID);
-  svc->AddDevice("eth0");
-  svc->AddDevice("eth1");
-  svc->AddDevice("wlan0");
-  svc->AddDevice("wlan1");
-  svc->AddDevice("wwan0");
+  svc->OnDevicesChanged({"eth0", "eth1", "wlan0", "wlan1", "wwan0"}, {});
 }
 
 TEST_F(ArcServiceTest, VerifyAddrOrder) {
@@ -152,13 +158,20 @@
   EXPECT_CALL(*datapath_, AddInboundIPv4DNAT(_, _))
       .WillRepeatedly(Return(true));
   EXPECT_CALL(*datapath_, AddOutboundIPv4(_)).WillRepeatedly(Return(true));
+  EXPECT_CALL(*datapath_, AddVirtualInterfacePair(StrEq("arc_netns"), _, _))
+      .WillRepeatedly(Return(true));
+  EXPECT_CALL(*datapath_, ToggleInterface(_, true))
+      .WillRepeatedly(Return(true));
+  EXPECT_CALL(*datapath_, ConfigureInterface(_, _, _, _, _, _))
+      .WillRepeatedly(Return(true));
+  EXPECT_CALL(*datapath_, AddToBridge(_, _)).WillRepeatedly(Return(true));
 
   auto svc = NewService(GuestMessage::ARC);
   svc->Start(kTestPID);
-  svc->AddDevice("wlan0");
-  svc->AddDevice("eth0");
-  svc->RemoveDevice("eth0");
-  svc->AddDevice("eth0");
+  svc->OnDevicesChanged({"wlan0"}, {});
+  svc->OnDevicesChanged({"eth0"}, {});
+  svc->OnDevicesChanged({}, {"eth0"});
+  svc->OnDevicesChanged({"eth0"}, {});
 }
 
 TEST_F(ArcServiceTest, StableArcVmMacAddrs) {
@@ -206,13 +219,14 @@
 TEST_F(ArcServiceTest, ContainerImpl_FailsToCreateInterface) {
   EXPECT_CALL(*datapath_, NetnsAttachName(StrEq("arc_netns"), kTestPID))
       .WillOnce(Return(true));
-  EXPECT_CALL(*datapath_, AddBridge(StrEq("arcbr0"), kArcHostIP, 30))
-      .WillOnce(Return(true));
   EXPECT_CALL(*datapath_,
               AddVirtualInterfacePair(StrEq("arc_netns"), StrEq("vetharc0"),
                                       StrEq("arc0")))
       .WillOnce(Return(false));
+
   EXPECT_CALL(*datapath_, ConfigureInterface(_, _, _, _, _, _)).Times(0);
+  EXPECT_CALL(*datapath_, ToggleInterface(StrEq("vetharc0"), true)).Times(0);
+  EXPECT_CALL(*datapath_, AddBridge(StrEq("arcbr0"), kArcHostIP, 30)).Times(0);
   EXPECT_CALL(*datapath_, RemoveBridge(_)).Times(0);
 
   auto svc = NewService(GuestMessage::ARC);
@@ -222,8 +236,6 @@
 TEST_F(ArcServiceTest, ContainerImpl_FailsToConfigureInterface) {
   EXPECT_CALL(*datapath_, NetnsAttachName(StrEq("arc_netns"), kTestPID))
       .WillOnce(Return(true));
-  EXPECT_CALL(*datapath_, AddBridge(StrEq("arcbr0"), kArcHostIP, 30))
-      .WillOnce(Return(true));
   EXPECT_CALL(*datapath_,
               AddVirtualInterfacePair(StrEq("arc_netns"), StrEq("vetharc0"),
                                       StrEq("arc0")))
@@ -231,9 +243,9 @@
   EXPECT_CALL(*datapath_,
               ConfigureInterface(StrEq("arc0"), _, kArcGuestIP, 30, true, _))
       .WillOnce(Return(false));
+
   EXPECT_CALL(*datapath_, ToggleInterface(StrEq("vetharc0"), true)).Times(0);
-  EXPECT_CALL(*datapath_, AddToBridge(StrEq("arcbr0"), StrEq("vetharc0")))
-      .Times(0);
+  EXPECT_CALL(*datapath_, AddBridge(StrEq("arcbr0"), kArcHostIP, 30)).Times(0);
   EXPECT_CALL(*datapath_, RemoveBridge(_)).Times(0);
 
   auto svc = NewService(GuestMessage::ARC);
@@ -306,7 +318,52 @@
 
   auto svc = NewService(GuestMessage::ARC);
   svc->Start(kTestPID);
-  svc->AddDevice("eth0");
+  svc->OnDevicesChanged({"eth0"}, {});
+}
+
+TEST_F(ArcServiceTest, ContainerImpl_StartAfterDevice) {
+  EXPECT_CALL(*datapath_, NetnsAttachName(StrEq("arc_netns"), kTestPID))
+      .WillOnce(Return(true));
+  // Expectations for arc0 setup.
+  EXPECT_CALL(*datapath_, AddBridge(StrEq("arcbr0"), kArcHostIP, 30))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*datapath_,
+              AddVirtualInterfacePair(StrEq("arc_netns"), StrEq("vetharc0"),
+                                      StrEq("arc0")))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*datapath_,
+              ConfigureInterface(StrEq("arc0"), _, kArcGuestIP, 30, true, _))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*datapath_, ToggleInterface(StrEq("vetharc0"), true))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*datapath_, AddToBridge(StrEq("arcbr0"), StrEq("vetharc0")))
+      .WillOnce(Return(true));
+  // Expectations for eth0 setup.
+  EXPECT_CALL(*datapath_, AddBridge(StrEq("arc_eth0"), kFirstEthHostIP, 30))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*datapath_,
+              AddVirtualInterfacePair(StrEq("arc_netns"), StrEq("vetheth0"),
+                                      StrEq("eth0")))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*datapath_, ConfigureInterface(StrEq("eth0"), _, kFirstEthGuestIP,
+                                             30, true, _))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*datapath_, ToggleInterface(StrEq("vetheth0"), true))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*datapath_, AddToBridge(StrEq("arc_eth0"), StrEq("vetheth0")))
+      .WillOnce(Return(true));
+  EXPECT_CALL(forwarder_,
+              StartForwarding(StrEq("eth0"), StrEq("arc_eth0"), _, _));
+  EXPECT_CALL(
+      *datapath_,
+      AddInboundIPv4DNAT(StrEq("eth0"), IPv4AddressToString(kFirstEthGuestIP)))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*datapath_, AddOutboundIPv4(StrEq("arc_eth0")))
+      .WillOnce(Return(true));
+
+  auto svc = NewService(GuestMessage::ARC);
+  svc->OnDevicesChanged({"eth0"}, {});
+  svc->Start(kTestPID);
 }
 
 TEST_F(ArcServiceTest, ContainerImpl_Stop) {
@@ -381,8 +438,8 @@
 
   auto svc = NewService(GuestMessage::ARC);
   svc->Start(kTestPID);
-  svc->AddDevice("eth0");
-  svc->RemoveDevice("eth0");
+  svc->OnDevicesChanged({"eth0"}, {});
+  svc->OnDevicesChanged({}, {"eth0"});
 }
 
 // VM Impl
@@ -433,7 +490,7 @@
 
   auto svc = NewService(GuestMessage::ARC_VM);
   svc->Start(kTestPID);
-  svc->AddDevice("eth0");
+  svc->OnDevicesChanged({"eth0"}, {});
 }
 
 TEST_F(ArcServiceTest, VmImpl_StartMultipleDevices) {
@@ -483,9 +540,9 @@
 
   auto svc = NewService(GuestMessage::ARC_VM);
   svc->Start(kTestPID);
-  svc->AddDevice("eth0");
-  svc->AddDevice("wlan0");
-  svc->AddDevice("eth1");
+  svc->OnDevicesChanged({"eth0"}, {});
+  svc->OnDevicesChanged({"wlan0"}, {});
+  svc->OnDevicesChanged({"eth1"}, {});
 }
 
 TEST_F(ArcServiceTest, VmImpl_Stop) {
@@ -553,8 +610,8 @@
 
   auto svc = NewService(GuestMessage::ARC_VM);
   svc->Start(kTestPID);
-  svc->AddDevice("eth0");
-  svc->RemoveDevice("eth0");
+  svc->OnDevicesChanged({"eth0"}, {});
+  svc->OnDevicesChanged({}, {"eth0"});
 }
 
 }  // namespace patchpanel