bmahler commented on code in PR #552:
URL: https://github.com/apache/mesos/pull/552#discussion_r1559811944
##########
src/slave/containerizer/mesos/linux_launcher.cpp:
##########
@@ -53,17 +56,24 @@ namespace mesos {
namespace internal {
namespace slave {
+struct CgroupsInfo
+{
+ // Flag indicating whether cgroups v2 is used.
+ bool cgroupsV2;
+
+ Option<std::string> freezerHierarchy;
+
+ Option<std::string> systemdHierarchy;
Review Comment:
it would be good if we could explain when these are set, e.g. if cgroupsv1
then freezer is set, but when is systemd set and how should it be used?
##########
src/slave/flags.cpp:
##########
@@ -1234,7 +1244,7 @@ mesos::internal::slave::Flags::Flags()
"minimum_egress_rate_limit and maximum_egress_rate_limit flags."
"If set to 'auto' the rate limit is automatically calculated\n"
"by determining the link speed and dividing by the number of available\n"
- "CPU resources.\n"
+ "CPU resources.\n"
Review Comment:
in general try not to touch unrelated lines in the diffs, looks like this is
just your editor automatically removing all trailing whitespace from the file?
##########
src/slave/containerizer/mesos/linux_launcher.cpp:
##########
@@ -251,48 +303,104 @@ Future<ContainerStatus> LinuxLauncher::status(
}
-// `_systemdHierarchy` is only set if running on a systemd environment.
LinuxLauncherProcess::LinuxLauncherProcess(
- const Flags& _flags,
- const string& _freezerHierarchy,
- const Option<string>& _systemdHierarchy)
- : flags(_flags),
- freezerHierarchy(_freezerHierarchy),
- systemdHierarchy(_systemdHierarchy) {}
+ const Flags& _flags, const CgroupsInfo& _cgroupsInfo)
+ : flags(_flags), cgroupsInfo(_cgroupsInfo) {}
Future<hashset<ContainerID>> LinuxLauncherProcess::recover(
const vector<ContainerState>& states)
{
LOG(INFO) << "Recovering Linux launcher";
+ // 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
Review Comment:
this looks stale now in the v2 case?
also, this logic looks wrong? the logic is now looking in containers before
it actually recovered them, so it looks like it's the wrong order? did the root
tests pass with this code?
--
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]
