diff --git a/include/envoy/access_log/BUILD b/include/envoy/access_log/BUILD index d05c37b156130..da0469451a0b8 100644 --- a/include/envoy/access_log/BUILD +++ b/include/envoy/access_log/BUILD @@ -12,7 +12,6 @@ envoy_cc_library( name = "access_log_interface", hdrs = ["access_log.h"], deps = [ - "//include/envoy/filesystem:filesystem_interface", "//include/envoy/http:header_map_interface", "//include/envoy/stream_info:stream_info_interface", ], diff --git a/include/envoy/access_log/access_log.h b/include/envoy/access_log/access_log.h index af53b244d82c2..952265933065d 100644 --- a/include/envoy/access_log/access_log.h +++ b/include/envoy/access_log/access_log.h @@ -4,13 +4,34 @@ #include #include "envoy/common/pure.h" -#include "envoy/filesystem/filesystem.h" #include "envoy/http/header_map.h" #include "envoy/stream_info/stream_info.h" namespace Envoy { namespace AccessLog { +class AccessLogFile { +public: + virtual ~AccessLogFile() {} + + /** + * Write data to the file. + */ + virtual void write(absl::string_view) PURE; + + /** + * Reopen the file. + */ + virtual void reopen() PURE; + + /** + * Synchronously flush all pending data to disk. + */ + virtual void flush() PURE; +}; + +using AccessLogFileSharedPtr = std::shared_ptr; + class AccessLogManager { public: virtual ~AccessLogManager() {} @@ -25,10 +46,10 @@ class AccessLogManager { * @param file_name specifies the file to create/open. * @return the opened file. */ - virtual Filesystem::FileSharedPtr createAccessLog(const std::string& file_name) PURE; + virtual AccessLogFileSharedPtr createAccessLog(const std::string& file_name) PURE; }; -typedef std::unique_ptr AccessLogManagerPtr; +using AccessLogManagerPtr = std::unique_ptr; /** * Interface for access log filters. @@ -46,7 +67,7 @@ class Filter { const Http::HeaderMap& response_trailers) PURE; }; -typedef std::unique_ptr FilterPtr; +using FilterPtr = std::unique_ptr; /** * Abstract access logger for requests and connections. @@ -68,7 +89,7 @@ class Instance { const StreamInfo::StreamInfo& stream_info) PURE; }; -typedef std::shared_ptr InstanceSharedPtr; +using InstanceSharedPtr = std::shared_ptr; /** * Interface for access log formatter. @@ -92,7 +113,7 @@ class Formatter { const StreamInfo::StreamInfo& stream_info) const PURE; }; -typedef std::unique_ptr FormatterPtr; +using FormatterPtr = std::unique_ptr; /** * Interface for access log provider. @@ -116,7 +137,7 @@ class FormatterProvider { const StreamInfo::StreamInfo& stream_info) const PURE; }; -typedef std::unique_ptr FormatterProviderPtr; +using FormatterProviderPtr = std::unique_ptr; } // namespace AccessLog } // namespace Envoy diff --git a/include/envoy/api/os_sys_calls.h b/include/envoy/api/os_sys_calls.h index 51f90a1afa6d4..d3edee58fa6df 100644 --- a/include/envoy/api/os_sys_calls.h +++ b/include/envoy/api/os_sys_calls.h @@ -34,6 +34,7 @@ typedef SysCallResult SysCallIntResult; typedef SysCallResult SysCallSizeResult; typedef SysCallResult SysCallPtrResult; typedef SysCallResult SysCallStringResult; +typedef SysCallResult SysCallBoolResult; class OsSysCalls { public: @@ -49,18 +50,6 @@ class OsSysCalls { */ virtual SysCallIntResult ioctl(int sockfd, unsigned long int request, void* argp) PURE; - /** - * Open file by full_path with given flags and mode. - * @return file descriptor. - */ - virtual SysCallIntResult open(const std::string& full_path, int flags, int mode) PURE; - - /** - * Write num_bytes to fd from buffer. - * @return number of bytes written if non negative, otherwise error code. - */ - virtual SysCallSizeResult write(int fd, const void* buffer, size_t num_bytes) PURE; - /** * @see writev (man 2 writev) */ diff --git a/include/envoy/filesystem/BUILD b/include/envoy/filesystem/BUILD index 022c770c1c4ae..d61b9ed36d0a1 100644 --- a/include/envoy/filesystem/BUILD +++ b/include/envoy/filesystem/BUILD @@ -12,6 +12,7 @@ envoy_cc_library( name = "filesystem_interface", hdrs = ["filesystem.h"], deps = [ + "//include/envoy/api:os_sys_calls_interface", "//include/envoy/event:dispatcher_interface", ], ) diff --git a/include/envoy/filesystem/filesystem.h b/include/envoy/filesystem/filesystem.h index 63915bee12f28..194a5a6376600 100644 --- a/include/envoy/filesystem/filesystem.h +++ b/include/envoy/filesystem/filesystem.h @@ -4,9 +4,8 @@ #include #include +#include "envoy/api/os_sys_calls.h" #include "envoy/common/pure.h" -#include "envoy/event/dispatcher.h" -#include "envoy/thread/thread.h" #include "absl/strings/string_view.h" @@ -14,58 +13,65 @@ namespace Envoy { namespace Filesystem { /** - * Abstraction for a file on disk. + * Abstraction for a basic file on disk. */ class File { public: virtual ~File() {} /** - * Write data to the file. + * Open the file with O_RDWR | O_APPEND | O_CREAT + * The file will be closed when this object is destructed + * + * @return bool whether the open succeeded + */ + virtual Api::SysCallBoolResult open() PURE; + + /** + * Write the buffer to the file. The file must be explicitly opened before writing. + * + * @return ssize_t number of bytes written, or -1 for failure + */ + virtual Api::SysCallSizeResult write(absl::string_view buffer) PURE; + + /** + * Close the file. + * + * @return bool whether the close succeeded */ - virtual void write(absl::string_view) PURE; + virtual Api::SysCallBoolResult close() PURE; /** - * Reopen the file. + * @return bool is the file open */ - virtual void reopen() PURE; + virtual bool isOpen() PURE; /** - * Synchronously flush all pending data to disk. + * @return string the file path */ - virtual void flush() PURE; + virtual std::string path() PURE; + + /** + * @return string a human-readable string describing the error code + * TODO(sesmith177) Use the IOError class after #5829 merges + */ + virtual std::string errorToString(int error) PURE; }; -typedef std::shared_ptr FileSharedPtr; +using FilePtr = std::unique_ptr; /** - * Captures state, properties, and stats of a file-system. + * Abstraction for some basic filesystem operations */ class Instance { public: virtual ~Instance() {} /** - * Creates a file, overriding the flush-interval set in the class. - * - * @param path The path of the file to open. - * @param dispatcher The dispatcher used for set up timers to run flush(). - * @param lock The lock. - * @param file_flush_interval_msec Number of milliseconds to delay before flushing. + * @param path The path of the File + * @return a FilePtr. The file is not opened. */ - virtual FileSharedPtr createFile(const std::string& path, Event::Dispatcher& dispatcher, - Thread::BasicLockable& lock, - std::chrono::milliseconds file_flush_interval_msec) PURE; - - /** - * Creates a file, using the default flush-interval for the class. - * - * @param path The path of the file to open. - * @param dispatcher The dispatcher used for set up timers to run flush(). - * @param lock The lock. - */ - virtual FileSharedPtr createFile(const std::string& path, Event::Dispatcher& dispatcher, - Thread::BasicLockable& lock) PURE; + virtual FilePtr createFile(const std::string& path) PURE; /** * @return bool whether a file exists on disk and can be opened for read. @@ -109,8 +115,6 @@ class Instance { virtual bool illegalPath(const std::string& path) PURE; }; -typedef std::unique_ptr WatcherPtr; - enum class FileType { Regular, Directory, Other }; struct DirectoryEntry { diff --git a/source/common/access_log/BUILD b/source/common/access_log/BUILD index c7be03032909e..48bb5b769d97d 100644 --- a/source/common/access_log/BUILD +++ b/source/common/access_log/BUILD @@ -30,6 +30,8 @@ envoy_cc_library( deps = [ "//include/envoy/access_log:access_log_interface", "//include/envoy/api:api_interface", + "//source/common/buffer:buffer_lib", + "//source/common/common:thread_lib", ], ) diff --git a/source/common/access_log/access_log_manager_impl.cc b/source/common/access_log/access_log_manager_impl.cc index ecc62535b0b6f..14a27be7b61e5 100644 --- a/source/common/access_log/access_log_manager_impl.cc +++ b/source/common/access_log/access_log_manager_impl.cc @@ -2,6 +2,11 @@ #include +#include "common/common/assert.h" +#include "common/common/fmt.h" +#include "common/common/lock_guard.h" +#include "common/common/stack_array.h" + namespace Envoy { namespace AccessLog { @@ -11,14 +16,178 @@ void AccessLogManagerImpl::reopen() { } } -Filesystem::FileSharedPtr AccessLogManagerImpl::createAccessLog(const std::string& file_name) { +AccessLogFileSharedPtr AccessLogManagerImpl::createAccessLog(const std::string& file_name) { if (access_logs_.count(file_name)) { return access_logs_[file_name]; } - access_logs_[file_name] = api_.fileSystem().createFile(file_name, dispatcher_, lock_); + access_logs_[file_name] = std::make_shared( + api_.fileSystem().createFile(file_name), dispatcher_, lock_, file_stats_, + file_flush_interval_msec_, api_.threadFactory()); return access_logs_[file_name]; } +AccessLogFileImpl::AccessLogFileImpl(Filesystem::FilePtr&& file, Event::Dispatcher& dispatcher, + Thread::BasicLockable& lock, AccessLogFileStats& stats, + std::chrono::milliseconds flush_interval_msec, + Thread::ThreadFactory& thread_factory) + : file_(std::move(file)), file_lock_(lock), + flush_timer_(dispatcher.createTimer([this]() -> void { + stats_.flushed_by_timer_.inc(); + flush_event_.notifyOne(); + flush_timer_->enableTimer(flush_interval_msec_); + })), + thread_factory_(thread_factory), flush_interval_msec_(flush_interval_msec), stats_(stats) { + open(); +} + +void AccessLogFileImpl::open() { + const Api::SysCallBoolResult result = file_->open(); + if (!result.rc_) { + throw EnvoyException(fmt::format("unable to open file '{}': {}", file_->path(), + file_->errorToString(result.errno_))); + } +} + +void AccessLogFileImpl::reopen() { reopen_file_ = true; } + +AccessLogFileImpl::~AccessLogFileImpl() { + { + Thread::LockGuard lock(write_lock_); + flush_thread_exit_ = true; + flush_event_.notifyOne(); + } + + if (flush_thread_ != nullptr) { + flush_thread_->join(); + } + + // Flush any remaining data. If file was not opened for some reason, skip flushing part. + if (file_->isOpen()) { + if (flush_buffer_.length() > 0) { + doWrite(flush_buffer_); + } + + const Api::SysCallBoolResult result = file_->close(); + ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", file_->path(), + file_->errorToString(result.errno_))); + } +} + +void AccessLogFileImpl::doWrite(Buffer::Instance& buffer) { + uint64_t num_slices = buffer.getRawSlices(nullptr, 0); + STACK_ARRAY(slices, Buffer::RawSlice, num_slices); + buffer.getRawSlices(slices.begin(), num_slices); + + // We must do the actual writes to disk under lock, so that we don't intermix chunks from + // different AccessLogFileImpl pointing to the same underlying file. This can happen either via + // hot restart or if calling code opens the same underlying file into a different + // AccessLogFileImpl in the same process. + // TODO PERF: Currently, we use a single cross process lock to serialize all disk writes. This + // will never block network workers, but does mean that only a single flush thread can + // actually flush to disk. In the future it would be nice if we did away with the cross + // process lock or had multiple locks. + { + Thread::LockGuard lock(file_lock_); + for (const Buffer::RawSlice& slice : slices) { + absl::string_view data(static_cast(slice.mem_), slice.len_); + const Api::SysCallSizeResult result = file_->write(data); + ASSERT(result.rc_ == static_cast(slice.len_)); + stats_.write_completed_.inc(); + } + } + + stats_.write_total_buffered_.sub(buffer.length()); + buffer.drain(buffer.length()); +} + +void AccessLogFileImpl::flushThreadFunc() { + + while (true) { + std::unique_lock flush_lock; + + { + Thread::LockGuard write_lock(write_lock_); + + // flush_event_ can be woken up either by large enough flush_buffer or by timer. + // In case it was timer, flush_buffer_ can be empty. + while (flush_buffer_.length() == 0 && !flush_thread_exit_) { + // CondVar::wait() does not throw, so it's safe to pass the mutex rather than the guard. + flush_event_.wait(write_lock_); + } + + if (flush_thread_exit_) { + return; + } + + flush_lock = std::unique_lock(flush_lock_); + ASSERT(flush_buffer_.length() > 0); + about_to_write_buffer_.move(flush_buffer_); + ASSERT(flush_buffer_.length() == 0); + } + + // if we failed to open file before, then simply ignore + if (file_->isOpen()) { + try { + if (reopen_file_) { + reopen_file_ = false; + const Api::SysCallBoolResult result = file_->close(); + ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", file_->path(), + file_->errorToString(result.errno_))); + open(); + } + + doWrite(about_to_write_buffer_); + } catch (const EnvoyException&) { + stats_.reopen_failed_.inc(); + } + } + } +} + +void AccessLogFileImpl::flush() { + std::unique_lock flush_buffer_lock; + + { + Thread::LockGuard write_lock(write_lock_); + + // flush_lock_ must be held while checking this or else it is + // possible that flushThreadFunc() has already moved data from + // flush_buffer_ to about_to_write_buffer_, has unlocked write_lock_, + // but has not yet completed doWrite(). This would allow flush() to + // return before the pending data has actually been written to disk. + flush_buffer_lock = std::unique_lock(flush_lock_); + + if (flush_buffer_.length() == 0) { + return; + } + + about_to_write_buffer_.move(flush_buffer_); + ASSERT(flush_buffer_.length() == 0); + } + + doWrite(about_to_write_buffer_); +} + +void AccessLogFileImpl::write(absl::string_view data) { + Thread::LockGuard lock(write_lock_); + + if (flush_thread_ == nullptr) { + createFlushStructures(); + } + + stats_.write_buffered_.inc(); + stats_.write_total_buffered_.add(data.length()); + flush_buffer_.add(data.data(), data.size()); + if (flush_buffer_.length() > MIN_FLUSH_SIZE) { + flush_event_.notifyOne(); + } +} + +void AccessLogFileImpl::createFlushStructures() { + flush_thread_ = thread_factory_.createThread([this]() -> void { flushThreadFunc(); }); + flush_timer_->enableTimer(flush_interval_msec_); +} + } // namespace AccessLog } // namespace Envoy diff --git a/source/common/access_log/access_log_manager_impl.h b/source/common/access_log/access_log_manager_impl.h index 502ca50c74e86..d009a933042d1 100644 --- a/source/common/access_log/access_log_manager_impl.h +++ b/source/common/access_log/access_log_manager_impl.h @@ -5,24 +5,130 @@ #include "envoy/access_log/access_log.h" #include "envoy/api/api.h" +#include "envoy/event/dispatcher.h" +#include "envoy/filesystem/filesystem.h" +#include "envoy/stats/stats_macros.h" +#include "envoy/stats/store.h" + +#include "common/buffer/buffer_impl.h" +#include "common/common/thread.h" namespace Envoy { + +// clang-format off +#define ACCESS_LOG_FILE_STATS(COUNTER, GAUGE) \ + COUNTER(write_buffered) \ + COUNTER(write_completed) \ + COUNTER(flushed_by_timer) \ + COUNTER(reopen_failed) \ + GAUGE (write_total_buffered) +// clang-format on + +struct AccessLogFileStats { + ACCESS_LOG_FILE_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT) +}; + namespace AccessLog { class AccessLogManagerImpl : public AccessLogManager { public: - AccessLogManagerImpl(Api::Api& api, Event::Dispatcher& dispatcher, Thread::BasicLockable& lock) - : api_(api), dispatcher_(dispatcher), lock_(lock) {} + AccessLogManagerImpl(std::chrono::milliseconds file_flush_interval_msec, Api::Api& api, + Event::Dispatcher& dispatcher, Thread::BasicLockable& lock, + Stats::Store& stats_store) + : file_flush_interval_msec_(file_flush_interval_msec), api_(api), dispatcher_(dispatcher), + lock_(lock), file_stats_{ACCESS_LOG_FILE_STATS( + POOL_COUNTER_PREFIX(stats_store, "access_log_file."), + POOL_GAUGE_PREFIX(stats_store, "access_log_file."))} {} // AccessLog::AccessLogManager void reopen() override; - Filesystem::FileSharedPtr createAccessLog(const std::string& file_name) override; + AccessLogFileSharedPtr createAccessLog(const std::string& file_name) override; private: + const std::chrono::milliseconds file_flush_interval_msec_; Api::Api& api_; Event::Dispatcher& dispatcher_; Thread::BasicLockable& lock_; - std::unordered_map access_logs_; + AccessLogFileStats file_stats_; + std::unordered_map access_logs_; +}; + +/** + * This is a file implementation geared for writing out access logs. It turn out that in certain + * cases even if a standard file is opened with O_NONBLOCK, the kernel can still block when writing. + * This implementation uses a flush thread per file, with the idea there there aren't that many + * files. If this turns out to be a good implementation we can potentially have a single flush + * thread that flushes all files, but we will start with this. + */ +class AccessLogFileImpl : public AccessLogFile { +public: + AccessLogFileImpl(Filesystem::FilePtr&& file, Event::Dispatcher& dispatcher, + Thread::BasicLockable& lock, AccessLogFileStats& stats_, + std::chrono::milliseconds flush_interval_msec, + Thread::ThreadFactory& thread_factory); + ~AccessLogFileImpl(); + + // AccessLog::AccessLogFile + void write(absl::string_view data) override; + + /** + * Reopen file asynchronously. + * This only sets reopen flag, actual reopen operation is delayed. + * Reopen happens before the next write operation. + */ + void reopen() override; + void flush() override; + +private: + void doWrite(Buffer::Instance& buffer); + void flushThreadFunc(); + void open(); + void createFlushStructures(); + + // Minimum size before the flush thread will be told to flush. + static const uint64_t MIN_FLUSH_SIZE = 1024 * 64; + + Filesystem::FilePtr file_; + + // These locks are always acquired in the following order if multiple locks are held: + // 1) write_lock_ + // 2) flush_lock_ + // 3) file_lock_ + Thread::BasicLockable& file_lock_; // This lock is used only by the flush thread when writing + // to disk. This is used to make sure that file blocks do + // not get interleaved by multiple processes writing to + // the same file during hot-restart. + Thread::MutexBasicLockable flush_lock_; // This lock is used to prevent simultaneous flushes from + // the flush thread and a synchronous flush. This protects + // concurrent access to the about_to_write_buffer_, fd_, + // and all other data used during flushing and file + // re-opening. + Thread::MutexBasicLockable + write_lock_; // The lock is used when filling the flush buffer. It allows + // multiple threads to write to the same file at relatively + // high performance. It is always local to the process. + Thread::ThreadPtr flush_thread_; + Thread::CondVar flush_event_; + std::atomic flush_thread_exit_{}; + std::atomic reopen_file_{}; + Buffer::OwnedImpl + flush_buffer_ GUARDED_BY(write_lock_); // This buffer is used by multiple threads. It gets + // filled and then flushed either when max size is + // reached or when a timer fires. + // TODO(jmarantz): this should be GUARDED_BY(flush_lock_) but the analysis cannot poke through + // the std::make_unique assignment. I do not believe it's possible to annotate this properly now + // due to limitations in the clang thread annotation analysis. + Buffer::OwnedImpl about_to_write_buffer_; // This buffer is used only by the flush thread. Data + // is moved from flush_buffer_ under lock, and then + // the lock is released so that flush_buffer_ can + // continue to fill. This buffer is then used for the + // final write to disk. + Event::TimerPtr flush_timer_; + Thread::ThreadFactory& thread_factory_; + const std::chrono::milliseconds flush_interval_msec_; // Time interval buffer gets flushed no + // matter if it reached the MIN_FLUSH_SIZE + // or not. + AccessLogFileStats& stats_; }; } // namespace AccessLog diff --git a/source/common/api/BUILD b/source/common/api/BUILD index c42676952895d..92623ea51279f 100644 --- a/source/common/api/BUILD +++ b/source/common/api/BUILD @@ -14,7 +14,6 @@ envoy_cc_library( hdrs = ["api_impl.h"], deps = [ "//include/envoy/api:api_interface", - "//source/common/api:os_sys_calls_lib", "//source/common/common:thread_lib", "//source/common/event:dispatcher_lib", "//source/common/filesystem:filesystem_lib", diff --git a/source/common/api/api_impl.cc b/source/common/api/api_impl.cc index 76ac0c95461ff..43477603fdd3c 100644 --- a/source/common/api/api_impl.cc +++ b/source/common/api/api_impl.cc @@ -9,12 +9,8 @@ namespace Envoy { namespace Api { -Impl::Impl(std::chrono::milliseconds file_flush_interval_msec, - Thread::ThreadFactory& thread_factory, Stats::Store& stats_store, - Event::TimeSystem& time_system) - : thread_factory_(thread_factory), - file_system_(file_flush_interval_msec, thread_factory, stats_store), - time_system_(time_system) {} +Impl::Impl(Thread::ThreadFactory& thread_factory, Stats::Store&, Event::TimeSystem& time_system) + : thread_factory_(thread_factory), time_system_(time_system) {} Event::DispatcherPtr Impl::allocateDispatcher() { return std::make_unique(*this, time_system_); diff --git a/source/common/api/api_impl.h b/source/common/api/api_impl.h index e95132437234a..f2c6e8b671a07 100644 --- a/source/common/api/api_impl.h +++ b/source/common/api/api_impl.h @@ -18,8 +18,7 @@ namespace Api { */ class Impl : public Api { public: - Impl(std::chrono::milliseconds file_flush_interval_msec, Thread::ThreadFactory& thread_factory, - Stats::Store& stats_store, Event::TimeSystem& time_system); + Impl(Thread::ThreadFactory& thread_factory, Stats::Store&, Event::TimeSystem& time_system); // Api::Api Event::DispatcherPtr allocateDispatcher() override; diff --git a/source/common/api/os_sys_calls_impl.cc b/source/common/api/os_sys_calls_impl.cc index c6e6a43c17e15..e0c42130a9076 100644 --- a/source/common/api/os_sys_calls_impl.cc +++ b/source/common/api/os_sys_calls_impl.cc @@ -18,21 +18,11 @@ SysCallIntResult OsSysCallsImpl::ioctl(int sockfd, unsigned long int request, vo return {rc, errno}; } -SysCallIntResult OsSysCallsImpl::open(const std::string& full_path, int flags, int mode) { - const int rc = ::open(full_path.c_str(), flags, mode); - return {rc, errno}; -} - SysCallIntResult OsSysCallsImpl::close(int fd) { const int rc = ::close(fd); return {rc, errno}; } -SysCallSizeResult OsSysCallsImpl::write(int fd, const void* buffer, size_t num_bytes) { - const ssize_t rc = ::write(fd, buffer, num_bytes); - return {rc, errno}; -} - SysCallSizeResult OsSysCallsImpl::writev(int fd, const iovec* iovec, int num_iovec) { const ssize_t rc = ::writev(fd, iovec, num_iovec); return {rc, errno}; diff --git a/source/common/api/os_sys_calls_impl.h b/source/common/api/os_sys_calls_impl.h index eed6d1798645f..c5991190b7fc2 100644 --- a/source/common/api/os_sys_calls_impl.h +++ b/source/common/api/os_sys_calls_impl.h @@ -12,8 +12,6 @@ class OsSysCallsImpl : public OsSysCalls { // Api::OsSysCalls SysCallIntResult bind(int sockfd, const sockaddr* addr, socklen_t addrlen) override; SysCallIntResult ioctl(int sockfd, unsigned long int request, void* argp) override; - SysCallIntResult open(const std::string& full_path, int flags, int mode) override; - SysCallSizeResult write(int fd, const void* buffer, size_t num_bytes) override; SysCallSizeResult writev(int fd, const iovec* iovec, int num_iovec) override; SysCallSizeResult readv(int fd, const iovec* iovec, int num_iovec) override; SysCallSizeResult recv(int socket, void* buffer, size_t length, int flags) override; diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 6b1d9c0209868..156a52c90eae5 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -140,7 +140,6 @@ envoy_cc_library( ":macros", ":minimal_logger_lib", "//include/envoy/access_log:access_log_interface", - "//include/envoy/filesystem:filesystem_interface", ], ) diff --git a/source/common/common/logger_delegates.h b/source/common/common/logger_delegates.h index 1dcd4d6926943..67264d29c3a33 100644 --- a/source/common/common/logger_delegates.h +++ b/source/common/common/logger_delegates.h @@ -5,7 +5,6 @@ #include #include "envoy/access_log/access_log.h" -#include "envoy/filesystem/filesystem.h" #include "common/common/logger.h" #include "common/common/macros.h" @@ -31,7 +30,7 @@ class FileSinkDelegate : public SinkDelegate { void flush() override; private: - Filesystem::FileSharedPtr log_file_; + AccessLog::AccessLogFileSharedPtr log_file_; }; } // namespace Logger diff --git a/source/common/filesystem/BUILD b/source/common/filesystem/BUILD index f4f0d62e7d0b7..99b0c654fc404 100644 --- a/source/common/filesystem/BUILD +++ b/source/common/filesystem/BUILD @@ -43,13 +43,7 @@ envoy_cc_library( srcs = ["filesystem_impl.cc"], hdrs = ["filesystem_impl.h"], deps = [ - "//include/envoy/api:api_interface", - "//include/envoy/api:os_sys_calls_interface", - "//include/envoy/event:dispatcher_interface", "//include/envoy/filesystem:filesystem_interface", - "//source/common/api:os_sys_calls_lib", - "//source/common/buffer:buffer_lib", - "//source/common/common:thread_lib", ], ) diff --git a/source/common/filesystem/filesystem_impl.cc b/source/common/filesystem/filesystem_impl.cc index 1fd3ce8a455cd..a940c12f6fae9 100644 --- a/source/common/filesystem/filesystem_impl.cc +++ b/source/common/filesystem/filesystem_impl.cc @@ -1,49 +1,76 @@ #include "common/filesystem/filesystem_impl.h" #include +#include #include +#include -#include -#include +#include #include #include #include #include -#include #include "envoy/common/exception.h" -#include "envoy/common/time.h" -#include "envoy/event/dispatcher.h" -#include "envoy/thread/thread.h" -#include "common/api/os_sys_calls_impl.h" #include "common/common/assert.h" #include "common/common/fmt.h" -#include "common/common/lock_guard.h" -#include "common/common/stack_array.h" +#include "common/common/logger.h" #include "absl/strings/match.h" namespace Envoy { namespace Filesystem { -InstanceImpl::InstanceImpl(std::chrono::milliseconds file_flush_interval_msec, - Thread::ThreadFactory& thread_factory, Stats::Store& stats_store) - : file_flush_interval_msec_(file_flush_interval_msec), - file_stats_{FILESYSTEM_STATS(POOL_COUNTER_PREFIX(stats_store, "filesystem."), - POOL_GAUGE_PREFIX(stats_store, "filesystem."))}, - thread_factory_(thread_factory) {} - -FileSharedPtr InstanceImpl::createFile(const std::string& path, Event::Dispatcher& dispatcher, - Thread::BasicLockable& lock, - std::chrono::milliseconds file_flush_interval_msec) { - return std::make_shared(path, dispatcher, lock, file_stats_, - file_flush_interval_msec, thread_factory_); -}; - -FileSharedPtr InstanceImpl::createFile(const std::string& path, Event::Dispatcher& dispatcher, - Thread::BasicLockable& lock) { - return createFile(path, dispatcher, lock, file_flush_interval_msec_); +FileImpl::FileImpl(const std::string& path) : fd_(-1), path_(path) {} + +FileImpl::~FileImpl() { + if (isOpen()) { + const Api::SysCallBoolResult result = close(); + ASSERT(result.rc_); + } +} + +Api::SysCallBoolResult FileImpl::open() { + if (isOpen()) { + return {true, 0}; + } + + const int flags = O_RDWR | O_APPEND | O_CREAT; + const int mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH; + + fd_ = ::open(path_.c_str(), flags, mode); + if (-1 == fd_) { + return {false, errno}; + } + return {true, 0}; +} + +Api::SysCallSizeResult FileImpl::write(absl::string_view buffer) { + const ssize_t rc = ::write(fd_, buffer.data(), buffer.size()); + return {rc, errno}; +} + +Api::SysCallBoolResult FileImpl::close() { + ASSERT(isOpen()); + + const int rc = ::close(fd_); + if (rc == -1) { + return {false, errno}; + } + + fd_ = -1; + return {true, 0}; +} + +bool FileImpl::isOpen() { return fd_ != -1; } + +std::string FileImpl::path() { return path_; } + +std::string FileImpl::errorToString(int error) { return ::strerror(error); } + +FilePtr InstanceImpl::createFile(const std::string& path) { + return std::make_unique(path); } bool InstanceImpl::fileExists(const std::string& path) { @@ -119,164 +146,5 @@ bool InstanceImpl::illegalPath(const std::string& path) { return false; } -FileImpl::FileImpl(const std::string& path, Event::Dispatcher& dispatcher, - Thread::BasicLockable& lock, FileSystemStats& stats, - std::chrono::milliseconds flush_interval_msec, - Thread::ThreadFactory& thread_factory) - : path_(path), file_lock_(lock), flush_timer_(dispatcher.createTimer([this]() -> void { - stats_.flushed_by_timer_.inc(); - flush_event_.notifyOne(); - flush_timer_->enableTimer(flush_interval_msec_); - })), - os_sys_calls_(Api::OsSysCallsSingleton::get()), thread_factory_(thread_factory), - flush_interval_msec_(flush_interval_msec), stats_(stats) { - open(); -} - -void FileImpl::open() { - Api::SysCallIntResult result = - os_sys_calls_.open(path_, O_RDWR | O_APPEND | O_CREAT, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); - fd_ = result.rc_; - if (-1 == fd_) { - throw EnvoyException( - fmt::format("unable to open file '{}': {}", path_, strerror(result.errno_))); - } -} - -void FileImpl::reopen() { reopen_file_ = true; } - -FileImpl::~FileImpl() { - { - Thread::LockGuard lock(write_lock_); - flush_thread_exit_ = true; - flush_event_.notifyOne(); - } - - if (flush_thread_ != nullptr) { - flush_thread_->join(); - } - - // Flush any remaining data. If file was not opened for some reason, skip flushing part. - if (fd_ != -1) { - if (flush_buffer_.length() > 0) { - doWrite(flush_buffer_); - } - - os_sys_calls_.close(fd_); - } -} - -void FileImpl::doWrite(Buffer::Instance& buffer) { - uint64_t num_slices = buffer.getRawSlices(nullptr, 0); - STACK_ARRAY(slices, Buffer::RawSlice, num_slices); - buffer.getRawSlices(slices.begin(), num_slices); - - // We must do the actual writes to disk under lock, so that we don't intermix chunks from - // different FileImpl pointing to the same underlying file. This can happen either via hot - // restart or if calling code opens the same underlying file into a different FileImpl in the - // same process. - // TODO PERF: Currently, we use a single cross process lock to serialize all disk writes. This - // will never block network workers, but does mean that only a single flush thread can - // actually flush to disk. In the future it would be nice if we did away with the cross - // process lock or had multiple locks. - { - Thread::LockGuard lock(file_lock_); - for (const Buffer::RawSlice& slice : slices) { - const Api::SysCallSizeResult result = os_sys_calls_.write(fd_, slice.mem_, slice.len_); - ASSERT(result.rc_ == static_cast(slice.len_)); - stats_.write_completed_.inc(); - } - } - - stats_.write_total_buffered_.sub(buffer.length()); - buffer.drain(buffer.length()); -} - -void FileImpl::flushThreadFunc() { - - while (true) { - std::unique_lock flush_lock; - - { - Thread::LockGuard write_lock(write_lock_); - - // flush_event_ can be woken up either by large enough flush_buffer or by timer. - // In case it was timer, flush_buffer_ can be empty. - while (flush_buffer_.length() == 0 && !flush_thread_exit_) { - // CondVar::wait() does not throw, so it's safe to pass the mutex rather than the guard. - flush_event_.wait(write_lock_); - } - - if (flush_thread_exit_) { - return; - } - - flush_lock = std::unique_lock(flush_lock_); - ASSERT(flush_buffer_.length() > 0); - about_to_write_buffer_.move(flush_buffer_); - ASSERT(flush_buffer_.length() == 0); - } - - // if we failed to open file before (-1 == fd_), then simply ignore - if (fd_ != -1) { - try { - if (reopen_file_) { - reopen_file_ = false; - os_sys_calls_.close(fd_); - open(); - } - - doWrite(about_to_write_buffer_); - } catch (const EnvoyException&) { - stats_.reopen_failed_.inc(); - } - } - } -} - -void FileImpl::flush() { - std::unique_lock flush_buffer_lock; - - { - Thread::LockGuard write_lock(write_lock_); - - // flush_lock_ must be held while checking this or else it is - // possible that flushThreadFunc() has already moved data from - // flush_buffer_ to about_to_write_buffer_, has unlocked write_lock_, - // but has not yet completed doWrite(). This would allow flush() to - // return before the pending data has actually been written to disk. - flush_buffer_lock = std::unique_lock(flush_lock_); - - if (flush_buffer_.length() == 0) { - return; - } - - about_to_write_buffer_.move(flush_buffer_); - ASSERT(flush_buffer_.length() == 0); - } - - doWrite(about_to_write_buffer_); -} - -void FileImpl::write(absl::string_view data) { - Thread::LockGuard lock(write_lock_); - - if (flush_thread_ == nullptr) { - createFlushStructures(); - } - - stats_.write_buffered_.inc(); - stats_.write_total_buffered_.add(data.length()); - flush_buffer_.add(data.data(), data.size()); - if (flush_buffer_.length() > MIN_FLUSH_SIZE) { - flush_event_.notifyOne(); - } -} - -void FileImpl::createFlushStructures() { - flush_thread_ = thread_factory_.createThread([this]() -> void { flushThreadFunc(); }); - flush_timer_->enableTimer(flush_interval_msec_); -} - } // namespace Filesystem } // namespace Envoy diff --git a/source/common/filesystem/filesystem_impl.h b/source/common/filesystem/filesystem_impl.h index a28317bbfa2d5..ec6424932323e 100644 --- a/source/common/filesystem/filesystem_impl.h +++ b/source/common/filesystem/filesystem_impl.h @@ -1,145 +1,43 @@ #pragma once -#include -#include #include -#include #include -#include "envoy/api/api.h" -#include "envoy/api/os_sys_calls.h" -#include "envoy/event/dispatcher.h" #include "envoy/filesystem/filesystem.h" -#include "envoy/stats/stats_macros.h" -#include "envoy/stats/store.h" - -#include "common/buffer/buffer_impl.h" -#include "common/common/thread.h" namespace Envoy { -// clang-format off -#define FILESYSTEM_STATS(COUNTER, GAUGE) \ - COUNTER(write_buffered) \ - COUNTER(write_completed) \ - COUNTER(flushed_by_timer) \ - COUNTER(reopen_failed) \ - GAUGE (write_total_buffered) -// clang-format on +namespace Filesystem { -struct FileSystemStats { - FILESYSTEM_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT) -}; +class FileImpl : public File { +public: + FileImpl(const std::string& path); + ~FileImpl(); -namespace Filesystem { + // Filesystem::File + Api::SysCallBoolResult open() override; + Api::SysCallSizeResult write(absl::string_view buffer) override; + Api::SysCallBoolResult close() override; + bool isOpen() override; + std::string path() override; + std::string errorToString(int error) override; + +private: + int fd_; + const std::string path_; + friend class FileSystemImplTest; +}; -/** - * Captures state, properties, and stats of a file-system. - */ class InstanceImpl : public Instance { public: - InstanceImpl(std::chrono::milliseconds file_flush_interval_msec, - Thread::ThreadFactory& thread_factory, Stats::Store& store); - // Filesystem::Instance - FileSharedPtr createFile(const std::string& path, Event::Dispatcher& dispatcher, - Thread::BasicLockable& lock, - std::chrono::milliseconds file_flush_interval_msec) override; - FileSharedPtr createFile(const std::string& path, Event::Dispatcher& dispatcher, - Thread::BasicLockable& lock) override; + FilePtr createFile(const std::string& path) override; bool fileExists(const std::string& path) override; bool directoryExists(const std::string& path) override; ssize_t fileSize(const std::string& path) override; std::string fileReadToEnd(const std::string& path) override; Api::SysCallStringResult canonicalPath(const std::string& path) override; bool illegalPath(const std::string& path) override; - -private: - const std::chrono::milliseconds file_flush_interval_msec_; - FileSystemStats file_stats_; - Thread::ThreadFactory& thread_factory_; -}; - -/** - * This is a file implementation geared for writing out access logs. It turn out that in certain - * cases even if a standard file is opened with O_NONBLOCK, the kernel can still block when writing. - * This implementation uses a flush thread per file, with the idea there there aren't that many - * files. If this turns out to be a good implementation we can potentially have a single flush - * thread that flushes all files, but we will start with this. - */ -class FileImpl : public File { -public: - FileImpl(const std::string& path, Event::Dispatcher& dispatcher, Thread::BasicLockable& lock, - FileSystemStats& stats_, std::chrono::milliseconds flush_interval_msec, - Thread::ThreadFactory& thread_factory); - ~FileImpl(); - - // Filesystem::File - void write(absl::string_view data) override; - - /** - * Filesystem::File - * Reopen file asynchronously. - * This only sets reopen flag, actual reopen operation is delayed. - * Reopen happens before the next write operation. - */ - void reopen() override; - - // Filesystem::File - void flush() override; - -private: - void doWrite(Buffer::Instance& buffer); - void flushThreadFunc(); - void open(); - void createFlushStructures(); - - // Minimum size before the flush thread will be told to flush. - static const uint64_t MIN_FLUSH_SIZE = 1024 * 64; - - int fd_; - std::string path_; - - // These locks are always acquired in the following order if multiple locks are held: - // 1) write_lock_ - // 2) flush_lock_ - // 3) file_lock_ - Thread::BasicLockable& file_lock_; // This lock is used only by the flush thread when writing - // to disk. This is used to make sure that file blocks do - // not get interleaved by multiple processes writing to - // the same file during hot-restart. - Thread::MutexBasicLockable flush_lock_; // This lock is used to prevent simultaneous flushes from - // the flush thread and a synchronous flush. This protects - // concurrent access to the about_to_write_buffer_, fd_, - // and all other data used during flushing and file - // re-opening. - Thread::MutexBasicLockable - write_lock_; // The lock is used when filling the flush buffer. It allows - // multiple threads to write to the same file at relatively - // high performance. It is always local to the process. - Thread::ThreadPtr flush_thread_; - Thread::CondVar flush_event_; - std::atomic flush_thread_exit_{}; - std::atomic reopen_file_{}; - Buffer::OwnedImpl - flush_buffer_ GUARDED_BY(write_lock_); // This buffer is used by multiple threads. It gets - // filled and then flushed either when max size is - // reached or when a timer fires. - // TODO(jmarantz): this should be GUARDED_BY(flush_lock_) but the analysis cannot poke through - // the std::make_unique assignment. I do not believe it's possible to annotate this properly now - // due to limitations in the clang thread annotation analysis. - Buffer::OwnedImpl about_to_write_buffer_; // This buffer is used only by the flush thread. Data - // is moved from flush_buffer_ under lock, and then - // the lock is released so that flush_buffer_ can - // continue to fill. This buffer is then used for the - // final write to disk. - Event::TimerPtr flush_timer_; - Api::OsSysCalls& os_sys_calls_; - Thread::ThreadFactory& thread_factory_; - const std::chrono::milliseconds flush_interval_msec_; // Time interval buffer gets flushed no - // matter if it reached the MIN_FLUSH_SIZE - // or not. - FileSystemStats& stats_; }; } // namespace Filesystem -} // namespace Envoy +} // namespace Envoy \ No newline at end of file diff --git a/source/common/upstream/health_checker_base_impl.h b/source/common/upstream/health_checker_base_impl.h index 1485d1020d74a..2fd1d7f56a91f 100644 --- a/source/common/upstream/health_checker_base_impl.h +++ b/source/common/upstream/health_checker_base_impl.h @@ -162,7 +162,7 @@ class HealthCheckEventLoggerImpl : public HealthCheckEventLogger { const HostDescription& host, std::function callback) const; TimeSource& time_source_; - Filesystem::FileSharedPtr file_; + AccessLog::AccessLogFileSharedPtr file_; }; } // namespace Upstream diff --git a/source/common/upstream/outlier_detection_impl.h b/source/common/upstream/outlier_detection_impl.h index 639a483c9abd6..fb7efd2b03ea6 100644 --- a/source/common/upstream/outlier_detection_impl.h +++ b/source/common/upstream/outlier_detection_impl.h @@ -278,7 +278,7 @@ class EventLoggerImpl : public EventLogger { const HostDescriptionConstSharedPtr& host, absl::optional time); - Filesystem::FileSharedPtr file_; + AccessLog::AccessLogFileSharedPtr file_; TimeSource& time_source_; }; diff --git a/source/extensions/access_loggers/file/file_access_log_impl.h b/source/extensions/access_loggers/file/file_access_log_impl.h index 93bee65b0f7ad..b14b396befd8e 100644 --- a/source/extensions/access_loggers/file/file_access_log_impl.h +++ b/source/extensions/access_loggers/file/file_access_log_impl.h @@ -21,7 +21,7 @@ class FileAccessLog : public AccessLog::Instance { const StreamInfo::StreamInfo& stream_info) override; private: - Filesystem::FileSharedPtr log_file_; + AccessLog::AccessLogFileSharedPtr log_file_; AccessLog::FilterPtr filter_; AccessLog::FormatterPtr formatter_; }; diff --git a/source/extensions/filters/network/mongo_proxy/proxy.h b/source/extensions/filters/network/mongo_proxy/proxy.h index b43b07cff2a9d..c1e183e93bb11 100644 --- a/source/extensions/filters/network/mongo_proxy/proxy.h +++ b/source/extensions/filters/network/mongo_proxy/proxy.h @@ -93,7 +93,7 @@ class AccessLog { private: TimeSource& time_source_; - Filesystem::FileSharedPtr file_; + Envoy::AccessLog::AccessLogFileSharedPtr file_; }; typedef std::shared_ptr AccessLogSharedPtr; diff --git a/source/server/config_validation/api.cc b/source/server/config_validation/api.cc index 1080a53ff11db..f03da1c16a1ca 100644 --- a/source/server/config_validation/api.cc +++ b/source/server/config_validation/api.cc @@ -7,11 +7,9 @@ namespace Envoy { namespace Api { -ValidationImpl::ValidationImpl(std::chrono::milliseconds file_flush_interval_msec, - Thread::ThreadFactory& thread_factory, Stats::Store& stats_store, +ValidationImpl::ValidationImpl(Thread::ThreadFactory& thread_factory, Stats::Store& stats_store, Event::TimeSystem& time_system) - : Impl(file_flush_interval_msec, thread_factory, stats_store, time_system), - time_system_(time_system) {} + : Impl(thread_factory, stats_store, time_system), time_system_(time_system) {} Event::DispatcherPtr ValidationImpl::allocateDispatcher() { return Event::DispatcherPtr{new Event::ValidationDispatcher(*this, time_system_)}; diff --git a/source/server/config_validation/api.h b/source/server/config_validation/api.h index 023f39c572445..46161e14aa894 100644 --- a/source/server/config_validation/api.h +++ b/source/server/config_validation/api.h @@ -15,8 +15,7 @@ namespace Api { */ class ValidationImpl : public Impl { public: - ValidationImpl(std::chrono::milliseconds file_flush_interval_msec, - Thread::ThreadFactory& thread_factory, Stats::Store& stats_store, + ValidationImpl(Thread::ThreadFactory& thread_factory, Stats::Store& stats_store, Event::TimeSystem& time_system); Event::DispatcherPtr allocateDispatcher() override; diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 0acb8a5cbc10f..4cd901bc5df91 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -41,12 +41,12 @@ ValidationInstance::ValidationInstance(const Options& options, Event::TimeSystem ComponentFactory& component_factory, Thread::ThreadFactory& thread_factory) : options_(options), stats_store_(store), - api_(new Api::ValidationImpl(options.fileFlushIntervalMsec(), thread_factory, store, - time_system)), + api_(new Api::ValidationImpl(thread_factory, store, time_system)), dispatcher_(api_->allocateDispatcher()), singleton_manager_(new Singleton::ManagerImpl(api_->threadFactory().currentThreadId())), - access_log_manager_(*api_, *dispatcher_, access_log_lock), mutex_tracer_(nullptr), - time_system_(time_system) { + access_log_manager_(options.fileFlushIntervalMsec(), *api_, *dispatcher_, access_log_lock, + store), + mutex_tracer_(nullptr), time_system_(time_system) { try { initialize(options, local_address, component_factory); } catch (const EnvoyException& e) { diff --git a/source/server/server.cc b/source/server/server.cc index 5983973e45086..382e260181a56 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -55,15 +55,16 @@ InstanceImpl::InstanceImpl(const Options& options, Event::TimeSystem& time_syste : secret_manager_(std::make_unique()), shutdown_(false), options_(options), time_source_(time_system), restarter_(restarter), start_time_(time(nullptr)), original_start_time_(start_time_), stats_store_(store), - thread_local_(tls), - api_(new Api::Impl(options.fileFlushIntervalMsec(), thread_factory, store, time_system)), + thread_local_(tls), api_(new Api::Impl(thread_factory, store, time_system)), dispatcher_(api_->allocateDispatcher()), singleton_manager_(new Singleton::ManagerImpl(api_->threadFactory().currentThreadId())), handler_(new ConnectionHandlerImpl(ENVOY_LOGGER(), *dispatcher_)), random_generator_(std::move(random_generator)), listener_component_factory_(*this), worker_factory_(thread_local_, *api_, hooks), dns_resolver_(dispatcher_->createDnsResolver({})), - access_log_manager_(*api_, *dispatcher_, access_log_lock), terminated_(false), + access_log_manager_(options.fileFlushIntervalMsec(), *api_, *dispatcher_, access_log_lock, + store), + terminated_(false), mutex_tracer_(options.mutexTracingEnabled() ? &Envoy::MutexTracerImpl::getOrCreateTracer() : nullptr) { diff --git a/test/common/access_log/access_log_impl_test.cc b/test/common/access_log/access_log_impl_test.cc index 899ae69ec8bc9..3b57f66cf3230 100644 --- a/test/common/access_log/access_log_impl_test.cc +++ b/test/common/access_log/access_log_impl_test.cc @@ -50,7 +50,7 @@ envoy::config::filter::accesslog::v2::AccessLog parseAccessLogFromV2Yaml(const s class AccessLogImplTest : public testing::Test { public: - AccessLogImplTest() : file_(new Filesystem::MockFile()) { + AccessLogImplTest() : file_(new MockAccessLogFile()) { ON_CALL(context_, runtime()).WillByDefault(ReturnRef(runtime_)); ON_CALL(context_, accessLogManager()).WillByDefault(ReturnRef(log_manager_)); ON_CALL(log_manager_, createAccessLog(_)).WillByDefault(Return(file_)); @@ -61,7 +61,7 @@ class AccessLogImplTest : public testing::Test { Http::TestHeaderMapImpl response_headers_; Http::TestHeaderMapImpl response_trailers_; TestStreamInfo stream_info_; - std::shared_ptr file_; + std::shared_ptr file_; StringViewSaver output_; NiceMock runtime_; diff --git a/test/common/access_log/access_log_manager_impl_test.cc b/test/common/access_log/access_log_manager_impl_test.cc index 06838ee9df287..8dae2ca1e2189 100644 --- a/test/common/access_log/access_log_manager_impl_test.cc +++ b/test/common/access_log/access_log_manager_impl_test.cc @@ -12,34 +12,326 @@ #include "gtest/gtest.h" using testing::_; +using testing::NiceMock; using testing::Return; +using testing::ReturnNew; using testing::ReturnRef; +using testing::Sequence; namespace Envoy { namespace AccessLog { -TEST(AccessLogManagerImpl, reopenAllFiles) { - Api::MockApi api; - Filesystem::MockInstance file_system; - EXPECT_CALL(api, fileSystem()).WillRepeatedly(ReturnRef(file_system)); - Event::MockDispatcher dispatcher; - Thread::MutexBasicLockable lock; - - std::shared_ptr log1(new Filesystem::MockFile()); - std::shared_ptr log2(new Filesystem::MockFile()); - AccessLogManagerImpl access_log_manager(api, dispatcher, lock); - EXPECT_CALL(file_system, createFile("foo", _, _)).WillOnce(Return(log1)); - access_log_manager.createAccessLog("foo"); - EXPECT_CALL(file_system, createFile("bar", _, _)).WillOnce(Return(log2)); - access_log_manager.createAccessLog("bar"); +class AccessLogManagerImplTest : public testing::Test { +protected: + AccessLogManagerImplTest() + : file_(new NiceMock), thread_factory_(Thread::threadFactoryForTest()), + access_log_manager_(timeout_40ms_, api_, dispatcher_, lock_, store_) { + EXPECT_CALL(file_system_, createFile("foo")) + .WillOnce(Return(ByMove(std::unique_ptr>(file_)))); + + EXPECT_CALL(api_, fileSystem()).WillRepeatedly(ReturnRef(file_system_)); + EXPECT_CALL(api_, threadFactory()).WillRepeatedly(ReturnRef(thread_factory_)); + } + + NiceMock api_; + NiceMock file_system_; + NiceMock* file_; + const std::chrono::milliseconds timeout_40ms_{40}; + Stats::IsolatedStoreImpl store_; + Thread::ThreadFactory& thread_factory_; + NiceMock dispatcher_; + Thread::MutexBasicLockable lock_; + AccessLogManagerImpl access_log_manager_; +}; + +TEST_F(AccessLogManagerImplTest, BadFile) { + EXPECT_CALL(dispatcher_, createTimer_(_)); + EXPECT_CALL(*file_, open_()).WillOnce(Return(Api::SysCallBoolResult{false, 0})); + EXPECT_THROW(access_log_manager_.createAccessLog("foo"), EnvoyException); +} + +TEST_F(AccessLogManagerImplTest, flushToLogFilePeriodically) { + NiceMock* timer = new NiceMock(&dispatcher_); + + EXPECT_CALL(*file_, open_()).WillOnce(Return(Api::SysCallBoolResult{true, 0})); + AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo"); + + EXPECT_CALL(*timer, enableTimer(timeout_40ms_)); + EXPECT_CALL(*file_, write_(_)) + .WillOnce(Invoke([](absl::string_view data) -> Api::SysCallSizeResult { + EXPECT_EQ(0, data.compare("test")); + return {static_cast(data.length()), 0}; + })); + + log_file->write("test"); + + { + Thread::LockGuard lock(file_->write_mutex_); + while (file_->num_writes_ != 1) { + file_->write_event_.wait(file_->write_mutex_); + } + } + + EXPECT_CALL(*file_, write_(_)) + .WillOnce(Invoke([](absl::string_view data) -> Api::SysCallSizeResult { + EXPECT_EQ(0, data.compare("test2")); + return {static_cast(data.length()), 0}; + })); + + // make sure timer is re-enabled on callback call + log_file->write("test2"); + EXPECT_CALL(*timer, enableTimer(timeout_40ms_)); + timer->callback_(); + + { + Thread::LockGuard lock(file_->write_mutex_); + while (file_->num_writes_ != 2) { + file_->write_event_.wait(file_->write_mutex_); + } + } + EXPECT_CALL(*file_, close_()).WillOnce(Return(Api::SysCallBoolResult{true, 0})); +} + +TEST_F(AccessLogManagerImplTest, flushToLogFileOnDemand) { + NiceMock* timer = new NiceMock(&dispatcher_); + + EXPECT_CALL(*file_, open_()).WillOnce(Return(Api::SysCallBoolResult{true, 0})); + AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo"); + + EXPECT_CALL(*timer, enableTimer(timeout_40ms_)); + + // The first write to a given file will start the flush thread, which can flush + // immediately (race on whether it will or not). So do a write and flush to + // get that state out of the way, then test that small writes don't trigger a flush. + EXPECT_CALL(*file_, write_(_)) + .WillOnce(Invoke([](absl::string_view data) -> Api::SysCallSizeResult { + return {static_cast(data.length()), 0}; + })); + log_file->write("prime-it"); + log_file->flush(); + uint32_t expected_writes = 1; + { + Thread::LockGuard lock(file_->write_mutex_); + EXPECT_EQ(expected_writes, file_->num_writes_); + } + + EXPECT_CALL(*file_, write_(_)) + .WillOnce(Invoke([](absl::string_view data) -> Api::SysCallSizeResult { + EXPECT_EQ(0, data.compare("test")); + return {static_cast(data.length()), 0}; + })); + + log_file->write("test"); + + { + Thread::LockGuard lock(file_->write_mutex_); + EXPECT_EQ(expected_writes, file_->num_writes_); + } + + log_file->flush(); + expected_writes++; + { + Thread::LockGuard lock(file_->write_mutex_); + EXPECT_EQ(expected_writes, file_->num_writes_); + } + + EXPECT_CALL(*file_, write_(_)) + .WillOnce(Invoke([](absl::string_view data) -> Api::SysCallSizeResult { + EXPECT_EQ(0, data.compare("test2")); + return {static_cast(data.length()), 0}; + })); + + // make sure timer is re-enabled on callback call + log_file->write("test2"); + EXPECT_CALL(*timer, enableTimer(timeout_40ms_)); + timer->callback_(); + expected_writes++; + + { + Thread::LockGuard lock(file_->write_mutex_); + while (file_->num_writes_ != expected_writes) { + file_->write_event_.wait(file_->write_mutex_); + } + } + EXPECT_CALL(*file_, close_()).WillOnce(Return(Api::SysCallBoolResult{true, 0})); +} + +TEST_F(AccessLogManagerImplTest, reopenFile) { + NiceMock* timer = new NiceMock(&dispatcher_); + + Sequence sq; + EXPECT_CALL(*file_, open_()).InSequence(sq).WillOnce(Return(Api::SysCallBoolResult{true, 0})); + AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo"); + + EXPECT_CALL(*file_, write_(_)) + .InSequence(sq) + .WillOnce(Invoke([](absl::string_view data) -> Api::SysCallSizeResult { + EXPECT_EQ(0, data.compare("before")); + return {static_cast(data.length()), 0}; + })); + + log_file->write("before"); + timer->callback_(); + + { + Thread::LockGuard lock(file_->write_mutex_); + while (file_->num_writes_ != 1) { + file_->write_event_.wait(file_->write_mutex_); + } + } + + EXPECT_CALL(*file_, close_()).InSequence(sq).WillOnce(Return(Api::SysCallBoolResult{true, 0})); + EXPECT_CALL(*file_, open_()).InSequence(sq).WillOnce(Return(Api::SysCallBoolResult{true, 0})); + + EXPECT_CALL(*file_, write_(_)) + .InSequence(sq) + .WillOnce(Invoke([](absl::string_view data) -> Api::SysCallSizeResult { + EXPECT_EQ(0, data.compare("reopened")); + return {static_cast(data.length()), 0}; + })); + + EXPECT_CALL(*file_, close_()).InSequence(sq).WillOnce(Return(Api::SysCallBoolResult{true, 0})); + + log_file->reopen(); + log_file->write("reopened"); + timer->callback_(); + + { + Thread::LockGuard lock(file_->write_mutex_); + while (file_->num_writes_ != 2) { + file_->write_event_.wait(file_->write_mutex_); + } + } +} + +TEST_F(AccessLogManagerImplTest, reopenThrows) { + NiceMock* timer = new NiceMock(&dispatcher_); + + EXPECT_CALL(*file_, write_(_)) + .WillRepeatedly(Invoke([](absl::string_view data) -> Api::SysCallSizeResult { + return {static_cast(data.length()), 0}; + })); + + Sequence sq; + EXPECT_CALL(*file_, open_()).InSequence(sq).WillOnce(Return(Api::SysCallBoolResult{true, 0})); + + AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo"); + EXPECT_CALL(*file_, close_()).InSequence(sq).WillOnce(Return(Api::SysCallBoolResult{true, 0})); + EXPECT_CALL(*file_, open_()).InSequence(sq).WillOnce(Return(Api::SysCallBoolResult{false, 0})); + + log_file->write("test write"); + timer->callback_(); + { + Thread::LockGuard lock(file_->write_mutex_); + while (file_->num_writes_ != 1) { + file_->write_event_.wait(file_->write_mutex_); + } + } + log_file->reopen(); + + log_file->write("this is to force reopen"); + timer->callback_(); + + { + Thread::LockGuard lock(file_->open_mutex_); + while (file_->num_opens_ != 2) { + file_->open_event_.wait(file_->open_mutex_); + } + } + + // write call should not cause any exceptions + log_file->write("random data"); + timer->callback_(); +} + +TEST_F(AccessLogManagerImplTest, bigDataChunkShouldBeFlushedWithoutTimer) { + EXPECT_CALL(*file_, open_()).WillOnce(Return(Api::SysCallBoolResult{true, 0})); + AccessLogFileSharedPtr log_file = access_log_manager_.createAccessLog("foo"); + + EXPECT_CALL(*file_, write_(_)) + .WillOnce(Invoke([](absl::string_view data) -> Api::SysCallSizeResult { + EXPECT_EQ(0, data.compare("a")); + return {static_cast(data.length()), 0}; + })); + + log_file->write("a"); + + { + Thread::LockGuard lock(file_->write_mutex_); + while (file_->num_writes_ != 1) { + file_->write_event_.wait(file_->write_mutex_); + } + } + + // First write happens without waiting on thread_flush_. Now make a big string and it should be + // flushed even when timer is not enabled + EXPECT_CALL(*file_, write_(_)) + .WillOnce(Invoke([](absl::string_view data) -> Api::SysCallSizeResult { + std::string expected(1024 * 64 + 1, 'b'); + EXPECT_EQ(0, data.compare(expected)); + return {static_cast(data.length()), 0}; + })); + + std::string big_string(1024 * 64 + 1, 'b'); + log_file->write(big_string); + + { + Thread::LockGuard lock(file_->write_mutex_); + while (file_->num_writes_ != 2) { + file_->write_event_.wait(file_->write_mutex_); + } + } + EXPECT_CALL(*file_, close_()).WillOnce(Return(Api::SysCallBoolResult{true, 0})); +} + +TEST_F(AccessLogManagerImplTest, reopenAllFiles) { + EXPECT_CALL(dispatcher_, createTimer_(_)).WillRepeatedly(ReturnNew>()); + + EXPECT_CALL(*file_, open_()).Times(2).WillRepeatedly(Return(Api::SysCallBoolResult{true, 0})); + AccessLogFileSharedPtr log = access_log_manager_.createAccessLog("foo"); + + NiceMock* file2 = new NiceMock; + EXPECT_CALL(file_system_, createFile("bar")) + .WillOnce(Return(ByMove(std::unique_ptr>(file2)))); + EXPECT_CALL(*file2, open_()).Times(2).WillRepeatedly(Return(Api::SysCallBoolResult{true, 0})); + AccessLogFileSharedPtr log2 = access_log_manager_.createAccessLog("bar"); // Make sure that getting the access log with the same name returns the same underlying file. - EXPECT_EQ(log1, access_log_manager.createAccessLog("foo")); - EXPECT_EQ(log2, access_log_manager.createAccessLog("bar")); + EXPECT_EQ(log, access_log_manager_.createAccessLog("foo")); + EXPECT_EQ(log2, access_log_manager_.createAccessLog("bar")); + + EXPECT_CALL(*file_, close_()).Times(2).WillRepeatedly(Return(Api::SysCallBoolResult{true, 0})); + EXPECT_CALL(*file2, close_()).Times(2).WillRepeatedly(Return(Api::SysCallBoolResult{true, 0})); + + // Test that reopen reopens all of the files + EXPECT_CALL(*file_, write_(_)) + .WillRepeatedly(Invoke([](absl::string_view data) -> Api::SysCallSizeResult { + return {static_cast(data.length()), 0}; + })); + + EXPECT_CALL(*file2, write_(_)) + .WillRepeatedly(Invoke([](absl::string_view data) -> Api::SysCallSizeResult { + return {static_cast(data.length()), 0}; + })); + + access_log_manager_.reopen(); + + log->write("this is to force reopen"); + log2->write("this is to force reopen"); + + { + Thread::LockGuard lock(file_->open_mutex_); + while (file_->num_opens_ != 2) { + file_->open_event_.wait(file_->open_mutex_); + } + } - EXPECT_CALL(*log1, reopen()); - EXPECT_CALL(*log2, reopen()); - access_log_manager.reopen(); + { + Thread::LockGuard lock(file2->open_mutex_); + while (file2->num_opens_ != 2) { + file2->open_event_.wait(file2->open_mutex_); + } + } } } // namespace AccessLog diff --git a/test/common/filesystem/BUILD b/test/common/filesystem/BUILD index c34f8969df993..6f385036615a6 100644 --- a/test/common/filesystem/BUILD +++ b/test/common/filesystem/BUILD @@ -12,20 +12,8 @@ envoy_cc_test( name = "filesystem_impl_test", srcs = ["filesystem_impl_test.cc"], deps = [ - "//source/common/api:api_lib", - "//source/common/api:os_sys_calls_lib", - "//source/common/common:thread_lib", - "//source/common/event:dispatcher_includes", - "//source/common/event:dispatcher_lib", "//source/common/filesystem:filesystem_lib", - "//source/common/stats:isolated_store_lib", - "//source/common/stats:stats_lib", - "//test/mocks/api:api_mocks", - "//test/mocks/event:event_mocks", - "//test/mocks/filesystem:filesystem_mocks", "//test/test_common:environment_lib", - "//test/test_common:threadsafe_singleton_injector_lib", - "//test/test_common:utility_lib", ], ) @@ -46,7 +34,6 @@ envoy_cc_test( "//source/common/event:dispatcher_includes", "//source/common/event:dispatcher_lib", "//source/common/filesystem:watcher_lib", - "//source/common/stats:isolated_store_lib", "//test/test_common:environment_lib", ], ) diff --git a/test/common/filesystem/filesystem_impl_test.cc b/test/common/filesystem/filesystem_impl_test.cc index 29269f78e65a8..cc7bb2cdff8e5 100644 --- a/test/common/filesystem/filesystem_impl_test.cc +++ b/test/common/filesystem/filesystem_impl_test.cc @@ -1,53 +1,28 @@ #include #include -#include "common/api/api_impl.h" -#include "common/api/os_sys_calls_impl.h" -#include "common/common/lock_guard.h" -#include "common/common/thread.h" -#include "common/event/dispatcher_impl.h" +#include "common/common/assert.h" #include "common/filesystem/filesystem_impl.h" -#include "common/stats/isolated_store_impl.h" -#include "test/mocks/api/mocks.h" -#include "test/mocks/event/mocks.h" -#include "test/mocks/filesystem/mocks.h" #include "test/test_common/environment.h" -#include "test/test_common/threadsafe_singleton_injector.h" -#include "test/test_common/utility.h" #include "gmock/gmock.h" #include "gtest/gtest.h" -using testing::_; -using testing::InSequence; -using testing::Invoke; -using testing::NiceMock; -using testing::Return; -using testing::SaveArg; -using testing::Sequence; -using testing::Throw; - namespace Envoy { +namespace Filesystem { class FileSystemImplTest : public testing::Test { protected: - FileSystemImplTest() - : file_system_(std::chrono::milliseconds(10000), Thread::threadFactoryForTest(), - stats_store_) {} + int getFd(File* file) { + auto file_impl = dynamic_cast(file); + RELEASE_ASSERT(file_impl != nullptr, "failed to cast File* to FileImpl*"); + return file_impl->fd_; + } - const std::chrono::milliseconds timeout_40ms_{40}; - Stats::IsolatedStoreImpl stats_store_; - Filesystem::InstanceImpl file_system_; + InstanceImpl file_system_; }; -TEST_F(FileSystemImplTest, BadFile) { - Event::MockDispatcher dispatcher; - Thread::MutexBasicLockable lock; - EXPECT_CALL(dispatcher, createTimer_(_)); - EXPECT_THROW(file_system_.createFile("", dispatcher, lock), EnvoyException); -} - TEST_F(FileSystemImplTest, fileExists) { EXPECT_TRUE(file_system_.fileExists("/dev/null")); EXPECT_FALSE(file_system_.fileExists("/dev/blahblahblah")); @@ -124,288 +99,110 @@ TEST_F(FileSystemImplTest, IllegalPath) { EXPECT_TRUE(file_system_.illegalPath("/_some_non_existent_file")); } -TEST_F(FileSystemImplTest, flushToLogFilePeriodically) { - NiceMock dispatcher; - NiceMock* timer = new NiceMock(&dispatcher); - - Thread::MutexBasicLockable mutex; - NiceMock os_sys_calls; - TestThreadsafeSingletonInjector os_calls(&os_sys_calls); - - EXPECT_CALL(os_sys_calls, open_(_, _, _)).WillOnce(Return(5)); - Filesystem::FileSharedPtr file = file_system_.createFile("", dispatcher, mutex, timeout_40ms_); - - EXPECT_CALL(*timer, enableTimer(timeout_40ms_)); - EXPECT_CALL(os_sys_calls, write_(_, _, _)) - .WillOnce(Invoke([](int fd, const void* buffer, size_t num_bytes) -> ssize_t { - std::string written = std::string(reinterpret_cast(buffer), num_bytes); - EXPECT_EQ("test", written); - EXPECT_EQ(5, fd); - - return num_bytes; - })); - - file->write("test"); +TEST_F(FileSystemImplTest, ConstructedFileNotOpen) { + const std::string new_file_path = TestEnvironment::temporaryPath("envoy_this_not_exist"); + ::unlink(new_file_path.c_str()); - { - Thread::LockGuard lock(os_sys_calls.write_mutex_); - while (os_sys_calls.num_writes_ != 1) { - os_sys_calls.write_event_.wait(os_sys_calls.write_mutex_); - } - } - - EXPECT_CALL(os_sys_calls, write_(_, _, _)) - .WillOnce(Invoke([](int fd, const void* buffer, size_t num_bytes) -> ssize_t { - std::string written = std::string(reinterpret_cast(buffer), num_bytes); - EXPECT_EQ("test2", written); - EXPECT_EQ(5, fd); - - return num_bytes; - })); - - // make sure timer is re-enabled on callback call - file->write("test2"); - EXPECT_CALL(*timer, enableTimer(timeout_40ms_)); - timer->callback_(); - - { - Thread::LockGuard lock(os_sys_calls.write_mutex_); - while (os_sys_calls.num_writes_ != 2) { - os_sys_calls.write_event_.wait(os_sys_calls.write_mutex_); - } - } + FilePtr file = file_system_.createFile(new_file_path); + EXPECT_FALSE(file->isOpen()); } -TEST_F(FileSystemImplTest, flushToLogFileOnDemand) { - NiceMock dispatcher; - NiceMock* timer = new NiceMock(&dispatcher); - - Thread::MutexBasicLockable mutex; - NiceMock os_sys_calls; - TestThreadsafeSingletonInjector os_calls(&os_sys_calls); - - EXPECT_CALL(os_sys_calls, open_(_, _, _)).WillOnce(Return(5)); - Filesystem::FileSharedPtr file = file_system_.createFile("", dispatcher, mutex, timeout_40ms_); - - EXPECT_CALL(*timer, enableTimer(timeout_40ms_)); - - // The first write to a given file will start the flush thread, which can flush - // immediately (race on whether it will or not). So do a write and flush to - // get that state out of the way, then test that small writes don't trigger a flush. - EXPECT_CALL(os_sys_calls, write_(_, _, _)) - .WillOnce(Invoke([](int, const void*, size_t num_bytes) -> ssize_t { return num_bytes; })); - file->write("prime-it"); - file->flush(); - uint32_t expected_writes = 1; - { - Thread::LockGuard lock(os_sys_calls.write_mutex_); - EXPECT_EQ(expected_writes, os_sys_calls.num_writes_); - } - - EXPECT_CALL(os_sys_calls, write_(_, _, _)) - .WillOnce(Invoke([](int fd, const void* buffer, size_t num_bytes) -> ssize_t { - std::string written = std::string(reinterpret_cast(buffer), num_bytes); - EXPECT_EQ("test", written); - EXPECT_EQ(5, fd); - - return num_bytes; - })); - - file->write("test"); - - { - Thread::LockGuard lock(os_sys_calls.write_mutex_); - EXPECT_EQ(expected_writes, os_sys_calls.num_writes_); - } - - file->flush(); - expected_writes++; - { - Thread::LockGuard lock(os_sys_calls.write_mutex_); - EXPECT_EQ(expected_writes, os_sys_calls.num_writes_); - } - - EXPECT_CALL(os_sys_calls, write_(_, _, _)) - .WillOnce(Invoke([](int fd, const void* buffer, size_t num_bytes) -> ssize_t { - std::string written = std::string(reinterpret_cast(buffer), num_bytes); - EXPECT_EQ("test2", written); - EXPECT_EQ(5, fd); - - return num_bytes; - })); +TEST_F(FileSystemImplTest, Open) { + const std::string new_file_path = TestEnvironment::temporaryPath("envoy_this_not_exist"); + ::unlink(new_file_path.c_str()); - // make sure timer is re-enabled on callback call - file->write("test2"); - EXPECT_CALL(*timer, enableTimer(timeout_40ms_)); - timer->callback_(); - expected_writes++; - - { - Thread::LockGuard lock(os_sys_calls.write_mutex_); - while (os_sys_calls.num_writes_ != expected_writes) { - os_sys_calls.write_event_.wait(os_sys_calls.write_mutex_); - } - } + FilePtr file = file_system_.createFile(new_file_path); + const Api::SysCallBoolResult result = file->open(); + EXPECT_TRUE(result.rc_); + EXPECT_TRUE(file->isOpen()); } -TEST_F(FileSystemImplTest, reopenFile) { - NiceMock dispatcher; - NiceMock* timer = new NiceMock(&dispatcher); +TEST_F(FileSystemImplTest, OpenTwice) { + const std::string new_file_path = TestEnvironment::temporaryPath("envoy_this_not_exist"); + ::unlink(new_file_path.c_str()); - Thread::MutexBasicLockable mutex; - NiceMock os_sys_calls; - TestThreadsafeSingletonInjector os_calls(&os_sys_calls); + FilePtr file = file_system_.createFile(new_file_path); + EXPECT_EQ(getFd(file.get()), -1); - Sequence sq; - EXPECT_CALL(os_sys_calls, open_(_, _, _)).InSequence(sq).WillOnce(Return(5)); - Filesystem::FileSharedPtr file = file_system_.createFile("", dispatcher, mutex, timeout_40ms_); + Api::SysCallBoolResult result = file->open(); + const int initial_fd = getFd(file.get()); + EXPECT_TRUE(result.rc_); + EXPECT_TRUE(file->isOpen()); - EXPECT_CALL(os_sys_calls, write_(_, _, _)) - .InSequence(sq) - .WillOnce(Invoke([](int fd, const void* buffer, size_t num_bytes) -> ssize_t { - std::string written = std::string(reinterpret_cast(buffer), num_bytes); - EXPECT_EQ("before", written); - EXPECT_EQ(5, fd); + // check that we don't leak a file descriptor + result = file->open(); + EXPECT_EQ(initial_fd, getFd(file.get())); + EXPECT_TRUE(result.rc_); + EXPECT_TRUE(file->isOpen()); +} - return num_bytes; - })); +TEST_F(FileSystemImplTest, OpenBadFilePath) { + FilePtr file = file_system_.createFile(""); + const Api::SysCallBoolResult result = file->open(); + EXPECT_FALSE(result.rc_); +} - file->write("before"); - timer->callback_(); +TEST_F(FileSystemImplTest, ExistingFile) { + const std::string file_path = + TestEnvironment::writeStringToFileForTest("test_envoy", "existing file"); { - Thread::LockGuard lock(os_sys_calls.write_mutex_); - while (os_sys_calls.num_writes_ != 1) { - os_sys_calls.write_event_.wait(os_sys_calls.write_mutex_); - } + FilePtr file = file_system_.createFile(file_path); + const Api::SysCallBoolResult open_result = file->open(); + EXPECT_TRUE(open_result.rc_); + std::string data(" new data"); + const Api::SysCallSizeResult result = file->write(data); + EXPECT_EQ(data.length(), result.rc_); } - EXPECT_CALL(os_sys_calls, close(5)).InSequence(sq); - EXPECT_CALL(os_sys_calls, open_(_, _, _)).InSequence(sq).WillOnce(Return(10)); - - EXPECT_CALL(os_sys_calls, write_(_, _, _)) - .InSequence(sq) - .WillOnce(Invoke([](int fd, const void* buffer, size_t num_bytes) -> ssize_t { - std::string written = std::string(reinterpret_cast(buffer), num_bytes); - EXPECT_EQ("reopened", written); - EXPECT_EQ(10, fd); - - return num_bytes; - })); - - EXPECT_CALL(os_sys_calls, close(10)).InSequence(sq); - - file->reopen(); - file->write("reopened"); - timer->callback_(); - - { - Thread::LockGuard lock(os_sys_calls.write_mutex_); - while (os_sys_calls.num_writes_ != 2) { - os_sys_calls.write_event_.wait(os_sys_calls.write_mutex_); - } - } + auto contents = TestEnvironment::readFileToStringForTest(file_path); + EXPECT_EQ("existing file new data", contents); } -TEST_F(FileSystemImplTest, reopenThrows) { - NiceMock dispatcher; - NiceMock* timer = new NiceMock(&dispatcher); - - Thread::MutexBasicLockable mutex; - Stats::IsolatedStoreImpl stats_store; - NiceMock os_sys_calls; - TestThreadsafeSingletonInjector os_calls(&os_sys_calls); - - EXPECT_CALL(os_sys_calls, write_(_, _, _)) - .WillRepeatedly(Invoke([](int fd, const void* buffer, size_t num_bytes) -> ssize_t { - UNREFERENCED_PARAMETER(fd); - UNREFERENCED_PARAMETER(buffer); - - return num_bytes; - })); - - Sequence sq; - EXPECT_CALL(os_sys_calls, open_(_, _, _)).InSequence(sq).WillOnce(Return(5)); - - Filesystem::FileSharedPtr file = file_system_.createFile("", dispatcher, mutex, timeout_40ms_); - EXPECT_CALL(os_sys_calls, close(5)).InSequence(sq); - EXPECT_CALL(os_sys_calls, open_(_, _, _)).InSequence(sq).WillOnce(Return(-1)); +TEST_F(FileSystemImplTest, NonExistingFile) { + const std::string new_file_path = TestEnvironment::temporaryPath("envoy_this_not_exist"); + ::unlink(new_file_path.c_str()); - file->write("test write"); - timer->callback_(); { - Thread::LockGuard lock(os_sys_calls.write_mutex_); - while (os_sys_calls.num_writes_ != 1) { - os_sys_calls.write_event_.wait(os_sys_calls.write_mutex_); - } + FilePtr file = file_system_.createFile(new_file_path); + const Api::SysCallBoolResult open_result = file->open(); + EXPECT_TRUE(open_result.rc_); + std::string data(" new data"); + const Api::SysCallSizeResult result = file->write(data); + EXPECT_EQ(data.length(), result.rc_); } - file->reopen(); - file->write("this is to force reopen"); - timer->callback_(); - - { - Thread::LockGuard lock(os_sys_calls.open_mutex_); - while (os_sys_calls.num_open_ != 2) { - os_sys_calls.open_event_.wait(os_sys_calls.open_mutex_); - } - } - - // write call should not cause any exceptions - file->write("random data"); - timer->callback_(); + auto contents = TestEnvironment::readFileToStringForTest(new_file_path); + EXPECT_EQ(" new data", contents); } -TEST_F(FileSystemImplTest, bigDataChunkShouldBeFlushedWithoutTimer) { - NiceMock dispatcher; - Thread::MutexBasicLockable mutex; - Stats::IsolatedStoreImpl stats_store; - NiceMock os_sys_calls; - TestThreadsafeSingletonInjector os_calls(&os_sys_calls); - - Filesystem::FileSharedPtr file = file_system_.createFile("", dispatcher, mutex, timeout_40ms_); +TEST_F(FileSystemImplTest, Close) { + const std::string new_file_path = TestEnvironment::temporaryPath("envoy_this_not_exist"); + ::unlink(new_file_path.c_str()); - EXPECT_CALL(os_sys_calls, write_(_, _, _)) - .WillOnce(Invoke([](int fd, const void* buffer, size_t num_bytes) -> ssize_t { - UNREFERENCED_PARAMETER(fd); + FilePtr file = file_system_.createFile(new_file_path); + Api::SysCallBoolResult result = file->open(); + EXPECT_TRUE(result.rc_); + EXPECT_TRUE(file->isOpen()); - std::string written = std::string(reinterpret_cast(buffer), num_bytes); - std::string expected("a"); - EXPECT_EQ(expected, written); - - return num_bytes; - })); - - file->write("a"); - - { - Thread::LockGuard lock(os_sys_calls.write_mutex_); - while (os_sys_calls.num_writes_ != 1) { - os_sys_calls.write_event_.wait(os_sys_calls.write_mutex_); - } - } - - // First write happens without waiting on thread_flush_. Now make a big string and it should be - // flushed even when timer is not enabled - EXPECT_CALL(os_sys_calls, write_(_, _, _)) - .WillOnce(Invoke([](int fd, const void* buffer, size_t num_bytes) -> ssize_t { - UNREFERENCED_PARAMETER(fd); - - std::string written = std::string(reinterpret_cast(buffer), num_bytes); - std::string expected(1024 * 64 + 1, 'b'); - EXPECT_EQ(expected, written); - - return num_bytes; - })); + result = file->close(); + EXPECT_TRUE(result.rc_); + EXPECT_FALSE(file->isOpen()); +} - std::string big_string(1024 * 64 + 1, 'b'); - file->write(big_string); +TEST_F(FileSystemImplTest, WriteAfterClose) { + const std::string new_file_path = TestEnvironment::temporaryPath("envoy_this_not_exist"); + ::unlink(new_file_path.c_str()); - { - Thread::LockGuard lock(os_sys_calls.write_mutex_); - while (os_sys_calls.num_writes_ != 2) { - os_sys_calls.write_event_.wait(os_sys_calls.write_mutex_); - } - } + FilePtr file = file_system_.createFile(new_file_path); + Api::SysCallBoolResult bool_result = file->open(); + EXPECT_TRUE(bool_result.rc_); + bool_result = file->close(); + EXPECT_TRUE(bool_result.rc_); + const Api::SysCallSizeResult result = file->write(" new data"); + EXPECT_EQ(-1, result.rc_); + EXPECT_EQ(EBADF, result.errno_); } + +} // namespace Filesystem } // namespace Envoy diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index 8c2b29fbd8f46..0e8d56ca695bc 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -3656,7 +3656,7 @@ TEST(Printer, HealthTransitionPrinter) { TEST(HealthCheckEventLoggerImplTest, All) { AccessLog::MockAccessLogManager log_manager; - std::shared_ptr file(new Filesystem::MockFile()); + std::shared_ptr file(new AccessLog::MockAccessLogFile()); EXPECT_CALL(log_manager, createAccessLog("foo")).WillOnce(Return(file)); std::shared_ptr host(new NiceMock()); diff --git a/test/common/upstream/outlier_detection_impl_test.cc b/test/common/upstream/outlier_detection_impl_test.cc index bc1408b8744e0..f27facd28094c 100644 --- a/test/common/upstream/outlier_detection_impl_test.cc +++ b/test/common/upstream/outlier_detection_impl_test.cc @@ -772,7 +772,7 @@ TEST(DetectorHostMonitorNullImplTest, All) { TEST(OutlierDetectionEventLoggerImplTest, All) { AccessLog::MockAccessLogManager log_manager; - std::shared_ptr file(new Filesystem::MockFile()); + std::shared_ptr file(new AccessLog::MockAccessLogFile()); NiceMock cluster; std::shared_ptr host(new NiceMock()); ON_CALL(*host, cluster()).WillByDefault(ReturnRef(cluster)); diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index 54f8ef9d10e25..d84fe3c8b505a 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -235,7 +235,7 @@ class AdminRequestTest : public MainCommonTest { TEST_P(AdminRequestTest, AdminRequestGetStatsAndQuit) { startEnvoy(); started_.WaitForNotification(); - EXPECT_THAT(adminRequest("/stats", "GET"), HasSubstr("filesystem.reopen_failed")); + EXPECT_THAT(adminRequest("/stats", "GET"), HasSubstr("access_log_file.reopen_failed")); adminRequest("/quitquitquit", "POST"); EXPECT_TRUE(waitForEnvoyToExit()); } @@ -245,7 +245,7 @@ TEST_P(AdminRequestTest, AdminRequestGetStatsAndQuit) { TEST_P(AdminRequestTest, AdminRequestGetStatsAndKill) { startEnvoy(); started_.WaitForNotification(); - EXPECT_THAT(adminRequest("/stats", "GET"), HasSubstr("filesystem.reopen_failed")); + EXPECT_THAT(adminRequest("/stats", "GET"), HasSubstr("access_log_file.reopen_failed")); kill(getpid(), SIGTERM); EXPECT_TRUE(waitForEnvoyToExit()); } @@ -255,7 +255,7 @@ TEST_P(AdminRequestTest, AdminRequestGetStatsAndKill) { TEST_P(AdminRequestTest, AdminRequestGetStatsAndCtrlC) { startEnvoy(); started_.WaitForNotification(); - EXPECT_THAT(adminRequest("/stats", "GET"), HasSubstr("filesystem.reopen_failed")); + EXPECT_THAT(adminRequest("/stats", "GET"), HasSubstr("access_log_file.reopen_failed")); kill(getpid(), SIGINT); EXPECT_TRUE(waitForEnvoyToExit()); } @@ -318,7 +318,7 @@ TEST_P(AdminRequestTest, AdminRequestBeforeRun) { EXPECT_TRUE(admin_handler_was_called); // This just checks that some stat output was reported. We could pick any stat. - EXPECT_THAT(out, HasSubstr("filesystem.reopen_failed")); + EXPECT_THAT(out, HasSubstr("access_log_file.reopen_failed")); } // Class to track whether an object has been destroyed, which it does by bumping an atomic. diff --git a/test/extensions/filters/network/mongo_proxy/BUILD b/test/extensions/filters/network/mongo_proxy/BUILD index 21ada59634012..3eb1f8fa0cf9b 100644 --- a/test/extensions/filters/network/mongo_proxy/BUILD +++ b/test/extensions/filters/network/mongo_proxy/BUILD @@ -46,7 +46,6 @@ envoy_extension_cc_test( "//test/common/stream_info:test_util", "//test/mocks/access_log:access_log_mocks", "//test/mocks/event:event_mocks", - "//test/mocks/filesystem:filesystem_mocks", "//test/mocks/network:network_mocks", "//test/mocks/runtime:runtime_mocks", ], diff --git a/test/extensions/filters/network/mongo_proxy/proxy_test.cc b/test/extensions/filters/network/mongo_proxy/proxy_test.cc index ffa9b16112292..5a27208b02f5b 100644 --- a/test/extensions/filters/network/mongo_proxy/proxy_test.cc +++ b/test/extensions/filters/network/mongo_proxy/proxy_test.cc @@ -14,7 +14,6 @@ #include "test/common/stream_info/test_util.h" #include "test/mocks/access_log/mocks.h" #include "test/mocks/event/mocks.h" -#include "test/mocks/filesystem/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/runtime/mocks.h" #include "test/test_common/printers.h" @@ -115,7 +114,8 @@ class MongoProxyFilterTest : public testing::Test { NiceMock store_; NiceMock runtime_; NiceMock dispatcher_; - std::shared_ptr file_{new NiceMock()}; + std::shared_ptr file_{ + new NiceMock()}; AccessLogSharedPtr access_log_; FaultConfigSharedPtr fault_config_; std::unique_ptr filter_; diff --git a/test/integration/utility.cc b/test/integration/utility.cc index f7d46289ceece..76fc69623a202 100644 --- a/test/integration/utility.cc +++ b/test/integration/utility.cc @@ -62,8 +62,7 @@ IntegrationUtil::makeSingleRequest(const Network::Address::InstanceConstSharedPt NiceMock mock_stats_store; Event::GlobalTimeSystem time_system; - Api::Impl api(std::chrono::milliseconds(9000), Thread::threadFactoryForTest(), mock_stats_store, - time_system); + Api::Impl api(Thread::threadFactoryForTest(), mock_stats_store, time_system); Event::DispatcherPtr dispatcher(api.allocateDispatcher()); std::shared_ptr cluster{new NiceMock()}; Upstream::HostDescriptionConstSharedPtr host_description{ diff --git a/test/mocks/access_log/mocks.cc b/test/mocks/access_log/mocks.cc index 6f7edb0232aa2..0220ae53f4504 100644 --- a/test/mocks/access_log/mocks.cc +++ b/test/mocks/access_log/mocks.cc @@ -10,6 +10,9 @@ using testing::ReturnRef; namespace Envoy { namespace AccessLog { +MockAccessLogFile::MockAccessLogFile() {} +MockAccessLogFile::~MockAccessLogFile() {} + MockFilter::MockFilter() {} MockFilter::~MockFilter() {} diff --git a/test/mocks/access_log/mocks.h b/test/mocks/access_log/mocks.h index cefd0c0c5435b..562d817e896b1 100644 --- a/test/mocks/access_log/mocks.h +++ b/test/mocks/access_log/mocks.h @@ -5,13 +5,22 @@ #include "envoy/access_log/access_log.h" -#include "test/mocks/filesystem/mocks.h" - #include "gmock/gmock.h" namespace Envoy { namespace AccessLog { +class MockAccessLogFile : public AccessLogFile { +public: + MockAccessLogFile(); + ~MockAccessLogFile(); + + // AccessLog::AccessLogFile + MOCK_METHOD1(write, void(absl::string_view data)); + MOCK_METHOD0(reopen, void()); + MOCK_METHOD0(flush, void()); +}; + class MockFilter : public Filter { public: MockFilter(); @@ -31,9 +40,9 @@ class MockAccessLogManager : public AccessLogManager { // AccessLog::AccessLogManager MOCK_METHOD0(reopen, void()); - MOCK_METHOD1(createAccessLog, Filesystem::FileSharedPtr(const std::string& file_name)); + MOCK_METHOD1(createAccessLog, AccessLogFileSharedPtr(const std::string& file_name)); - std::shared_ptr file_{new testing::NiceMock()}; + std::shared_ptr file_{new testing::NiceMock()}; }; class MockInstance : public Instance { diff --git a/test/mocks/api/mocks.cc b/test/mocks/api/mocks.cc index 13535d296195b..2e4c6b6a74fff 100644 --- a/test/mocks/api/mocks.cc +++ b/test/mocks/api/mocks.cc @@ -19,35 +19,14 @@ MockApi::~MockApi() {} Event::DispatcherPtr MockApi::allocateDispatcher() { return Event::DispatcherPtr{allocateDispatcher_(time_system_)}; } - Event::DispatcherPtr MockApi::allocateDispatcher(Buffer::WatermarkFactoryPtr&& watermark_factory) { return Event::DispatcherPtr{allocateDispatcher_(std::move(watermark_factory), time_system_)}; } -MockOsSysCalls::MockOsSysCalls() { num_writes_ = num_open_ = 0; } +MockOsSysCalls::MockOsSysCalls() {} MockOsSysCalls::~MockOsSysCalls() {} -SysCallIntResult MockOsSysCalls::open(const std::string& full_path, int flags, int mode) { - Thread::LockGuard lock(open_mutex_); - - int rc = open_(full_path, flags, mode); - num_open_++; - open_event_.notifyOne(); - - return SysCallIntResult{rc, errno}; -} - -SysCallSizeResult MockOsSysCalls::write(int fd, const void* buffer, size_t num_bytes) { - Thread::LockGuard lock(write_mutex_); - - ssize_t rc = write_(fd, buffer, num_bytes); - num_writes_++; - write_event_.notifyOne(); - - return SysCallSizeResult{rc, errno}; -} - SysCallIntResult MockOsSysCalls::setsockopt(int sockfd, int level, int optname, const void* optval, socklen_t optlen) { ASSERT(optlen == sizeof(int)); diff --git a/test/mocks/api/mocks.h b/test/mocks/api/mocks.h index 8fa207c92cef0..4a1be0b0b0df5 100644 --- a/test/mocks/api/mocks.h +++ b/test/mocks/api/mocks.h @@ -7,7 +7,6 @@ #include "envoy/api/os_sys_calls.h" #include "envoy/event/dispatcher.h" #include "envoy/event/timer.h" -#include "envoy/stats/store.h" #include "common/api/os_sys_calls_impl.h" @@ -46,8 +45,6 @@ class MockOsSysCalls : public OsSysCallsImpl { ~MockOsSysCalls(); // Api::OsSysCalls - SysCallSizeResult write(int fd, const void* buffer, size_t num_bytes) override; - SysCallIntResult open(const std::string& full_path, int flags, int mode) override; SysCallIntResult setsockopt(int sockfd, int level, int optname, const void* optval, socklen_t optlen) override; SysCallIntResult getsockopt(int sockfd, int level, int optname, void* optval, @@ -56,8 +53,6 @@ class MockOsSysCalls : public OsSysCallsImpl { MOCK_METHOD3(bind, SysCallIntResult(int sockfd, const sockaddr* addr, socklen_t addrlen)); MOCK_METHOD3(ioctl, SysCallIntResult(int sockfd, unsigned long int request, void* argp)); MOCK_METHOD1(close, SysCallIntResult(int)); - MOCK_METHOD3(open_, int(const std::string& full_path, int flags, int mode)); - MOCK_METHOD3(write_, ssize_t(int, const void*, size_t)); MOCK_METHOD3(writev, SysCallSizeResult(int, const iovec*, int)); MOCK_METHOD3(readv, SysCallSizeResult(int, const iovec*, int)); MOCK_METHOD4(recv, SysCallSizeResult(int socket, void* buffer, size_t length, int flags)); @@ -74,12 +69,6 @@ class MockOsSysCalls : public OsSysCallsImpl { int(int sockfd, int level, int optname, void* optval, socklen_t* optlen)); MOCK_METHOD3(socket, SysCallIntResult(int domain, int type, int protocol)); - size_t num_writes_; - size_t num_open_; - Thread::MutexBasicLockable write_mutex_; - Thread::MutexBasicLockable open_mutex_; - Thread::CondVar write_event_; - Thread::CondVar open_event_; // Map from (sockfd,level,optname) to boolean socket option. using SockOptKey = std::tuple; std::map boolsockopts_; diff --git a/test/mocks/filesystem/mocks.cc b/test/mocks/filesystem/mocks.cc index 42bc97cda3445..5fe62371c2058 100644 --- a/test/mocks/filesystem/mocks.cc +++ b/test/mocks/filesystem/mocks.cc @@ -1,13 +1,44 @@ #include "test/mocks/filesystem/mocks.h" -#include +#include "common/common/lock_guard.h" namespace Envoy { namespace Filesystem { -MockFile::MockFile() {} +MockFile::MockFile() : num_opens_(0), num_writes_(0), is_open_(false) {} MockFile::~MockFile() {} +Api::SysCallBoolResult MockFile::open() { + Thread::LockGuard lock(open_mutex_); + + const Api::SysCallBoolResult result = open_(); + is_open_ = result.rc_; + num_opens_++; + open_event_.notifyOne(); + + return result; +} + +Api::SysCallSizeResult MockFile::write(absl::string_view buffer) { + Thread::LockGuard lock(write_mutex_); + if (!is_open_) { + return {-1, EBADF}; + } + + const Api::SysCallSizeResult result = write_(buffer); + num_writes_++; + write_event_.notifyOne(); + + return result; +} + +Api::SysCallBoolResult MockFile::close() { + const Api::SysCallBoolResult result = close_(); + is_open_ = !result.rc_; + + return result; +} + MockInstance::MockInstance() {} MockInstance::~MockInstance() {} diff --git a/test/mocks/filesystem/mocks.h b/test/mocks/filesystem/mocks.h index 45e6e60b61d64..821cf2b3c17f8 100644 --- a/test/mocks/filesystem/mocks.h +++ b/test/mocks/filesystem/mocks.h @@ -4,6 +4,7 @@ #include #include "envoy/filesystem/filesystem.h" +#include "envoy/filesystem/watcher.h" #include "common/common/thread.h" @@ -18,9 +19,26 @@ class MockFile : public File { ~MockFile(); // Filesystem::File - MOCK_METHOD1(write, void(absl::string_view data)); - MOCK_METHOD0(reopen, void()); - MOCK_METHOD0(flush, void()); + Api::SysCallBoolResult open() override; + Api::SysCallSizeResult write(absl::string_view buffer) override; + Api::SysCallBoolResult close() override; + bool isOpen() override { return is_open_; }; + MOCK_METHOD0(path, std::string()); + MOCK_METHOD1(errorToString, std::string(int)); + + MOCK_METHOD0(open_, Api::SysCallBoolResult()); + MOCK_METHOD1(write_, Api::SysCallSizeResult(absl::string_view buffer)); + MOCK_METHOD0(close_, Api::SysCallBoolResult()); + + size_t num_opens_; + size_t num_writes_; + Thread::MutexBasicLockable open_mutex_; + Thread::MutexBasicLockable write_mutex_; + Thread::CondVar open_event_; + Thread::CondVar write_event_; + +private: + bool is_open_; }; class MockInstance : public Instance { @@ -29,10 +47,7 @@ class MockInstance : public Instance { ~MockInstance(); // Filesystem::Instance - MOCK_METHOD4(createFile, FileSharedPtr(const std::string&, Event::Dispatcher&, - Thread::BasicLockable&, std::chrono::milliseconds)); - MOCK_METHOD3(createFile, - FileSharedPtr(const std::string&, Event::Dispatcher&, Thread::BasicLockable&)); + MOCK_METHOD1(createFile, FilePtr(const std::string&)); MOCK_METHOD1(fileExists, bool(const std::string&)); MOCK_METHOD1(directoryExists, bool(const std::string&)); MOCK_METHOD1(fileSize, ssize_t(const std::string&)); diff --git a/test/server/config_validation/dispatcher_test.cc b/test/server/config_validation/dispatcher_test.cc index ddba8fdc3e7ca..e1c78c8b1dec3 100644 --- a/test/server/config_validation/dispatcher_test.cc +++ b/test/server/config_validation/dispatcher_test.cc @@ -24,8 +24,7 @@ class ConfigValidation : public testing::TestWithParam(std::chrono::milliseconds(1000), - Thread::threadFactoryForTest(), + validation_ = std::make_unique(Thread::threadFactoryForTest(), stats_store_, test_time_.timeSystem()); dispatcher_ = validation_->allocateDispatcher(); } diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 6157227f7f3a5..b0730e0aabe8a 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -410,35 +410,26 @@ class TestImplProvider { class TestImpl : public TestImplProvider, public Impl { public: - TestImpl(std::chrono::milliseconds file_flush_interval_msec, - Thread::ThreadFactory& thread_factory, Stats::Store& stats_store) - : Impl(file_flush_interval_msec, thread_factory, stats_store, global_time_system_) {} - TestImpl(std::chrono::milliseconds file_flush_interval_msec, - Thread::ThreadFactory& thread_factory, Event::TimeSystem& time_system) - : Impl(file_flush_interval_msec, thread_factory, default_stats_store_, time_system) {} - TestImpl(std::chrono::milliseconds file_flush_interval_msec, - Thread::ThreadFactory& thread_factory) - : Impl(file_flush_interval_msec, thread_factory, default_stats_store_, global_time_system_) {} + TestImpl(Thread::ThreadFactory& thread_factory, Stats::Store& stats_store) + : Impl(thread_factory, stats_store, global_time_system_) {} + TestImpl(Thread::ThreadFactory& thread_factory, Event::TimeSystem& time_system) + : Impl(thread_factory, default_stats_store_, time_system) {} + TestImpl(Thread::ThreadFactory& thread_factory) + : Impl(thread_factory, default_stats_store_, global_time_system_) {} }; -ApiPtr createApiForTest() { - return std::make_unique(std::chrono::milliseconds(1000), - Thread::threadFactoryForTest()); -} +ApiPtr createApiForTest() { return std::make_unique(Thread::threadFactoryForTest()); } ApiPtr createApiForTest(Stats::Store& stat_store) { - return std::make_unique(std::chrono::milliseconds(1000), Thread::threadFactoryForTest(), - stat_store); + return std::make_unique(Thread::threadFactoryForTest(), stat_store); } ApiPtr createApiForTest(Event::TimeSystem& time_system) { - return std::make_unique(std::chrono::milliseconds(1000), Thread::threadFactoryForTest(), - time_system); + return std::make_unique(Thread::threadFactoryForTest(), time_system); } ApiPtr createApiForTest(Stats::Store& stat_store, Event::TimeSystem& time_system) { - return std::make_unique(std::chrono::milliseconds(1000), Thread::threadFactoryForTest(), - stat_store, time_system); + return std::make_unique(Thread::threadFactoryForTest(), stat_store, time_system); } } // namespace Api diff --git a/tools/BUILD b/tools/BUILD index e504a8889e703..ab8452c075a78 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -31,8 +31,8 @@ envoy_cc_binary( deps = [ "//source/common/api:api_lib", "//source/common/common:assert_lib", - "//source/common/stats:isolated_store_lib", "//source/common/protobuf:utility_lib", + "//source/common/stats:isolated_store_lib", "@envoy_api//envoy/config/bootstrap/v2:bootstrap_cc", ] + envoy_cc_platform_dep("//source/exe:platform_impl_lib"), ) @@ -45,8 +45,8 @@ envoy_cc_binary( "//source/common/config:bootstrap_json_lib", "//source/common/json:json_loader_lib", "//source/common/protobuf:utility_lib", - "//source/common/stats:stats_options_lib", "//source/common/stats:isolated_store_lib", + "//source/common/stats:stats_options_lib", "@envoy_api//envoy/config/bootstrap/v2:bootstrap_cc", ] + envoy_cc_platform_dep("//source/exe:platform_impl_lib"), ) diff --git a/tools/bootstrap2pb.cc b/tools/bootstrap2pb.cc index 8b298b3da8c49..6ca6eee145d5e 100644 --- a/tools/bootstrap2pb.cc +++ b/tools/bootstrap2pb.cc @@ -30,8 +30,7 @@ int main(int argc, char** argv) { Envoy::PlatformImpl platform_impl_; Envoy::Stats::IsolatedStoreImpl stats_store; Envoy::Event::RealTimeSystem time_system; // NO_CHECK_FORMAT(real_time) - Envoy::Api::Impl api(std::chrono::milliseconds(1000), platform_impl_.threadFactory(), stats_store, - time_system); + Envoy::Api::Impl api(platform_impl_.threadFactory(), stats_store, time_system); envoy::config::bootstrap::v2::Bootstrap bootstrap; Envoy::MessageUtil::loadFromFile(argv[1], bootstrap, api); diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index b79fb3dead32b..ec33876783605 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -29,6 +29,7 @@ CPU CQ CR CRC +CREAT CRL CRLFs CRT @@ -179,6 +180,7 @@ RBAC RCU RDN RDS +RDWR REQ RFC RHS diff --git a/tools/v1_to_bootstrap.cc b/tools/v1_to_bootstrap.cc index 48f8861552898..76a2cecaf4e8f 100644 --- a/tools/v1_to_bootstrap.cc +++ b/tools/v1_to_bootstrap.cc @@ -30,8 +30,7 @@ int main(int argc, char** argv) { Envoy::PlatformImpl platform_impl_; Envoy::Stats::IsolatedStoreImpl stats_store; Envoy::Event::RealTimeSystem time_system; // NO_CHECK_FORMAT(real_time) - Envoy::Api::Impl api(std::chrono::milliseconds(1000), platform_impl_.threadFactory(), stats_store, - time_system); + Envoy::Api::Impl api(platform_impl_.threadFactory(), stats_store, time_system); envoy::config::bootstrap::v2::Bootstrap bootstrap; auto config_json = Envoy::Json::Factory::loadFromFile(argv[1], api);