You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ch...@apache.org on 2018/10/31 18:28:18 UTC
[mesos] branch master updated (b5d884b -> 002acd7)
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git.
from b5d884b Included corresponding header file first.
new 5a10007 Stout: Added a sync option for `write` and `rename`.
new 80fa731 Stout: Added a sync option for `mkdir`.
new 16bcf61 Synced SLRP checkpoints to the filesystem.
new 6e15ff3 Added MESOS-9281 to the 1.7.1 CHANGELOG.
new 6448d82 Fixed outdated comments for mocking the secret generator.
new 002acd7 Added a comment for `Resource.provider_id`.
The 6 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:
3rdparty/stout/include/stout/os/posix/fsync.hpp | 27 +++++-
3rdparty/stout/include/stout/os/posix/mkdir.hpp | 40 +++++++-
3rdparty/stout/include/stout/os/posix/rename.hpp | 37 +++++++-
3rdparty/stout/include/stout/os/windows/mkdir.hpp | 7 +-
3rdparty/stout/include/stout/os/windows/rename.hpp | 10 +-
3rdparty/stout/include/stout/os/write.hpp | 31 +++++--
3rdparty/stout/include/stout/protobuf.hpp | 29 +++++-
CHANGELOG | 1 +
include/mesos/mesos.proto | 1 +
include/mesos/v1/mesos.proto | 1 +
src/resource_provider/storage/provider.cpp | 102 +++++++++++++--------
src/slave/state.hpp | 34 ++++---
src/tests/cluster.cpp | 2 +-
src/tests/slave_authorization_tests.cpp | 1 -
src/tests/slave_tests.cpp | 1 -
15 files changed, 243 insertions(+), 81 deletions(-)
[mesos] 03/06: Synced SLRP checkpoints to the filesystem.
Posted by ch...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 16bcf61231b6d14019b1d703d887c55b01b85aee
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Fri Oct 12 15:12:41 2018 -0700
Synced SLRP checkpoints to the filesystem.
Currently if a system crashes, SLRP checkpoints might not be synced to
the filesystem, so it is possible that an old or empty checkpoint will
be read upon recovery. Moreover, if a CSI call has been issued right
before the crash, the recovered state may be inconsistent with the
actual state reported by the plugin. For example, the plugin might have
created a volume but the checkpointed state does not know about it.
To avoid this inconsistency, we always call fsync() when checkpointing
SLRP states.
Review: https://reviews.apache.org/r/69010
---
src/resource_provider/storage/provider.cpp | 102 ++++++++++++++++++-----------
src/slave/state.hpp | 34 ++++++----
2 files changed, 83 insertions(+), 53 deletions(-)
diff --git a/src/resource_provider/storage/provider.cpp b/src/resource_provider/storage/provider.cpp
index db783b5..025b13b 100644
--- a/src/resource_provider/storage/provider.cpp
+++ b/src/resource_provider/storage/provider.cpp
@@ -21,6 +21,7 @@
#include <memory>
#include <numeric>
#include <utility>
+#include <vector>
#include <glog/logging.h>
@@ -28,6 +29,7 @@
#include <process/collect.hpp>
#include <process/defer.hpp>
#include <process/delay.hpp>
+#include <process/dispatch.hpp>
#include <process/id.hpp>
#include <process/loop.hpp>
#include <process/process.hpp>
@@ -88,23 +90,24 @@ using std::shared_ptr;
using std::string;
using std::vector;
+using process::after;
+using process::await;
using process::Break;
+using process::collect;
using process::Continue;
using process::ControlFlow;
+using process::defer;
+using process::delay;
+using process::dispatch;
using process::Failure;
using process::Future;
+using process::loop;
using process::Owned;
using process::Process;
using process::Promise;
using process::Sequence;
-using process::Timeout;
-
-using process::after;
-using process::await;
-using process::collect;
-using process::defer;
-using process::loop;
using process::spawn;
+using process::Timeout;
using process::http::authentication::Principal;
@@ -425,6 +428,8 @@ private:
const id::UUID& operationUuid,
const Try<vector<ResourceConversion>>& conversions);
+ void garbageCollectOperationPath(const id::UUID& operationUuid);
+
void checkpointResourceProviderState();
void checkpointVolumeState(const string& volumeId);
@@ -1154,7 +1159,16 @@ StorageLocalResourceProviderProcess::reconcileOperationStatuses()
uuid.error());
}
- CHECK(operations.contains(uuid.get()));
+ // NOTE: This could happen if we failed to remove the operation path before.
+ if (!operations.contains(uuid.get())) {
+ LOG(WARNING)
+ << "Ignoring unknown operation (uuid: " << uuid.get()
+ << ") for resource provider " << info.id();
+
+ garbageCollectOperationPath(uuid.get());
+ continue;
+ }
+
operationUuids.emplace_back(std::move(uuid.get()));
}
@@ -1165,27 +1179,23 @@ StorageLocalResourceProviderProcess::reconcileOperationStatuses()
using StreamState =
typename OperationStatusUpdateManagerState::StreamState;
- // Clean up the operations that are terminated.
+ // Clean up the operations that are completed.
+ vector<id::UUID> completedOperations;
foreachpair (const id::UUID& uuid,
const Option<StreamState>& stream,
statusUpdateManagerState.streams) {
if (stream.isSome() && stream->terminated) {
operations.erase(uuid);
-
- // Garbage collect the operation metadata.
- const string path = slave::paths::getOperationPath(
- slave::paths::getResourceProviderPath(
- metaDir, slaveId, info.type(), info.name(), info.id()),
- uuid);
-
- Try<Nothing> rmdir = os::rmdir(path);
- if (rmdir.isError()) {
- return Failure(
- "Failed to remove directory '" + path + "': " + rmdir.error());
- }
+ completedOperations.push_back(uuid);
}
}
+ // Garbage collect the operation streams after checkpointing.
+ checkpointResourceProviderState();
+ foreach (const id::UUID& uuid, completedOperations) {
+ garbageCollectOperationPath(uuid);
+ }
+
// Send updates for all missing statuses.
foreachpair (const id::UUID& uuid,
const Operation& operation,
@@ -1790,25 +1800,11 @@ void StorageLocalResourceProviderProcess::acknowledgeOperationStatus(
// acknowledgement will be received. In this case, the following call
// will fail, so we just leave an error log.
statusUpdateManager.acknowledgement(operationUuid.get(), statusUuid.get())
- .then(defer(self(), [=](bool continuation) -> Future<Nothing> {
+ .then(defer(self(), [=](bool continuation) {
if (!continuation) {
operations.erase(operationUuid.get());
-
- // Garbage collect the operation metadata.
- const string path = slave::paths::getOperationPath(
- slave::paths::getResourceProviderPath(
- metaDir, slaveId, info.type(), info.name(), info.id()),
- operationUuid.get());
-
- // NOTE: We check if the path exists since we do not checkpoint
- // some status updates, such as OPERATION_DROPPED.
- if (os::exists(path)) {
- Try<Nothing> rmdir = os::rmdir(path);
- if (rmdir.isError()) {
- return Failure(
- "Failed to remove directory '" + path + "': " + rmdir.error());
- }
- }
+ checkpointResourceProviderState();
+ garbageCollectOperationPath(operationUuid.get());
}
return Nothing();
@@ -3436,6 +3432,28 @@ Try<Nothing> StorageLocalResourceProviderProcess::updateOperationStatus(
}
+void StorageLocalResourceProviderProcess::garbageCollectOperationPath(
+ const id::UUID& operationUuid)
+{
+ CHECK(!operations.contains(operationUuid));
+
+ const string path = slave::paths::getOperationPath(
+ slave::paths::getResourceProviderPath(
+ metaDir, slaveId, info.type(), info.name(), info.id()),
+ operationUuid);
+
+ // NOTE: We check if the path exists since we do not checkpoint some status
+ // updates, such as OPERATION_DROPPED.
+ if (os::exists(path)) {
+ Try<Nothing> rmdir = os::rmdir(path);
+ if (rmdir.isError()) {
+ LOG(ERROR)
+ << "Failed to remove directory '" << path << "': " << rmdir.error();
+ }
+ }
+}
+
+
void StorageLocalResourceProviderProcess::checkpointResourceProviderState()
{
ResourceProviderState state;
@@ -3476,7 +3494,9 @@ void StorageLocalResourceProviderProcess::checkpointResourceProviderState()
const string statePath = slave::paths::getResourceProviderStatePath(
metaDir, slaveId, info.type(), info.name(), info.id());
- Try<Nothing> checkpoint = slave::state::checkpoint(statePath, state);
+ // NOTE: We ensure the checkpoint is synced to the filesystem to avoid
+ // resulting in a stale or empty checkpoint when a system crash happens.
+ Try<Nothing> checkpoint = slave::state::checkpoint(statePath, state, true);
CHECK_SOME(checkpoint)
<< "Failed to checkpoint resource provider state to '" << statePath << "': "
<< checkpoint.error();
@@ -3492,8 +3512,10 @@ void StorageLocalResourceProviderProcess::checkpointVolumeState(
info.storage().plugin().name(),
volumeId);
+ // NOTE: We ensure the checkpoint is synced to the filesystem to avoid
+ // resulting in a stale or empty checkpoint when a system crash happens.
Try<Nothing> checkpoint =
- slave::state::checkpoint(statePath, volumes.at(volumeId).state);
+ slave::state::checkpoint(statePath, volumes.at(volumeId).state, true);
CHECK_SOME(checkpoint)
<< "Failed to checkpoint volume state to '" << statePath << "':"
diff --git a/src/slave/state.hpp b/src/slave/state.hpp
index 003211e..4f3d4ce 100644
--- a/src/slave/state.hpp
+++ b/src/slave/state.hpp
@@ -122,9 +122,10 @@ namespace internal {
inline Try<Nothing> checkpoint(
const std::string& path,
- const std::string& message)
+ const std::string& message,
+ bool sync)
{
- return ::os::write(path, message);
+ return ::os::write(path, message, sync);
}
@@ -133,7 +134,7 @@ template <
typename std::enable_if<
std::is_convertible<T*, google::protobuf::Message*>::value,
int>::type = 0>
-inline Try<Nothing> checkpoint(const std::string& path, T message)
+inline Try<Nothing> checkpoint(const std::string& path, T message, bool sync)
{
// If the `Try` from `downgradeResources` returns an `Error`, we currently
// continue to checkpoint the resources in a partially downgraded state.
@@ -144,13 +145,14 @@ inline Try<Nothing> checkpoint(const std::string& path, T message)
// TODO(mpark): Do something smarter with the result once
// something like an agent recovery capability is introduced.
downgradeResources(&message);
- return ::protobuf::write(path, message);
+ return ::protobuf::write(path, message, sync);
}
inline Try<Nothing> checkpoint(
const std::string& path,
- google::protobuf::RepeatedPtrField<Resource> resources)
+ google::protobuf::RepeatedPtrField<Resource> resources,
+ bool sync)
{
// If the `Try` from `downgradeResources` returns an `Error`, we currently
// continue to checkpoint the resources in a partially downgraded state.
@@ -161,16 +163,17 @@ inline Try<Nothing> checkpoint(
// TODO(mpark): Do something smarter with the result once
// something like an agent recovery capability is introduced.
downgradeResources(&resources);
- return ::protobuf::write(path, resources);
+ return ::protobuf::write(path, resources, sync);
}
inline Try<Nothing> checkpoint(
const std::string& path,
- const Resources& resources)
+ const Resources& resources,
+ bool sync)
{
const google::protobuf::RepeatedPtrField<Resource>& messages = resources;
- return checkpoint(path, messages);
+ return checkpoint(path, messages, sync);
}
} // namespace internal {
@@ -187,14 +190,19 @@ inline Try<Nothing> checkpoint(
//
// NOTE: We provide atomic (all-or-nothing) semantics here by always
// writing to a temporary file first then using os::rename to atomically
-// move it to the desired path.
+// move it to the desired path. If `sync` is set to true, this call succeeds
+// only if `fsync` is supported and successfully commits the changes to the
+// filesystem for the checkpoint file and each created directory.
+//
+// TODO(chhsiao): Consider enabling syncing by default after evaluating its
+// performance impact.
template <typename T>
-Try<Nothing> checkpoint(const std::string& path, const T& t)
+Try<Nothing> checkpoint(const std::string& path, const T& t, bool sync = false)
{
// Create the base directory.
std::string base = Path(path).dirname();
- Try<Nothing> mkdir = os::mkdir(base);
+ Try<Nothing> mkdir = os::mkdir(base, true, sync);
if (mkdir.isError()) {
return Error("Failed to create directory '" + base + "': " + mkdir.error());
}
@@ -211,7 +219,7 @@ Try<Nothing> checkpoint(const std::string& path, const T& t)
}
// Now checkpoint the instance of T to the temporary file.
- Try<Nothing> checkpoint = internal::checkpoint(temp.get(), t);
+ Try<Nothing> checkpoint = internal::checkpoint(temp.get(), t, sync);
if (checkpoint.isError()) {
// Try removing the temporary file on error.
os::rm(temp.get());
@@ -221,7 +229,7 @@ Try<Nothing> checkpoint(const std::string& path, const T& t)
}
// Rename the temporary file to the path.
- Try<Nothing> rename = os::rename(temp.get(), path);
+ Try<Nothing> rename = os::rename(temp.get(), path, sync);
if (rename.isError()) {
// Try removing the temporary file on error.
os::rm(temp.get());
[mesos] 05/06: Fixed outdated comments for mocking the secret
generator.
Posted by ch...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 6448d82bfea4398456e5b5cb744288f28077ca96
Author: Chun-Hung Hsiao <ch...@apache.org>
AuthorDate: Wed Oct 31 11:16:52 2018 -0700
Fixed outdated comments for mocking the secret generator.
Review: https://reviews.apache.org/r/68806/
---
src/tests/cluster.cpp | 2 +-
src/tests/slave_authorization_tests.cpp | 1 -
src/tests/slave_tests.cpp | 1 -
3 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/tests/cluster.cpp b/src/tests/cluster.cpp
index a7226c7..2b351ca 100644
--- a/src/tests/cluster.cpp
+++ b/src/tests/cluster.cpp
@@ -535,7 +535,7 @@ Try<process::Owned<Slave>> Slave::create(
slave->qosController.reset(_qosController.get());
}
- // If the QoS controller is not provided, create a default one.
+ // If the secret generator is not provided, create a default one.
if (secretGenerator.isNone()) {
SecretGenerator* _secretGenerator = nullptr;
diff --git a/src/tests/slave_authorization_tests.cpp b/src/tests/slave_authorization_tests.cpp
index 061e230..efd2248 100644
--- a/src/tests/slave_authorization_tests.cpp
+++ b/src/tests/slave_authorization_tests.cpp
@@ -801,7 +801,6 @@ TEST_F(ExecutorAuthorizationTest, FailedSubscribe)
Owned<TestContainerizer> containerizer(
new TestContainerizer(devolve(executorInfo.executor_id()), executor));
- // This pointer is passed to the agent, which will perform the cleanup.
Owned<MockSecretGenerator> mockSecretGenerator(new MockSecretGenerator());
Try<Owned<cluster::Slave>> slave = StartSlave(
diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
index 637bedc..1e91871 100644
--- a/src/tests/slave_tests.cpp
+++ b/src/tests/slave_tests.cpp
@@ -7847,7 +7847,6 @@ TEST_F(SlaveTest, RunTaskGroupFailedSecretGeneration)
StandaloneMasterDetector detector(master.get()->pid);
- // This pointer is passed to the agent, which will perform the cleanup.
Owned<MockSecretGenerator> secretGenerator(new MockSecretGenerator());
Try<Owned<cluster::Slave>> slave = StartSlave(
[mesos] 01/06: Stout: Added a sync option for `write` and `rename`.
Posted by ch...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 5a10007b86e4c7039a5260f6aacb75376270d57f
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Wed Oct 10 15:37:33 2018 -0700
Stout: Added a sync option for `write` and `rename`.
This patch adds an option for the caller to sync the file and directory
contents to the disk after a write to prevent filesystem inconsistency
against reboots.
Review: https://reviews.apache.org/r/69009
---
3rdparty/stout/include/stout/os/posix/fsync.hpp | 27 +++++++++++++++-
3rdparty/stout/include/stout/os/posix/rename.hpp | 37 +++++++++++++++++++++-
3rdparty/stout/include/stout/os/windows/rename.hpp | 10 ++++--
3rdparty/stout/include/stout/os/write.hpp | 31 ++++++++++++------
3rdparty/stout/include/stout/protobuf.hpp | 29 ++++++++++++++---
5 files changed, 115 insertions(+), 19 deletions(-)
diff --git a/3rdparty/stout/include/stout/os/posix/fsync.hpp b/3rdparty/stout/include/stout/os/posix/fsync.hpp
index 9a6bbf6..2cc5f76 100644
--- a/3rdparty/stout/include/stout/os/posix/fsync.hpp
+++ b/3rdparty/stout/include/stout/os/posix/fsync.hpp
@@ -15,9 +15,14 @@
#include <unistd.h>
+#include <string>
+
#include <stout/nothing.hpp>
#include <stout/try.hpp>
+#include <stout/os/close.hpp>
+#include <stout/os/int_fd.hpp>
+#include <stout/os/open.hpp>
namespace os {
@@ -30,7 +35,27 @@ inline Try<Nothing> fsync(int fd)
return Nothing();
}
-} // namespace os {
+// A wrapper function for the above `fsync()` with opening and closing the file.
+// NOTE: This function is POSIX only and can be used to commit changes to a
+// directory (e.g., renaming files) to the filesystem.
+inline Try<Nothing> fsync(const std::string& path)
+{
+ Try<int_fd> fd = os::open(path, O_RDONLY | O_CLOEXEC);
+
+ if (fd.isError()) {
+ return Error(fd.error());
+ }
+
+ Try<Nothing> result = fsync(fd.get());
+
+ // We ignore the return value of `close()` since any I/O-related failure
+ // scenarios would already have been triggered by `open()` or `fsync()`.
+ os::close(fd.get());
+
+ return result;
+}
+
+} // namespace os {
#endif // __STOUT_OS_POSIX_FSYNC_HPP__
diff --git a/3rdparty/stout/include/stout/os/posix/rename.hpp b/3rdparty/stout/include/stout/os/posix/rename.hpp
index 9cff6db..9bc6ee1 100644
--- a/3rdparty/stout/include/stout/os/posix/rename.hpp
+++ b/3rdparty/stout/include/stout/os/posix/rename.hpp
@@ -16,20 +16,55 @@
#include <stdio.h>
#include <string>
+#include <vector>
#include <stout/error.hpp>
+#include <stout/foreach.hpp>
#include <stout/nothing.hpp>
+#include <stout/path.hpp>
#include <stout/try.hpp>
+#include <stout/os/fsync.hpp>
namespace os {
-inline Try<Nothing> rename(const std::string& from, const std::string& to)
+// Rename a given path to another one. If `sync` is set to true, `fsync()` will
+// be called on both the source directory and the destination directory to
+// ensure that the result is committed to their filesystems.
+//
+// NOTE: This function can fail with `sync` set to true if either the source
+// directory or the destination directory gets removed before it returns. If
+// multiple processes or threads access to the filesystems concurrently, the
+// caller should either enforce a proper synchronization, or set `sync` to false
+// and call `fsync()` explicitly on POSIX systems to handle such failures.
+inline Try<Nothing> rename(
+ const std::string& from,
+ const std::string& to,
+ bool sync = false)
{
if (::rename(from.c_str(), to.c_str()) != 0) {
return ErrnoError();
}
+ if (sync) {
+ const std::string to_dir = Path(to).dirname();
+ const std::string from_dir = Path(from).dirname();
+
+ std::vector<std::string> dirs = {to_dir};
+ if (from_dir != to_dir) {
+ dirs.emplace_back(from_dir);
+ }
+
+ foreach (const std::string& dir, dirs) {
+ Try<Nothing> fsync = os::fsync(dir);
+
+ if (fsync.isError()) {
+ return Error(
+ "Failed to fsync directory '" + dir + "': " + fsync.error());
+ }
+ }
+ }
+
return Nothing();
}
diff --git a/3rdparty/stout/include/stout/os/windows/rename.hpp b/3rdparty/stout/include/stout/os/windows/rename.hpp
index 523912a..6747f29 100644
--- a/3rdparty/stout/include/stout/os/windows/rename.hpp
+++ b/3rdparty/stout/include/stout/os/windows/rename.hpp
@@ -22,10 +22,12 @@
#include <stout/internal/windows/longpath.hpp>
-
namespace os {
-inline Try<Nothing> rename(const std::string& from, const std::string& to)
+inline Try<Nothing> rename(
+ const std::string& from,
+ const std::string& to,
+ bool sync = false)
{
// Use `MoveFile` to perform the file move. The MSVCRT implementation of
// `::rename` fails if the `to` file already exists[1], while some UNIX
@@ -41,7 +43,9 @@ inline Try<Nothing> rename(const std::string& from, const std::string& to)
const BOOL result = ::MoveFileExW(
::internal::windows::longpath(from).data(),
::internal::windows::longpath(to).data(),
- MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING);
+ MOVEFILE_COPY_ALLOWED |
+ MOVEFILE_REPLACE_EXISTING |
+ (sync ? MOVEFILE_WRITE_THROUGH : 0));
if (!result) {
return WindowsError(
diff --git a/3rdparty/stout/include/stout/os/write.hpp b/3rdparty/stout/include/stout/os/write.hpp
index cd35285..f7538f9 100644
--- a/3rdparty/stout/include/stout/os/write.hpp
+++ b/3rdparty/stout/include/stout/os/write.hpp
@@ -20,6 +20,7 @@
#include <stout/try.hpp>
#include <stout/os/close.hpp>
+#include <stout/os/fsync.hpp>
#include <stout/os/int_fd.hpp>
#include <stout/os/open.hpp>
#include <stout/os/socket.hpp>
@@ -30,7 +31,6 @@
#include <stout/os/posix/write.hpp>
#endif // __WINDOWS__
-
namespace os {
namespace signal_safe {
@@ -110,9 +110,12 @@ inline Try<Nothing> write(int_fd fd, const std::string& message)
}
-// A wrapper function that wraps the above write() with
-// open and closing the file.
-inline Try<Nothing> write(const std::string& path, const std::string& message)
+// A wrapper function for the above `write()` with opening and closing the file.
+// If `sync` is set to true, an `fsync()` will be called before `close()`.
+inline Try<Nothing> write(
+ const std::string& path,
+ const std::string& message,
+ bool sync = false)
{
Try<int_fd> fd = os::open(
path,
@@ -125,9 +128,16 @@ inline Try<Nothing> write(const std::string& path, const std::string& message)
Try<Nothing> result = write(fd.get(), message);
- // We ignore the return value of close(). This is because users
- // calling this function are interested in the return value of
- // write(). Also an unsuccessful close() doesn't affect the write.
+ if (sync && result.isSome()) {
+ // We call `fsync()` before closing the file instead of opening it with the
+ // `O_SYNC` flag for better performance. See:
+ // http://lkml.iu.edu/hypermail/linux/kernel/0105.3/0353.html
+ result = os::fsync(fd.get());
+ }
+
+ // We ignore the return value of `close()` because users calling this function
+ // are interested in the return value of `write()`, or `fsync()` if `sync` is
+ // set to true. Also an unsuccessful `close()` doesn't affect the write.
os::close(fd.get());
return result;
@@ -136,9 +146,12 @@ inline Try<Nothing> write(const std::string& path, const std::string& message)
// NOTE: This overload is necessary to disambiguate between arguments
// of type `HANDLE` (`typedef void*`) and `char*` on Windows.
-inline Try<Nothing> write(const char* path, const std::string& message)
+inline Try<Nothing> write(
+ const char* path,
+ const std::string& message,
+ bool sync = false)
{
- return write(std::string(path), message);
+ return write(std::string(path), message, sync);
}
} // namespace os {
diff --git a/3rdparty/stout/include/stout/protobuf.hpp b/3rdparty/stout/include/stout/protobuf.hpp
index 1d03e1e..eb4adef 100644
--- a/3rdparty/stout/include/stout/protobuf.hpp
+++ b/3rdparty/stout/include/stout/protobuf.hpp
@@ -46,6 +46,7 @@
#include <stout/try.hpp>
#include <stout/os/close.hpp>
+#include <stout/os/fsync.hpp>
#include <stout/os/int_fd.hpp>
#include <stout/os/lseek.hpp>
#include <stout/os/open.hpp>
@@ -132,8 +133,10 @@ Try<Nothing> write(
}
+// A wrapper function for the above `write()` with opening and closing the file.
+// If `sync` is set to true, an `fsync()` will be called before `close()`.
template <typename T>
-Try<Nothing> write(const std::string& path, const T& t)
+Try<Nothing> write(const std::string& path, const T& t, bool sync = false)
{
Try<int_fd> fd = os::open(
path,
@@ -146,18 +149,28 @@ Try<Nothing> write(const std::string& path, const T& t)
Try<Nothing> result = write(fd.get(), t);
- // NOTE: We ignore the return value of close(). This is because
- // users calling this function are interested in the return value of
- // write(). Also an unsuccessful close() doesn't affect the write.
+ if (sync && result.isSome()) {
+ // We call `fsync()` before closing the file instead of opening it with the
+ // `O_SYNC` flag for better performance. See:
+ // http://lkml.iu.edu/hypermail/linux/kernel/0105.3/0353.html
+ result = os::fsync(fd.get());
+ }
+
+ // We ignore the return value of `close()` because users calling this function
+ // are interested in the return value of `write()`, or `fsync()` if `sync` is
+ // set to true. Also an unsuccessful `close()` doesn't affect the write.
os::close(fd.get());
return result;
}
+// A wrapper function to append a protobuf message with opening and closing the
+// file. If `sync` is set to true, an `fsync()` will be called before `close()`.
inline Try<Nothing> append(
const std::string& path,
- const google::protobuf::Message& message)
+ const google::protobuf::Message& message,
+ bool sync = false)
{
Try<int_fd> fd = os::open(
path,
@@ -170,6 +183,12 @@ inline Try<Nothing> append(
Try<Nothing> result = write(fd.get(), message);
+ if (sync && result.isSome()) {
+ // We call `fsync()` before closing the file instead of opening it with the
+ // `O_SYNC` flag for better performance.
+ result = os::fsync(fd.get());
+ }
+
// NOTE: We ignore the return value of close(). This is because
// users calling this function are interested in the return value of
// write(). Also an unsuccessful close() doesn't affect the write.
[mesos] 04/06: Added MESOS-9281 to the 1.7.1 CHANGELOG.
Posted by ch...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 6e15ff3808f88c1aa1a1c8464276f870f4e96445
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Wed Oct 17 15:48:31 2018 -0700
Added MESOS-9281 to the 1.7.1 CHANGELOG.
---
CHANGELOG | 1 +
1 file changed, 1 insertion(+)
diff --git a/CHANGELOG b/CHANGELOG
index e39c503..1ae1cb0 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -16,6 +16,7 @@ Release Notes - Mesos - Version 1.7.1 (WIP)
* [MESOS-9267] - Mesos agent crashes when CNI network is not configured but used.
* [MESOS-9274] - v1 JAVA scheduler library can drop TEARDOWN upon destruction.
* [MESOS-9279] - Docker Containerizer 'usage' call might be expensive if mount table is big.
+ * [MESOS-9281] - SLRP gets a stale checkpoint after system crash.
* [MESOS-9283] - Docker containerizer actor can get backlogged with large number of containers.
* [MESOS-9295] - Nested container launch could fail if the agent upgrade with new cgroup subsystems.
* [MESOS-9308] - URI disk profile adaptor could deadlock.
[mesos] 02/06: Stout: Added a sync option for `mkdir`.
Posted by ch...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 80fa7318432c043d4763d900c41b53b440fae459
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Fri Oct 19 09:28:38 2018 -0700
Stout: Added a sync option for `mkdir`.
To ensure the directories created by `mkdir` are commited to their
filesystems, an `fsync` will be called on the parent of each created
directory if the `sync` option is set to true. This option has no
effect on Windows.
Review: https://reviews.apache.org/r/69085
---
3rdparty/stout/include/stout/os/posix/mkdir.hpp | 40 ++++++++++++++++++++---
3rdparty/stout/include/stout/os/windows/mkdir.hpp | 7 ++--
2 files changed, 41 insertions(+), 6 deletions(-)
diff --git a/3rdparty/stout/include/stout/os/posix/mkdir.hpp b/3rdparty/stout/include/stout/os/posix/mkdir.hpp
index 418db9a..806ec39 100644
--- a/3rdparty/stout/include/stout/os/posix/mkdir.hpp
+++ b/3rdparty/stout/include/stout/os/posix/mkdir.hpp
@@ -19,21 +19,44 @@
#include <vector>
#include <stout/error.hpp>
+#include <stout/foreach.hpp>
#include <stout/nothing.hpp>
#include <stout/strings.hpp>
+#include <stout/path.hpp>
#include <stout/try.hpp>
#include <stout/os/constants.hpp>
-
+#include <stout/os/fsync.hpp>
namespace os {
-inline Try<Nothing> mkdir(const std::string& directory, bool recursive = true)
+// Make a directory.
+//
+// If `recursive` is set to true, all intermediate directories will be created
+// as required. If `sync` is set to true, `fsync()` will be called on the parent
+// of each created directory to ensure that the result is committed to its
+// filesystem.
+//
+// NOTE: This function doesn't ensure that any existing directory is committed
+// to its filesystem, and it does not perform any cleanup in case of a failure.
+inline Try<Nothing> mkdir(
+ const std::string& directory,
+ bool recursive = true,
+ bool sync = false)
{
if (!recursive) {
if (::mkdir(directory.c_str(), 0755) < 0) {
return ErrnoError();
}
+
+ if (sync) {
+ const std::string parent = Path(directory).dirname();
+ Try<Nothing> fsync = os::fsync(parent);
+ if (fsync.isError()) {
+ return Error(
+ "Failed to fsync directory '" + parent + "': " + fsync.error());
+ }
+ }
} else {
std::vector<std::string> tokens =
strings::tokenize(directory, stringify(os::PATH_SEPARATOR));
@@ -47,8 +70,17 @@ inline Try<Nothing> mkdir(const std::string& directory, bool recursive = true)
foreach (const std::string& token, tokens) {
path += token;
- if (::mkdir(path.c_str(), 0755) < 0 && errno != EEXIST) {
- return ErrnoError();
+ if (::mkdir(path.c_str(), 0755) < 0) {
+ if (errno != EEXIST) {
+ return ErrnoError();
+ }
+ } else if (sync) {
+ const std::string parent = Path(path).dirname();
+ Try<Nothing> fsync = os::fsync(parent);
+ if (fsync.isError()) {
+ return Error(
+ "Failed to fsync directory '" + parent + "': " + fsync.error());
+ }
}
path += os::PATH_SEPARATOR;
diff --git a/3rdparty/stout/include/stout/os/windows/mkdir.hpp b/3rdparty/stout/include/stout/os/windows/mkdir.hpp
index 2aef22a..77d292c 100644
--- a/3rdparty/stout/include/stout/os/windows/mkdir.hpp
+++ b/3rdparty/stout/include/stout/os/windows/mkdir.hpp
@@ -27,10 +27,13 @@
#include <stout/internal/windows/longpath.hpp>
-
namespace os {
-inline Try<Nothing> mkdir(const std::string& directory, bool recursive = true)
+// NOTE: `sync` has no effect on Windows.
+inline Try<Nothing> mkdir(
+ const std::string& directory,
+ bool recursive = true,
+ bool sync = false)
{
if (!recursive) {
// NOTE: We check for existence because parts of certain directories
[mesos] 06/06: Added a comment for `Resource.provider_id`.
Posted by ch...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 002acd743caa98e4a8bd0138b837bb70f7fc45da
Author: Chun-Hung Hsiao <ch...@apache.org>
AuthorDate: Wed Oct 31 11:18:46 2018 -0700
Added a comment for `Resource.provider_id`.
Review: https://reviews.apache.org/r/69035/
---
include/mesos/mesos.proto | 1 +
include/mesos/v1/mesos.proto | 1 +
2 files changed, 2 insertions(+)
diff --git a/include/mesos/mesos.proto b/include/mesos/mesos.proto
index f6989cd..06a901d 100644
--- a/include/mesos/mesos.proto
+++ b/include/mesos/mesos.proto
@@ -1173,6 +1173,7 @@ message Attribute {
* which correspond to partial shares of a CPU.
*/
message Resource {
+ // Specified if the resource comes from a particular resource provider.
optional ResourceProviderID provider_id = 12;
required string name = 1;
diff --git a/include/mesos/v1/mesos.proto b/include/mesos/v1/mesos.proto
index edad35a..75cdb28 100644
--- a/include/mesos/v1/mesos.proto
+++ b/include/mesos/v1/mesos.proto
@@ -1165,6 +1165,7 @@ message Attribute {
* which correspond to partial shares of a CPU.
*/
message Resource {
+ // Specified if the resource comes from a particular resource provider.
optional ResourceProviderID provider_id = 12;
required string name = 1;