Fix/fdb 580 fdb hammer encapsulate barrier #194

nicolau-manubens · 2025-11-11T09:23:17Z

Description

Encapsulates the barrier code in fdb-hammer in a new class. The new code has been verified to work with an fdb-benchmark run in our HPC, mimicking 1 member on 3 writer nodes and 3 "pgen" jobs on 2 reader nodes each. Write ppn was set to 16 and read ppn was set to 32.

Contributor Declaration

By opening this pull request, I affirm the following:

All authors agree to the Contributor License Agreement.
The code follows the project's coding standards.
I have performed self-review and added comments where needed.
I have added or updated tests to verify that my changes are effective and functional.
I have run all existing tests and confirmed they pass.

🌈🌦️📖🚧 Documentation Z3FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/z3fdb/pull-requests/PR-194

🌈🌦️📖🚧 Documentation FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/fdb/pull-requests/PR-194

codecov-commenter · 2025-11-11T09:29:35Z

Codecov Report

❌ Patch coverage is 0% with 148 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.45%. Comparing base (24b9d9d) to head (d33d077).

Files with missing lines	Patch %	Lines
src/fdb5/tools/fdb-hammer.cc	0.00%	148 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #194      +/-   ##
===========================================
- Coverage    72.46%   72.45%   -0.01%     
===========================================
  Files          360      360              
  Lines        21693    21707      +14     
  Branches      2240     2245       +5     
===========================================
+ Hits         15719    15728       +9     
- Misses        5974     5979       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

simondsmart · 2025-11-11T09:37:55Z

src/fdb5/tools/fdb-hammer.cc

+    max_wait_(max_wait),
+    server_connections_(nodes_.size() ? (nodes_.size() - 1) : 0) {
+
+    hostname_ = Main::hostname();


Is there a reason this is done in the function body, not as an initialiser?

simondsmart · 2025-11-11T09:38:54Z

src/fdb5/tools/fdb-hammer.cc

+    eckit::PathName default_run_path{"/var/run/user"};
+    default_run_path /= uid_to_str(uid);
+
+    eckit::PathName run_path(eckit::Resource<std::string>("$FDB_HAMMER_RUN_PATH", default_run_path));


This logic feels like it should belong in the config object. And then the Barrier object probably should accept a config object as an argument.

At least, the determination of the run directory should. The specific files used matter less.

I see the barrier code as potentially useful for other purposes than fdb-hammer. If so, it'd be better not to make it depend on fdb-hammer-specific configuration. What about adding a "run_path" argument in the Barrier constuctor, with default value "/var/run/user", which the fdb-hammer code can override with FDB_HAMMER_RUN_PATH?

For now I've renamed FDB_HAMMER_RUN_PATH to FDB_BARRIER_RUN_PATH, and renamed all fdb-hammer.* files (fifo and lock fiels) to barrier.*

simondsmart · 2025-11-11T09:43:06Z

src/fdb5/tools/fdb-hammer.cc

+            }
+        }
+
+        leader_found = true;


This is a bit odd that by default the loop doesn't loop. Unless we hit a continue somewhere.

Hopefully improved by removing the continue and setting leader_found = true where relevant.

The loop was removed altogether while resolving the race condition in leftover PID file handling.

simondsmart · 2025-11-11T09:44:02Z

src/fdb5/tools/fdb-hammer.cc

+
+            /// the leadr PID is checked to exist. If not, clean up and
+            /// restart election procedure
+            if (::kill(leader_pid, 0) != 0) {


How can this happen? Surely if the leader PID has died then the whole run is a failure as we no longer have the correct number of nodes?

Or is this code that allows a restart of the tests in the same directory

This is to handle the case where barriering processes are abruply terminated without a chance to clean up the PID file.

simondsmart · 2025-11-11T09:46:41Z

src/fdb5/tools/fdb-hammer.cc

+            /// restart election procedure
+            if (::kill(leader_pid, 0) != 0) {
+                try {
+                    pid_file_.unlink();


This feels like there is a race condition.

The leader pid doesn't exist, so you prepare to unlink the file. But in the mean time another process has removed the file, and yet another process has become the leader and recreated it.

Thanks, now addressed with a file lock as you've suggested elsewhere.

simondsmart · 2025-11-11T09:50:44Z

src/fdb5/tools/fdb-hammer.cc

+    }
+}
+
+void Barrier::init() {


Is there a reason that this functionality is not called either (a) from the constructor, or (b) lazily from the barrier() function.

Generally best to avoid having an init_ member unless strictly necessary. And the only reason I can see that being necessary is to initialise lazily.

It was not called from the constructor to avoid establishign TCP connections and so on as part of the constructor. There was no reason though for this to not be called lazily from barrier(). Now made private and called lazily.

simondsmart · 2025-11-11T09:51:38Z

src/fdb5/tools/fdb-hammer.cc

+    init_ = true;
+}
+
+void Barrier::fini() {


fini() is not a sensible (English) name for the function.

I don't see why these methods shouldn't be in a destructor, to make the object inherently safe if the surrounding code calsl exceptions.

Renamed to finalise. This was not put in a destructor as it requires file removal (and potentially throwing) operations.

simondsmart · 2025-11-11T09:55:28Z

src/fdb5/tools/fdb-hammer.cc

+    if (is_leader_process_) {
+
+        /// a signal from each peer process in the node is received
+        std::vector<char> message = {'S', 'I', 'G'};


Message doesn't need dynamic size, and allocation on the heap. Should be a std::array<char, 3>

simondsmart · 2025-11-11T09:56:47Z

src/fdb5/tools/fdb-hammer.cc

-        //        Barrier.block();
-        barrier(config_.parallel.ppn, config_.parallel.nodelist, config_.parallel.barrierPort,
-                config_.parallel.barrierMaxWait);
+        Barrier barrier{(size_t)config_.parallel.ppn, config_.parallel.nodelist, config_.parallel.barrierPort,


This code should be:

Barrier(config_).barrier();

Not sure that the init/fini functions add any value.

As discussed above, I'd keep the Barrier class independent from fdb-hammer specifics and finalise() seemed necessary.

simondsmart · 2025-11-11T09:57:27Z

src/fdb5/tools/fdb-hammer.cc

@@ -1188,9 +1269,11 @@ void FDBHammer::executeWrite() {
    if (config_.execution.itt) {
        eckit::Timer barrier_timer;


eckit::Timer can be inside the Barrier object.

nicolau-manubens added 4 commits November 10, 2025 12:16

Encapsulated fdb-hammer barrier code in a class. FDB-580.

0d9a112

fdb-hammer fix for barrier.

16236d6

Fixes and improvements in fdb-hammer barrier encapsulation.

e3424ee

Code-formatting fdb-hammer barrier.

db26f03

nicolau-manubens requested review from danovaro and simondsmart November 11, 2025 09:23

simondsmart requested changes Nov 11, 2025

View reviewed changes

nicolau-manubens added 4 commits November 12, 2025 09:25

Fix race condition in fdb-hammer barrier leader election.

38321c1

Address comments in PR-194.

c50f218

Fix for fdb-hammer barrier leader election.

3e6e12a

Rename barrier files and run path env variable.

81499af

nicolau-manubens requested a review from simondsmart November 12, 2025 14:58

Fixes in fdb-hammer barrier.

d33d077

		@@ -1188,9 +1269,11 @@ void FDBHammer::executeWrite() {
		if (config_.execution.itt) {
		eckit::Timer barrier_timer;

Fix/fdb 580 fdb hammer encapsulate barrier #194

Are you sure you want to change the base?

Fix/fdb 580 fdb hammer encapsulate barrier #194

Conversation

nicolau-manubens commented Nov 11, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Contributor Declaration

Uh oh!

codecov-commenter commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolau-manubens Nov 11, 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!

simondsmart Nov 11, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nicolau-manubens commented Nov 11, 2025 •

edited by github-actions bot

Loading

codecov-commenter commented Nov 11, 2025 •

edited

Loading

nicolau-manubens Nov 11, 2025 •

edited

Loading

simondsmart Nov 11, 2025 •

edited

Loading