You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jason Zhou <ja...@gmail.com> on 2024/07/15 19:26:16 UTC
Review Request 75090: [veth] provide the ability to set veth peer link MAC address on creation
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75090/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Repository: mesos
Description
-------
This addresses the previous todo where we want to set the MAC address
of the peer link when we are creating a veth pair so that we can avoid
the race condition we are racing against udev to see who will set the
MAC address of the interface last.
see: https://reviews.apache.org/r/75087/
Diffs
-----
src/linux/routing/link/veth.hpp 1e54ce696bf219fd83874efb88a7de2e245fe048
src/linux/routing/link/veth.cpp 36aad5fec719e40cb5377ef80ab2c4b54ef66496
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 894d77c5680f5ea4e9c2b54a41e05a6d1aaf10f2
src/tests/containerizer/routing_tests.cpp b18f741e0ef673e275666a4a7a054bd96297590e
Diff: https://reviews.apache.org/r/75090/diff/1/
Testing
-------
Added test to check that veth::create with a target MAC address is able to create a link with that target MAC address, and that we are also able to set the MAC address of the peer link in the in veth::create.
RoutingVethTest and PortMappingIsolatorTest pass (except ROOT_ContainerARPExternal which has always failed)
Thanks,
Jason Zhou
Re: Review Request 75090: [veth] provide the ability to set veth peer link MAC address on creation
Posted by Jason Zhou <ja...@gmail.com>.
> On July 15, 2024, 8:48 p.m., Benjamin Mahler wrote:
> > src/linux/routing/link/veth.cpp
> > Lines 44-56 (patched)
> > <https://reviews.apache.org/r/75090/diff/1/?file=2291553#file2291553line44>
> >
> > Take non-option mac here and return a non-option, but in call sites, you can keep the optional address and use a ternary or if condition to decide to call this function.
done
> On July 15, 2024, 8:48 p.m., Benjamin Mahler wrote:
> > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
> > Lines 3602-3603 (original), 3602-3603 (patched)
> > <https://reviews.apache.org/r/75090/diff/1/?file=2291554#file2291554line3602>
> >
> > remove todo?
done
> On July 15, 2024, 8:48 p.m., Benjamin Mahler wrote:
> > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
> > Line 5181 (original), 5182 (patched)
> > <https://reviews.apache.org/r/75090/diff/1/?file=2291554#file2291554line5182>
> >
> > nit: end w/ period
done
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75090/#review226684
-----------------------------------------------------------
On July 15, 2024, 7:53 p.m., Jason Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/75090/
> -----------------------------------------------------------
>
> (Updated July 15, 2024, 7:53 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10243
> https://issues.apache.org/jira/browse/MESOS-10243
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This addresses the previous todo where we want to set the MAC address
> of the peer link when we are creating a veth pair so that we can avoid
> the race condition we are racing against udev to see who will set the
> MAC address of the interface last.
> see: https://reviews.apache.org/r/75087/
> see: https://issues.apache.org/jira/browse/MESOS-10243
>
>
> Diffs
> -----
>
> src/linux/routing/link/veth.hpp 1e54ce696bf219fd83874efb88a7de2e245fe048
> src/linux/routing/link/veth.cpp 36aad5fec719e40cb5377ef80ab2c4b54ef66496
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 894d77c5680f5ea4e9c2b54a41e05a6d1aaf10f2
> src/tests/containerizer/routing_tests.cpp b18f741e0ef673e275666a4a7a054bd96297590e
>
>
> Diff: https://reviews.apache.org/r/75090/diff/1/
>
>
> Testing
> -------
>
> Added test to check that veth::create with a target MAC address is able to create a link with that target MAC address, and that we are also able to set the MAC address of the peer link in the in veth::create.
> RoutingVethTest and PortMappingIsolatorTest pass (except ROOT_ContainerARPExternal which has always failed)
>
>
> Thanks,
>
> Jason Zhou
>
>
Re: Review Request 75090: [veth] provide the ability to set veth peer link MAC address on creation
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75090/#review226684
-----------------------------------------------------------
src/linux/routing/link/veth.cpp
Lines 44-56 (patched)
<https://reviews.apache.org/r/75090/#comment314950>
Take non-option mac here and return a non-option, but in call sites, you can keep the optional address and use a ternary or if condition to decide to call this function.
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 3602-3603 (original), 3602-3603 (patched)
<https://reviews.apache.org/r/75090/#comment314948>
remove todo?
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Line 5181 (original), 5182 (patched)
<https://reviews.apache.org/r/75090/#comment314949>
nit: end w/ period
- Benjamin Mahler
On July 15, 2024, 7:53 p.m., Jason Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/75090/
> -----------------------------------------------------------
>
> (Updated July 15, 2024, 7:53 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10243
> https://issues.apache.org/jira/browse/MESOS-10243
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This addresses the previous todo where we want to set the MAC address
> of the peer link when we are creating a veth pair so that we can avoid
> the race condition we are racing against udev to see who will set the
> MAC address of the interface last.
> see: https://reviews.apache.org/r/75087/
> see: https://issues.apache.org/jira/browse/MESOS-10243
>
>
> Diffs
> -----
>
> src/linux/routing/link/veth.hpp 1e54ce696bf219fd83874efb88a7de2e245fe048
> src/linux/routing/link/veth.cpp 36aad5fec719e40cb5377ef80ab2c4b54ef66496
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 894d77c5680f5ea4e9c2b54a41e05a6d1aaf10f2
> src/tests/containerizer/routing_tests.cpp b18f741e0ef673e275666a4a7a054bd96297590e
>
>
> Diff: https://reviews.apache.org/r/75090/diff/1/
>
>
> Testing
> -------
>
> Added test to check that veth::create with a target MAC address is able to create a link with that target MAC address, and that we are also able to set the MAC address of the peer link in the in veth::create.
> RoutingVethTest and PortMappingIsolatorTest pass (except ROOT_ContainerARPExternal which has always failed)
>
>
> Thanks,
>
> Jason Zhou
>
>
Re: Review Request 75090: [veth] provide the ability to set veth peer link MAC address on creation
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75090/#review226686
-----------------------------------------------------------
Ship it!
src/linux/routing/link/veth.cpp
Lines 45 (patched)
<https://reviews.apache.org/r/75090/#comment314954>
no need for explicit return type hint now
- Benjamin Mahler
On July 15, 2024, 9:21 p.m., Jason Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/75090/
> -----------------------------------------------------------
>
> (Updated July 15, 2024, 9:21 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10243
> https://issues.apache.org/jira/browse/MESOS-10243
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This addresses the previous todo where we want to set the MAC address
> of the peer link when we are creating a veth pair so that we can avoid
> the race condition we are racing against udev to see who will set the
> MAC address of the interface last.
> see: https://reviews.apache.org/r/75087/
> see: https://issues.apache.org/jira/browse/MESOS-10243
>
>
> Diffs
> -----
>
> src/linux/routing/link/veth.hpp 1e54ce696bf219fd83874efb88a7de2e245fe048
> src/linux/routing/link/veth.cpp 36aad5fec719e40cb5377ef80ab2c4b54ef66496
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 894d77c5680f5ea4e9c2b54a41e05a6d1aaf10f2
> src/tests/containerizer/routing_tests.cpp b18f741e0ef673e275666a4a7a054bd96297590e
>
>
> Diff: https://reviews.apache.org/r/75090/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, and that we are also able to set the MAC address of the peer link in the in veth::create.
> RoutingVethTest and PortMappingIsolatorTest pass (except ROOT_ContainerARPExternal which has always failed)
>
>
> Thanks,
>
> Jason Zhou
>
>
Re: Review Request 75090: [veth] provide the ability to set veth peer link MAC address on creation
Posted by Jason Zhou <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75090/
-----------------------------------------------------------
(Updated July 15, 2024, 9:21 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Address review feedback
Bugs: MESOS-10243
https://issues.apache.org/jira/browse/MESOS-10243
Repository: mesos
Description (updated)
-------
This addresses the previous todo where we want to set the MAC address
of the peer link when we are creating a veth pair so that we can avoid
the race condition we are racing against udev to see who will set the
MAC address of the interface last.
see: https://reviews.apache.org/r/75087/
Diffs (updated)
-----
src/linux/routing/link/veth.hpp 1e54ce696bf219fd83874efb88a7de2e245fe048
src/linux/routing/link/veth.cpp 36aad5fec719e40cb5377ef80ab2c4b54ef66496
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 894d77c5680f5ea4e9c2b54a41e05a6d1aaf10f2
src/tests/containerizer/routing_tests.cpp b18f741e0ef673e275666a4a7a054bd96297590e
Diff: https://reviews.apache.org/r/75090/diff/2/
Changes: https://reviews.apache.org/r/75090/diff/1-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, and that we are also able to set the MAC address of the peer link in the in veth::create.
RoutingVethTest and PortMappingIsolatorTest pass (except ROOT_ContainerARPExternal which has always failed)
Thanks,
Jason Zhou
Re: Review Request 75090: [veth] provide the ability to set veth peer link MAC address on creation
Posted by Jason Zhou <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75090/
-----------------------------------------------------------
(Updated July 15, 2024, 7:53 p.m.)
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-10243
https://issues.apache.org/jira/browse/MESOS-10243
Repository: mesos
Description (updated)
-------
This addresses the previous todo where we want to set the MAC address
of the peer link when we are creating a veth pair so that we can avoid
the race condition we are racing against udev to see who will set the
MAC address of the interface last.
see: https://reviews.apache.org/r/75087/
see: https://issues.apache.org/jira/browse/MESOS-10243
Diffs
-----
src/linux/routing/link/veth.hpp 1e54ce696bf219fd83874efb88a7de2e245fe048
src/linux/routing/link/veth.cpp 36aad5fec719e40cb5377ef80ab2c4b54ef66496
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 894d77c5680f5ea4e9c2b54a41e05a6d1aaf10f2
src/tests/containerizer/routing_tests.cpp b18f741e0ef673e275666a4a7a054bd96297590e
Diff: https://reviews.apache.org/r/75090/diff/1/
Testing
-------
Added test to check that veth::create with a target MAC address is able to create a link with that target MAC address, and that we are also able to set the MAC address of the peer link in the in veth::create.
RoutingVethTest and PortMappingIsolatorTest pass (except ROOT_ContainerARPExternal which has always failed)
Thanks,
Jason Zhou
Re: Review Request 75090: [veth] provide the ability to set veth peer link MAC address on creation
Posted by Jason Zhou <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75090/
-----------------------------------------------------------
(Updated July 15, 2024, 7:53 p.m.)
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-10243
https://issues.apache.org/jira/browse/MESOS-10243
Repository: mesos
Description
-------
This addresses the previous todo where we want to set the MAC address
of the peer link when we are creating a veth pair so that we can avoid
the race condition we are racing against udev to see who will set the
MAC address of the interface last.
see: https://reviews.apache.org/r/75087/
Diffs
-----
src/linux/routing/link/veth.hpp 1e54ce696bf219fd83874efb88a7de2e245fe048
src/linux/routing/link/veth.cpp 36aad5fec719e40cb5377ef80ab2c4b54ef66496
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 894d77c5680f5ea4e9c2b54a41e05a6d1aaf10f2
src/tests/containerizer/routing_tests.cpp b18f741e0ef673e275666a4a7a054bd96297590e
Diff: https://reviews.apache.org/r/75090/diff/1/
Testing
-------
Added test to check that veth::create with a target MAC address is able to create a link with that target MAC address, and that we are also able to set the MAC address of the peer link in the in veth::create.
RoutingVethTest and PortMappingIsolatorTest pass (except ROOT_ContainerARPExternal which has always failed)
Thanks,
Jason Zhou