Skip to content

Commit b6b1506

Browse files
julianbrostjulianbrost
authored
Merge pull request #10140 from Icinga/drop-cpu-bound-work-usage-from-ifwapi
Don't use thread-local var in coroutine & drop superfluous `CpuBoundWork` usage
2 parents 92df9ef + 74009f0 commit b6b1506

File tree

2 files changed

+83
-137
lines changed

2 files changed

+83
-137
lines changed

‎lib/methods/ifwapichecktask.cpp‎

Lines changed: 83 additions & 135 deletions
Original file line numberDiff line numberDiff line change
@@ -33,55 +33,6 @@ using namespace icinga;
3333

3434
REGISTER_FUNCTION_NONCONST(Internal, IfwApiCheck, &IfwApiCheckTask::ScriptFunc, "checkable:cr:resolvedMacros:useResolvedMacros");
3535

36-
static void ReportIfwCheckResult(
37-
const Checkable::Ptr& checkable, const Value& cmdLine, const CheckResult::Ptr& cr,
38-
const String& output, double start, double end, int exitcode = 3, const Array::Ptr& perfdata = nullptr
39-
)
40-
{
41-
if (Checkable::ExecuteCommandProcessFinishedHandler) {
42-
ProcessResult pr;
43-
pr.PID = -1;
44-
pr.Output = perfdata ? output + " |" + String(perfdata->Join(" ")) : output;
45-
pr.ExecutionStart = start;
46-
pr.ExecutionEnd = end;
47-
pr.ExitStatus = exitcode;
48-
49-
Checkable::ExecuteCommandProcessFinishedHandler(cmdLine, pr);
50-
} else {
51-
auto splittedPerfdata (perfdata);
52-
53-
if (perfdata) {
54-
splittedPerfdata = new Array();
55-
ObjectLock oLock (perfdata);
56-
57-
for (String pv : perfdata) {
58-
PluginUtility::SplitPerfdata(pv)->CopyTo(splittedPerfdata);
59-
}
60-
}
61-
62-
cr->SetOutput(output);
63-
cr->SetPerformanceData(splittedPerfdata);
64-
cr->SetState((ServiceState)exitcode);
65-
cr->SetExitStatus(exitcode);
66-
cr->SetExecutionStart(start);
67-
cr->SetExecutionEnd(end);
68-
cr->SetCommand(cmdLine);
69-
70-
checkable->ProcessCheckResult(cr);
71-
}
72-
}
73-
74-
static void ReportIfwCheckResult(
75-
boost::asio::yield_context yc, const Checkable::Ptr& checkable, const Value& cmdLine,
76-
const CheckResult::Ptr& cr, const String& output, double start
77-
)
78-
{
79-
double end = Utility::GetTime();
80-
CpuBoundWork cbw (yc);
81-
82-
ReportIfwCheckResult(checkable, cmdLine, cr, output, start, end);
83-
}
84-
8536
static const char* GetUnderstandableError(const std::exception& ex)
8637
{
8738
auto se (dynamic_cast<const boost::system::system_error*>(&ex));
@@ -93,10 +44,12 @@ static const char* GetUnderstandableError(const std::exception& ex)
9344
return ex.what();
9445
}
9546

47+
// Note: If DoIfwNetIo returns due to an error, the plugin output of the specified CheckResult (cr) will always be set,
48+
// and if it was successful, the cr exit status, plugin state and performance data (if any) will also be overridden.
49+
// Therefore, you have to take care yourself of setting all the other necessary fields for the check result.
9650
static void DoIfwNetIo(
97-
boost::asio::yield_context yc, const Checkable::Ptr& checkable, const Array::Ptr& cmdLine,
98-
const CheckResult::Ptr& cr, const String& psCommand, const String& psHost, const String& san, const String& psPort,
99-
AsioTlsStream& conn, boost::beast::http::request<boost::beast::http::string_body>& req, double start
51+
boost::asio::yield_context yc, const CheckResult::Ptr& cr, const String& psCommand, const String& psHost, const String& san,
52+
const String& psPort, AsioTlsStream& conn, boost::beast::http::request<boost::beast::http::string_body>& req
10053
)
10154
{
10255
namespace http = boost::beast::http;
@@ -107,11 +60,7 @@ static void DoIfwNetIo(
10760
try {
10861
Connect(conn.lowest_layer(), psHost, psPort, yc);
10962
} catch (const std::exception& ex) {
110-
ReportIfwCheckResult(
111-
yc, checkable, cmdLine, cr,
112-
"Can't connect to IfW API on host '" + psHost + "' port '" + psPort + "': " + GetUnderstandableError(ex),
113-
start
114-
);
63+
cr->SetOutput("Can't connect to IfW API on host '" + psHost + "' port '" + psPort + "': " + GetUnderstandableError(ex));
11564
return;
11665
}
11766

@@ -120,12 +69,7 @@ static void DoIfwNetIo(
12069
try {
12170
sslConn.async_handshake(conn.next_layer().client, yc);
12271
} catch (const std::exception& ex) {
123-
ReportIfwCheckResult(
124-
yc, checkable, cmdLine, cr,
125-
"TLS handshake with IfW API on host '" + psHost + "' (SNI: '" + san
126-
+ "') port '" + psPort + "' failed: " + GetUnderstandableError(ex),
127-
start
128-
);
72+
cr->SetOutput("TLS handshake with IfW API on host '" + psHost + "' (SNI: '" + san+ "') port '" + psPort + "' failed: " + GetUnderstandableError(ex));
12973
return;
13074
}
13175

@@ -135,131 +79,93 @@ static void DoIfwNetIo(
13579

13680
try {
13781
cn = GetCertificateCN(cert);
138-
} catch (const std::exception&) {
139-
}
82+
} catch (const std::exception&) { }
14083

141-
ReportIfwCheckResult(
142-
yc, checkable, cmdLine, cr,
143-
"Certificate validation failed for IfW API on host '" + psHost + "' (SNI: '" + san + "'; CN: "
144-
+ (cn.IsString() ? "'" + cn + "'" : "N/A") + ") port '" + psPort + "': " + sslConn.GetVerifyError(),
145-
start
146-
);
84+
cr->SetOutput("Certificate validation failed for IfW API on host '" + psHost + "' (SNI: '" + san + "'; CN: "
85+
+ (cn.IsString() ? "'" + cn + "'" : "N/A") + ") port '" + psPort + "': " + sslConn.GetVerifyError());
14786
return;
14887
}
14988

15089
try {
15190
http::async_write(conn, req, yc);
15291
conn.async_flush(yc);
15392
} catch (const std::exception& ex) {
154-
ReportIfwCheckResult(
155-
yc, checkable, cmdLine, cr,
156-
"Can't send HTTP request to IfW API on host '" + psHost + "' port '" + psPort + "': " + GetUnderstandableError(ex),
157-
start
158-
);
93+
cr->SetOutput("Can't send HTTP request to IfW API on host '" + psHost + "' port '" + psPort + "': " + GetUnderstandableError(ex));
15994
return;
16095
}
16196

16297
try {
16398
http::async_read(conn, buf, resp, yc);
16499
} catch (const std::exception& ex) {
165-
ReportIfwCheckResult(
166-
yc, checkable, cmdLine, cr,
167-
"Can't read HTTP response from IfW API on host '" + psHost + "' port '" + psPort + "': " + GetUnderstandableError(ex),
168-
start
169-
);
100+
cr->SetOutput("Can't read HTTP response from IfW API on host '" + psHost + "' port '" + psPort + "': " + GetUnderstandableError(ex));
170101
return;
171102
}
172103

173-
double end = Utility::GetTime();
174-
175104
{
176105
boost::system::error_code ec;
177106
sslConn.async_shutdown(yc[ec]);
178107
}
179108

180-
CpuBoundWork cbw (yc);
181109
Value jsonRoot;
182110

183111
try {
184112
jsonRoot = JsonDecode(resp.body());
185113
} catch (const std::exception& ex) {
186-
ReportIfwCheckResult(
187-
checkable, cmdLine, cr,
188-
"Got bad JSON from IfW API on host '" + psHost + "' port '" + psPort + "': " + ex.what(), start, end
189-
);
114+
cr->SetOutput("Got bad JSON from IfW API on host '" + psHost + "' port '" + psPort + "': " + ex.what());
190115
return;
191116
}
192117

193118
if (!jsonRoot.IsObjectType<Dictionary>()) {
194-
ReportIfwCheckResult(
195-
checkable, cmdLine, cr,
196-
"Got JSON, but not an object, from IfW API on host '"
197-
+ psHost + "' port '" + psPort + "': " + JsonEncode(jsonRoot),
198-
start, end
199-
);
119+
cr->SetOutput("Got JSON, but not an object, from IfW API on host '"+ psHost + "' port '" + psPort + "': " + JsonEncode(jsonRoot));
200120
return;
201121
}
202122

203123
Value jsonBranch;
204124

205125
if (!Dictionary::Ptr(jsonRoot)->Get(psCommand, &jsonBranch)) {
206-
ReportIfwCheckResult(
207-
checkable, cmdLine, cr,
208-
"Missing ." + psCommand + " in JSON object from IfW API on host '"
209-
+ psHost + "' port '" + psPort + "': " + JsonEncode(jsonRoot),
210-
start, end
211-
);
126+
cr->SetOutput("Missing ." + psCommand + " in JSON object from IfW API on host '" + psHost + "' port '" + psPort + "': " + JsonEncode(jsonRoot));
212127
return;
213128
}
214129

215130
if (!jsonBranch.IsObjectType<Dictionary>()) {
216-
ReportIfwCheckResult(
217-
checkable, cmdLine, cr,
218-
"." + psCommand + " in JSON from IfW API on host '"
219-
+ psHost + "' port '" + psPort + "' is not an object: " + JsonEncode(jsonBranch),
220-
start, end
221-
);
131+
cr->SetOutput("." + psCommand + " in JSON from IfW API on host '" + psHost + "' port '" + psPort + "' is not an object: " + JsonEncode(jsonBranch));
222132
return;
223133
}
224134

225135
Dictionary::Ptr result = jsonBranch;
226136

227-
Value exitcode;
137+
Value rawExitcode;
228138

229-
if (!result->Get("exitcode", &exitcode)) {
230-
ReportIfwCheckResult(
231-
checkable, cmdLine, cr,
139+
if (!result->Get("exitcode", &rawExitcode)) {
140+
cr->SetOutput(
232141
"Missing ." + psCommand + ".exitcode in JSON object from IfW API on host '"
233-
+ psHost + "' port '" + psPort + "': " + JsonEncode(result),
234-
start, end
142+
+ psHost + "' port '" + psPort + "': " + JsonEncode(result)
235143
);
236144
return;
237145
}
238146

239147
static const std::set<double> exitcodes {ServiceOK, ServiceWarning, ServiceCritical, ServiceUnknown};
240148
static const auto exitcodeList (Array::FromSet(exitcodes)->Join(", "));
241149

242-
if (!exitcode.IsNumber() || exitcodes.find(exitcode) == exitcodes.end()) {
243-
ReportIfwCheckResult(
244-
checkable, cmdLine, cr,
245-
"Got bad exitcode " + JsonEncode(exitcode) + " from IfW API on host '" + psHost + "' port '" + psPort
246-
+ "', expected one of: " + exitcodeList,
247-
start, end
150+
if (!rawExitcode.IsNumber() || exitcodes.find(rawExitcode) == exitcodes.end()) {
151+
cr->SetOutput(
152+
"Got bad exitcode " + JsonEncode(rawExitcode) + " from IfW API on host '" + psHost + "' port '" + psPort
153+
+ "', expected one of: " + exitcodeList
248154
);
249155
return;
250156
}
251157

158+
auto exitcode (static_cast<ServiceState>(rawExitcode.Get<double>()));
159+
252160
auto perfdataVal (result->Get("perfdata"));
253161
Array::Ptr perfdata;
254162

255163
try {
256164
perfdata = perfdataVal;
257165
} catch (const std::exception&) {
258-
ReportIfwCheckResult(
259-
checkable, cmdLine, cr,
166+
cr->SetOutput(
260167
"Got bad perfdata " + JsonEncode(perfdataVal) + " from IfW API on host '"
261-
+ psHost + "' port '" + psPort + "', expected an array",
262-
start, end
168+
+ psHost + "' port '" + psPort + "', expected an array"
263169
);
264170
return;
265171
}
@@ -269,18 +175,20 @@ static void DoIfwNetIo(
269175

270176
for (auto& pv : perfdata) {
271177
if (!pv.IsString()) {
272-
ReportIfwCheckResult(
273-
checkable, cmdLine, cr,
178+
cr->SetOutput(
274179
"Got bad perfdata value " + JsonEncode(perfdata) + " from IfW API on host '"
275-
+ psHost + "' port '" + psPort + "', expected an array of strings",
276-
start, end
180+
+ psHost + "' port '" + psPort + "', expected an array of strings"
277181
);
278182
return;
279183
}
280184
}
185+
186+
cr->SetPerformanceData(PluginUtility::SplitPerfdata(perfdata->Join(" ")));
281187
}
282188

283-
ReportIfwCheckResult(checkable, cmdLine, cr, result->Get("checkresult"), start, end, exitcode, perfdata);
189+
cr->SetState(exitcode);
190+
cr->SetExitStatus(exitcode);
191+
cr->SetOutput(result->Get("checkresult"));
284192
}
285193

286194
void IfwApiCheckTask::ScriptFunc(const Checkable::Ptr& checkable, const CheckResult::Ptr& cr,
@@ -344,6 +252,34 @@ void IfwApiCheckTask::ScriptFunc(const Checkable::Ptr& checkable, const CheckRes
344252
String username = resolveMacros("$ifw_api_username$");
345253
String password = resolveMacros("$ifw_api_password$");
346254

255+
// Use this lambda to process the final Ifw check result. Callers don't need to pass the check result
256+
// as an argument, as the lambda already captures the `cr` and notices all the `cr` changes made across
257+
// the code. You just need to set the necessary cr fields when appropriated and then call this closure.
258+
std::function<void()> reportResult;
259+
260+
if (auto callback = Checkable::ExecuteCommandProcessFinishedHandler; callback) {
261+
reportResult = [cr, callback = std::move(callback)]() {
262+
ProcessResult pr;
263+
pr.PID = -1;
264+
if (auto pd = cr->GetPerformanceData(); pd) {
265+
pr.Output = cr->GetOutput() +" |" + String(pd->Join(" "));
266+
} else {
267+
pr.Output = cr->GetOutput();
268+
}
269+
pr.ExecutionStart = cr->GetExecutionStart();
270+
pr.ExecutionEnd = cr->GetExecutionEnd();
271+
pr.ExitStatus = cr->GetExitStatus();
272+
273+
callback(cr->GetCommand(), pr);
274+
};
275+
} else {
276+
reportResult = [checkable, cr]() { checkable->ProcessCheckResult(cr); };
277+
}
278+
279+
// Set the default check result state and exit code to unknown for the moment!
280+
cr->SetExitStatus(ServiceUnknown);
281+
cr->SetState(ServiceUnknown);
282+
347283
Dictionary::Ptr params = new Dictionary();
348284

349285
if (arguments) {
@@ -369,11 +305,12 @@ void IfwApiCheckTask::ScriptFunc(const Checkable::Ptr& checkable, const CheckRes
369305
if (kv.second.GetType() == ValueObject) {
370306
auto now (Utility::GetTime());
371307

372-
ReportIfwCheckResult(
373-
checkable, command->GetName(), cr,
374-
"$ifw_api_arguments$ may not directly contain objects (especially functions).", now, now
375-
);
308+
cr->SetCommand(command->GetName());
309+
cr->SetExecutionStart(now);
310+
cr->SetExecutionEnd(now);
311+
cr->SetOutput("$ifw_api_arguments$ may not directly contain objects (especially functions).");
376312

313+
reportResult();
377314
return;
378315
}
379316
}
@@ -498,20 +435,25 @@ void IfwApiCheckTask::ScriptFunc(const Checkable::Ptr& checkable, const CheckRes
498435
auto& io (IoEngine::Get().GetIoContext());
499436
auto strand (Shared<asio::io_context::strand>::Make(io));
500437
Shared<asio::ssl::context>::Ptr ctx;
501-
double start = Utility::GetTime();
438+
439+
cr->SetExecutionStart(Utility::GetTime());
440+
cr->SetCommand(cmdLine);
502441

503442
try {
504443
ctx = SetupSslContext(cert, key, ca, crl, DEFAULT_TLS_CIPHERS, DEFAULT_TLS_PROTOCOLMIN, DebugInfo());
505444
} catch (const std::exception& ex) {
506-
ReportIfwCheckResult(checkable, cmdLine, cr, ex.what(), start, Utility::GetTime());
445+
cr->SetOutput(ex.what());
446+
cr->SetExecutionEnd(Utility::GetTime());
447+
448+
reportResult();
507449
return;
508450
}
509451

510452
auto conn (Shared<AsioTlsStream>::Make(io, *ctx, expectedSan));
511453

512454
IoEngine::SpawnCoroutine(
513455
*strand,
514-
[strand, checkable, cmdLine, cr, psCommand, psHost, expectedSan, psPort, conn, req, start, checkTimeout](asio::yield_context yc) {
456+
[strand, checkable, cr, psCommand, psHost, expectedSan, psPort, conn, req, checkTimeout, reportResult = std::move(reportResult)](asio::yield_context yc) {
515457
Timeout::Ptr timeout = new Timeout(strand->context(), *strand, boost::posix_time::microseconds(int64_t(checkTimeout * 1e6)),
516458
[&conn, &checkable](boost::asio::yield_context yc) {
517459
Log(LogNotice, "IfwApiCheckTask")
@@ -525,7 +467,13 @@ void IfwApiCheckTask::ScriptFunc(const Checkable::Ptr& checkable, const CheckRes
525467

526468
Defer cancelTimeout ([&timeout]() { timeout->Cancel(); });
527469

528-
DoIfwNetIo(yc, checkable, cmdLine, cr, psCommand, psHost, expectedSan, psPort, *conn, *req, start);
470+
DoIfwNetIo(yc, cr, psCommand, psHost, expectedSan, psPort, *conn, *req);
471+
472+
cr->SetExecutionEnd(Utility::GetTime());
473+
474+
// Post the check result processing to the global pool not to block the I/O threads,
475+
// which could affect processing important RPC messages and HTTP connections.
476+
Utility::QueueAsyncCallback(reportResult);
529477
}
530478
);
531479
}

‎lib/remote/httpserverconnection.cpp‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -348,8 +348,6 @@ bool EnsureValidBody(
348348
Array::Ptr permissions = authenticatedUser->GetPermissions();
349349

350350
if (permissions) {
351-
CpuBoundWork evalPermissions (yc);
352-
353351
ObjectLock olock(permissions);
354352

355353
for (const Value& permissionInfo : permissions) {

0 commit comments

Comments
 (0)