----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74871/#review226248 -----------------------------------------------------------
src/tests/environment.cpp Line 197 (original), 198 (patched) <https://reviews.apache.org/r/74871/#comment314486> This one is matching Cgroups2 tests by accident and will potentially disable them? We could either combine the v1 and v2 filters, or make sure that this one doesn't accidentally look at Cgroups2 tests src/tests/environment.cpp Lines 225-228 (patched) <https://reviews.apache.org/r/74871/#comment314488> `Cgroups2Filter() {}` Taking a look at this file, it seems the approach is generally to do anything potentially "expensive" in the constructor then cheaply checking the result in the disable function, I don't think we need to do that here *if* we gate any of the system calls etc behind the cheap string match (same as the v1 filter logic): ``` if (matches(test, "CGROUPS2_") || ...) { // logic to check for a cgroups2 test } ``` Right now you have the cgroups2 checking happening for every test, even if they aren't related to cgroups2 src/tests/environment.cpp Lines 234 (patched) <https://reviews.apache.org/r/74871/#comment314487> nit: no whitespace in front of the ifdef clauses src/tests/environment.cpp Lines 237 (patched) <https://reviews.apache.org/r/74871/#comment314489> nbd, but this could be `*user == "root"` - Benjamin Mahler On Feb. 27, 2024, 2:52 p.m., Devin Leamy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74871/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2024, 2:52 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > - Introduces cgroups2_tests.cpp which will hold the cgroups2 tests. > - Introduces Cgroups2Filter that will enable cgroups2 tests only if > cgroups2 is enabled on the system and the user is "root". > - Adds a test that checks that cgroups2 is enabled on the testing host. > > > Diffs > ----- > > src/Makefile.am 03aa881e84cf19e95f573ad9d1df3f08a033ef3a > src/tests/CMakeLists.txt 6beb74e382acbda5bca816709312b00785673c4a > src/tests/containerizer/cgroups2_tests.cpp PRE-CREATION > src/tests/environment.cpp 737c2d916bed0449536dc754249d1b7708aeb681 > > > Diff: https://reviews.apache.org/r/74871/diff/5/ > > > Testing > ------- > > make -j128 check > > > Thanks, > > Devin Leamy > >
