Fix broken runtime config sync

[yhabteab]

Basically the same as in #9980, but this PR additionally prohibits concurrent API requests targeting the same object. Consequently, no API request is being processed for an object whilst processing a cluster "config update/delete" event for that very same object and vice versa.

Tests

Master 1

object Endpoint "master1" {
}

object Endpoint "master2" {
	host = "IP-Address"
	log_duration = 30m
}

object Zone "master" {
	endpoints = [ "master1", "master2" ]
}

object Endpoint "satellite1" {
	log_duration = 30m
}

object Endpoint "satellite2" {
	log_duration = 30m
}

object Zone "satellite" {
	endpoints = [ "satellite1", "satellite2" ]
	parent = "master"
}

object Zone "global-templates" {
	global = true
}

Master 2

object Endpoint "master1" {
//	host = "IP-Address"
//	port = "5665"
	log_duration = 30m
}

object Endpoint "master2" { }

object Zone "master" {
	endpoints = [ "master1", "master2" ]
}

object Endpoint "satellite1" {
	log_duration = 30m
}

object Endpoint "satellite2" {
	log_duration = 30m
}

object Zone "satellite" {
	endpoints = [ "satellite1", "satellite2" ]
	parent = "master"
}

object Zone "global-templates" {
	global = true
}

Satellite

object Endpoint "master1" {
	host = "IP"
	port = "5665"
}

object Endpoint "master2" {
	host = "IP"
	port = "5665"
}

object Zone "master" {
	endpoints = [ "master1", "master2" ]
}

object Endpoint "satellite1" {
}

object Endpoint "satellite2" {
	log_duration = 30m
}

object Zone "satellite" {
	endpoints = [ "satellite1", "satellite2" ]
	parent = "master"
}

object Zone "global-templates" {
	global = true
}

Start both masters and create some objects via the API, wait until the two masters have synchronised their configuration and then start the satellite.

Tests for #10012

Start two Icinga endpoints that are connected to each other and must accept config updates from each other (accept_config = true).

for j in {1..100} ; do
    curl -k -s -S -i -u root:icinga -H 'Accept: application/json' \
    -X PUT "https://localhost:5665/v1/objects/hosts/example-$j" \
    -d '{ "templates": [ "generic-host" ], "attrs": { "address": "127.0.0.1", "check_command": "hostalive", "vars.os" : "Linux" }, "pretty": true }'
done

It doesn't silently aborts the synchronisation attempts.

fixes #9721

[julianbrost]

This looks like it significantly changes what was only added in #9980. If it stays that way, it might make sense to revert #9980 as a whole and make a complete new attempt. That would make the backport of this cleaner (as you wouldn't have to backport a commit and its reversion at once).

What's the reason for moving ObjectNameMutex to types.{hpp,cpp}?

ceab5a6 seems to add a second independent ObjectNameMutex. What's the reason for that, shouldn't this use the same one?

[yhabteab]

What's the reason for moving ObjectNameMutex to types.{hpp,cpp}?

Well, my first thought was that we might be using it even more than we thought in #9980, and that it might introduce a circular dependency. However, if you are strongly against it, then of course I can undo it.

ceab5a6 seems to add a second independent ObjectNameMutex. What's the reason for that, shouldn't this use the same one?

Feels strange to use the same mutex for the cluster config sync and the API request. Otherwise, there's nothing against it, I guess.

[yhabteab]

ceab5a6 seems to add a second independent ObjectNameMutex. What's the reason for that, shouldn't this use the same one?

Feels strange to use the same mutex for the cluster config sync and the API request. Otherwise, there's nothing against it, I guess.

Plus ConfigObjectUtility:CreateObject() can also be used without an API listener.

[yhabteab]

ceab5a6 seems to add a second independent ObjectNameMutex. What's the reason for that, shouldn't this use the same one?

Feels strange to use the same mutex for the cluster config sync and the API request. Otherwise, there's nothing against it, I guess.

Plus ConfigObjectUtility:CreateObject() can also be used without an API listener.

Plus ApiListener::ConfigUpdateObjectAPIHandler() calls CreateObject() while holding a lock on that object and its type, so that's a good reason not to use the same mutex, I'd say.

[julianbrost]

Well, my first thought was that we might be using it even more than we thought in #9980, and that it might introduce a circular dependency. However, if you are strongly against it, then of course I can undo it.

I mainly asked the question because if it is moved, it's more code that was added in #9980 that wouldn't stay the way it was added there. So if it's moved, that would be more reason for me to revert #9980 completely and do a clean new PR. (I haven't looked at the PR in enough detail to assess whether it should be moved.)

Feels strange to use the same mutex for the cluster config sync and the API request. Otherwise, there's nothing against it, I guess.

Why does it feel strange? Think of what this mutex is supposed to protect: the consistency of the configuration of a specific object. So anything that attempts to modify the object at runtime should use the same mutex.

Imagine someone would send a PUT /v1/objects/hosts/foo request to Master 1 and then the same HTTP request to Master 2 at about the same it receives the JSON-RPC request from Master 1 to create the object. Should both requests be processed at the same time on Master 2?

Plus ApiListener::ConfigUpdateObjectAPIHandler() calls CreateObject() while holding a lock on that object and its type, so that's a good reason not to use the same mutex, I'd say.

Does the locking in ConfigUpdateObjectAPIHandler() actually do anything useful anymore, or would the locking performed in CreateObject() already suffice for ConfigUpdateObjectAPIHandler()?

[yhabteab]

Does the locking in ConfigUpdateObjectAPIHandler() actually do anything useful anymore,

IMHO yes, it does! It acquires the lock before it does ctype->GetObject(objName);, which would cause two config updates to think the object doesn't exist yet and try to create it when it doesn't.

or would the locking performed in CreateObject() already suffice for ConfigUpdateObjectAPIHandler()?

That would be too late for the cluster config sync part.

[julianbrost]

Feels strange to use the same mutex for the cluster config sync and the API request. Otherwise, there's nothing against it, I guess.

Why does it feel strange? Think of what this mutex is supposed to protect: the consistency of the configuration of a specific object. So anything that attempts to modify the object at runtime should use the same mutex.

Imagine someone would send a PUT /v1/objects/hosts/foo request to Master 1 and then the same HTTP request to Master 2 at about the same it receives the JSON-RPC request from Master 1 to create the object. Should both requests be processed at the same time on Master 2?

What about that part? Currently, the PR adds different locking based on whether an object is created via HTTP or JSON-RPC. Both would create the same object though, so both should use the same locking. Wouldn't you end up with race conditions between HTTP and JSON-RPC otherwise?

[yhabteab]

What about that part? Currently, the PR adds different locking based on whether an object is created via HTTP or JSON-RPC. Both would create the same object though, so both should use the same locking. Wouldn't you end up with race conditions between HTTP and JSON-RPC otherwise?

I don't think so! In case of a JSON-RPC connection, the object is locked twice! There is only one case in which a race condition may occur. This's when the API request enters CreateObject(), while the cluster sync takes place in this part of the code. It's hard to trigger, I'd say!

icinga2/lib/remote/apilistener-configsync.cpp

Lines 117 to 133 in a259a68

	ConfigObject::Ptr object = ctype->GetObject(objName);
	String config = params->Get("config");
	bool newObject = false;
	if (!object && !config.IsEmpty()) {
	newObject = true;
	/* object does not exist, create it through the API */
	Array::Ptr errors = new Array();
	/*
	* Create the config object through our internal API.
	* IMPORTANT: Pass the origin to prevent cluster sync loops.
	*/
	if (!ConfigObjectUtility::CreateObject(ptype, objName, config, errors, nullptr, origin)) {

[yhabteab]

I've tested all the use cases of the config sync and it is working perfectly now. However, we still need to address the use of ObjectNameMutex in the modifyobjecthandler.cpp file, particularly whether it is even necessary to lock the object name there.

[julianbrost]

However, we still need to address the use of ObjectNameMutex in the modifyobjecthandler.cpp file, particularly whether it is even necessary to lock the object name there.

Some synchronization has to happen there for sure. Am I missing something or isn't even using ObjectLock so that two post requests that modify the same object could result in some race condition?

At first glance, ObjectNameMutex also doesn't sound like a bad idea there. That will prevent both starting a modification before the object was fully created and triggering a deletion while the modification is still being processed.

[yhabteab]

Am I missing something or isn't even using ObjectLock so that two post requests that modify the same object could result in some race condition?

There is no olock on the actual configuration object, but only on the restore_attributes and these acquired in RestoreAttribute().

[Al2Klimov]

Consider unordered_{map,set}.

[julianbrost]

The exact types won't matter that much here as the overall size of those containers will stay rather small anyways.

For completeness, as I already talked with @yhabteab about it: I was thinking about maybe making this a std::set<std::pair<Type*, String>> instead (or std::unordered_set for that matter, there won't be much of a difference), as that would reduce the map + set operations down to just the set operations. But this seems to bring no huge improvements for simplifying the code (which would have been my motivation here, not performance).

[julianbrost]

Looks fine in general to me. One commit message refers to the outdated class name ObjectNameMutex though. And if you're touching the PR anyways, you might do a slight improvement to one of the comments (see my inline comment).

[tbauriedel]

ref/NC/804054

[julianbrost]

I'm somewhat confused how PRs and issues belong together here.

This PR kind of reverts 008fcd1

That part of the PR description is outdated, isn't it? That was already reverted in #10018.

If I understand correctly, #10012 describes the problem introduced by #9980 and was thus fixed with the revert in #10018. What remains is the original issue from #9721 where this PR is now the second attempt to fix it.

Therefor, I'd do the following:

• Close Runtime created objects don't get synced #10012 as "fixed by Revert "Process config::update/delete cluster events gracefully" #10018"
• Change this PR to "fixes API ConficSync - Packages - Out of Sync/Missing #9721"

Do you agree that this correctly represents the current state of these issues and PRs?

[yhabteab]

Done! I've also just rebased the PR!

[julianbrost]

Can you please provide instructions how to test this? Probably similar to #9980 but even that PR description misses information on when to create objects via the API and when to restart what. Additionally, how can one test now that this doesn't have the same problem as #10012?

[julianbrost]

I don't understand why the comment says "again".

[yhabteab]

Do you reach that point if the objectName is already in the m_LockedObjectNames map? No, so insert it again to block all other waiting threads. It's that simple!

[julianbrost]

"Again" sounds like it was important that it was in there before. That's not the case as you can lock an object for the first time, and even if it's the second time, that's not relevant for anything here.