bmahler commented on code in PR #586: URL: https://github.com/apache/mesos/pull/586#discussion_r1828426706
########## src/tests/containerizer/docker_cni_tests.cpp: ########## @@ -0,0 +1,326 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY K:5IND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include <gmock/gmock.h> + +#include <gtest/gtest.h> + +#include <mesos/v1/mesos.hpp> +#include <mesos/slave/container_logger.hpp> + +#include <process/owned.hpp> + +#include "slave/containerizer/docker.hpp" +#include "slave/containerizer/fetcher.hpp" +#include "slave/containerizer/mesos/isolators/network/cni/cni.hpp" + +#include "slave/paths.hpp" +#include "slave/slave.hpp" +#include "slave/state.hpp" + +#include "tests/flags.hpp" +#include "tests/mesos.hpp" +#include "tests/environment.hpp" +#include "tests/containerizer/docker_common.hpp" +#include "tests/mock_docker.hpp" + + +using namespace process; + +using namespace mesos::internal::slave::paths; +using namespace mesos::internal::slave::state; + +using mesos::internal::master::Master; +using mesos::internal::slave::DockerContainerizer; +using mesos::internal::slave::DockerContainerizerProcess; +using mesos::internal::slave::Fetcher; +using mesos::internal::slave::Slave; + +using mesos::master::detector::MasterDetector; + +using mesos::slave::ContainerConfig; +using mesos::slave::ContainerLogger; +using mesos::slave::ContainerTermination; + +using std::list; +using std::string; +using std::vector; + +using testing::_; +using testing::DoAll; +using testing::DoDefault; +using testing::Eq; +using testing::Invoke; +using testing::Return; + +namespace mesos { +namespace internal { +namespace tests { + +constexpr char MESOS_MOCK_CNI_CONFIG[] = "mockConfig"; + + +static ContainerInfo createDockerInfo(const string& imageName) +{ + ContainerInfo containerInfo; + + containerInfo.set_type(ContainerInfo::DOCKER); + containerInfo.mutable_docker()->set_image(imageName); + + return containerInfo; +} + +class DockerCniTest : public MesosTest +{ +public: + string cniPluginDir; + string cniConfigDir; + + void SetUp() override + { + MesosTest::SetUp(); + + cniPluginDir = path::join(sandbox.get(), "plugins"); + cniConfigDir = path::join(sandbox.get(), "configs"); + + // This is a veth CNI plugin that is written in bash. It creates a + // veth virtual network pair, one end of the pair will be moved to + // container network namespace. + // + // The veth CNI plugin uses 203.0.113.0/24 subnet, it is reserved + // for documentation and examples [rfc5737]. The plugin can + // allocate up to 128 veth pairs. + Try<Nothing> result = setupMockPlugin( + strings::format(R"~( + #!/bin/sh + set -e + IP_ADDR="203.0.113.10/32" + + NETNS="mesos-test-veth-${PPID}" + mkdir -p /var/run/netns + cleanup() { + rm -f /var/run/netns/$NETNS + } + trap cleanup EXIT + ln -sf $CNI_NETNS /var/run/netns/$NETNS + + VETH0="vmesos1" + VETH1="eth0" + + case $CNI_COMMAND in + "ADD"*) + ip link delete $VETH0 2>/dev/null || true + ip netns exec $NETNS ip link delete $VETH1 2>/dev/null || true + + ip link add name $VETH0 type veth peer name $VETH1 > /tmp/cni.log || continue + ip addr add "${IP_ADDR}" dev $VETH0 >> /tmp/cni.log + ip link set $VETH0 up + ip link set $VETH1 netns $NETNS + ip netns exec $NETNS ip addr add "${IP_ADDR}" dev $VETH1 + ip netns exec $NETNS ip link set $VETH1 up + + echo "{" + echo " \"ip4\": {" + echo " \"ip\": \"${IP_ADDR}\"" + echo " }," + echo " \"dns\": {" + echo " \"nameservers\": [ \"8.8.8.8\" ]" + echo " }" + echo "}" + ;; + "DEL"*) + # $VETH0 on host network namespace will be removed automatically. + # If the plugin can't destroy the veth pair, it will be destroyed + # with the container network namespace. + ip link delete $VETH0 2>/dev/null || true + ip netns exec $NETNS ip link del $VETH1 || true + ;; + esac + )~").get()); + + ASSERT_SOME(result); + + // Generate the mock CNI config for veth CNI plugin. + ASSERT_SOME(os::mkdir(cniConfigDir)); + + result = os::write( + path::join(cniConfigDir, MESOS_MOCK_CNI_CONFIG), + R"~( + { + "name": "__MESOS_TEST__", + "type": "mockPlugin" + })~"); + } + + // Generate the mock CNI plugin based on the given script. + Try<Nothing> setupMockPlugin(const string& pluginScript) + { + return setupPlugin(pluginScript, "mockPlugin"); + } + + // Generate the CNI plugin based on the given script. + Try<Nothing> setupPlugin(const string& pluginScript, const string& pluginName) + { + Try<Nothing> mkdir = os::mkdir(cniPluginDir); + if (mkdir.isError()) { + return Error("Failed to mkdir '" + cniPluginDir + "': " + mkdir.error()); + } + + string pluginPath = path::join(cniPluginDir, pluginName); + + Try<Nothing> write = os::write(pluginPath, pluginScript); + if (write.isError()) { + return Error("Failed to write '" + pluginPath + "': " + write.error()); + } + + // Make sure the plugin has execution permission. + Try<Nothing> chmod = os::chmod( + pluginPath, + S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH); + + if (chmod.isError()) { + return Error("Failed to chmod '" + pluginPath + "': " + chmod.error()); + } + + return Nothing(); + } + + static string containerName(const ContainerID& containerId) + { + return slave::DOCKER_NAME_PREFIX + containerId.value(); + } + +protected: + slave::Flags CreateSlaveFlags() override + { + slave::Flags flags = MesosTest::CreateSlaveFlags(); + + flags.network_cni_plugins_dir = cniPluginDir; + flags.network_cni_config_dir = cniConfigDir; + + + return flags; + } +}; + +// This test tests the functionality of the docker's interfaces. +TEST_F(DockerCniTest, ROOT_DOCKER_ip_interface) +{ + SetUp(); Review Comment: nit: inconsistent indentation in your patch, can you sweep it to use a 2 space indent? ########## src/docker/executor.cpp: ########## @@ -318,7 +329,26 @@ class DockerExecutorProcess : public ProtobufProcess<DockerExecutorProcess> } }; - if (container.ipAddress.isSome()) { + bool useCNI = false; +#ifdef __linux__ + if (network_cni_config_dir.size() > 0 && network_cni_plugins_dir.size() > 0) { Review Comment: use `!x.empty()`? Or it seems like these should be `Option<string>` instead to explicitly type in the optionality and avoid empty string == unset Also, does it make sense for only one to be set? If not, should there be some validation somewhere for this? ########## src/docker/executor.cpp: ########## @@ -318,7 +329,26 @@ class DockerExecutorProcess : public ProtobufProcess<DockerExecutorProcess> } }; - if (container.ipAddress.isSome()) { + bool useCNI = false; +#ifdef __linux__ + if (network_cni_config_dir.size() > 0 && network_cni_plugins_dir.size() > 0) { + Try<std::tuple<Subprocess, std::string>> cni = attachCNI(task); + if (!cni.isError()) { + Subprocess cniSub = std::get<0>(cni.get()); + std::string cniNetworkName = std::get<1>(cni.get()); + + WIFEXITED(cniSub.status().get().get()); + Try<std::string> cniIP = attachCNISuccess(cniSub); + if (cniIP.isError()) { + LOG(ERROR) << cniIP.error(); + } else { + useCNI = true; + setIPAddresses(cniIP.get(), false); + } + } Review Comment: hm.. this looks like a bad design? docker run has already been called, *and then* we attach CNI? This means the workload could have started and failed to perform networking operations? It seems like the correct way to do this is to perform CNI work while setting up the container, whereas this looks like a hack where it's set up *after* the container has been started already? ########## src/launcher/docker_executor.cpp: ########## @@ -148,6 +148,14 @@ int main(int argc, char** argv) << flags.usage("Missing required option --mapped_directory"); } + if (flags.network_cni_plugins_dir.isNone()) { + flags.network_cni_plugins_dir = "/usr/lib/cni/"; + } + + if (flags.network_cni_config_dir.isNone()) { + flags.network_cni_config_dir = "/etc/mesos/cni/net.d"; + } Review Comment: These defaults look a bit odd, as if the agent's CNI flags actually have default values only when used with the docker executor? It seems like the agent flags should be directly used and therefore the operator has to set these explicitly? Will that cause a problem for you? Right now this is a bit odd: 1. Don't set the agent's CNI flags, this gets you these default values only for docker containerizer / executor, but no defaults for the mesos containerizer. That looks strange. 2. Set the agent's CNI flags, in which case both the docker containerizer / mesos containerizer would use the values? ########## src/docker/executor.cpp: ########## @@ -785,6 +825,261 @@ class DockerExecutorProcess : public ProtobufProcess<DockerExecutorProcess> driver.get()->stop(); } + // Return the content of the CNI plugin config file and return it as json object + Try<JSON::Object> getNetworkConfigJSON(const string& fname) + { + Try<string> read = os::read(fname); + if (read.isError()) { + return Error("Cannot read CNI plugin file: " + read.error()); + } + + Try<JSON::Object> parse = JSON::parse<JSON::Object>(read.get()); + if (parse.isError()) { + return Error("Cannot parse CNI plugin file." + parse.error()); + } + + return parse; + } + + // Search the config file of the network in the path + Try<std::string> getNetworkConfigFile(const string& network, const string& path) + { + struct dirent *ent; + DIR *dir; + JSON::Object parse; + + // path can include mutiple path's seperated by ":". + char *sPath = strtok(const_cast<char*>(path.c_str()), ":"); + while (sPath != NULL) { + if ((dir = opendir(sPath)) != NULL) { + while ((ent = readdir(dir)) != NULL) { + if (ent->d_type != DT_REG || !strcmp(ent->d_name, ".") || !strcmp(ent->d_name, "..")) { + continue; + } + + std::string fname = string(sPath) + "/" + string(ent->d_name); + Try<string> read = os::read(fname); + if (read.isError()) { + LOG(ERROR) << "Cannot read CNI network config file: " << fname; + continue; + } + + Try<JSON::Object> parse = JSON::parse<JSON::Object>(read.get()); + if (parse.isError()) { + LOG(ERROR) << "Cannot parse CNI network config file."; + continue; + } + + Result<JSON::String> name = parse->at<JSON::String>("name"); + if (!name.isSome()) { + return Error( + "Cannot determine the 'name' of the CNI network for this " + "configuration " + + (name.isNone() ? "'" : ("': " + name.error()))); + } + + // Verify the configuration is for this network + if (network == name->value) { + return fname; + } + } + closedir(dir); + } + sPath = strtok(NULL, ":"); + } + return Error("Cannot find a CNI plugin config for this network name: " + network); + } + + // attach CNI at the container + Try<std::tuple<Subprocess, std::string>> attachCNI(const TaskInfo& task) + { + container = task.container(); + return commandCNI(container, "ADD"); + } + + // detach CNI from the container + Try<std::tuple<Subprocess, std::string>> detachCNI() + { + return commandCNI(container, "DEL"); + } + + // function to attach or detach CNI from/to the container + Try<std::tuple<Subprocess, std::string>> commandCNI( + const mesos::ContainerInfo& container, + const string command) Review Comment: feels like these should probably just return a Future of whatever information you want? not clear what the string here would represent ########## src/docker/executor.hpp: ########## @@ -90,6 +90,20 @@ struct Flags : public virtual mesos::internal::logging::Flags "Cgroups feature flag to enable hard limits on CPU resources\n" "via the CFS bandwidth limiting subfeature.\n", false); + + add(&Flags::network_cni_plugins_dir, + "network_cni_plugins_dir", + "A search path for CNI plugin binaries. The docker executer\n" + "will find CNI plugins under these set of directories so that\n" + "it can execute the plugins to add/delete container from the CNI\n" + "networks."); + + add(&Flags::network_cni_config_dir, + "network_cni_config_dir", + "Directory path of the CNI network configuration files. For each\n" + "network that containers launched in Mesos agent can connect to,\n" + "the operator should install a network configuration file in JSON\n" + "format in the specified directory."); Review Comment: nit: inconsistent alignment -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
