You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ch...@apache.org on 2018/09/22 06:34:04 UTC
[mesos] branch master updated (3eec390 -> 454a16a)
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git.
from 3eec390 Fixed couple of typos in the allocator.
new 0581437 Removed unnecessary failure handling in agent HTTP API handlers.
new 1adceaa Performed RP-specific validations when adding/updating RP configs.
new ae41c14 Removed `ROOT` requirements for `AgentResourceProviderConfigApiTest`.
new c174b9d Set master/agent flags in `AgentResourceProviderConfigApiTest` fixture.
new 1eeeadb Set master/agent flags in `StorageLocalResourceProviderTest` fixture.
new a31509e Added unit tests for adding/updating invalid resource provider configs.
new 88b28f2 Moved the container ID prefix generation to `LocalResourceProvider`.
new 6366b5c Cleaned up residual containers when removing resource provider configs.
new 678fa8b Tested container cleanup in `AgentResourceProviderConfigApiTest.Remove`.
new 454a16a Added MESOS-9228 to the 1.7.1 CHANGELOG.
The 10 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
CHANGELOG | 1 +
src/resource_provider/daemon.cpp | 254 ++++++++++--
src/resource_provider/local.cpp | 73 ++--
src/resource_provider/local.hpp | 10 +-
src/resource_provider/storage/provider.cpp | 67 ++--
src/resource_provider/storage/provider.hpp | 9 +-
src/slave/http.cpp | 109 ++----
.../agent_resource_provider_config_api_tests.cpp | 433 ++++++++++++++-------
.../storage_local_resource_provider_tests.cpp | 112 ++----
9 files changed, 665 insertions(+), 403 deletions(-)
[mesos] 03/10: Removed `ROOT` requirements for
`AgentResourceProviderConfigApiTest`.
Posted by ch...@apache.org.
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 ae41c14e47dd09db82fb237a9987fde8e100c8be
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Tue Sep 18 11:54:29 2018 -0700
Removed `ROOT` requirements for `AgentResourceProviderConfigApiTest`.
These tests required `ROOT` in order to use `filesystem/linux`
isolation this is not a requirement anymore so we can run the tests in
general. These tests appear to be able to run in parallel as well.
We also changed the `AddConflict` test a bit to make it more robust.
Review: https://reviews.apache.org/r/68757
---
.../agent_resource_provider_config_api_tests.cpp | 55 +++++++++++-----------
1 file changed, 28 insertions(+), 27 deletions(-)
diff --git a/src/tests/agent_resource_provider_config_api_tests.cpp b/src/tests/agent_resource_provider_config_api_tests.cpp
index e6a68ba..d4a8cae 100644
--- a/src/tests/agent_resource_provider_config_api_tests.cpp
+++ b/src/tests/agent_resource_provider_config_api_tests.cpp
@@ -129,7 +129,23 @@ public:
"--volumes=%s",
"--work_dir=%s"
]
- }
+ },
+ "resources": [
+ {
+ "name": "cpus",
+ "type": "SCALAR",
+ "scalar": {
+ "value": 0.1
+ }
+ },
+ {
+ "name": "mem",
+ "type": "SCALAR",
+ "scalar": {
+ "value": 1024
+ }
+ }
+ ]
}
]
}
@@ -282,7 +298,7 @@ INSTANTIATE_TEST_CASE_P(
// This test adds a new resource provider config on the fly.
-TEST_P(AgentResourceProviderConfigApiTest, ROOT_Add)
+TEST_P(AgentResourceProviderConfigApiTest, Add)
{
const ContentType contentType = GetParam();
@@ -295,8 +311,6 @@ TEST_P(AgentResourceProviderConfigApiTest, ROOT_Add)
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.isolation = "filesystem/linux";
-
slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
@@ -366,7 +380,7 @@ TEST_P(AgentResourceProviderConfigApiTest, ROOT_Add)
// This test checks that adding a resource provider config that is identical to
// an existing one is allowed due to idempotency.
-TEST_P(AgentResourceProviderConfigApiTest, ROOT_IdempotentAdd)
+TEST_P(AgentResourceProviderConfigApiTest, IdempotentAdd)
{
const ContentType contentType = GetParam();
@@ -378,8 +392,6 @@ TEST_P(AgentResourceProviderConfigApiTest, ROOT_IdempotentAdd)
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.isolation = "filesystem/linux";
-
slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
// Generate a pre-existing config.
@@ -431,7 +443,7 @@ TEST_P(AgentResourceProviderConfigApiTest, ROOT_IdempotentAdd)
// This test checks that adding a resource provider config that already
// exists is not allowed.
-TEST_P(AgentResourceProviderConfigApiTest, ROOT_AddConflict)
+TEST_P(AgentResourceProviderConfigApiTest, AddConflict)
{
const ContentType contentType = GetParam();
@@ -444,15 +456,12 @@ TEST_P(AgentResourceProviderConfigApiTest, ROOT_AddConflict)
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.isolation = "filesystem/linux";
-
slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
// Generate a pre-existing config.
const string configPath = path::join(resourceProviderConfigDir, "test.json");
- ASSERT_SOME(os::write(
- configPath,
- stringify(JSON::protobuf(createResourceProviderInfo("volume1:4GB")))));
+ ResourceProviderInfo oldInfo = createResourceProviderInfo("volume1:4GB");
+ ASSERT_SOME(os::write(configPath, stringify(JSON::protobuf(oldInfo))));
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _);
@@ -462,11 +471,11 @@ TEST_P(AgentResourceProviderConfigApiTest, ROOT_AddConflict)
AWAIT_READY(slaveRegisteredMessage);
- ResourceProviderInfo info = createResourceProviderInfo("volume1:2GB");
+ ResourceProviderInfo newInfo = createResourceProviderInfo("volume1:2GB");
AWAIT_EXPECT_RESPONSE_STATUS_EQ(
http::Conflict().status,
- addResourceProviderConfig(slave.get()->pid, contentType, info));
+ addResourceProviderConfig(slave.get()->pid, contentType, newInfo));
// Check that no new config is created, and the existing one is not
// overwritten.
@@ -485,12 +494,12 @@ TEST_P(AgentResourceProviderConfigApiTest, ROOT_AddConflict)
Try<ResourceProviderInfo> _info =
::protobuf::parse<ResourceProviderInfo>(json.get());
ASSERT_SOME(_info);
- EXPECT_NE(_info.get(), info);
+ EXPECT_EQ(_info.get(), oldInfo);
}
// This test updates an existing resource provider config on the fly.
-TEST_P(AgentResourceProviderConfigApiTest, ROOT_Update)
+TEST_P(AgentResourceProviderConfigApiTest, Update)
{
const ContentType contentType = GetParam();
@@ -503,8 +512,6 @@ TEST_P(AgentResourceProviderConfigApiTest, ROOT_Update)
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.isolation = "filesystem/linux";
-
slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
// Generate a pre-existing config.
@@ -602,7 +609,7 @@ TEST_P(AgentResourceProviderConfigApiTest, ROOT_Update)
// This test checks that updating an existing resource provider config with an
// identical one will not relaunch the resource provider due to idempotency.
-TEST_P(AgentResourceProviderConfigApiTest, ROOT_IdempotentUpdate)
+TEST_P(AgentResourceProviderConfigApiTest, IdempotentUpdate)
{
const ContentType contentType = GetParam();
@@ -614,8 +621,6 @@ TEST_P(AgentResourceProviderConfigApiTest, ROOT_IdempotentUpdate)
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.isolation = "filesystem/linux";
-
slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
// Generate a pre-existing config.
@@ -680,7 +685,6 @@ TEST_P(AgentResourceProviderConfigApiTest, UpdateNotFound)
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
-
slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
@@ -706,7 +710,7 @@ TEST_P(AgentResourceProviderConfigApiTest, UpdateNotFound)
// This test removes an existing resource provider config on the fly.
-TEST_P(AgentResourceProviderConfigApiTest, ROOT_Remove)
+TEST_P(AgentResourceProviderConfigApiTest, Remove)
{
const ContentType contentType = GetParam();
@@ -719,8 +723,6 @@ TEST_P(AgentResourceProviderConfigApiTest, ROOT_Remove)
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.isolation = "filesystem/linux";
-
slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
// Generate a pre-existing config.
@@ -801,7 +803,6 @@ TEST_P(AgentResourceProviderConfigApiTest, IdempotentRemove)
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
-
slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
[mesos] 09/10: Tested container cleanup in
`AgentResourceProviderConfigApiTest.Remove`.
Posted by ch...@apache.org.
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 678fa8b44bc9c09f5f9908a3a4511f7195150d7b
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Tue Sep 18 21:40:35 2018 -0700
Tested container cleanup in `AgentResourceProviderConfigApiTest.Remove`.
Review: https://reviews.apache.org/r/68762
---
.../agent_resource_provider_config_api_tests.cpp | 29 +++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/src/tests/agent_resource_provider_config_api_tests.cpp b/src/tests/agent_resource_provider_config_api_tests.cpp
index 891678f..15c83b3 100644
--- a/src/tests/agent_resource_provider_config_api_tests.cpp
+++ b/src/tests/agent_resource_provider_config_api_tests.cpp
@@ -37,6 +37,10 @@
#include "slave/slave.hpp"
+#include "slave/containerizer/fetcher.hpp"
+
+#include "slave/containerizer/mesos/containerizer.hpp"
+
#include "tests/flags.hpp"
#include "tests/mesos.hpp"
@@ -867,10 +871,21 @@ TEST_P(AgentResourceProviderConfigApiTest, Remove)
ResourceProviderInfo info = createResourceProviderInfo("volume1:4GB");
ASSERT_SOME(os::write(configPath, stringify(JSON::protobuf(info))));
+ slave::Flags slaveFlags = CreateSlaveFlags();
+
+ slave::Fetcher fetcher(slaveFlags);
+
+ Try<slave::MesosContainerizer*> _containerizer =
+ slave::MesosContainerizer::create(slaveFlags, false, &fetcher);
+ ASSERT_SOME(_containerizer);
+
+ Owned<slave::MesosContainerizer> containerizer(_containerizer.get());
+
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _);
- Try<Owned<cluster::Slave>> slave = StartSlave(detector.get());
+ Try<Owned<cluster::Slave>> slave =
+ StartSlave(detector.get(), containerizer.get(), slaveFlags);
ASSERT_SOME(slave);
AWAIT_READY(slaveRegisteredMessage);
@@ -907,6 +922,15 @@ TEST_P(AgentResourceProviderConfigApiTest, Remove)
AWAIT_READY(oldOffers);
ASSERT_FALSE(oldOffers->empty());
+ Future<hashset<ContainerID>> pluginContainers = containerizer->containers();
+
+ // Check that there is one plugin container running.
+ AWAIT_READY(pluginContainers);
+ ASSERT_EQ(1u, pluginContainers->size());
+
+ Future<Option<mesos::slave::ContainerTermination>> pluginTermination =
+ containerizer->wait(*pluginContainers->begin());
+
Future<OfferID> rescinded;
EXPECT_CALL(sched, offerRescinded(&driver, oldOffers->at(0).id()))
@@ -922,6 +946,9 @@ TEST_P(AgentResourceProviderConfigApiTest, Remove)
// Wait for the old offer to be rescinded.
AWAIT_READY(rescinded);
+
+ // Check that the plugin container has been destroyed.
+ AWAIT_READY(pluginTermination);
}
[mesos] 06/10: Added unit tests for adding/updating invalid
resource provider configs.
Posted by ch...@apache.org.
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 a31509e4206edc12fbc7ed6a8e8f87c36e02f34d
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Tue Sep 18 13:25:06 2018 -0700
Added unit tests for adding/updating invalid resource provider configs.
Review: https://reviews.apache.org/r/68758
---
.../agent_resource_provider_config_api_tests.cpp | 195 ++++++++++++++++++---
1 file changed, 172 insertions(+), 23 deletions(-)
diff --git a/src/tests/agent_resource_provider_config_api_tests.cpp b/src/tests/agent_resource_provider_config_api_tests.cpp
index 0c1ad4c..891678f 100644
--- a/src/tests/agent_resource_provider_config_api_tests.cpp
+++ b/src/tests/agent_resource_provider_config_api_tests.cpp
@@ -20,7 +20,7 @@
#include <process/gtest.hpp>
#include <process/gmock.hpp>
-#include <stout/fs.hpp>
+#include <stout/os.hpp>
#include <stout/json.hpp>
#include <stout/protobuf.hpp>
#include <stout/stringify.hpp>
@@ -33,6 +33,8 @@
#include "internal/evolve.hpp"
+#include "resource_provider/local.hpp"
+
#include "slave/slave.hpp"
#include "tests/flags.hpp"
@@ -235,6 +237,30 @@ public:
return info.get();
}
+ list<string> getResourceProviderConfigPaths()
+ {
+ list<string> result;
+
+ // This extra closure is necessary in order to use `ASSERT_*`, as
+ // these macros require a void return type.
+ [&] {
+ Try<list<string>> configFiles = os::ls(resourceProviderConfigDir.get());
+ ASSERT_SOME(configFiles);
+
+ foreach (const string& configFile, configFiles.get()) {
+ const string configPath =
+ path::join(resourceProviderConfigDir.get(), configFile);
+
+ // We only keep regular files to filter out the staging directory.
+ if (os::stat::isfile(configPath)) {
+ result.push_back(configPath);
+ }
+ }
+ }();
+
+ return result;
+ }
+
Future<http::Response> addResourceProviderConfig(
const PID<Slave>& pid,
const ContentType& contentType,
@@ -366,12 +392,10 @@ TEST_P(AgentResourceProviderConfigApiTest, Add)
addResourceProviderConfig(slave.get()->pid, contentType, info));
// Check that a new config file is created.
- Try<list<string>> configPaths =
- fs::list(path::join(resourceProviderConfigDir.get(), "*"));
- ASSERT_SOME(configPaths);
- EXPECT_EQ(1u, configPaths->size());
+ list<string> configPaths = getResourceProviderConfigPaths();
+ EXPECT_EQ(1u, configPaths.size());
- Try<string> read = os::read(configPaths->back());
+ Try<string> read = os::read(configPaths.back());
ASSERT_SOME(read);
Try<JSON::Object> json = JSON::parse<JSON::Object>(read.get());
@@ -451,7 +475,7 @@ TEST_P(AgentResourceProviderConfigApiTest, IdempotentAdd)
// This test checks that adding a resource provider config that already
-// exists is not allowed.
+// exists is rejected.
TEST_P(AgentResourceProviderConfigApiTest, AddConflict)
{
const ContentType contentType = GetParam();
@@ -483,11 +507,9 @@ TEST_P(AgentResourceProviderConfigApiTest, AddConflict)
// Check that no new config is created, and the existing one is not
// overwritten.
- Try<list<string>> configPaths =
- fs::list(path::join(resourceProviderConfigDir.get(), "*"));
- ASSERT_SOME(configPaths);
- EXPECT_EQ(1u, configPaths->size());
- EXPECT_EQ(configPath, configPaths->back());
+ list<string> configPaths = getResourceProviderConfigPaths();
+ EXPECT_EQ(1u, configPaths.size());
+ EXPECT_EQ(configPath, configPaths.back());
Try<string> read = os::read(configPath);
ASSERT_SOME(read);
@@ -502,6 +524,46 @@ TEST_P(AgentResourceProviderConfigApiTest, AddConflict)
}
+// This test checks that adding an invalid resource provider config is rejected.
+TEST_P(AgentResourceProviderConfigApiTest, AddInvalid)
+{
+ const ContentType contentType = GetParam();
+
+ master::Flags masterFlags = CreateMasterFlags();
+ masterFlags.allocation_interval = Milliseconds(50);
+
+ Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ ASSERT_SOME(master);
+
+ Owned<MasterDetector> detector = master.get()->createDetector();
+
+ slave::Flags slaveFlags = CreateSlaveFlags();
+ slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
+
+ Future<SlaveRegisteredMessage> slaveRegisteredMessage =
+ FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _);
+
+ Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), slaveFlags);
+ ASSERT_SOME(slave);
+
+ AWAIT_READY(slaveRegisteredMessage);
+
+ // Generate an invalid config by clearing the `storage` field, which is
+ // required by the storage local resource provider.
+ ResourceProviderInfo info = createResourceProviderInfo("volume1:4GB");
+ info.clear_storage();
+ ASSERT_SOME(LocalResourceProvider::validate(info));
+
+ AWAIT_EXPECT_RESPONSE_STATUS_EQ(
+ http::BadRequest().status,
+ updateResourceProviderConfig(slave.get()->pid, contentType, info));
+
+ // Check that no new config is created.
+ list<string> configPaths = getResourceProviderConfigPaths();
+ EXPECT_TRUE(configPaths.empty());
+}
+
+
// This test updates an existing resource provider config on the fly.
TEST_P(AgentResourceProviderConfigApiTest, Update)
{
@@ -577,11 +639,9 @@ TEST_P(AgentResourceProviderConfigApiTest, Update)
updateResourceProviderConfig(slave.get()->pid, contentType, info));
// Check that no new config is created, and the existing one is overwritten.
- Try<list<string>> configPaths =
- fs::list(path::join(resourceProviderConfigDir.get(), "*"));
- ASSERT_SOME(configPaths);
- EXPECT_EQ(1u, configPaths->size());
- EXPECT_EQ(configPath, configPaths->back());
+ list<string> configPaths = getResourceProviderConfigPaths();
+ EXPECT_EQ(1u, configPaths.size());
+ EXPECT_EQ(configPath, configPaths.back());
Try<string> read = os::read(configPath);
ASSERT_SOME(read);
@@ -669,8 +729,8 @@ TEST_P(AgentResourceProviderConfigApiTest, IdempotentUpdate)
}
-// This test checks that updating a nonexistent resource provider config
-// is not allowed.
+// This test checks that updating a nonexistent resource provider config is
+// rejected.
TEST_P(AgentResourceProviderConfigApiTest, UpdateNotFound)
{
const ContentType contentType = GetParam();
@@ -695,10 +755,99 @@ TEST_P(AgentResourceProviderConfigApiTest, UpdateNotFound)
updateResourceProviderConfig(slave.get()->pid, contentType, info));
// Check that no new config is created.
- Try<list<string>> configPaths =
- fs::list(path::join(resourceProviderConfigDir.get(), "*"));
- ASSERT_SOME(configPaths);
- EXPECT_TRUE(configPaths->empty());
+ list<string> configPaths = getResourceProviderConfigPaths();
+ EXPECT_TRUE(configPaths.empty());
+}
+
+
+// This test checks that updating with an invalid resource provider config is
+// rejected.
+TEST_P(AgentResourceProviderConfigApiTest, UpdateInvalid)
+{
+ const ContentType contentType = GetParam();
+
+ master::Flags masterFlags = CreateMasterFlags();
+ masterFlags.allocation_interval = Milliseconds(50);
+
+ Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ ASSERT_SOME(master);
+
+ Owned<MasterDetector> detector = master.get()->createDetector();
+
+ slave::Flags slaveFlags = CreateSlaveFlags();
+ slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
+
+ // Generate a pre-existing config.
+ const string configPath =
+ path::join(resourceProviderConfigDir.get(), "test.json");
+ ResourceProviderInfo oldInfo = createResourceProviderInfo("volume1:4GB");
+ ASSERT_SOME(os::write(configPath, stringify(JSON::protobuf(oldInfo))));
+
+ Future<SlaveRegisteredMessage> slaveRegisteredMessage =
+ FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _);
+
+ Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), slaveFlags);
+ ASSERT_SOME(slave);
+
+ AWAIT_READY(slaveRegisteredMessage);
+
+ // Register a framework to wait for an offer having the provider resource.
+ FrameworkInfo framework = DEFAULT_FRAMEWORK_INFO;
+ framework.set_roles(0, "storage");
+
+ MockScheduler sched;
+ MesosSchedulerDriver driver(
+ &sched, framework, master.get()->pid, DEFAULT_CREDENTIAL);
+
+ EXPECT_CALL(sched, registered(&driver, _, _));
+
+ // We use the following filter to filter offers that do not have
+ // wanted resources for 365 days (the maximum).
+ Filters declineFilters;
+ declineFilters.set_refuse_seconds(Days(365).secs());
+
+ // Decline offers that contain only the agent's default resources.
+ EXPECT_CALL(sched, resourceOffers(&driver, _))
+ .WillRepeatedly(DeclineOffers(declineFilters));
+
+ Future<vector<Offer>> oldOffers;
+
+ EXPECT_CALL(sched, resourceOffers(&driver, OffersHaveAnyResource(
+ std::bind(&Resources::hasResourceProvider, lambda::_1))))
+ .WillOnce(FutureArg<1>(&oldOffers));
+
+ driver.start();
+
+ // Wait for an offer having the old provider resource.
+ AWAIT_READY(oldOffers);
+ ASSERT_FALSE(oldOffers->empty());
+
+ // Generate an invalid config by clearing the `storage` field, which is
+ // required by the storage local resource provider.
+ ResourceProviderInfo newInfo = oldInfo;
+ newInfo.clear_storage();
+ ASSERT_SOME(LocalResourceProvider::validate(newInfo));
+
+ AWAIT_EXPECT_RESPONSE_STATUS_EQ(
+ http::BadRequest().status,
+ updateResourceProviderConfig(slave.get()->pid, contentType, newInfo));
+
+ // Check that no new config is created, and the existing one is not
+ // overwritten.
+ list<string> configPaths = getResourceProviderConfigPaths();
+ EXPECT_EQ(1u, configPaths.size());
+ EXPECT_EQ(configPath, configPaths.back());
+
+ Try<string> read = os::read(configPath);
+ ASSERT_SOME(read);
+
+ Try<JSON::Object> json = JSON::parse<JSON::Object>(read.get());
+ ASSERT_SOME(json);
+
+ Try<ResourceProviderInfo> _info =
+ ::protobuf::parse<ResourceProviderInfo>(json.get());
+ ASSERT_SOME(_info);
+ EXPECT_EQ(_info.get(), oldInfo);
}
[mesos] 08/10: Cleaned up residual containers when removing
resource provider configs.
Posted by ch...@apache.org.
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 6366b5c1e5e60dfda5ca0368d6a22da998f0cfa4
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Thu Sep 20 15:06:51 2018 -0700
Cleaned up residual containers when removing resource provider configs.
When processing `REMOVE_RESOURCE_PROVIDER_CONFIG`, the local resource
provider daemon now performs a best-effort cleanup by killing all
standalone containers prefixed by the 'cid_prefix' of the resource
provider. During the cleanup, no resource provider config with the same
type and name can be added.
Review: https://reviews.apache.org/r/68763
---
src/resource_provider/daemon.cpp | 239 +++++++++++++++++++++++++++++++++++----
1 file changed, 220 insertions(+), 19 deletions(-)
diff --git a/src/resource_provider/daemon.cpp b/src/resource_provider/daemon.cpp
index f1a941c..2fd82ad 100644
--- a/src/resource_provider/daemon.cpp
+++ b/src/resource_provider/daemon.cpp
@@ -22,6 +22,7 @@
#include <mesos/type_utils.hpp>
+#include <process/collect.hpp>
#include <process/defer.hpp>
#include <process/dispatch.hpp>
#include <process/id.hpp>
@@ -35,25 +36,31 @@
#include <stout/os.hpp>
#include <stout/path.hpp>
#include <stout/protobuf.hpp>
+#include <stout/strings.hpp>
#include <stout/try.hpp>
+#include "common/http.hpp"
#include "common/validation.hpp"
+#include "internal/devolve.hpp"
+#include "internal/evolve.hpp"
+
#include "resource_provider/local.hpp"
namespace http = process::http;
using std::list;
using std::string;
+using std::vector;
+using process::await;
+using process::defer;
+using process::dispatch;
using process::Failure;
using process::Future;
using process::Owned;
using process::Process;
using process::ProcessBase;
-
-using process::defer;
-using process::dispatch;
using process::spawn;
using process::terminate;
using process::wait;
@@ -109,11 +116,17 @@ private:
const string path;
ResourceProviderInfo info;
+ Option<string> authToken;
// The `version` is used to check if `provider` holds a resource
// provider instance that is in sync with the current config.
id::UUID version;
Owned<LocalResourceProvider> provider;
+
+ // If set, it means that the resource provider is being removed, and the
+ // future would be completed once the removal is done. Note that this object
+ // will be destructed as soon as the future is ready.
+ Option<Future<Nothing>> removing;
};
Try<Nothing> load(const string& path);
@@ -133,6 +146,10 @@ private:
Future<Option<string>> generateAuthToken(const ResourceProviderInfo& info);
+ Future<Nothing> cleanupContainers(
+ const ResourceProviderInfo& info,
+ const Option<string>& authToken);
+
const http::URL url;
const string workDir;
const Option<string> configDir;
@@ -161,7 +178,13 @@ void LocalResourceProviderDaemonProcess::start(const SlaveID& _slaveId)
slaveId = _slaveId;
foreachkey (const string& type, providers) {
- foreachkey (const string& name, providers[type]) {
+ foreachpair (const string& name,
+ const ProviderData& data,
+ providers[type]) {
+ if (data.removing.isSome()) {
+ continue;
+ }
+
auto error = [=](const string& message) {
LOG(ERROR) << "Failed to launch resource provider with type '" << type
<< "' and name '" << name << "': " << message;
@@ -186,7 +209,15 @@ Future<bool> LocalResourceProviderDaemonProcess::add(
// Return true if the info has been added for idempotency.
if (providers[info.type()].contains(info.name())) {
- return providers[info.type()].at(info.name()).info == info;
+ const ProviderData& data = providers[info.type()].at(info.name());
+
+ if (data.removing.isSome()) {
+ return Failure(
+ "Failed to add resource provider with type '" + info.type() +
+ "' and name '" + info.name() + "' as a removal is still in progress");
+ }
+
+ return data.info == info;
}
// Generate a filename for the config.
@@ -239,6 +270,12 @@ Future<bool> LocalResourceProviderDaemonProcess::update(
ProviderData& data = providers[info.type()].at(info.name());
+ if (data.removing.isSome()) {
+ return Failure(
+ "Failed to update resource provider with type '" + info.type() +
+ "' and name '" + info.name() + "' as a removal is still in progress");
+ }
+
// Return true if the info has been updated for idempotency.
if (data.info == info) {
return true;
@@ -285,19 +322,39 @@ Future<Nothing> LocalResourceProviderDaemonProcess::remove(
return Nothing();
}
- const string path = providers[type].at(name).path;
+ ProviderData& data = providers[type].at(name);
- Try<Nothing> rm = os::rm(path);
- if (rm.isError()) {
- return Failure(
- "Failed to remove config file '" + path + "': " + rm.error());
+ // Return the same future if it is being removed for idempotency.
+ if (data.removing.isSome() && data.removing->isPending()) {
+ return data.removing.get();
}
- // Removing the provider data from `providers` will cause the resource
- // provider to be destructed.
- providers[type].erase(name);
+ // Destruct the resource provider instance to stop its container daemons, then
+ // do a best-effort cleanup of the standalone containers launched by the
+ // removed resource provider.
+ // TODO(chhsiao): This is not ideal since the daemon does not know how to
+ // perform resource-provider-specific cleanups. We should refactor this into a
+ // `LocalResourceProvider::stop` virtual function. However this also means
+ // that we need to ensure that we must have a resource provider instance with
+ // a running actor to invoke `stop`. Given that `launch` is asynchronous, we
+ // need to carefully redesign the state machine, and the semantics of the
+ // `{ADD,UPDATE,REMOVE}_RESOURCE_PROVIDER_CONFIG` API calls might be affected.
+ // In addition, we should consider how to reconcile orphaned containers.
+ data.provider.reset();
+ data.removing = cleanupContainers(data.info, data.authToken)
+ .then(defer(self(), [this, type, name]() -> Future<Nothing> {
+ Try<Nothing> rm = os::rm(providers[type].at(name).path);
+ if (rm.isError()) {
+ return Failure(
+ "Failed to remove config file '" + providers[type].at(name).path +
+ "': " + rm.error());
+ }
- return Nothing();
+ providers[type].erase(name);
+ return Nothing();
+ }));
+
+ return data.removing.get();
}
@@ -420,13 +477,10 @@ Future<Nothing> LocalResourceProviderDaemonProcess::launch(
const string& name)
{
CHECK_SOME(slaveId);
-
- // If the resource provider config is removed, nothing needs to be done.
- if (!providers[type].contains(name)) {
- return Nothing();
- }
+ CHECK(providers[type].contains(name));
ProviderData& data = providers[type].at(name);
+ CHECK(data.removing.isNone());
// Destruct the previous resource provider (which will synchronously
// terminate its actor and driver) if there is one.
@@ -450,6 +504,11 @@ Future<Nothing> LocalResourceProviderDaemonProcess::_launch(
ProviderData& data = providers[type].at(name);
+ // If the resource provider config is being removed, nothing needs to be done.
+ if (data.removing.isSome()) {
+ return Nothing();
+ }
+
// If there is a version mismatch, abort the launch sequence since
// `authToken` might be outdated. The callback updating the version
// should have dispatched another launch sequence.
@@ -466,6 +525,7 @@ Future<Nothing> LocalResourceProviderDaemonProcess::_launch(
"' and name '" + name + "': " + provider.error());
}
+ data.authToken = authToken;
data.provider = std::move(provider.get());
return Nothing();
@@ -501,6 +561,147 @@ Future<Option<string>> LocalResourceProviderDaemonProcess::generateAuthToken(
}
+Future<Nothing> LocalResourceProviderDaemonProcess::cleanupContainers(
+ const ResourceProviderInfo& info,
+ const Option<string>& authToken)
+{
+ const Principal principal = LocalResourceProvider::principal(info);
+ CHECK(principal.claims.contains("cid_prefix"));
+
+ const string& cidPrefix = principal.claims.at("cid_prefix");
+
+ LOG(INFO) << "Cleaning up containers prefixed by '" << cidPrefix << "'";
+
+ // TODO(chhsiao): Consider using a more reliable way to get the v1 endpoint.
+ http::URL agentUrl = url;
+ agentUrl.path = Path(url.path).dirname();
+
+ http::Headers headers{{"Accept", stringify(ContentType::PROTOBUF)}};
+ if (authToken.isSome()) {
+ headers["Authorization"] = "Bearer " + authToken.get();
+ }
+
+ agent::Call call;
+ call.set_type(agent::Call::GET_CONTAINERS);
+ call.mutable_get_containers()->set_show_nested(false);
+ call.mutable_get_containers()->set_show_standalone(true);
+
+ return http::post(
+ agentUrl,
+ headers,
+ serialize(ContentType::PROTOBUF, evolve(call)),
+ stringify(ContentType::PROTOBUF))
+ .then(defer(self(), [=](
+ const http::Response& httpResponse) -> Future<Nothing> {
+ if (httpResponse.status != http::OK().status) {
+ return Failure(
+ "Failed to get containers: Unexpected response '" +
+ httpResponse.status + "' (" + httpResponse.body + ")");
+ }
+
+ Try<v1::agent::Response> v1Response = deserialize<v1::agent::Response>(
+ ContentType::PROTOBUF, httpResponse.body);
+ if (v1Response.isError()) {
+ return Failure("Failed to get containers: " + v1Response.error());
+ }
+
+ vector<Future<Nothing>> futures;
+
+ agent::Response response = devolve(v1Response.get());
+ foreach (const agent::Response::GetContainers::Container& container,
+ response.get_containers().containers()) {
+ const ContainerID& containerId = container.container_id();
+
+ if (!strings::startsWith(containerId.value(), cidPrefix)) {
+ continue;
+ }
+
+ // NOTE: We skip containers that are not actually running by checking
+ // their `executor_pid`s to avoid `ESRCH` errors or killing an arbitrary
+ // process. But we might skip the ones that are being launched as well.
+ if (!container.has_container_status() ||
+ !container.container_status().has_executor_pid()) {
+ LOG(WARNING)
+ << "Skipped killing container '" << containerId
+ << "' because it is not running";
+
+ continue;
+ }
+
+ agent::Call call;
+ call.set_type(agent::Call::KILL_CONTAINER);
+ call.mutable_kill_container()->mutable_container_id()
+ ->CopyFrom(containerId);
+
+ LOG(INFO) << "Killing container '" << containerId << "'";
+
+ futures.push_back(http::post(
+ agentUrl,
+ headers,
+ serialize(ContentType::PROTOBUF, evolve(call)),
+ stringify(ContentType::PROTOBUF))
+ .then(defer(self(), [=](
+ const http::Response& response) -> Future<Nothing> {
+ if (response.status == http::NotFound().status) {
+ LOG(WARNING)
+ << "Skipped waiting for container '" << containerId
+ << "' because it no longer exists";
+
+ return Nothing();
+ }
+
+ if (response.status != http::OK().status) {
+ return Failure(
+ "Failed to kill container '" + stringify(containerId) +
+ "': Unexpected response '" + response.status + "' (" +
+ response.body + ")");
+ }
+
+ LOG(INFO) << "Waiting for container '" << containerId << "'";
+
+ agent::Call call;
+ call.set_type(agent::Call::WAIT_CONTAINER);
+ call.mutable_wait_container()->mutable_container_id()
+ ->CopyFrom(containerId);
+
+ return http::post(
+ agentUrl,
+ headers,
+ serialize(ContentType::PROTOBUF, evolve(call)),
+ stringify(ContentType::PROTOBUF))
+ .then([containerId](
+ const http::Response& response) -> Future<Nothing> {
+ if (response.status != http::OK().status &&
+ response.status != http::NotFound().status) {
+ return Failure(
+ "Failed to wait for container '" +
+ stringify(containerId) + "': Unexpected response '" +
+ response.status + "' (" + response.body + ")");
+ }
+
+ return Nothing();
+ });
+ })));
+ }
+
+ // We use await here to do a best-effort cleanup.
+ return await(futures)
+ .then([cidPrefix](
+ const vector<Future<Nothing>>& futures) -> Future<Nothing> {
+ foreach (const Future<Nothing>& future, futures) {
+ if (!future.isReady()) {
+ return Failure(
+ "Failed to clean up containers prefixed by '" + cidPrefix +
+ "': " + stringify(futures));
+ }
+ }
+
+ return Nothing();
+ });
+ }));
+}
+
+
Try<Owned<LocalResourceProviderDaemon>> LocalResourceProviderDaemon::create(
const http::URL& url,
const slave::Flags& flags,
[mesos] 02/10: Performed RP-specific validations when
adding/updating RP configs.
Posted by ch...@apache.org.
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 <ch...@mesosphere.io>
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) {
[mesos] 04/10: Set master/agent flags in
`AgentResourceProviderConfigApiTest` fixture.
Posted by ch...@apache.org.
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 c174b9d3a7eb842b6de66256f0373c5e12b00cce
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Wed Sep 19 18:07:50 2018 -0700
Set master/agent flags in `AgentResourceProviderConfigApiTest` fixture.
Review: https://reviews.apache.org/r/68777
---
.../agent_resource_provider_config_api_tests.cpp | 206 ++++++++++-----------
1 file changed, 94 insertions(+), 112 deletions(-)
diff --git a/src/tests/agent_resource_provider_config_api_tests.cpp b/src/tests/agent_resource_provider_config_api_tests.cpp
index d4a8cae..0c1ad4c 100644
--- a/src/tests/agent_resource_provider_config_api_tests.cpp
+++ b/src/tests/agent_resource_provider_config_api_tests.cpp
@@ -76,7 +76,66 @@ public:
resourceProviderConfigDir =
path::join(sandbox.get(), "resource_provider_configs");
- ASSERT_SOME(os::mkdir(resourceProviderConfigDir));
+ ASSERT_SOME(os::mkdir(resourceProviderConfigDir.get()));
+ }
+
+ void TearDown() override
+ {
+ foreach (const string& slaveWorkDir, slaveWorkDirs) {
+ // Clean up CSI endpoint directories if there is any.
+ const string csiRootDir = slave::paths::getCsiRootDir(slaveWorkDir);
+
+ Try<list<string>> csiContainerPaths =
+ csi::paths::getContainerPaths(csiRootDir, "*", "*");
+ ASSERT_SOME(csiContainerPaths);
+
+ foreach (const string& path, csiContainerPaths.get()) {
+ Try<csi::paths::ContainerPath> containerPath =
+ csi::paths::parseContainerPath(csiRootDir, path);
+ ASSERT_SOME(containerPath);
+
+ Result<string> endpointDir =
+ os::realpath(csi::paths::getEndpointDirSymlinkPath(
+ csiRootDir,
+ containerPath->type,
+ containerPath->name,
+ containerPath->containerId));
+
+ if (endpointDir.isSome()) {
+ ASSERT_SOME(os::rmdir(endpointDir.get()));
+ }
+ }
+ }
+
+ ContainerizerTest<slave::MesosContainerizer>::TearDown();
+ }
+
+ master::Flags CreateMasterFlags() override
+ {
+ master::Flags flags =
+ ContainerizerTest<slave::MesosContainerizer>::CreateMasterFlags();
+
+ // Use a small allocation interval to speed up the test. We do this instead
+ // of manipulating the clock because the storage local resource provider
+ // relies on a running clock to wait for the CSI plugin to be ready.
+ flags.allocation_interval = Milliseconds(50);
+
+ return flags;
+ }
+
+ slave::Flags CreateSlaveFlags() override
+ {
+ slave::Flags flags =
+ ContainerizerTest<slave::MesosContainerizer>::CreateSlaveFlags();
+
+ flags.resource_provider_config_dir = resourceProviderConfigDir;
+
+ // Store the agent work directory for cleaning up CSI endpoint
+ // directories during teardown.
+ // NOTE: DO NOT change the work directory afterward.
+ slaveWorkDirs.push_back(flags.work_dir);
+
+ return flags;
}
ResourceProviderInfo createResourceProviderInfo(const string& volumes)
@@ -85,7 +144,7 @@ public:
// This extra closure is necessary in order to use `ASSERT_*`, as
// these macros require a void return type.
- [&]() {
+ [&] {
// Randomize the plugin name so we get a clean work directory for
// each created config.
const string testCsiPluginName =
@@ -176,50 +235,6 @@ public:
return info.get();
}
- void TearDown() override
- {
- foreach (const string& slaveWorkDir, slaveWorkDirs) {
- // Clean up CSI endpoint directories if there is any.
- const string csiRootDir = slave::paths::getCsiRootDir(slaveWorkDir);
-
- Try<list<string>> csiContainerPaths =
- csi::paths::getContainerPaths(csiRootDir, "*", "*");
- ASSERT_SOME(csiContainerPaths);
-
- foreach (const string& path, csiContainerPaths.get()) {
- Try<csi::paths::ContainerPath> containerPath =
- csi::paths::parseContainerPath(csiRootDir, path);
- ASSERT_SOME(containerPath);
-
- Result<string> endpointDir =
- os::realpath(csi::paths::getEndpointDirSymlinkPath(
- csiRootDir,
- containerPath->type,
- containerPath->name,
- containerPath->containerId));
-
- if (endpointDir.isSome()) {
- ASSERT_SOME(os::rmdir(endpointDir.get()));
- }
- }
- }
-
- ContainerizerTest<slave::MesosContainerizer>::TearDown();
- }
-
- slave::Flags CreateSlaveFlags() override
- {
- slave::Flags flags =
- ContainerizerTest<slave::MesosContainerizer>::CreateSlaveFlags();
-
- // Store the agent work directory for cleaning up CSI endpoint
- // directories during teardown.
- // NOTE: DO NOT change the work directory afterward.
- slaveWorkDirs.push_back(flags.work_dir);
-
- return flags;
- }
-
Future<http::Response> addResourceProviderConfig(
const PID<Slave>& pid,
const ContentType& contentType,
@@ -286,7 +301,7 @@ public:
protected:
vector<string> slaveWorkDirs;
- string resourceProviderConfigDir;
+ Option<string> resourceProviderConfigDir;
};
@@ -302,21 +317,15 @@ TEST_P(AgentResourceProviderConfigApiTest, Add)
{
const ContentType contentType = GetParam();
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
- slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
-
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _);
- Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), slaveFlags);
+ Try<Owned<cluster::Slave>> slave = StartSlave(detector.get());
ASSERT_SOME(slave);
AWAIT_READY(slaveRegisteredMessage);
@@ -358,7 +367,7 @@ TEST_P(AgentResourceProviderConfigApiTest, Add)
// Check that a new config file is created.
Try<list<string>> configPaths =
- fs::list(path::join(resourceProviderConfigDir, "*"));
+ fs::list(path::join(resourceProviderConfigDir.get(), "*"));
ASSERT_SOME(configPaths);
EXPECT_EQ(1u, configPaths->size());
@@ -391,13 +400,13 @@ TEST_P(AgentResourceProviderConfigApiTest, IdempotentAdd)
Owned<MasterDetector> detector = master.get()->createDetector();
- slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
-
// Generate a pre-existing config.
- const string configPath = path::join(resourceProviderConfigDir, "test.json");
ResourceProviderInfo info = createResourceProviderInfo("volume1:4GB");
- ASSERT_SOME(os::write(configPath, stringify(JSON::protobuf(info))));
+ ASSERT_SOME(os::write(
+ path::join(resourceProviderConfigDir.get(), "test.json"),
+ stringify(JSON::protobuf(info))));
+
+ slave::Flags slaveFlags = CreateSlaveFlags();
// Since the local resource provider daemon is started after the agent is
// registered, it is guaranteed that the slave will send two
@@ -447,26 +456,21 @@ TEST_P(AgentResourceProviderConfigApiTest, AddConflict)
{
const ContentType contentType = GetParam();
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
- slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
-
// Generate a pre-existing config.
- const string configPath = path::join(resourceProviderConfigDir, "test.json");
+ const string configPath =
+ path::join(resourceProviderConfigDir.get(), "test.json");
ResourceProviderInfo oldInfo = createResourceProviderInfo("volume1:4GB");
ASSERT_SOME(os::write(configPath, stringify(JSON::protobuf(oldInfo))));
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _);
- Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), slaveFlags);
+ Try<Owned<cluster::Slave>> slave = StartSlave(detector.get());
ASSERT_SOME(slave);
AWAIT_READY(slaveRegisteredMessage);
@@ -480,7 +484,7 @@ TEST_P(AgentResourceProviderConfigApiTest, AddConflict)
// Check that no new config is created, and the existing one is not
// overwritten.
Try<list<string>> configPaths =
- fs::list(path::join(resourceProviderConfigDir, "*"));
+ fs::list(path::join(resourceProviderConfigDir.get(), "*"));
ASSERT_SOME(configPaths);
EXPECT_EQ(1u, configPaths->size());
EXPECT_EQ(configPath, configPaths->back());
@@ -503,19 +507,14 @@ TEST_P(AgentResourceProviderConfigApiTest, Update)
{
const ContentType contentType = GetParam();
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
- slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
-
// Generate a pre-existing config.
- const string configPath = path::join(resourceProviderConfigDir, "test.json");
+ const string configPath =
+ path::join(resourceProviderConfigDir.get(), "test.json");
ASSERT_SOME(os::write(
configPath,
stringify(JSON::protobuf(createResourceProviderInfo("volume1:4GB")))));
@@ -523,7 +522,7 @@ TEST_P(AgentResourceProviderConfigApiTest, Update)
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _);
- Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), slaveFlags);
+ Try<Owned<cluster::Slave>> slave = StartSlave(detector.get());
ASSERT_SOME(slave);
AWAIT_READY(slaveRegisteredMessage);
@@ -579,7 +578,7 @@ TEST_P(AgentResourceProviderConfigApiTest, Update)
// Check that no new config is created, and the existing one is overwritten.
Try<list<string>> configPaths =
- fs::list(path::join(resourceProviderConfigDir, "*"));
+ fs::list(path::join(resourceProviderConfigDir.get(), "*"));
ASSERT_SOME(configPaths);
EXPECT_EQ(1u, configPaths->size());
EXPECT_EQ(configPath, configPaths->back());
@@ -620,13 +619,13 @@ TEST_P(AgentResourceProviderConfigApiTest, IdempotentUpdate)
Owned<MasterDetector> detector = master.get()->createDetector();
- slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
-
// Generate a pre-existing config.
- const string configPath = path::join(resourceProviderConfigDir, "test.json");
ResourceProviderInfo info = createResourceProviderInfo("volume1:4GB");
- ASSERT_SOME(os::write(configPath, stringify(JSON::protobuf(info))));
+ ASSERT_SOME(os::write(
+ path::join(resourceProviderConfigDir.get(), "test.json"),
+ stringify(JSON::protobuf(info))));
+
+ slave::Flags slaveFlags = CreateSlaveFlags();
// Since the local resource provider daemon is started after the agent is
// registered, it is guaranteed that the slave will send two
@@ -676,21 +675,15 @@ TEST_P(AgentResourceProviderConfigApiTest, UpdateNotFound)
{
const ContentType contentType = GetParam();
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
- slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
-
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _);
- Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), slaveFlags);
+ Try<Owned<cluster::Slave>> slave = StartSlave(detector.get());
ASSERT_SOME(slave);
AWAIT_READY(slaveRegisteredMessage);
@@ -703,7 +696,7 @@ TEST_P(AgentResourceProviderConfigApiTest, UpdateNotFound)
// Check that no new config is created.
Try<list<string>> configPaths =
- fs::list(path::join(resourceProviderConfigDir, "*"));
+ fs::list(path::join(resourceProviderConfigDir.get(), "*"));
ASSERT_SOME(configPaths);
EXPECT_TRUE(configPaths->empty());
}
@@ -714,26 +707,21 @@ TEST_P(AgentResourceProviderConfigApiTest, Remove)
{
const ContentType contentType = GetParam();
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
- slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
-
// Generate a pre-existing config.
- const string configPath = path::join(resourceProviderConfigDir, "test.json");
+ const string configPath =
+ path::join(resourceProviderConfigDir.get(), "test.json");
ResourceProviderInfo info = createResourceProviderInfo("volume1:4GB");
ASSERT_SOME(os::write(configPath, stringify(JSON::protobuf(info))));
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _);
- Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), slaveFlags);
+ Try<Owned<cluster::Slave>> slave = StartSlave(detector.get());
ASSERT_SOME(slave);
AWAIT_READY(slaveRegisteredMessage);
@@ -794,21 +782,15 @@ TEST_P(AgentResourceProviderConfigApiTest, IdempotentRemove)
{
const ContentType contentType = GetParam();
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
- slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
-
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _);
- Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), slaveFlags);
+ Try<Owned<cluster::Slave>> slave = StartSlave(detector.get());
ASSERT_SOME(slave);
AWAIT_READY(slaveRegisteredMessage);
[mesos] 10/10: Added MESOS-9228 to the 1.7.1 CHANGELOG.
Posted by ch...@apache.org.
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 454a16a9eecf1684e68082c08f023fb5916a6e28
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Fri Sep 21 23:33:35 2018 -0700
Added MESOS-9228 to the 1.7.1 CHANGELOG.
---
CHANGELOG | 1 +
1 file changed, 1 insertion(+)
diff --git a/CHANGELOG b/CHANGELOG
index 7231535..38c9db1 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -5,6 +5,7 @@ Release Notes - Mesos - Version 1.7.1 (WIP)
** Bug
* [MESOS-8545] - AgentAPIStreamingTest.AttachInputToNestedContainerSession is flaky.
* [MESOS-9131] - Health checks launching nested containers while a container is being destroyed lead to unkillable tasks.
+ * [MESOS-9228] - SLRP does not clean up plugin containers after it is removed.
Release Notes - Mesos - Version 1.7.0
[mesos] 07/10: Moved the container ID prefix generation to
`LocalResourceProvider`.
Posted by ch...@apache.org.
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 88b28f2cd900a53a8d54b7be837ccc9813e3b764
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Thu Sep 20 11:25:11 2018 -0700
Moved the container ID prefix generation to `LocalResourceProvider`.
It is more reasonable to not allow each specific resource provider to
construct their own container ID prefix, otherwise it would be hard to
avoid conflicts. Therefore we now established the convension of how the
prefix is constructed in `LocalResourceProvider`.
Review: https://reviews.apache.org/r/68790
---
src/resource_provider/daemon.cpp | 11 +------
src/resource_provider/local.cpp | 28 +++++++++-------
src/resource_provider/local.hpp | 10 ++++--
src/resource_provider/storage/provider.cpp | 51 +++++++++---------------------
4 files changed, 40 insertions(+), 60 deletions(-)
diff --git a/src/resource_provider/daemon.cpp b/src/resource_provider/daemon.cpp
index b07ffe3..f1a941c 100644
--- a/src/resource_provider/daemon.cpp
+++ b/src/resource_provider/daemon.cpp
@@ -480,16 +480,7 @@ Future<Option<string>> LocalResourceProviderDaemonProcess::generateAuthToken(
return None();
}
- Try<Principal> principal = LocalResourceProvider::principal(info);
-
- if (principal.isError()) {
- return Failure(
- "Failed to generate resource provider principal with type '" +
- info.type() + "' and name '" + info.name() + "': " +
- principal.error());
- }
-
- return secretGenerator->generate(principal.get())
+ return secretGenerator->generate(LocalResourceProvider::principal(info))
.then(defer(self(), [](const Secret& secret) -> Future<Option<string>> {
Option<Error> error = common::validation::validateSecret(secret);
diff --git a/src/resource_provider/local.cpp b/src/resource_provider/local.cpp
index bd443aa..e5efd95 100644
--- a/src/resource_provider/local.cpp
+++ b/src/resource_provider/local.cpp
@@ -36,7 +36,6 @@ namespace internal {
struct ProviderAdaptor
{
decltype(LocalResourceProvider::create)* const create;
- decltype(LocalResourceProvider::principal)* const principal;
decltype(LocalResourceProvider::validate)* const validate;
};
@@ -46,7 +45,6 @@ static const hashmap<string, ProviderAdaptor> adaptors = {
#ifdef __linux__
{"org.apache.mesos.rp.local.storage",
{&StorageLocalResourceProvider::create,
- &StorageLocalResourceProvider::principal,
&StorageLocalResourceProvider::validate}},
#endif
};
@@ -69,24 +67,32 @@ Try<Owned<LocalResourceProvider>> LocalResourceProvider::create(
}
-Try<Principal> LocalResourceProvider::principal(
- const ResourceProviderInfo& info)
+Option<Error> LocalResourceProvider::validate(const ResourceProviderInfo& info)
{
if (!adaptors.contains(info.type())) {
return Error("Unknown local resource provider type '" + info.type() + "'");
}
- return adaptors.at(info.type()).principal(info);
+ return adaptors.at(info.type()).validate(info);
}
-Option<Error> LocalResourceProvider::validate(const ResourceProviderInfo& info)
+Principal LocalResourceProvider::principal(const ResourceProviderInfo& info)
{
- if (!adaptors.contains(info.type())) {
- return Error("Unknown local resource provider type '" + info.type() + "'");
- }
-
- return adaptors.at(info.type()).validate(info);
+ // The 'cid_prefix' for the resource provider is of the following format:
+ // <rp_type>-<rp_name>--
+ // where <rp_type> and <rp_name> are the type and name of the resource
+ // provider, with dots replaced by dashes. We use a double-dash at the end to
+ // explicitly mark the end of the prefix.
+ const string cidPrefix = strings::join(
+ "-",
+ strings::replace(info.type(), ".", "-"),
+ info.name(),
+ "-");
+
+ return Principal(
+ Option<string>::none(),
+ {{"cid_prefix", cidPrefix}});
}
} // namespace internal {
diff --git a/src/resource_provider/local.hpp b/src/resource_provider/local.hpp
index ff4e7f4..75ce0f2 100644
--- a/src/resource_provider/local.hpp
+++ b/src/resource_provider/local.hpp
@@ -41,11 +41,15 @@ public:
const Option<std::string>& authToken,
bool strict);
- static Try<process::http::authentication::Principal> principal(
- const ResourceProviderInfo& info);
-
static Option<Error> validate(const ResourceProviderInfo& info);
+ // Returns a principal that contains a 'cid_prefix' claim based on the type
+ // and name of the resource provider. The resource provider can only launch
+ // standalone containers whose ID is prefixed by the value of 'cid_prefix'.
+ // TODO(chhsiao): We should move this to a utility function.
+ static process::http::authentication::Principal principal(
+ const ResourceProviderInfo& info);
+
virtual ~LocalResourceProvider() = default;
};
diff --git a/src/resource_provider/storage/provider.cpp b/src/resource_provider/storage/provider.cpp
index 18f934b..db783b5 100644
--- a/src/resource_provider/storage/provider.cpp
+++ b/src/resource_provider/storage/provider.cpp
@@ -166,36 +166,21 @@ static bool isValidType(const string& s)
static const Duration CSI_ENDPOINT_CREATION_TIMEOUT = Minutes(1);
-// Returns a prefix for naming standalone containers to run CSI plugins
-// for the resource provider. The prefix is of the following format:
-// <rp_type>-<rp_name>--
-// where <rp_type> and <rp_name> are the type and name of the resource
-// provider, with dots replaced by dashes. We use a double-dash at the
-// end to explicitly mark the end of the prefix.
-static inline string getContainerIdPrefix(const ResourceProviderInfo& info)
-{
- return strings::join(
- "-",
- strings::replace(info.type(), ".", "-"),
- info.name(),
- "-");
-}
-
-
-// Returns the container ID of the standalone container to run a CSI
-// plugin component. The container ID is of the following format:
-// <rp_type>-<rp_name>--<csi_type>-<csi_name>--<list_of_services>
-// where <rp_type> and <rp_name> are the type and name of the resource
-// provider, and <csi_type> and <csi_name> are the type and name of the
-// CSI plugin, with dots replaced by dashes. <list_of_services> lists
-// the CSI services provided by the component, concatenated with dashes.
+// Returns the container ID of the standalone container to run a CSI plugin
+// component. The container ID is of the following format:
+// <cid_prefix><csi_type>-<csi_name>--<list_of_services>
+// where <cid_prefix> comes from the principal of the resource provider,
+// <csi_type> and <csi_name> are the type and name of the CSI plugin, with dots
+// replaced by dashes. <list_of_services> lists the CSI services provided by the
+// component, concatenated with dashes.
static inline ContainerID getContainerId(
const ResourceProviderInfo& info,
const CSIPluginContainerInfo& container)
{
- string value = getContainerIdPrefix(info);
+ const Principal principal = LocalResourceProvider::principal(info);
+ CHECK(principal.claims.contains("cid_prefix"));
- value += strings::join(
+ string value = principal.claims.at("cid_prefix") + strings::join(
"-",
strings::replace(info.storage().plugin().type(), ".", "-"),
info.storage().plugin().name(),
@@ -2101,12 +2086,15 @@ StorageLocalResourceProviderProcess::getContainers()
return Failure("Failed to get containers: " + v1Response.error());
}
- const string prefix = getContainerIdPrefix(info);
+ const Principal principal = LocalResourceProvider::principal(info);
+ CHECK(principal.claims.contains("cid_prefix"));
+
+ const string& cidPrefix = principal.claims.at("cid_prefix");
agent::Response response = devolve(v1Response.get());
foreach (const agent::Response::GetContainers::Container& container,
response.get_containers().containers()) {
- if (strings::startsWith(container.container_id().value(), prefix)) {
+ if (strings::startsWith(container.container_id().value(), cidPrefix)) {
result.put(
container.container_id(),
container.has_container_status()
@@ -3765,15 +3753,6 @@ Try<Owned<LocalResourceProvider>> StorageLocalResourceProvider::create(
}
-Try<Principal> StorageLocalResourceProvider::principal(
- const ResourceProviderInfo& info)
-{
- return Principal(
- Option<string>::none(),
- {{"cid_prefix", getContainerIdPrefix(info)}});
-}
-
-
Option<Error> StorageLocalResourceProvider::validate(
const ResourceProviderInfo& info)
{
[mesos] 01/10: Removed unnecessary failure handling in agent HTTP
API handlers.
Posted by ch...@apache.org.
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 0581437999a262277f695592c6a1be9d30bf8c31
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Tue Sep 18 12:59:52 2018 -0700
Removed unnecessary failure handling in agent HTTP API handlers.
The current agent HTTP API handlers either unnecessarily handle failures
on always-ready futures, or return "500 Internal Server Error"
unnecessarily. This patch removes those unnecessarily code.
Review: https://reviews.apache.org/r/68755
---
src/slave/http.cpp | 91 ++++++------------------------------------------------
1 file changed, 9 insertions(+), 82 deletions(-)
diff --git a/src/slave/http.cpp b/src/slave/http.cpp
index 0011ced..0adc7c0 100644
--- a/src/slave/http.cpp
+++ b/src/slave/http.cpp
@@ -2085,21 +2085,11 @@ Future<Response> Http::getContainers(
call.get_containers().show_nested(),
call.get_containers().show_standalone());
}))
- .then([acceptType](const Future<JSON::Array>& result) -> Response {
- if (!result.isReady()) {
- LOG(WARNING) << "Could not collect container status and statistics: "
- << (result.isFailed()
- ? result.failure()
- : "Discarded");
- return result.isFailed()
- ? InternalServerError(result.failure())
- : InternalServerError();
- }
-
+ .then([acceptType](const JSON::Array& result) -> Response {
return OK(
serialize(
acceptType,
- evolve<v1::agent::Response::GET_CONTAINERS>(result.get())),
+ evolve<v1::agent::Response::GET_CONTAINERS>(result)),
stringify(acceptType));
});
}
@@ -2126,20 +2116,8 @@ Future<Response> Http::_containers(
false,
false);
}))
- .then([request](const Future<JSON::Array>& result) -> Response {
- if (!result.isReady()) {
- LOG(WARNING) << "Could not collect container status and statistics: "
- << (result.isFailed()
- ? result.failure()
- : "Discarded");
-
- return result.isFailed()
- ? InternalServerError(result.failure())
- : InternalServerError();
- }
-
- return process::http::OK(
- result.get(), request.url.query.get("jsonp"));
+ .then([request](const JSON::Array& result) -> Response {
+ return process::http::OK(result, request.url.query.get("jsonp"));
});
}
@@ -2342,23 +2320,7 @@ Future<Response> Http::pruneImages(
}
return slave->containerizer->pruneImages(excludedImages)
- .then([](const Future<Nothing>& result) -> Response {
- if (!result.isReady()) {
- // TODO(zhitao): Because `containerizer::pruneImages` returns
- // a `Nothing` now, we cannot distinguish between actual
- // failure or the case that operator should drain the agent.
- // Consider returning more information.
- LOG(WARNING)
- << "Failed to prune images: "
- << (result.isFailed() ? result.failure() : "discarded");
-
- return result.isFailed()
- ? InternalServerError(result.failure())
- : InternalServerError();
- }
-
- return OK();
- });
+ .then([]() -> Response { return OK(); });
}));
}
@@ -3056,15 +3018,7 @@ Future<Response> Http::_removeContainer(
Future<Nothing> remove = slave->containerizer->remove(containerId);
return remove
- .then([=](const Future<Nothing>& result) -> Response {
- if (result.isFailed()) {
- LOG(ERROR) << "Failed to remove container " << containerId
- << ": " << result.failure();
- return InternalServerError(result.failure());
- }
-
- return OK();
- });
+ .then([=]() -> Response { return OK(); });
}
@@ -3259,15 +3213,7 @@ Future<Response> Http::addResourceProviderConfig(
}
return OK();
- })
- .repair([info](const Future<Response>& future) {
- LOG(ERROR)
- << "Failed to add resource provider config with type '"
- << info.type() << "' and name '" << info.name() << "': "
- << future.failure();
-
- return InternalServerError(future.failure());
- });
+ });
}));
}
@@ -3304,15 +3250,7 @@ Future<Response> Http::updateResourceProviderConfig(
}
return OK();
- })
- .repair([info](const Future<Response>& future) {
- LOG(ERROR)
- << "Failed to update resource provider config with type '"
- << info.type() << "' and name '" << info.name() << "': "
- << future.failure();
-
- return InternalServerError(future.failure());
- });
+ });
}));
}
@@ -3343,18 +3281,7 @@ Future<Response> Http::removeResourceProviderConfig(
<< type << "' and name '" << name << "'";
return slave->localResourceProviderDaemon->remove(type, name)
- .then([]() -> Response {
- // Config removal is always successful due to idempotency.
- return OK();
- })
- .repair([type, name](const Future<Response>& future) {
- LOG(ERROR)
- << "Failed to remove resource provider config with type '"
- << type << "' and name '" << name << "': "
- << future.failure();
-
- return InternalServerError(future.failure());
- });
+ .then([]() -> Response { return OK(); });
}));
}
[mesos] 05/10: Set master/agent flags in
`StorageLocalResourceProviderTest` fixture.
Posted by ch...@apache.org.
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 1eeeadb4a19785a11dc47bb0912b8f13d9a7434c
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Wed Sep 19 19:07:26 2018 -0700
Set master/agent flags in `StorageLocalResourceProviderTest` fixture.
Review: https://reviews.apache.org/r/68778
---
.../storage_local_resource_provider_tests.cpp | 112 ++++++---------------
1 file changed, 32 insertions(+), 80 deletions(-)
diff --git a/src/tests/storage_local_resource_provider_tests.cpp b/src/tests/storage_local_resource_provider_tests.cpp
index d191783..077a465 100644
--- a/src/tests/storage_local_resource_provider_tests.cpp
+++ b/src/tests/storage_local_resource_provider_tests.cpp
@@ -92,7 +92,8 @@ public:
resourceProviderConfigDir =
path::join(sandbox.get(), "resource_provider_configs");
- ASSERT_SOME(os::mkdir(resourceProviderConfigDir));
+
+ ASSERT_SOME(os::mkdir(resourceProviderConfigDir.get()));
}
void TearDown() override
@@ -135,11 +136,26 @@ public:
ContainerizerTest<slave::MesosContainerizer>::TearDown();
}
+ master::Flags CreateMasterFlags() override
+ {
+ master::Flags flags =
+ ContainerizerTest<slave::MesosContainerizer>::CreateMasterFlags();
+
+ // Use a small allocation interval to speed up the test. We do this instead
+ // of manipulating the clock because the storage local resource provider
+ // relies on a running clock to wait for the CSI plugin to be ready.
+ flags.allocation_interval = Milliseconds(50);
+
+ return flags;
+ }
+
slave::Flags CreateSlaveFlags() override
{
slave::Flags flags =
ContainerizerTest<slave::MesosContainerizer>::CreateSlaveFlags();
+ flags.resource_provider_config_dir = resourceProviderConfigDir;
+
// Store the agent work directory for cleaning up CSI endpoint
// directories during teardown.
// NOTE: DO NOT change the work directory afterward.
@@ -252,7 +268,7 @@ public:
ASSERT_SOME(resourceProviderConfig);
ASSERT_SOME(os::write(
- path::join(resourceProviderConfigDir, "test.json"),
+ path::join(resourceProviderConfigDir.get(), "test.json"),
resourceProviderConfig.get()));
}
@@ -293,10 +309,10 @@ public:
stringify(TEST_SLRP_NAME) + "/" + basename;
}
-protected:
+private:
Modules modules;
vector<string> slaveWorkDirs;
- string resourceProviderConfigDir;
+ Option<string> resourceProviderConfigDir;
};
@@ -314,7 +330,6 @@ TEST_F(StorageLocalResourceProviderTest, NoResource)
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
// Since the local resource provider daemon is started after the agent
// is registered, it is guaranteed that the slave will send two
@@ -390,14 +405,12 @@ TEST_F(StorageLocalResourceProviderTest, DISABLED_ZeroSizedDisk)
setupResourceProviderConfig(Bytes(0), "volume0:0B");
- master::Flags masterFlags = CreateMasterFlags();
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
// Since the local resource provider daemon is started after the agent
// is registered, it is guaranteed that the slave will send two
@@ -446,21 +459,12 @@ TEST_F(StorageLocalResourceProviderTest, DISABLED_SmallDisk)
setupResourceProviderConfig(Kilobytes(512), "volume0:512KB");
- master::Flags masterFlags = CreateMasterFlags();
-
- // Use a small allocation interval to speed up the test. We do this
- // instead of manipulating the clock to keep the test concise and
- // avoid waiting for `UpdateSlaveMessage`s and pausing/resuming the
- // clock multiple times.
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
slaveFlags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
@@ -566,14 +570,12 @@ TEST_F(StorageLocalResourceProviderTest, ProfileAppeared)
setupResourceProviderConfig(Gigabytes(4));
master::Flags masterFlags = CreateMasterFlags();
-
Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
slaveFlags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
// Since the local resource provider daemon is started after the agent
@@ -684,16 +686,12 @@ TEST_F(StorageLocalResourceProviderTest, CreateDestroyDisk)
setupResourceProviderConfig(Gigabytes(4));
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
slaveFlags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
@@ -864,16 +862,12 @@ TEST_F(StorageLocalResourceProviderTest, CreateDestroyDiskRecovery)
setupResourceProviderConfig(Gigabytes(4));
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
slaveFlags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
@@ -1081,14 +1075,12 @@ TEST_F(StorageLocalResourceProviderTest, ProfileDisappeared)
setupResourceProviderConfig(Gigabytes(4));
master::Flags masterFlags = CreateMasterFlags();
-
Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
slaveFlags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
// Since the local resource provider daemon is started after the agent
@@ -1319,16 +1311,12 @@ TEST_F(StorageLocalResourceProviderTest, AgentFailoverPluginKilled)
{
setupResourceProviderConfig(Bytes(0), "volume0:4GB");
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
slave::Fetcher fetcher(slaveFlags);
@@ -1434,16 +1422,12 @@ TEST_F(StorageLocalResourceProviderTest, AgentRegisteredWithNewId)
setupResourceProviderConfig(Gigabytes(4));
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
slaveFlags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
@@ -1633,10 +1617,7 @@ TEST_F(StorageLocalResourceProviderTest, ROOT_PublishResources)
setupResourceProviderConfig(Gigabytes(4));
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
@@ -1644,7 +1625,6 @@ TEST_F(StorageLocalResourceProviderTest, ROOT_PublishResources)
slave::Flags slaveFlags = CreateSlaveFlags();
slaveFlags.isolation = "filesystem/linux";
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
slaveFlags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
@@ -1846,10 +1826,7 @@ TEST_F(StorageLocalResourceProviderTest, ROOT_PublishResourcesRecovery)
setupResourceProviderConfig(Gigabytes(4));
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
@@ -1857,7 +1834,6 @@ TEST_F(StorageLocalResourceProviderTest, ROOT_PublishResourcesRecovery)
slave::Flags slaveFlags = CreateSlaveFlags();
slaveFlags.isolation = "filesystem/linux";
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
slaveFlags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
@@ -2116,10 +2092,7 @@ TEST_F(StorageLocalResourceProviderTest, ROOT_PublishResourcesReboot)
setupResourceProviderConfig(Gigabytes(4));
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
@@ -2127,7 +2100,6 @@ TEST_F(StorageLocalResourceProviderTest, ROOT_PublishResourcesReboot)
slave::Flags slaveFlags = CreateSlaveFlags();
slaveFlags.isolation = "filesystem/linux";
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
slaveFlags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
@@ -2427,10 +2399,7 @@ TEST_F(
setupResourceProviderConfig(Gigabytes(4));
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
@@ -2438,7 +2407,6 @@ TEST_F(
slave::Flags slaveFlags = CreateSlaveFlags();
slaveFlags.isolation = "filesystem/linux";
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
slaveFlags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
slave::Fetcher fetcher(slaveFlags);
@@ -2692,14 +2660,12 @@ TEST_F(StorageLocalResourceProviderTest, ConvertPreExistingVolume)
setupResourceProviderConfig(Bytes(0), "volume1:2GB;volume2:2GB");
master::Flags masterFlags = CreateMasterFlags();
-
Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
// Since the local resource provider daemon is started after the agent
// is registered, it is guaranteed that the slave will send two
@@ -2911,7 +2877,6 @@ TEST_F(StorageLocalResourceProviderTest, RetryOperationStatusUpdate)
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags flags = CreateSlaveFlags();
- flags.resource_provider_config_dir = resourceProviderConfigDir;
flags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
// Since the local resource provider daemon is started after the agent
@@ -3067,7 +3032,6 @@ TEST_F(
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags flags = CreateSlaveFlags();
- flags.resource_provider_config_dir = resourceProviderConfigDir;
flags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
// Since the local resource provider daemon is started after the agent
@@ -3235,7 +3199,6 @@ TEST_F(
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
slave::Fetcher fetcher(slaveFlags);
@@ -3311,16 +3274,12 @@ TEST_F(StorageLocalResourceProviderTest, OperationStateMetrics)
setupResourceProviderConfig(Gigabytes(4));
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
slaveFlags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
@@ -3560,16 +3519,12 @@ TEST_F(StorageLocalResourceProviderTest, CsiPluginRpcMetrics)
setupResourceProviderConfig(Gigabytes(4));
- master::Flags masterFlags = CreateMasterFlags();
- masterFlags.allocation_interval = Milliseconds(50);
-
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
slaveFlags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
@@ -3855,7 +3810,6 @@ TEST_F(StorageLocalResourceProviderTest, ReconcileDroppedOperation)
StandaloneMasterDetector detector(master.get()->pid);
slave::Flags slaveFlags = CreateSlaveFlags();
- slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _);
@@ -4063,7 +4017,6 @@ TEST_F(
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags flags = CreateSlaveFlags();
- flags.resource_provider_config_dir = resourceProviderConfigDir;
flags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
// Since the local resource provider daemon is started after the agent
@@ -4247,7 +4200,6 @@ TEST_F(
Owned<MasterDetector> detector = master.get()->createDetector();
slave::Flags flags = CreateSlaveFlags();
- flags.resource_provider_config_dir = resourceProviderConfigDir;
flags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
// Since the local resource provider daemon is started after the agent