This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
The following commit(s) were added to refs/heads/master by this push:
new 9968628 Properly handled disk resources in operator API `CREATE`
handler.
9968628 is described below
commit 996862828ca9b7675e40b495fe24d95615bb832b
Author: Benjamin Bannier <[email protected]>
AuthorDate: Thu Mar 7 14:53:34 2019 -0800
Properly handled disk resources in operator API `CREATE` handler.
This code was previously assuming that any `CREATE` operation would add
a `DiskInfo` to resources (to hold the `PersistenceInfo`). To compute
the resources required to perform the operation it then just removed the
full `DiskInfo`. This is incorrect as e.g., CSI disk resources carry
information unrelated to persistence in their `DiskInfo`.
This patch fixes `CREATE` handling by leveraging resource conversions
which were introduced in the meantime. This allows extracting the
required resource in a lower scope which allows us refactor handlers for
other operator API calls. With that less information about the layout of
operations is needed here.
Review: https://reviews.apache.org/r/70154/
---
src/master/http.cpp | 55 +++++++++++++++------------------------------------
src/master/master.hpp | 8 ++------
2 files changed, 18 insertions(+), 45 deletions(-)
diff --git a/src/master/http.cpp b/src/master/http.cpp
index 012ee4f..d6d4740 100644
--- a/src/master/http.cpp
+++ b/src/master/http.cpp
@@ -723,19 +723,6 @@ Future<Response> Master::Http::scheduler(
}
-static Resources removeDiskInfos(const Resources& resources)
-{
- Resources result;
-
- foreach (Resource resource, resources) {
- resource.clear_disk();
- result += resource;
- }
-
- return result;
-}
-
-
string Master::Http::CREATE_VOLUMES_HELP()
{
return HELP(
@@ -877,11 +864,7 @@ Future<Response> Master::Http::_createVolumes(
return Forbidden();
}
- // The resources required for this operation are equivalent to the
- // volumes specified by the user minus any DiskInfo (DiskInfo will
- // be created when this operation is applied).
- return _operation(
- slaveId, removeDiskInfos(operation.create().volumes()), operation);
+ return _operation(slaveId, operation);
}));
}
@@ -1049,7 +1032,7 @@ Future<Response> Master::Http::_destroyVolumes(
return Forbidden();
}
- return _operation(slaveId, operation.destroy().volumes(), operation);
+ return _operation(slaveId, operation);
}));
}
@@ -1136,13 +1119,7 @@ Future<Response> Master::Http::growVolume(
return Forbidden();
}
- // The `volume` and `addition` fields contain the resources required for
- // this operation.
- return _operation(
- slaveId,
- Resources(operation.grow_volume().volume()) +
- Resources(operation.grow_volume().addition()),
- operation);
+ return _operation(slaveId, operation);
}));
}
@@ -1205,9 +1182,7 @@ Future<Response> Master::Http::shrinkVolume(
return Forbidden();
}
- // The `volume` field contains the resources required for this operation.
- return _operation(
- slaveId, operation.shrink_volume().volume(), operation);
+ return _operation(slaveId, operation);
}));
}
@@ -2000,12 +1975,7 @@ Future<Response> Master::Http::_reserve(
return Forbidden();
}
- // We only allow "pushing" a single reservation at a time, so we require
- // the resources with one reservation "popped" to be present on the
agent.
- Resources required =
- Resources(operation.reserve().resources()).popReservation();
-
- return _operation(slaveId, required, operation);
+ return _operation(slaveId, operation);
}));
}
@@ -4009,16 +3979,23 @@ Future<Response> Master::Http::_unreserve(
return Forbidden();
}
- return _operation(slaveId, operation.unreserve().resources(), operation);
+ return _operation(slaveId, operation);
}));
}
Future<Response> Master::Http::_operation(
const SlaveID& slaveId,
- Resources required,
const Offer::Operation& operation) const
{
+ Try<Resources> required = protobuf::getConsumedResources(operation);
+
+ if (required.isError()) {
+ return BadRequest(
+ "Invalid " + stringify(operation.type()) + " operation: " +
+ required.error());
+ }
+
Slave* slave = master->slaves.registered.get(slaveId);
if (slave == nullptr) {
return BadRequest("No agent found with specified ID");
@@ -4039,12 +4016,12 @@ Future<Response> Master::Http::_operation(
Resources recovered = offer->resources();
recovered.unallocate();
- if (required == required - recovered) {
+ if (required.get() == required.get() - recovered) {
continue;
}
totalRecovered += recovered;
- required -= recovered;
+ required.get() -= recovered;
// We explicitly pass 'Filters()' which has a default 'refuse_seconds'
// of 5 seconds rather than 'None()' here, so that we can virtually
diff --git a/src/master/master.hpp b/src/master/master.hpp
index 90e0814..953cc5b 100644
--- a/src/master/master.hpp
+++ b/src/master/master.hpp
@@ -1645,17 +1645,13 @@ private:
*
* @param slaveId The ID of the slave that the operation is
* updating.
- * @param required The resources needed to satisfy the operation.
- * This is used for an optimization where we try to only
- * rescind offers that would contribute to satisfying the
- * operation.
* @param operation The operation to be performed.
*
- * @return Returns 'OK' if successful, 'Conflict' otherwise.
+ * @return Returns 'OK' if successful, 'BadRequest' if the
+ * operation is malformed, 'Conflict' otherwise.
*/
process::Future<process::http::Response> _operation(
const SlaveID& slaveId,
- Resources required,
const Offer::Operation& operation) const;
// Master API handlers.
