Refactor docker provisioner store to act as read-through cache. Review: https://reviews.apache.org/r/38040
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/f96e0dfc Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/f96e0dfc Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/f96e0dfc Branch: refs/heads/master Commit: f96e0dfc0d4a3f4100d3f9bc6c087dcd4c0b8c6a Parents: 19d787b Author: Timothy Chen <[email protected]> Authored: Tue Sep 1 15:02:36 2015 -0700 Committer: Timothy Chen <[email protected]> Committed: Fri Sep 25 09:02:05 2015 -0700 ---------------------------------------------------------------------- src/slave/containerizer/provisioners/docker.cpp | 61 ++++++++++---- src/slave/containerizer/provisioners/docker.hpp | 42 +--------- .../provisioners/docker/local_store.cpp | 84 +++++--------------- .../containerizer/provisioners/docker/store.hpp | 19 +---- src/tests/containerizer/provisioner.hpp | 8 +- 5 files changed, 72 insertions(+), 142 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/f96e0dfc/src/slave/containerizer/provisioners/docker.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/provisioners/docker.cpp b/src/slave/containerizer/provisioners/docker.cpp index 888f17a..bac29d3 100644 --- a/src/slave/containerizer/provisioners/docker.cpp +++ b/src/slave/containerizer/provisioners/docker.cpp @@ -47,6 +47,45 @@ namespace internal { namespace slave { namespace docker { +class DockerProvisionerProcess : + public process::Process<DockerProvisionerProcess> +{ +public: + static Try<process::Owned<DockerProvisionerProcess>> create( + const Flags& flags, + Fetcher* fetcher); + + process::Future<Nothing> recover( + const std::list<mesos::slave::ContainerState>& states, + const hashset<ContainerID>& orphans); + + process::Future<std::string> provision( + const ContainerID& containerId, + const Image& image); + + process::Future<bool> destroy(const ContainerID& containerId); + +private: + DockerProvisionerProcess( + const Flags& flags, + const process::Owned<Store>& store, + const process::Owned<mesos::internal::slave::Backend>& backend); + + process::Future<std::string> _provision( + const ContainerID& containerId, + const DockerImage& image); + + process::Future<DockerImage> fetch( + const std::string& name, + const std::string& sandbox); + + const Flags flags; + + process::Owned<Store> store; + process::Owned<mesos::internal::slave::Backend> backend; +}; + + ImageName::ImageName(const std::string& name) { registry = None(); @@ -107,15 +146,13 @@ Future<Nothing> DockerProvisioner::recover( Future<string> DockerProvisioner::provision( const ContainerID& containerId, - const Image& image, - const std::string& sandbox) + const Image& image) { return dispatch( process.get(), &DockerProvisionerProcess::provision, containerId, - image, - sandbox); + image); } @@ -173,8 +210,7 @@ Future<Nothing> DockerProvisionerProcess::recover( Future<string> DockerProvisionerProcess::provision( const ContainerID& containerId, - const Image& image, - const string& sandbox) + const Image& image) { if (image.type() != Image::DOCKER) { return Failure("Unsupported container image type"); @@ -184,7 +220,7 @@ Future<string> DockerProvisionerProcess::provision( return Failure("Missing Docker image info"); } - return fetch(image.docker().name(), sandbox) + return fetch(image.docker().name()) .then(defer(self(), &Self::_provision, containerId, @@ -234,16 +270,9 @@ Future<string> DockerProvisionerProcess::_provision( // Fetch an image and all dependencies. Future<DockerImage> DockerProvisionerProcess::fetch( - const string& name, - const string& sandbox) + const string& name) { - return store->get(name) - .then([=](const Option<DockerImage>& image) -> Future<DockerImage> { - if (image.isSome()) { - return image.get(); - } - return store->put(name, sandbox); - }); + return store->get(name); } http://git-wip-us.apache.org/repos/asf/mesos/blob/f96e0dfc/src/slave/containerizer/provisioners/docker.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/provisioners/docker.hpp b/src/slave/containerizer/provisioners/docker.hpp index 9cca662..cda83cb 100644 --- a/src/slave/containerizer/provisioners/docker.hpp +++ b/src/slave/containerizer/provisioners/docker.hpp @@ -112,8 +112,7 @@ public: virtual process::Future<std::string> provision( const ContainerID& containerId, - const Image& image, - const std::string& sandbox); + const Image& image); virtual process::Future<bool> destroy(const ContainerID& containerId); @@ -126,45 +125,6 @@ private: }; -class DockerProvisionerProcess : - public process::Process<DockerProvisionerProcess> -{ -public: - static Try<process::Owned<DockerProvisionerProcess>> create( - const Flags& flags, - Fetcher* fetcher); - - process::Future<Nothing> recover( - const std::list<mesos::slave::ContainerState>& states, - const hashset<ContainerID>& orphans); - - process::Future<std::string> provision( - const ContainerID& containerId, - const Image& image, - const std::string& sandbox); - - process::Future<bool> destroy(const ContainerID& containerId); - -private: - DockerProvisionerProcess( - const Flags& flags, - const process::Owned<Store>& store, - const process::Owned<mesos::internal::slave::Backend>& backend); - - process::Future<std::string> _provision( - const ContainerID& containerId, - const DockerImage& image); - - process::Future<DockerImage> fetch( - const std::string& name, - const std::string& sandbox); - - const Flags flags; - - process::Owned<Store> store; - process::Owned<mesos::internal::slave::Backend> backend; -}; - } // namespace docker { } // namespace slave { } // namespace internal { http://git-wip-us.apache.org/repos/asf/mesos/blob/f96e0dfc/src/slave/containerizer/provisioners/docker/local_store.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/provisioners/docker/local_store.cpp b/src/slave/containerizer/provisioners/docker/local_store.cpp index d46f4bc..58af655 100644 --- a/src/slave/containerizer/provisioners/docker/local_store.cpp +++ b/src/slave/containerizer/provisioners/docker/local_store.cpp @@ -60,11 +60,7 @@ public: const Flags& flags, Fetcher* fetcher); - process::Future<DockerImage> put( - const std::string& name, - const std::string& sandbox); - - process::Future<Option<DockerImage>> get(const std::string& name); + process::Future<DockerImage> get(const std::string& name); private: LocalStoreProcess(const Flags& flags); @@ -75,8 +71,7 @@ private: process::Future<DockerImage> putImage( const std::string& name, - const std::string& staging, - const std::string& sandbox); + const std::string& staging) Result<std::string> getParentId( const std::string& staging, @@ -84,18 +79,15 @@ private: process::Future<Nothing> putLayers( const std::string& staging, - const std::list<std::string>& layers, - const std::string& sandbox); + const std::list<std::string>& layers) process::Future<Nothing> untarLayer( const std::string& staging, - const std::string& id, - const std::string& sandbox); + const std::string& id) process::Future<Nothing> moveLayer( const std::string& staging, - const std::string& id, - const std::string& sandbox); + const std::string& id) const Flags flags; @@ -147,14 +139,6 @@ LocalStore::~LocalStore() } -Future<DockerImage> LocalStore::put( - const string& name, - const string& sandbox) -{ - return dispatch(process.get(), &LocalStoreProcess::put, name, sandbox); -} - - Future<Option<DockerImage>> LocalStore::get(const string& name) { return dispatch(process.get(), &LocalStoreProcess::get, name); @@ -173,10 +157,13 @@ LocalStoreProcess::LocalStoreProcess(const Flags& flags) : flags(flags), refStore(ReferenceStore::create(flags).get()) {} -Future<DockerImage> LocalStoreProcess::put( - const string& name, - const string& sandbox) +Future<DockerImage> LocalStoreProcess::get(const string& name) { + Option<DockerImage> image = refStore->get(name); + if (image.isSome()) { + return image.get(); + } + string tarPath = paths::getLocalImageTarPath(flags.docker_discovery_local_dir, name); if (!os::exists(tarPath)) { @@ -195,7 +182,7 @@ Future<DockerImage> LocalStoreProcess::put( } return untarImage(tarPath, staging.get()) - .then(defer(self(), &Self::putImage, name, staging.get(), sandbox)); + .then(defer(self(), &Self::putImage, name, staging.get())); } @@ -244,8 +231,7 @@ Future<Nothing> LocalStoreProcess::untarImage( Future<DockerImage> LocalStoreProcess::putImage( const std::string& name, - const string& staging, - const string& sandbox) + const string& staging) { ImageName imageName(name); @@ -302,7 +288,7 @@ Future<DockerImage> LocalStoreProcess::putImage( return Failure("Failed to obtain parent layer id: " + parentId.error()); } - return putLayers(staging, layers, sandbox) + return putLayers(staging, layers) .then([=]() -> Future<DockerImage> { return refStore->put(name, layers); }); @@ -336,14 +322,13 @@ Result<string> LocalStoreProcess::getParentId( Future<Nothing> LocalStoreProcess::putLayers( const string& staging, - const list<string>& layers, - const string& sandbox) + const list<string>& layers) { list<Future<Nothing>> futures{ Nothing() }; foreach (const string& layer, layers) { futures.push_back( futures.back().then( - defer(self(), &Self::untarLayer, staging, layer, sandbox))); + defer(self(), &Self::untarLayer, staging, layer))); } return collect(futures) @@ -353,8 +338,7 @@ Future<Nothing> LocalStoreProcess::putLayers( Future<Nothing> LocalStoreProcess::untarLayer( const string& staging, - const string& id, - const string& sandbox) + const string& id) { // Check if image layer is already in store. if (os::exists(paths::getImageLayerPath(flags.docker_store_dir, id))) { @@ -368,7 +352,7 @@ Future<Nothing> LocalStoreProcess::untarLayer( if (os::exists(localRootfsPath)) { LOG(WARNING) << "Image layer rootfs present at but not in store directory: " << localRootfsPath << "Skipping untarLayer."; - return moveLayer(staging, id, sandbox); + return moveLayer(staging, id); } os::mkdir(localRootfsPath); @@ -401,36 +385,15 @@ Future<Nothing> LocalStoreProcess::untarLayer( WSTRINGIFY(status.get())); } - return moveLayer(staging, id, sandbox); + return moveLayer(staging, id); }); } Future<Nothing> LocalStoreProcess::moveLayer( const string& staging, - const string& id, - const string& sandbox){ - - Try<int> out = os::open( - path::join(sandbox, "stdout"), - O_WRONLY | O_CREAT | O_TRUNC | O_NONBLOCK | O_CLOEXEC, - S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); - - if (out.isError()) { - return Failure("Failed to create 'stdout' file: " + out.error()); - } - - // Repeat for stderr. - Try<int> err = os::open( - path::join(sandbox, "stderr"), - O_WRONLY | O_CREAT | O_TRUNC | O_NONBLOCK | O_CLOEXEC, - S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); - - if (err.isError()) { - os::close(out.get()); - return Failure("Failed to create 'stderr' file: " + err.error()); - } - + const string& id) +{ if (!os::exists(flags.docker_store_dir)) { VLOG(1) << "Creating docker store directory"; os::mkdir(flags.docker_store_dir); @@ -452,11 +415,6 @@ Future<Nothing> LocalStoreProcess::moveLayer( } -Future<Option<DockerImage>> LocalStoreProcess::get(const string& name) -{ - return refStore->get(name); -} - } // namespace docker { } // namespace slave { } // namespace internal { http://git-wip-us.apache.org/repos/asf/mesos/blob/f96e0dfc/src/slave/containerizer/provisioners/docker/store.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/provisioners/docker/store.hpp b/src/slave/containerizer/provisioners/docker/store.hpp index 2eda083..0520a2c 100644 --- a/src/slave/containerizer/provisioners/docker/store.hpp +++ b/src/slave/containerizer/provisioners/docker/store.hpp @@ -49,28 +49,13 @@ public: virtual ~Store() {} /** - * Put an image into the store. Returns the DockerImage containing - * the manifest, hash of the image, and the path to the extracted - * image. - * - * @param name The name of the Docker image being stored. - * @param sandbox The path of the directory in which the stderr and - * stdout logs will be placed. - * - * @return The DockerImage placed in the store. - */ - virtual process::Future<DockerImage> put( - const std::string& name, - const std::string& sandbox) = 0; - - /** * Get image by name. * * @param name The name of the Docker image to retrieve from store. * - * @return The DockerImage or none if image is not in the store. + * @return The DockerImage that holds the Docker layers. */ - virtual process::Future<Option<DockerImage>> get(const std::string& name) = 0; + virtual process::Future<DockerImage> get(const std::string& name) = 0; // TODO(chenlily): Implement removing an image. http://git-wip-us.apache.org/repos/asf/mesos/blob/f96e0dfc/src/tests/containerizer/provisioner.hpp ---------------------------------------------------------------------- diff --git a/src/tests/containerizer/provisioner.hpp b/src/tests/containerizer/provisioner.hpp index c01a441..54aab5f 100644 --- a/src/tests/containerizer/provisioner.hpp +++ b/src/tests/containerizer/provisioner.hpp @@ -67,12 +67,11 @@ public: const std::list<mesos::slave::ContainerState>& states, const hashset<ContainerID>& orphans)); - MOCK_METHOD3( + MOCK_METHOD2( provision, process::Future<std::string>( const ContainerID& containerId, - const Image& image, - const std::string& sandbox)); + const Image& image)); MOCK_METHOD1( destroy, @@ -88,8 +87,7 @@ public: process::Future<std::string> unmocked_provision( const ContainerID& containerId, - const Image& image, - const std::string& sandbox) + const Image& image) { if (image.type() != Image::APPC) { return process::Failure(
