From cf94fd32b19773aad738f536c8da91a4c4e69279 Mon Sep 17 00:00:00 2001 From: Igor Peshansky Date: Thu, 12 Jul 2018 12:13:18 -0400 Subject: [PATCH 1/4] Remove uninitialized (nullptr) entry from the owners cache on exception. --- src/kubernetes.cc | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/kubernetes.cc b/src/kubernetes.cc index 5433dd30..e956e850 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -943,21 +943,26 @@ json::value KubernetesReader::GetOwner( LOG(DEBUG) << "KindPath returned {" << path_component.first << ", " << path_component.second << "}"; #endif - owner = std::move( - QueryMaster(path_component.first + "/namespaces/" + ns + "/" + - path_component.second + "/" + name)); - // Sanity check: because we are looking up by name, the object we get - // back might have a different uid. - const json::Object* owner_obj = owner->As(); - const json::Object* metadata = owner_obj->Get("metadata"); - const std::string owner_uid = metadata->Get("uid"); - if (owner_uid != uid) { - LOG(WARNING) << "Owner " << kind << "'" << name << "' (id " << uid - << ") disappeared before we could query it. Found id " - << owner_uid << " instead."; - owner.reset(); - throw QueryException("Owner " + kind + " " + name + " (id " + uid + - ") disappeared"); + try { + owner = std::move( + QueryMaster(path_component.first + "/namespaces/" + ns + "/" + + path_component.second + "/" + name)); + // Sanity check: because we are looking up by name, the object we get + // back might have a different uid. + const json::Object* owner_obj = owner->As(); + const json::Object* metadata = owner_obj->Get("metadata"); + const std::string owner_uid = metadata->Get("uid"); + if (owner_uid != uid) { + LOG(WARNING) << "Owner " << kind << "'" << name << "' (id " << uid + << ") disappeared before we could query it. Found id " + << owner_uid << " instead."; + owner.reset(); + throw QueryException("Owner " + kind + " " + name + " (id " + uid + + ") disappeared"); + } + } catch (const QueryException& e) { + owners_.erase(encoded_ref); // Remove the nullptr entry from owners_. + throw; } } return owner->Clone(); From ff5b64cfd836cdd4dd38da225d9a5cb2783826de Mon Sep 17 00:00:00 2001 From: Igor Peshansky Date: Fri, 13 Jul 2018 18:11:48 -0400 Subject: [PATCH 2/4] Don't update owners cache until the value is available. --- src/kubernetes.cc | 45 +++++++++++++++++++-------------------------- 1 file changed, 19 insertions(+), 26 deletions(-) diff --git a/src/kubernetes.cc b/src/kubernetes.cc index e956e850..a48b7db5 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -933,39 +933,32 @@ json::value KubernetesReader::GetOwner( std::vector{api_version, kind, uid}, "/"); std::lock_guard lock(mutex_); - auto found = owners_.emplace(std::piecewise_construct, - std::forward_as_tuple(encoded_ref), - std::forward_as_tuple()); - json::value& owner = found.first->second; - if (found.second) { // Not found, inserted new. + auto found = owners_.find(encoded_ref); + if (found == owners_.end()) { // Not found, add new. const auto path_component = KindPath(api_version, kind); #ifdef VERBOSE LOG(DEBUG) << "KindPath returned {" << path_component.first << ", " << path_component.second << "}"; #endif - try { - owner = std::move( - QueryMaster(path_component.first + "/namespaces/" + ns + "/" + - path_component.second + "/" + name)); - // Sanity check: because we are looking up by name, the object we get - // back might have a different uid. - const json::Object* owner_obj = owner->As(); - const json::Object* metadata = owner_obj->Get("metadata"); - const std::string owner_uid = metadata->Get("uid"); - if (owner_uid != uid) { - LOG(WARNING) << "Owner " << kind << "'" << name << "' (id " << uid - << ") disappeared before we could query it. Found id " - << owner_uid << " instead."; - owner.reset(); - throw QueryException("Owner " + kind + " " + name + " (id " + uid + - ") disappeared"); - } - } catch (const QueryException& e) { - owners_.erase(encoded_ref); // Remove the nullptr entry from owners_. - throw; + json::value owner = + QueryMaster(path_component.first + "/namespaces/" + ns + "/" + + path_component.second + "/" + name); + // Sanity check: because we are looking up by name, the object we get + // back might have a different uid. + const json::Object* owner_obj = owner->As(); + const json::Object* metadata = owner_obj->Get("metadata"); + const std::string owner_uid = metadata->Get("uid"); + if (owner_uid != uid) { + LOG(WARNING) << "Owner " << kind << "'" << name << "' (id " << uid + << ") disappeared before we could query it. Found id " + << owner_uid << " instead."; + throw QueryException("Owner " + kind + " " + name + " (id " + uid + + ") disappeared"); } + owners_.emplace(encoded_ref, owner->Clone()); + return std::move(owner); } - return owner->Clone(); + return found->second->Clone(); } json::value KubernetesReader::FindTopLevelController( From 02419ff03fba5ac4e22e194f72c99c1f61800d14 Mon Sep 17 00:00:00 2001 From: Igor Peshansky Date: Tue, 17 Jul 2018 13:00:00 -0400 Subject: [PATCH 3/4] Go back to single return; return a clone of the newly inserted entry. --- src/kubernetes.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/kubernetes.cc b/src/kubernetes.cc index a48b7db5..f023fc3a 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -955,8 +955,8 @@ json::value KubernetesReader::GetOwner( throw QueryException("Owner " + kind + " " + name + " (id " + uid + ") disappeared"); } - owners_.emplace(encoded_ref, owner->Clone()); - return std::move(owner); + auto inserted = owners_.emplace(encoded_ref, std::move(owner)); + found = inserted.first; } return found->second->Clone(); } From 26edef796356b1af7da525e907f3b23dfada8937 Mon Sep 17 00:00:00 2001 From: Igor Peshansky Date: Tue, 17 Jul 2018 13:00:38 -0400 Subject: [PATCH 4/4] Rename `found` to `owner_it`. --- src/kubernetes.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/kubernetes.cc b/src/kubernetes.cc index f023fc3a..e634e871 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -933,8 +933,8 @@ json::value KubernetesReader::GetOwner( std::vector{api_version, kind, uid}, "/"); std::lock_guard lock(mutex_); - auto found = owners_.find(encoded_ref); - if (found == owners_.end()) { // Not found, add new. + auto owner_it = owners_.find(encoded_ref); + if (owner_it == owners_.end()) { // Not found, add new. const auto path_component = KindPath(api_version, kind); #ifdef VERBOSE LOG(DEBUG) << "KindPath returned {" << path_component.first << ", " @@ -956,9 +956,9 @@ json::value KubernetesReader::GetOwner( ") disappeared"); } auto inserted = owners_.emplace(encoded_ref, std::move(owner)); - found = inserted.first; + owner_it = inserted.first; } - return found->second->Clone(); + return owner_it->second->Clone(); } json::value KubernetesReader::FindTopLevelController(