You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by as...@apache.org on 2020/09/03 15:00:48 UTC
[mesos] branch master updated (a16f343 -> 5274e97)
This is an automated email from the ASF dual-hosted git repository.
asekretenko pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git.
from a16f343 Moved the `volume/csi` isolator's root dir under work dir.
new a4a2937 Deduplicated concurrent image pulls by docker store.
new d7d6498 Reverted removal of the PullingSameImageSimutanuously test.
new f679109 Fixed typos in the name of `PullingSameImageSimutanuously` test.
new 3ee545c Added discarding a future returned by get() into the docker store test.
new 5274e97 Added a test for discarding image pull on discard of getting image.
The 5 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:
.../mesos/provisioner/docker/store.cpp | 110 ++++++++++----
.../containerizer/provisioner_docker_tests.cpp | 163 +++++++++++++++++++++
2 files changed, 248 insertions(+), 25 deletions(-)
[mesos] 03/05: Fixed typos in the name of
`PullingSameImageSimutanuously` test.
Posted by as...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
asekretenko pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit f6791097995f0e736349a852fdb7fef11e809e63
Author: Andrei Sekretenko <as...@apache.org>
AuthorDate: Thu Aug 20 18:45:44 2020 +0200
Fixed typos in the name of `PullingSameImageSimutanuously` test.
Review: https://reviews.apache.org/r/72792
---
src/tests/containerizer/provisioner_docker_tests.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/tests/containerizer/provisioner_docker_tests.cpp b/src/tests/containerizer/provisioner_docker_tests.cpp
index 5d5a355..477a875 100644
--- a/src/tests/containerizer/provisioner_docker_tests.cpp
+++ b/src/tests/containerizer/provisioner_docker_tests.cpp
@@ -335,7 +335,7 @@ public:
// This tests the store to pull the same image simultaneously.
// This test verifies that the store only calls the puller once
// when multiple requests for the same image is in flight.
-TEST_F(ProvisionerDockerLocalStoreTest, PullingSameImageSimutanuously)
+TEST_F(ProvisionerDockerLocalStoreTest, PullingSameImageSimultaneously)
{
slave::Flags flags;
flags.docker_registry = path::join(os::getcwd(), "images");
[mesos] 01/05: Deduplicated concurrent image pulls by docker store.
Posted by as...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
asekretenko pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit a4a2937218c654c4b1490820d0b1d119ba0ae16d
Author: Andrei Sekretenko <as...@apache.org>
AuthorDate: Thu Aug 20 17:02:51 2020 +0200
Deduplicated concurrent image pulls by docker store.
This patch makes the docker store reuse a pending pull if asked for
an image that is already being pulled.
The pull caching follows the same approach as the earlier attempt in
https://reviews.apache.org/r/39331 . However, handing out futures to
the store callers and handling their discards are performed differently,
using a form of reference counting, so that the pull itself is discarded
only if all the futures returned by `Store::get()` have been discarded.
Review: https://reviews.apache.org/r/72790
---
.../mesos/provisioner/docker/store.cpp | 110 ++++++++++++++++-----
1 file changed, 85 insertions(+), 25 deletions(-)
diff --git a/src/slave/containerizer/mesos/provisioner/docker/store.cpp b/src/slave/containerizer/mesos/provisioner/docker/store.cpp
index bf2be90..bbba8cf 100644
--- a/src/slave/containerizer/mesos/provisioner/docker/store.cpp
+++ b/src/slave/containerizer/mesos/provisioner/docker/store.cpp
@@ -143,6 +143,26 @@ private:
Owned<MetadataManager> metadataManager;
Owned<Puller> puller;
+ // The get() method deduplicates image pulls by caching a single Future
+ // per each initiated image pull, handing out `undiscardable(pull)` to the
+ // caller (to prevent discards by the callers from propagating and discarding
+ // the pull) and tracking the number of the handed out `undiscardable`s for
+ // each pending pull that themselves have not been discarded yet (`useCount`).
+ //
+ // The pull future itself is discarded as soon as its `useCount` drops to
+ // zero to notify the puller that the pull has been cancelled.
+
+ // A pull future and its use count.
+ struct Pull
+ {
+ Future<Image> future;
+ size_t useCount;
+ };
+
+ // The pending pull futures are keyed by the stringified `ImageReference`
+ // for the image being pulled.
+ hashmap<string, Pull> pulling;
+
// For executing path removals in a separated actor.
process::Executor executor;
@@ -338,34 +358,69 @@ Future<Image> StoreProcess::_get(
}
}
- Try<string> staging =
- os::mkdtemp(paths::getStagingTempDir(flags.docker_store_dir));
+ const string pullKey = stringify(reference);
- if (staging.isError()) {
- return Failure(
- "Failed to create a staging directory: " + staging.error());
- }
+ Future<Image> pull = [&]() -> Future<Image> {
+ if (pulling.contains(pullKey)) {
+ pulling.at(pullKey).useCount += 1;
- LOG(INFO) << "Pulling image '" << reference << "'";
-
- return metrics.image_pull.time(puller->pull(
- reference,
- staging.get(),
- backend,
- config)
- .then(defer(self(), &Self::moveLayers, staging.get(), lambda::_1, backend))
- .then(defer(self(), [=](const Image& image) {
- LOG(INFO) << "Caching image '" << reference << "'";
- return metadataManager->put(image);
- }))
- .onAny(defer(self(), [=](const Future<Image>& image) {
- LOG(INFO) << "Removing staging directory '" << staging.get() << "'";
- Try<Nothing> rmdir = os::rmdir(staging.get());
- if (rmdir.isError()) {
- LOG(WARNING) << "Failed to remove staging directory '" << staging.get()
- << "': " << rmdir.error();
+ // NOTE: In the rare case when the future has already failed but the
+ // onAny() callback has not been executed yet, we will hand out an already
+ // FAILED future. This is basically equivalent to handing out a PENDING
+ // future that will fail immediately, and should not be an issue.
+ CHECK(!pulling.at(pullKey).future.hasDiscard());
+ return pulling.at(pullKey).future;
+ }
+
+ Try<string> staging =
+ os::mkdtemp(paths::getStagingTempDir(flags.docker_store_dir));
+
+ if (staging.isError()) {
+ return Failure(
+ "Failed to create a staging directory: " + staging.error());
+ }
+
+ LOG(INFO) << "Pulling image '" << reference << "'";
+
+ Future<Image> pull = metrics.image_pull.time(
+ puller->pull(reference, staging.get(), backend, config)
+ .then(defer(
+ self(), &Self::moveLayers, staging.get(), lambda::_1, backend))
+ .then(defer(
+ self(),
+ [=](const Image& image) {
+ LOG(INFO) << "Caching image '" << reference << "'";
+ return metadataManager->put(image);
+ }))
+ .onAny(defer(self(), [=](const Future<Image>& image) {
+ pulling.erase(pullKey);
+
+ LOG(INFO) << "Removing staging directory '" << staging.get() << "'";
+ Try<Nothing> rmdir = os::rmdir(staging.get());
+ if (rmdir.isError()) {
+ LOG(WARNING) << "Failed to remove staging directory '"
+ << staging.get() << "': " << rmdir.error();
+ }
+ })));
+
+ pulling[pullKey] = {pull, 1};
+ return pull;
+ }();
+
+ // Each call of this method returns an `undiscardable` future referencing the
+ // `pull` future. The `pull` future itself will be discarded as soon as
+ // all the `undiscardable`s have been discarded.
+ return undiscardable(pull)
+ .onDiscard(defer(self(), [=]() {
+ if (pulling.contains(pullKey)) {
+ pulling.at(pullKey).useCount -= 1;
+
+ if (pulling.at(pullKey).useCount == 0) {
+ pulling.at(pullKey).future.discard();
+ pulling.erase(pullKey);
+ }
}
- })));
+ }));
}
@@ -537,6 +592,11 @@ Future<Nothing> StoreProcess::prune(
const vector<mesos::Image>& excludedImages,
const hashset<string>& activeLayerPaths)
{
+ // All existing pulling should have finished.
+ if (!pulling.empty()) {
+ return Failure("Cannot prune and pull at the same time");
+ }
+
vector<spec::ImageReference> imageReferences;
imageReferences.reserve(excludedImages.size());
[mesos] 05/05: Added a test for discarding image pull on discard of
getting image.
Posted by as...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
asekretenko pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 5274e973c9d9ff06f9c80af9c80efd510aa492b7
Author: Andrei Sekretenko <as...@apache.org>
AuthorDate: Thu Aug 20 18:36:40 2020 +0200
Added a test for discarding image pull on discard of getting image.
This test checks that when all futures returned by docker store's
`get()` that are pending image pull are discarded by the callers,
the pull future (returned by the puller to the store) is discarded
by the store as well.
Review: https://reviews.apache.org/r/72794
---
.../containerizer/provisioner_docker_tests.cpp | 45 ++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/src/tests/containerizer/provisioner_docker_tests.cpp b/src/tests/containerizer/provisioner_docker_tests.cpp
index 5de97e2..b1f38ae 100644
--- a/src/tests/containerizer/provisioner_docker_tests.cpp
+++ b/src/tests/containerizer/provisioner_docker_tests.cpp
@@ -23,6 +23,7 @@
#include <stout/path.hpp>
#include <stout/stringify.hpp>
+#include <process/clock.hpp>
#include <process/future.hpp>
#include <process/gmock.hpp>
#include <process/owned.hpp>
@@ -59,6 +60,7 @@ namespace spec = ::docker::spec;
using std::string;
using std::vector;
+using process::Clock;
using process::Future;
using process::Owned;
using process::PID;
@@ -415,6 +417,49 @@ TEST_F(ProvisionerDockerLocalStoreTest, PullingSameImageSimultaneously)
}
+// This tests that pulling the image will be cancelled if all the pending
+// futures returned by `Store::get()` for this pull are discarded.
+TEST_F(ProvisionerDockerLocalStoreTest, PullDiscarded)
+{
+ slave::Flags flags;
+ flags.docker_registry = path::join(os::getcwd(), "images");
+ flags.docker_store_dir = path::join(os::getcwd(), "store");
+
+ MockPuller* puller = new MockPuller();
+ Future<Nothing> pullCalled;
+ Promise<slave::docker::Image> promise;
+
+ EXPECT_CALL(*puller, pull(_, _, _, _))
+ .WillOnce(testing::DoAll(FutureSatisfy(&pullCalled),
+ Return(promise.future())));
+
+ Try<Owned<slave::Store>> store =
+ slave::docker::Store::create(flags, Owned<Puller>(puller));
+
+ ASSERT_SOME(store);
+
+ Image mesosImage;
+ mesosImage.set_type(Image::DOCKER);
+ mesosImage.mutable_docker()->set_name("abc");
+
+ Future<slave::ImageInfo> imageInfo1 =
+ store.get()->get(mesosImage, COPY_BACKEND);
+
+ Future<slave::ImageInfo> imageInfo2 =
+ store.get()->get(mesosImage, COPY_BACKEND);
+
+ AWAIT_READY(pullCalled);
+
+ imageInfo1.discard();
+ imageInfo2.discard();
+
+ Clock::pause();
+ Clock::settle();
+
+ ASSERT_TRUE(promise.future().hasDiscard());
+}
+
+
#ifdef __linux__
class ProvisionerDockerTest
: public MesosTest,
[mesos] 02/05: Reverted removal of the
PullingSameImageSimutanuously test.
Posted by as...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
asekretenko pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit d7d6498bd4cd88d9d929ef89441de80c54118193
Author: Andrei Sekretenko <as...@apache.org>
AuthorDate: Thu Aug 20 17:49:13 2020 +0200
Reverted removal of the PullingSameImageSimutanuously test.
Now that the docker store triggers pull at most once per multiple
simultaneous requests to the store, removal of the
`PullingSameImageSimutanuously` test in
33c61a1907129126f3b2e37b1f53827a04e89a34 can be reverted.
Review: https://reviews.apache.org/r/72791
---
.../containerizer/provisioner_docker_tests.cpp | 107 +++++++++++++++++++++
1 file changed, 107 insertions(+)
diff --git a/src/tests/containerizer/provisioner_docker_tests.cpp b/src/tests/containerizer/provisioner_docker_tests.cpp
index c3ef4a0..5d5a355 100644
--- a/src/tests/containerizer/provisioner_docker_tests.cpp
+++ b/src/tests/containerizer/provisioner_docker_tests.cpp
@@ -39,6 +39,8 @@
#include "slave/containerizer/mesos/provisioner/docker/message.hpp"
#include "slave/containerizer/mesos/provisioner/docker/metadata_manager.hpp"
#include "slave/containerizer/mesos/provisioner/docker/paths.hpp"
+#include "slave/containerizer/mesos/provisioner/docker/puller.hpp"
+#include "slave/containerizer/mesos/provisioner/docker/registry_puller.hpp"
#include "slave/containerizer/mesos/provisioner/docker/store.hpp"
#include "tests/environment.hpp"
@@ -79,6 +81,8 @@ using mesos::slave::ContainerTermination;
using slave::ImageInfo;
using slave::Slave;
+using slave::docker::Puller;
+using slave::docker::RegistryPuller;
using slave::docker::Store;
using testing::WithParamInterface;
@@ -297,6 +301,109 @@ TEST_F(ProvisionerDockerLocalStoreTest, MissingLayer)
}
+class MockPuller : public Puller
+{
+public:
+ MockPuller()
+ {
+ EXPECT_CALL(*this, pull(_, _, _, _))
+ .WillRepeatedly(Invoke(this, &MockPuller::unmocked_pull));
+ }
+
+ ~MockPuller() override {}
+
+ MOCK_METHOD4(
+ pull,
+ Future<slave::docker::Image>(
+ const spec::ImageReference&,
+ const string&,
+ const string&,
+ const Option<Secret>&));
+
+ Future<slave::docker::Image> unmocked_pull(
+ const spec::ImageReference& reference,
+ const string& directory,
+ const string& backend,
+ const Option<Secret>& config)
+ {
+ // TODO(gilbert): Allow return Image to be overridden.
+ return slave::docker::Image();
+ }
+};
+
+
+// This tests the store to pull the same image simultaneously.
+// This test verifies that the store only calls the puller once
+// when multiple requests for the same image is in flight.
+TEST_F(ProvisionerDockerLocalStoreTest, PullingSameImageSimutanuously)
+{
+ slave::Flags flags;
+ flags.docker_registry = path::join(os::getcwd(), "images");
+ flags.docker_store_dir = path::join(os::getcwd(), "store");
+
+ MockPuller* puller = new MockPuller();
+ Future<Nothing> pull;
+ Future<string> directory;
+ Promise<slave::docker::Image> promise;
+
+ EXPECT_CALL(*puller, pull(_, _, _, _))
+ .WillOnce(testing::DoAll(FutureSatisfy(&pull),
+ FutureArg<1>(&directory),
+ Return(promise.future())));
+
+ Try<Owned<slave::Store>> store =
+ slave::docker::Store::create(flags, Owned<Puller>(puller));
+ ASSERT_SOME(store);
+
+ Image mesosImage;
+ mesosImage.set_type(Image::DOCKER);
+ mesosImage.mutable_docker()->set_name("abc");
+
+ Future<slave::ImageInfo> imageInfo1 =
+ store.get()->get(mesosImage, COPY_BACKEND);
+
+ AWAIT_READY(pull);
+ AWAIT_READY(directory);
+
+ // TODO(gilbert): Need a helper method to create test layers
+ // which will allow us to set manifest so that we can add
+ // checks here.
+ const string layerPath = path::join(directory.get(), "456");
+
+ Try<Nothing> mkdir = os::mkdir(layerPath);
+ ASSERT_SOME(mkdir);
+
+ JSON::Value manifest = JSON::parse(
+ "{"
+ " \"parent\": \"\""
+ "}").get();
+
+ ASSERT_SOME(
+ os::write(path::join(layerPath, "json"), stringify(manifest)));
+
+ ASSERT_TRUE(imageInfo1.isPending());
+ Future<slave::ImageInfo> imageInfo2 =
+ store.get()->get(mesosImage, COPY_BACKEND);
+
+ Try<spec::ImageReference> reference =
+ spec::parseImageReference(mesosImage.docker().name());
+
+ ASSERT_SOME(reference);
+
+ slave::docker::Image result;
+ result.mutable_reference()->CopyFrom(reference.get());
+ result.add_layer_ids("456");
+
+ ASSERT_TRUE(imageInfo2.isPending());
+ promise.set(result);
+
+ AWAIT_READY(imageInfo1);
+ AWAIT_READY(imageInfo2);
+
+ EXPECT_EQ(imageInfo1->layers, imageInfo2->layers);
+}
+
+
#ifdef __linux__
class ProvisionerDockerTest
: public MesosTest,
[mesos] 04/05: Added discarding a future returned by get() into the
docker store test.
Posted by as...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
asekretenko pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 3ee545c4f039000adc80d4bac3550e40616999db
Author: Andrei Sekretenko <as...@apache.org>
AuthorDate: Thu Aug 20 18:16:39 2020 +0200
Added discarding a future returned by get() into the docker store test.
This patch extends the `PullingSameImageSimultaneously` test
with discarding one of the futures returned by `store->get()`
and making sure that the pull still completes.
Review: https://reviews.apache.org/r/72793
---
src/tests/containerizer/provisioner_docker_tests.cpp | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/tests/containerizer/provisioner_docker_tests.cpp b/src/tests/containerizer/provisioner_docker_tests.cpp
index 477a875..5de97e2 100644
--- a/src/tests/containerizer/provisioner_docker_tests.cpp
+++ b/src/tests/containerizer/provisioner_docker_tests.cpp
@@ -335,6 +335,9 @@ public:
// This tests the store to pull the same image simultaneously.
// This test verifies that the store only calls the puller once
// when multiple requests for the same image is in flight.
+// In addition, this test verifies that if some of the futures
+// returned by `Store::get()` that are pending image pull are discarded,
+// the pull still completes.
TEST_F(ProvisionerDockerLocalStoreTest, PullingSameImageSimultaneously)
{
slave::Flags flags;
@@ -385,6 +388,9 @@ TEST_F(ProvisionerDockerLocalStoreTest, PullingSameImageSimultaneously)
Future<slave::ImageInfo> imageInfo2 =
store.get()->get(mesosImage, COPY_BACKEND);
+ Future<slave::ImageInfo> imageInfo3 =
+ store.get()->get(mesosImage, COPY_BACKEND);
+
Try<spec::ImageReference> reference =
spec::parseImageReference(mesosImage.docker().name());
@@ -395,6 +401,11 @@ TEST_F(ProvisionerDockerLocalStoreTest, PullingSameImageSimultaneously)
result.add_layer_ids("456");
ASSERT_TRUE(imageInfo2.isPending());
+ ASSERT_TRUE(imageInfo3.isPending());
+
+ // Pull should still complete, despite of the discard of a single future.
+ imageInfo3.discard();
+
promise.set(result);
AWAIT_READY(imageInfo1);