Fix broken runtime config sync

[yhabteab]

This PR kind of reverts 008fcd1 but it's still using AtomicFile to persist the configs to disk. Additionally, ConfigObjectUtility::CreateObject() utilises the ObjectNameMutex class to prevent an object of a given type from being created concurrently, even if it is hard to trigger that data race via API requests.

Tests

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

Before

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 silently aborts the synchronisation attempts.

After

Works as it should!

fixes #10012

[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