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/10 09:25:01 UTC
[2/2] mesos git commit: Refactored Docker registry puller.
Refactored Docker registry puller.
Changes:
- Changes the getBlob API to accept Image name instead of a path.
- Moved the intellegence of mapping remote path to registry client.
Review: https://reviews.apache.org/r/40120
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/57c87a53
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/57c87a53
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/57c87a53
Branch: refs/heads/master
Commit: 57c87a53bfa029eb2863e1113eea7b3a4ff67a40
Parents: 8ff1859
Author: Timothy Chen <tn...@gmail.com>
Authored: Mon Nov 9 23:56:47 2015 -0800
Committer: Timothy Chen <tn...@apache.org>
Committed: Tue Nov 10 00:24:55 2015 -0800
----------------------------------------------------------------------
.../provisioner/docker/registry_client.cpp | 53 +++--
.../provisioner/docker/registry_client.hpp | 4 +-
.../provisioner/docker/registry_puller.cpp | 223 ++++++-------------
.../provisioner/docker/registry_puller.hpp | 5 +-
.../containerizer/provisioner_docker_tests.cpp | 4 +-
5 files changed, 108 insertions(+), 181 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/57c87a53/src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp b/src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp
index 60239e4..f6ae9e8 100644
--- a/src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp
+++ b/src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp
@@ -75,7 +75,7 @@ public:
const Image::Name& imageName);
Future<size_t> getBlob(
- const string& path,
+ const Image::Name& imageName,
const Option<string>& digest,
const Path& filePath);
@@ -113,6 +113,10 @@ private:
int fd,
Pipe::Reader reader);
+ std::string getRepositoryPath(const Image::Name& imageName) const;
+
+ std::string getAPIVersion() const;
+
const http::URL registryServer_;
Owned<TokenManager> tokenManager_;
const Option<Credentials> credentials_;
@@ -178,16 +182,16 @@ Future<Manifest> RegistryClient::getManifest(
Future<size_t> RegistryClient::getBlob(
- const string& _path,
- const Option<string>& _digest,
- const Path& _filePath)
+ const Image::Name& imageName,
+ const Option<string>& digest,
+ const Path& filePath)
{
return dispatch(
process_.get(),
&RegistryClientProcess::getBlob,
- _path,
- _digest,
- _filePath);
+ imageName,
+ digest,
+ filePath);
}
@@ -653,12 +657,32 @@ Try<Manifest> Manifest::create(const string& jsonString)
}
+// TODO(tnachen): Support other Docker registry API versions.
+string RegistryClientProcess::getAPIVersion() const
+{
+ return "v2";
+}
+
+
+// Returns the path to the repository that is used to construct
+// URL to call the Docker registry server.
+// Currently we only support offical repositories that always prefix
+// repository with library/.
+// TODO(tnachen): Support unoffical repositories and loading in repository
+// information.
+string RegistryClientProcess::getRepositoryPath(
+ const Image::Name& imageName) const
+{
+ return getAPIVersion() + "/library/" + imageName.repository();
+}
+
+
Future<Manifest> RegistryClientProcess::getManifest(
const Image::Name& imageName)
{
http::URL manifestURL(registryServer_);
manifestURL.path =
- "v2/" + imageName.repository() + "/manifests/" + imageName.tag();
+ getRepositoryPath(imageName) + "/manifests/" + imageName.tag();
return doHttpGet(manifestURL, None(), false, true, None())
.then(defer(self(), [this] (
@@ -711,23 +735,18 @@ Future<size_t> RegistryClientProcess::saveBlob(
Future<size_t> RegistryClientProcess::getBlob(
- const string& path,
+ const Image::Name& imageName,
const Option<string>& digest,
const Path& filePath)
{
- const string dirName = filePath.dirname();
-
- Try<Nothing> mkdir = os::mkdir(dirName, true);
+ Try<Nothing> mkdir = os::mkdir(filePath.dirname(), true);
if (mkdir.isError()) {
return Failure(
"Failed to create directory to download blob: " + mkdir.error());
}
- if (strings::contains(path, " ")) {
- return Failure("Invalid repository path: " + path);
- }
-
- const string blobURLPath = "v2/" + path + "/blobs/" + digest.getOrElse("");
+ const string blobURLPath = getRepositoryPath(imageName) + "/blobs/" +
+ digest.getOrElse("");
http::URL blobURL(registryServer_);
blobURL.path = blobURLPath;
http://git-wip-us.apache.org/repos/asf/mesos/blob/57c87a53/src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp b/src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp
index 7bba371..3fd84e3 100644
--- a/src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp
+++ b/src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp
@@ -112,14 +112,14 @@ public:
/**
* Fetches blob for a repository from the client's remote registry server.
*
- * @param path path of the repository on the registry.
+ * @param imageName the Docker image to download.
* @param digest digest of the blob (from manifest).
* @param filePath file path to store the fetched blob.
* @return size of downloaded blob on success.
* Failure in case of any errors.
*/
process::Future<size_t> getBlob(
- const std::string& path,
+ const Image::Name& imageName,
const Option<std::string>& digest,
const Path& filePath);
http://git-wip-us.apache.org/repos/asf/mesos/blob/57c87a53/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 1892786..e883b40 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;
@@ -47,10 +48,6 @@ namespace internal {
namespace slave {
namespace docker {
-
-using PulledLayerInfo = RegistryPuller::PulledLayerInfo;
-using PulledImageInfo = RegistryPuller::PulledImageInfo;
-
using FileSystemLayerInfo = registry::FileSystemLayerInfo;
using Manifest = registry::Manifest;
using RegistryClient = registry::RegistryClient;
@@ -61,43 +58,36 @@ 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);
-
- Duration getTimeout() const
- {
- return pullTimeout_;
- }
+ const Path& downloadDirectory);
private:
explicit RegistryPullerProcess(
const Owned<RegistryClient>& registry,
const Duration& timeout);
- Try<Image::Name> getRemoteNameFromLocalName(const Image::Name& name);
-
- Future<PulledLayerInfo> downloadLayer(
- const string& remoteName,
- const Path& downloadDir,
+ Future<pair<string, string>> downloadLayer(
+ const Image::Name& imageName,
+ const Path& downloadDirectory,
const FileSystemLayerInfo& layer);
- Future<PulledImageInfo> downloadLayers(
+ Future<list<pair<string, string>>> downloadLayers(
const Manifest& manifest,
- const Image::Name& remoteImageName,
+ const Image::Name& imageName,
const Path& downloadDir);
- process::Future<PulledImageInfo> _pull(
+ process::Future<list<pair<string, string>>> _pull(
const Image::Name& imageName,
const Path& downloadDir);
- Future<PulledImageInfo> untarLayers(
- const Future<list<PulledLayerInfo>>& layerFutures,
+ Future<list<pair<string, string>>> untarLayers(
+ const Future<list<pair<string, string>>>& layerFutures,
const Path& downloadDir);
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;
@@ -131,7 +121,7 @@ RegistryPuller::~RegistryPuller()
}
-Future<PulledImageInfo> RegistryPuller::pull(
+Future<list<pair<string, string>>> RegistryPuller::pull(
const Image::Name& imageName,
const Path& downloadDir)
{
@@ -198,117 +188,56 @@ RegistryPullerProcess::RegistryPullerProcess(
pullTimeout_(timeout) {}
-Future<PulledLayerInfo> RegistryPullerProcess::downloadLayer(
- const string& remoteName,
- const Path& downloadDir,
+Future<pair<string, string>> RegistryPullerProcess::downloadLayer(
+ const Image::Name& imageName,
+ 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 '" << stringify(imageName) << "'";
- downloadTracker_.insert({layer.layerId, downloadPromise});
- } else {
- VLOG(1) << "Download already in progress for image '" << remoteName
- << "''s layer '" << layer.layerId << "'";
+ if (downloadTracker_.contains(layer.layerId)) {
+ VLOG(1) << "Download already in progress for image '"
+ << stringify(imageName) << "', 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_.insert({layer.layerId, downloadPromise});
+
+ const Path downloadFile(path::join(downloadDirectory, layer.layerId));
registryClient_->getBlob(
- remoteName,
+ imageName,
layer.checksumInfo,
downloadFile)
.onAny(process::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 {
+ if (!future.isReady()) {
downloadPromise->fail(
"Failed to download layer '" + layer.layerId + "': " +
- blobSizeFuture.failure());
- }
-
- return;
- }
-
- if (blobSizeFuture.get() <= 0) {
+ (future.isFailed() ? future.failure() : "future discarded"));
+ } else if (future.get() == 0) {
+ // We don't expect Docker registry to return empty response
+ // even with empty layers.
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();
}
-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.
- if (strings::contains(imageName.repository(), "/")) {
- return Error("Unexpected '/' in local image name");
- }
-
- // Create a remote Image::Name from the given input name.
- Image::Name remoteImageName = imageName;
- remoteImageName.set_repository("library/" + imageName.repository());
-
- return remoteImageName;
-}
-
-
Future<Nothing> untar(const string& file, const string& directory)
{
const vector<string> argv = {
@@ -352,36 +281,40 @@ Future<Nothing> untar(const string& file, const string& directory)
}
-Future<PulledImageInfo> RegistryPullerProcess::pull(
+Future<list<pair<string, string>>> RegistryPullerProcess::pull(
const Image::Name& imageName,
- const Path& downloadDir)
+ const Path& downloadDirectory)
{
- return _pull(imageName, downloadDir)
- .after(getTimeout(), [imageName](
- Future<PulledImageInfo> future) {
- LOG(WARNING) << "Failed to download image '"
- << imageName.repository()
- << "': timeout";
+ // TODO(jojy): Have one outgoing manifest request per image.
+ return registryClient_->getManifest(imageName)
+ .then(process::defer(self(), [this, downloadDirectory, imageName](
+ const Manifest& manifest) {
+ return downloadLayers(manifest, imageName, downloadDirectory);
+ }))
+ .then(process::defer(self(), [this, downloadDirectory](
+ const Future<list<pair<string, string>>>& layerFutures)
+ -> Future<list<pair<string, string>>> {
+ return untarLayers(layerFutures, downloadDirectory);
+ }))
+ .after(pullTimeout_, [imageName](
+ Future<list<pair<string, string>>> future) {
future.discard();
- return Failure(
- "Failed to download image '" + imageName.repository() + "': timeout");
+ return Failure("Timed out");
});
}
-Future<PulledImageInfo> RegistryPullerProcess::downloadLayers(
+Future<list<pair<string, string>>> RegistryPullerProcess::downloadLayers(
const Manifest& manifest,
- const Image::Name& remoteImageName,
- const Path& downloadDir)
+ const Image::Name& imageName,
+ const Path& downloadDirectory)
{
- list<Future<PulledLayerInfo>> downloadFutures;
+ list<Future<pair<string, string>>> downloadFutures;
foreach (const FileSystemLayerInfo& layer, manifest.fsLayerInfos) {
- downloadFutures.push_back(downloadLayer(
- remoteImageName.repository(),
- downloadDir,
- layer));
+ downloadFutures.push_back(
+ downloadLayer(imageName, downloadDirectory, layer));
}
// TODO(jojy): Delete downloaded files in the directory on discard and
@@ -391,21 +324,23 @@ Future<PulledImageInfo> RegistryPullerProcess::downloadLayers(
}
-Future<PulledImageInfo> RegistryPullerProcess::untarLayers(
- const Future<list<PulledLayerInfo>>& layerFutures,
- const Path& downloadDir)
+Future<list<pair<string, string>>> RegistryPullerProcess::untarLayers(
+ const Future<list<pair<string, string>>>& layerFutures,
+ const Path& downloadDirectory)
{
list<Future<Nothing>> untarFutures;
- foreach (const PulledLayerInfo layerInfo, layerFutures.get()) {
- untarFutures.emplace_back(untar(layerInfo.second, downloadDir));
+ pair<string, string> layerInfo;
+ foreach (layerInfo, layerFutures.get()) {
+ untarFutures.emplace_back(untar(layerInfo.second, downloadDirectory));
}
return collect(untarFutures)
.then([layerFutures]() {
- PulledImageInfo layers;
+ list<pair<string, string>> layers;
- foreach (const PulledLayerInfo layerInfo, layerFutures.get()) {
+ pair<string, string> layerInfo;
+ foreach (layerInfo, layerFutures.get()) {
layers.emplace_back(layerInfo);
}
@@ -413,30 +348,6 @@ Future<PulledImageInfo> RegistryPullerProcess::untarLayers(
});
}
-
-Future<PulledImageInfo> RegistryPullerProcess::_pull(
- const Image::Name& imageName,
- const Path& downloadDir)
-{
- Try<Image::Name> remoteImageName = getRemoteNameFromLocalName(imageName);
- if (remoteImageName.isError()) {
- return Failure(
- "Failed to get remote name from local name for '" +
- imageName.repository() + "': " + remoteImageName.error());
- }
-
- return registryClient_->getManifest(remoteImageName.get())
- .then(process::defer(self(), [this, downloadDir, remoteImageName](
- const Manifest& manifest) {
- return downloadLayers(manifest, remoteImageName.get(), downloadDir);
- }))
- .then(process::defer(self(), [this, downloadDir](
- const Future<list<PulledLayerInfo>>& layerFutures)
- -> Future<PulledImageInfo> {
- return untarLayers(layerFutures, downloadDir);
- }));
-}
-
} // namespace docker {
} // namespace slave {
} // namespace internal {
http://git-wip-us.apache.org/repos/asf/mesos/blob/57c87a53/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 7855a1d..2445c81 100644
--- a/src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp
+++ b/src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp
@@ -47,9 +47,6 @@ class RegistryPullerProcess;
class RegistryPuller : public Puller
{
public:
- typedef std::pair<std::string, std::string> PulledLayerInfo;
- typedef std::list<PulledLayerInfo> PulledImageInfo;
-
/**
* Factory method for creating RegistryPuller.
*/
@@ -64,7 +61,7 @@ public:
* @param imageName local name of the image.
* @param downloadDir 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);
http://git-wip-us.apache.org/repos/asf/mesos/blob/57c87a53/src/tests/containerizer/provisioner_docker_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/containerizer/provisioner_docker_tests.cpp b/src/tests/containerizer/provisioner_docker_tests.cpp
index 2808001..9227666 100644
--- a/src/tests/containerizer/provisioner_docker_tests.cpp
+++ b/src/tests/containerizer/provisioner_docker_tests.cpp
@@ -856,7 +856,7 @@ TEST_F(RegistryClientTest, SimpleGetBlob)
Future<size_t> result =
registryClient.get()->getBlob(
- "/blob",
+ parseImageName("blob"),
"digest",
blobPath);
@@ -961,7 +961,7 @@ TEST_F(RegistryClientTest, BadRequest)
Future<size_t> result =
registryClient.get()->getBlob(
- "/blob",
+ parseImageName("blob"),
"digest",
blobPath);