-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75006/#review226708
-----------------------------------------------------------
Fix it, then Ship it!
Local edits:
```
diff --git a/src/slave/containerizer/device_manager/device_manager.cpp
b/src/slave/containerizer/device_manager/device_manager.cpp
index c1cfdee0e..a392000ea 100644
--- a/src/slave/containerizer/device_manager/device_manager.cpp
+++ b/src/slave/containerizer/device_manager/device_manager.cpp
@@ -17,30 +17,20 @@
#include <algorithm>
#include <string>
-#include <process/async.hpp>
#include <process/dispatch.hpp>
#include <process/future.hpp>
+#include <process/id.hpp>
#include <process/process.hpp>
+
#include <stout/foreach.hpp>
-#include <stout/os/exists.hpp>
#include <stout/stringify.hpp>
-#include "common/values.hpp"
-
-#ifdef __linux__
-#include "linux/fs.hpp"
-#endif // __linux__
-
-#include "linux/cgroups2.hpp"
#include "slave/containerizer/device_manager/device_manager.hpp"
#include "slave/paths.hpp"
-#include "slave/state.hpp"
-using std::pair;
using std::string;
using std::vector;
-using process::async;
using process::dispatch;
using process::Failure;
using process::Future;
@@ -52,142 +42,115 @@ namespace mesos {
namespace internal {
namespace slave {
+
+vector<Entry> convert_to_entries(
+ const vector<DeviceManager::NonWildcardEntry>& non_wildcards_entries)
+{
+ vector<Entry> entries = {};
+ foreach (const DeviceManager::NonWildcardEntry& non_wildcards_entry,
+ non_wildcards_entries) {
+ Entry entry;
+ entry.access = non_wildcards_entry.access;
+ entry.selector.type = [&]() {
+ switch (non_wildcards_entry.selector.type) {
+ case DeviceManager::NonWildcardEntry::Selector::Type::BLOCK:
+ return Entry::Selector::Type::BLOCK;
+ case DeviceManager::NonWildcardEntry::Selector::Type::CHARACTER:
+ return Entry::Selector::Type::CHARACTER;
+ }
+ }();
+ entry.selector.major = non_wildcards_entry.selector.major;
+ entry.selector.minor = non_wildcards_entry.selector.minor;
+ entries.push_back(entry);
+ }
+ return entries;
+}
+
+
class DeviceManagerProcess : public process::Process<DeviceManagerProcess>
{
public:
DeviceManagerProcess(const string& work_dir)
: ProcessBase(process::ID::generate("device-manager")),
- meta_dir(paths::getMetaRootDir(work_dir))
- {
- }
-
+ meta_dir(paths::getMetaRootDir(work_dir)) {}
Future<Nothing> configure(
- const string& cgroup,
- const vector<Entry>& allow_list,
- const vector<DeviceManager::NonWildcardEntry>& deny_list)
+ const string& cgroup,
+ const vector<Entry>& allow_list,
+ const vector<DeviceManager::NonWildcardEntry>& non_wildcard_deny_list)
{
- vector<Entry> converted_deny_list =
- DeviceManager::NonWildcardEntry::convert_to_entries(deny_list);
+ vector<Entry> deny_list = convert_to_entries(non_wildcard_deny_list);
foreach (const Entry& allow_entry, allow_list) {
- foreach (const Entry& deny_entry, converted_deny_list) {
+ foreach (const Entry& deny_entry, deny_list) {
if (deny_entry.encompasses(allow_entry)) {
return Failure(
- "Failed to configure allow and deny devices: allow entry: " +
- stringify(allow_entry) +
- " cannot be encompassed by deny entry: " + stringify(deny_entry));
+ "Failed to configure allow and deny devices:"
+ " allow entry '" + stringify(allow_entry) + "' cannot be"
+ " encompassed by deny entry '" + stringify(deny_entry) + "'");
}
}
}
+
device_access_per_cgroup[cgroup].allow_list = allow_list;
- device_access_per_cgroup[cgroup].deny_list = converted_deny_list;
+ device_access_per_cgroup[cgroup].deny_list = deny_list;
return Nothing();
}
-
Future<Nothing> reconfigure(
- const string& cgroup,
- const vector<DeviceManager::NonWildcardEntry>& non_wildcard_additions,
- const vector<DeviceManager::NonWildcardEntry>& non_wildcard_removals)
+ const string& cgroup,
+ const vector<DeviceManager::NonWildcardEntry>& non_wildcard_additions,
+ const vector<DeviceManager::NonWildcardEntry>& non_wildcard_removals)
{
- vector<Entry> additions =
- DeviceManager::NonWildcardEntry::convert_to_entries(
- non_wildcard_additions);
- vector<Entry> removals =
- DeviceManager::NonWildcardEntry::convert_to_entries(
- non_wildcard_removals);
+ vector<Entry> additions = convert_to_entries(non_wildcard_additions);
+ vector<Entry> removals = convert_to_entries(non_wildcard_removals);
foreach (const Entry& addition, additions) {
foreach (const Entry& removal, removals) {
if (removal.encompasses(addition)) {
return Failure(
- "Failed to configure allow and deny devices: addition: " +
- stringify(addition) +
- " cannot be encompassed by removal: " + stringify(removal));
+ "Failed to configure allow and deny devices:"
+ " addition '" + stringify(addition) + "' cannot be"
+ " encompassed by removal '" + stringify(removal) + "'");
}
}
}
+
device_access_per_cgroup[cgroup] = DeviceManager::apply_diff(
- device_access_per_cgroup[cgroup],
- non_wildcard_additions,
- non_wildcard_removals);
+ device_access_per_cgroup[cgroup],
+ non_wildcard_additions,
+ non_wildcard_removals);
+
return Nothing();
}
-
- hashmap<std::string, DeviceManager::CgroupDeviceAccess> state() const
+ hashmap<string, DeviceManager::CgroupDeviceAccess> state() const
{
return device_access_per_cgroup;
}
-
DeviceManager::CgroupDeviceAccess state(const string& cgroup) const
{
return device_access_per_cgroup.contains(cgroup)
- ? device_access_per_cgroup.at(cgroup)
- : DeviceManager::CgroupDeviceAccess();
+ ? device_access_per_cgroup.at(cgroup)
+ : DeviceManager::CgroupDeviceAccess();
}
-
private:
- const std::string meta_dir;
+ const string meta_dir;
- hashmap<std::string, DeviceManager::CgroupDeviceAccess>
- device_access_per_cgroup;
+ hashmap<string, DeviceManager::CgroupDeviceAccess> device_access_per_cgroup;
};
-Try<vector<DeviceManager::NonWildcardEntry>>
-DeviceManager::NonWildcardEntry::convert_to_non_wildcards(
- const vector<Entry>& entries)
-{
- vector<DeviceManager::NonWildcardEntry> non_wildcards = {};
- foreach (const Entry& entry, entries) {
- if (entry.selector.has_wildcard()) {
- return Error("Entry cannot have wildcard");
- }
- DeviceManager::NonWildcardEntry non_wildcard;
- non_wildcard.access = entry.access;
- non_wildcard.selector.major = *entry.selector.major;
- non_wildcard.selector.minor = *entry.selector.minor;
- non_wildcard.selector.type =
- entry.selector.type == cgroups::devices::Entry::Selector::Type::BLOCK
- ? NonWildcardEntry::Selector::Type::BLOCK
- : NonWildcardEntry::Selector::Type::CHARACTER;
- non_wildcards.push_back(non_wildcard);
- }
- return non_wildcards;
-}
-
-vector<Entry> DeviceManager::NonWildcardEntry::convert_to_entries(
- const vector<DeviceManager::NonWildcardEntry>& non_wildcards)
-{
- vector<Entry> entries = {};
- foreach (const DeviceManager::NonWildcardEntry non_wildcard, non_wildcards) {
- Entry entry;
- entry.access = non_wildcard.access;
- entry.selector.type =
- non_wildcard.selector.type ==
- DeviceManager::NonWildcardEntry::Selector::Type::BLOCK
- ? Entry::Selector::Type::BLOCK
- : Entry::Selector::Type::CHARACTER;
- entry.selector.major = non_wildcard.selector.major;
- entry.selector.minor = non_wildcard.selector.minor;
- entries.push_back(entry);
- }
- return entries;
-}
-
-
Try<DeviceManager*> DeviceManager::create(const Flags& flags)
{
return new DeviceManager(
- Owned<DeviceManagerProcess>(new DeviceManagerProcess(flags.work_dir)));
+ Owned<DeviceManagerProcess>(new DeviceManagerProcess(flags.work_dir)));
}
DeviceManager::DeviceManager(
- const process::Owned<DeviceManagerProcess>& _process)
+ const Owned<DeviceManagerProcess>& _process)
: process(_process)
{
spawn(*process);
@@ -202,85 +165,77 @@ DeviceManager::~DeviceManager()
Future<Nothing> DeviceManager::reconfigure(
- const string& cgroup,
- const vector<DeviceManager::NonWildcardEntry>& additions,
- const vector<DeviceManager::NonWildcardEntry>& removals)
+ const string& cgroup,
+ const vector<DeviceManager::NonWildcardEntry>& additions,
+ const vector<DeviceManager::NonWildcardEntry>& removals)
{
return dispatch(
- *process, &DeviceManagerProcess::reconfigure, cgroup, additions, removals);
+ *process,
+ &DeviceManagerProcess::reconfigure,
+ cgroup,
+ additions,
+ removals);
}
Future<Nothing> DeviceManager::configure(
- const string& cgroup,
- const vector<Entry>& allow_list,
- const vector<DeviceManager::NonWildcardEntry>& deny_list)
+ const string& cgroup,
+ const vector<Entry>& allow_list,
+ const vector<DeviceManager::NonWildcardEntry>& deny_list)
{
return dispatch(
- *process, &DeviceManagerProcess::configure, cgroup, allow_list, deny_list);
+ *process,
+ &DeviceManagerProcess::configure,
+ cgroup,
+ allow_list,
+ deny_list);
}
-hashmap<std::string, DeviceManager::CgroupDeviceAccess> DeviceManager::state()
- const
+hashmap<string, DeviceManager::CgroupDeviceAccess> DeviceManager::state() const
{
return process->state();
}
DeviceManager::CgroupDeviceAccess DeviceManager::state(
- const string& cgroup) const
+ const string& cgroup) const
{
return process->state(cgroup);
}
DeviceManager::CgroupDeviceAccess DeviceManager::apply_diff(
- const DeviceManager::CgroupDeviceAccess& old_state,
- const vector<DeviceManager::NonWildcardEntry>& non_wildcard_additions,
- const vector<DeviceManager::NonWildcardEntry>& non_wildcard_removals)
+ const DeviceManager::CgroupDeviceAccess& old_state,
+ const vector<DeviceManager::NonWildcardEntry>& non_wildcard_additions,
+ const vector<DeviceManager::NonWildcardEntry>& non_wildcard_removals)
{
- auto get_access_not_none_entries = [](const vector<Entry>& entries) {
- vector<Entry> res = {};
- foreach (const Entry& entry, entries) {
- if (!entry.access.none()) {
- res.push_back(entry);
- };
+ auto revoke_accesses = [](Entry* entry, const Entry& diff_entry) {
+ CHECK(!entry->selector.has_wildcard());
+ CHECK(!diff_entry.selector.has_wildcard());
+
+ if (entry->selector.major == diff_entry.selector.major
+ && entry->selector.minor == diff_entry.selector.minor
+ && entry->selector.type == diff_entry.selector.type) {
+ entry->access.mknod = entry->access.mknod && !diff_entry.access.mknod;
+ entry->access.read = entry->access.read && !diff_entry.access.read;
+ entry->access.write = entry->access.write && !diff_entry.access.write;
}
- return res;
- };
-
- auto revoke_accesses =
- [](Entry* entry, const Entry& diff_entry) -> Try<Nothing> {
- if (entry->selector.has_wildcard() || diff_entry.selector.has_wildcard()) {
- return Error("Call args for revoke_access cannot have wildcards");
- }
- if (
- entry->selector.major != diff_entry.selector.major ||
- entry->selector.minor != diff_entry.selector.minor ||
- entry->selector.type != diff_entry.selector.type) {
- return Nothing();
- }
- entry->access.mknod = entry->access.mknod && !diff_entry.access.mknod;
- entry->access.read = entry->access.read && !diff_entry.access.read;
- entry->access.write = entry->access.write && !diff_entry.access.write;
- return Nothing();
};
DeviceManager::CgroupDeviceAccess new_state = old_state;
- vector<Entry> additions =
-
DeviceManager::NonWildcardEntry::convert_to_entries(non_wildcard_additions);
- vector<Entry> removals =
- DeviceManager::NonWildcardEntry::convert_to_entries(non_wildcard_removals);
+ vector<Entry> additions = convert_to_entries(non_wildcard_additions);
+ vector<Entry> removals = convert_to_entries(non_wildcard_removals);
foreach (const Entry& addition, additions) {
// Go over each entry in deny list, find any entries that match the new
// addition's major & minor numbers, remove any accesses they specify
- // that the addition also specifies. If deny access becomes none, remove
it.
- // Invariant: No wildcards in deny list.
- foreach (Entry& existing_deny_entry, new_state.deny_list) {
- revoke_accesses(&existing_deny_entry, addition);
+ // that the addition also specifies.
+ // Invariant: No device wildcards are allowed in the deny list.
+ foreach (Entry& deny_entry, new_state.deny_list) {
+ revoke_accesses(&deny_entry, addition);
}
+
new_state.allow_list.push_back(addition);
}
@@ -289,58 +244,61 @@ DeviceManager::CgroupDeviceAccess
DeviceManager::apply_diff(
accesses_by_matching_wildcards.read = false;
accesses_by_matching_wildcards.write = false;
accesses_by_matching_wildcards.mknod = false;
- // Check if there is an entry with an exact match in major and minor
- // number in the allow list. If there is, revoke its access privileges
- // like we did when adding allow entries.
- foreach (Entry& existing_allow_entry, new_state.allow_list) {
- if (existing_allow_entry.selector.has_wildcard()) {
- // Matching against wildcard - we cannot revoke wildcard privileges
- // so we will insert a deny entry replicating whatever privileges we
- // deny which the wildcard grants.
- // Get privileges that the existing_allow_entry takes, check which
- // ones we both specify, add it to new deny entry and insert.
- Entry::Selector wildcard_selector = existing_allow_entry.selector;
- if (
- (wildcard_selector.type != Entry::Selector::Type::ALL &&
- wildcard_selector.type != removal.selector.type) ||
- (wildcard_selector.major.isSome() &&
- wildcard_selector.major != removal.selector.major) ||
- (wildcard_selector.minor.isSome() &&
- wildcard_selector.minor != removal.selector.minor)) {
- continue;
+
+ foreach (Entry& allow_entry, new_state.allow_list) {
+ // Matching against wildcard - we cannot revoke wildcard privileges
+ // so we will insert a deny entry replicating whatever privileges we
+ // need to deny which the wildcard grants.
+ if (allow_entry.selector.has_wildcard()) {
+ // Does the allow wildcard match the removal device? Skip if not.
+ if (allow_entry.selector.type != Entry::Selector::Type::ALL
+ && allow_entry.selector.type != removal.selector.type) {
+ continue; // Type doesn't match.
}
- accesses_by_matching_wildcards.mknod =
- accesses_by_matching_wildcards.mknod ||
- existing_allow_entry.access.mknod;
- accesses_by_matching_wildcards.read =
- accesses_by_matching_wildcards.read ||
- existing_allow_entry.access.read;
- accesses_by_matching_wildcards.write =
- accesses_by_matching_wildcards.write ||
- existing_allow_entry.access.write;
+ if (allow_entry.selector.major.isSome()
+ && allow_entry.selector.major != removal.selector.major) {
+ continue; // Major doesn't match.
+ }
+ if (allow_entry.selector.minor.isSome()
+ && allow_entry.selector.minor != removal.selector.minor) {
+ continue; // Minor doesn't match.
+ }
+ accesses_by_matching_wildcards.mknod |= allow_entry.access.mknod;
+ accesses_by_matching_wildcards.read |= allow_entry.access.read;
+ accesses_by_matching_wildcards.write |= allow_entry.access.write;
} else {
- revoke_accesses(&existing_allow_entry, removal);
+ revoke_accesses(&allow_entry, removal);
}
}
+
Entry::Access removal_access = removal.access;
- removal_access.mknod =
- accesses_by_matching_wildcards.mknod && removal_access.mknod;
- removal_access.read =
- accesses_by_matching_wildcards.read && removal_access.read;
- removal_access.write =
- accesses_by_matching_wildcards.write && removal_access.write;
+ removal_access.mknod &= accesses_by_matching_wildcards.mknod;
+ removal_access.read &= accesses_by_matching_wildcards.read;
+ removal_access.write &= accesses_by_matching_wildcards.write;
+
if (!removal_access.none()) {
Entry to_push = removal;
to_push.access = removal_access;
new_state.deny_list.push_back(to_push);
}
}
- new_state.allow_list = get_access_not_none_entries(new_state.allow_list);
- new_state.deny_list = get_access_not_none_entries(new_state.deny_list);
+
+ auto strip_empties = [](const vector<Entry>& entries) {
+ vector<Entry> res = {};
+ foreach (const Entry& entry, entries) {
+ if (!entry.access.none()) {
+ res.push_back(entry);
+ }
+ }
+ return res;
+ };
+
+ new_state.allow_list = strip_empties(new_state.allow_list);
+ new_state.deny_list = strip_empties(new_state.deny_list);
return new_state;
}
} // namespace slave {
} // namespace internal {
-} // namespace mesos {
\ No newline at end of file
+} // namespace mesos {
diff --git a/src/slave/containerizer/device_manager/device_manager.hpp
b/src/slave/containerizer/device_manager/device_manager.hpp
index 59aeb057b..a6e87dd54 100644
--- a/src/slave/containerizer/device_manager/device_manager.hpp
+++ b/src/slave/containerizer/device_manager/device_manager.hpp
@@ -18,9 +18,7 @@
#define __DEVICE_MANAGER_HPP__
#include <process/future.hpp>
-#include <process/process.hpp>
#include <stout/hashmap.hpp>
-#include <stout/hashset.hpp>
#include <stout/nothing.hpp>
#include <stout/try.hpp>
@@ -33,45 +31,39 @@ namespace slave {
class DeviceManagerProcess;
-// Manages Cgroups V2 device access state.
+// Manages cgroups V2 device access state.
//
-// In Cgroups V2, device control is managed via ebpf programs, as opposed to
the
-// control files in Cgroups V1. As such we lost the ability to checkpoint
-// a cgroup's device access state via the control files, and we have to
-// checkpoint this information ourselves via this centralized device manager.
+// In cgroups V2, device control is managed via ebpf programs, as opposed
+// to the control files in cgroups V1. As such we lose the ability to
+// checkpoint a cgroup's device access state via the control files, and
+// we have to checkpoint this information ourselves via this centralized
+// device manager.
//
// In addition, we always construct the ebpf programs from the cgroup's entire
// state, which we cannot keep track of via control files anymore. So we need
// the centralized device manager to manage the state and provide an interface
// to incrementally adjust the device access state for a cgroup and generate
// the appropriate ebpf programs.
-
+//
+// TODO(jasonzhou): This initial implementation only adds in-memory tracking
+// of the access, add bpf program attachment and checkpointing in follow-on
+// patches.
class DeviceManager
{
public:
// Used to enforce non-wildcard entry restrictions at compile time.
- struct NonWildcardEntry : cgroups::devices::Entry
+ struct NonWildcardEntry
{
struct Selector
{
- enum class Type
- {
- BLOCK,
- CHARACTER,
- };
-
+ enum class Type { BLOCK, CHARACTER };
Type type;
unsigned int major;
unsigned int minor;
};
- // Converts non-wildcard entries to regular entries
- static std::vector<cgroups::devices::Entry> convert_to_entries(
- const std::vector<NonWildcardEntry>& non_wildcards);
- // Converts regular entries to non-wildcard entries, for testing only
- static Try<std::vector<NonWildcardEntry>> convert_to_non_wildcards(
- const std::vector<cgroups::devices::Entry>& entries);
+
Selector selector;
- Access access;
+ cgroups::devices::Entry::Access access;
};
struct CgroupDeviceAccess
@@ -105,19 +97,19 @@ public:
const std::vector<NonWildcardEntry>& additionals,
const std::vector<NonWildcardEntry>& removals);
- // Return the cgroup's device access state in DeviceManager, which can be
+ // Return the cgroup's device access state, which can be
// used to query if a device access would be granted.
CgroupDeviceAccess state(const std::string& cgroup) const;
- // Return state of DeviceManager.
+ // Return the cgroup's device access state for all cgroups tracked.
hashmap<std::string, CgroupDeviceAccess> state() const;
- // Returns cgroup device state if additions and removals were applied to it.
+ // Returns cgroup device state with additions and removals applied to it.
+ // Exposed for unit testing.
static DeviceManager::CgroupDeviceAccess apply_diff(
- const DeviceManager::CgroupDeviceAccess& old_state,
- const std::vector<DeviceManager::NonWildcardEntry>& additions,
- const std::vector<DeviceManager::NonWildcardEntry>& removals);
-
+ const DeviceManager::CgroupDeviceAccess& state,
+ const std::vector<DeviceManager::NonWildcardEntry>& additions,
+ const std::vector<DeviceManager::NonWildcardEntry>& removals);
private:
explicit DeviceManager(const process::Owned<DeviceManagerProcess>& process);
@@ -128,4 +120,4 @@ private:
} // namespace internal {
} // namespace mesos {
-#endif // __DEVICE_MANAGER_HPP__
\ No newline at end of file
+#endif // __DEVICE_MANAGER_HPP__
diff --git a/src/tests/device_manager_tests.cpp
b/src/tests/device_manager_tests.cpp
index e735e1e95..398846bdd 100644
--- a/src/tests/device_manager_tests.cpp
+++ b/src/tests/device_manager_tests.cpp
@@ -17,26 +17,20 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>
-#include <vector>
+#include <utility>
+#include <tuple>
-#include <mesos/v1/scheduler.hpp>
+#include <process/gtest.hpp>
+
+#include <stout/unreachable.hpp>
+#include <stout/tests/utils.hpp>
#include "linux/cgroups2.hpp"
#include "slave/containerizer/device_manager/device_manager.hpp"
-#include "slave/containerizer/mesos/containerizer.hpp"
-#include "slave/paths.hpp"
-#include "slave/slave.hpp"
-#include "tests/mesos.hpp"
using mesos::internal::slave::DeviceManager;
using mesos::internal::slave::DeviceManagerProcess;
-using mesos::internal::slave::Slave;
-
-using mesos::master::detector::MasterDetector;
-using mesos::v1::scheduler::Event;
-
-using process::Clock;
using process::Future;
using process::Owned;
@@ -44,9 +38,6 @@ using std::string;
using std::tuple;
using std::vector;
-using testing::AtMost;
-using testing::DoAll;
-
namespace devices = cgroups2::devices;
namespace mesos {
@@ -55,6 +46,37 @@ namespace tests {
const string TEST_CGROUP = "test";
+
+Try<vector<DeviceManager::NonWildcardEntry>> convert_to_non_wildcards(
+ const vector<devices::Entry>& entries)
+{
+ vector<DeviceManager::NonWildcardEntry> non_wildcards = {};
+
+ foreach (const devices::Entry& entry, entries) {
+ if (entry.selector.has_wildcard()) {
+ return Error("Entry cannot have wildcard");
+ }
+ DeviceManager::NonWildcardEntry non_wildcard;
+ non_wildcard.access = entry.access;
+ non_wildcard.selector.major = *entry.selector.major;
+ non_wildcard.selector.minor = *entry.selector.minor;
+ non_wildcard.selector.type = [&]() {
+ switch (entry.selector.type) {
+ case cgroups::devices::Entry::Selector::Type::BLOCK:
+ return DeviceManager::NonWildcardEntry::Selector::Type::BLOCK;
+ case cgroups::devices::Entry::Selector::Type::CHARACTER:
+ return DeviceManager::NonWildcardEntry::Selector::Type::CHARACTER;
+ case cgroups::devices::Entry::Selector::Type::ALL:
+ UNREACHABLE();
+ }
+ }();
+ non_wildcards.push_back(non_wildcard);
+ }
+
+ return non_wildcards;
+}
+
+
class DeviceManagerTest : public TemporaryDirectoryTest
{
void SetUp() override
@@ -81,8 +103,8 @@ class DeviceManagerTest : public TemporaryDirectoryTest
TEST(NonWildcardEntry, NonWildcardFromWildcard)
{
- EXPECT_ERROR(DeviceManager::NonWildcardEntry::convert_to_non_wildcards(
- vector<devices::Entry>{*devices::Entry::parse("c *:1 w")}));
+ EXPECT_ERROR(convert_to_non_wildcards(
+ vector<devices::Entry>{*devices::Entry::parse("c *:1 w")}));
}
@@ -97,12 +119,10 @@ TEST_F(DeviceManagerTest,
ROOT_DeviceManagerConfigure_Normal)
vector<devices::Entry> allow_list = {*devices::Entry::parse("c 1:3 w")};
vector<devices::Entry> deny_list = {*devices::Entry::parse("c 3:1 w")};
-
AWAIT_ASSERT_READY(dm->configure(
- TEST_CGROUP,
- allow_list,
- CHECK_NOTERROR(
- DeviceManager::NonWildcardEntry::convert_to_non_wildcards(deny_list))));
+ TEST_CGROUP,
+ allow_list,
+ CHECK_NOTERROR(convert_to_non_wildcards(deny_list))));
DeviceManager::CgroupDeviceAccess cgroup_state = dm->state(TEST_CGROUP);
@@ -122,12 +142,10 @@ TEST_F(DeviceManagerTest,
ROOT_DeviceManagerReconfigure_Normal)
vector<devices::Entry> allow_list = {*devices::Entry::parse("c 1:3 w")};
vector<devices::Entry> deny_list = {*devices::Entry::parse("c 3:1 w")};
-
AWAIT_ASSERT_READY(dm->configure(
- TEST_CGROUP,
- allow_list,
- CHECK_NOTERROR(
- DeviceManager::NonWildcardEntry::convert_to_non_wildcards(deny_list))));
+ TEST_CGROUP,
+ allow_list,
+ CHECK_NOTERROR(convert_to_non_wildcards(deny_list))));
DeviceManager::CgroupDeviceAccess cgroup_state = dm->state(TEST_CGROUP);
EXPECT_EQ(cgroup_state.allow_list, allow_list);
@@ -137,11 +155,9 @@ TEST_F(DeviceManagerTest,
ROOT_DeviceManagerReconfigure_Normal)
vector<devices::Entry> removals = allow_list;
AWAIT_ASSERT_READY(dm->reconfigure(
- TEST_CGROUP,
- CHECK_NOTERROR(
- DeviceManager::NonWildcardEntry::convert_to_non_wildcards(additions)),
- CHECK_NOTERROR(
- DeviceManager::NonWildcardEntry::convert_to_non_wildcards(removals))));
+ TEST_CGROUP,
+ CHECK_NOTERROR(convert_to_non_wildcards(additions)),
+ CHECK_NOTERROR(convert_to_non_wildcards(removals))));
cgroup_state = dm->state(TEST_CGROUP);
EXPECT_EQ(cgroup_state.allow_list, additions);
@@ -159,13 +175,14 @@ TEST_F(DeviceManagerTest,
ROOT_DeviceManagerConfigure_AllowMatchesDeny)
vector<devices::Entry> allow_list = {*devices::Entry::parse("c 1:3 w")};
vector<devices::Entry> deny_list = {
- *devices::Entry::parse("c 1:3 w"), *devices::Entry::parse("c 21:1 w")};
+ *devices::Entry::parse("c 1:3 w"),
+ *devices::Entry::parse("c 21:1 w")
+ };
AWAIT_ASSERT_FAILED(dm->configure(
- TEST_CGROUP,
- allow_list,
- CHECK_NOTERROR(
- DeviceManager::NonWildcardEntry::convert_to_non_wildcards(deny_list))));
+ TEST_CGROUP,
+ allow_list,
+ CHECK_NOTERROR(convert_to_non_wildcards(deny_list))));
}
@@ -180,12 +197,10 @@ TEST_F(DeviceManagerTest,
ROOT_DeviceManagerConfigure_AllowWildcard)
vector<devices::Entry> allow_list = {*devices::Entry::parse("a *:* m")};
vector<devices::Entry> deny_list = {*devices::Entry::parse("c 3:1 m")};
-
AWAIT_ASSERT_READY(dm->configure(
- TEST_CGROUP,
- allow_list,
- CHECK_NOTERROR(
- DeviceManager::NonWildcardEntry::convert_to_non_wildcards(deny_list))));
+ TEST_CGROUP,
+ allow_list,
+ CHECK_NOTERROR(convert_to_non_wildcards(deny_list))));
DeviceManager::CgroupDeviceAccess dm_state = dm->state(TEST_CGROUP);
@@ -204,30 +219,25 @@ TEST_F(DeviceManagerTest,
ROOT_DeviceManagerGetDiffState_AllowMatchesDeny)
vector<devices::Entry> additions = {*devices::Entry::parse("c 1:3 w")};
vector<devices::Entry> removals = {
- *devices::Entry::parse("c 1:3 w"), *devices::Entry::parse("c 21:1 w")};
+ *devices::Entry::parse("c 1:3 w"),
+ *devices::Entry::parse("c 21:1 w")
+ };
AWAIT_ASSERT_FAILED(dm->reconfigure(
- TEST_CGROUP,
- CHECK_NOTERROR(
- DeviceManager::NonWildcardEntry::convert_to_non_wildcards(additions)),
- CHECK_NOTERROR(
- DeviceManager::NonWildcardEntry::convert_to_non_wildcards(removals))));
+ TEST_CGROUP,
+ CHECK_NOTERROR(convert_to_non_wildcards(additions)),
+ CHECK_NOTERROR(convert_to_non_wildcards(removals))));
}
using DeviceManagerGetDiffStateTestParams = tuple<
- // Allow list for initial configure
- vector<devices::Entry>,
- // Deny list for initial configure
- vector<devices::Entry>,
- // Additions for reconfigure
- vector<devices::Entry>,
- // Removals for reconfigure
- vector<devices::Entry>,
- // Expected allow list after reconfigure
- vector<devices::Entry>,
- // Expected deny list after reconfigure
- vector<devices::Entry>>;
+ vector<devices::Entry>, // Allow list for initial configure.
+ vector<devices::Entry>, // Deny list for initial configure.
+ vector<devices::Entry>, // Additions for reconfigure.
+ vector<devices::Entry>, // Removals for reconfigure.
+ vector<devices::Entry>, // Expected allow list after reconfigure.
+ vector<devices::Entry> // Expected deny list after reconfigure.
+>;
class DeviceManagerGetDiffStateTestFixture
@@ -253,10 +263,9 @@ TEST_P(DeviceManagerGetDiffStateTestFixture,
ROOT_DeviceManagerGetDiffState)
Owned<DeviceManager>(CHECK_NOTERROR(DeviceManager::create(flags)));
AWAIT_ASSERT_READY(dm->configure(
- TEST_CGROUP,
- setup_allow,
- CHECK_NOTERROR(
- DeviceManager::NonWildcardEntry::convert_to_non_wildcards(setup_deny))));
+ TEST_CGROUP,
+ setup_allow,
+ CHECK_NOTERROR(convert_to_non_wildcards(setup_deny))));
DeviceManager::CgroupDeviceAccess dm_state = dm->state(TEST_CGROUP);
@@ -264,11 +273,9 @@ TEST_P(DeviceManagerGetDiffStateTestFixture,
ROOT_DeviceManagerGetDiffState)
EXPECT_EQ(dm_state.deny_list, setup_deny);
dm_state = dm->apply_diff(
- dm->state(TEST_CGROUP),
- CHECK_NOTERROR(
- DeviceManager::NonWildcardEntry::convert_to_non_wildcards(additions)),
- CHECK_NOTERROR(
- DeviceManager::NonWildcardEntry::convert_to_non_wildcards(removals)));
+ dm->state(TEST_CGROUP),
+ CHECK_NOTERROR(convert_to_non_wildcards(additions)),
+ CHECK_NOTERROR(convert_to_non_wildcards(removals)));
EXPECT_EQ(dm_state.allow_list, reconfigured_allow);
EXPECT_EQ(dm_state.deny_list, reconfigured_deny);
@@ -279,7 +286,7 @@ INSTANTIATE_TEST_CASE_P(
DeviceManagerGetDiffStateTestParams,
DeviceManagerGetDiffStateTestFixture,
::testing::Values<DeviceManagerGetDiffStateTestParams>(
- // Remove existing allow entry accesses
+ // Remove existing allow entry accesses:
DeviceManagerGetDiffStateTestParams{
vector<devices::Entry>{*devices::Entry::parse("c 3:1 rwm")},
vector<devices::Entry>{},
@@ -287,7 +294,7 @@ INSTANTIATE_TEST_CASE_P(
vector<devices::Entry>{*devices::Entry::parse("c 3:1 rm")},
vector<devices::Entry>{*devices::Entry::parse("c 3:1 w")},
vector<devices::Entry>{}},
- // Remove existing deny entry accesses
+ // Remove existing deny entry accesses:
DeviceManagerGetDiffStateTestParams{
vector<devices::Entry>{*devices::Entry::parse("c 3:* rwm")},
vector<devices::Entry>{*devices::Entry::parse("c 3:1 rwm")},
@@ -297,7 +304,7 @@ INSTANTIATE_TEST_CASE_P(
*devices::Entry::parse("c 3:* rwm"),
*devices::Entry::parse("c 3:1 rm")},
vector<devices::Entry>{*devices::Entry::parse("c 3:1 w")}},
- // Remove entire existing allow entry
+ // Remove entire existing allow entry:
DeviceManagerGetDiffStateTestParams{
vector<devices::Entry>{*devices::Entry::parse("c 3:1 rm")},
vector<devices::Entry>{},
@@ -305,7 +312,7 @@ INSTANTIATE_TEST_CASE_P(
vector<devices::Entry>{*devices::Entry::parse("c 3:1 rwm")},
vector<devices::Entry>{},
vector<devices::Entry>{}},
- // Remove entire existing deny entry
+ // Remove entire existing deny entry:
DeviceManagerGetDiffStateTestParams{
vector<devices::Entry>{*devices::Entry::parse("c 3:* rm")},
vector<devices::Entry>{*devices::Entry::parse("c 3:1 rm")},
@@ -314,7 +321,7 @@ INSTANTIATE_TEST_CASE_P(
vector<devices::Entry>{
*devices::Entry::parse("c 3:* rm"), *devices::Entry::parse("c 3:1
rm")},
vector<devices::Entry>{}},
- // Overlapping entries where none encompasses the other
+ // Overlapping entries where none encompasses the other:
DeviceManagerGetDiffStateTestParams{
vector<devices::Entry>{*devices::Entry::parse("c 3:* rm")},
vector<devices::Entry>{*devices::Entry::parse("c 3:1 rm")},
@@ -323,7 +330,7 @@ INSTANTIATE_TEST_CASE_P(
vector<devices::Entry>{
*devices::Entry::parse("c 3:* rm"), *devices::Entry::parse("c 3:1
rw")},
vector<devices::Entry>{*devices::Entry::parse("c 3:1 m")}},
- // Overlapping with non-encompassing wildcard
+ // Overlapping with non-encompassing wildcard:
DeviceManagerGetDiffStateTestParams{
vector<devices::Entry>{*devices::Entry::parse("c 3:* rm")},
vector<devices::Entry>{},
```
src/slave/containerizer/device_manager/device_manager.hpp
Lines 21 (patched)
<https://reviews.apache.org/r/75006/#comment314976>
unused
src/slave/containerizer/device_manager/device_manager.hpp
Lines 23 (patched)
<https://reviews.apache.org/r/75006/#comment314977>
unused
src/slave/containerizer/device_manager/device_manager.hpp
Lines 39-41 (patched)
<https://reviews.apache.org/r/75006/#comment314975>
we don't checkpoint yet, so we need a TODO here
ditto for actually generating/applying the ebpf program?
src/slave/containerizer/device_manager/device_manager.hpp
Lines 53 (patched)
<https://reviews.apache.org/r/75006/#comment314972>
Did you intend to use inheritance here? Looks wrong since you're not
overriding the methods?
src/slave/containerizer/device_manager/device_manager.hpp
Lines 67-69 (patched)
<https://reviews.apache.org/r/75006/#comment314973>
this doesn't look like it needs to be in the class and the header?
src/slave/containerizer/device_manager/device_manager.hpp
Lines 70-72 (patched)
<https://reviews.apache.org/r/75006/#comment314974>
If we only need this for testing, we can just define it in the tests?
src/slave/containerizer/device_manager/device_manager.cpp
Lines 20 (patched)
<https://reviews.apache.org/r/75006/#comment314978>
unused
src/slave/containerizer/device_manager/device_manager.cpp
Lines 25 (patched)
<https://reviews.apache.org/r/75006/#comment314979>
unused
src/slave/containerizer/device_manager/device_manager.cpp
Lines 30-32 (patched)
<https://reviews.apache.org/r/75006/#comment314981>
unused as well, but note that you don't need to guard a linux include with
an ifdef here since we're already assuming linux when compiling this file
src/slave/containerizer/device_manager/device_manager.cpp
Lines 39 (patched)
<https://reviews.apache.org/r/75006/#comment314980>
unused
src/slave/containerizer/device_manager/device_manager.cpp
Lines 166 (patched)
<https://reviews.apache.org/r/75006/#comment314982>
missing the ref here, so taking a copy
src/slave/containerizer/device_manager/device_manager.cpp
Lines 169-173 (patched)
<https://reviews.apache.org/r/75006/#comment314983>
prefer switches for enum, that way we get a compile error if future cases
are added:
```
entry.selector.type = [&]() {
switch (non_wildcards_entry.selector.type) {
case DeviceManager::NonWildcardEntry::Selector::Type::BLOCK:
return Entry::Selector::Type::BLOCK;
case DeviceManager::NonWildcardEntry::Selector::Type::CHARACTER:
return Entry::Selector::Type::CHARACTER;
}
}();
```
src/slave/containerizer/device_manager/device_manager.cpp
Lines 248 (patched)
<https://reviews.apache.org/r/75006/#comment314987>
stray semi-colon
src/slave/containerizer/device_manager/device_manager.cpp
Lines 255-257 (patched)
<https://reviews.apache.org/r/75006/#comment314988>
should probably be a CHECK since you're not checking the Try in the call
sites?
src/slave/containerizer/device_manager/device_manager.cpp
Lines 279 (patched)
<https://reviews.apache.org/r/75006/#comment314989>
> If deny access becomes none, remove it.
stale comment now?
src/slave/containerizer/device_manager/device_manager.cpp
Lines 284 (patched)
<https://reviews.apache.org/r/75006/#comment314990>
Ok, so we always add it, even if it's already present? I suppose in
addition to stripping out empties, we could be de-duping the entries
afterwards? Do you have a test for adding the same device twice, then removing
it twice? IIUC it works, but it will add duplicate entries then remove both
entries on the first removal?
src/slave/containerizer/device_manager/device_manager.cpp
Lines 292-294 (patched)
<https://reviews.apache.org/r/75006/#comment314991>
stale?
src/slave/containerizer/device_manager/device_manager.cpp
Lines 312-320 (patched)
<https://reviews.apache.org/r/75006/#comment314992>
shorten with |= here?
src/tests/device_manager_tests.cpp
Lines 17-48 (patched)
<https://reviews.apache.org/r/75006/#comment314984>
lots of unused stuff going on here?
src/tests/device_manager_tests.cpp
Lines 97 (patched)
<https://reviews.apache.org/r/75006/#comment314985>
These naked de-references should really be CHECK_NOTERRORs, but for brevity
I will leave them as is across these tests
src/tests/device_manager_tests.cpp
Lines 266-271 (patched)
<https://reviews.apache.org/r/75006/#comment314986>
hm.. didn't touch this, but why isn't this just calling re-configure then
checking the re-configured state? you already have a device manager in hand, so
this isn't really a unit test of the apply_diff function..
- Benjamin Mahler
On July 22, 2024, 6:50 p.m., Jason Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/75006/
> -----------------------------------------------------------
>
> (Updated July 22, 2024, 6:50 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change introduces the DeviceManagerProcess to help facilitate
> device access management in cgroups2 via ebpf program file changes.
>
> Device requests can be made to the manager by calling
> `configure` or `reconfigure`. Note that `configure`
> should only be used when setting up a cgroup's device access, i.e. it
> has not requested any device to be allowed/denied before.
> In addition, `reconfigure` cannot be used to add deny entries containing
> wildcards.
> This manager will be made available to all controllers under the
> cgroups2 isolator, and the GPU isolator.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt ea0fee1bbaed6f2494e9b9739bb65812a4a0042b
> src/Makefile.am 03eb0cc28ee18da7f1a13f35a7e3255e56869b56
> src/slave/containerizer/device_manager/device_manager.hpp PRE-CREATION
> src/slave/containerizer/device_manager/device_manager.cpp PRE-CREATION
> src/tests/containerizer/cgroups2_tests.cpp
> 3982e25987c40bc3748bc9be4e7b19c5b53dc211
> src/tests/device_manager_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/75006/diff/21/
>
>
> Testing
> -------
>
> Added unit tests for DeviceManager to test configure and reconfigure. All
> unit tests pass
>
>
> Thanks,
>
> Jason Zhou
>
>
