----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/75086/#review226677 -----------------------------------------------------------
per my comment on another patch: mind creating a MESOS ticket for this issue and linking it in the Bugs field? Also mentioning it in the description would be good. src/linux/routing/link/veth.hpp Lines 41 (patched) <https://reviews.apache.org/r/75086/#comment314929> let's say which mac this is, so `veth_mac`? also, let's explain in the comment on this function? maybe explain why we provide the ability to set the mac address on creation? src/linux/routing/link/veth.cpp Line 48 (original), 51 (patched) <https://reviews.apache.org/r/75086/#comment314933> why not inline this one? also, you can do: ``` rtnl_link_set_ns_pid(peer_link, pid.getOrElse(getpid())); ``` src/linux/routing/link/veth.cpp Lines 63 (patched) <https://reviews.apache.org/r/75086/#comment314934> nit: stick with "error" as before src/linux/routing/link/veth.cpp Lines 66 (patched) <https://reviews.apache.org/r/75086/#comment314935> you're returning an integer from a Try<bool> function which might be implicitly casting your int to a bool, whereas you want the Try to be an error this needs something like: return Error(proper_rtnl_strerror(NLE_NOMEM)) src/linux/routing/link/veth.cpp Lines 69-77 (patched) <https://reviews.apache.org/r/75086/#comment314936> it might be good to document on setMAC's interface that one should be careful using it due to this race src/linux/routing/link/veth.cpp Lines 92 (patched) <https://reviews.apache.org/r/75086/#comment314931> nit: indentation has gone wild here? src/slave/containerizer/mesos/isolators/network/port_mapping.cpp Lines 3626-3633 (original) <https://reviews.apache.org/r/75086/#comment314932> do you plan to remove the "workaround" logic inside link::setMAC in a separate patch? - Benjamin Mahler On July 12, 2024, 9:09 p.m., Jason Zhou wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/75086/ > ----------------------------------------------------------- > > (Updated July 12, 2024, 9:09 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > 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/ > > > Diffs > ----- > > 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/2/ > > > 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 > >
