Runtime RPC sync failures

[Al2Klimov]

Backport of

• DependencyGraph: switch "parent" and "child" terminology #10263
• DependencyGraph: use ConfigObject*, not Object* #10264
• Fix broken downtime comment sync #10000

[Al2Klimov]

Colleagues, did enough Icinga 2.14 releases not misbehave because of C++17 for us to backport C++17 to v2.13?

[yhabteab]

Why?

diff --git a/lib/base/dependencygraph.cpp b/lib/base/dependencygraph.cpp
index d209e77b6..06446cfc2 100644
--- a/lib/base/dependencygraph.cpp
+++ b/lib/base/dependencygraph.cpp
@@ -42,7 +42,7 @@ std::vector<ConfigObject::Ptr> DependencyGraph::GetParents(const ConfigObject::P
 {
 	std::vector<ConfigObject::Ptr> objects;

-	std::unique_lock<std::mutex> lock(m_Mutex);
+	std::unique_lock lock(m_Mutex);
 	auto [begin, end] = m_Dependencies.get<2>().equal_range(child.get());
 	std::transform(begin, end, std::back_inserter(objects), [](const Edge& edge) {
 		return edge.parent;
@@ -62,7 +62,7 @@ std::vector<ConfigObject::Ptr> DependencyGraph::GetChildren(const ConfigObject::
 {
 	std::vector<ConfigObject::Ptr> objects;

-	std::unique_lock<std::mutex> lock(m_Mutex);
+	std::unique_lock lock(m_Mutex);
 	auto [begin, end] = m_Dependencies.get<1>().equal_range(parent.get());
 	std::transform(begin, end, std::back_inserter(objects), [](const Edge& edge) {
 		return edge.child;
diff --git a/lib/remote/apilistener-configsync.cpp b/lib/remote/apilistener-configsync.cpp
index 14da96754..a45f0fc2a 100644
--- a/lib/remote/apilistener-configsync.cpp
+++ b/lib/remote/apilistener-configsync.cpp
@@ -11,6 +11,7 @@
 #include "config/vmops.hpp"
 #include "remote/configobjectslock.hpp"
 #include <fstream>
+#include <unordered_set>

 using namespace icinga;

diff --git a/lib/remote/apilistener.hpp b/lib/remote/apilistener.hpp
index 128458a42..ad2c5ee3f 100644
--- a/lib/remote/apilistener.hpp
+++ b/lib/remote/apilistener.hpp
@@ -25,7 +25,6 @@
 #include <cstdint>
 #include <mutex>
 #include <set>
-#include <unordered_set>

 namespace icinga
 {

[Al2Klimov]

I performed (the reverse of) that diff for this PR to compile despite lack of C++17.

[yhabteab]

Colleagues, did enough Icinga 2.14 releases not misbehave because of C++17 for us to backport C++17 to v2.13?

I don't know 🤷‍♂️ if it makes sense to backport it now. I would rather not backport this PR to 2.13 at all.

[julianbrost]

Colleagues, did enough Icinga 2.14 releases not misbehave because of C++17 for us to backport C++17 to v2.13?

I had to read this like 5 times to understand that you're probably suggesting to switch to C++17 for the support/2.13 branch.

Backporting a PR may involve adapting code to the old support branch, just like you have to resolve merge conflicts some time. So is there anything in the original PR that needs an unreasonable effort to get working without C++17?

I don't know 🤷‍♂️ if it makes sense to backport it now. I would rather not backport this PR to 2.13 at all.

I'm also confused why this backport PR now appears only on the day before the planned release. But it doesn't look critical enough to justify blocking the bugfix release for our older support branch. So if in doubt, I'd say this doesn't make it into 2.13.11.

[Al2Klimov]

Colleagues, did enough Icinga 2.14 releases not misbehave because of C++17 for us to backport C++17 to v2.13?

I had to read this like 5 times to understand that you're probably suggesting to switch to C++17 for the support/2.13 branch.

• We had four releases with C++17 (Icinga 2.14)
• Three of them were bugfix releases

Did any of them made any trouble BECAUSE OF C++17?

If no (I'm 1000000000% sure), what's stopping us from backporting C++17 to v2.13 then?

Backporting a PR may involve adapting code to the old support branch, just like you have to resolve merge conflicts some time.

Sure. Sometimes you have to. And sometimes you have a choice. E.g to cherry-pick more/less stuff and/or in the correct order. (Just have a look at b55b536 which you even approved IIRC.)

So is there anything in the original PR that needs an unreasonable effort to get working without C++17?

1. Runtime RPC sync failures #10316 (review) (already done, so doesn't matter)
2. Especially Windows (Runtime RPC sync failures #10316 (comment)) complains about:
   • if (auto x = y; x) {
   • auto [x, y] = z;
   • Lack of std::transform

[yhabteab]

If no (I'm 1000000000% sure), what's stopping us from backporting C++17 to v2.13 then?

Backporting a PR may involve adapting code to the old support branch, just like you have to resolve merge conflicts some time.

Sure. Sometimes you have to. And sometimes you have a choice. E.g to cherry-pick more/less stuff and/or in the correct order. (Just have a look at b55b536 which you even approved IIRC.)

You could have done either of those things over the last two days, but now, just hours before the actual release, adding such extra headaches unnecessarily doesn't make anyone happy. You even partially changed the original code, so it could have been a matter of minutes to change the other parts that causes Windows to fail as well, and yet arguing over backporting or not some unrelated PR just because of this now doesn't justify it.

So please forget this PR and get on with the other relevant things as this is not the super critical PR that must be backported anyway.

[Al2Klimov]

If no (I'm 1000000000% sure), what's stopping us from backporting C++17 to v2.13 then?

Backporting a PR may involve adapting code to the old support branch, just like you have to resolve merge conflicts some time.

Sure. Sometimes you have to. And sometimes you have a choice. E.g to cherry-pick more/less stuff and/or in the correct order. (Just have a look at b55b536 which you even approved IIRC.)

You could have done either of those things over the last two days,

Just like you _{(https://github.com/Icinga/icinga2/pulls?q=is%3Apr+author%3Ayhabteab+milestone%3A2.14.4+is%3Aclosed)}. I'm not the (only) Chief Backport Officer.

just hours before the actual release, adding such extra headaches unnecessarily doesn't make anyone happy. You even partially changed the original code, so it could have been a matter of minutes to change the other parts that causes Windows to fail as well,

Hell no! I've looked at the board, just to be sure and created the backports WE ALL have overseen. And then, imagine, I(!) asked others' opinions instead of just backporting C++17 along with it. 👀

arguing over backporting or not some unrelated PR just because of this now doesn't justify it.

No, but my original question (no one has answered, yet) justifies it.

this is not the super critical PR that must be backported anyway.

it doesn't look critical enough to justify blocking the bugfix release for our older support branch.

If you two think that INDEPENDENT OF how hard it's to backport, I'm fine with it.

[yhabteab]

Just built it locally and it succeeded! So, now it's up to @julianbrost!

[julianbrost]

I vote against doing any last minute changes now that are not very important for the release. I mean it compiles, but did anyone actually test the adapted code so far? Hence, I'm removing it from the milestone.

[Al2Klimov]

And I vote for people answering my original question (#10316 (comment)) as a base of the discussion how to proceed here.

While rebasing this PR for making #10316 (review) a commit on its own, I kept da5d8e7 just not to start an actual so-called edit war and not because I agree with it.

[julianbrost]

If no (I'm 1000000000% sure), what's stopping us from backporting C++17 to v2.13 then?

There is no need to do so. The point of the support branches is to provide fixes for problems. So what is the problem that switching the old support branch to C++17 would solve? Sure, things that itself don't fix anything may be included if they are a requirement for a fix. But here there's a perfectly reasonable alternative that does not need extending the scope of the backport.

Just have a look at b55b536 which you even approved IIRC.

That's simply not comparable. That only changed two lines that are overwritten in a merge anyways, so that commit doesn't even have an effect on the code after the merge, it just simplified the merge.

So is there anything in the original PR that needs an unreasonable effort to get working without C++17?

1. Runtime RPC sync failures #10316 (review) (already done, so doesn't matter)

2. Especially Windows (Runtime RPC sync failures #10316 (comment)) complains about:

   • if (auto x = y; x) {
   • auto [x, y] = z;
   • Lack of std::transform

Just because something needs a bit of work doesn't mean that's unreasonable effort. All of this is pretty easy to adapt to C++14, so it's perfectly reasonable to do that.