-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75086/#review226682
-----------------------------------------------------------
Ship it!
local edits:
```
diff --git a/src/linux/routing/link/veth.cpp b/src/linux/routing/link/veth.cpp
index be433744b..36aad5fec 100644
--- a/src/linux/routing/link/veth.cpp
+++ b/src/linux/routing/link/veth.cpp
@@ -46,18 +46,6 @@ Try<bool> create(
}
struct nl_sock *sock = socket->get();
- const char *name = veth.c_str();
- const char *peer_name = peer.c_str();
- unsigned char mac_addr_uchar[6];
-
- if (veth_mac.isSome()) {
- // Copy values from uint8_t array to unsigned char array
- for (int i = 0; i < 6; i++) {
- mac_addr_uchar[i] = (unsigned char) (*veth_mac)[i];
- }
- }
-
-
struct rtnl_link *link, *peer_link;
int error = -NLE_NOMEM;
@@ -67,17 +55,18 @@ Try<bool> create(
peer_link = rtnl_link_veth_get_peer(link);
struct nl_addr *addr;
+
if (veth_mac.isSome()) {
- addr = nl_addr_build(AF_LLC, mac_addr_uchar, 6);
- rtnl_link_set_addr(link, addr);
+ unsigned char mac_addr_uchar[6];
+ for (int i = 0; i < 6; i++) {
+ mac_addr_uchar[i] = (unsigned char) (*veth_mac)[i];
+ }
+ addr = nl_addr_build(AF_LLC, mac_addr_uchar, 6);
+ rtnl_link_set_addr(link, addr);
}
- if (name) {
- rtnl_link_set_name(link, name);
- }
- if (peer_name) {
- rtnl_link_set_name(peer_link, peer_name);
- }
+ rtnl_link_set_name(link, veth.c_str());
+ rtnl_link_set_name(peer_link, peer.c_str());
rtnl_link_set_ns_pid(peer_link, pid.getOrElse(getpid()));
error = rtnl_link_add(sock, link, NLM_F_CREATE | NLM_F_EXCL);
diff --git a/src/linux/routing/link/veth.hpp b/src/linux/routing/link/veth.hpp
index 822f40256..1e54ce696 100644
--- a/src/linux/routing/link/veth.hpp
+++ b/src/linux/routing/link/veth.hpp
@@ -35,13 +35,14 @@ namespace veth {
// caller's network namespace. Returns false if the virtual network
// links (with the same name) already exist.
//
-// We provide the ability to assign a specified MAC to the veth device on
-// creation via veth_mac
+// We provide the ability to assign a specified MAC to the host network
+// namespace veth device on creation via veth_mac:
+//
// In systems with systemd version above 242, there is a potential data race
// where udev will try to update the MAC address of the device at the same
// time as us if the systemd's MacAddressPolicy is set to 'persistent'.
// To prevent udev from trying to set the veth device's MAC address by itself,
-// we must set the device MAC address on creation so that addr_assign_type
+// we *must* set the device MAC address on creation so that addr_assign_type
// will be set to NET_ADDR_SET, which prevents udev from attempting to
// change the MAC address of the veth device.
// see:
https://github.com/torvalds/linux/commit/2afb9b533423a9b97f84181e773cf9361d98fed6
diff --git a/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
b/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
index 28d967bd2..894d77c56 100644
--- a/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
+++ b/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
@@ -3595,8 +3595,12 @@ Future<Nothing> PortMappingIsolatorProcess::isolate(
LOG(INFO) << "Created network namespace handle symlink '"
<< linker << "' -> '" << target << "'";
- // Create a virtual ethernet pair for this container, also set its MAC
- // address to match host public interface eth0
+ // Create a virtual ethernet pair for this container, also set the MAC
+ // address of the host network namespace veth<pid> interface to match
+ // that of the host network namespace eth0.
+ //
+ // TODO(jasonzhou): Also set the mac address of the container network
+ // namespace eth0 here on creation, to avoid the race with udev: MESOS-10243.
Try<bool> createVethPair = link::veth::create(veth(pid), eth0, pid, hostMAC);
if (createVethPair.isError()) {
return Failure(
diff --git a/src/tests/containerizer/routing_tests.cpp
b/src/tests/containerizer/routing_tests.cpp
index f07c8f8ea..b18f741e0 100644
--- a/src/tests/containerizer/routing_tests.cpp
+++ b/src/tests/containerizer/routing_tests.cpp
@@ -260,7 +260,8 @@ protected:
TEST_F(RoutingVethTest, ROOT_LinkCreate)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -269,13 +270,18 @@ TEST_F(RoutingVethTest, ROOT_LinkCreate)
EXPECT_SOME_NE(0, link::index(TEST_PEER_LINK));
// Test the case where the veth (with the same name) already exists.
- EXPECT_SOME_FALSE(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ EXPECT_SOME_FALSE(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
}
+
TEST_F(RoutingVethTest, ROOT_LinkCreateWithTargetMAC)
{
uint8_t bytes[6] = {0x12, 0x34, 0x56, 0x78, 0x9a, 0xbc};
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
net::MAC(bytes)));
+ net::MAC target_mac = net::MAC(bytes);
+
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), target_mac));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -286,16 +292,18 @@ TEST_F(RoutingVethTest, ROOT_LinkCreateWithTargetMAC)
Result<net::MAC> mac = net::mac(TEST_VETH_LINK);
ASSERT_SOME(mac);
- EXPECT_EQ(mac.get(), net::MAC(bytes));
+ EXPECT_EQ(*mac, target_mac);
// Test the case where the veth (with the same name) already exists.
- EXPECT_SOME_FALSE(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ EXPECT_SOME_FALSE(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
}
TEST_F(RoutingVethTest, ROOT_LinkRemove)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::remove(TEST_VETH_LINK));
EXPECT_SOME_FALSE(link::remove(TEST_VETH_LINK));
@@ -354,7 +362,8 @@ TEST_F(RoutingVethTest, ROOT_LinkWait)
{
AWAIT_READY(link::removed(TEST_VETH_LINK));
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -369,7 +378,8 @@ TEST_F(RoutingVethTest, ROOT_LinkWait)
TEST_F(RoutingVethTest, ROOT_LinkSetUp)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -389,7 +399,8 @@ TEST_F(RoutingVethTest, ROOT_LinkSetUp)
TEST_F(RoutingVethTest, ROOT_LinkSetMAC)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -420,7 +431,8 @@ TEST_F(RoutingVethTest, ROOT_LinkSetMAC)
TEST_F(RoutingVethTest, ROOT_LinkMTU)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -441,7 +453,8 @@ TEST_F(RoutingVethTest, ROOT_IngressQdisc)
// Test for a qdisc on a nonexistent interface should fail.
EXPECT_SOME_FALSE(ingress::exists("noSuchInterface"));
- EXPECT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ EXPECT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -505,7 +518,8 @@ TEST_F(RoutingVethTest, ROOT_HTBQdisc)
// Test for a qdisc on a nonexistent interface should fail.
EXPECT_SOME_FALSE(htb::exists("noSuchInterface", EGRESS_ROOT));
- EXPECT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ EXPECT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -589,7 +603,8 @@ TEST_F(RoutingVethTest, ROOT_FqCodeQdisc)
// Test for a qdisc on a nonexistent interface should fail.
EXPECT_SOME_FALSE(fq_codel::exists("noSuchInterface", EGRESS_ROOT));
- EXPECT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ EXPECT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -670,7 +685,8 @@ TEST_F(RoutingVethTest, ROOT_FqCodeQdisc)
TEST_F(RoutingVethTest, ROOT_FqCodelClassifier)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -746,7 +762,8 @@ TEST_F(RoutingVethTest, ROOT_FqCodelClassifier)
TEST_F(RoutingVethTest, ROOT_ARPFilterCreate)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -766,7 +783,8 @@ TEST_F(RoutingVethTest, ROOT_ARPFilterCreate)
TEST_F(RoutingVethTest, ROOT_ARPFilterCreateDuplicated)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -796,7 +814,8 @@ TEST_F(RoutingVethTest, ROOT_ARPFilterCreateDuplicated)
TEST_F(RoutingVethTest, ROOT_ARPFilterRemove)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -821,7 +840,8 @@ TEST_F(RoutingVethTest, ROOT_ARPFilterRemove)
TEST_F(RoutingVethTest, ROOT_ARPFilterUpdate)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -858,7 +878,8 @@ TEST_F(RoutingVethTest, ROOT_ARPFilterUpdate)
TEST_F(RoutingVethTest, ROOT_ICMPFilterCreate)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -890,7 +911,8 @@ TEST_F(RoutingVethTest, ROOT_ICMPFilterCreate)
TEST_F(RoutingVethTest, ROOT_ICMPFilterCreateDuplicated)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -923,7 +945,8 @@ TEST_F(RoutingVethTest, ROOT_ICMPFilterCreateDuplicated)
TEST_F(RoutingVethTest, ROOT_ICMPFilterCreateMultiple)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -993,7 +1016,8 @@ TEST_F(RoutingVethTest, ROOT_ICMPFilterRemove)
TEST_F(RoutingVethTest, ROOT_ICMPFilterUpdate)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -1049,7 +1073,8 @@ TEST_F(RoutingVethTest, ROOT_ICMPFilterUpdate)
TEST_F(RoutingVethTest, ROOT_IPFilterCreate)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -1107,7 +1132,8 @@ TEST_F(RoutingVethTest, ROOT_IPFilterCreate)
TEST_F(RoutingVethTest, ROOT_IPFilterCreate2)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -1142,7 +1168,8 @@ TEST_F(RoutingVethTest, ROOT_IPFilterCreate2)
TEST_F(RoutingVethTest, ROOT_IPFilterCreateDuplicated)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -1191,7 +1218,8 @@ TEST_F(RoutingVethTest, ROOT_IPFilterCreateDuplicated)
TEST_F(RoutingVethTest, ROOT_IPFilterCreateMultiple)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -1283,7 +1311,8 @@ TEST_F(RoutingVethTest, ROOT_IPFilterCreateMultiple)
TEST_F(RoutingVethTest, ROOT_IPFilterRemove)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
@@ -1360,7 +1389,8 @@ TEST_F(RoutingVethTest, ROOT_IPFilterRemove)
// Test the workaround introduced for MESOS-1617.
TEST_F(RoutingVethTest, ROOT_HandleGeneration)
{
- ASSERT_SOME(link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(),
None()));
+ ASSERT_SOME(
+ link::veth::create(TEST_VETH_LINK, TEST_PEER_LINK, None(), None()));
EXPECT_SOME_TRUE(link::exists(TEST_VETH_LINK));
EXPECT_SOME_TRUE(link::exists(TEST_PEER_LINK));
```
- Benjamin Mahler
On July 15, 2024, 2:42 p.m., Jason Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/75086/
> -----------------------------------------------------------
>
> (Updated July 15, 2024, 2:42 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10243
> https://issues.apache.org/jira/browse/MESOS-10243
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In systems with systemd version above 242, there is a potential data
> race where udev will try to update the MAC address of the device at the
> same time as us if the systemd's MacAddressPolicy is set to 'persistent'.
> To prevent udev from trying to set the veth device's MAC address by
> itself, we must set the device MAC address on creation so that
> addr_assign_type will be set to NET_ADDR_SET, which prevents udev from
> attempting to change the MAC address of the veth device.
> see:
> https://github.com/torvalds/linux/commit/2afb9b533423a9b97f84181e773cf9361d98fed6
> see:
> https://lore.kernel.org/netdev/cahxsexy8lkzocbdbzss_vjopc_tqmyzm87kc192hpmuhmcq...@mail.gmail.com/T/
> see: https://issues.apache.org/jira/browse/MESOS-10243
>
>
> Diffs
> -----
>
> src/linux/routing/link/link.hpp 09790f31711cf271567176d3d67ed51025d2d712
> src/linux/routing/link/link.cpp bb97e328175dea5c0937defce2e234b0de3cfe32
> src/linux/routing/link/veth.hpp a4acbe9b5c7dd77f3736fc5348743081c04cabf1
> src/linux/routing/link/veth.cpp 6aeb95e710098056b464c2088b0b60cdd3fe3f65
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
> 3b3b899fb43364f545eb9748ab4215fd5b2e2895
> src/tests/containerizer/routing_tests.cpp
> c9fa2e86a52873575c77bd0984d4eaba34282eff
>
>
> Diff: https://reviews.apache.org/r/75086/diff/4/
>
>
> Testing
> -------
>
> Added test to check that veth::create with a target MAC address is able to
> create a link with that target MAC address.
> RoutingVethTest and PortMappingIsolatorTest pass (except
> ROOT_ContainerARPExternal which has always failed)
>
>
> Thanks,
>
> Jason Zhou
>
>
