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
commit 1adceaacc81bb33332a52a9f28a864ac928ac706 Author: Chun-Hung Hsiao <[email protected]> AuthorDate: Tue Sep 18 14:40:55 2018 -0700 Performed RP-specific validations when adding/updating RP configs. Each type of RP might have some specific validations for RP info. For example, SLRP requires the `storage` field to be set. This patch makes the local RP daemon to perform such validations when adding/updating configs, so the `ADD_RESOURCE_PROVIDER_CONFIG` and `UPDATE_RESOURCE_PROVIDER_CONFIG` calls can fail fast. Review: https://reviews.apache.org/r/68756 --- src/resource_provider/daemon.cpp | 2 +- src/resource_provider/local.cpp | 61 ++++++++++++++++++------------ src/resource_provider/local.hpp | 4 ++ src/resource_provider/storage/provider.cpp | 34 +++++++++++------ src/resource_provider/storage/provider.hpp | 9 +---- src/slave/http.cpp | 18 +++++++++ 6 files changed, 85 insertions(+), 43 deletions(-) diff --git a/src/resource_provider/daemon.cpp b/src/resource_provider/daemon.cpp index 0a76cc6..b07ffe3 100644 --- a/src/resource_provider/daemon.cpp +++ b/src/resource_provider/daemon.cpp @@ -466,7 +466,7 @@ Future<Nothing> LocalResourceProviderDaemonProcess::_launch( "' and name '" + name + "': " + provider.error()); } - data.provider = provider.get(); + data.provider = std::move(provider.get()); return Nothing(); } diff --git a/src/resource_provider/local.cpp b/src/resource_provider/local.cpp index 801e6c4..bd443aa 100644 --- a/src/resource_provider/local.cpp +++ b/src/resource_provider/local.cpp @@ -14,8 +14,9 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include <type_traits> + #include <stout/hashmap.hpp> -#include <stout/lambda.hpp> #include "resource_provider/local.hpp" @@ -32,6 +33,25 @@ using process::http::authentication::Principal; namespace mesos { namespace internal { +struct ProviderAdaptor +{ + decltype(LocalResourceProvider::create)* const create; + decltype(LocalResourceProvider::principal)* const principal; + decltype(LocalResourceProvider::validate)* const validate; +}; + + +// TODO(jieyu): Document the built-in local resource providers. +static const hashmap<string, ProviderAdaptor> adaptors = { +#ifdef __linux__ + {"org.apache.mesos.rp.local.storage", + {&StorageLocalResourceProvider::create, + &StorageLocalResourceProvider::principal, + &StorageLocalResourceProvider::validate}}, +#endif +}; + + Try<Owned<LocalResourceProvider>> LocalResourceProvider::create( const http::URL& url, const string& workDir, @@ -40,40 +60,33 @@ Try<Owned<LocalResourceProvider>> LocalResourceProvider::create( const Option<string>& authToken, bool strict) { - // TODO(jieyu): Document the built-in local resource providers. - const hashmap<string, lambda::function<decltype(create)>> creators = { -#if defined(__linux__) - {"org.apache.mesos.rp.local.storage", &StorageLocalResourceProvider::create} -#endif - }; - - if (creators.contains(info.type())) { - return creators.at(info.type())( - url, workDir, info, slaveId, authToken, strict); + if (!adaptors.contains(info.type())) { + return Error("Unknown local resource provider type '" + info.type() + "'"); } - return Error("Unknown local resource provider type '" + info.type() + "'"); + return adaptors.at(info.type()).create( + url, workDir, info, slaveId, authToken, strict); } Try<Principal> LocalResourceProvider::principal( const ResourceProviderInfo& info) { - // TODO(chhsiao): Document the principals for built-in local resource - // providers. - const hashmap<string, lambda::function<decltype(principal)>> - principalGenerators = { -#if defined(__linux__) - {"org.apache.mesos.rp.local.storage", - &StorageLocalResourceProvider::principal} -#endif - }; + if (!adaptors.contains(info.type())) { + return Error("Unknown local resource provider type '" + info.type() + "'"); + } + + return adaptors.at(info.type()).principal(info); +} + - if (principalGenerators.contains(info.type())) { - return principalGenerators.at(info.type())(info); +Option<Error> LocalResourceProvider::validate(const ResourceProviderInfo& info) +{ + if (!adaptors.contains(info.type())) { + return Error("Unknown local resource provider type '" + info.type() + "'"); } - return Error("Unknown local resource provider type '" + info.type() + "'"); + return adaptors.at(info.type()).validate(info); } } // namespace internal { diff --git a/src/resource_provider/local.hpp b/src/resource_provider/local.hpp index 20bcc78..ff4e7f4 100644 --- a/src/resource_provider/local.hpp +++ b/src/resource_provider/local.hpp @@ -23,6 +23,8 @@ #include <process/http.hpp> #include <process/owned.hpp> +#include <stout/error.hpp> +#include <stout/option.hpp> #include <stout/try.hpp> namespace mesos { @@ -42,6 +44,8 @@ public: static Try<process::http::authentication::Principal> principal( const ResourceProviderInfo& info); + static Option<Error> validate(const ResourceProviderInfo& info); + virtual ~LocalResourceProvider() = default; }; diff --git a/src/resource_provider/storage/provider.cpp b/src/resource_provider/storage/provider.cpp index 6475f65..18f934b 100644 --- a/src/resource_provider/storage/provider.cpp +++ b/src/resource_provider/storage/provider.cpp @@ -3755,6 +3755,28 @@ Try<Owned<LocalResourceProvider>> StorageLocalResourceProvider::create( const Option<string>& authToken, bool strict) { + Option<Error> error = validate(info); + if (error.isSome()) { + return error.get(); + } + + return Owned<LocalResourceProvider>(new StorageLocalResourceProvider( + url, workDir, info, slaveId, authToken, strict)); +} + + +Try<Principal> StorageLocalResourceProvider::principal( + const ResourceProviderInfo& info) +{ + return Principal( + Option<string>::none(), + {{"cid_prefix", getContainerIdPrefix(info)}}); +} + + +Option<Error> StorageLocalResourceProvider::validate( + const ResourceProviderInfo& info) +{ if (info.has_id()) { return Error("'ResourceProviderInfo.id' must not be set"); } @@ -3805,17 +3827,7 @@ Try<Owned<LocalResourceProvider>> StorageLocalResourceProvider::create( stringify(CSIPluginContainerInfo::NODE_SERVICE) + " not found"); } - return Owned<LocalResourceProvider>(new StorageLocalResourceProvider( - url, workDir, info, slaveId, authToken, strict)); -} - - -Try<Principal> StorageLocalResourceProvider::principal( - const ResourceProviderInfo& info) -{ - return Principal( - Option<string>::none(), - {{"cid_prefix", getContainerIdPrefix(info)}}); + return None(); } diff --git a/src/resource_provider/storage/provider.hpp b/src/resource_provider/storage/provider.hpp index 5a371b1..331f7b7 100644 --- a/src/resource_provider/storage/provider.hpp +++ b/src/resource_provider/storage/provider.hpp @@ -17,13 +17,6 @@ #ifndef __RESOURCE_PROVIDER_STORAGE_PROVIDER_HPP__ #define __RESOURCE_PROVIDER_STORAGE_PROVIDER_HPP__ -#include <process/http.hpp> -#include <process/owned.hpp> - -#include <stout/try.hpp> - -#include <mesos/mesos.hpp> - #include "resource_provider/local.hpp" namespace mesos { @@ -47,6 +40,8 @@ public: static Try<process::http::authentication::Principal> principal( const mesos::ResourceProviderInfo& info); + static Option<Error> validate(const mesos::ResourceProviderInfo& info); + ~StorageLocalResourceProvider() override; StorageLocalResourceProvider( diff --git a/src/slave/http.cpp b/src/slave/http.cpp index 0adc7c0..a4db532 100644 --- a/src/slave/http.cpp +++ b/src/slave/http.cpp @@ -68,6 +68,8 @@ #include "mesos/mesos.hpp" #include "mesos/resources.hpp" +#include "resource_provider/local.hpp" + #include "slave/http.hpp" #include "slave/slave.hpp" #include "slave/validation.hpp" @@ -3206,6 +3208,14 @@ Future<Response> Http::addResourceProviderConfig( << "Processing ADD_RESOURCE_PROVIDER_CONFIG call with type '" << info.type() << "' and name '" << info.name() << "'"; + Option<Error> error = LocalResourceProvider::validate(info); + if (error.isSome()) { + return BadRequest( + "Failed to validate resource provider config with type '" + + info.type() + "' and name '" + info.name() + "': " + + error->message); + } + return slave->localResourceProviderDaemon->add(info) .then([](bool added) -> Response { if (!added) { @@ -3243,6 +3253,14 @@ Future<Response> Http::updateResourceProviderConfig( << "Processing UPDATE_RESOURCE_PROVIDER_CONFIG call with type '" << info.type() << "' and name '" << info.name() << "'"; + Option<Error> error = LocalResourceProvider::validate(info); + if (error.isSome()) { + return BadRequest( + "Failed to validate resource provider config with type '" + + info.type() + "' and name '" + info.name() + "': " + + error->message); + } + return slave->localResourceProviderDaemon->update(info) .then([](bool updated) -> Response { if (!updated) {
