DAOS-17322 ddb: DTX aggregation with ddb (Part 2) by knard38 · Pull Request #16746 · daos-stack/daos

knard38 · 2025-08-18T08:05:21Z

Description

Add the following new command to ddb:

dtx_aggr: Aggregate DTX entries

Steps for the author:

Commit message follows the guidelines.
Appropriate Features or Test-tag pragmas were used.
Appropriate Functional Test Stages were run.
At least two positive code reviews including at least one code owner from each category referenced in the PR.
Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

Gatekeeper requested (daos-gatekeeper added as a reviewer).

github-actions · 2025-08-18T08:05:37Z

Ticket title is 'DTX aggregation with ddb'
Status is 'In Review'
Labels: '2.8pp'
https://daosio.atlassian.net/browse/DAOS-17322

daosbuild3 · 2025-09-15T15:12:15Z

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16746/2/testReport/

daosbuild3 · 2025-09-15T15:29:09Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16746/2/testReport/

daosbuild3 · 2025-09-15T15:44:12Z

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16746/2/testReport/

daosbuild3 · 2025-09-15T17:06:35Z

Test stage Unit Test bdev with memcheck on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16746/2/testReport/

daosbuild3 · 2025-09-15T21:31:26Z

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16746/3/testReport/

daosbuild3 · 2025-09-15T21:46:16Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16746/3/testReport/

daosbuild3 · 2025-09-15T22:01:53Z

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16746/3/testReport/

daosbuild3 · 2025-09-15T23:54:19Z

Test stage Unit Test bdev with memcheck on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16746/3/testReport/

* DAOS-17322 ddb: DTX aggregation with ddb This preliminary PR introduce the following modifications: - Cleanup of the SCons file src/utils/ddb/tests/SConscript - Fix miscellaneous issues detected by clangd linter. - Add a new DDB sub command dtx_stat allowing to print statistics on DTX entries. - This PR will be followed by the PR #16746 introducing a new DDB sub command dtx_aggr allowing to aggregate DTX entries with DDB. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

Fix clang format issues. Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

daosbuild3 · 2025-10-15T20:34:42Z

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16746/23/testReport/

daosbuild3 · 2025-10-15T21:04:33Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16746/23/testReport/

daosbuild3 · 2025-10-15T21:04:48Z

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16746/23/testReport/

Fix unit test. Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

Fix function argument name. Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

Fix function argument comment. Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

Nasf-Fan

Thanks @knard38 for the update. Looks more clean than former ones. Generally, it is fine, some trivial suggestions.

Nasf-Fan · 2025-10-17T03:27:04Z

- * \return		Zero on success, negative value if error.
+ * \return			Negative value if error, zero if all DTX entries with an epoch lower
+ * 				or equal to ep_max have been aggregated, one if all the targeted DTX
+ * 				entries may not have been aggregated.


I would suggest:

Zero if maybe more DTX entries can be aggregated. Positive value if all qualified DTX entries have been removed. Negative value for error.

Then only dtx aggregation via ddb needs to check the postive return value, all the other existing vos_dtx_aggregate() callers do not change return value checking logic.

I would suggest:

Zero if maybe more DTX entries can be aggregated. Positive value if all qualified DTX entries have been removed. Negative value for error.

Then only dtx aggregation via ddb needs to check the postive return value, all the other existing vos_dtx_aggregate() callers do not change return value checking logic.

Fix comment

I would suggest:

Zero if maybe more DTX entries can be aggregated. Positive value if all qualified DTX entries have been removed. Negative value for error.

Then only dtx aggregation via ddb needs to check the postive return value, all the other existing vos_dtx_aggregate() callers do not change return value checking logic.

Fix comment

Fixed with commit c6ee641

Nasf-Fan · 2025-10-17T03:28:44Z

 /* Parse a string representing a date into an epoch */
 int
-ddb_date2epoch(const char *date, uint64_t *epoch);
+ddb_date2cmt_time(const char *date, uint64_t *cmt_time);


Please refresh the comment to match the changes.

Please refresh the comment to match the changes.

Fix comment

Please refresh the comment to match the changes.

Fix comment

Fixed with commit c6ee641

Nasf-Fan · 2025-10-17T03:30:15Z

+	ddb_printf(ctx, "\t- %s%s", prefix, align);
+
+	rc = 0;
+	for (i = 0; i < 3; i++) {


It will be more safe to pass the length of epoch_stats as parameter instead of hard coded as "3".

It will be more safe to pass the length of epoch_stats as parameter instead of hard coded as "3".

Use constant instead of hard coded value

It will be more safe to pass the length of epoch_stats as parameter instead of hard coded as "3".

Use constant instead of hard coded value

Fixed with commit c6ee641

Nasf-Fan · 2025-10-17T03:30:19Z

+	ddb_printf(ctx, "\t- %s%s", prefix, align);
+
+	rc = 0;
+	for (i = 0; i < 3; i++) {


Similar as above.

Similar as above.

Use constant instead of hard coded value

Similar as above.

Use constant instead of hard coded value

Fixed with commit c6ee641

Nasf-Fan · 2025-10-17T03:34:08Z

 		goto done;
-	args->cmt_cnt += cnt;
+
+	args->cmt_cnt += args_tmp.cmt_cnt;


For details case, move it after calculating the mean value for time/epoch?

Arghhhh, too quick copy/paste from my canceled PR https://github.com/daos-stack/daos/pull/16954/files#diff-c5fc3d2ed692ce1cc27be1a6d725d458e14d22d35374f71e40dd8921b6365698R1442
Thanks a lot to have found this issue, which also some hole into the unit tests.

Fix invalid mean calculation.

~~Improve unit test of the ddb_run_dtx_stat() function.~~

Adding new unit tests to have full control over the behaviour of ddb_run_dtx_stat(), makes the testing code over complicated for a non critical feature. Thus, I will not add new unit tests accurately checking the mean computation and other DTX commit statistics.
Is it OK for you @Nasf-Fan ?

Arghhhh, too quick copy/paste from my canceled PR https://github.com/daos-stack/daos/pull/16954/files#diff-c5fc3d2ed692ce1cc27be1a6d725d458e14d22d35374f71e40dd8921b6365698R1442 Thanks a lot to have found this issue, which also some hole into the unit tests.

Fix invalid mean calculation.

~~Improve unit test of the ddb_run_dtx_stat() function.~~

Fixed with commit c6ee641

Adding new unit tests to have full control over the behaviour of ddb_run_dtx_stat(), makes the testing code over complicated for a non critical feature. Thus, I will not add new unit tests accurately checking the mean computation and other DTX commit statistics. Is it OK for you @Nasf-Fan ?

That's fine for me.

Nasf-Fan · 2025-10-17T03:35:09Z

+		ddb_error(ctx, "Failed to close container ");
+		itp_print_full(ctx, itp);
+		ddb_errorf(ctx, ": " DF_RC "\n", DP_RC(rc));
+		return rc;


This return is unnecessary since there is another following return.

This return is unnecessary since there is another following return.

Remove useless return

This return is unnecessary since there is another following return.

Remove useless return

Fixed with commit c6ee641

…/daos-17322-part2

Fix reviewers comments: - Fix comments - Use constant instead of hard coded value - Fix invalid mean calculation - Remove useless return Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

…/daos-17322-part2

Fix invalid last epoch aggregation computation. Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

Fix minor coding sytle. Features: aggregation Signed-off-by: Cedri,c Koch-Hofer <cedric.koch-hofer@hpe.com>

knard38 · 2025-10-17T17:37:51Z

@kjacque , please could you have a look at the go code part of the code.
Thanks in advance for your attention.

Fix clang format issues. Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

daosbuild3 · 2025-10-19T01:40:44Z

Test stage Functional Hardware Medium MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16746/30/execution/node/1291/log

daosbuild3 · 2025-10-19T02:31:21Z

Test stage Functional Hardware Medium Verbs Provider MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16746/30/execution/node/1336/log

daosbuild3 · 2025-10-19T14:27:29Z

Test stage Functional Hardware Large MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16746/30/execution/node/1475/log

johannlombardi · 2025-10-20T14:27:16Z

@knard38 could you please check whether any of the failures are related to your changes?

knard38 · 2025-10-21T08:17:47Z

@knard38 could you please check whether any of the failures are related to your changes?

The CI failures are known CI issues:

test_ec_online_rebuild_mdtest - test timeout waiting for rebuild: ticket DAOS-17751
pool_destroy_race.py:RbldPoolDestroyWithIO.test_pool_destroy_with_io - IOR failed: ticket DAOS-18031
suite.py:DaosCoreTest.test_daos_rebuild_ec - At least one multi-variant server was not found in its expected state: ticket DAOS-17091
From the ticket DAOS-17091, the remaining cmocka tests failure are a consequence of the previous failure.

I have restarted a run of the functional test https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos/job/PR-16746/31/pipeline-overview/

knard38 · 2025-10-21T14:39:30Z

@knard38 could you please check whether any of the failures are related to your changes?

The CI failures are known CI issues:

test_ec_online_rebuild_mdtest - test timeout waiting for rebuild: ticket DAOS-17751

pool_destroy_race.py:RbldPoolDestroyWithIO.test_pool_destroy_with_io - IOR failed: ticket DAOS-18031

suite.py:DaosCoreTest.test_daos_rebuild_ec - At least one multi-variant server was not found in its expected state: ticket DAOS-17091

From the ticket DAOS-17091, the remaining cmocka tests failure are a consequence of the previous failure.

I have restarted a run of the functional test https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos/job/PR-16746/31/pipeline-overview/

@johannlombardi , all the functional test have been passed with the new run confirming the hypothesis that the CI failures of the build 30 are not related to this PR.

The DCO failure is a mistake from my side on an old commit:I prefer to not amend this commit to not touch the commit history.

kjacque · 2025-10-22T17:03:48Z

+			a.String("path", "Optional, VOS tree path of a container to aggregate.", grumble.Default(""))
+		},
+		Flags: func(f *grumble.Flags) {
+			f.Uint64("t", "cmt_time", math.MaxUint64, "Max aggregfation committed time in seconds")


minor, typo

Suggested change

f.Uint64("t", "cmt_time", math.MaxUint64, "Max aggregfation committed time in seconds")

f.Uint64("t", "cmt_time", math.MaxUint64, "Max aggregation committed time in seconds")

minor, typo

I am going to fix it in a follow-up PR

minor, typo

Fixed with PR #17022

kjacque · 2025-10-22T17:06:01Z

 func (cmdStr ddbCmdStr) Complete(match string) (comps []flags.Completion) {
 	// hack to get at command names
-	ctx, cleanup, err := InitDdb()
+	ctx, cleanup, err := InitDdb(nil)


Is it OK to pass nil here?

Yep, the associated complete function are indeed never using the log attributes.
I agree, that it is not very nice but it is the only way that I found to pass the log object for the standard call of InitDB().

gnailzenh · 2025-10-23T04:04:37Z

-			D_ERROR("Failed to update tail for DTX aggregation "
-				UMOFF_PF": "DF_RC"\n",
+		rc = umem_tx_add_ptr(umm, &dbd->dbd_committed_data[0],
+				     sizeof(dbd->dbd_committed_data[0]) * dbd->dbd_count);


Not a big deal, but it's better to change it to "sizeof(...) * (dbd->dbd_count - dtx_aggr_count)" right?

As illustrated by the following figure, the value is exact as we are translating some memory on all this memory area.

gnailzenh · 2025-10-23T04:09:21Z

 vos_oi_exist(daos_handle_t coh, daos_unit_oid_t oid);

+/* Timing statistic of DTX entries */
+#define DTX_TIME_STAT_COUNT 3


style: I think the clearer style would be like:

enum { DTX_TIME_STAT_MIN, DTX_TIME_STAT_MAX, DTX_TIME_STAT_MEAN, DTX_TIME_STAT_COUNT };

so you can use them as alias of the array index instead of using 0, 1, 2

I am going to fix it in a following-up PR

Fixed with PR #17022

knard38 mentioned this pull request Aug 19, 2025

DAOS-17322 ddb: DTX aggregation with ddb (Part 1) #16745

Merged

6 tasks done

knard38 self-assigned this Aug 19, 2025

knard38 force-pushed the ckochhof/dev/master/daos-17322-part2 branch from 52ac348 to 65436bd Compare September 15, 2025 14:33

knard38 force-pushed the ckochhof/dev/master/daos-17322-part2 branch from 65436bd to fbff07e Compare September 15, 2025 20:46

knard38 force-pushed the ckochhof/dev/master/daos-17322-part2 branch 2 times, most recently from a70b32a to 79e20a2 Compare September 22, 2025 08:36

Base automatically changed from ckochhof/dev/master/daos-17322-part1 to master September 26, 2025 02:29

knard38 force-pushed the ckochhof/dev/master/daos-17322-part2 branch from 79e20a2 to c6ff645 Compare September 26, 2025 06:15

knard38 marked this pull request as ready for review September 26, 2025 06:16

knard38 requested review from a team September 26, 2025 06:16

knard38 requested review from a team as code owners September 26, 2025 06:16

knard38 marked this pull request as draft September 26, 2025 06:17

knard38 force-pushed the ckochhof/dev/master/daos-17322-part2 branch from c6ff645 to d99caab Compare September 26, 2025 06:24

knard38 marked this pull request as ready for review September 26, 2025 06:25

knard38 requested review from gnailzenh and kjacque September 26, 2025 06:26

DAOS-17322 ddb: DTX aggregation with ddb

9ed9532

Fix clang format issues. Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

knard38 requested a review from Nasf-Fan October 15, 2025 19:39

kanard38 added 3 commits October 16, 2025 06:31

DAOS-17322 ddb: DTX aggregation with ddb

a7bbda1

Fix unit test. Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

DAOS-17322 ddb: DTX aggregation with ddb

2a604ef

Fix function argument name. Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

DAOS-17322 ddb: DTX aggregation with ddb

74a85bb

Fix function argument comment. Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

Nasf-Fan reviewed Oct 17, 2025

View reviewed changes

kanard38 added 5 commits October 17, 2025 06:49

Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…

9dfa864

…/daos-17322-part2

DAOS-17322 ddb: DTX aggregation with ddb

c6ee641

Fix reviewers comments: - Fix comments - Use constant instead of hard coded value - Fix invalid mean calculation - Remove useless return Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…

92d784d

…/daos-17322-part2

DAOS-17322 ddb: DTX aggregation with ddb

12c8084

Fix invalid last epoch aggregation computation. Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

DAOS-17322 ddb: DTX aggregation with ddb

643bef1

Fix minor coding sytle. Features: aggregation Signed-off-by: Cedri,c Koch-Hofer <cedric.koch-hofer@hpe.com>

knard38 requested a review from Nasf-Fan October 17, 2025 17:35

DAOS-17322 ddb: DTX aggregation with ddb

ec9038c

Fix clang format issues. Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

Nasf-Fan approved these changes Oct 18, 2025

View reviewed changes

knard38 added the CR Catastrophic Recovery Feature label Oct 21, 2025

kjacque reviewed Oct 22, 2025

View reviewed changes

gnailzenh approved these changes Oct 23, 2025

View reviewed changes

gnailzenh merged commit e68a72f into master Oct 23, 2025
40 of 41 checks passed

gnailzenh deleted the ckochhof/dev/master/daos-17322-part2 branch October 23, 2025 04:11

knard38 mentioned this pull request Oct 23, 2025

DAOS-17322 ddb: DTX aggregation with ddb (part 3) #17022

Merged

5 of 6 tasks

	f.Uint64("t", "cmt_time", math.MaxUint64, "Max aggregfation committed time in seconds")
	f.Uint64("t", "cmt_time", math.MaxUint64, "Max aggregation committed time in seconds")

Conversation

knard38 commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Steps for the author:

After all prior steps are complete:

Uh oh!

github-actions Bot commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daosbuild3 commented Sep 15, 2025

Uh oh!

daosbuild3 commented Sep 15, 2025

Uh oh!

daosbuild3 commented Sep 15, 2025

Uh oh!

daosbuild3 commented Sep 15, 2025

Uh oh!

daosbuild3 commented Sep 15, 2025

Uh oh!

daosbuild3 commented Sep 15, 2025

Uh oh!

daosbuild3 commented Sep 15, 2025

Uh oh!

daosbuild3 commented Sep 15, 2025

Uh oh!

daosbuild3 commented Oct 15, 2025

Uh oh!

daosbuild3 commented Oct 15, 2025

Uh oh!

daosbuild3 commented Oct 15, 2025

Uh oh!

Nasf-Fan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knard38 Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

knard38 Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

knard38 Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

knard38 Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

knard38 Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knard38 Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

knard38 commented Aug 18, 2025 •

edited

Loading

github-actions Bot commented Aug 18, 2025 •

edited

Loading

knard38 Oct 17, 2025 •

edited

Loading

knard38 Oct 17, 2025 •

edited

Loading

knard38 Oct 17, 2025 •

edited

Loading

knard38 Oct 17, 2025 •

edited

Loading

knard38 Oct 17, 2025 •

edited

Loading

knard38 Oct 17, 2025 •

edited

Loading

knard38 Oct 17, 2025 •

edited

Loading

knard38 commented Oct 21, 2025 •

edited

Loading