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/c423e2ce Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/c423e2ce Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/c423e2ce Branch: refs/heads/1.5.x Commit: c423e2cee766b01d24deece79e366e80e3d141df Parents: aa57ae8 Author: Chun-Hung Hsiao <chhs...@mesosphere.io> Authored: Tue Mar 27 19:58:49 2018 -0700 Committer: Chun-Hung Hsiao <chhs...@mesosphere.io> Committed: Wed Mar 28 19:40:15 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 | 7 ++--- 4 files changed, 33 insertions(+), 17 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/c423e2ce/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/c423e2ce/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/c423e2ce/src/resource_provider/storage/provider.cpp ---------------------------------------------------------------------- diff --git a/src/resource_provider/storage/provider.cpp b/src/resource_provider/storage/provider.cpp index f0f4f80..aeb6d6f 100644 --- a/src/resource_provider/storage/provider.cpp +++ b/src/resource_provider/storage/provider.cpp @@ -3134,6 +3134,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/c423e2ce/src/slave/http.cpp ---------------------------------------------------------------------- diff --git a/src/slave/http.cpp b/src/slave/http.cpp index a72050d..3abcc1e 100644 --- a/src/slave/http.cpp +++ b/src/slave/http.cpp @@ -3447,11 +3447,8 @@ Future<Response> Http::removeResourceProviderConfig( << "' and name '" << name << "'"; return slave->localResourceProviderDaemon->remove(type, name) - .then([](bool removed) -> Response { - if (!removed) { - return NotFound(); - } - + .then([]() -> Response { + // Config removal is always successful due to idempotency. return OK(); }) .repair([type, name](const Future<Response>& future) {