You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by tn...@apache.org on 2015/11/09 08:34:20 UTC
[3/3] mesos git commit: Refactored registry puller pull to smaller
functions.
Refactored registry puller pull to smaller functions.
Review: https://reviews.apache.org/r/40070
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/d71ac987
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/d71ac987
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/d71ac987
Branch: refs/heads/master
Commit: d71ac98754d9c96ca0900431e03315b2b3820ea2
Parents: 0436141
Author: Timothy Chen <tn...@gmail.com>
Authored: Sat Nov 7 20:10:10 2015 -0800
Committer: Timothy Chen <tn...@apache.org>
Committed: Sun Nov 8 23:34:10 2015 -0800
----------------------------------------------------------------------
.../provisioner/docker/registry_puller.cpp | 254 +++++++++----------
.../provisioner/docker/registry_puller.hpp | 12 +-
2 files changed, 131 insertions(+), 135 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/d71ac987/src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp b/src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
index 6bd9838..b80213d 100644
--- a/src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
+++ b/src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
@@ -30,6 +30,7 @@
#include "slave/containerizer/mesos/provisioner/docker/registry_client.hpp"
using std::list;
+using std::pair;
using std::string;
using std::vector;
@@ -57,7 +58,7 @@ class RegistryPullerProcess : public Process<RegistryPullerProcess>
public:
static Try<Owned<RegistryPullerProcess>> create(const Flags& flags);
- process::Future<PulledImageInfo> pull(
+ process::Future<list<pair<string, string>>> pull(
const Image::Name& imageName,
const Path& downloadDir);
@@ -73,14 +74,19 @@ private:
Try<Image::Name> getRemoteNameFromLocalName(const Image::Name& name);
- Future<PulledLayerInfo> downloadLayer(
- const string& remoteName,
- const Path& downloadDir,
+ Future<list<pair<string, string>>> downloadLayers(
+ const Path& downloadDirectory,
+ const Image::Name& imageName,
+ const Manifest& manifest);
+
+ Future<pair<string, string>> downloadLayer(
+ const string& path,
+ const Path& downloadDirectory,
const FileSystemLayerInfo& layer);
Owned<RegistryClient> registryClient_;
const Duration pullTimeout_;
- hashmap<string, Owned<Promise<PulledLayerInfo>>> downloadTracker_;
+ hashmap<string, Owned<Promise<pair<string, string>>>> downloadTracker_;
RegistryPullerProcess(const RegistryPullerProcess&) = delete;
RegistryPullerProcess& operator=(const RegistryPullerProcess&) = delete;
@@ -114,7 +120,7 @@ RegistryPuller::~RegistryPuller()
}
-Future<PulledImageInfo> RegistryPuller::pull(
+Future<list<pair<string, string>>> RegistryPuller::pull(
const Image::Name& imageName,
const Path& downloadDir)
{
@@ -122,20 +128,7 @@ Future<PulledImageInfo> RegistryPuller::pull(
process_.get(),
&RegistryPullerProcess::pull,
imageName,
- downloadDir)
- // We have 'after' here because the local puller need not have a timeout.
- // Timeout is a flag for the remote puller and needs to be handled here.
- .after(process_->getTimeout(), [imageName](
- Future<PulledImageInfo> future) {
- LOG(WARNING) << "Failed to download image '"
- << imageName.repository()
- << "': timeout";
- future.discard();
-
- return Failure(
- "Failed to download image '" + imageName.repository() + "': timeout");
- });
-}
+ downloadDir);}
Try<Owned<RegistryPullerProcess>> RegistryPullerProcess::create(
@@ -193,110 +186,98 @@ RegistryPullerProcess::RegistryPullerProcess(
pullTimeout_(timeout) {}
-Future<PulledLayerInfo> RegistryPullerProcess::downloadLayer(
- const string& remoteName,
- const Path& downloadDir,
+Future<pair<string, string>> RegistryPullerProcess::downloadLayer(
+ const string& path,
+ const Path& downloadDirectory,
const FileSystemLayerInfo& layer)
{
VLOG(1) << "Downloading layer '" << layer.layerId
- << "' for image '" << remoteName << "'";
-
- Owned<Promise<PulledLayerInfo>> downloadPromise;
-
- if (!downloadTracker_.contains(layer.layerId)) {
- downloadPromise =
- Owned<Promise<PulledLayerInfo>>(new Promise<PulledLayerInfo>());
+ << "' for image '" << path << "'";
- downloadTracker_.insert({layer.layerId, downloadPromise});
- } else {
- VLOG(1) << "Download already in progress for image '" << remoteName
+ if (downloadTracker_.contains(layer.layerId)) {
+ VLOG(1) << "Download already in progress for image '" << path
<< "''s layer '" << layer.layerId << "'";
return downloadTracker_.at(layer.layerId)->future();
}
- const Path downloadFile(path::join(downloadDir, layer.layerId));
+ Owned<Promise<pair<string, string>>> downloadPromise(
+ new Promise<pair<string, string>>());
+
+ downloadTracker_[layer.layerId] = downloadPromise;
+
+ const Path downloadFile(path::join(downloadDirectory, layer.layerId));
registryClient_->getBlob(
- remoteName,
+ path,
layer.checksumInfo,
downloadFile)
.onAny(defer(
self(),
- [this, remoteName, downloadDir, layer, downloadPromise, downloadFile](
- const Future<size_t>& blobSizeFuture) {
+ [this, layer, downloadPromise, downloadFile](
+ const Future<size_t>& future) {
downloadTracker_.erase(layer.layerId);
- if(!blobSizeFuture.isReady()) {
- if (blobSizeFuture.isDiscarded()) {
- downloadPromise->fail(
- "Failed to download layer '" + layer.layerId +
- "': future discarded");
- } else {
- downloadPromise->fail(
- "Failed to download layer '" + layer.layerId + "': " +
- blobSizeFuture.failure());
- }
-
- return;
- }
-
- if (blobSizeFuture.get() <= 0) {
+ if (!future.isReady()) {
+ downloadPromise->fail(
+ "Failed to download layer '" + layer.layerId +
+ "': " +
+ (future.isFailed() ? future.failure() : "future discarded"));
+ } else if (future.get() <= 0) {
+ // We don't ever expect Docker registry to return empty response
+ // even for empty layers in the image.
downloadPromise->fail(
"Failed to download layer '" + layer.layerId + "': no content");
- return;
+ } else {
+ downloadPromise->set({layer.layerId, downloadFile});
}
-
- downloadPromise->set(PulledLayerInfo(layer.layerId, downloadFile));
}));
return downloadPromise->future();
}
+// Canonical names could be something like "ubuntu14.04" but the
+// remote name for it could be library/ubuntu14.04.
+// TODO(jojy): This mapping has to com from a configuration in the
+// near future. Docker has different namings depending on whether its an
+// "official" repo or not.
+// From the docker github code documentation:
+// RepositoryInfo Examples:
+// {
+// "Index" : {
+// "Name" : "docker.io",
+// "Mirrors" : ["https://registry-2.docker.io/v1/",
+// "https://registry-3.docker.io/v1/"],
+// "Secure" : true,
+// "Official" : true,
+// },
+// "RemoteName" : "library/debian",
+// "LocalName" : "debian",
+// "CanonicalName" : "docker.io/debian"
+// "Official" : true,
+// }
+//
+// {
+// "Index" : {
+// "Name" : "127.0.0.1:5000",
+// "Mirrors" : [],
+// "Secure" : false,
+// "Official" : false,
+// },
+// "RemoteName" : "user/repo",
+// "LocalName" : "127.0.0.1:5000/user/repo",
+// "CanonicalName" : "127.0.0.1:5000/user/repo",
+// "Official" : false,
+// }
Try<Image::Name> RegistryPullerProcess::getRemoteNameFromLocalName(
const Image::Name& imageName)
{
- //TODO(jojy): Canonical names could be something like "ubuntu14.04" but the
- //remote name for it could be library/ubuntu14.04. This mapping has to come
- //from a configuration in the near future. Docker has different namings
- //depending on whether its an "official" repo or not.
-
- // From the docker github code documentation:
- // RepositoryInfo Examples:
- // {
- // "Index" : {
- // "Name" : "docker.io",
- // "Mirrors" : ["https://registry-2.docker.io/v1/",
- // "https://registry-3.docker.io/v1/"],
- // "Secure" : true,
- // "Official" : true,
- // },
- // "RemoteName" : "library/debian",
- // "LocalName" : "debian",
- // "CanonicalName" : "docker.io/debian"
- // "Official" : true,
- // }
- //
- // {
- // "Index" : {
- // "Name" : "127.0.0.1:5000",
- // "Mirrors" : [],
- // "Secure" : false,
- // "Official" : false,
- // },
- // "RemoteName" : "user/repo",
- // "LocalName" : "127.0.0.1:5000/user/repo",
- // "CanonicalName" : "127.0.0.1:5000/user/repo",
- // "Official" : false,
- // }
-
- // For now, assuming that its an "official" repo.
+ // TODO(jojy): Support unoffical repos.
if (strings::contains(imageName.repository(), "/")) {
- return Error("Unexpected '/' in local image name");
+ return Error("Currently only support offical repos");
}
- // Create a remote Image::Name from the given input name.
Image::Name remoteImageName = imageName;
remoteImageName.set_repository("library/" + imageName.repository());
@@ -347,10 +328,48 @@ Future<Nothing> untar(const string& file, const string& directory)
}
-Future<PulledImageInfo> RegistryPullerProcess::pull(
+Future<list<pair<string, string>>> RegistryPullerProcess::downloadLayers(
+ const Path& downloadDirectory,
+ const Image::Name& remoteName,
+ const Manifest& manifest)
+{
+ list<Future<pair<string, string>>> downloadFutures;
+
+ foreach (const FileSystemLayerInfo& layer, manifest.fsLayerInfos) {
+ downloadFutures.push_back(
+ downloadLayer(
+ remoteName.repository(),
+ downloadDirectory,
+ layer));
+ }
+
+ return collect(downloadFutures);
+}
+
+
+Future<list<pair<string, string>>> untarLayers(
+ const Path& downloadDirectory,
+ const list<pair<string, string>>& imageInfo)
+{
+ list<Future<Nothing>> untarFutures;
+
+ pair<string, string> layerInfo;
+ foreach (layerInfo, imageInfo) {
+ untarFutures.emplace_back(untar(layerInfo.second, downloadDirectory));
+ }
+
+ return collect(untarFutures)
+ .then([imageInfo]() { return imageInfo; });
+}
+
+
+Future<list<pair<string, string>>> RegistryPullerProcess::pull(
const Image::Name& imageName,
- const Path& downloadDir)
+ const Path& downloadDirectory)
{
+ // We assume the name passed from the framework is local name.
+ // For more information please refer to comments in
+ // getRemoteNameFromLocalName.
Try<Image::Name> remoteImageName = getRemoteNameFromLocalName(imageName);
if (remoteImageName.isError()) {
return Failure(
@@ -358,43 +377,24 @@ Future<PulledImageInfo> RegistryPullerProcess::pull(
imageName.repository() + "': " + remoteImageName.error());
}
-
// TODO(jojy): Delete downloaded files in the directory on discard and
// failure?
- // TODO(jojy): Iterate through the futures and log the failed future.
return registryClient_->getManifest(remoteImageName.get())
- .then(defer(self(), [this, downloadDir, remoteImageName](
- const Manifest& manifest) {
- list<Future<PulledLayerInfo>> downloadFutures;
-
- foreach (const FileSystemLayerInfo& layer, manifest.fsLayerInfos) {
- downloadFutures.push_back(downloadLayer(
- remoteImageName.get().repository(),
- downloadDir,
- layer));
- }
-
- return collect(downloadFutures);
- }))
- .then([downloadDir]
- (const Future<list<PulledLayerInfo>>& layerFutures)
- -> Future<PulledImageInfo> {
- list<Future<Nothing>> untarFutures;
-
- foreach (const PulledLayerInfo layerInfo, layerFutures.get()) {
- untarFutures.emplace_back(untar(layerInfo.second, downloadDir));
- }
-
- return collect(untarFutures)
- .then([layerFutures]() {
- PulledImageInfo layers;
-
- foreach (const PulledLayerInfo layerInfo, layerFutures.get()) {
- layers.emplace_back(layerInfo);
- }
+ .then(defer(self(),
+ &Self::downloadLayers,
+ downloadDirectory,
+ remoteImageName.get(),
+ lambda::_1))
+ .then(lambda::bind(&untarLayers, downloadDirectory, lambda::_1))
+ .then([](const list<pair<string, string>>& imageInfo) { return imageInfo; })
+ .after(pullTimeout_, [remoteImageName](
+ Future<list<pair<string, string>>> future)
+ -> Future<list<pair<string, string>>> {
+ future.discard();
- return layers;
- });
+ return Failure(
+ "Failed to download image '" + remoteImageName.get().repository() +
+ "': timeout");
});
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/d71ac987/src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp b/src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp
index a664955..b4fe86b 100644
--- a/src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp
+++ b/src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp
@@ -42,14 +42,11 @@ namespace docker {
class RegistryPullerProcess;
/*
- * Pulls an image from remote registry.
+ * Pulls an image from Docker registry.
*/
class RegistryPuller : public Puller
{
public:
- typedef std::pair<std::string, std::string> PulledLayerInfo;
- typedef std::list<PulledLayerInfo> PulledImageInfo;
-
/**
* Factory method for creating RegistryPuller.
*/
@@ -61,12 +58,11 @@ public:
* Pulls an image into a download directory.
*
* @param imageName local name of the image.
- * @param downloadDir path to which the layers should be downloaded.
+ * @param downloadDirectory path to which the layers should be downloaded.
*/
- process::Future<PulledImageInfo> pull(
+ process::Future<std::list<std::pair<std::string, std::string>>> pull(
const Image::Name& imageName,
- const Path& downloadDir);
-
+ const Path& downloadDirectory);
private:
RegistryPuller(const process::Owned<RegistryPullerProcess>& process);