You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ch...@apache.org on 2018/03/29 05:07:37 UTC
[2/4] mesos git commit: Implemented idempotency for agent resource
provider config API calls.
Implemented idempotency for agent resource provider config API calls.
`ADD_RESOURCE_PROVIDER_CONFIG`, `UPDATE_RESOURCE_PROVIDER_CONFIG` and
`REMOVE_RESOURCE_PROVIDER_CONFIG` now return 200 for identical
repetivite calls.
Review: https://reviews.apache.org/r/66325
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/667d0431
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/667d0431
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/667d0431
Branch: refs/heads/master
Commit: 667d04319aa85674308aef1b8932995b741c5d2b
Parents: d9b5cf4
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
Authored: Tue Mar 27 19:58:49 2018 -0700
Committer: Chun-Hung Hsiao <ch...@mesosphere.io>
Committed: Wed Mar 28 19:08:18 2018 -0700
----------------------------------------------------------------------
src/resource_provider/daemon.cpp | 37 +++++++++++++++++--------
src/resource_provider/daemon.hpp | 2 +-
src/resource_provider/storage/provider.cpp | 4 +++
src/slave/http.cpp | 31 ++++++++++-----------
4 files changed, 45 insertions(+), 29 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/667d0431/src/resource_provider/daemon.cpp
----------------------------------------------------------------------
diff --git a/src/resource_provider/daemon.cpp b/src/resource_provider/daemon.cpp
index d0a8186..0a76cc6 100644
--- a/src/resource_provider/daemon.cpp
+++ b/src/resource_provider/daemon.cpp
@@ -96,7 +96,7 @@ public:
Future<bool> add(const ResourceProviderInfo& info);
Future<bool> update(const ResourceProviderInfo& info);
- Future<bool> remove(const string& type, const string& name);
+ Future<Nothing> remove(const string& type, const string& name);
protected:
void initialize() override;
@@ -178,15 +178,18 @@ void LocalResourceProviderDaemonProcess::start(const SlaveID& _slaveId)
Future<bool> LocalResourceProviderDaemonProcess::add(
const ResourceProviderInfo& info)
{
+ CHECK(!info.has_id()); // Should have already been validated by the agent.
+
if (configDir.isNone()) {
- return Failure("`--resource_provider_config_dir` must be specified");
+ return Failure("Missing required flag --resource_provider_config_dir");
}
+ // Return true if the info has been added for idempotency.
if (providers[info.type()].contains(info.name())) {
- return false;
+ return providers[info.type()].at(info.name()).info == info;
}
- // Generate a filename for the the config.
+ // Generate a filename for the config.
// NOTE: We use the following template `<type>.<name>.<uuid>.json`
// with a random UUID to generate a new filename to avoid any conflict
// with existing ad-hoc config files.
@@ -224,8 +227,10 @@ Future<bool> LocalResourceProviderDaemonProcess::add(
Future<bool> LocalResourceProviderDaemonProcess::update(
const ResourceProviderInfo& info)
{
+ CHECK(!info.has_id()); // Should have already been validated by the agent.
+
if (configDir.isNone()) {
- return Failure("`--resource_provider_config_dir` must be specified");
+ return Failure("Missing required flag --resource_provider_config_dir");
}
if (!providers[info.type()].contains(info.name())) {
@@ -234,6 +239,11 @@ Future<bool> LocalResourceProviderDaemonProcess::update(
ProviderData& data = providers[info.type()].at(info.name());
+ // Return true if the info has been updated for idempotency.
+ if (data.info == info) {
+ return true;
+ }
+
Try<Nothing> _save = save(data.path, info);
if (_save.isError()) {
return Failure(
@@ -262,16 +272,17 @@ Future<bool> LocalResourceProviderDaemonProcess::update(
}
-Future<bool> LocalResourceProviderDaemonProcess::remove(
+Future<Nothing> LocalResourceProviderDaemonProcess::remove(
const string& type,
const string& name)
{
if (configDir.isNone()) {
- return Failure("`--resource_provider_config_dir` must be specified");
+ return Failure("Missing required flag --resource_provider_config_dir");
}
+ // Do nothing if the info has been removed for idempotency.
if (!providers[type].contains(name)) {
- return false;
+ return Nothing();
}
const string path = providers[type].at(name).path;
@@ -286,7 +297,7 @@ Future<bool> LocalResourceProviderDaemonProcess::remove(
// provider to be destructed.
providers[type].erase(name);
- return true;
+ return Nothing();
}
@@ -340,6 +351,10 @@ Try<Nothing> LocalResourceProviderDaemonProcess::load(const string& path)
return Error("Not a valid resource provider config: " + info.error());
}
+ if (info->has_id()) {
+ return Error("'ResourceProviderInfo.id' must not be set");
+ }
+
// Ensure that ('type', 'name') pair is unique.
if (providers[info->type()].contains(info->name())) {
return Error(
@@ -362,7 +377,7 @@ Try<Nothing> LocalResourceProviderDaemonProcess::save(
{
CHECK_SOME(configDir);
- // NOTE: We create the staging direcotry in the resource provider
+ // NOTE: We create the staging directory in the resource provider
// config directory to make sure that the renaming below does not
// cross devices (MESOS-2319).
// TODO(chhsiao): Consider adding a way to garbage collect the staging
@@ -560,7 +575,7 @@ Future<bool> LocalResourceProviderDaemon::update(
}
-Future<bool> LocalResourceProviderDaemon::remove(
+Future<Nothing> LocalResourceProviderDaemon::remove(
const string& type,
const string& name)
{
http://git-wip-us.apache.org/repos/asf/mesos/blob/667d0431/src/resource_provider/daemon.hpp
----------------------------------------------------------------------
diff --git a/src/resource_provider/daemon.hpp b/src/resource_provider/daemon.hpp
index a6d0013..f2017f4 100644
--- a/src/resource_provider/daemon.hpp
+++ b/src/resource_provider/daemon.hpp
@@ -61,7 +61,7 @@ public:
process::Future<bool> add(const ResourceProviderInfo& info);
process::Future<bool> update(const ResourceProviderInfo& info);
- process::Future<bool> remove(
+ process::Future<Nothing> remove(
const std::string& type,
const std::string& name);
http://git-wip-us.apache.org/repos/asf/mesos/blob/667d0431/src/resource_provider/storage/provider.cpp
----------------------------------------------------------------------
diff --git a/src/resource_provider/storage/provider.cpp b/src/resource_provider/storage/provider.cpp
index 8db1ce0..a07620d 100644
--- a/src/resource_provider/storage/provider.cpp
+++ b/src/resource_provider/storage/provider.cpp
@@ -3189,6 +3189,10 @@ Try<Owned<LocalResourceProvider>> StorageLocalResourceProvider::create(
const Option<string>& authToken,
bool strict)
{
+ if (info.has_id()) {
+ return Error("'ResourceProviderInfo.id' must not be set");
+ }
+
// Verify that the name follows Java package naming convention.
// TODO(chhsiao): We should move this check to a validation function
// for `ResourceProviderInfo`.
http://git-wip-us.apache.org/repos/asf/mesos/blob/667d0431/src/slave/http.cpp
----------------------------------------------------------------------
diff --git a/src/slave/http.cpp b/src/slave/http.cpp
index 65081c9..d500fde 100644
--- a/src/slave/http.cpp
+++ b/src/slave/http.cpp
@@ -3176,12 +3176,12 @@ Future<Response> Http::addResourceProviderConfig(
return slave->localResourceProviderDaemon->add(info)
.then([](bool added) -> Response {
- if (!added) {
- return Conflict();
- }
+ if (!added) {
+ return Conflict();
+ }
- return OK();
- })
+ return OK();
+ })
.repair([info](const Future<Response>& future) {
LOG(ERROR)
<< "Failed to add resource provider config with type '"
@@ -3221,12 +3221,12 @@ Future<Response> Http::updateResourceProviderConfig(
return slave->localResourceProviderDaemon->update(info)
.then([](bool updated) -> Response {
- if (!updated) {
- return NotFound();
- }
+ if (!updated) {
+ return NotFound();
+ }
- return OK();
- })
+ return OK();
+ })
.repair([info](const Future<Response>& future) {
LOG(ERROR)
<< "Failed to update resource provider config with type '"
@@ -3265,13 +3265,10 @@ Future<Response> Http::removeResourceProviderConfig(
<< type << "' and name '" << name << "'";
return slave->localResourceProviderDaemon->remove(type, name)
- .then([](bool removed) -> Response {
- if (!removed) {
- return NotFound();
- }
-
- return OK();
- })
+ .then([]() -> Response {
+ // Config removal is always successful due to idempotency.
+ return OK();
+ })
.repair([type, name](const Future<Response>& future) {
LOG(ERROR)
<< "Failed to remove resource provider config with type '"