bmahler commented on code in PR #552:
URL: https://github.com/apache/mesos/pull/552#discussion_r1559998295
##########
src/slave/containerizer/mesos/linux_launcher.cpp:
##########
@@ -53,17 +56,29 @@ namespace mesos {
namespace internal {
namespace slave {
+struct CgroupsInfo
+{
+ // Flag indicating whether cgroups v2 is used.
+ bool cgroupsV2;
+
+ // Absolute path of the cgroup freezer hierarchy.
+ // Only present when cgroups v1 is used.
+ Option<std::string> freezerHierarchy;
+
+ // Absolute path the systemd hierarchy.
+ // Only present in cgroups v1 when systemd is enabled
+ // (i.e. `systemd::enabled()`).
+ Option<std::string> systemdHierarchy;
+};
+
// Launcher for Linux systems with cgroups. Uses a freezer cgroup to
// track pids.
Review Comment:
looks like this needs an adjustment now
##########
src/slave/containerizer/mesos/linux_launcher.cpp:
##########
@@ -589,72 +698,91 @@ Future<Nothing> LinuxLauncherProcess::destroy(const
ContainerID& containerId)
//
// NOTE: it's safe to use `container->id` from here on because it's
// a copy of the Container that we're about to delete.
- containers.erase(container->id);
+ containers.erase(container.id);
// Determine if this is a partially destroyed container. A container
// is considered partially destroyed if we have recovered it from
// ContainerState but we don't have a freezer cgroup for it. If this
// is a partially destroyed container than there is nothing to do.
- if (!cgroups::exists(freezerHierarchy, cgroup)) {
+ if (!cgroups::exists(cgroupsInfo.freezerHierarchy.get(), cgroup)) {
LOG(WARNING) << "Couldn't find freezer cgroup for container "
- << container->id << " so assuming partially destroyed";
+ << container.id << " so assuming partially destroyed";
- return _destroy(containerId);
+ return _destroyCgroups(container);
}
LOG(INFO) << "Destroying cgroup '"
- << path::join(freezerHierarchy, cgroup) << "'";
+ << path::join(cgroupsInfo.freezerHierarchy.get(), cgroup) << "'";
// TODO(benh): If this is the last container at a nesting level,
// should we also delete the `CGROUP_SEPARATOR` cgroup too?
// TODO(benh): What if we fail to destroy the container? Should we
// retry?
return cgroups::destroy(
- freezerHierarchy,
+ cgroupsInfo.freezerHierarchy.get(),
cgroup,
flags.cgroups_destroy_timeout)
- .then(defer(
- self(),
- &LinuxLauncherProcess::_destroy,
- containerId));
+ .then(defer(self(), &LinuxLauncherProcess::_destroyCgroups, container));
}
-Future<Nothing> LinuxLauncherProcess::_destroy(const ContainerID& containerId)
+Future<Nothing> LinuxLauncherProcess::_destroyCgroups(
+ const Container& container)
{
- if (systemdHierarchy.isNone()) {
+ if (cgroupsInfo.systemdHierarchy.isNone()) {
return Nothing();
}
const string cgroup =
- containerizer::paths::getCgroupPath(flags.cgroups_root, containerId);
+ containerizer::paths::getCgroupPath(flags.cgroups_root, container.id);
- if (!cgroups::exists(systemdHierarchy.get(), cgroup)) {
+ if (!cgroups::exists(cgroupsInfo.systemdHierarchy.get(), cgroup)) {
return Nothing();
}
LOG(INFO) << "Destroying cgroup '"
- << path::join(systemdHierarchy.get(), cgroup) << "'";
+ << path::join(cgroupsInfo.systemdHierarchy.get(), cgroup) << "'";
return cgroups::destroy(
- systemdHierarchy.get(),
+ cgroupsInfo.systemdHierarchy.get(),
cgroup,
flags.cgroups_destroy_timeout);
}
+Try<Nothing> LinuxLauncherProcess::destroyCgroups2(
+ const Container& container)
+{
+#ifdef ENABLE_CGROUPS_V2
+ const string& cgroup =
+ containerizer::paths::cgroups2::container(flags.cgroups_root,
container.id);
+
+ containers.erase(container.id);
+
+ LOG(INFO) << "Destroying cgroup '" << cgroup << "'";
+
+ Try<Nothing> destroy = cgroups2::destroy(cgroup);
+ if (destroy.isError()) {
+ return Error(
+ "Failed to destory cgroup '" + cgroup + "': " + destroy.error());
+ }
+#endif // ENABLE_CGROUPS_V2
+ LOG(INFO) << "Destroyed container " << container.id;
+
+ return Nothing();
Review Comment:
hm.. we shouldn't be logging and returning success when the ifdef is not
present and we're not doing anything, ditto for cgroups 2 recovery code
##########
src/slave/containerizer/mesos/linux_launcher.cpp:
##########
@@ -326,103 +514,40 @@ Future<hashset<ContainerID>>
LinuxLauncherProcess::recover(
LOG(INFO) << "Recovered container " << container.id;
}
- // Now loop through the containers expected by ContainerState so we
- // can have a complete list of the containers we might ever want to
- // destroy as well as be able to determine orphans below.
- hashset<ContainerID> expected = {};
-
- foreach (const ContainerState& state, states) {
- expected.insert(state.container_id());
-
- if (!containers.contains(state.container_id())) {
- // The fact that we did not have a freezer (or systemd) cgroup
- // for this container implies this container has already been
- // destroyed but we need to add it to `containers` so that when
- // `LinuxLauncher::destroy` does get called below for this
- // container we will not fail.
- Container container;
- container.id = state.container_id();
- container.pid = state.pid();
+ return Nothing();
+}
- containers.put(container.id, container);
- LOG(INFO) << "Recovered (destroyed) container " << container.id;
- } else {
- // This container exists, so we save the pid so we can check
- // that it's part of the systemd "Mesos executor slice" below.
- containers[state.container_id()].pid = state.pid();
- }
+Try<Nothing> LinuxLauncherProcess::recoverContainersFromCgroups2()
Review Comment:
seems like these functions should just return the Containers rather than
have side effects
##########
src/slave/containerizer/mesos/linux_launcher.cpp:
##########
@@ -371,7 +456,7 @@ Future<hashset<ContainerID>> LinuxLauncherProcess::recover(
// but not in the `MESOS_EXECUTORS_SLICE` or Mesos cgroup root under
// the systemd hierarhcy). We need a flag to support the upgrade
// path.
- if (systemdHierarchy.isSome()) {
+ if (cgroupsInfo.systemdHierarchy.isSome()) {
foreachvalue (const Container& container, containers) {
if (container.pid.isNone()) {
continue;
Review Comment:
hm.. this logic looks bad? the pid is never going to be set here because we
haven't done the pid assignment from the checkpointed state yet now that we've
moved the code around
##########
src/slave/containerizer/mesos/linux_launcher.cpp:
##########
@@ -442,107 +567,105 @@ Try<pid_t> LinuxLauncherProcess::fork(
return Error("Container '" + stringify(containerId) + "' already exists");
}
- Option<pid_t> target = None();
+ Option<pid_t> parentPid = None();
// Ensure nested containers have known parents.
if (containerId.has_parent()) {
- Option<Container> container = containers.get(containerId.parent());
- if (container.isNone()) {
+ Option<Container> parent = containers.get(containerId.parent());
+ if (parent.isNone()) {
return Error("Unknown parent container");
}
- if (container->pid.isNone()) {
+ if (parent->pid.isNone()) {
// TODO(benh): Could also look up a pid in the container and use
// that in order to enter the namespaces? This would be best
// effort because we don't know the namespaces that had been
// created for the original pid.
return Error("Unknown parent container pid, can not enter namespaces");
}
- target = container->pid.get();
+ parentPid = parent->pid;
}
- // Ensure we didn't pass `enterNamespaces`
- // if we aren't forking a nested container.
Review Comment:
In general try not to touch unrelated lines in the PRs, it adds a lot of
diff noise, for pure refactoring we usually just create patch sets (with
reviewboard) to split out cleanups like this. Ditto elsewhere in this patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
