----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/75074/#review226769 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/gpu/isolator.cpp Lines 291 (patched) <https://reviews.apache.org/r/75074/#comment315043> nit: always place .then on the next line (look around for example formatting) src/slave/containerizer/mesos/isolators/gpu/isolator.cpp Lines 292 (patched) <https://reviews.apache.org/r/75074/#comment315044> nit: 4 space indent on paren wrapping src/slave/containerizer/mesos/isolators/gpu/isolator.cpp Lines 335 (patched) <https://reviews.apache.org/r/75074/#comment315045> nit: remove extra newline src/slave/containerizer/mesos/isolators/gpu/isolator.cpp Lines 337-339 (patched) <https://reviews.apache.org/r/75074/#comment315046> comment on what this means? and why it's skipped? src/slave/containerizer/mesos/isolators/gpu/isolator.cpp Lines 340-341 (patched) <https://reviews.apache.org/r/75074/#comment315047> nit: take a const ref rather than copy? src/slave/containerizer/mesos/isolators/gpu/isolator.cpp Lines 380 (patched) <https://reviews.apache.org/r/75074/#comment315048> in general if taking a mutable, we would take a pointer, however, why not just return these futures here? or inline this before the collect? src/slave/containerizer/mesos/isolators/gpu/isolator.cpp Lines 380 (patched) <https://reviews.apache.org/r/75074/#comment315049> in general if taking a mutable, we would take a pointer, however, why not just return these futures here? or inline this before the collect? src/slave/containerizer/mesos/isolators/gpu/isolator.cpp Lines 425-430 (patched) <https://reviews.apache.org/r/75074/#comment315050> can you just use controlDeviceEntries.entries() inline rather than the loop here? or initialize a vector with .begin() / .end() src/slave/containerizer/mesos/isolators/gpu/isolator.cpp Lines 681-685 (patched) <https://reviews.apache.org/r/75074/#comment315051> seems like this could be shortened with an asDeviceEntries? - Benjamin Mahler On Aug. 2, 2024, 7:13 p.m., Jason Zhou wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/75074/ > ----------------------------------------------------------- > > (Updated Aug. 2, 2024, 7:13 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > Currently, the GPU isolator assumes we are only using cgroups v1, and > makes use of the cgroups::devices::allow and deny functions to control > GPU access. > > In Cgroups2, we need to attach ebpf programs for the specific cgroups, > which is done for us in the DeviceManager. Hence, we need to use the > DeviceManager in the GPU isolator depending on whether cgroups v1 or v2 > is currently mounted > > > Diffs > ----- > > src/slave/containerizer/mesos/containerizer.cpp > 9bafb137ba67bfed5cfb24c1a3af9203fde9e821 > src/slave/containerizer/mesos/isolators/gpu/isolator.hpp > e4f221d5f63ed1db044acabbbd427a30a0f69ced > src/slave/containerizer/mesos/isolators/gpu/isolator.cpp > 99119f938e2eb5ca6a8b64d073c87ca5032a00b8 > > > Diff: https://reviews.apache.org/r/75074/diff/6/ > > > Testing > ------- > > Existing GPU isolator tests pass, with the exception of the > DefaultExecutorVerifyDeviceAccess test which is because we currently don't > support nested containers. > > > Thanks, > > Jason Zhou > >
