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_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