From 87d4575af8b53977cbc2b54a6774de61eda8ba30 Mon Sep 17 00:00:00 2001 From: Diana Flach Date: Tue, 9 Jul 2019 15:36:20 +0200 Subject: [PATCH 1/4] Cluster Sync: Ensure that files are synced everytime --- lib/remote/apilistener-filesync.cpp | 98 ++++++++++++++++++++--------- 1 file changed, 70 insertions(+), 28 deletions(-) diff --git a/lib/remote/apilistener-filesync.cpp b/lib/remote/apilistener-filesync.cpp index 54a1bdf04..d1048ff6d 100644 --- a/lib/remote/apilistener-filesync.cpp +++ b/lib/remote/apilistener-filesync.cpp @@ -441,10 +441,6 @@ Value ApiListener::ConfigUpdateHandler(const MessageOrigin::Ptr& origin, const D * */ relativePaths.push_back(zoneName + "/" + kv.first); - // Ignore same config content. This is an expensive comparison. - if (productionConfig->Get(kv.first) == kv.second) - continue; - String path = stageConfigZoneDir + "/" + kv.first; if (Utility::Match("*.conf", path)) { @@ -667,35 +663,81 @@ bool ApiListener::CheckConfigChange(const ConfigDirInformation& oldConfig, const << "' vs. new (" << newChecksums->GetLength() << "): '" << JsonEncode(newChecksums) << "'."; - // Don't check for different length, this may be influenced from internal files - ObjectLock olock(oldChecksums); - for (const Dictionary::Pair& kv : oldChecksums) { - String path = kv.first; - String oldChecksum = kv.second; + /* Since internal files are synced here too, we can not depend on length. + * So we need to go through both checksum sets to cover the cases"everything is new" and "everything was deleted". + */ + { + ObjectLock olock(oldChecksums); + for (const Dictionary::Pair& kv : oldChecksums) { + String path = kv.first; + String oldChecksum = kv.second; - // TODO: Figure out if config changes only apply to '.conf'. Leaving this open for other config files. - //if (!Utility::Match("*.conf", path)) - // continue; + // TODO: Figure out if config changes only apply to '.conf'. Leaving this open for other config files. + //if (!Utility::Match("*.conf", path)) + // continue; - /* Ignore internal files, especially .timestamp and .checksums. - * - * If we don't, this results in "always change" restart loops. - */ - if (Utility::Match("/.*", path)) - continue; + /* Ignore internal files, especially .timestamp and .checksums. + * + * If we don't, this results in "always change" restart loops. + */ + if (Utility::Match("/.*", path)) { + Log(LogDebug, "ApiListener") + << "Ignoring internal file '" << path << "'"; + continue; + } - Log(LogDebug, "ApiListener") - << "Checking " << path << " for checksum: " << oldChecksum; - - // Check whether our key exists in the new checksums, and they have an equal value. - String newChecksum = newChecksums->Get(path); - - if (newChecksums->Get(path) != kv.second) { Log(LogDebug, "ApiListener") - << "Path '" << path << "' doesn't match old checksum '" - << newChecksum << "' with new checksum '" << oldChecksum << "'."; - return true; + << "Checking " << path << " for old checksum: " << oldChecksum; + + // Check if key exists first for more verbose logging. + // TODO: Don't do this later on. + if (!newChecksums->Contains(path)) { + Log(LogDebug, "ApiListener") + << "File '" << path << "' was deleted by remote."; + return true; + } + + String newChecksum = newChecksums->Get(path); + if (newChecksum != kv.second) { + Log(LogDebug, "ApiListener") + << "Path '" << path << "' doesn't match old checksum '" + << oldChecksum << "' with new checksum '" << newChecksum << "'."; + return true; + } + } + } + + { + ObjectLock olock(newChecksums); + for (const Dictionary::Pair& kv : newChecksums) { + String path = kv.first; + String newChecksum = kv.second; + + // TODO: Figure out if config changes only apply to '.conf'. Leaving this open for other config files. + //if (!Utility::Match("*.conf", path)) + // continue; + + /* Ignore internal files, especially .timestamp and .checksums. + * + * If we don't, this results in "always change" restart loops. + */ + if (Utility::Match("/.*", path)) { + Log(LogDebug, "ApiListener") + << "Ignoring internal file '" << path << "'"; + continue; + } + + Log(LogDebug, "ApiListener") + << "Checking " << path << " for new checksum: " << newChecksum; + + // Here we only need to check if the checksum exists, checksums in both sets have already been compared + // TODO: Don't do this later on. + if (!oldChecksums->Contains(path)) { + Log(LogDebug, "ApiListener") + << "File '" << path << "' was added by remote."; + return true; + } } } From b00e1d0c6777e509a397931cb4a6646fae0181ba Mon Sep 17 00:00:00 2001 From: Michael Friedrich Date: Wed, 10 Jul 2019 12:34:40 +0200 Subject: [PATCH 2/4] Config sync: Count the updates and log them ``` [2019-07-10 12:34:27 +0200] information/ApiListener: Received configuration updates (2) from endpoint 'master1' are equal to production, not triggering reload. ``` --- lib/remote/apilistener-filesync.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/remote/apilistener-filesync.cpp b/lib/remote/apilistener-filesync.cpp index d1048ff6d..5d1412bc2 100644 --- a/lib/remote/apilistener-filesync.cpp +++ b/lib/remote/apilistener-filesync.cpp @@ -301,6 +301,8 @@ Value ApiListener::ConfigUpdateHandler(const MessageOrigin::Ptr& origin, const D Utility::MkDirP(apiZonesStageDir, 0700); // Analyse and process the update. + size_t count = 0; + ObjectLock olock(updateV1); for (const Dictionary::Pair& kv : updateV1) { @@ -480,6 +482,8 @@ Value ApiListener::ConfigUpdateHandler(const MessageOrigin::Ptr& origin, const D } } } + + count++; } /* @@ -493,13 +497,13 @@ Value ApiListener::ConfigUpdateHandler(const MessageOrigin::Ptr& origin, const D */ if (configChange) { Log(LogInformation, "ApiListener") - << "Received configuration from endpoint '" << fromEndpointName - << "' is different to production, triggering validation and reload."; + << "Received configuration updates (" << count << ") from endpoint '" << fromEndpointName + << "' are different to production, triggering validation and reload."; AsyncTryActivateZonesStage(relativePaths); } else { Log(LogInformation, "ApiListener") - << "Received configuration from endpoint '" << fromEndpointName - << "' is equal to production, not triggering reload."; + << "Received configuration updates (" << count << ") from endpoint '" << fromEndpointName + << "' are equal to production, not triggering reload."; } return Empty; From 5fbc052aba77635c184c1ba097a88b765546b058 Mon Sep 17 00:00:00 2001 From: Diana Flach Date: Thu, 11 Jul 2019 15:11:16 +0200 Subject: [PATCH 3/4] Cluster Sync: Improve log messages --- lib/remote/apilistener-filesync.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/remote/apilistener-filesync.cpp b/lib/remote/apilistener-filesync.cpp index 5d1412bc2..1a3069aa2 100644 --- a/lib/remote/apilistener-filesync.cpp +++ b/lib/remote/apilistener-filesync.cpp @@ -687,12 +687,12 @@ bool ApiListener::CheckConfigChange(const ConfigDirInformation& oldConfig, const */ if (Utility::Match("/.*", path)) { Log(LogDebug, "ApiListener") - << "Ignoring internal file '" << path << "'"; + << "Ignoring old internal file '" << path << "'."; continue; } Log(LogDebug, "ApiListener") - << "Checking " << path << " for old checksum: " << oldChecksum; + << "Checking " << path << " for old checksum: " << oldChecksum << "."; // Check if key exists first for more verbose logging. // TODO: Don't do this later on. @@ -728,15 +728,14 @@ bool ApiListener::CheckConfigChange(const ConfigDirInformation& oldConfig, const */ if (Utility::Match("/.*", path)) { Log(LogDebug, "ApiListener") - << "Ignoring internal file '" << path << "'"; + << "Ignoring new internal file '" << path << "'."; continue; } Log(LogDebug, "ApiListener") - << "Checking " << path << " for new checksum: " << newChecksum; + << "Checking " << path << " for new checksum: " << newChecksum << "."; // Here we only need to check if the checksum exists, checksums in both sets have already been compared - // TODO: Don't do this later on. if (!oldChecksums->Contains(path)) { Log(LogDebug, "ApiListener") << "File '" << path << "' was added by remote."; From eff6e7662ca031346fbc9d44787ec1189cb9dc4b Mon Sep 17 00:00:00 2001 From: Michael Friedrich Date: Mon, 15 Jul 2019 09:48:38 +0200 Subject: [PATCH 4/4] Fix style and comments --- lib/remote/apilistener-filesync.cpp | 31 +++++++++++++---------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/lib/remote/apilistener-filesync.cpp b/lib/remote/apilistener-filesync.cpp index 1a3069aa2..79fa55b55 100644 --- a/lib/remote/apilistener-filesync.cpp +++ b/lib/remote/apilistener-filesync.cpp @@ -667,7 +667,6 @@ bool ApiListener::CheckConfigChange(const ConfigDirInformation& oldConfig, const << "' vs. new (" << newChecksums->GetLength() << "): '" << JsonEncode(newChecksums) << "'."; - /* Since internal files are synced here too, we can not depend on length. * So we need to go through both checksum sets to cover the cases"everything is new" and "everything was deleted". */ @@ -677,36 +676,36 @@ bool ApiListener::CheckConfigChange(const ConfigDirInformation& oldConfig, const String path = kv.first; String oldChecksum = kv.second; - // TODO: Figure out if config changes only apply to '.conf'. Leaving this open for other config files. - //if (!Utility::Match("*.conf", path)) - // continue; - /* Ignore internal files, especially .timestamp and .checksums. * * If we don't, this results in "always change" restart loops. */ if (Utility::Match("/.*", path)) { Log(LogDebug, "ApiListener") - << "Ignoring old internal file '" << path << "'."; + << "Ignoring old internal file '" << path << "'."; + continue; } Log(LogDebug, "ApiListener") - << "Checking " << path << " for old checksum: " << oldChecksum << "."; + << "Checking " << path << " for old checksum: " << oldChecksum << "."; // Check if key exists first for more verbose logging. - // TODO: Don't do this later on. + // Note: Don't do this later on. if (!newChecksums->Contains(path)) { Log(LogDebug, "ApiListener") - << "File '" << path << "' was deleted by remote."; + << "File '" << path << "' was deleted by remote."; + return true; } String newChecksum = newChecksums->Get(path); + if (newChecksum != kv.second) { Log(LogDebug, "ApiListener") - << "Path '" << path << "' doesn't match old checksum '" - << oldChecksum << "' with new checksum '" << newChecksum << "'."; + << "Path '" << path << "' doesn't match old checksum '" + << oldChecksum << "' with new checksum '" << newChecksum << "'."; + return true; } } @@ -718,27 +717,25 @@ bool ApiListener::CheckConfigChange(const ConfigDirInformation& oldConfig, const String path = kv.first; String newChecksum = kv.second; - // TODO: Figure out if config changes only apply to '.conf'. Leaving this open for other config files. - //if (!Utility::Match("*.conf", path)) - // continue; - /* Ignore internal files, especially .timestamp and .checksums. * * If we don't, this results in "always change" restart loops. */ if (Utility::Match("/.*", path)) { Log(LogDebug, "ApiListener") - << "Ignoring new internal file '" << path << "'."; + << "Ignoring new internal file '" << path << "'."; + continue; } Log(LogDebug, "ApiListener") << "Checking " << path << " for new checksum: " << newChecksum << "."; - // Here we only need to check if the checksum exists, checksums in both sets have already been compared + // Check if the checksum exists, checksums in both sets have already been compared if (!oldChecksums->Contains(path)) { Log(LogDebug, "ApiListener") << "File '" << path << "' was added by remote."; + return true; } }