This is an automated email from the ASF dual-hosted git repository.
jpeach pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
The following commit(s) were added to refs/heads/master by this push:
new bc5a571 Adopted container file operations for secrets volumes.
bc5a571 is described below
commit bc5a57122635700f3943d73f1df13820aef202e2
Author: James Peach <[email protected]>
AuthorDate: Thu Jun 6 13:18:18 2019 -0700
Adopted container file operations for secrets volumes.
Switched the `volumes/secrets` isolator from using container
pre-exec commands to using file operations. This allows us to
reduce the number of forked child processes, consolidate code
and leaves the `network/port_mapping` isolator as the last
consumer of pre-exec commands.
Review: https://reviews.apache.org/r/70741/
---
include/mesos/slave/containerizer.proto | 23 ++++++
src/common/protobuf_utils.cpp | 40 ++++++++++
src/common/protobuf_utils.hpp | 14 ++++
.../mesos/isolators/volume/secret.cpp | 64 ++++++----------
src/slave/containerizer/mesos/launch.cpp | 87 ++++++++++++++++++----
5 files changed, 172 insertions(+), 56 deletions(-)
diff --git a/include/mesos/slave/containerizer.proto
b/include/mesos/slave/containerizer.proto
index b2e35cb..2d04f3c 100644
--- a/include/mesos/slave/containerizer.proto
+++ b/include/mesos/slave/containerizer.proto
@@ -200,6 +200,16 @@ message ContainerFileOperation {
enum Operation {
// Create a symlink that points to `source` at `target`.
SYMLINK = 1;
+
+ // Perform a container mount. This can be used when a mount is part
+ // of a sequence of steps involving other filesystem operations.
+ MOUNT = 2;
+
+ // Rename a path;
+ RENAME = 3;
+
+ // Create a directory.
+ MKDIR = 4;
};
message Symlink {
@@ -207,10 +217,23 @@ message ContainerFileOperation {
optional string target = 2;
}
+ message Rename {
+ optional string source = 1;
+ optional string target = 2;
+ }
+
+ message Mkdir {
+ optional string target = 1;
+ optional bool recursive = 2;
+ }
+
optional Operation operation = 1;
oneof parameters {
Symlink symlink = 2;
+ Mkdir mkdir = 3;
+ Rename rename = 4;
+ ContainerMountInfo mount = 5;
}
}
diff --git a/src/common/protobuf_utils.cpp b/src/common/protobuf_utils.cpp
index 870859d..9ff0cf5 100644
--- a/src/common/protobuf_utils.cpp
+++ b/src/common/protobuf_utils.cpp
@@ -1256,6 +1256,46 @@ mesos::slave::ContainerFileOperation
containerSymlinkOperation(
return op;
}
+
+mesos::slave::ContainerFileOperation containerRenameOperation(
+ const std::string& source,
+ const std::string& target)
+{
+ ContainerFileOperation op;
+
+ op.set_operation(ContainerFileOperation::RENAME);
+ op.mutable_rename()->set_source(source);
+ op.mutable_rename()->set_target(target);
+
+ return op;
+}
+
+
+mesos::slave::ContainerFileOperation containerMkdirOperation(
+ const std::string& target,
+ const bool recursive)
+{
+ ContainerFileOperation op;
+
+ op.set_operation(ContainerFileOperation::MKDIR);
+ op.mutable_mkdir()->set_target(target);
+ op.mutable_mkdir()->set_recursive(recursive);
+
+ return op;
+}
+
+
+mesos::slave::ContainerFileOperation containerMountOperation(
+ const ContainerMountInfo& mnt)
+{
+ ContainerFileOperation op;
+
+ op.set_operation(ContainerFileOperation::MOUNT);
+ *op.mutable_mount() = mnt;
+
+ return op;
+}
+
} // namespace slave {
namespace maintenance {
diff --git a/src/common/protobuf_utils.hpp b/src/common/protobuf_utils.hpp
index 8aa9aff..ecaf8ea 100644
--- a/src/common/protobuf_utils.hpp
+++ b/src/common/protobuf_utils.hpp
@@ -419,6 +419,20 @@ mesos::slave::ContainerFileOperation
containerSymlinkOperation(
const std::string& source,
const std::string& target);
+
+mesos::slave::ContainerFileOperation containerRenameOperation(
+ const std::string& source,
+ const std::string& target);
+
+
+mesos::slave::ContainerFileOperation containerMkdirOperation(
+ const std::string& target,
+ const bool recursive);
+
+
+mesos::slave::ContainerFileOperation containerMountOperation(
+ const mesos::slave::ContainerMountInfo& mnt);
+
} // namespace slave {
namespace maintenance {
diff --git a/src/slave/containerizer/mesos/isolators/volume/secret.cpp
b/src/slave/containerizer/mesos/isolators/volume/secret.cpp
index 7a9bb82..4bbcc7a 100644
--- a/src/slave/containerizer/mesos/isolators/volume/secret.cpp
+++ b/src/slave/containerizer/mesos/isolators/volume/secret.cpp
@@ -16,6 +16,8 @@
#include "slave/containerizer/mesos/isolators/volume/secret.hpp"
+#include <sys/mount.h>
+
#include <string>
#include <vector>
@@ -38,6 +40,7 @@
#include "linux/ns.hpp"
#endif // __linux__
+#include "common/protobuf_utils.hpp"
#include "common/validation.hpp"
using std::string;
@@ -47,12 +50,18 @@ using process::Failure;
using process::Future;
using process::Owned;
+using mesos::internal::protobuf::slave::containerMkdirOperation;
+using mesos::internal::protobuf::slave::containerMountOperation;
+using mesos::internal::protobuf::slave::containerRenameOperation;
+using mesos::internal::protobuf::slave::createContainerMount;
+
using mesos::slave::ContainerClass;
using mesos::slave::ContainerConfig;
using mesos::slave::ContainerLaunchInfo;
using mesos::slave::ContainerState;
using mesos::slave::Isolator;
+
namespace mesos {
namespace internal {
namespace slave {
@@ -135,15 +144,8 @@ Future<Option<ContainerLaunchInfo>>
VolumeSecretIsolatorProcess::prepare(
}
// Mount ramfs in the container.
- CommandInfo* command = launchInfo.add_pre_exec_commands();
- command->set_shell(false);
- command->set_value("mount");
- command->add_arguments("mount");
- command->add_arguments("-n");
- command->add_arguments("-t");
- command->add_arguments("ramfs");
- command->add_arguments("ramfs");
- command->add_arguments(sandboxSecretRootDir);
+ *launchInfo.add_file_operations() = containerMountOperation(
+ createContainerMount("ramfs", sandboxSecretRootDir, "ramfs", 0));
vector<Future<Nothing>> futures;
foreach (const Volume& volume, containerInfo.volumes()) {
@@ -250,44 +252,20 @@ Future<Option<ContainerLaunchInfo>>
VolumeSecretIsolatorProcess::prepare(
}
// Create directory tree inside sandbox secret root dir.
- command = launchInfo.add_pre_exec_commands();
- command->set_shell(false);
- command->set_value("mkdir");
- command->add_arguments("mkdir");
- command->add_arguments("-p");
- command->add_arguments(Path(sandboxSecretPath).dirname());
+ *launchInfo.add_file_operations() = containerMkdirOperation(
+ Path(sandboxSecretPath).dirname(), true /* recursive */);
// Move secret from hostSecretPath to sandboxSecretPath.
- command = launchInfo.add_pre_exec_commands();
- command->set_shell(false);
- command->set_value("mv");
- command->add_arguments("mv");
- command->add_arguments("-f");
- command->add_arguments(hostSecretPath);
- command->add_arguments(sandboxSecretPath);
+ *launchInfo.add_file_operations() = containerRenameOperation(
+ hostSecretPath, sandboxSecretPath);
+
+ const unsigned long flags =
+ volume.mode() == Volume::RO ? (MS_BIND | MS_REC | MS_RDONLY)
+ : (MS_BIND | MS_REC);
// Bind mount sandboxSecretPath to targetContainerPath
- command = launchInfo.add_pre_exec_commands();
- command->set_shell(false);
- command->set_value("mount");
- command->add_arguments("mount");
- command->add_arguments("-n");
- command->add_arguments("--rbind");
- command->add_arguments(sandboxSecretPath);
- command->add_arguments(targetContainerPath);
-
- // If the mount needs to be read-only, do a remount.
- if (volume.mode() == Volume::RO) {
- command = launchInfo.add_pre_exec_commands();
- command->set_shell(false);
- command->set_value("mount");
- command->add_arguments("mount");
- command->add_arguments("-n");
- command->add_arguments("-o");
- command->add_arguments("bind,ro,remount");
- command->add_arguments(sandboxSecretPath);
- command->add_arguments(targetContainerPath);
- }
+ *launchInfo.add_file_operations() = containerMountOperation(
+ createContainerMount(sandboxSecretPath, targetContainerPath, flags));
Future<Nothing> future = secretResolver->resolve(secret)
.then([hostSecretPath](const Secret::Value& value) -> Future<Nothing> {
diff --git a/src/slave/containerizer/mesos/launch.cpp
b/src/slave/containerizer/mesos/launch.cpp
index 0c482f4..a69a688 100644
--- a/src/slave/containerizer/mesos/launch.cpp
+++ b/src/slave/containerizer/mesos/launch.cpp
@@ -259,6 +259,19 @@ static void exitWithStatus(int status)
}
+#ifdef __linux__
+static Try<Nothing> mountContainerFilesystem(const ContainerMountInfo& mount)
+{
+ return fs::mount(
+ mount.has_source() ? Option<string>(mount.source()) : None(),
+ mount.target(),
+ mount.has_type() ? Option<string>(mount.type()) : None(),
+ mount.has_flags() ? mount.flags() : 0,
+ mount.has_options() ? Option<string>(mount.options()) : None());
+}
+#endif // __linux__
+
+
static Try<Nothing> prepareMounts(const ContainerLaunchInfo& launchInfo)
{
#ifdef __linux__
@@ -435,13 +448,7 @@ static Try<Nothing> prepareMounts(const
ContainerLaunchInfo& launchInfo)
}
}
- Try<Nothing> mnt = fs::mount(
- (mount.has_source() ? Option<string>(mount.source()) : None()),
- mount.target(),
- (mount.has_type() ? Option<string>(mount.type()) : None()),
- (mount.has_flags() ? mount.flags() : 0),
- (mount.has_options() ? Option<string>(mount.options()) : None()));
-
+ Try<Nothing> mnt = mountContainerFilesystem(mount);
if (mnt.isError()) {
return Error(
"Failed to mount '" + stringify(JSON::protobuf(mount)) +
@@ -476,20 +483,74 @@ static Try<Nothing> maskPath(const string& target)
static Try<Nothing> executeFileOperation(const ContainerFileOperation& op)
{
- Try<Nothing> result = Nothing();
-
switch (op.operation()) {
- case ContainerFileOperation::SYMLINK:
- result = ::fs::symlink(op.symlink().source(), op.symlink().target());
+ case ContainerFileOperation::SYMLINK: {
+ Try<Nothing> result =
+ ::fs::symlink(op.symlink().source(), op.symlink().target());
if (result.isError()) {
return Error(
"Failed to link '" + op.symlink().source() + "' as '" +
op.symlink().target() + "': " + result.error());
}
- break;
+
+ return Nothing();
+ }
+
+#ifdef __linux__
+ case ContainerFileOperation::MOUNT: {
+ Try<Nothing> result = mountContainerFilesystem(op.mount());
+ if (result.isError()) {
+ return Error(
+ "Failed to mount '" + stringify(JSON::protobuf(op.mount())) +
+ "': " + result.error());
+ }
+
+ return Nothing();
+ }
+#endif // __linux__
+
+ case ContainerFileOperation::RENAME: {
+ Try<Nothing> result =
+ os::rename(op.rename().source(), op.rename().target());
+
+ // TODO(jpeach): We should only fall back to `mv` if the error
+ // is EXDEV, in which case a rename is a copy+unlink.
+ if (result.isError()) {
+ Option<int> status = os::spawn(
+ "mv", {"mv", "-f", op.rename().source(), op.rename().target()});
+
+ if (status.isNone()) {
+ return Error(
+ "Failed to rename '" + op.rename().source() + "' to '" +
+ op.rename().target() + "': spawn failed");
+ }
+
+ if (!WSUCCEEDED(status.get())) {
+ return Error(
+ "Failed to rename '" + op.rename().source() + "' to '" +
+ op.rename().target() + "': " + WSTRINGIFY(status.get()));
+ }
+
+ result = Nothing();
+ }
+
+ return Nothing();
}
- return result;
+ case ContainerFileOperation::MKDIR: {
+ Try<Nothing> result =
+ os::mkdir(op.mkdir().target(), op.mkdir().recursive());
+ if (result.isError()) {
+ return Error(
+ "Failed to create directory '" + op.mkdir().target() + "': " +
+ result.error());
+ }
+
+ return result;
+ }
+ }
+
+ return Nothing();
}
