bmahler commented on code in PR #541:
URL: https://github.com/apache/mesos/pull/541#discussion_r1546668109
##########
src/linux/cgroups2.cpp:
##########
@@ -536,6 +576,59 @@ Try<Stats> parse(const string& content)
} // namespace stat {
+namespace max {
+
+// Reads from 'cpu.max' and returns the state as a `BandwidthLimit`.
+//
+// Format
+// -----------------------------
+// $MAX $PERIOD
+// -----------------------------
+// $MAX Maximum CPU time, in microseconds, processes in the cgroup can
+// collectively use during one $PERIOD. If set to “max” then there
+// is no limit.
+//
+// $PERIOD Length of one period, in microseconds.
+Try<BandwidthLimit> read(const string& cgroup)
+{
+ Try<string> content = cgroups2::read<string>(cgroup, cpu::control::MAX);
+ if (content.isError()) {
+ return Error(
+ "Failed the read 'cpu.max' for cgroup '" + cgroup + "': "
+ + content.error());
+ }
+
+ Try<BandwidthLimit> limit = BandwidthLimit::parse(*content);
+ if (limit.isError()) {
+ return Error(
+ "Failed to parse '" + *content + "' as a bandwidth limit: "
+ + limit.error());
+ }
+
+ return *limit;
+}
+
+
+Try<Nothing> write(const string& cgroup, const cpu::BandwidthLimit& limit)
+{
+ if (limit.limit.isNone()) {
+ return cgroups2::write(cgroup, cpu::control::MAX, "max");
+ }
+
+ if (limit.limit.isSome() && limit.period.isNone()) {
+ return Error(
+ "Invalid bandwidth limit: `period` can only be `None` if it's a"
+ " limitless bandwidth limit");
+ }
+
+ return cgroups2::write(
+ cgroup,
+ cpu::control::MAX,
+ to_string(limit.limit->us()) + " " + to_string(limit.period->us()));
Review Comment:
this is going to print a floating point number since us() is returning a
double, so we probably want to cast to unsigned integer here to never print
floating point, and we might want to validate this as well before we write
(e.g. not fractional microseconds, non-negative durations)
##########
src/linux/cgroups2.cpp:
##########
@@ -536,6 +576,59 @@ Try<Stats> parse(const string& content)
} // namespace stat {
+namespace max {
+
+// Reads from 'cpu.max' and returns the state as a `BandwidthLimit`.
+//
+// Format
+// -----------------------------
+// $MAX $PERIOD
+// -----------------------------
+// $MAX Maximum CPU time, in microseconds, processes in the cgroup can
+// collectively use during one $PERIOD. If set to “max” then there
+// is no limit.
+//
+// $PERIOD Length of one period, in microseconds.
+Try<BandwidthLimit> read(const string& cgroup)
+{
+ Try<string> content = cgroups2::read<string>(cgroup, cpu::control::MAX);
+ if (content.isError()) {
+ return Error(
+ "Failed the read 'cpu.max' for cgroup '" + cgroup + "': "
+ + content.error());
+ }
+
+ Try<BandwidthLimit> limit = BandwidthLimit::parse(*content);
+ if (limit.isError()) {
+ return Error(
+ "Failed to parse '" + *content + "' as a bandwidth limit: "
+ + limit.error());
+ }
+
+ return *limit;
+}
+
+
+Try<Nothing> write(const string& cgroup, const cpu::BandwidthLimit& limit)
+{
+ if (limit.limit.isNone()) {
+ return cgroups2::write(cgroup, cpu::control::MAX, "max");
+ }
+
+ if (limit.limit.isSome() && limit.period.isNone()) {
+ return Error(
+ "Invalid bandwidth limit: `period` can only be `None` if it's a"
Review Comment:
we don't use backticks in error log messages, you can use a single quote but
here I don't think we need any quoting
##########
src/linux/cgroups2.cpp:
##########
@@ -571,6 +664,26 @@ Try<cpu::Stats> stats(const string& cgroup)
return cpu::control::stat::parse(*content);
}
+
+Try<Nothing> bandwidth(const string& cgroup, const cpu::BandwidthLimit& limit)
+{
+ if (cgroup == ROOT_CGROUP) {
+ return Error("Operation not supported for the root cgroup");
+ }
+
+ return cpu::control::max::write(cgroup, limit);
Review Comment:
we don't have these extra layers of functions for others like weight, so
let's avoid them here too for consistency
##########
src/linux/cgroups2.hpp:
##########
@@ -168,9 +168,45 @@ struct Stats
};
+// Specifies the maximum CPU bandwidth available over a given period.
+// Represents a snapshot of the 'cpu.max' control file.
+struct BandwidthLimit
+{
+ BandwidthLimit() = default;
+
+ // Create a bandwidth limit of `limit` every time `period`.
+ BandwidthLimit(Duration limit, Duration period);
+
+ // Create a limitless bandwidth limit.
+ static BandwidthLimit limitless();
+
+ // Parse a string with the format of the 'cpu.max' control file as
+ // a `BandwidthLimit`.
+ static Try<BandwidthLimit> parse(const std::string& content);
+
+ // Maximum CPU time quota (AKA bandwidth) per quota. If set to Option::none()
+ // then there is no quota limit.
+ Option<Duration> limit;
+
+ // Duration of the period where the bandwidth can be used. Can only be
+ // `None()` if `limit` is set `None()`, implying that there is no
+ // bandwidth limit.
+ Option<Duration> period;
+};
+
// Get the CPU usage statistics for a cgroup.
Try<Stats> stats(const std::string& cgroup);
+
+// Set the bandwidth limit for a cgroup.
+// Cannot be used for the root cgroup.
+Try<Nothing> bandwidth(const std::string& cgroup, const BandwidthLimit& limit);
+
+
+// Determine the bandwidth limit for a cgroup.
+// Cannot be used for the root cgroup.
+Try<BandwidthLimit> bandwidth(const std::string& cgroup);
Review Comment:
max and set_max would be more clear here since that matches the control
files that a user is used to, and max is a pretty clear name already vs
bandwidth
##########
src/linux/cgroups2.cpp:
##########
@@ -536,6 +576,59 @@ Try<Stats> parse(const string& content)
} // namespace stat {
+namespace max {
+
+// Reads from 'cpu.max' and returns the state as a `BandwidthLimit`.
+//
+// Format
+// -----------------------------
+// $MAX $PERIOD
+// -----------------------------
+// $MAX Maximum CPU time, in microseconds, processes in the cgroup can
+// collectively use during one $PERIOD. If set to “max” then there
Review Comment:
note that you used non-ascii open and close quotes here
##########
src/linux/cgroups2.cpp:
##########
@@ -482,6 +483,45 @@ Try<set<string>> enabled(const string& cgroup)
namespace cpu {
+BandwidthLimit::BandwidthLimit(Duration _limit, Duration _period)
+ : limit{_limit},
+ period{_period} {}
+
+
+BandwidthLimit BandwidthLimit::limitless()
+{
+ BandwidthLimit limit;
+ limit.limit = Option<Duration>::none();
Review Comment:
In general you want to use None() as syntax sugar
##########
src/linux/cgroups2.cpp:
##########
@@ -482,6 +483,45 @@ Try<set<string>> enabled(const string& cgroup)
namespace cpu {
+BandwidthLimit::BandwidthLimit(Duration _limit, Duration _period)
+ : limit{_limit},
+ period{_period} {}
+
+
+BandwidthLimit BandwidthLimit::limitless()
+{
+ BandwidthLimit limit;
+ limit.limit = Option<Duration>::none();
+ limit.period = Option<Duration>::none();
+ return limit;
+}
+
+
+Try<BandwidthLimit> BandwidthLimit::parse(const string& content)
+{
+ vector<string> split = strings::split(content, " ", 2);
+ if (split.size() != 2) {
+ return Error("Expected format '$MAX $PERIOD' but received: " + content);
+ }
+
+ string _bandwidth = split[0], _period = split[1];
Review Comment:
to avoid confusion since we call this limit in the struct, let's call it
limit here too
##########
src/linux/cgroups2.cpp:
##########
@@ -536,6 +576,59 @@ Try<Stats> parse(const string& content)
} // namespace stat {
+namespace max {
+
+// Reads from 'cpu.max' and returns the state as a `BandwidthLimit`.
+//
+// Format
+// -----------------------------
+// $MAX $PERIOD
+// -----------------------------
+// $MAX Maximum CPU time, in microseconds, processes in the cgroup can
+// collectively use during one $PERIOD. If set to “max” then there
+// is no limit.
+//
+// $PERIOD Length of one period, in microseconds.
+Try<BandwidthLimit> read(const string& cgroup)
+{
+ Try<string> content = cgroups2::read<string>(cgroup, cpu::control::MAX);
+ if (content.isError()) {
+ return Error(
+ "Failed the read 'cpu.max' for cgroup '" + cgroup + "': "
+ + content.error());
+ }
+
+ Try<BandwidthLimit> limit = BandwidthLimit::parse(*content);
+ if (limit.isError()) {
+ return Error(
+ "Failed to parse '" + *content + "' as a bandwidth limit: "
+ + limit.error());
+ }
+
+ return *limit;
+}
+
+
+Try<Nothing> write(const string& cgroup, const cpu::BandwidthLimit& limit)
+{
+ if (limit.limit.isNone()) {
+ return cgroups2::write(cgroup, cpu::control::MAX, "max");
+ }
+
+ if (limit.limit.isSome() && limit.period.isNone()) {
Review Comment:
we already know that limit is some here..?
##########
src/linux/cgroups2.hpp:
##########
@@ -168,9 +168,45 @@ struct Stats
};
+// Specifies the maximum CPU bandwidth available over a given period.
+// Represents a snapshot of the 'cpu.max' control file.
+struct BandwidthLimit
+{
+ BandwidthLimit() = default;
+
+ // Create a bandwidth limit of `limit` every time `period`.
+ BandwidthLimit(Duration limit, Duration period);
+
+ // Create a limitless bandwidth limit.
+ static BandwidthLimit limitless();
+
+ // Parse a string with the format of the 'cpu.max' control file as
+ // a `BandwidthLimit`.
+ static Try<BandwidthLimit> parse(const std::string& content);
+
+ // Maximum CPU time quota (AKA bandwidth) per quota. If set to Option::none()
Review Comment:
per period
##########
src/linux/cgroups2.hpp:
##########
@@ -168,9 +168,45 @@ struct Stats
};
+// Specifies the maximum CPU bandwidth available over a given period.
+// Represents a snapshot of the 'cpu.max' control file.
+struct BandwidthLimit
+{
+ BandwidthLimit() = default;
Review Comment:
this is also a limitless constructor, so we have two ways to make a
limitless object?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
