Added `PluginError` to simplify error reporting for CNI plugins. https://reviews.apache.org/r/51737/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/539d67f6 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/539d67f6 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/539d67f6 Branch: refs/heads/master Commit: 539d67f6cd4dd15c28ae0e6ff4f90f927e801329 Parents: fba4c1e Author: Avinash sridharan <avin...@mesosphere.io> Authored: Wed Oct 12 09:48:46 2016 -0700 Committer: Jie Yu <yujie....@gmail.com> Committed: Wed Oct 12 10:59:45 2016 -0700 ---------------------------------------------------------------------- .../network/cni/plugins/port_mapper/main.cpp | 11 +-- .../cni/plugins/port_mapper/port_mapper.cpp | 74 +++++++++++--------- .../cni/plugins/port_mapper/port_mapper.hpp | 5 +- .../mesos/isolators/network/cni/spec.hpp | 17 +++++ 4 files changed, 64 insertions(+), 43 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/539d67f6/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp b/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp index 11c03fd..0bb58c4 100644 --- a/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp +++ b/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp @@ -34,6 +34,7 @@ using std::string; using process::Owned; using mesos::internal::slave::cni::PortMapper; +using mesos::internal::slave::cni::spec::PluginError; constexpr int STDIN_READ_LENGTH = 1000; @@ -60,19 +61,13 @@ int main(int argc, char** argv) return EXIT_FAILURE; } - // If the `PortMapper` returns an error it will already be a JSON - // formatted string of type `spec::Error` so we don't need to format - // it again. Reason we rely on the `PortMapper` to return a JSON - // formatted `spec::Error` is that the error codes for `spec::Error` - // might vary, depending on the cause of error, and the context of - // the error is only visible to the `PortMapper` object. - Try<Owned<PortMapper>> portMapper = PortMapper::create(config); + Try<Owned<PortMapper>, PluginError> portMapper = PortMapper::create(config); if (portMapper.isError()) { cout << portMapper.error() << endl; return EXIT_FAILURE; } - Try<string> result = portMapper.get()->execute(); + Try<string, PluginError> result = portMapper.get()->execute(); if (result.isError()) { cout << result.error() << endl; return EXIT_FAILURE; http://git-wip-us.apache.org/repos/asf/mesos/blob/539d67f6/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp b/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp index 836fed5..5caa6d7 100644 --- a/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp +++ b/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp @@ -17,6 +17,12 @@ #include <stout/os.hpp> #include <stout/protobuf.hpp> +#include <process/collect.hpp> +#include <process/dispatch.hpp> +#include <process/io.hpp> +#include <process/subprocess.hpp> + +#include "slave/containerizer/mesos/isolators/network/cni/spec.hpp" #include "slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp" using std::string; @@ -27,18 +33,20 @@ using process::Owned; using mesos::NetworkInfo; +using mesos::internal::slave::cni::spec::PluginError; + namespace mesos { namespace internal { namespace slave { namespace cni { -Try<Owned<PortMapper>> PortMapper::create(const string& _cniConfig) +Try<Owned<PortMapper>, PluginError> PortMapper::create(const string& _cniConfig) { Option<string> cniCommand = os::getenv("CNI_COMMAND"); if (cniCommand.isNone()) { - return Error(spec::error( + return PluginError( "Unable to find environment variable 'CNI_COMMAND'", - ERROR_BAD_ARGS)); + ERROR_BAD_ARGS); } // 'CNI_CONTAINERID' is optional. @@ -46,16 +54,16 @@ Try<Owned<PortMapper>> PortMapper::create(const string& _cniConfig) Option<string> cniNetNs = os::getenv("CNI_NETNS"); if (cniNetNs.isNone()) { - return Error(spec::error( + return PluginError( "Unable to find environment variable 'CNI_NETNS'", - ERROR_BAD_ARGS)); + ERROR_BAD_ARGS); } Option<string> cniIfName = os::getenv("CNI_IFNAME"); if (cniIfName.isNone()) { - return Error(spec::error( + return PluginError( "Unable to find environment variable 'CNI_IFNAME'", - ERROR_BAD_ARGS)); + ERROR_BAD_ARGS); } // 'CNI_ARGS' is optional. @@ -63,33 +71,33 @@ Try<Owned<PortMapper>> PortMapper::create(const string& _cniConfig) Option<string> cniPath = os::getenv("CNI_PATH"); if (cniPath.isNone()) { - return Error(spec::error( + return PluginError( "Unable to find environment variable 'CNI_PATH'", - ERROR_BAD_ARGS)); + ERROR_BAD_ARGS); } // Verify the CNI config for this plugin. Try<JSON::Object> cniConfig = JSON::parse<JSON::Object>(_cniConfig); if (cniConfig.isError()) { - return Error(spec::error(cniConfig.error(), ERROR_BAD_ARGS)); + return PluginError(cniConfig.error(), ERROR_BAD_ARGS); } // TODO(jieyu): Validate 'cniVersion' and 'type'. Result<JSON::String> name = cniConfig->find<JSON::String>("name"); if (!name.isSome()) { - return Error(spec::error( + return PluginError( "Failed to get the required field 'name': " + (name.isError() ? name.error() : "Not found"), - ERROR_BAD_ARGS)); + ERROR_BAD_ARGS); } Result<JSON::String> chain = cniConfig->find<JSON::String>("chain"); if (!chain.isSome()) { - return Error(spec::error( + return PluginError( "Failed to get the required field 'chain': " + (chain.isError() ? chain.error() : "Not found"), - ERROR_BAD_ARGS)); + ERROR_BAD_ARGS); } vector<string> excludeDevices; @@ -98,17 +106,17 @@ Try<Owned<PortMapper>> PortMapper::create(const string& _cniConfig) cniConfig->find<JSON::Array>("excludeDevices"); if (_excludeDevices.isError()) { - return Error(spec::error( + return PluginError( "Failed to parse field 'excludeDevices': " + _excludeDevices.error(), - ERROR_BAD_ARGS)); + ERROR_BAD_ARGS); } else if (_excludeDevices.isSome()) { foreach (const JSON::Value& value, _excludeDevices->values) { if (!value.is<JSON::String>()) { - return Error(spec::error( + return PluginError( "Failed to parse 'excludeDevices' list. " "The excluded device needs to be a string", - ERROR_BAD_ARGS)); + ERROR_BAD_ARGS); } excludeDevices.push_back(value.as<JSON::String>().value); @@ -120,10 +128,10 @@ Try<Owned<PortMapper>> PortMapper::create(const string& _cniConfig) // framework might have requested for this container. Result<JSON::Object> args = cniConfig->find<JSON::Object>("args"); if (!args.isSome()) { - return Error(spec::error( + return PluginError( "Failed to get the required field 'args': " + (args.isError() ? args.error() : "Not found"), - ERROR_BAD_ARGS)); + ERROR_BAD_ARGS); } // NOTE: We can't directly use `find` to check for 'network_info' @@ -133,27 +141,27 @@ Try<Owned<PortMapper>> PortMapper::create(const string& _cniConfig) // 'network_info'. Result<JSON::Object> mesos = args->at<JSON::Object>("org.apache.mesos"); if (!mesos.isSome()) { - return Error(spec::error( + return PluginError( "Failed to get the field 'args{org.apache.mesos}': " + (mesos.isError() ? mesos.error() : "Not found"), - ERROR_BAD_ARGS)); + ERROR_BAD_ARGS); } Result<JSON::Object> _networkInfo = mesos->find<JSON::Object>("network_info"); if (!_networkInfo.isSome()) { - return Error(spec::error( + return PluginError( "Failed to get the field 'args{org.apache.mesos}{network_info}': " + (_networkInfo.isError() ? _networkInfo.error() : "Not found"), - ERROR_BAD_ARGS)); + ERROR_BAD_ARGS); } Try<NetworkInfo> networkInfo = ::protobuf::parse<NetworkInfo>(_networkInfo.get()); if (networkInfo.isError()) { - return Error(spec::error( + return PluginError( "Unable to parse `NetworkInfo`: " + networkInfo.error(), - ERROR_BAD_ARGS)); + ERROR_BAD_ARGS); } // The port-mapper should always be used in conjunction with another @@ -162,10 +170,10 @@ Try<Owned<PortMapper>> PortMapper::create(const string& _cniConfig) cniConfig->find<JSON::Object>("delegate"); if (!delegateConfig.isSome()) { - return Error(spec::error( + return PluginError( "Failed to get the required field 'delegate'" + (delegateConfig.isError() ? delegateConfig.error() : "Not found"), - ERROR_BAD_ARGS)); + ERROR_BAD_ARGS); } // TODO(jieyu): Validate that 'cniVersion' and 'name' exist in @@ -176,17 +184,17 @@ Try<Owned<PortMapper>> PortMapper::create(const string& _cniConfig) delegateConfig->find<JSON::String>("type"); if (!delegatePlugin.isSome()) { - return Error(spec::error( + return PluginError( "Failed to get the delegate plugin 'type'" + (delegatePlugin.isError() ? delegatePlugin.error() : "Not found"), - ERROR_BAD_ARGS)); + ERROR_BAD_ARGS); } if (os::which(delegatePlugin->value, cniPath.get()).isNone()) { - return Error(spec::error( + return PluginError( "Could not find the delegate plugin '" + delegatePlugin->value + "' in '" + cniPath.get() + "'", - ERROR_BAD_ARGS)); + ERROR_BAD_ARGS); } return Owned<PortMapper>( @@ -205,7 +213,7 @@ Try<Owned<PortMapper>> PortMapper::create(const string& _cniConfig) } -Try<string> PortMapper::execute() +Try<string, PluginError> PortMapper::execute() { return "OK"; } http://git-wip-us.apache.org/repos/asf/mesos/blob/539d67f6/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp b/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp index b943254..c0bbef6 100644 --- a/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp +++ b/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp @@ -65,7 +65,8 @@ public: // // In case of an error returns a JSON formatted string of type // `spec::Error` as the error message for the `Try`. - static Try<process::Owned<PortMapper>> create(const std::string& cniConfig); + static Try<process::Owned<PortMapper>, spec::PluginError> create( + const std::string& cniConfig); // Executes the CNI plugin specified in 'delegate'. On successful // execution of the 'delegate' plugin will install port-forwarding @@ -74,7 +75,7 @@ public: // 'delegate' plugin. In case of an error will return a JSON string // representation of `spec::Error` as the error message for the // `Try`. - Try<std::string> execute(); + Try<std::string, spec::PluginError> execute(); virtual ~PortMapper() {}; http://git-wip-us.apache.org/repos/asf/mesos/blob/539d67f6/src/slave/containerizer/mesos/isolators/network/cni/spec.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/isolators/network/cni/spec.hpp b/src/slave/containerizer/mesos/isolators/network/cni/spec.hpp index 2d0187b..9f0542a 100644 --- a/src/slave/containerizer/mesos/isolators/network/cni/spec.hpp +++ b/src/slave/containerizer/mesos/isolators/network/cni/spec.hpp @@ -48,6 +48,23 @@ Try<NetworkInfo> parseNetworkInfo(const std::string& s); // https://github.com/containernetworking/cni/blob/master/SPEC.md#result std::string error(const std::string& msg, uint32_t code); + +// This class encapsulates a JSON formatted string of type +// `spec::Error`. It can be used by CNI plugins to return a CNI error +// in a `Try` object when a failure occurs. +class PluginError : public ::Error +{ +public: + PluginError(const std::string& msg, uint32_t code) + : ::Error(error(msg, code)) {} +}; + + +inline std::ostream& operator<<(std::ostream& stream, const PluginError& _error) +{ + return stream << _error.message; +} + } // namespace spec { } // namespace cni { } // namespace slave {
