[PR] CHANGE: add default network if no net options was set. [mesos]
andreaspeters opened a new pull request, #585:
URL: https://github.com/apache/mesos/pull/585
Short:
With these PR I want to add support for custom docker networks created by
docker network ...
Long:
The Mesos DockerInfo Network object only supports four network modes:
BRIDGE, HOST, USER, and NONE. If we want to use a network created by the Docker
command docker network create ..., we cannot use the network mode. In that
case, we have to add the DockerInfo parameter. The problem is, if we do not add
a DockerInfo network mode, then the Docker executor will add a default one
(Lines 839, 841). This means the Docker executor could try to run the container
with two incompatible network modes. With this PR the docker-executor will
check if there is one (or many) `--net` or a `--network` parameters. If it's
so, it will use the first one as default network mode and all others as
additional.
As example, the docker inspect on a container with two networks will looks
like that:
```json
"Networks": {
"test": {
"IPAMConfig": null,
"Links": null,
"Aliases": [
"60abf484d0f9"
],
"MacAddress": "02:42:ac:12:00:02",
"NetworkID":
"5556742fbb49569bd0e3b9219564e6db7d00e29ccc01e7eba63d0ee7a3054868",
"EndpointID":
"b4bccb21281417ad65e4cad7703585c27039c611bee93115af8a72650eacdc0e",
"Gateway": "172.18.0.1",
"IPAddress": "172.18.0.2",
"IPPrefixLen": 16,
"IPv6Gateway": "",
"GlobalIPv6Address": "",
"GlobalIPv6PrefixLen": 0,
"DriverOpts": null,
"DNSNames": [
"mesos-fa5fdb98-10c3-479d-907f-3b95ccd9f42d",
"60abf484d0f9"
]
},
"test2": {
"IPAMConfig": null,
"Links": null,
"Aliases": [
"60abf484d0f9"
],
"MacAddress": "02:42:ac:13:00:02",
"NetworkID":
"185bca7da0a98d35ead37dbfb6b159f533448877eba269afaf229c1de0b17401",
"EndpointID":
"8ea16d6ded939908818259f9aa243131db8789e5fb690bfd09aae564f83c690b",
"Gateway": "172.19.0.1",
"IPAddress": "172.19.0.2",
"IPPrefixLen": 16,
"IPv6Gateway": "",
"GlobalIPv6Address": "",
"GlobalIPv6PrefixLen": 0,
"DriverOpts": null,
"DNSNames": [
"mesos-fa5fdb98-10c3-479d-907f-3b95ccd9f42d",
"60abf484d0f9"
]
}
}
```
The mesos container info will looks like that:
```json
"container": {
"type": "DOCKER",
"docker": {
"image": "alpine:latest",
"network": "BRIDGE",
"privileged": false,
"parameters": [
{
"key": "network",
"value": "test"
},
{
"key": "network",
"value": "test2"
}
],
"force_pull_image": true
},
"network_infos": [
{
"ip_addresses": [
{
"protocol": "IPv4",
"ip_address": "172.18.0.2"
}
],
"name": "test"
}
],
```
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Revert "Add support for docker network options" [mesos]
bmahler merged PR #584: URL: https://github.com/apache/mesos/pull/584 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] Revert "Add support for docker network options" [mesos]
bmahler opened a new pull request, #584: URL: https://github.com/apache/mesos/pull/584 Reverts apache/mesos#583 Broke the build by making network un-intialized, will revert until a new patch arrives. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Add support for docker network options [mesos]
bmahler merged PR #583: URL: https://github.com/apache/mesos/pull/583 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] Add support for docker network options [mesos]
andreaspeters opened a new pull request, #583: URL: https://github.com/apache/mesos/pull/583 Short: With these PR I want to add support for custom docker networks created by `docker network ...` Long: The Mesos DockerInfo Network object only supports four network modes: BRIDGE, HOST, USER, and NONE. If we want to use a network created by the Docker command docker network create ..., we cannot use the network mode. In that case, we have to add the DockerInfo parameter. The problem is, if we do not add a DockerInfo network mode, then the Docker executor will add a default one (Lines 839, 841). This means the Docker executor will try to run the container with two network parameters. Newer Docker engines do not accept this. To address this, my idea is to only add a default network mode if DockerInfo has no network and also no network parameter. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] FIX: Add missing source file in cgroups list. [mesos]
bmahler merged PR #582: URL: https://github.com/apache/mesos/pull/582 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] FIX: Add missing source file in cgroups list. [mesos]
andreaspeters opened a new pull request, #582: URL: https://github.com/apache/mesos/pull/582 The building via cmake will fail if cgroups v2 is enabled. That's because of a missing file in the source list. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] FIX: mesos rpm add command and change mesos_isolation. [mesos]
AVENTER-UG closed pull request #422: FIX: mesos rpm add command and change mesos_isolation. URL: https://github.com/apache/mesos/pull/422 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduces the MemoryControllerProcess. [mesos]
bmahler closed pull request #581: [cgroups2] Introduces the MemoryControllerProcess. URL: https://github.com/apache/mesos/pull/581 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduces the MemoryControllerProcess. [mesos]
bmahler commented on code in PR #581:
URL: https://github.com/apache/mesos/pull/581#discussion_r1596062375
##
src/slave/containerizer/mesos/isolators/cgroups2/controllers/memory.cpp:
##
@@ -0,0 +1,203 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include
+
+#include
+#include
+#include
+
+#include
+
+#include "common/protobuf_utils.hpp"
+
+#include "linux/cgroups2.hpp"
+
+#include "slave/containerizer/mesos/isolators/cgroups2/constants.hpp"
+#include "slave/containerizer/mesos/isolators/cgroups2/controllers/memory.hpp"
+
+using process::Failure;
+using process::Future;
+using process::PID;
+using process::Owned;
+
+using cgroups2::memory::Stats;
+
+using mesos::slave::ContainerConfig;
+using mesos::slave::ContainerLimitation;
+
+using std::ostringstream;
+using std::string;
+
+namespace mesos {
+namespace internal {
+namespace slave {
+
+Try> MemoryControllerProcess::create(const Flags&
flags)
+{
+ return Owned(new MemoryControllerProcess(flags));
+}
+
+
+MemoryControllerProcess::MemoryControllerProcess(const Flags& _flags)
+ : ProcessBase(process::ID::generate("cgroups-v2-memory-controller")),
+ControllerProcess(_flags) {}
+
+
+string MemoryControllerProcess::name() const
+{
+ return CGROUPS_V2_CONTROLLER_MEMORY_NAME;
+}
+
+
+Future MemoryControllerProcess::prepare(
+const ContainerID& containerId,
+const string& cgroup,
+const ContainerConfig& containerConfig)
+{
+ if (infos.contains(containerId)) {
+return Failure("Already prepared");
+ }
+
+ infos.put(containerId, Info());
+
+ return Nothing();
+}
+
+
+Future MemoryControllerProcess::isolate(
+const ContainerID& containerId,
+const string& cgroup,
+pid_t pid)
+{
+ if (!infos.contains(containerId)) {
+return Failure("Unknown container");
+ }
+
+ // TODO(dleamy): Implement manual OOM score adjustment, similar to as it done
+ // in the cgroups v1 isolator.
+
+ return Nothing();
+}
+
+
+Future MemoryControllerProcess::recover(
+const ContainerID& containerId,
+const string& cgroup)
+{
+ if (infos.contains(containerId)) {
+return Failure("Already recovered");
+ }
+
+ infos.put(containerId, Info());
+ infos[containerId].hardLimitUpdated = true;
+
+ return Nothing();
+}
+
+
+Future MemoryControllerProcess::update(
+ const ContainerID& containerId,
+ const string& cgroup,
+ const Resources& resourceRequests,
+ const google::protobuf::Map& resourceLimits)
+{
+ if (!infos.contains(containerId)) {
+return Failure("Unknown container");
+ }
+
+ if (resourceRequests.mem().isNone()) {
+return Failure("No memory resources requested");
+ }
+
+ Bytes memory = *resourceRequests.mem();
+ Bytes softLimit = std::max(memory, MIN_MEMORY);
+
+ // Set the soft memory limit.
+ Try high = cgroups2::memory::set_low(cgroup, softLimit);
Review Comment:
s/high/low/
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Introduces the MemoryControllerProcess. [mesos]
bmahler commented on PR #577: URL: https://github.com/apache/mesos/pull/577#issuecomment-2103522764 Going to land this via #581 instead since it needed a minor fix. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduces the MemoryControllerProcess. [mesos]
bmahler closed pull request #577: [cgroups2] Introduces the MemoryControllerProcess. URL: https://github.com/apache/mesos/pull/577 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [cgroups2] Introduces the MemoryControllerProcess. [mesos]
ZhouJas opened a new pull request, #581: URL: https://github.com/apache/mesos/pull/581 Introduces the `MemoryControllerProcess`, the cgroups v2 memory isolator, which will be used by the `Cgroups2IsolatorProcess`. Unlike the `MemorySubsystemProcess`, the cgroups v1 memory isolator, we: - Don't allow limits on swap memory to be set. - Don't report memory pressure levels (this facility is no longer part of the cgroups memory controller's API) Future work may include: - Adding support for swap memory, and - Reporting the (now available) memory pressure stall information This patch updates the ROOT_MemUsage so it passes on a cgroups v2 machine using the new MemoryControllerProcess. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduces the MemoryControllerProcess. [mesos]
ZhouJas commented on code in PR #577:
URL: https://github.com/apache/mesos/pull/577#discussion_r1594680567
##
src/slave/containerizer/mesos/isolators/cgroups2/controllers/memory.cpp:
##
@@ -0,0 +1,203 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include
+
+#include
+#include
+#include
+
+#include
+
+#include "common/protobuf_utils.hpp"
+
+#include "linux/cgroups2.hpp"
+
+#include "slave/containerizer/mesos/isolators/cgroups2/constants.hpp"
+#include "slave/containerizer/mesos/isolators/cgroups2/controllers/memory.hpp"
+
+using process::Failure;
+using process::Future;
+using process::PID;
+using process::Owned;
+
+using cgroups2::memory::Stats;
+
+using mesos::slave::ContainerConfig;
+using mesos::slave::ContainerLimitation;
+
+using std::ostringstream;
+using std::string;
+
+namespace mesos {
+namespace internal {
+namespace slave {
+
+Try> MemoryControllerProcess::create(const Flags&
flags)
+{
+ return Owned(new MemoryControllerProcess(flags));
+}
+
+
+MemoryControllerProcess::MemoryControllerProcess(const Flags& _flags)
+ : ProcessBase(process::ID::generate("cgroups-v2-memory-controller")),
+ControllerProcess(_flags) {}
+
+
+string MemoryControllerProcess::name() const
+{
+ return CGROUPS_V2_CONTROLLER_MEMORY_NAME;
+}
+
+
+Future MemoryControllerProcess::prepare(
+const ContainerID& containerId,
+const string& cgroup,
+const ContainerConfig& containerConfig)
+{
+ if (infos.contains(containerId)) {
+return Failure("Already prepared");
+ }
+
+ infos.put(containerId, Info());
+
+ return Nothing();
+}
+
+
+Future MemoryControllerProcess::isolate(
+const ContainerID& containerId,
+const string& cgroup,
+pid_t pid)
+{
+ if (!infos.contains(containerId)) {
+return Failure("Unknown container");
+ }
+
+ // TODO(dleamy): Implement manual OOM score adjustment, similar to as it done
+ // in the cgroups v1 isolator.
+
+ return Nothing();
+}
+
+
+Future MemoryControllerProcess::recover(
+const ContainerID& containerId,
+const string& cgroup)
+{
+ if (infos.contains(containerId)) {
+return Failure("Already recovered");
+ }
+
+ infos.put(containerId, Info());
+ infos[containerId].hardLimitUpdated = true;
+
+ return Nothing();
+}
+
+
+Future MemoryControllerProcess::update(
+ const ContainerID& containerId,
+ const string& cgroup,
+ const Resources& resourceRequests,
+ const google::protobuf::Map& resourceLimits)
+{
+ if (!infos.contains(containerId)) {
+return Failure("Unknown container");
+ }
+
+ if (resourceRequests.mem().isNone()) {
+return Failure("No memory resources requested");
+ }
+
+ Bytes memory = *resourceRequests.mem();
+ Bytes softLimit = std::max(memory, MIN_MEMORY);
+
+ // Set the soft memory limit.
+ Try high = cgroups2::memory::set_high(cgroup, softLimit);
Review Comment:
This should be set_low instead for the same behavior as soft limit in v1
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Watch and respond to container limitations. [mesos]
bmahler merged PR #580: URL: https://github.com/apache/mesos/pull/580 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [cgroups2] Watch and respond to container limitations. [mesos]
DevinLeamy opened a new pull request, #580: URL: https://github.com/apache/mesos/pull/580 Each `ControllerProcess` used by the cgroups v2 isolator can optionally override `::watch` which is a future that resolves when a container limitation (e.g. memory limit reached) is detected. Here we introduce listening and responding to these container limitations, like is done in cgroups v1. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [cgroups2] Add memory usage reporting to the MemoryControllerProcess [mesos]
DevinLeamy opened a new pull request, #579: URL: https://github.com/apache/mesos/pull/579 Introduces `::usage` to the MemoryControllerProcess to report the total memory usage of a cgroup as well as memory usage statistics provided by `cgroups2::memory:stats`. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [cgroups2] Add OOM listening to the MemoryControllerProcess. [mesos]
DevinLeamy opened a new pull request, #578: URL: https://github.com/apache/mesos/pull/578 Introduces OOM listening to the MemoryControllerProcess so that we detect, report, and respond to OOM events. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Supplement missing 'kernel' field in 'memory.stat' with other kernel fields. [mesos]
bmahler closed pull request #576: [cgroups2] Supplement missing 'kernel' field in 'memory.stat' with other kernel fields. URL: https://github.com/apache/mesos/pull/576 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [cgroups2] Supplement missing 'kernel' field in 'memory.stat' with other kernel fields. [mesos]
DevinLeamy opened a new pull request, #576: URL: https://github.com/apache/mesos/pull/576 The 'kernel' key was introduces to 'memory.stat' in Kernel 5.18 and therefore may be missing. If it is missing, we set `kernel` in `cgroups2::memory::Stats` to be the sum of the other kernel usage fields provided in 'memory.stat'. As part of this, we add the 'slab' key (one of the kernel memory usage fields) to the `memory::Stats` structure. --- Kernel patch introducing 'kernel': https://github.com/torvalds/linux/commit/a8c49af3be5f0b4e105ef678bcf14ef102c270be -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Fix CentOS 7 build error [mesos]
bmahler closed pull request #575: [cgroups2] Fix CentOS 7 build error URL: https://github.com/apache/mesos/pull/575 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [cgroups2] Fix CentOS 7 build error [mesos]
DevinLeamy opened a new pull request, #575: URL: https://github.com/apache/mesos/pull/575 (no comment) -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Report usage statistics for the cgroups v2 isolator process. [mesos]
bmahler merged PR #574: URL: https://github.com/apache/mesos/pull/574 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Add cgroups v2 initialization and teardown logic to tests fixtures. [mesos]
bmahler closed pull request #573: [cgroups2] Add cgroups v2 initialization and teardown logic to tests fixtures. URL: https://github.com/apache/mesos/pull/573 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Split out cgroups logic from the Mesos testing `Setup` and `Teardown`. [mesos]
bmahler closed pull request #572: Split out cgroups logic from the Mesos testing `Setup` and `Teardown`. URL: https://github.com/apache/mesos/pull/572 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Update Agent test fixture to work on both cgroups v1 and v2 machines. [mesos]
DevinLeamy closed pull request #567: [cgroups2] Update Agent test fixture to work on both cgroups v1 and v2 machines. URL: https://github.com/apache/mesos/pull/567 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Update Agent test fixture to work on both cgroups v1 and v2 machines. [mesos]
DevinLeamy commented on PR #567: URL: https://github.com/apache/mesos/pull/567#issuecomment-2075697661 Closed in favor of https://github.com/apache/mesos/pull/571 and https://github.com/apache/mesos/pull/572. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] Split out cgroups logic from the Mesos testing `Setup` and `Teardown`. [mesos]
DevinLeamy opened a new pull request, #572: URL: https://github.com/apache/mesos/pull/572 (no comment) -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Update destroy to be async more robust. [mesos]
bmahler merged PR #571: URL: https://github.com/apache/mesos/pull/571 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Update destroy to be async more robust. [mesos]
bmahler commented on code in PR #571:
URL: https://github.com/apache/mesos/pull/571#discussion_r1578387065
##
src/slave/containerizer/mesos/isolators/cgroups2/cgroups2.cpp:
##
@@ -751,13 +751,27 @@ Future Cgroups2IsolatorProcess::_cleanup(
+ strings::join(", ", errors));
}
- if (cgroups2::exists(infos[containerId]->cgroup)) {
-Try destroy = cgroups2::destroy(infos[containerId]->cgroup);
-if (destroy.isError()) {
- return Failure(
- "Failed to destroy cgroup '" + infos[containerId]->cgroup + "': "
- + destroy.error());
-}
+ if (!cgroups2::exists(infos[containerId]->cgroup)) {
+return Nothing();
Review Comment:
we're not erasing from infos in this case anymore?
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Update destroy to be async more robust. [mesos]
DevinLeamy commented on code in PR #571:
URL: https://github.com/apache/mesos/pull/571#discussion_r1578339692
##
src/linux/cgroups2.cpp:
##
@@ -381,41 +381,66 @@ Try kill(const std::string& cgroup)
}
-Try destroy(const string& cgroup)
+Future destroy(const string& cgroup)
{
if (!cgroups2::exists(cgroup)) {
-return Error("Cgroup '" + cgroup + "' does not exist");
+return Failure("Cgroup '" + cgroup + "' does not exist");
}
// To destroy a subtree of cgroups we first kill all of the processes inside
// of the cgroup and then remove all of the cgroup directories, removing
// the most deeply nested directories first.
+
Try kill = cgroups2::kill(cgroup);
if (kill.isError()) {
-return Error("Failed to kill processes in cgroup: " + kill.error());
+return Failure("Failed to kill processes in cgroup: " + kill.error());
}
- Try> cgroups = cgroups2::get(cgroup);
- if (cgroups.isError()) {
-return Error("Failed to get nested cgroups: " + cgroups.error());
- }
+ // Wait until all of the processes have been killed.
+ int retries = 50;
+ Future processes = loop(
+[]() { return process::after(Milliseconds(10)); },
+[=](const Nothing&) mutable -> Future> {
+ Try> pids = cgroups2::processes(cgroup, true);
+ if (pids.isError()) {
+return Failure("Failed to fetch pids in cgroup: " + pids.error());
+ }
- vector sorted(
- std::make_move_iterator(cgroups->begin()),
- std::make_move_iterator(cgroups->end()));
- sorted.push_back(cgroup);
- std::sort(sorted.rbegin(), sorted.rend());
+ if (pids->size() == 0) {
+return Break();
+ }
- foreach (const string& cgroup, sorted) {
-const string path = cgroups2::path(cgroup);
-Try rmdir = os::rmdir(path, false);
-if (rmdir.isError()) {
- return Error(
- "Failed to remove directory '" + path + "': " + rmdir.error());
+ --retries;
+ if (retries == 0) {
+return Failure("Failed to kill all processes in cgroup");
+ }
Review Comment:
I believe this should trigger a failure because otherwise we'll run into an
error on `rmdir` which I think is less illustrative of the problem.
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Update destroy to be async more robust. [mesos]
DevinLeamy commented on code in PR #571:
URL: https://github.com/apache/mesos/pull/571#discussion_r1578338470
##
src/linux/cgroups2.cpp:
##
@@ -381,41 +381,66 @@ Try kill(const std::string& cgroup)
}
-Try destroy(const string& cgroup)
+Future destroy(const string& cgroup)
{
if (!cgroups2::exists(cgroup)) {
-return Error("Cgroup '" + cgroup + "' does not exist");
+return Failure("Cgroup '" + cgroup + "' does not exist");
}
// To destroy a subtree of cgroups we first kill all of the processes inside
// of the cgroup and then remove all of the cgroup directories, removing
// the most deeply nested directories first.
+
Try kill = cgroups2::kill(cgroup);
if (kill.isError()) {
-return Error("Failed to kill processes in cgroup: " + kill.error());
+return Failure("Failed to kill processes in cgroup: " + kill.error());
}
- Try> cgroups = cgroups2::get(cgroup);
- if (cgroups.isError()) {
-return Error("Failed to get nested cgroups: " + cgroups.error());
- }
+ // Wait until all of the processes have been killed.
+ int retries = 50;
+ Future processes = loop(
+[]() { return process::after(Milliseconds(10)); },
+[=](const Nothing&) mutable -> Future> {
+ Try> pids = cgroups2::processes(cgroup, true);
+ if (pids.isError()) {
+return Failure("Failed to fetch pids in cgroup: " + pids.error());
+ }
Review Comment:
Opted to convert the `.then` into a `.onAny` because I think I failure here
warrants on error.
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Made `cgroups2::processes` optionally recursive. [mesos]
bmahler closed pull request #570: [cgroups2] Made `cgroups2::processes` optionally recursive. URL: https://github.com/apache/mesos/pull/570 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Made `cgroups2::processes` optionally recursive. [mesos]
bmahler commented on code in PR #570:
URL: https://github.com/apache/mesos/pull/570#discussion_r1578248075
##
src/linux/cgroups2.cpp:
##
@@ -487,6 +487,27 @@ Try> processes(const string& cgroup)
pids.insert(*pid);
}
+ if (!recursive) {
+return pids;
+ }
+
+ Try> cgroups = cgroups2::get(cgroup);
+ foreach (const string& cgroup, *cgroups) {
+Try> _pids = cgroups2::processes(cgroup);
+if (_pids.isError() && cgroups2::exists(cgroup)) {
+ return Error(
+ "Failed to get processes in '" + cgroup + "': " + _pids.error());
+} else if (_pids.isError()) {
+ // The cgroup was just removed. Ignore the error and treat it as if
+ // the cgroup had no processes.
+ continue;
Review Comment:
this is going to ignore parse failures as well since you're looking at any
error from the processes call, would be better to check this on read
##
src/linux/cgroups2.cpp:
##
@@ -487,6 +487,27 @@ Try> processes(const string& cgroup)
pids.insert(*pid);
}
+ if (!recursive) {
+return pids;
+ }
+
+ Try> cgroups = cgroups2::get(cgroup);
+ foreach (const string& cgroup, *cgroups) {
Review Comment:
this can crash, naked de-reference without handling the error case
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Update destroy to be async more robust. [mesos]
bmahler commented on code in PR #571:
URL: https://github.com/apache/mesos/pull/571#discussion_r1578197877
##
src/linux/cgroups2.cpp:
##
@@ -381,41 +381,66 @@ Try kill(const std::string& cgroup)
}
-Try destroy(const string& cgroup)
+Future destroy(const string& cgroup)
{
if (!cgroups2::exists(cgroup)) {
-return Error("Cgroup '" + cgroup + "' does not exist");
+return Failure("Cgroup '" + cgroup + "' does not exist");
}
// To destroy a subtree of cgroups we first kill all of the processes inside
// of the cgroup and then remove all of the cgroup directories, removing
// the most deeply nested directories first.
+
Try kill = cgroups2::kill(cgroup);
if (kill.isError()) {
-return Error("Failed to kill processes in cgroup: " + kill.error());
+return Failure("Failed to kill processes in cgroup: " + kill.error());
}
- Try> cgroups = cgroups2::get(cgroup);
- if (cgroups.isError()) {
-return Error("Failed to get nested cgroups: " + cgroups.error());
- }
+ // Wait until all of the processes have been killed.
+ int retries = 50;
+ Future processes = loop(
+[]() { return process::after(Milliseconds(10)); },
Review Comment:
can you reduce this to 1 millisecond?
##
src/linux/cgroups2.cpp:
##
@@ -381,41 +381,66 @@ Try kill(const std::string& cgroup)
}
-Try destroy(const string& cgroup)
+Future destroy(const string& cgroup)
{
if (!cgroups2::exists(cgroup)) {
-return Error("Cgroup '" + cgroup + "' does not exist");
+return Failure("Cgroup '" + cgroup + "' does not exist");
}
// To destroy a subtree of cgroups we first kill all of the processes inside
// of the cgroup and then remove all of the cgroup directories, removing
// the most deeply nested directories first.
+
Try kill = cgroups2::kill(cgroup);
if (kill.isError()) {
-return Error("Failed to kill processes in cgroup: " + kill.error());
+return Failure("Failed to kill processes in cgroup: " + kill.error());
}
- Try> cgroups = cgroups2::get(cgroup);
- if (cgroups.isError()) {
-return Error("Failed to get nested cgroups: " + cgroups.error());
- }
+ // Wait until all of the processes have been killed.
+ int retries = 50;
+ Future processes = loop(
+[]() { return process::after(Milliseconds(10)); },
+[=](const Nothing&) mutable -> Future> {
+ Try> pids = cgroups2::processes(cgroup, true);
+ if (pids.isError()) {
+return Failure("Failed to fetch pids in cgroup: " + pids.error());
+ }
- vector sorted(
- std::make_move_iterator(cgroups->begin()),
- std::make_move_iterator(cgroups->end()));
- sorted.push_back(cgroup);
- std::sort(sorted.rbegin(), sorted.rend());
+ if (pids->size() == 0) {
+return Break();
+ }
- foreach (const string& cgroup, sorted) {
-const string path = cgroups2::path(cgroup);
-Try rmdir = os::rmdir(path, false);
-if (rmdir.isError()) {
- return Error(
- "Failed to remove directory '" + path + "': " + rmdir.error());
+ --retries;
+ if (retries == 0) {
+return Failure("Failed to kill all processes in cgroup");
+ }
Review Comment:
ditto here, maybe this should just Break?
##
src/linux/cgroups2.cpp:
##
@@ -381,41 +381,66 @@ Try kill(const std::string& cgroup)
}
-Try destroy(const string& cgroup)
+Future destroy(const string& cgroup)
{
if (!cgroups2::exists(cgroup)) {
-return Error("Cgroup '" + cgroup + "' does not exist");
+return Failure("Cgroup '" + cgroup + "' does not exist");
}
// To destroy a subtree of cgroups we first kill all of the processes inside
// of the cgroup and then remove all of the cgroup directories, removing
// the most deeply nested directories first.
+
Try kill = cgroups2::kill(cgroup);
if (kill.isError()) {
-return Error("Failed to kill processes in cgroup: " + kill.error());
+return Failure("Failed to kill processes in cgroup: " + kill.error());
}
- Try> cgroups = cgroups2::get(cgroup);
- if (cgroups.isError()) {
-return Error("Failed to get nested cgroups: " + cgroups.error());
- }
+ // Wait until all of the processes have been killed.
+ int retries = 50;
+ Future processes = loop(
+[]() { return process::after(Milliseconds(10)); },
+[=](const Nothing&) mutable -> Future> {
+ Try> pids = cgroups2::processes(cgroup, true);
+ if (pids.isError()) {
+return Failure("Failed to fetch pids in cgroup: " + pids.error());
+ }
- vector sorted(
- std::make_move_iterator(cgroups->begin()),
- std::make_move_iterator(cgroups->end()));
- sorted.push_back(cgroup);
- std::sort(sorted.rbegin(), sorted.rend());
+ if (pids->size() == 0) {
+return Break();
+ }
- foreach (const string& cgroup, sorted) {
-const string path = cgroups2::path(cgroup);
-Try rmdir = os::rmdir(path, false);
-if (rmdir.isError()) {
-
Re: [PR] [cgroups2] Don't enable controllers in the leaf cgroup. [mesos]
bmahler merged PR #569: URL: https://github.com/apache/mesos/pull/569 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Update Agent test fixture to work on both cgroups v1 and v2 machines. [mesos]
DevinLeamy commented on code in PR #567:
URL: https://github.com/apache/mesos/pull/567#discussion_r1578178126
##
src/tests/mesos.cpp:
##
@@ -579,7 +585,13 @@ slave::Flags
ContainerizerTest::CreateSlaveFlags()
// Use cgroup isolators if they're available and we're root.
// TODO(idownes): Refactor the cgroups/non-cgroups code.
- if (cgroups::enabled() && user.get() == "root") {
+ if (cgroupsV2() && *user == "root") {
+// TODO(dleamy): Add the memory isolator once it's supported by the cgroups
+// v2 isolator.
+flags.isolation = "cgroups/cpu";
+flags.cgroups_hierarchy = "/sys/fs/cgroup";
+flags.cgroups_root = TEST_CGROUPS_ROOT;
Review Comment:
We don't create a unique `flags.cgroups_root` like in cgroups v1. The source
of that change [[1]](https://issues.apache.org/jira/browse/MESOS-926),
[[2]](https://github.com/apache/mesos/commit/0f3f8f35a73dd5f9b35d657b3912449f570243d3#diff-a6984932a4ddcf84bf808f053133927e627d81583011efe943ba46531181a4d4)
doesn't draw specific attention to the change. My view, in cgroups v2, is that
we can trust our creation and destruction fixtures to create and destroy the
test cgroup hierarchy on every test run, removing the need for this.
The reason I don't _just follow the convention in cgroups v1_ here is
because we need to know the _exact_ name of the root test cgroup to create it,
inside of `SetupCgroupsV2`, and we don't have access the flag
`flags.cgroups_root` directly. We _could_ store the root cgroup inside of a
member variable; I thought this was simpler.
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Update destroy to be async more robust. [mesos]
DevinLeamy commented on code in PR #571: URL: https://github.com/apache/mesos/pull/571#discussion_r1578088671 ## src/slave/containerizer/mesos/isolators/cgroups2/cgroups2.hpp: ## @@ -149,7 +149,7 @@ class Cgroups2IsolatorProcess : public MesosIsolatorProcess process::Future __cleanup( const ContainerID& containerId, - const std::vector>& futures); + const process::Future& future); Review Comment: There was already a `__cleanup` function (that wasn't being used) inside of the `Cgroups2IsolatorProcess`. Therefore, rather than having to _add_ a `__cleanup` function, we just needed to update the parameters it took. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [cgroups2] Made `cgroups2::processes` optionally recursive. [mesos]
DevinLeamy opened a new pull request, #570: URL: https://github.com/apache/mesos/pull/570 Previously, `cgroups2::processes` could only fetch processes from inside of the provided cgroup. We can now fetch all of the processes inside of a cgroup subtree by passing an (optional) `recursive` flag. ```c++ Try> processes( const std::string& cgroup, bool recursive = false); ``` -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Fix error message to show the correct path. [mesos]
bmahler merged PR #568: URL: https://github.com/apache/mesos/pull/568 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduces API to read the peak memory usage. [mesos]
DevinLeamy commented on PR #566: URL: https://github.com/apache/mesos/pull/566#issuecomment-2072846999 Was intended to be used when there is an OOM event to report the maximum memory usage. Opted to use the value of `memory.current` at the time when the OOM takes place instead, because `memory.peak` does not exist on all hosts that support cgroups v2. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduces API to read the peak memory usage. [mesos]
DevinLeamy closed pull request #566: [cgroups2] Introduces API to read the peak memory usage. URL: https://github.com/apache/mesos/pull/566 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Update Agent test fixture to work on both cgroups v1 and v2 machines. [mesos]
DevinLeamy commented on code in PR #567:
URL: https://github.com/apache/mesos/pull/567#discussion_r1576512117
##
src/tests/mesos.cpp:
##
@@ -645,9 +664,16 @@ void
ContainerizerTest::TearDownTestCase()
Result user = os::user();
EXPECT_SOME(user);
- if (cgroups::enabled() && user.get() == "root") {
+ if (cgroupsV2() && *user == "root") {
+#ifdef ENABLE_CGROUPS_V2
+// Clean up test cgroups.
+if (cgroups2::exists(TEST_CGROUPS_ROOT)) {
+ ASSERT_SOME(cgroups2::destroy(TEST_CGROUPS_ROOT));
+}
+#endif // ENABLE_CGROUPS_V2
+ } else if (cgroups::enabled() && *user == "root") {
// Clean up any testing hierarchies.
-Try> hierarchies = cgroups::hierarchies();
+Try> hierarchies = cgroups::hierarchies();
Review Comment:
There are a few places where I removed `std::`. This is one instance. I
didn't feel it warranted a separate PR.
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] [cgroups2] Update Agent test fixture to work on both cgroups v1 and v2 machines. [mesos]
DevinLeamy opened a new pull request, #567: URL: https://github.com/apache/mesos/pull/567 `StartSlave()` and similar test-setup functions mounted cgroups v1 hierarchies and initialized controllers. On cgroups v2 machines, this setup would fail or result in irregular cgroup setups. As a step towards end-to-end testing for the MesosContainerizer, we update the Agent test fixtures such that they work correctly on both cgroups v1 and v2 hosts. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Implement Cgroups 2 isolator w/o nested containers and systemd. [mesos]
bmahler closed pull request #556: [cgroups2] Implement Cgroups 2 isolator w/o nested containers and systemd. URL: https://github.com/apache/mesos/pull/556 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Implement Cgroups 2 isolator w/o nested containers and systemd. [mesos]
bmahler commented on code in PR #556:
URL: https://github.com/apache/mesos/pull/556#discussion_r1575260114
##
src/slave/containerizer/mesos/isolators/cgroups2/cgroups2.cpp:
##
@@ -120,6 +144,622 @@ bool Cgroups2IsolatorProcess::supportsStandalone()
return true;
}
+
+Future> Cgroups2IsolatorProcess::prepare(
+const ContainerID& containerId,
+const ContainerConfig& containerConfig)
+{
+ if (containerId.has_parent()) {
+return Failure("cgroups v2 does not support nested containers");
+ }
+
+ if (infos.contains(containerId)) {
+return Failure(
+"Container with id '" + stringify(containerId) + "' "
+"has already been prepared");
+ }
+
+ CHECK(containerConfig.container_class() != ContainerClass::DEBUG);
+
+ // Create the non-leaf and leaf cgroups for the container, enable
+ // controllers in the non-leaf cgroup, and `prepare` each of the controllers.
+ const string& nonLeafCgroup = cgroups2_paths::container(
+ flags.cgroups_root, containerId);
+ if (cgroups2::exists(nonLeafCgroup)) {
+return Failure("Cgroup '" + nonLeafCgroup + "' already exists");
+ }
+
+ Try createNonLeafCgroup = cgroups2::create(nonLeafCgroup);
+ if (createNonLeafCgroup.isError()) {
+return Failure("Failed to create cgroup '" + nonLeafCgroup + "': "
+ + createNonLeafCgroup.error());
+ }
+
+ const string& leafCgroup = cgroups2_paths::container(
+ flags.cgroups_root, containerId, true);
+ if (cgroups2::exists(leafCgroup)) {
+return Failure("Cgroup '" + leafCgroup + "' already exists");
+ }
+
+ Try createLeafCgroup = cgroups2::create(leafCgroup);
+ if (createLeafCgroup.isError()) {
+return Failure("Failed to create cgroup '" + leafCgroup + "': "
+ + createLeafCgroup.error());
+ }
+
+ LOG(INFO) << "Created cgroups '" << nonLeafCgroup << "' "
+<< "and '" << leafCgroup << "'";
+
+ infos[containerId] = Owned(new Info(containerId, nonLeafCgroup));
+ vector> prepares;
+ foreachvalue (const Owned& controller, controllers) {
+if (controller->name() == "core") {
+ // The "core" controller is always enabled because the "cgroup.*" control
+ // files exist for all cgroups. Additionally, since "core" isn't a
+ // valid controller name (i.e. it doesn't exist in "cgroup.controllers"),
+ // calling `cgroups2::controllers::enable` with the "core" cgroup will
+ // fail with "Invalid argument".
+ //
+ // For that reason, we skip enabling the "core" controller here.
+ continue;
+}
+
+Try enableInNonLeaf =
+ cgroups2::controllers::enable(nonLeafCgroup, {controller->name()});
+if (enableInNonLeaf.isError()) {
+ return Failure(
+ "Failed to enable controller '" + controller->name() + "' "
+ "in cgroup '" + nonLeafCgroup + "': " + enableInNonLeaf.error());
+}
+
+// We enable controllers in the leaf cgroup to allow the container process
+// to manage their own cgroups, if they choose.
+Try enableInLeaf =
+ cgroups2::controllers::enable(leafCgroup, {controller->name()});
+if (enableInLeaf.isError()) {
+ return Failure(
+ "Failed to enable controllers in cgroup '" + nonLeafCgroup + "': "
+ + enableInLeaf.error());
+}
+
+infos[containerId]->controllers.insert(controller->name());
+prepares.push_back(
+controller->prepare(containerId, nonLeafCgroup, containerConfig));
+ }
+
+ return await(prepares)
+.then(defer(
+PID(this),
+::_prepare,
+containerId,
+containerConfig,
+lambda::_1));
+}
+
+
+Future> Cgroups2IsolatorProcess::_prepare(
+const ContainerID& containerId,
+const ContainerConfig& containerConfig,
+const vector>& futures)
+{
+ vector errors;
+ foreach (const Future& future, futures) {
+if (!future.isReady()) {
+ errors.push_back(future.isFailed() ? future.failure() : "discarded");
+}
+ }
+
+ if (!errors.empty()) {
+return Failure(
+"Failed to prepare controllers: " + strings::join(": ", errors));
+ }
+
+ return update(
+ containerId,
+ containerConfig.resources(),
+ containerConfig.limits())
+.then(defer(
+PID(this),
+::__prepare,
+containerId,
+containerConfig));
+}
+
+
+Future> Cgroups2IsolatorProcess::__prepare(
+const ContainerID& containerId,
+const ContainerConfig& containerConfig)
+{
+ // Only create cgroup mounts for containers with rootfs.
+ if (!containerConfig.has_rootfs()) {
+return None();
+ }
+
+ Owned info = cgroupInfo(containerId);
+ if (!info.get()) {
+return Failure(
+"Failed to get cgroup for container "
+"'" + stringify(containerId) + "'");
+ }
+
+ ContainerLaunchInfo launchInfo;
+ // Create a new cgroup namespace. The child process will only be able to
+ // see the cgroups that are in its cgroup subtree.
+ launchInfo.add_clone_namespaces(CLONE_NEWCGROUP);
+
+ //
Re: [PR] [cgroups2] Error if `--cgroups_limit_swap` is used when cgroups v2 is used. [mesos]
DevinLeamy commented on PR #565: URL: https://github.com/apache/mesos/pull/565#issuecomment-2069528306 > > This is because the cgroups v2 API does not allow us to directly control the swap memory usage limit. Rather, we can only control the combined swap and RAM usage limit. > > This doesn't sound accurate? in v2 we are able to separately control swap usage It's the opposite - v1 combines RAM and swap and v2 gives direct control over both; you're right :) -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduces an interface to read a subset of the memory usage statistics. [mesos]
bmahler closed pull request #564: [cgroups2] Introduces an interface to read a subset of the memory usage statistics. URL: https://github.com/apache/mesos/pull/564 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduces an interface to read a subset of the memory usage statistics. [mesos]
bmahler commented on code in PR #564:
URL: https://github.com/apache/mesos/pull/564#discussion_r1573045523
##
src/linux/cgroups2.hpp:
##
@@ -243,6 +243,41 @@ Try max(const std::string& cgroup);
// See: https://docs.kernel.org/admin-guide/cgroup-v2.html
namespace memory {
+// Memory usage statistics.
+//
+// Snapshot of the 'memory.stat' control file.
+//
+// Note:
+// We only record a subset of the memory statistics; a complete list can be
+// found in the kernel documentation.
+// https://docs.kernel.org/admin-guide/cgroup-v2.html#memory-interface-files
+struct Stats
+{
+ // Anonymous memory usage.
Review Comment:
let's just use the cgroups names and docs for these?
##
src/linux/cgroups2.cpp:
##
@@ -964,6 +1005,17 @@ Result high(const string& cgroup)
return internal::parse_bytelimit(*contents);
}
+
+Try stats(const string& cgroup)
+{
+ Try contents = cgroups2::read(cgroup, control::STAT);
+ if (contents.isError()) {
+return Error("Failed to read 'memory.stat': " + contents.error());
+ }
+
+ return control::stat::parse(strings::trim(*contents));
Review Comment:
hm.. the parse function should know how to parse without the trimming, was
this actually needed?
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Error if `--cgroups_limit_swap` is used when cgroups v2 is used. [mesos]
bmahler closed pull request #565: [cgroups2] Error if `--cgroups_limit_swap` is used when cgroups v2 is used. URL: https://github.com/apache/mesos/pull/565 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Error if `--cgroups_limit_swap` is used when cgroups v2 is used. [mesos]
bmahler commented on PR #565: URL: https://github.com/apache/mesos/pull/565#issuecomment-2067370799 > This is because the cgroups v2 API does not allow us to directly control the swap memory usage limit. Rather, we can only control the combined swap and RAM usage limit. This doesn't sound accurate? in v2 we are able to separately control swap usage -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Error if `--cgroups_limit_swap` is used when cgroups v2 is used. [mesos]
bmahler commented on code in PR #565:
URL: https://github.com/apache/mesos/pull/565#discussion_r1573038742
##
src/slave/flags.cpp:
##
@@ -652,8 +657,25 @@ mesos::internal::slave::Flags::Flags()
add(::cgroups_limit_swap,
"cgroups_limit_swap",
"Cgroups feature flag to enable memory limits on both memory and\n"
- "swap instead of just memory.\n",
- false);
+ "swap instead of just memory. Not supported if cgroups v2 is used.\n",
+ false,
+ [](const bool& limit_swap) -> Option {
+#ifdef ENABLE_CGROUPS_V2
+Try mounted = cgroups2::mounted();
+if (mounted.isError()) {
+return Error("Failed to check if cgroup2 filesystem is mounted: "
Review Comment:
nit: 2 space indent here and below
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] [cgroups2] Error if `--cgroups_limit_swap` is used when cgroups v2 is used. [mesos]
DevinLeamy opened a new pull request, #565: URL: https://github.com/apache/mesos/pull/565 Mesos does not support limiting swap memory when using cgroups v2. This is because the cgroups v2 API does not allow us to directly control the swap memory usage limit. Rather, we can only control the combined swap _and_ RAM usage limit. Therefore, when the `--cgroups_limit_swap` flag is provided and cgroups v2 is used we error during flag validation. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [cgroups2] Introduces an interface to read a subset of the memory usage statistics. [mesos]
DevinLeamy opened a new pull request, #564: URL: https://github.com/apache/mesos/pull/564 Cgroups v2 exposes memory statistics through the 'memory.stat' control. Here we introduce `cgroups2::memory::stats` to read a subset of the memory usage statistics into a new `memory::Stats` object. These statistics will be used by the `MemoryControllerProcess` to populate a `ResourceStatistics` object, like is done by the `MemorySubsystemProcess` in cgroups v1. Additional statistics from the 'memory.stat' control can be included as they are required. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Implement Cgroups 2 isolator w/o nested containers and systemd. [mesos]
DevinLeamy commented on code in PR #556:
URL: https://github.com/apache/mesos/pull/556#discussion_r1572831258
##
src/slave/containerizer/mesos/isolators/cgroups2/cgroups2.cpp:
##
@@ -120,6 +144,569 @@ bool Cgroups2IsolatorProcess::supportsStandalone()
return true;
}
+
+Future> Cgroups2IsolatorProcess::prepare(
+const ContainerID& containerId,
+const ContainerConfig& containerConfig)
+{
+ if (containerId.has_parent()) {
+return Failure("cgroups v2 does not support nested containers");
+ }
+
+ if (infos.contains(containerId)) {
+return Failure(
+"Container with id '" + stringify(containerId) + "' "
+"has already been prepared");
+ }
+
+ CHECK(containerConfig.container_class() != ContainerClass::DEBUG);
+
+ // Create the non-leaf and leaf cgroups for the container, enable
+ // controllers in the non-leaf cgroup, and `prepare` each of the controllers.
+ const string& cgroup = cgroups2_paths::container(
+ flags.cgroups_root, containerId);
+ if (cgroups2::exists(cgroup)) {
+return Failure("Cgroup '" + cgroup + "' already exists");
+ }
+
+ Try createCgroup = cgroups2::create(cgroup);
+ if (createCgroup.isError()) {
+return Failure("Failed to create cgroup '" + cgroup + "': "
+ + createCgroup.error());
+ }
+
+ const string& leafCgroup = cgroups2_paths::container(
+ flags.cgroups_root, containerId, true);
+ if (cgroups2::exists(leafCgroup)) {
+return Failure("Cgroup '" + leafCgroup + "' already exists");
+ }
+
+ Try createLeafCgroup = cgroups2::create(leafCgroup);
+ if (createLeafCgroup.isError()) {
+return Failure("Failed to create cgroup '" + leafCgroup + "': "
+ + createLeafCgroup.error());
+ }
+
+ LOG(INFO) << "Created cgroups '" << cgroup << "' and '" << leafCgroup << "'";
+
+ infos[containerId] = Owned(new Info(containerId, cgroup));
+ vector> prepares;
+ foreachvalue (const Owned& controller, controllers) {
+// Enable controllers in the non-leaf cgroup.
+Try enable =
+ cgroups2::controllers::enable(cgroup, {controller->name()});
+
+infos[containerId]->controllers.insert(controller->name());
+prepares.push_back(
+controller->prepare(containerId, cgroup, containerConfig));
+ }
+
+ return await(prepares)
+.then(defer(
+PID(this),
+::_prepare,
+containerId,
+containerConfig,
+lambda::_1));
+}
+
+
+Future> Cgroups2IsolatorProcess::_prepare(
+const ContainerID& containerId,
+const ContainerConfig& containerConfig,
+const vector>& futures)
+{
+ vector errors;
+ foreach (const Future& future, futures) {
+if (!future.isReady()) {
+ errors.push_back(future.isFailed() ? future.failure() : "discarded");
+}
+ }
+
+ if (!errors.empty()) {
+return Failure(
+"Failed to prepare controllers: " + strings::join(": ", errors));
+ }
+
+ return update(
+ containerId,
+ containerConfig.resources(),
+ containerConfig.limits())
+.then(defer(
+PID(this),
+::__prepare,
+containerId,
+containerConfig));
+}
+
+
+Future> Cgroups2IsolatorProcess::__prepare(
+const ContainerID& containerId,
+const ContainerConfig& containerConfig)
+{
+ // Only create cgroup mounts for containers with rootfs.
+ if (!containerConfig.has_rootfs()) {
+return None();
+ }
+
+ Owned info = cgroupInfo(containerId);
+ if (!info.get()) {
+return Failure(
+"Failed to get cgroup for container "
+"'" + stringify(containerId) + "'");
+ }
+
+ ContainerLaunchInfo launchInfo;
+ // Create a new cgroup namespace. The child process will only be able to
+ // see the cgroups that are in its cgroup subtree.
+ launchInfo.add_clone_namespaces(CLONE_NEWCGROUP);
+
+ // Create a new mount namespace and mount the root cgroup at /sys/fs/cgroup.
+ launchInfo.add_clone_namespaces(CLONE_NEWNS);
+ *launchInfo.add_mounts() = protobuf::slave::createContainerMount(
+ cgroups2::path(info->cgroup),
+ path::join(containerConfig.rootfs(), "/sys/fs/cgroup"),
+ MS_BIND | MS_REC);
+
+ return launchInfo;
+}
+
+
+Future Cgroups2IsolatorProcess::recover(
+const vector& states,
+const hashset& orphans)
+{
+ vector> recovers;
+ foreach (const ContainerState& state, states) {
+recovers.push_back(___recover(state.container_id()));
+ }
+
+ return await(recovers)
+.then(defer(
+PID(this),
+::_recover,
+orphans,
+lambda::_1));
+}
+
+
+Future Cgroups2IsolatorProcess::_recover(
+ const hashset& orphans,
+ const vector>& futures)
+{
+ vector errors;
+ foreach (const Future& future, futures) {
+if (!future.isReady()) {
+ errors.push_back(future.isFailed() ? future.failure() : "discarded");
+}
+ }
+
+ if (errors.size() > 0) {
+return Failure("Failed to recover active containers: " +
+ strings::join(": ", errors));
+ }
+
+ hashset knownOrphans;
+ hashset
Re: [PR] [cgroups2] Introduces API to make cgroups threaded and read their type. [mesos]
DevinLeamy commented on PR #562:
URL: https://github.com/apache/mesos/pull/562#issuecomment-2066644211
Since we want the isolated ("containerized") process to be able to
manipulate its own cgroups, if it so chooses, we're opting to no have the leaf
cgroup for each container be threaded, hence removing the motivation for 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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Introduces API to make cgroups threaded and read their type. [mesos]
DevinLeamy closed pull request #562: [cgroups2] Introduces API to make cgroups threaded and read their type. URL: https://github.com/apache/mesos/pull/562 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduced API to listen for OOM events. [mesos]
bmahler commented on code in PR #563:
URL: https://github.com/apache/mesos/pull/563#discussion_r1571502311
##
src/linux/cgroups2.hpp:
##
@@ -241,6 +243,48 @@ Try max(const std::string& cgroup);
// See: https://docs.kernel.org/admin-guide/cgroup-v2.html
namespace memory {
+// Cgroup memory controller events.
+//
+// Snapshot of the 'memory.events' or 'memory.local.events' control files.
+struct Events
+{
+ // Number of times memory is reclaimed from the cgroup despite being under
+ // the low memory threshold. Usually indicates that the low threshold is too
+ // high.
Review Comment:
low threshold is too high -> low boundaries are over-committed
come to think of it, let's just use the cgroups v2 explanations since they
are pretty short?
##
src/tests/containerizer/cgroups2_tests.cpp:
##
@@ -484,6 +489,46 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_GetCgroups)
}
+TEST_F(Cgroups2Test, ROOT_CGROUPS2_OomDetection)
+{
+ // Check that exceeding the hard memory limit will trigger an OOM event.
+ //
+ // We set a hard memory limit on the test cgroup, move a process inside of
+ // the test cgroup subtree, listen for an OOM event, deliberately trigger an
+ // OOM event by exceeding the hard limit, and then check that an event was
+ // triggered.
+ ASSERT_SOME(enable_controllers({"memory"}));
+
+ ASSERT_SOME(cgroups2::create(TEST_CGROUP));
+ ASSERT_SOME(cgroups2::controllers::enable(TEST_CGROUP, {"memory"}));
+
+ const string leaf_cgroup = TEST_CGROUP + "/leaf";
+ ASSERT_SOME(cgroups2::create(leaf_cgroup));
+
+ Try memory = os::memory();
+ ASSERT_SOME(memory);
Review Comment:
not needed anymore?
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Introduced API to listen for OOM events. [mesos]
bmahler closed pull request #563: [cgroups2] Introduced API to listen for OOM events. URL: https://github.com/apache/mesos/pull/563 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Implement Cgroups 2 isolator w/o nested containers and systemd. [mesos]
DevinLeamy commented on code in PR #556:
URL: https://github.com/apache/mesos/pull/556#discussion_r1569570036
##
src/slave/containerizer/mesos/isolators/cgroups2/cgroups2.cpp:
##
@@ -120,6 +144,569 @@ bool Cgroups2IsolatorProcess::supportsStandalone()
return true;
}
+
+Future> Cgroups2IsolatorProcess::prepare(
+const ContainerID& containerId,
+const ContainerConfig& containerConfig)
+{
+ if (containerId.has_parent()) {
+return Failure("cgroups v2 does not support nested containers");
+ }
+
+ if (infos.contains(containerId)) {
+return Failure(
+"Container with id '" + stringify(containerId) + "' "
+"has already been prepared");
+ }
+
+ CHECK(containerConfig.container_class() != ContainerClass::DEBUG);
+
+ // Create the non-leaf and leaf cgroups for the container, enable
+ // controllers in the non-leaf cgroup, and `prepare` each of the controllers.
+ const string& cgroup = cgroups2_paths::container(
+ flags.cgroups_root, containerId);
+ if (cgroups2::exists(cgroup)) {
+return Failure("Cgroup '" + cgroup + "' already exists");
+ }
+
+ Try createCgroup = cgroups2::create(cgroup);
+ if (createCgroup.isError()) {
+return Failure("Failed to create cgroup '" + cgroup + "': "
+ + createCgroup.error());
+ }
+
+ const string& leafCgroup = cgroups2_paths::container(
+ flags.cgroups_root, containerId, true);
+ if (cgroups2::exists(leafCgroup)) {
+return Failure("Cgroup '" + leafCgroup + "' already exists");
+ }
+
+ Try createLeafCgroup = cgroups2::create(leafCgroup);
+ if (createLeafCgroup.isError()) {
+return Failure("Failed to create cgroup '" + leafCgroup + "': "
+ + createLeafCgroup.error());
+ }
+
+ LOG(INFO) << "Created cgroups '" << cgroup << "' and '" << leafCgroup << "'";
+
+ infos[containerId] = Owned(new Info(containerId, cgroup));
+ vector> prepares;
+ foreachvalue (const Owned& controller, controllers) {
+// Enable controllers in the non-leaf cgroup.
+Try enable =
+ cgroups2::controllers::enable(cgroup, {controller->name()});
+
+infos[containerId]->controllers.insert(controller->name());
+prepares.push_back(
+controller->prepare(containerId, cgroup, containerConfig));
+ }
+
+ return await(prepares)
+.then(defer(
+PID(this),
+::_prepare,
+containerId,
+containerConfig,
+lambda::_1));
+}
+
+
+Future> Cgroups2IsolatorProcess::_prepare(
+const ContainerID& containerId,
+const ContainerConfig& containerConfig,
+const vector>& futures)
+{
+ vector errors;
+ foreach (const Future& future, futures) {
+if (!future.isReady()) {
+ errors.push_back(future.isFailed() ? future.failure() : "discarded");
+}
+ }
+
+ if (!errors.empty()) {
+return Failure(
+"Failed to prepare controllers: " + strings::join(": ", errors));
+ }
+
+ return update(
+ containerId,
+ containerConfig.resources(),
+ containerConfig.limits())
+.then(defer(
+PID(this),
+::__prepare,
+containerId,
+containerConfig));
+}
+
+
+Future> Cgroups2IsolatorProcess::__prepare(
+const ContainerID& containerId,
+const ContainerConfig& containerConfig)
+{
+ // Only create cgroup mounts for containers with rootfs.
+ if (!containerConfig.has_rootfs()) {
+return None();
+ }
+
+ Owned info = cgroupInfo(containerId);
+ if (!info.get()) {
+return Failure(
+"Failed to get cgroup for container "
+"'" + stringify(containerId) + "'");
+ }
+
+ ContainerLaunchInfo launchInfo;
+ // Create a new cgroup namespace. The child process will only be able to
+ // see the cgroups that are in its cgroup subtree.
+ launchInfo.add_clone_namespaces(CLONE_NEWCGROUP);
+
+ // Create a new mount namespace and mount the root cgroup at /sys/fs/cgroup.
+ launchInfo.add_clone_namespaces(CLONE_NEWNS);
+ *launchInfo.add_mounts() = protobuf::slave::createContainerMount(
+ cgroups2::path(info->cgroup),
+ path::join(containerConfig.rootfs(), "/sys/fs/cgroup"),
+ MS_BIND | MS_REC);
+
+ return launchInfo;
+}
+
+
+Future Cgroups2IsolatorProcess::recover(
+const vector& states,
+const hashset& orphans)
+{
+ vector> recovers;
+ foreach (const ContainerState& state, states) {
+recovers.push_back(___recover(state.container_id()));
+ }
+
+ return await(recovers)
+.then(defer(
+PID(this),
+::_recover,
+orphans,
+lambda::_1));
+}
+
+
+Future Cgroups2IsolatorProcess::_recover(
+ const hashset& orphans,
+ const vector>& futures)
+{
+ vector errors;
+ foreach (const Future& future, futures) {
+if (!future.isReady()) {
+ errors.push_back(future.isFailed() ? future.failure() : "discarded");
+}
+ }
+
+ if (errors.size() > 0) {
+return Failure("Failed to recover active containers: " +
+ strings::join(": ", errors));
+ }
+
+ hashset knownOrphans;
+ hashset
Re: [PR] [cgroups2] Implement Cgroups 2 isolator w/o nested containers and systemd. [mesos]
DevinLeamy commented on code in PR #556:
URL: https://github.com/apache/mesos/pull/556#discussion_r1569562534
##
src/slave/containerizer/mesos/isolators/cgroups2/cgroups2.cpp:
##
@@ -120,6 +144,569 @@ bool Cgroups2IsolatorProcess::supportsStandalone()
return true;
}
+
+Future> Cgroups2IsolatorProcess::prepare(
+const ContainerID& containerId,
+const ContainerConfig& containerConfig)
+{
+ if (containerId.has_parent()) {
+return Failure("cgroups v2 does not support nested containers");
+ }
+
+ if (infos.contains(containerId)) {
+return Failure(
+"Container with id '" + stringify(containerId) + "' "
+"has already been prepared");
+ }
+
+ CHECK(containerConfig.container_class() != ContainerClass::DEBUG);
+
+ // Create the non-leaf and leaf cgroups for the container, enable
+ // controllers in the non-leaf cgroup, and `prepare` each of the controllers.
+ const string& cgroup = cgroups2_paths::container(
+ flags.cgroups_root, containerId);
+ if (cgroups2::exists(cgroup)) {
+return Failure("Cgroup '" + cgroup + "' already exists");
+ }
+
+ Try createCgroup = cgroups2::create(cgroup);
+ if (createCgroup.isError()) {
+return Failure("Failed to create cgroup '" + cgroup + "': "
+ + createCgroup.error());
+ }
+
+ const string& leafCgroup = cgroups2_paths::container(
+ flags.cgroups_root, containerId, true);
+ if (cgroups2::exists(leafCgroup)) {
+return Failure("Cgroup '" + leafCgroup + "' already exists");
+ }
+
+ Try createLeafCgroup = cgroups2::create(leafCgroup);
+ if (createLeafCgroup.isError()) {
+return Failure("Failed to create cgroup '" + leafCgroup + "': "
+ + createLeafCgroup.error());
+ }
+
+ LOG(INFO) << "Created cgroups '" << cgroup << "' and '" << leafCgroup << "'";
+
+ infos[containerId] = Owned(new Info(containerId, cgroup));
+ vector> prepares;
+ foreachvalue (const Owned& controller, controllers) {
+// Enable controllers in the non-leaf cgroup.
+Try enable =
+ cgroups2::controllers::enable(cgroup, {controller->name()});
Review Comment:
Enable the controllers in the leaf controllers so that containers have the
ability to manage their own resources and/or enable controllers in sub cgroups.
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Implement Cgroups 2 isolator w/o nested containers and systemd. [mesos]
DevinLeamy commented on code in PR #556:
URL: https://github.com/apache/mesos/pull/556#discussion_r1569553390
##
src/slave/containerizer/mesos/isolators/cgroups2/cgroups2.hpp:
##
@@ -32,6 +35,23 @@ namespace mesos {
namespace internal {
namespace slave {
+// Cgroups v2 Mesos isolator.
+//
+// Manages the cgroup v2 controllers that are used by containers. Each
+// container is associated with two cgroups: a non-leaf cgroup whose control
+// files are updated and a leaf cgroup where the container process lives.
+// The container pid cannot live in the non-leaf cgroup because of the cgroups
+// v2 internal process constraint.
+// Learn more here:
https://docs.kernel.org/admin-guide/cgroup-v2.html#no-internal-process-constraint
Review Comment:
Add no-lint.
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Implement Cgroups 2 isolator w/o nested containers and systemd. [mesos]
DevinLeamy commented on PR #556: URL: https://github.com/apache/mesos/pull/556#issuecomment-2062130720 Renamed "domain cgroup" to "non-leaf cgroup". Added a comment with a diagram to the Cgroups2IsolatorProcess header to illustrate the cgroup structure for a container. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduces API to make cgroups threaded and check their type. [mesos]
DevinLeamy commented on code in PR #562:
URL: https://github.com/apache/mesos/pull/562#discussion_r1569352376
##
src/linux/cgroups2.hpp:
##
@@ -101,6 +101,41 @@ Try> threads(const std::string& cgroup);
// Get the absolute of a cgroup. The cgroup provided should not start with '/'.
std::string path(const std::string& cgroup);
+// Cgroup types.
+// "domain": Allows the cgroup to manage resources.
+//
+// "threaded": Allows threads to live in internal nodes but can only enable
+// the "cpu", "cpuset", "perf_event", and "pids" controllers.
+namespace type {
+
+// Domain cgroup.
+const std::string DOMAIN = "domain";
+
+// Threaded cgroup.
+const std::string THREADED = "threaded";
+
+// Domain cgroup that serves as the root of a threaded subtree.
+const std::string THREADED_DOMAIN = "domain threaded";
+
+// Cgroup in an invalid state. Can't be assigned processes or have controllers
+// enabled.
+const std::string INVALID = "domain invalid";
Review Comment:
Added for completeness; `cgroup.type` only has four possible values.
src: https://docs.kernel.org/admin-guide/cgroup-v2.html#core-interface-files
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] [cgroups2] Introduces API to make cgroups threaded and check their type. [mesos]
DevinLeamy opened a new pull request, #562: URL: https://github.com/apache/mesos/pull/562 The control 'cgroup.type' contains the type of a cgroup. Cgroups are either 'threaded' or 'domain'. Threaded cgroups can contain threads in internal cgroups but can only enable the "cpu", "cpuset", "perf_event", and "pids" controllers. Domain cgroups cannot contain internal processes but can enable any controller. In cgroups v2, container pids are to be placed exclusively in "leaf" cgroups in the cgroup hierarchy. Controllers should not be enabled in "leaf" cgroups and they should not have any child cgroups. To help enforce these constraints, we will make "leaf cgroups" threaded, hence this commit which extends our library to allow us to make a cgroup threaded. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduce API to set a soft memory limit. [mesos]
bmahler merged PR #559: URL: https://github.com/apache/mesos/pull/559 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduce API to set a soft memory limit. [mesos]
bmahler commented on code in PR #559: URL: https://github.com/apache/mesos/pull/559#discussion_r1567973794 ## src/linux/cgroups2.hpp: ## @@ -275,6 +275,24 @@ Try set_max(const std::string& cgroup, const Option& limit); // Cannot be used for the root cgroup. Result max(const std::string& cgroup); + +// Set the soft memory limit for a cgroup and its descendants. Exceeding the +// soft limit will cause processes in the cgroup to be throttled and put under +// heavy memory pressure. Review Comment: reclaim pressure -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduce API to set soft memory protection. [mesos]
bmahler merged PR #560: URL: https://github.com/apache/mesos/pull/560 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Removed trailing spaces. [mesos]
bmahler merged PR #561: URL: https://github.com/apache/mesos/pull/561 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] Removed trailing spaces. [mesos]
DevinLeamy opened a new pull request, #561: URL: https://github.com/apache/mesos/pull/561 (no comment) -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [cgroups2] Introduce API to set soft memory protection. [mesos]
DevinLeamy opened a new pull request, #560: URL: https://github.com/apache/mesos/pull/560 The 'memory.low' control is for soft memory protection. This only applies when the system is trying to reclaim memory. Soft memory protection means that the kernel will do its best to not reclaim memory from the cgroup if its usage is below the value in 'memory.low'. Before it reclaims any memory below the value in 'memory.low' it will first reclaim unprotected memory from other cgroups. We introduce `cgroups2::memory::low` and `cgroups2::memory::set_low` to set and get this soft memory protection limit. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduce API to set a soft memory limit. [mesos]
bmahler commented on code in PR #559: URL: https://github.com/apache/mesos/pull/559#discussion_r1567740967 ## src/linux/cgroups2.hpp: ## @@ -275,6 +275,24 @@ Try set_max(const std::string& cgroup, const Option& limit); // Cannot be used for the root cgroup. Result max(const std::string& cgroup); + +// Set the soft memory limit for a cgroup and its descendants. Exceeding the +// soft limit will cause processes in the cgroup to be throttled and put under +// heavy memory pressure. +// If limit is None, then there is no soft memory limit. +// Note: See the top-level `cgroups2::memory` comment about byte alignment. +// +// Cannot be used for the root cgroup. +Try set_soft_max( +const std::string& cgroup, const Option& limit); + + +// Get the soft memory limit for a cgroup and its descendants. +// If the returned limit is None, then there is no soft memory limit. +// +// Cannot be used for the root cgroup. +Result soft_max(const std::string& cgroup); Review Comment: let's just call this high and set_high to keep the cgroups terminology intact and avoid any confusion, we don't really want people to have to learn our own terminology here and how it maps to the cgroups terminology it seems to be called the "memory usage throttle limit" in cgroups v2 and fb's docs FWIW -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduce API to set a soft memory limit. [mesos]
bmahler commented on code in PR #559:
URL: https://github.com/apache/mesos/pull/559#discussion_r1567736625
##
src/linux/cgroups2.cpp:
##
@@ -795,6 +795,7 @@ Result parse_bytelimit(const string& value)
namespace control {
const string CURRENT = "memory.current";
+const string HIGH = "memory.high";
const string MAX = "memory.max";
const string MIN = "memory.min";
Review Comment:
we can add all the control functionality here, but it looks like we're only
going to use memory.low and memory.max:
https://facebookmicrosites.github.io/cgroup2/docs/memory-controller.html
How crun handles requests vs limits:
https://github.com/containers/crun/blob/68df0832427e692c705bd3485719174091b37cb5/crun.1.md#memory-controller
This is how mesos would do request vs limits as well for burstable
containers:
https://mesos.apache.org/documentation/latest/running-workloads/#resource-requests-and-limits
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Mitigate a case where the agent gets stuck sending TASK_DROPPED. [mesos]
bmahler merged PR #558: URL: https://github.com/apache/mesos/pull/558 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Mitigate a case where the agent gets stuck sending TASK_DROPPED. [mesos]
bmahler commented on code in PR #558: URL: https://github.com/apache/mesos/pull/558#discussion_r1567700962 ## src/master/master.cpp: ## @@ -7746,6 +7746,22 @@ void Master::updateSlave(UpdateSlaveMessage&& message) // providers as well. } + // We don't expect the agent's resource version to change, but above we + // do have a check to see if it's changed and therefore set `updated` + // to true, so we might as well assign the new value here rather than + // ignore it. + // + // Now that we have resource versions in the agent, the lack of the + // update to the version here was causing MESOS-7187 to be triggered + // when the master receives a re-registration message from the old run + // of an agent and then ignores the new re-registration message from a + // new run of the agent. When the new agent sends the update message, + // the master was seeing the difference resource uuid and setting Review Comment: "different" thanks! -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Mitigate a case where the agent gets stuck sending TASK_DROPPED. [mesos]
DevinLeamy commented on code in PR #558: URL: https://github.com/apache/mesos/pull/558#discussion_r1567443437 ## src/master/master.cpp: ## @@ -7746,6 +7746,22 @@ void Master::updateSlave(UpdateSlaveMessage&& message) // providers as well. } + // We don't expect the agent's resource version to change, but above we + // do have a check to see if it's changed and therefore set `updated` + // to true, so we might as well assign the new value here rather than + // ignore it. + // + // Now that we have resource versions in the agent, the lack of the + // update to the version here was causing MESOS-7187 to be triggered + // when the master receives a re-registration message from the old run + // of an agent and then ignores the new re-registration message from a + // new run of the agent. When the new agent sends the update message, + // the master was seeing the difference resource uuid and setting Review Comment: Is a "difference resource uuid" a thing, or is this a typo? -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduced an interface to set a hard memory limit. [mesos]
bmahler closed pull request #557: [cgroups2] Introduced an interface to set a hard memory limit. URL: https://github.com/apache/mesos/pull/557 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduced an interface to set a hard memory limit. [mesos]
bmahler commented on code in PR #557:
URL: https://github.com/apache/mesos/pull/557#discussion_r1566358512
##
src/tests/containerizer/cgroups2_tests.cpp:
##
@@ -372,6 +372,27 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_MemoryBytesRounding)
}
+TEST_F(Cgroups2Test, ROOT_CGROUPS2_MemoryMaximum)
+{
+ ASSERT_SOME(enable_controllers({"memory"}));
+
+ ASSERT_SOME(cgroups2::create(TEST_CGROUP));
+ ASSERT_SOME(cgroups2::controllers::enable(TEST_CGROUP, {"memory"}));
+
+ Bytes limit = Bytes(os::pagesize()) * 5;
+
+ // Does not exist for the root cgroup.
+ EXPECT_ERROR(cgroups2::memory::max(cgroups2::ROOT_CGROUP));
+ EXPECT_ERROR(cgroups2::memory::set_max(cgroups2::ROOT_CGROUP, limit));
+
+ EXPECT_SOME(cgroups2::memory::set_max(TEST_CGROUP, limit));
+ EXPECT_SOME_EQ(limit, cgroups2::memory::max(TEST_CGROUP));
+
+ EXPECT_SOME(cgroups2::memory::set_max(TEST_CGROUP, None()));
+ EXPECT_NONE(cgroups2::memory::max(TEST_CGROUP));
+}
Review Comment:
let's move this next to the minimum test
##
src/linux/cgroups2.cpp:
##
@@ -769,9 +769,33 @@ Try max(const string& cgroup)
namespace memory {
+namespace internal {
+
+// Parse a byte limit from a string.
+//
+// Format: "max" OR
+Result parse_bytelimit(const string& value)
+{
+ const string& trimmed = strings::trim(value);
+ if (trimmed == "max") {
+return None();
+ }
+
+ Try bytes = Bytes::parse(trimmed + "B");
+ if (bytes.isError()) {
+return Error("Invalid byte format '" + trimmed + "': " + bytes.error());
+ }
Review Comment:
let's use numify rather than the B string parsing hack here
##
src/linux/cgroups2.cpp:
##
@@ -769,9 +769,33 @@ Try max(const string& cgroup)
namespace memory {
+namespace internal {
+
+// Parse a byte limit from a string.
+//
+// Format: "max" OR
Review Comment:
u64
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Introduced an interface to set a hard memory limit. [mesos]
bmahler commented on code in PR #557:
URL: https://github.com/apache/mesos/pull/557#discussion_r1566293276
##
src/linux/cgroups2.hpp:
##
@@ -217,9 +217,49 @@ Try max(const std::string& cgroup);
namespace memory {
+// Memory usage limit.
+//
+// Represents a snapshot of "memory.high" and "memory.max".
+struct ByteLimit
+{
+ // Constructs a limitless byte limit.
+ ByteLimit() = default;
+
+ ByteLimit(const Bytes& limit);
+
+ bool operator==(const ByteLimit& other) const;
+
+ // Limit in bytes. None if the limit is "unlimited".
+ Option limit;
+};
Review Comment:
yeah, s/ByteLimit/Bytes/ in my snippet, whoops
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Introduced API to set the hard memory minimum for a cgroup. [mesos]
bmahler commented on code in PR #554:
URL: https://github.com/apache/mesos/pull/554#discussion_r1566284559
##
src/linux/cgroups2.cpp:
##
@@ -786,6 +787,23 @@ Try usage(const string& cgroup)
return Bytes(*contents);
}
+
+Try set_min(const string& cgroup, const Bytes& bytes)
+{
+ return cgroups2::write(cgroup, control::MIN, stringify(bytes.bytes()));
Review Comment:
we already have a write that takes uint64_t?
##
src/linux/cgroups2.cpp:
##
@@ -786,6 +787,23 @@ Try usage(const string& cgroup)
return Bytes(*contents);
}
+
+Try set_min(const string& cgroup, const Bytes& bytes)
+{
+ return cgroups2::write(cgroup, control::MIN, stringify(bytes.bytes()));
+}
+
+
+Try min(const string& cgroup)
+{
+ Try contents = cgroups2::read(cgroup, control::MIN);
+ if (contents.isError()) {
+return Error("Failed to read 'memory.min': " + contents.error());
+ }
+
+ return Bytes::parse(strings::trim(*contents) + "B");
Review Comment:
no need to parse with a B here, can just cgroups::read a uint64_t and
directly construct Bytes with it?
##
src/tests/containerizer/cgroups2_tests.cpp:
##
@@ -325,6 +326,52 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_MemoryUsage)
}
+TEST_F(Cgroups2Test, ROOT_CGROUPS2_MemoryMinimum)
+{
+ ASSERT_SOME(enable_controllers({"memory"}));
+
+ ASSERT_SOME(cgroups2::create(TEST_CGROUP));
+ ASSERT_SOME(cgroups2::controllers::enable(TEST_CGROUP, {"memory"}));
+
+ const Bytes bytes = Bytes(os::pagesize()) * 5;
+
+ // Does not exist for the root cgroup.
+ EXPECT_ERROR(cgroups2::memory::min(cgroups2::ROOT_CGROUP));
+ EXPECT_ERROR(cgroups2::memory::set_min(cgroups2::ROOT_CGROUP, bytes));
+
+ EXPECT_SOME(cgroups2::memory::set_min(TEST_CGROUP, bytes));
+ EXPECT_SOME_EQ(bytes, cgroups2::memory::min(TEST_CGROUP));
+}
+
+
+// Check that byte amounts written to the memory controller are rounded
+// down to the nearest page size.
+TEST_F(Cgroups2Test, ROOT_CGROUPS2_MemoryBytesRounding)
Review Comment:
is the plan to add other memory setters in here, hence not calling this a
Minimum test?
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Introduced API to set the hard memory minimum for a cgroup. [mesos]
bmahler closed pull request #554: [cgroups2] Introduced API to set the hard memory minimum for a cgroup. URL: https://github.com/apache/mesos/pull/554 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduced an interface to set a hard memory limit. [mesos]
DevinLeamy commented on code in PR #557:
URL: https://github.com/apache/mesos/pull/557#discussion_r1566279961
##
src/linux/cgroups2.hpp:
##
@@ -217,9 +217,49 @@ Try max(const std::string& cgroup);
namespace memory {
+// Memory usage limit.
+//
+// Represents a snapshot of "memory.high" and "memory.max".
+struct ByteLimit
+{
+ // Constructs a limitless byte limit.
+ ByteLimit() = default;
+
+ ByteLimit(const Bytes& limit);
+
+ bool operator==(const ByteLimit& other) const;
+
+ // Limit in bytes. None if the limit is "unlimited".
+ Option limit;
+};
Review Comment:
Similar to `cpu::BandwidthLimit`, a `ByteLimit` can either be some byte
amount or "max". The idea is to be more explicit about Option
representing an uncapped limit.
IIUC we would then have:
```c++
Try set_max(const std::string& cgroup, const Option& limit);
Result max(const std::string& cgroup); // instead of
Result?
```
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Introduced an interface to set a hard memory limit. [mesos]
bmahler commented on code in PR #557:
URL: https://github.com/apache/mesos/pull/557#discussion_r1566143430
##
src/linux/cgroups2.hpp:
##
@@ -217,9 +217,49 @@ Try max(const std::string& cgroup);
namespace memory {
+// Memory usage limit.
+//
+// Represents a snapshot of "memory.high" and "memory.max".
+struct ByteLimit
+{
+ // Constructs a limitless byte limit.
+ ByteLimit() = default;
+
+ ByteLimit(const Bytes& limit);
+
+ bool operator==(const ByteLimit& other) const;
+
+ // Limit in bytes. None if the limit is "unlimited".
+ Option limit;
+};
Review Comment:
what's up with this? is this only to avoid `Try>`? If so,
that's what `Result` provides:
```
Try set_max(const std::string& cgroup, const Option& limit);
Result max(const std::string& cgroup);
```
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Introduced API to set the hard memory minimum for a cgroup. [mesos]
bmahler commented on code in PR #554:
URL: https://github.com/apache/mesos/pull/554#discussion_r1566127015
##
src/tests/containerizer/cgroups2_tests.cpp:
##
@@ -52,6 +54,13 @@ namespace mesos {
namespace internal {
namespace tests {
+// System page size.
+//
+// Byte amounts written to the memory controller that are not aligned with the
+// system page size will be rounded up when read. Therefore, for simplicity,
+// we only test with multiples of the page size.
+const long PAGE_SIZE = sysconf(_SC_PAGE_SIZE);
Review Comment:
no need to store a constant for this, and let's use os/pagesize.hpp inline
for this:
```
Bytes(os::pagesize()) * 5
```
##
src/tests/containerizer/cgroups2_tests.cpp:
##
@@ -325,6 +334,24 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_MemoryUsage)
}
+TEST_F(Cgroups2Test, ROOT_CGROUPS2_MemoryMinimum)
+{
+ ASSERT_SOME(enable_controllers({"memory"}));
+
+ ASSERT_SOME(cgroups2::create(TEST_CGROUP));
+ ASSERT_SOME(cgroups2::controllers::enable(TEST_CGROUP, {"memory"}));
+
+ const Bytes bytes = Bytes(5 * PAGE_SIZE);
Review Comment:
see above comment (multiplication can be moved outside the bytes here
##
src/tests/containerizer/cgroups2_tests.cpp:
##
@@ -52,6 +54,13 @@ namespace mesos {
namespace internal {
namespace tests {
+// System page size.
+//
+// Byte amounts written to the memory controller that are not aligned with the
+// system page size will be rounded up when read. Therefore, for simplicity,
Review Comment:
might be good to have the test just show what happens directly here:
1. Test a value that is not page size aligned (gets rounded down despite the
docs saying rounded up)
2. Test a page size aligned value, comes back the same
##
src/linux/cgroups2.hpp:
##
@@ -220,6 +220,19 @@ namespace memory {
// Current memory usage of a cgroup and its descendants in bytes.
Try usage(const std::string& cgroup);
+
+// Set the minimum memory that is guaranteed to not be reclaimed under any
+// conditions. If the value is larger than the parent cgroup's value, the
+// parent's value is the effective value.
Review Comment:
you mentioned that this depends on a configuration or something? would be
good to spell out this subtlety
could also be more clear about what effective value means here (i.e.
effective seems to suggest that you can actually set it higher but the parent's
will effectively limit it before that, as opposed to the value you set getting
rounded down to the parent's value)
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] [cgroups2] Cgroups v2 isolator initialization and mount setup. [mesos]
DevinLeamy opened a new pull request, #556: URL: https://github.com/apache/mesos/pull/556 Updates the cgroups v2 isolator to include initialization, cleanup, update, and recovery logic. Unlike cgroups v1 we: - Create a new cgroup namespace during isolation, by introducing a new clone namespace flag. This implies that the contained process will only have access to cgroups in its cgroup subtree - We only need to recover two cgroups (the domain and leaf cgroups [1]) for each container, rather than having to recover one cgroup for each controller the container used. - We do not (yet) support nested containers. Using the cgroups v2 isolator requires Mesos to be compiled with `--enable-cgroups-v2` and to have the cgroup2 filesystem mounted at /sys/fs/cgroup. Selecting the correct isolator version (v1 or v2) is done automatically. V2 is used if the host supports cgroups v2 and is correctly configured. [1] The "domain" cgroup is the cgroup for a container where resource constraints are imposed. The "leaf" cgroup, which has the path /leaf, is where the container PID is put. Container PIDs are only put in leaf cgroups. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Cgroups v2 isolator initialization and mount setup. [mesos]
DevinLeamy closed pull request #555: [cgroups2] Cgroups v2 isolator initialization and mount setup. URL: https://github.com/apache/mesos/pull/555 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [cgroups2] Cgroups v2 isolator initialization and mount setup. [mesos]
DevinLeamy opened a new pull request, #555: URL: https://github.com/apache/mesos/pull/555 Updates the cgroups v2 isolator to include initialization, cleanup, update, and recovery logic. Unlike cgroups v1 we: - Create a new cgroup namespace during isolation, by introducing a new clone namespace flag. This implies that the contained process will only have access to cgroups in its cgroup subtree - We only need to recover two cgroups (the domain and leaf cgroups [1]) for each container, rather than having to recover one cgroup for each controller the container used. - We do not (yet) support nested containers. Using the cgroups v2 isolator requires Mesos to be compiled with `--enable-cgroups-v2` and to have the cgroup2 filesystem mounted at /sys/fs/cgroup. Selecting the correct isolator version (v1 or v2) is done automatically. V2 is used if the host supports cgroups v2 and is correctly configured. [1] The "domain" cgroup is the cgroup for a container where resource constraints are imposed. The "leaf" cgroup, which has the path /leaf, is where the container PID is put. Container PIDs are only put in leaf cgroups. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduced API to set the hard memory minimum for a cgroup. [mesos]
DevinLeamy commented on code in PR #554:
URL: https://github.com/apache/mesos/pull/554#discussion_r1565850696
##
src/tests/containerizer/cgroups2_tests.cpp:
##
@@ -52,6 +54,13 @@ namespace mesos {
namespace internal {
namespace tests {
+// System page size.
+//
+// Byte amounts written to the memory controller that are not aligned with the
+// system page size will be rounded up when read. Therefore, for simplicity,
+// we only test with multiples of the page size.
Review Comment:
src:
https://docs.kernel.org/admin-guide/cgroup-v2.html#memory-interface-files
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] [cgroups2] Introduced API to set the hard memory minimum for a cgroup. [mesos]
DevinLeamy opened a new pull request, #554: URL: https://github.com/apache/mesos/pull/554 Introduces ``` cgroups2::memory::min(cgroup)// get the minimum cgroups2::memory::set_min(cgroup, bytes) // set the minimum ``` to get and set the minimum memory in bytes that are guaranteed to not be reclaimed by the kernel under any conditions. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduce `memory` controller. [mesos]
bmahler commented on code in PR #553:
URL: https://github.com/apache/mesos/pull/553#discussion_r1561754342
##
src/linux/cgroups2.cpp:
##
@@ -767,6 +767,27 @@ Try max(const string& cgroup)
} // namespace cpu {
+namespace memory {
+
+namespace control {
+
+const string CURRENT = "memory.current";
+
+} // namespace control {
+
+Try usage(const string& cgroup)
+{
+ Try contents = cgroups2::read(
+ cgroup, memory::control::CURRENT);
+ if (contents.isError()) {
+return Error("Failed to read 'memory.current': " + contents.error());
+ }
+
+ return Bytes::parse(strings::trim(*contents) + "B");
Review Comment:
we can just read here and avoid the need for trimming / parsing
below, by directly constructing Bytes(uint64_t)
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Introduce `memory` controller. [mesos]
bmahler closed pull request #553: [cgroups2] Introduce `memory` controller. URL: https://github.com/apache/mesos/pull/553 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Update the LinuxLauncher to support cgroups v2. [mesos]
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 freezerHierarchy;
+
+ // Absolute path the systemd hierarchy.
+ // Only present in cgroups v1 when systemd is enabled
+ // (i.e. `systemd::enabled()`).
+ Option 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 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(),
-::_destroy,
-containerId));
+.then(defer(self(), ::_destroyCgroups, container));
}
-Future LinuxLauncherProcess::_destroy(const ContainerID& containerId)
+Future 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 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 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>
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 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
Re: [PR] [cgroups2] Update the LinuxLauncher to support cgroups v2. [mesos]
bmahler closed pull request #552: [cgroups2] Update the LinuxLauncher to support cgroups v2. URL: https://github.com/apache/mesos/pull/552 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Update the LinuxLauncher to support cgroups v2. [mesos]
DevinLeamy commented on code in PR #552:
URL: https://github.com/apache/mesos/pull/552#discussion_r1559901682
##
src/slave/containerizer/mesos/linux_launcher.cpp:
##
@@ -251,48 +303,104 @@ Future LinuxLauncher::status(
}
-// `_systemdHierarchy` is only set if running on a systemd environment.
LinuxLauncherProcess::LinuxLauncherProcess(
-const Flags& _flags,
-const string& _freezerHierarchy,
-const Option& _systemdHierarchy)
- : flags(_flags),
-freezerHierarchy(_freezerHierarchy),
-systemdHierarchy(_systemdHierarchy) {}
+ const Flags& _flags, const CgroupsInfo& _cgroupsInfo)
+ : flags(_flags), cgroupsInfo(_cgroupsInfo) {}
Future> LinuxLauncherProcess::recover(
const vector& 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 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:
You're right. I've updated the order to recover before looping here :+1:
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Update the LinuxLauncher to support cgroups v2. [mesos]
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 freezerHierarchy;
+
+ Option 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 LinuxLauncher::status(
}
-// `_systemdHierarchy` is only set if running on a systemd environment.
LinuxLauncherProcess::LinuxLauncherProcess(
-const Flags& _flags,
-const string& _freezerHierarchy,
-const Option& _systemdHierarchy)
- : flags(_flags),
-freezerHierarchy(_freezerHierarchy),
-systemdHierarchy(_systemdHierarchy) {}
+ const Flags& _flags, const CgroupsInfo& _cgroupsInfo)
+ : flags(_flags), cgroupsInfo(_cgroupsInfo) {}
Future> LinuxLauncherProcess::recover(
const vector& 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 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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Update the LinuxLauncher to support cgroups v2. [mesos]
DevinLeamy commented on code in PR #552:
URL: https://github.com/apache/mesos/pull/552#discussion_r1557888760
##
src/slave/containerizer/mesos/linux_launcher.hpp:
##
@@ -25,6 +25,17 @@ namespace mesos {
namespace internal {
namespace slave {
+// DO(dleamy): Move this into the header file.
+struct CgroupsInfo
+{
+ // Flag indicating whether cgroups v2 is used.
+ bool cgroupsV2;
+
+ Option freezerHierarchy;
+
+ Option systemdHierarchy;
+};
+
Review Comment:
Would like to move this into the source file and only have a forward
declaration in the header. Doing that was giving compilation errors. Perhaps
you have some insight here? @bmahler
--
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: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] [cgroups2] Introduce a `LinuxLauncherCgroups2` which uses cgroups v2. [mesos]
DevinLeamy opened a new pull request, #552: URL: https://github.com/apache/mesos/pull/552 Introduces a new Linux launcher that uses cgroups v2. Like with other cgroup v2 functionality, the new launcher is used by default if the host is correctly configured for cgroups v2 and Mesos has been compiled with the `--enable-cgroups-v2` flag. -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduce utility to parse a container id from a cgroup path. [mesos]
bmahler closed pull request #551: [cgroups2] Introduce utility to parse a container id from a cgroup path. URL: https://github.com/apache/mesos/pull/551 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduce utility to parse a container id from a cgroup path. [mesos]
bmahler commented on code in PR #551: URL: https://github.com/apache/mesos/pull/551#discussion_r1554329348 ## src/slave/containerizer/mesos/paths.hpp: ## @@ -320,6 +320,17 @@ std::string container( const ContainerID& containerId, bool leaf = false); + +// Get a containerId from a container's cgroup. +// Inverse of `cgroups2::container(root, id)`. +// +// The provided cgroup can be an absolute path or a relative path. +// Leaf paths (which end in `/leaf`) and non-leaf paths will resolve to the +// same container id. Review Comment: to be consistent with our other "cgroup" arguments and variables in the cgroups2 library, let's only support the relative path format here -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Introduced API to kill the processes inside of cgroup subtree. [mesos]
bmahler closed pull request #550: [cgroups2] Introduced API to kill the processes inside of cgroup subtree. URL: https://github.com/apache/mesos/pull/550 -- 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: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
