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/06/03 20:24:44 UTC
Re: Review Request 75006: [cgroups2] Introduce the DeviceManagerProcess
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75006/
-----------------------------------------------------------
(Updated June 3, 2024, 8:24 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
address reviewer feedback
Repository: mesos
Description
-------
This change introduces the DeviceManagerProcess to help
facilitate device access management in cgroups2 via
ebpf program file changes.
The manager keeps a global state that is checkpointed on change.
Device requests can be made to the manager by calling
DeviceManagerProcess::deny or DeviceManagerProcess::allow.
This manager will be made available to all controllers under the
cgroups2 isolator, and the GPU isolator.
Diffs (updated)
-----
src/CMakeLists.txt 3973737f1f0719be98350b25abddcba81716207b
src/Makefile.am 68a93674f867ca092ee035ea367f35f4580b073f
src/slave/device_manager/device_manager.hpp PRE-CREATION
src/slave/device_manager/device_manager.cpp PRE-CREATION
src/slave/device_manager/state.hpp PRE-CREATION
src/slave/device_manager/state.proto PRE-CREATION
src/slave/paths.hpp 7d43e9c0170c7bcced916a892590063005133d6f
src/slave/paths.cpp 3e44b636c76c01f88b00f5f8d2db39cc5fe00c1c
Diff: https://reviews.apache.org/r/75006/diff/5/
Changes: https://reviews.apache.org/r/75006/diff/4-5/
Testing
-------
Thanks,
Jason Zhou
Re: Review Request 75006: [cgroups2] Introduce the DeviceManagerProcess
Posted by Jason Zhou <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75006/#review226706
-----------------------------------------------------------
src/slave/containerizer/device_manager/device_manager.cpp
Lines 258 (patched)
<https://reviews.apache.org/r/75006/#comment314967>
we remove the entry's accesses if its major, minor and type match that of diff_entry
src/slave/containerizer/device_manager/device_manager.cpp
Lines 303 (patched)
<https://reviews.apache.org/r/75006/#comment314968>
If the wildcard_selector's type/major/minor fields aren't a wildcard and does not match with the removal entry, then there is no overlap, else, there is potential overlap between the existing_allow_entry and the removal entry, and we will have to create a new deny entry.
src/slave/containerizer/device_manager/device_manager.cpp
Lines 326 (patched)
<https://reviews.apache.org/r/75006/#comment314969>
For the accesses of the new deny entry to be inserted, we only set them to true if it's specified by an overlapping wildcard AND by the removal entry itself.
src/tests/device_manager_tests.cpp
Lines 317 (patched)
<https://reviews.apache.org/r/75006/#comment314971>
Entry `c 3:1 rm` and `c 3:1 rw` only overlap for the read access. Hence, the read access is removed from the `c 3:1 rm` in the original deny list, and only `c 3:1 m` is left.
src/tests/device_manager_tests.cpp
Lines 328 (patched)
<https://reviews.apache.org/r/75006/#comment314970>
The wildcard has accesses rm, the removal entry has accesses rw, the only overlapping access is r, so we insert it into the deny list.
- Jason Zhou
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, as well as DeviceManager::CgroupDeviceAccess::is_access_granted. All unit tests pass
>
>
> Thanks,
>
> Jason Zhou
>
>
Re: Review Request 75006: [cgroups2] Introduce the DeviceManagerProcess
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 75006: [cgroups2] Introduce the DeviceManagerProcess
Posted by Jason Zhou <ja...@gmail.com>.
-----------------------------------------------------------
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 (updated)
-----
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/20/
Changes: https://reviews.apache.org/r/75006/diff/19-20/
Testing (updated)
-------
Added unit tests for DeviceManager to test configure and reconfigure, as well as DeviceManager::CgroupDeviceAccess::is_access_granted. All unit tests pass
Thanks,
Jason Zhou
Re: Review Request 75006: [cgroups2] Introduce the DeviceManagerProcess
Posted by Jason Zhou <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75006/
-----------------------------------------------------------
(Updated July 19, 2024, 10:23 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Pulled out changes related to deconfiguring cgroups device accesses and state recovery. renamed get_diff_state to apply_diff and made it a static function inside DeviceManager only
Repository: mesos
Description (updated)
-------
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 (updated)
-----
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/18/
Changes: https://reviews.apache.org/r/75006/diff/17-18/
Testing
-------
Added unit tests for DeviceManager to test setup_configure and incremental_configure, added test for recover.
All unit tests pass
Thanks,
Jason Zhou
Re: Review Request 75006: [cgroups2] Introduce the DeviceManagerProcess
Posted by Jason Zhou <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75006/
-----------------------------------------------------------
(Updated July 17, 2024, 7:06 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Removed DeviceManagerProcess from device_manager.hpp per convention
Repository: mesos
Description
-------
This change introduces the DeviceManagerProcess to help facilitate
device access management in cgroups2 via ebpf program file changes.
The manager keeps a global state that is checkpointed on change.
Device requests can be made to the manager by calling
incremental_configure or setup_configure. Note that setup_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, setup_configure cannot be used to add deny wildcards.
This manager will be made available to all controllers under the
cgroups2 isolator, and the GPU isolator.
Diffs (updated)
-----
src/CMakeLists.txt ea0fee1bbaed6f2494e9b9739bb65812a4a0042b
src/Makefile.am 03eb0cc28ee18da7f1a13f35a7e3255e56869b56
src/linux/cgroups2.hpp accaebdaddc85acdd96b87161ea441c77b025099
src/linux/cgroups2.cpp cb3c425a46f33f5434f870c03dd7de5be3331ece
src/slave/device_manager/device_manager.hpp PRE-CREATION
src/slave/device_manager/device_manager.cpp PRE-CREATION
src/slave/device_manager/state.hpp PRE-CREATION
src/slave/device_manager/state.proto PRE-CREATION
src/slave/paths.hpp 7d43e9c0170c7bcced916a892590063005133d6f
src/slave/paths.cpp 3e44b636c76c01f88b00f5f8d2db39cc5fe00c1c
src/tests/device_manager_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/75006/diff/17/
Changes: https://reviews.apache.org/r/75006/diff/16-17/
Testing
-------
Added unit tests for DeviceManager to test setup_configure and incremental_configure, added test for recover.
All unit tests pass
Thanks,
Jason Zhou
Re: Review Request 75006: [cgroups2] Introduce the DeviceManagerProcess
Posted by Jason Zhou <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75006/
-----------------------------------------------------------
(Updated July 17, 2024, 6:36 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Cleaned up the code using lambdas, added ROOT requirement to DeviceManager tests as they create cgroups
Repository: mesos
Description
-------
This change introduces the DeviceManagerProcess to help facilitate
device access management in cgroups2 via ebpf program file changes.
The manager keeps a global state that is checkpointed on change.
Device requests can be made to the manager by calling
incremental_configure or setup_configure. Note that setup_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, setup_configure cannot be used to add deny wildcards.
This manager will be made available to all controllers under the
cgroups2 isolator, and the GPU isolator.
Diffs (updated)
-----
src/CMakeLists.txt ea0fee1bbaed6f2494e9b9739bb65812a4a0042b
src/Makefile.am 03eb0cc28ee18da7f1a13f35a7e3255e56869b56
src/linux/cgroups2.hpp accaebdaddc85acdd96b87161ea441c77b025099
src/linux/cgroups2.cpp cb3c425a46f33f5434f870c03dd7de5be3331ece
src/slave/device_manager/device_manager.hpp PRE-CREATION
src/slave/device_manager/device_manager.cpp PRE-CREATION
src/slave/device_manager/state.hpp PRE-CREATION
src/slave/device_manager/state.proto PRE-CREATION
src/slave/paths.hpp 7d43e9c0170c7bcced916a892590063005133d6f
src/slave/paths.cpp 3e44b636c76c01f88b00f5f8d2db39cc5fe00c1c
src/tests/device_manager_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/75006/diff/16/
Changes: https://reviews.apache.org/r/75006/diff/15-16/
Testing
-------
Added unit tests for DeviceManager to test setup_configure and incremental_configure, added test for recover.
All unit tests pass
Thanks,
Jason Zhou
Re: Review Request 75006: [cgroups2] Introduce the DeviceManagerProcess
Posted by Jason Zhou <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75006/
-----------------------------------------------------------
(Updated July 17, 2024, 5:21 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Added tests for configure and recovery functions
Repository: mesos
Description
-------
This change introduces the DeviceManagerProcess to help facilitate
device access management in cgroups2 via ebpf program file changes.
The manager keeps a global state that is checkpointed on change.
Device requests can be made to the manager by calling
incremental_configure or setup_configure. Note that setup_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, setup_configure cannot be used to add deny wildcards.
This manager will be made available to all controllers under the
cgroups2 isolator, and the GPU isolator.
Diffs (updated)
-----
src/CMakeLists.txt ea0fee1bbaed6f2494e9b9739bb65812a4a0042b
src/Makefile.am 03eb0cc28ee18da7f1a13f35a7e3255e56869b56
src/linux/cgroups2.hpp accaebdaddc85acdd96b87161ea441c77b025099
src/linux/cgroups2.cpp cb3c425a46f33f5434f870c03dd7de5be3331ece
src/slave/device_manager/device_manager.hpp PRE-CREATION
src/slave/device_manager/device_manager.cpp PRE-CREATION
src/slave/device_manager/state.hpp PRE-CREATION
src/slave/device_manager/state.proto PRE-CREATION
src/slave/paths.hpp 7d43e9c0170c7bcced916a892590063005133d6f
src/slave/paths.cpp 3e44b636c76c01f88b00f5f8d2db39cc5fe00c1c
src/tests/device_manager_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/75006/diff/15/
Changes: https://reviews.apache.org/r/75006/diff/14-15/
Testing (updated)
-------
Added unit tests for DeviceManager to test setup_configure and incremental_configure, added test for recover.
All unit tests pass
Thanks,
Jason Zhou
Re: Review Request 75006: [cgroups2] Introduce the DeviceManagerProcess
Posted by Jason Zhou <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75006/
-----------------------------------------------------------
(Updated July 9, 2024, 7:52 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Reworked logic
DeviceManager's state now contains two vectors of cgroups device entries
There are now two configure functions.
- The first, setup_configure, is to be used when setting up device accesses for a cgroup. For this reason, wildcard major/minor number entries are allowed for the input allow/deny list.
- The second, incremental_configure, is to be used when adding and removing individual devices for a cgroup. We disable wildcard major/minor numbers for this function to allow us to determine if every operation done using this function is possible. That means that every device entry in both allow and deny lists for this function must have their major and minor numbers set to a specific value.
Updated the recover, persist and commit_devices_changes functions to work the type change in CgroupDeviceAccess to use Entry instead of string.
Repository: mesos
Description (updated)
-------
This change introduces the DeviceManagerProcess to help facilitate device access management in cgroups2 via ebpf program file changes.
The manager keeps a global state that is checkpointed on change.
Device requests can be made to the manager by calling incremental_configure or setup_configure. Note that setup_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.
This manager will be made available to all controllers under the cgroups2 isolator, and the GPU isolator.
Diffs (updated)
-----
src/CMakeLists.txt ea0fee1bbaed6f2494e9b9739bb65812a4a0042b
src/Makefile.am 03eb0cc28ee18da7f1a13f35a7e3255e56869b56
src/linux/cgroups2.hpp 64254d04f65128713cf3489b25bcba42590b6020
src/linux/cgroups2.cpp d1fc2638cdf9a07199f90952e04998072021011c
src/slave/device_manager/device_manager.hpp PRE-CREATION
src/slave/device_manager/device_manager.cpp PRE-CREATION
src/slave/device_manager/state.hpp PRE-CREATION
src/slave/device_manager/state.proto PRE-CREATION
src/slave/paths.hpp 7d43e9c0170c7bcced916a892590063005133d6f
src/slave/paths.cpp 3e44b636c76c01f88b00f5f8d2db39cc5fe00c1c
Diff: https://reviews.apache.org/r/75006/diff/10/
Changes: https://reviews.apache.org/r/75006/diff/9-10/
Testing
-------
Thanks,
Jason Zhou