> On July 15, 2024, 1:55 a.m., Benjamin Mahler wrote: > > src/linux/routing/link/veth.hpp > > Lines 41 (patched) > > <https://reviews.apache.org/r/75086/diff/2/?file=2291468#file2291468line41> > > > > 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?
done > On July 15, 2024, 1:55 a.m., Benjamin Mahler wrote: > > src/linux/routing/link/veth.cpp > > Line 48 (original), 51 (patched) > > <https://reviews.apache.org/r/75086/diff/2/?file=2291469#file2291469line51> > > > > why not inline this one? > > > > also, you can do: > > > > ``` > > rtnl_link_set_ns_pid(peer_link, pid.getOrElse(getpid())); > > ``` done > On July 15, 2024, 1:55 a.m., Benjamin Mahler wrote: > > src/linux/routing/link/veth.cpp > > Lines 63 (patched) > > <https://reviews.apache.org/r/75086/diff/2/?file=2291469#file2291469line63> > > > > nit: stick with "error" as before done > On July 15, 2024, 1:55 a.m., Benjamin Mahler wrote: > > src/linux/routing/link/veth.cpp > > Lines 66 (patched) > > <https://reviews.apache.org/r/75086/diff/2/?file=2291469#file2291469line66> > > > > 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)) done, missed this when copy pasting from libnl > On July 15, 2024, 1:55 a.m., Benjamin Mahler wrote: > > src/linux/routing/link/veth.cpp > > Lines 69-77 (patched) > > <https://reviews.apache.org/r/75086/diff/2/?file=2291469#file2291469line69> > > > > it might be good to document on setMAC's interface that one should be > > careful using it due to this race done > On July 15, 2024, 1:55 a.m., Benjamin Mahler wrote: > > src/linux/routing/link/veth.cpp > > Lines 92 (patched) > > <https://reviews.apache.org/r/75086/diff/2/?file=2291469#file2291469line92> > > > > nit: indentation has gone wild here? stray tab character > On July 15, 2024, 1:55 a.m., Benjamin Mahler wrote: > > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp > > Lines 3626-3633 (original) > > <https://reviews.apache.org/r/75086/diff/2/?file=2291470#file2291470line3627> > > > > do you plan to remove the "workaround" logic inside link::setMAC in a > > separate patch? I'll undo the workaround logic in this patch - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/75086/#review226677 ----------------------------------------------------------- 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. > > > 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/ > > > 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 > >
