-
Notifications
You must be signed in to change notification settings - Fork 581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sync dependencies to Redis #10290
base: master
Are you sure you want to change the base?
Sync dependencies to Redis #10290
Conversation
This does not work in this state! Trying to refresh Dependency if a Host or Service being member of this Dependency has a state change.
Otherwise, it would require too much code changes to properly handle redundancy group runtime modification in Icinga DB for no real benefit.
95a27d3
to
17ba7c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm somewhat confused by the DependencyGroup
class as it doesn't really map to the mental model I had from our discussions on that topic.
So in my understanding, a DependencyGroup
would represent a set a set of checkables that are used as parents in dependency config objects combined with the attributes from that dependency object that affect how the availability of that dependency is determined, i.e. ignore_soft_states
, period
, and states
. For dependencies without a redundancy group, that information is all that's needed to determine if all dependency objects that share the parent and these attribute mark all their children as unreachable. With a redundancy group, you have to look at all the parents from the redundancy group with the three aforementioned additional attributes. So that would be how to determine what creates a DependencyGroup
object for redundancy groups.
For dependencies without a redundancy group, this grouping provides no value in itself, the dependency objects can be considered individually. There are two reasons why we might instantiate such trivial groups explicitly nonetheless: for one, it may allow simpler code by being able to treat both cases consistently, but more importantly, there was a request from Johannes that if two children depend on the same parent in such a way that the state of these dependencies is always the same (i.e. the three aforementioned attributes are identical), then the different graph edges should refer to the same shared state. These groups may be used for this deduplication as well.
Consider the following example (P = parent checkable, RG = redundancy group as represented in the generated graph, C = child checkable):
graph BT;
p1((P1));
p2((P2));
p3((P3));
c1((C1));
c2((C2));
c3((C3));
c4((C4));
c5((C5));
rg1(RG1);
c1-->rg1;
c2-->rg1;
c3-->rg1;
rg1-->p1;
rg1-->p2;
c4-->p3;
c5-->p3;
Here I'd expect the creation of the following two DependencyGroups
(...
refers to the three magic attributes attached to the parent in the corresponding dependency objects):
{(P1, ...), (P2, ...)}
: This basically represents RG1{(P3, ...)}
: This is a if there was an imaginary second redundancy with only one parent, P3.
m_Rcon->FireAndForgetQuery({"XADD", "icinga:schema", "MAXLEN", "1", "*", "version", "5"}, Prio::Heartbeat); | ||
m_Rcon->FireAndForgetQuery({"XADD", "icinga:schema", "MAXLEN", "1", "*", "version", "6"}, Prio::Heartbeat); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the incompatible change that makes this necessary?
@@ -323,3 +389,474 @@ void Dependency::SetChild(intrusive_ptr<Checkable> child) | |||
{ | |||
m_Child = child; | |||
} | |||
|
|||
// Is the default (dummy) dependency group name used to group non-redundant dependencies. | |||
static String l_DefaultDependencyGroup(Utility::NewUniqueID()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the problem with using the empty string? That's not a valid redundancy group name, so there should be no conflicts with that either.
struct Hash { | ||
/** | ||
* Calculates the hash value of a dependency group used by DependencyGroup::RegistryType. | ||
* | ||
* @param dependencyGroup The dependency group to calculate the hash value for. | ||
* | ||
* @return Returns the hash value of the dependency group. | ||
*/ | ||
size_t operator()(const DependencyGroup::Ptr& dependencyGroup) const | ||
{ | ||
return std::hash<String>{}(dependencyGroup->GetCompositeKey()); | ||
} | ||
}; | ||
|
||
struct Equal { | ||
/** | ||
* Checks two dependency groups for equality. | ||
* | ||
* The equality of two dependency groups is determined by the equality of their composite keys. | ||
* That composite key consists of a tuple of the parent name, the time period name (empty if not configured), | ||
* state filter, and the ignore soft states flag of the member. | ||
* | ||
* @param lhs The first dependency group to compare. | ||
* @param rhs The second dependency group to compare. | ||
* | ||
* @return Returns true if the composite keys of the two dependency groups are equal. | ||
*/ | ||
bool operator()(const DependencyGroup::Ptr& lhs, const DependencyGroup::Ptr& rhs) const | ||
{ | ||
return lhs->GetCompositeKey() == rhs->GetCompositeKey(); | ||
} | ||
}; | ||
|
||
using RegistryType = boost::multi_index_container< | ||
DependencyGroup*, // The type of the elements stored in the container. | ||
boost::multi_index::indexed_by< | ||
// The first index is a unique index based on the identity of the dependency group. | ||
// The identity of the dependency group is determined by the provided Hash and Equal functors. | ||
boost::multi_index::hashed_unique<boost::multi_index::identity<DependencyGroup*>, Hash, Equal>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be possible to create an index on GetCompositeKey()
directly, see CheckableNextCheckExtractor
. That should make defining struct Hash
and struct Equal
unnecessary.
/** | ||
* @ingroup icinga | ||
*/ | ||
class DependencyGroup final : public SharedObject | ||
{ | ||
public: | ||
DECLARE_PTR_TYPEDEFS(DependencyGroup); | ||
|
||
/** | ||
* Defines the key type of each dependency group members. | ||
* | ||
* For dependency groups **with** an explicitly configured redundancy group, that tuple consists of the dependency | ||
* parent name, the dependency time period name (empty if not configured), the state filter, and the | ||
* ignore soft states flag. | ||
* | ||
* For the non-redundant group (just a bunch of dependencies without a redundancy group) of a given Checkable, | ||
* the tuple consists of the dependency group name (which is a randomly generated unique UUID), the child | ||
* Checkable name, the state filter (is always 0), and the ignore soft states flag (is always false). | ||
*/ | ||
using MemberTuple = std::tuple<String, String, int, bool>; | ||
using MemberValueType = std::unordered_multimap<const Checkable*, Dependency*>; | ||
using MembersMap = std::map<MemberTuple, MemberValueType>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That class needs more documentation/explanation what a group and its members are/represent.
/** | ||
* Defines the key type of each dependency group members. | ||
* | ||
* For dependency groups **with** an explicitly configured redundancy group, that tuple consists of the dependency | ||
* parent name, the dependency time period name (empty if not configured), the state filter, and the | ||
* ignore soft states flag. | ||
* | ||
* For the non-redundant group (just a bunch of dependencies without a redundancy group) of a given Checkable, | ||
* the tuple consists of the dependency group name (which is a randomly generated unique UUID), the child | ||
* Checkable name, the state filter (is always 0), and the ignore soft states flag (is always false). | ||
*/ | ||
using MemberTuple = std::tuple<String, String, int, bool>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't immediately see why there is such a difference in which values are used here. Conceptually, a dependency outside of a redundancy group should behave the same as if it was alone in its very own redundancy group, the time period should either be relevant for both types or for neither.
Also, is the String
type only used to have references to different object types in the same member of that tuple? So why can't it be Checkable*
instead of the name for example? If there's really a difference needed between redundancy groups and individual dependencies, std::variant
could also be an option.
if (!m_AssertNoCyclesForIndividualDeps) { | ||
// Yes, this is rather a hack and introduces an inconsistent behaviour between file-based and | ||
// runtime-created dependencies. However, I don't see any other possibility for handling this | ||
// in a satisfiable manner. The reason for this change is simple, while Icinga DB doesn't need | ||
// to know about every un/registered dependencies on startup, we still need to track and notify | ||
// it about each and every dependency change at runtime to keep the database consistent. | ||
DependencyGroup::Register(this); | ||
m_Parent->AddReverseDependency(this); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly is the inconsistency and how will it (not) show in practice?
SSIA 🤪!
Just kidding! This PR synchronises all the required points from Icinga/icingadb#347 (comment) to Redis. However, I'm not going to explain every implementation detail of the PR here, but it can be roughly understood as follows.
host/service.affected_children
andstate.affects_children
attributes to Redis as described in Track effect of an object on dependent children #10158.no_user_modify
flag to theredundancy_group
attribute in thedependency.ti
file to prevent any runtime alteration of its value, as there is now too much logic and functionality depending on this value and changing it at runtime would have a disastrous end.DependencyGroup
to easily group and manage identical dependencies of any checkable in one place. Yes, this is also used to group non-redundant dependencies, but such a group is entirely unique for each checkable and is never referenced by other checkables.DependencyGroup
at any given time as described in Let redundancy groups not just fail #10190.failedDependency
parameter of theCheckable::IsReachable()
method. It is obsolete because none of the callers make use of it, it just adds unnecessary complexity to the method for no reason.Checkable::IsReachable()
method and utilises theDependencyGroup::GetState()
method introduced above.activation_priority
of theDependency
object is set to-10
(just like for downtime objects). This way, the dependency objects will always get activated before their child/parent Checkables.fixes #10158
fixes #10190
fixes #10227
fixes #10014