oomd: Fix Killed signal reason being lost (#40689)
Emitting "oom" doesn't mesh with the org.freedesktop.oom1.Manager Killed() API contract, which defines "memory-used" and "memory-pressure" as possible reasons. Consumers that key off reason thus will either lose policy attribution or may reject the unknown value completely. Plumb the reason through so it is visible to consumers.
This commit is contained in:
@@ -416,7 +416,7 @@ static int monitor_swap_contexts_handler(sd_event_source *s, uint64_t usec, void
|
||||
return 0;
|
||||
}
|
||||
|
||||
r = oomd_cgroup_kill_mark(m, selected);
|
||||
r = oomd_cgroup_kill_mark(m, selected, "memory-used");
|
||||
if (r == -ENOMEM)
|
||||
return log_oom();
|
||||
if (r < 0)
|
||||
@@ -534,7 +534,7 @@ static int monitor_memory_pressure_contexts_handler(sd_event_source *s, uint64_t
|
||||
return 0;
|
||||
}
|
||||
|
||||
r = oomd_cgroup_kill_mark(m, selected);
|
||||
r = oomd_cgroup_kill_mark(m, selected, "memory-pressure");
|
||||
if (r == -ENOMEM)
|
||||
return log_oom();
|
||||
if (r < 0)
|
||||
|
||||
+8
-4
@@ -27,6 +27,7 @@
|
||||
typedef struct OomdKillState {
|
||||
Manager *manager;
|
||||
OomdCGroupContext *ctx;
|
||||
const char *reason;
|
||||
/* This holds sd_varlink references */
|
||||
Set *links;
|
||||
} OomdKillState;
|
||||
@@ -245,11 +246,12 @@ int oomd_sort_cgroup_contexts(Hashmap *h, oomd_compare_t compare_func, const cha
|
||||
return (int) k;
|
||||
}
|
||||
|
||||
int oomd_cgroup_kill(Manager *m, OomdCGroupContext *ctx, bool recurse) {
|
||||
int oomd_cgroup_kill(Manager *m, OomdCGroupContext *ctx, bool recurse, const char *reason) {
|
||||
_cleanup_set_free_ Set *pids_killed = NULL;
|
||||
int r;
|
||||
|
||||
assert(ctx);
|
||||
assert(!m || reason);
|
||||
|
||||
pids_killed = set_new(NULL);
|
||||
if (!pids_killed)
|
||||
@@ -288,7 +290,7 @@ int oomd_cgroup_kill(Manager *m, OomdCGroupContext *ctx, bool recurse) {
|
||||
"Killed",
|
||||
"ss",
|
||||
ctx->path,
|
||||
"oom");
|
||||
reason);
|
||||
|
||||
return !set_isempty(pids_killed);
|
||||
}
|
||||
@@ -334,7 +336,7 @@ static void oomd_kill_state_remove(OomdKillState *ks) {
|
||||
if (!set_isempty(ks->links))
|
||||
return;
|
||||
|
||||
r = oomd_cgroup_kill(ks->manager, ks->ctx, /* recurse= */ true);
|
||||
r = oomd_cgroup_kill(ks->manager, ks->ctx, /* recurse= */ true, ks->reason);
|
||||
if (r < 0)
|
||||
log_debug_errno(r, "Failed to kill cgroup '%s', ignoring: %m", ks->ctx->path);
|
||||
oomd_kill_state_free(ks);
|
||||
@@ -465,11 +467,12 @@ static int oomd_prekill_hook(Manager *m, OomdKillState *ks) {
|
||||
return 0;
|
||||
}
|
||||
|
||||
int oomd_cgroup_kill_mark(Manager *m, OomdCGroupContext *ctx) {
|
||||
int oomd_cgroup_kill_mark(Manager *m, OomdCGroupContext *ctx, const char *reason) {
|
||||
int r;
|
||||
|
||||
assert(ctx);
|
||||
assert(m);
|
||||
assert(reason);
|
||||
|
||||
if (m->dry_run) {
|
||||
_cleanup_free_ char *cg_path = NULL;
|
||||
@@ -489,6 +492,7 @@ int oomd_cgroup_kill_mark(Manager *m, OomdCGroupContext *ctx) {
|
||||
*ks = (OomdKillState) {
|
||||
.manager = m,
|
||||
.ctx = oomd_cgroup_context_ref(ctx),
|
||||
.reason = reason,
|
||||
};
|
||||
|
||||
r = set_ensure_put(&m->kill_states, &oomd_kill_state_hash_ops, ks);
|
||||
|
||||
+2
-2
@@ -124,8 +124,8 @@ int oomd_sort_cgroup_contexts(Hashmap *h, oomd_compare_t compare_func, const cha
|
||||
int oomd_fetch_cgroup_oom_preference(OomdCGroupContext *ctx, const char *prefix);
|
||||
|
||||
/* Returns a negative value on error, 0 if no processes were killed, or 1 if processes were killed. */
|
||||
int oomd_cgroup_kill(Manager *m, OomdCGroupContext *ctx, bool recurse);
|
||||
int oomd_cgroup_kill_mark(Manager *m, OomdCGroupContext *ctx);
|
||||
int oomd_cgroup_kill(Manager *m, OomdCGroupContext *ctx, bool recurse, const char *reason);
|
||||
int oomd_cgroup_kill_mark(Manager *m, OomdCGroupContext *ctx, const char *reason);
|
||||
|
||||
/* The following oomd_kill_by_* functions return 1 if processes were killed, or negative otherwise. */
|
||||
/* If `prefix` is supplied, only cgroups whose paths start with `prefix` are eligible candidates. Otherwise,
|
||||
|
||||
+155
-1
@@ -1,20 +1,25 @@
|
||||
/* SPDX-License-Identifier: LGPL-2.1-or-later */
|
||||
|
||||
#include <stdlib.h>
|
||||
#include <sys/socket.h>
|
||||
#include <unistd.h>
|
||||
|
||||
#include "sd-bus.h"
|
||||
|
||||
#include "alloc-util.h"
|
||||
#include "cgroup-setup.h"
|
||||
#include "cgroup-util.h"
|
||||
#include "errno-util.h"
|
||||
#include "fd-util.h"
|
||||
#include "fileio.h"
|
||||
#include "oomd-manager.h"
|
||||
#include "oomd-util.h"
|
||||
#include "parse-util.h"
|
||||
#include "path-util.h"
|
||||
#include "pidref.h"
|
||||
#include "process-util.h"
|
||||
#include "set.h"
|
||||
#include "string-util.h"
|
||||
#include "tests.h"
|
||||
#include "time-util.h"
|
||||
#include "tmpfile-util.h"
|
||||
@@ -59,6 +64,103 @@ static int fork_and_sleep(unsigned sleep_min, PidRef *ret) {
|
||||
return r;
|
||||
}
|
||||
|
||||
static int setup_local_oomd_bus(sd_bus **ret_server, sd_bus **ret_client) {
|
||||
_cleanup_(sd_bus_flush_close_unrefp) sd_bus *server = NULL, *client = NULL;
|
||||
_cleanup_close_pair_ int pair[2] = EBADF_PAIR;
|
||||
int r;
|
||||
|
||||
assert(ret_server);
|
||||
assert(ret_client);
|
||||
|
||||
r = socketpair(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, pair);
|
||||
if (r < 0)
|
||||
return -errno;
|
||||
|
||||
r = sd_bus_new(&server);
|
||||
if (r < 0)
|
||||
return r;
|
||||
|
||||
r = sd_bus_set_fd(server, pair[0], pair[0]);
|
||||
if (r < 0)
|
||||
return r;
|
||||
pair[0] = -EBADF;
|
||||
|
||||
r = sd_bus_set_server(server, true, SD_ID128_NULL);
|
||||
if (r < 0)
|
||||
return r;
|
||||
|
||||
r = sd_bus_set_anonymous(server, true);
|
||||
if (r < 0)
|
||||
return r;
|
||||
|
||||
r = sd_bus_start(server);
|
||||
if (r < 0)
|
||||
return r;
|
||||
|
||||
r = sd_bus_new(&client);
|
||||
if (r < 0)
|
||||
return r;
|
||||
|
||||
r = sd_bus_set_fd(client, pair[1], pair[1]);
|
||||
if (r < 0)
|
||||
return r;
|
||||
pair[1] = -EBADF;
|
||||
|
||||
r = sd_bus_set_anonymous(client, true);
|
||||
if (r < 0)
|
||||
return r;
|
||||
|
||||
r = sd_bus_start(client);
|
||||
if (r < 0)
|
||||
return r;
|
||||
|
||||
*ret_server = TAKE_PTR(server);
|
||||
*ret_client = TAKE_PTR(client);
|
||||
return 0;
|
||||
}
|
||||
|
||||
static int wait_for_killed_signal(sd_bus *server, sd_bus *client, const char *cgroup_path, const char *reason) {
|
||||
int r;
|
||||
|
||||
assert(server);
|
||||
assert(client);
|
||||
assert(cgroup_path);
|
||||
assert(reason);
|
||||
|
||||
for (size_t i = 0; i < 200; i++) {
|
||||
_cleanup_(sd_bus_message_unrefp) sd_bus_message *m_client = NULL;
|
||||
|
||||
r = sd_bus_process(server, NULL);
|
||||
if (r < 0)
|
||||
return r;
|
||||
|
||||
r = sd_bus_process(client, &m_client);
|
||||
if (r < 0)
|
||||
return r;
|
||||
|
||||
if (m_client && sd_bus_message_is_signal(m_client, "org.freedesktop.oom1.Manager", "Killed")) {
|
||||
const char *got_cgroup, *got_reason;
|
||||
|
||||
r = sd_bus_message_read(m_client, "ss", &got_cgroup, &got_reason);
|
||||
if (r < 0)
|
||||
return r;
|
||||
|
||||
if (!streq(got_cgroup, cgroup_path))
|
||||
return -ENOMSG;
|
||||
if (!streq(got_reason, reason))
|
||||
return -ENOMSG;
|
||||
|
||||
return 0;
|
||||
}
|
||||
|
||||
r = sd_bus_wait(client, 50 * USEC_PER_MSEC);
|
||||
if (r < 0)
|
||||
return r;
|
||||
}
|
||||
|
||||
return -ETIMEDOUT;
|
||||
}
|
||||
|
||||
TEST(oomd_cgroup_kill) {
|
||||
_cleanup_free_ char *subcgroup = NULL;
|
||||
int r;
|
||||
@@ -88,7 +190,11 @@ TEST(oomd_cgroup_kill) {
|
||||
ASSERT_OK(fork_and_sleep(5, &two));
|
||||
ASSERT_OK(cg_attach(subcgroup, two.pid));
|
||||
|
||||
ASSERT_OK_POSITIVE(oomd_cgroup_kill(NULL /* manager */, &(OomdCGroupContext){ .path = subcgroup }, false /* recurse */));
|
||||
ASSERT_OK_POSITIVE(oomd_cgroup_kill(
|
||||
/* m= */ NULL,
|
||||
&(OomdCGroupContext){ .path = subcgroup },
|
||||
/* recurse= */ false,
|
||||
/* reason= */ NULL));
|
||||
|
||||
ASSERT_OK(cg_get_xattr(subcgroup, "user.oomd_ooms", &v, /* ret_size= */ NULL));
|
||||
ASSERT_STREQ(v, i == 0 ? "1" : "2");
|
||||
@@ -113,6 +219,54 @@ TEST(oomd_cgroup_kill) {
|
||||
ASSERT_OK(cg_trim(subcgroup, /* delete_root */ true));
|
||||
}
|
||||
|
||||
TEST(oomd_cgroup_kill_signal_reason) {
|
||||
_cleanup_(sd_bus_flush_close_unrefp) sd_bus *server = NULL, *client = NULL;
|
||||
_cleanup_free_ char *subcgroup = NULL;
|
||||
static const char *reasons[] = {
|
||||
"memory-used",
|
||||
"memory-pressure",
|
||||
};
|
||||
Manager m = {};
|
||||
int r;
|
||||
|
||||
r = enter_cgroup_root_cached();
|
||||
if (r < 0)
|
||||
return (void) log_tests_skipped_errno(r, "Failed to enter cgroup root");
|
||||
|
||||
ASSERT_NOT_NULL(subcgroup = path_join(cgroup, "oomdkillsignaltest"));
|
||||
ASSERT_OK(cg_trim(subcgroup, /* delete_root= */ true));
|
||||
ASSERT_OK(cg_create(subcgroup));
|
||||
|
||||
ASSERT_OK(setup_local_oomd_bus(&server, &client));
|
||||
m.bus = server;
|
||||
|
||||
for (size_t i = 0; i < ELEMENTSOF(reasons); i++) {
|
||||
_cleanup_(pidref_done) PidRef one = PIDREF_NULL, two = PIDREF_NULL;
|
||||
const char *reason = reasons[i];
|
||||
|
||||
ASSERT_OK(fork_and_sleep(5, &one));
|
||||
ASSERT_OK(cg_attach(subcgroup, one.pid));
|
||||
ASSERT_OK(fork_and_sleep(5, &two));
|
||||
ASSERT_OK(cg_attach(subcgroup, two.pid));
|
||||
|
||||
ASSERT_OK_POSITIVE(oomd_cgroup_kill(&m, &(OomdCGroupContext) { .path = subcgroup }, /* recurse= */ false, reason));
|
||||
ASSERT_OK(wait_for_killed_signal(server, client, subcgroup, reason));
|
||||
|
||||
/* Cleanup isn't instantaneous, so give it some grace */
|
||||
bool empty = false;
|
||||
for (size_t t = 0; t < 100; t++) {
|
||||
usleep_safe(100 * USEC_PER_MSEC);
|
||||
if (ASSERT_OK(cg_is_empty(subcgroup)) > 0) {
|
||||
empty = true;
|
||||
break;
|
||||
}
|
||||
}
|
||||
ASSERT_TRUE(empty);
|
||||
}
|
||||
|
||||
ASSERT_OK(cg_trim(subcgroup, /* delete_root= */ true));
|
||||
}
|
||||
|
||||
TEST(oomd_cgroup_context_acquire_and_insert) {
|
||||
_cleanup_hashmap_free_ Hashmap *h1 = NULL, *h2 = NULL;
|
||||
_cleanup_(oomd_cgroup_context_unrefp) OomdCGroupContext *ctx = NULL;
|
||||
|
||||
Reference in New Issue
Block a user