This is an automated email from the ASF dual-hosted git repository. qianzhang pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 577d4ebee6131356f768266c289fa24f0948b2b9 Author: Qian Zhang <[email protected]> AuthorDate: Thu Jan 2 14:22:23 2020 +0800 Updated containerizer's `update()` method to handle resource limits. Review: https://reviews.apache.org/r/71950 --- src/slave/containerizer/composing.cpp | 15 ++++++++++----- src/slave/containerizer/composing.hpp | 4 +++- src/slave/containerizer/containerizer.hpp | 4 +++- src/slave/containerizer/docker.cpp | 21 ++++++++++++--------- src/slave/containerizer/docker.hpp | 7 +++++-- src/slave/containerizer/mesos/containerizer.cpp | 14 +++++++++----- src/slave/containerizer/mesos/containerizer.hpp | 8 ++++++-- src/tests/containerizer.cpp | 11 +++++++---- src/tests/containerizer.hpp | 10 +++++++--- .../containerizer/docker_containerizer_tests.cpp | 8 ++++---- src/tests/containerizer/mock_containerizer.hpp | 5 +++-- src/tests/master_draining_tests.cpp | 2 +- src/tests/master_tests.cpp | 12 ++++++------ src/tests/mock_docker.hpp | 13 ++++++++----- src/tests/registrar_zookeeper_tests.cpp | 2 +- src/tests/scheduler_tests.cpp | 2 +- src/tests/slave_tests.cpp | 12 ++++++------ 17 files changed, 92 insertions(+), 58 deletions(-) diff --git a/src/slave/containerizer/composing.cpp b/src/slave/containerizer/composing.cpp index d854794..e25c604 100644 --- a/src/slave/containerizer/composing.cpp +++ b/src/slave/containerizer/composing.cpp @@ -74,7 +74,9 @@ public: Future<Nothing> update( const ContainerID& containerId, - const Resources& resources); + const Resources& resourceRequests, + const google::protobuf::Map< + std::string, Value::Scalar>& resourceLimits = {}); Future<ResourceStatistics> usage( const ContainerID& containerId); @@ -187,12 +189,14 @@ Future<Containerizer::LaunchResult> ComposingContainerizer::launch( Future<Nothing> ComposingContainerizer::update( const ContainerID& containerId, - const Resources& resources) + const Resources& resourceRequests, + const google::protobuf::Map<string, Value::Scalar>& resourceLimits) { return dispatch(process, &ComposingContainerizerProcess::update, containerId, - resources); + resourceRequests, + resourceLimits); } @@ -547,14 +551,15 @@ Future<http::Connection> ComposingContainerizerProcess::attach( Future<Nothing> ComposingContainerizerProcess::update( const ContainerID& containerId, - const Resources& resources) + const Resources& resourceRequests, + const google::protobuf::Map<string, Value::Scalar>& resourceLimits) { if (!containers_.contains(containerId)) { return Failure("Container not found"); } return containers_[containerId]->containerizer->update( - containerId, resources); + containerId, resourceRequests, resourceLimits); } diff --git a/src/slave/containerizer/composing.hpp b/src/slave/containerizer/composing.hpp index 0f838fa..dcd6a26 100644 --- a/src/slave/containerizer/composing.hpp +++ b/src/slave/containerizer/composing.hpp @@ -65,7 +65,9 @@ public: process::Future<Nothing> update( const ContainerID& containerId, - const Resources& resources) override; + const Resources& resourceRequests, + const google::protobuf::Map< + std::string, Value::Scalar>& resourceLimits = {}) override; process::Future<ResourceStatistics> usage( const ContainerID& containerId) override; diff --git a/src/slave/containerizer/containerizer.hpp b/src/slave/containerizer/containerizer.hpp index 6a9ebed..2b3c4c0 100644 --- a/src/slave/containerizer/containerizer.hpp +++ b/src/slave/containerizer/containerizer.hpp @@ -119,7 +119,9 @@ public: // Update the resources for a container. virtual process::Future<Nothing> update( const ContainerID& containerId, - const Resources& resources) = 0; + const Resources& resourceRequests, + const google::protobuf::Map< + std::string, Value::Scalar>& resourceLimits = {}) = 0; // Get resource usage statistics on the container. virtual process::Future<ResourceStatistics> usage( diff --git a/src/slave/containerizer/docker.cpp b/src/slave/containerizer/docker.cpp index 2a9b2ff..492ac27 100644 --- a/src/slave/containerizer/docker.cpp +++ b/src/slave/containerizer/docker.cpp @@ -819,13 +819,15 @@ Future<Containerizer::LaunchResult> DockerContainerizer::launch( Future<Nothing> DockerContainerizer::update( const ContainerID& containerId, - const Resources& resources) + const Resources& resourceRequests, + const google::protobuf::Map<string, Value::Scalar>& resourceLimits) { return dispatch( process.get(), &DockerContainerizerProcess::update, containerId, - resources, + resourceRequests, + resourceLimits, false); } @@ -1330,7 +1332,7 @@ Future<Containerizer::LaunchResult> DockerContainerizerProcess::_launch( // --cpu-quota to the 'docker run' call in // launchExecutorContainer. return update( - containerId, containerConfig.executor_info().resources(), true) + containerId, containerConfig.executor_info().resources(), {}, true) .then([=]() { return Future<Docker::Container>(dockerContainer); }); @@ -1654,7 +1656,8 @@ Future<Nothing> DockerContainerizerProcess::reapExecutor( Future<Nothing> DockerContainerizerProcess::update( const ContainerID& containerId, - const Resources& _resources, + const Resources& resourceRequests, + const google::protobuf::Map<string, Value::Scalar>& resourceLimits, bool force) { CHECK(!containerId.has_parent()); @@ -1672,7 +1675,7 @@ Future<Nothing> DockerContainerizerProcess::update( return Nothing(); } - if (container->resources == _resources && !force) { + if (container->resources == resourceRequests && !force) { LOG(INFO) << "Ignoring updating container " << containerId << " because resources passed to update are identical to" << " existing resources"; @@ -1685,17 +1688,17 @@ Future<Nothing> DockerContainerizerProcess::update( // TODO(gyliu): Support updating GPU resources. // Store the resources for usage(). - container->resources = _resources; + container->resources = resourceRequests; #ifdef __linux__ - if (!_resources.cpus().isSome() && !_resources.mem().isSome()) { + if (!resourceRequests.cpus().isSome() && !resourceRequests.mem().isSome()) { LOG(WARNING) << "Ignoring update as no supported resources are present"; return Nothing(); } // Skip inspecting the docker container if we already have the cgroups. if (container->cpuCgroup.isSome() && container->memoryCgroup.isSome()) { - return __update(containerId, _resources); + return __update(containerId, resourceRequests); } string containerName = containers_.at(containerId)->containerName; @@ -1739,7 +1742,7 @@ Future<Nothing> DockerContainerizerProcess::update( }); return inspectLoop - .then(defer(self(), &Self::_update, containerId, _resources, lambda::_1)); + .then(defer(self(), &Self::_update, containerId, resourceRequests, lambda::_1)); #else return Nothing(); #endif // __linux__ diff --git a/src/slave/containerizer/docker.hpp b/src/slave/containerizer/docker.hpp index 0349f53..09fc279 100644 --- a/src/slave/containerizer/docker.hpp +++ b/src/slave/containerizer/docker.hpp @@ -97,7 +97,9 @@ public: process::Future<Nothing> update( const ContainerID& containerId, - const Resources& resources) override; + const Resources& resourceRequests, + const google::protobuf::Map< + std::string, Value::Scalar>& resourceLimits = {}) override; process::Future<ResourceStatistics> usage( const ContainerID& containerId) override; @@ -151,7 +153,8 @@ public: // for the container, even if they match what it has cached. virtual process::Future<Nothing> update( const ContainerID& containerId, - const Resources& resources, + const Resources& resourceRequests, + const google::protobuf::Map<std::string, Value::Scalar>& resourceLimits, bool force); virtual process::Future<ResourceStatistics> usage( diff --git a/src/slave/containerizer/mesos/containerizer.cpp b/src/slave/containerizer/mesos/containerizer.cpp index d034c02..6aa4f3f 100644 --- a/src/slave/containerizer/mesos/containerizer.cpp +++ b/src/slave/containerizer/mesos/containerizer.cpp @@ -734,12 +734,14 @@ Future<Connection> MesosContainerizer::attach( Future<Nothing> MesosContainerizer::update( const ContainerID& containerId, - const Resources& resources) + const Resources& resourceRequests, + const google::protobuf::Map<string, Value::Scalar>& resourceLimits) { return dispatch(process.get(), &MesosContainerizerProcess::update, containerId, - resources); + resourceRequests, + resourceLimits); } @@ -2406,7 +2408,8 @@ Future<Option<ContainerTermination>> MesosContainerizerProcess::wait( Future<Nothing> MesosContainerizerProcess::update( const ContainerID& containerId, - const Resources& resources) + const Resources& resourceRequests, + const google::protobuf::Map<string, Value::Scalar>& resourceLimits) { CHECK(!containerId.has_parent()); @@ -2429,7 +2432,7 @@ Future<Nothing> MesosContainerizerProcess::update( // NOTE: We update container's resources before isolators are updated // so that subsequent containerizer->update can be handled properly. - container->resources = resources; + container->resources = resourceRequests; // Update each isolator. vector<Future<Nothing>> futures; @@ -2441,7 +2444,8 @@ Future<Nothing> MesosContainerizerProcess::update( continue; } - futures.push_back(isolator->update(containerId, resources)); + futures.push_back( + isolator->update(containerId, resourceRequests, resourceLimits)); } // Wait for all isolators to complete. diff --git a/src/slave/containerizer/mesos/containerizer.hpp b/src/slave/containerizer/mesos/containerizer.hpp index 9e86ff4..2ea033a 100644 --- a/src/slave/containerizer/mesos/containerizer.hpp +++ b/src/slave/containerizer/mesos/containerizer.hpp @@ -103,7 +103,9 @@ public: process::Future<Nothing> update( const ContainerID& containerId, - const Resources& resources) override; + const Resources& resourceRequests, + const google::protobuf::Map< + std::string, Value::Scalar>& resourceLimits = {}) override; process::Future<ResourceStatistics> usage( const ContainerID& containerId) override; @@ -197,7 +199,9 @@ public: virtual process::Future<Nothing> update( const ContainerID& containerId, - const Resources& resources); + const Resources& resourceRequests, + const google::protobuf::Map< + std::string, Value::Scalar>& resourceLimits = {}); virtual process::Future<ResourceStatistics> usage( const ContainerID& containerId); diff --git a/src/tests/containerizer.cpp b/src/tests/containerizer.cpp index 3ac992f..da78737 100644 --- a/src/tests/containerizer.cpp +++ b/src/tests/containerizer.cpp @@ -180,7 +180,8 @@ public: Future<Nothing> update( const ContainerID& containerId, - const Resources& resources) + const Resources& resourceRequests, + const google::protobuf::Map<string, Value::Scalar>& resourceLimits) { return Nothing(); } @@ -405,7 +406,7 @@ void TestContainerizer::setup() EXPECT_CALL(*this, status(_)) .WillRepeatedly(Invoke(this, &TestContainerizer::_status)); - EXPECT_CALL(*this, update(_, _)) + EXPECT_CALL(*this, update(_, _, _)) .WillRepeatedly(Invoke(this, &TestContainerizer::_update)); EXPECT_CALL(*this, launch(_, _, _, _)) @@ -466,13 +467,15 @@ Future<Connection> TestContainerizer::_attach( Future<Nothing> TestContainerizer::_update( const ContainerID& containerId, - const Resources& resources) + const Resources& resourceRequests, + const google::protobuf::Map<string, Value::Scalar>& resourceLimits) { return process::dispatch( process.get(), &TestContainerizerProcess::update, containerId, - resources); + resourceRequests, + resourceLimits); } diff --git a/src/tests/containerizer.hpp b/src/tests/containerizer.hpp index e05ce03..43de0c7 100644 --- a/src/tests/containerizer.hpp +++ b/src/tests/containerizer.hpp @@ -97,9 +97,12 @@ public: process::Future<process::http::Connection>( const ContainerID& containerId)); - MOCK_METHOD2( + MOCK_METHOD3( update, - process::Future<Nothing>(const ContainerID&, const Resources&)); + process::Future<Nothing>( + const ContainerID&, + const Resources&, + const google::protobuf::Map<std::string, Value::Scalar>&)); MOCK_METHOD1( usage, @@ -154,7 +157,8 @@ private: process::Future<Nothing> _update( const ContainerID& containerId, - const Resources& resources); + const Resources& resourceRequests, + const google::protobuf::Map<std::string, Value::Scalar>& resourceLimits); process::Future<ResourceStatistics> _usage( const ContainerID& containerId); diff --git a/src/tests/containerizer/docker_containerizer_tests.cpp b/src/tests/containerizer/docker_containerizer_tests.cpp index 689a722..658f821 100644 --- a/src/tests/containerizer/docker_containerizer_tests.cpp +++ b/src/tests/containerizer/docker_containerizer_tests.cpp @@ -1015,7 +1015,7 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Usage) &MockDockerContainerizer::_launch))); // We ignore all update calls to prevent resizing cgroup limits. - EXPECT_CALL(dockerContainerizer, update(_, _)) + EXPECT_CALL(dockerContainerizer, update(_, _, _)) .WillRepeatedly(Return(Nothing())); Future<TaskStatus> statusStarting; @@ -1183,7 +1183,7 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Update) ASSERT_SOME(newResources); Future<Nothing> update = - dockerContainerizer.update(containerId.get(), newResources.get()); + dockerContainerizer.update(containerId.get(), newResources.get(), {}); AWAIT_READY(update); @@ -1220,7 +1220,7 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Update) newResources = Resources::parse("cpus:1;mem:144"); // Issue second update that uses the cached cgroups instead of inspect. - update = dockerContainerizer.update(containerId.get(), newResources.get()); + update = dockerContainerizer.update(containerId.get(), newResources.get(), {}); AWAIT_READY(update); @@ -3541,7 +3541,7 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_ExecutorCleanupWhenLaunchFailed) &MockDockerContainerizer::_launch))); // Fail the update so we don't proceed to send run task to the executor. - EXPECT_CALL(dockerContainerizer, update(_, _)) + EXPECT_CALL(dockerContainerizer, update(_, _, _)) .WillRepeatedly(Return(Failure("Fail resource update"))); driver.launchTasks(offers.get()[0].id(), {task}); diff --git a/src/tests/containerizer/mock_containerizer.hpp b/src/tests/containerizer/mock_containerizer.hpp index 8700257..a668a6c 100644 --- a/src/tests/containerizer/mock_containerizer.hpp +++ b/src/tests/containerizer/mock_containerizer.hpp @@ -58,11 +58,12 @@ public: attach, process::Future<process::http::Connection>(const ContainerID&)); - MOCK_METHOD2( + MOCK_METHOD3( update, process::Future<Nothing>( const ContainerID&, - const Resources&)); + const Resources&, + const google::protobuf::Map<std::string, Value::Scalar>&)); MOCK_METHOD1( usage, diff --git a/src/tests/master_draining_tests.cpp b/src/tests/master_draining_tests.cpp index a91201e..f6b974f 100644 --- a/src/tests/master_draining_tests.cpp +++ b/src/tests/master_draining_tests.cpp @@ -1087,7 +1087,7 @@ TEST_P(MasterDrainingTest2, DisallowReactivationWhileDraining) EXPECT_CALL(exec, launchTask(_, _)) .WillRepeatedly(SendStatusUpdateFromTask(TASK_RUNNING)); - EXPECT_CALL(containerizer, update(_, _)) + EXPECT_CALL(containerizer, update(_, _, _)) .WillRepeatedly(Return(Nothing())); Promise<Option<ContainerTermination>> hang; diff --git a/src/tests/master_tests.cpp b/src/tests/master_tests.cpp index d0f53b2..617abfa 100644 --- a/src/tests/master_tests.cpp +++ b/src/tests/master_tests.cpp @@ -199,7 +199,7 @@ TEST_F(MasterTest, TaskRunning) Future<Nothing> update; EXPECT_CALL(containerizer, - update(_, Resources(offers.get()[0].resources()))) + update(_, Resources(offers.get()[0].resources()), _)) .WillOnce(DoAll(FutureSatisfy(&update), Return(Nothing()))); @@ -274,7 +274,7 @@ TEST_F(MasterTest, ShutdownFrameworkWhileTaskRunning) Future<Nothing> update; EXPECT_CALL(containerizer, - update(_, Resources(offer.resources()))) + update(_, Resources(offer.resources()), _)) .WillOnce(DoAll(FutureSatisfy(&update), Return(Nothing()))); @@ -5720,7 +5720,7 @@ TEST_F(MasterTest, TaskLabels) Future<Nothing> update; EXPECT_CALL(containerizer, - update(_, Resources(offers.get()[0].resources()))) + update(_, Resources(offers.get()[0].resources()), _)) .WillOnce(DoAll(FutureSatisfy(&update), Return(Nothing()))); @@ -6146,7 +6146,7 @@ TEST_F(MasterTest, TaskDiscoveryInfo) Future<Nothing> update; EXPECT_CALL(containerizer, - update(_, Resources(offers.get()[0].resources()))) + update(_, Resources(offers.get()[0].resources()), _)) .WillOnce(DoAll(FutureSatisfy(&update), Return(Nothing()))); @@ -10178,7 +10178,7 @@ TEST_P(MasterTestPrePostReservationRefinement, LaunchTask) .WillOnce(SendStatusUpdateFromTask(TASK_RUNNING)); Future<Nothing> update; - EXPECT_CALL(containerizer, update(_, inboundResources(offer.resources()))) + EXPECT_CALL(containerizer, update(_, inboundResources(offer.resources()), _)) .WillOnce(DoAll(FutureSatisfy(&update), Return(Nothing()))); Future<TaskStatus> status; @@ -10895,7 +10895,7 @@ TEST_F(MasterTest, LostTaskCleanup) { EXPECT_CALL(exec, launchTask(_, _)) .WillRepeatedly(SendStatusUpdateFromTask(TASK_RUNNING)); - EXPECT_CALL(containerizer, update(_, _)) + EXPECT_CALL(containerizer, update(_, _, _)) .WillRepeatedly(Return(Nothing())); Promise<Option<ContainerTermination>> hang; diff --git a/src/tests/mock_docker.hpp b/src/tests/mock_docker.hpp index 4a2266f..2d9a9c9 100644 --- a/src/tests/mock_docker.hpp +++ b/src/tests/mock_docker.hpp @@ -155,7 +155,7 @@ public: EXPECT_CALL(*this, launch(_, _, _, _)) .WillRepeatedly(Invoke(this, &MockDockerContainerizer::_launch)); - EXPECT_CALL(*this, update(_, _)) + EXPECT_CALL(*this, update(_, _, _)) .WillRepeatedly(Invoke(this, &MockDockerContainerizer::_update)); } @@ -167,11 +167,12 @@ public: const std::map<std::string, std::string>&, const Option<std::string>&)); - MOCK_METHOD2( + MOCK_METHOD3( update, process::Future<Nothing>( const ContainerID&, - const Resources&)); + const Resources&, + const google::protobuf::Map<std::string, Value::Scalar>&)); // Default 'launch' implementation (necessary because we can't just // use &slave::DockerContainerizer::launch with 'Invoke'). @@ -190,11 +191,13 @@ public: process::Future<Nothing> _update( const ContainerID& containerId, - const Resources& resources) + const Resources& resourceRequests, + const google::protobuf::Map<std::string, Value::Scalar>& resourceLimits) { return slave::DockerContainerizer::update( containerId, - resources); + resourceRequests, + resourceLimits); } }; diff --git a/src/tests/registrar_zookeeper_tests.cpp b/src/tests/registrar_zookeeper_tests.cpp index 9d3ea5f..bdcef13 100644 --- a/src/tests/registrar_zookeeper_tests.cpp +++ b/src/tests/registrar_zookeeper_tests.cpp @@ -96,7 +96,7 @@ TEST_F(RegistrarZooKeeperTest, TaskRunning) Future<Nothing> resourcesUpdated; EXPECT_CALL(containerizer, - update(_, Resources(offers.get()[0].resources()))) + update(_, Resources(offers.get()[0].resources()), _)) .WillOnce(DoAll(FutureSatisfy(&resourcesUpdated), Return(Nothing()))); diff --git a/src/tests/scheduler_tests.cpp b/src/tests/scheduler_tests.cpp index 299d3a0..7804ad0 100644 --- a/src/tests/scheduler_tests.cpp +++ b/src/tests/scheduler_tests.cpp @@ -510,7 +510,7 @@ TEST_P(SchedulerTest, TaskRunning) .WillOnce(FutureArg<1>(&statusUpdate)); Future<Nothing> update; - EXPECT_CALL(containerizer, update(_, _)) + EXPECT_CALL(containerizer, update(_, _, _)) .WillOnce(DoAll(FutureSatisfy(&update), Return(Nothing()))) .WillRepeatedly(Return(Future<Nothing>())); // Ignore subsequent calls. diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp index ff0dec6..6b264d0 100644 --- a/src/tests/slave_tests.cpp +++ b/src/tests/slave_tests.cpp @@ -3129,7 +3129,7 @@ TEST_F(SlaveTest, TerminalTaskContainerizerUpdateFailsWithLost) EXPECT_EQ(TASK_RUNNING, status2->state()); // Set up the containerizer so the next update() will fail. - EXPECT_CALL(containerizer, update(_, _)) + EXPECT_CALL(containerizer, update(_, _, _)) .WillOnce(Return(Failure("update() failed"))) .WillRepeatedly(Return(Nothing())); @@ -3242,7 +3242,7 @@ TEST_F(SlaveTest, TerminalTaskContainerizerUpdateFailsWithGone) EXPECT_EQ(TASK_RUNNING, status2->state()); // Set up the containerizer so the next update() will fail. - EXPECT_CALL(containerizer, update(_, _)) + EXPECT_CALL(containerizer, update(_, _, _)) .WillOnce(Return(Failure("update() failed"))) .WillRepeatedly(Return(Nothing())); @@ -3311,7 +3311,7 @@ TEST_F(SlaveTest, ContainerUpdatedBeforeTaskReachesExecutor) // sent to the executor. testing::Sequence sequence; - EXPECT_CALL(containerizer, update(_, _)) + EXPECT_CALL(containerizer, update(_, _, _)) .InSequence(sequence) .WillOnce(Return(Nothing())); @@ -3369,7 +3369,7 @@ TEST_F(SlaveTest, TaskLaunchContainerizerUpdateFails) .Times(AtMost(1)); // Set up the containerizer so update() will fail. - EXPECT_CALL(containerizer, update(_, _)) + EXPECT_CALL(containerizer, update(_, _, _)) .WillOnce(Return(Failure("update() failed"))) .WillRepeatedly(Return(Nothing())); @@ -7079,7 +7079,7 @@ TEST_F(SlaveTest, TaskLabels) Future<Nothing> update; EXPECT_CALL(containerizer, - update(_, Resources(offers.get()[0].resources()))) + update(_, Resources(offers.get()[0].resources()), _)) .WillOnce(DoAll(FutureSatisfy(&update), Return(Nothing()))); @@ -12380,7 +12380,7 @@ TEST_F(SlaveTest, DrainAgentKillsQueuedTask) FutureSatisfy(&launched), Return(launchResult.future()))); - EXPECT_CALL(mockContainerizer, update(_, _)) + EXPECT_CALL(mockContainerizer, update(_, _, _)) .WillOnce(Return(Nothing())); mesos.send(
