ENT-6511: Increase cf-monitord measurement slots from 100 to 300 by nickanderson · Pull Request #6239 · cfengine/core

nickanderson · 2026-07-20T15:23:44Z

Raises CF_OBSERVABLES from 100 to 300. Previously out of the box ~80 of the 100 slots are consumed by default measurements, leaving only ~19 for custom measurements: promises.

Notably, this change does increase the disk footprint required by an agent by ~~60MB (~~.27MB per slot). That storage is required even if the slots are not in active use (Ref: ENT-14329)

Ticket: ENT-6511

nickanderson · 2026-07-20T16:21:12Z

@cf-bottom jenkins please.

cf-bottom · 2026-07-20T16:23:08Z

Sure, I triggered a build:

Jenkins: https://ci.cfengine.com/job/pr-pipeline/14244/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-14244/

larsewi

Otherwise LGTM

nickanderson · 2026-07-21T16:35:59Z

@cf-bottom jenkins please.

cf-bottom · 2026-07-21T16:38:12Z

Alright, I triggered a build:

Jenkins: https://ci.cfengine.com/job/pr-pipeline/14253/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-14253/

larsewi · 2026-07-22T11:55:28Z

-        // Fall back to simple printing in release builds:
+        // Fall back to simple printing in release builds. Records written
+        // before CF_OBSERVABLES was raised are shorter, but a running agent
+        // migrates them (see dbm_migration_observations.c) before they are read.


Feels like this change does not belong here anymore

Reverted to the original // Fall back to simple printing in release builds:.

larsewi · 2026-07-22T11:57:05Z

+            // Migration version scalar (dbm_migration_observations.c), a short
+            // string rather than an Averages struct.


I'm not sure I understand this comment

Reworded. The migration stores a version bookkeeping key in these DBs; its value is a short version string, not an Averages struct, so it is printed as a string rather than falling through to print_struct_averages() (which would trip the struct-size assertion).

larsewi · 2026-07-22T12:06:38Z

@@ -0,0 +1,113 @@
+/*
+  Copyright 2024 Northern.tech AS


Suggested change

Copyright 2024 Northern.tech AS

Copyright 2026 Northern.tech AS

larsewi · 2026-07-22T12:10:50Z

+                /* The ts_key has fewer entries than CF_OBSERVABLES -- e.g. it
+                 * was written by an older agent with a smaller CF_OBSERVABLES.
+                 * Fill the remaining names so the returned array keeps its
+                 * documented non-NULL guarantee (callers index all
+                 * CF_OBSERVABLES entries). */
+                for (int j = i; j < CF_OBSERVABLES; ++j)
+                {
+                    snprintf(buf, CF_MAXVARSIZE, "spare[%d]", j);
+                    temp[j] = xstrdup(buf);
+                }


Is this still needed post migration fix ?

Yes — this is about the ts_key text file, which the DB migration does not touch. GetObservableNames() is documented to return a CF_OBSERVABLES-sized array of non-NULL strings. A ts_key written by an older agent has only 100 lines, so after the bump fgets hits EOF at slot 100 and, without this fill, temp[100..299] stay uninitialized (xmalloc, not zeroed); the sole caller (dump.c:179) then indexes and free()s all 300 → uninitialized read + invalid free. So it is an independent correctness fix.

And, just FYI, I have a follow-up PR waiting to changes the storage to only require what is used

larsewi · 2026-07-22T12:19:50Z

+    char cmd[CF_BUFSIZE];
+    xsnprintf(cmd, CF_BUFSIZE, "rm -rf '%s'/*", GetStateDir());
+    system(cmd);


I know, I know... Recursively deleting a directory is not as easy as one would think in C. However, there probably is a function you can use in libntech or libpromises so that you don't have to run a command.

Although I get the argument that the unit tests should be as lightweight as possible. Either way is fine by me.

Switched to DeleteDirectoryTree() from libntech (empties the tree, keeps the top dir), plus a trailing rmdir() in teardown.

larsewi · 2026-07-22T12:22:16Z

+    system(cmd);
+
+    DBHandle *db;
+    assert_int_equal(OpenDB(&db, dbid_observations), true);


Use assert_true instead of treating bool as an int, even though it really is. It will also give more descriptive error messages.

Suggested change

assert_int_equal(OpenDB(&db, dbid_observations), true);

assert_true(OpenDB(&db, dbid_observations));

Done — converted the bool-returning assertions (OpenDB/WriteDB/ReadDB/HasKeyDB/NewDBCursor/DeleteDBCursor) to assert_true.

larsewi · 2026-07-22T12:25:53Z

+
+/* STUBS */
+
+void FatalError(ARG_UNUSED char *s, ...)


Please add a comment explaining why this STUB is here

Added a comment: OpenDB() pulls in libpromises DB code that references FatalError(), whose real implementation lives in the daemons, not the test binary — so the stub is there to let the test link, and reaching it is itself a failure.

~80 of the 100 slots are consumed by default measurements, leaving little room for custom ones. Raise the limit to 300. Kept in sync: CF_OBSERVABLES and the two cf-check copies of the on-disk struct, plus the OBSERVABLES name table (GetObservable() indexes it for unregistered slots, so a short table crashes cf-monitord). cf-check also pads a pre-upgrade ts_key that has fewer entries than CF_OBSERVABLES. Ticket: ENT-6511 Changelog: Increased the maximum number of cf-monitord measurement slots (CF_OBSERVABLES) from 100 to 300

Raising CF_OBSERVABLES enlarges the fixed-size Averages record, so records written by an older agent are shorter than the current struct. Added a migration (modelled on dbm_migration_lastseen.c) that rewrites old records to the current size, zero-filling the new slots, registered for the observations and history DBs. cf-check dump only needs to recognise the new "version" key the migration writes. Ticket: ENT-6511 Changelog: None

larsewi · 2026-07-23T09:00:39Z

    if (structs)
    {
-        if (StringContains(file, "cf_lastseen.lmdb")
+        if ((StringContains(file, "cf_observations.lmdb")


Pre-existing. But, should probably be StringEndsWith()? Could be a follow-up commit.

in this PR or completely separately?

Up to you. I'm OK with leaving it as is as well

I think I will leave it for now. Seems to have been intentionally changed from StringsEndsWith to StringContains 3e98366c3

larsewi · 2026-07-23T09:08:26Z

@cf-bottom Jenkins please :)

cf-bottom · 2026-07-23T09:15:51Z

Sure, I triggered a build:

Jenkins: https://ci.cfengine.com/job/pr-pipeline/14272/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-14272/

nickanderson mentioned this pull request Jul 20, 2026

ENT-6511: Increase cf-monitord measurement slots from 100 to 300 nickanderson/core#20

Closed

nickanderson force-pushed the CFE-612-increase-monitor-slots branch from 50d4f8a to 2fbdc3d Compare July 20, 2026 15:29

nickanderson mentioned this pull request Jul 20, 2026

ENT-14329: Store only in-use measurement slots nickanderson/core#21

Draft

nickanderson marked this pull request as ready for review July 20, 2026 16:27

nickanderson requested a review from larsewi July 20, 2026 16:27

larsewi reviewed Jul 21, 2026

View reviewed changes

Comment thread cf-check/dump.c Outdated

nickanderson force-pushed the CFE-612-increase-monitor-slots branch from 1a91252 to 5f4e187 Compare July 21, 2026 14:41

nickanderson requested a review from larsewi July 21, 2026 15:56

larsewi reviewed Jul 22, 2026

View reviewed changes

nickanderson force-pushed the CFE-612-increase-monitor-slots branch from 5f4e187 to fa550dd Compare July 22, 2026 14:23

nickanderson requested a review from larsewi July 22, 2026 14:29

larsewi reviewed Jul 22, 2026

View reviewed changes

Comment thread cf-check/dump.c Outdated

nickanderson added 2 commits July 22, 2026 10:39

nickanderson force-pushed the CFE-612-increase-monitor-slots branch from fa550dd to d278e7e Compare July 22, 2026 15:39

nickanderson requested a review from larsewi July 22, 2026 15:42

larsewi approved these changes Jul 23, 2026

View reviewed changes

		// Migration version scalar (dbm_migration_observations.c), a short
		// string rather than an Averages struct.

	Copyright 2024 Northern.tech AS
	Copyright 2026 Northern.tech AS

	assert_int_equal(OpenDB(&db, dbid_observations), true);
	assert_true(OpenDB(&db, dbid_observations));

Uh oh!

Conversation

nickanderson commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickanderson commented Jul 20, 2026

Uh oh!

cf-bottom commented Jul 20, 2026

Uh oh!

larsewi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nickanderson commented Jul 21, 2026

Uh oh!

cf-bottom commented Jul 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsewi commented Jul 23, 2026

Uh oh!

cf-bottom commented Jul 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

nickanderson commented Jul 20, 2026 •

edited

Loading