Merge pull request #430 from yahoo/balder/do-not-copy-file-on-every-redeploy

Balder/do not copy file on every redeploy

7 files changed, 71 insertions, 19 deletions

diff --git a/filedistribution/src/vespa/filedistribution/distributor/filedownloader.h b/filedistribution/src/vespa/filedistribution/distributor/filedownloader.h
index 36ccd6a332a..9056f437664 100644
--- a/filedistribution/src/vespa/filedistribution/distributor/filedownloader.h
+++ b/filedistribution/src/vespa/filedistribution/distributor/filedownloader.h
@@ -17,6 +17,7 @@
 #include <vespa/filedistribution/common/buffer.h>
 #include <vespa/filedistribution/common/exceptionrethrower.h>
 #include <vespa/filedistribution/common/exception.h>
+#include <vespa/filedistribution/model/filedbmodel.h>
 
 namespace filedistribution {
 
@@ -69,6 +70,7 @@ public:
                    const boost::filesystem::path& dbPath,
                    const boost::shared_ptr<ExceptionRethrower>& exceptionRethrower);
     ~FileDownloader();
+    DirectoryGuard::UP getGuard() { return std::make_unique<DirectoryGuard>(_dbPath); }
 
     void runEventLoop();
     void addTorrent(const std::string& fileReference, const Buffer& buffer);
diff --git a/filedistribution/src/vespa/filedistribution/distributor/filedownloadermanager.cpp b/filedistribution/src/vespa/filedistribution/distributor/filedownloadermanager.cpp
index af66ed86afa..7bc57c57dd1 100644
--- a/filedistribution/src/vespa/filedistribution/distributor/filedownloadermanager.cpp
+++ b/filedistribution/src/vespa/filedistribution/distributor/filedownloadermanager.cpp
@@ -89,8 +89,7 @@ FileDownloaderManager::removePeerStatus(const std::string& fileReference) {
 void
 FileDownloaderManager::StartDownloads::downloadFile(const std::string& fileReference) {
     if (!_parent._fileDownloader->hasTorrent(fileReference)) {
-        Buffer torrent(
-                _parent._fileDistributionModel->getFileDBModel().getFile(fileReference));
+        Buffer torrent(_parent._fileDistributionModel->getFileDBModel().getFile(fileReference));
 
         _parent._fileDistributionModel->addPeer(fileReference);
         _parent._fileDownloader->addTorrent(fileReference, torrent);
@@ -102,10 +101,10 @@ void
 FileDownloaderManager::StartDownloads::operator()() {
     namespace ll = boost::lambda;
 
+    DirectoryGuard::UP guard = _parent._fileDownloader->getGuard();
     LockGuard updateFilesToDownloadGuard(_parent._updateFilesToDownloadMutex);
 
-    std::set<std::string> filesToDownload =
-            _parent._fileDistributionModel->getFilesToDownload();
+    std::set<std::string> filesToDownload = _parent._fileDistributionModel->getFilesToDownload();
     logStartDownload(filesToDownload);
 
     std::for_each(filesToDownload.begin(), filesToDownload.end(),
diff --git a/filedistribution/src/vespa/filedistribution/manager/filedb.cpp b/filedistribution/src/vespa/filedistribution/manager/filedb.cpp
index 10aeb5d8158..a5c440b93a0 100644
--- a/filedistribution/src/vespa/filedistribution/manager/filedb.cpp
+++ b/filedistribution/src/vespa/filedistribution/manager/filedb.cpp
@@ -32,13 +32,15 @@ FileDB::FileDB(fs::path dbPath)
     : _dbPath(dbPath) {}
 
 
-void
-FileDB::add(fs::path original, const std::string &name) {
-    fs::path targetPathTemp = _dbPath / (name + ".tmp");
+bool
+FileDB::add(const DirectoryGuard & directoryGuard, fs::path original, const std::string &name) {
+    (void) directoryGuard;
+    fs::path finalPath = _dbPath / name;
     fs::path targetPath = _dbPath / (name + ".new");
-    if (fs::exists(targetPath)) {
-        return;
+    if (fs::exists(finalPath) || fs::exists(targetPath)) {
+        return false;
     }
+    fs::path targetPathTemp = _dbPath / (name + ".tmp");
 
     if (fs::exists(targetPathTemp)) {
         fs::remove_all(targetPathTemp);
@@ -53,6 +55,7 @@ FileDB::add(fs::path original, const std::string &name) {
 
     assert(!fs::exists(targetPath));
     fs::rename(targetPathTemp, targetPath);
+    return true;
 }
 
 }
diff --git a/filedistribution/src/vespa/filedistribution/manager/filedb.h b/filedistribution/src/vespa/filedistribution/manager/filedb.h
index d44436f9e7f..d5031fff8f1 100644
--- a/filedistribution/src/vespa/filedistribution/manager/filedb.h
+++ b/filedistribution/src/vespa/filedistribution/manager/filedb.h
@@ -3,15 +3,23 @@
 
 #include <string>
 #include <boost/filesystem/path.hpp>
+#include <vespa/filedistribution/model/filedbmodel.h>
 
 namespace filedistribution {
 
 class FileDB {
     boost::filesystem::path _dbPath;
-    int _fd;
 public:
     FileDB(boost::filesystem::path dbPath);
-    void add(boost::filesystem::path original, const std::string& name);
+    DirectoryGuard::UP getGuard() { return std::make_unique<DirectoryGuard>(_dbPath); }
+    /**
+     *
+     * @param directoryGuard The guard you need to hold in order to prevent someone fidling with your directory.
+     * @param original The file top copy
+     * @param name The name the file shall have.
+     * @return true if it was added, false if it was already present.
+     */
+    bool add(const DirectoryGuard & directoryGuard, boost::filesystem::path original, const std::string& name);
 };
 
 } //namespace filedistribution
diff --git a/filedistribution/src/vespa/filedistribution/manager/filedistributionmanager.cpp b/filedistribution/src/vespa/filedistribution/manager/filedistributionmanager.cpp
index fde4302eee8..e9147e16cb2 100644
--- a/filedistribution/src/vespa/filedistribution/manager/filedistributionmanager.cpp
+++ b/filedistribution/src/vespa/filedistribution/manager/filedistributionmanager.cpp
@@ -130,12 +130,18 @@ Java_com_yahoo_vespa_filedistribution_FileDistributionManager_addFileImpl(
         std::string fileReference = createTorrent.fileReference();
         NativeFileDistributionManager& manager = *nativeFileDistributionManagerField.get(self, env);
 
-        manager._fileDB->add(completePath._value, fileReference);
+        DirectoryGuard::UP guard = manager._fileDB->getGuard();// This prevents the filedistributor from working in an inconsistent state.
+        bool freshlyAdded = manager._fileDB->add(*guard, completePath._value, fileReference);
 
         FileDBModel& model = *manager._fileDBModel;
-        if (! model.hasFile(fileReference) ) {
+        bool hasRegisteredFile = model.hasFile(fileReference);
+        if (! hasRegisteredFile ) {
             model.addFile(fileReference, createTorrent.bencode());
         }
+        if (freshlyAdded == hasRegisteredFile) {
+            std::cerr << "freshlyAdded(" << freshlyAdded << ") == hasRegisteredFile(" << hasRegisteredFile
+                      << "), which is very odd. File is '" << fileReference << std::endl;
+        }
 
         //contains string with the characters 0-9 a-f
         return env->NewStringUTF(fileReference.c_str());
diff --git a/filedistribution/src/vespa/filedistribution/model/filedbmodel.h b/filedistribution/src/vespa/filedistribution/model/filedbmodel.h
index 1c8ad181306..ea564668e45 100644
--- a/filedistribution/src/vespa/filedistribution/model/filedbmodel.h
+++ b/filedistribution/src/vespa/filedistribution/model/filedbmodel.h
@@ -8,6 +8,15 @@
 
 namespace filedistribution {
 
+class DirectoryGuard {
+public:
+    typedef std::unique_ptr<DirectoryGuard> UP;
+    DirectoryGuard(boost::filesystem::path path);
+    ~DirectoryGuard();
+private:
+    int _fd;
+};
+
 struct InvalidProgressException : public Exception {
     const char* what() const throw() {
         return "Invalid progress information reported by one of the filedistributors";
diff --git a/filedistribution/src/vespa/filedistribution/model/zkfiledbmodel.cpp b/filedistribution/src/vespa/filedistribution/model/zkfiledbmodel.cpp
index 3483a4eb359..f9a4c777b30 100644
--- a/filedistribution/src/vespa/filedistribution/model/zkfiledbmodel.cpp
+++ b/filedistribution/src/vespa/filedistribution/model/zkfiledbmodel.cpp
@@ -12,20 +12,21 @@
 #include "zkfiledbmodel.h"
 #include "deployedfilestodownload.h"
 #include <vespa/filedistribution/common/logfwd.h>
+#include <sys/file.h>
 
 namespace fs = boost::filesystem;
 
-using filedistribution::ZKFileDBModel;
+namespace filedistribution {
 
 namespace {
 
 fs::path
 createPath(const std::string& fileReference) {
-    return filedistribution::ZKFileDBModel::_fileDBPath / fileReference;
+    return ZKFileDBModel::_fileDBPath / fileReference;
 }
 
 void
-createNode(const fs::path & path, filedistribution::ZKFacade& zk) {
+createNode(const fs::path & path, ZKFacade& zk) {
     if (!zk.hasNode(path))
         zk.setData(path, "", 0);
 }
@@ -38,7 +39,7 @@ isEntryForHost(const std::string& host, const std::string& peerEntry) {
 }
 
 std::vector<std::string>
-getSortedChildren(filedistribution::ZKFacade& zk, const ZKFileDBModel::Path& path) {
+getSortedChildren(ZKFacade& zk, const ZKFileDBModel::Path& path) {
     std::vector<std::string> children = zk.getChildren(path);
     std::sort(children.begin(), children.end());
     return children;
@@ -60,7 +61,7 @@ ZKFileDBModel::addFile(const std::string& fileReference, const Buffer& buffer) {
     return _zk->setData(createPath(fileReference), buffer);
 }
 
-filedistribution::Move<filedistribution::Buffer>
+Move<Buffer>
 ZKFileDBModel::getFile(const std::string& fileReference) {
     try {
         return _zk->getData(createPath(fileReference));
@@ -295,4 +296,28 @@ ZKFileDBModel::getProgress(const std::string& fileReference,
     return progress;
 }
 
-filedistribution::FileDBModel::~FileDBModel() {}
+FileDBModel::~FileDBModel() {}
+
+DirectoryGuard::DirectoryGuard(boost::filesystem::path path) :
+    _fd(-1)
+{
+    _fd = open(path.c_str(), O_RDONLY);
+    assert(_fd != -1);
+    int retval = flock(_fd, LOCK_EX);
+    while ((retval != 0) && (errno == EINTR)) {
+        std::cout << "Got interupted while flock'ing ' " << path << std::endl;
+        retval = flock(_fd, LOCK_EX);
+    }
+    assert(retval == 0);
+}
+
+DirectoryGuard::~DirectoryGuard() {
+    if (_fd != -1) {
+        int retval = flock(_fd, LOCK_UN);
+        assert(retval == 0);
+        retval = close(_fd);
+        assert(retval ==0);
+    }
+}
+
+}