truncate resource journal at configurable size #6633

grondo · 2025-02-13T19:25:49Z

This is the simplified version of #6631. The in-memory resource eventlog is truncated at the configured limit, but the truncate event does not contain any context, it just marks the fact that events have been dropped.

Fixes #6610

chu11 · 2025-02-13T19:35:09Z

src/modules/resource/reslog.c

@@ -36,7 +36,7 @@ struct reslog {
    struct resource_ctx *ctx;
    zlist_t *pending;       // list of pending futures
    zlist_t *watchers;
-    json_t *eventlog;
+    zlistx_t *eventlog;


commit message typo "Swtich" and "reosurce"

chu11 · 2025-02-13T19:36:18Z

src/modules/resource/reslog.c

@@ -217,8 +217,7 @@ int reslog_post_pack (struct reslog *reslog,
    va_end (ap);
    if (!event)
        return -1;
-    if (json_array_append (reslog->eventlog, event) < 0) {
-        json_decref (event);
+    if (!zlistx_add_end (reslog->eventlog, json_incref (event))) {
        errno = ENOMEM;
        return -1;
    }


if zlistx_add_end() fails, I think you gotta call the json_decref() b/c you json_incref()ed above

It might be slightly simpler to just define an entry_duplicator().

chu11 · 2025-02-13T19:37:41Z

src/modules/resource/reslog.c

+            if (!zlistx_add_end (reslog->eventlog, json_incref (entry)))
                goto nomem;


gotta decref entry if zlistx_add_end() failed?

chu11 · 2025-02-13T19:46:10Z

src/modules/resource/reslog.c

@@ -197,6 +197,43 @@ int reslog_sync (struct reslog *reslog)
    return 0;
 }

+/*  Truncate resource journal if needed to reslog->journal_max


commit message "replace the most recent event" ... perhaps you mean "replace the oldest event"?

Meant to be "replace the most recently dropped event" - not the oldest event in case more than one event is dropped at a time.

chu11 · 2025-02-13T19:47:55Z

t/t2355-resource-journal.t

+test_expect_success '1st event is a truncate event' '
+	head -1 eventlog.trunc > eventlog.trunc.1 &&
+	jq -e ".name == \"truncate\"" eventlog.trunc.1
+'


suggestion: also check number of entries == limit, and likewise with 2 tests below this one

garlick

I poked this a bit and LGTM.

grondo · 2025-02-13T23:18:20Z

I think I've addressed the comments above, rebased, and forced a push.

There's also a slight change here that ensures the eventlog doesn't have limit + 1 entries after truncation. This means there's actually max - 1 actual eventlog entries in a truncated list.

Problem: The resource journal will need to truncated by dropping entries from the start, but a jansson json array is not conducive to this operation. Switch to a zlistx_t for the resource journal. This data structure allows the removal and addition of items to the front of the list (without moving the memory for the rest of the items on the list)

Problem: The in-memory resource journal grows without bound and there is no way to limit its size. In preparation for truncating the resource journal, add a new config key in the [resource] table, `journal_max`, which can be used to set a limit on the journal size. Default this limit to 100000 entries. Modify the reslog constructor to accept this journal_max limit, but don't do anything with it just yet.

Problem: The resource journal limit cannot be configured at runtime because configuration updates are ignored by the resource module. Capture any new resource.journal_max entry on config reload and send it to the reslog implementation.

Problem: The resource journal can grow without bound. Truncate the in-memory resource eventlog if configured and replace the most recently dropped event with an RFC 44 truncate event.

Problem: There's no testing of resource journal truncation in the testsuite. Add a set of very basic tests to t2355-resource-journal.t that ensure basic eventlog truncation works.

Problem: The resource.journal-max configuration key is not documented. Add it to flux-config-resource(5).

grondo · 2025-02-14T14:52:30Z

Uh, I guess I'll set MWP here now.

codecov · 2025-02-14T15:21:27Z

Codecov Report

Attention: Patch coverage is 88.46154% with 6 lines in your changes missing coverage. Please review.

Project coverage is 83.88%. Comparing base (04c71be) to head (6f79323).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
src/modules/resource/reslog.c	86.95%	6 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6633   +/-   ##
=======================================
  Coverage   83.87%   83.88%           
=======================================
  Files         533      533           
  Lines       88331    88372   +41     
=======================================
+ Hits        74087    74127   +40     
- Misses      14244    14245    +1

Files with missing lines	Coverage Δ
src/modules/resource/resource.c	`86.43% <100.00%> (-0.24%)`	⬇️
src/modules/resource/reslog.c	`74.27% <86.95%> (+1.89%)`	⬆️

... and 7 files with indirect coverage changes

chu11 reviewed Feb 13, 2025

View reviewed changes

garlick approved these changes Feb 13, 2025

View reviewed changes

grondo force-pushed the reslog-truncate-simple branch 2 times, most recently from bc88096 to 05a34be Compare February 13, 2025 23:10

grondo added 6 commits February 14, 2025 06:52

resource: allow reconfigure of journal limit

429de17

Problem: The resource journal limit cannot be configured at runtime because configuration updates are ignored by the resource module. Capture any new resource.journal_max entry on config reload and send it to the reslog implementation.

resource: truncate resource journal if configured

38de4e3

Problem: The resource journal can grow without bound. Truncate the in-memory resource eventlog if configured and replace the most recently dropped event with an RFC 44 truncate event.

testsuite: add resource journal truncation testing

d2bb51b

Problem: There's no testing of resource journal truncation in the testsuite. Add a set of very basic tests to t2355-resource-journal.t that ensure basic eventlog truncation works.

doc: document journal-max key in flux-config-resource(5)

6f79323

Problem: The resource.journal-max configuration key is not documented. Add it to flux-config-resource(5).

grondo force-pushed the reslog-truncate-simple branch from 05a34be to 6f79323 Compare February 14, 2025 14:52

grondo added the merge-when-passing label Feb 14, 2025

mergify bot merged commit f6bef37 into flux-framework:master Feb 14, 2025
35 checks passed

grondo deleted the reslog-truncate-simple branch February 14, 2025 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

truncate resource journal at configurable size #6633

truncate resource journal at configurable size #6633

grondo commented Feb 13, 2025 •

edited

Loading

chu11 Feb 13, 2025

chu11 Feb 13, 2025

garlick Feb 13, 2025

chu11 Feb 13, 2025

chu11 Feb 13, 2025

grondo Feb 13, 2025

chu11 Feb 13, 2025

garlick left a comment

grondo commented Feb 13, 2025

grondo commented Feb 14, 2025

codecov bot commented Feb 14, 2025

		if (!zlistx_add_end (reslog->eventlog, json_incref (entry)))
		goto nomem;

truncate resource journal at configurable size #6633

truncate resource journal at configurable size #6633

Conversation

grondo commented Feb 13, 2025 • edited Loading

chu11 Feb 13, 2025

Choose a reason for hiding this comment

chu11 Feb 13, 2025

Choose a reason for hiding this comment

garlick Feb 13, 2025

Choose a reason for hiding this comment

chu11 Feb 13, 2025

Choose a reason for hiding this comment

chu11 Feb 13, 2025

Choose a reason for hiding this comment

grondo Feb 13, 2025

Choose a reason for hiding this comment

chu11 Feb 13, 2025

Choose a reason for hiding this comment

garlick left a comment

Choose a reason for hiding this comment

grondo commented Feb 13, 2025

grondo commented Feb 14, 2025

codecov bot commented Feb 14, 2025

Codecov Report

grondo commented Feb 13, 2025 •

edited

Loading