New RadEnv prof-ldap test case

[marc-casavant]

New (RadEnv) prof-ldap test case.

Test functionality verified using the new profiling image as implemented in branch: https://github.com/marc-casavant/freeradius-server/tree/new-profiling-image-build

Includes CI workflow which has not been tested in CI yet.

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[arr2036]

@claude review

[claude]

Claude finished @arr2036's task in 3m 20s —— View job

---

Claude Code Review

• Gather context (read changed files)
• Review CI workflow
• Review multi-server config/test changes
• Review profiling script & Dockerfile
• Post review

---

Overall this is a solid first cut at the prof-ldap profiling suite — the makefile plumbing for prof-* suites (separate TEST_MULTI_SERVER_PROF_INSTANCE, ci/dev result-path modes, image prerequisites) is well-factored, and the valgrind harness is thoughtful about ordering instrumentation toggles around graceful shutdown. The issues below are mostly mechanical and concentrated in the test parameter files and a couple of dangling references.

Blocking / likely-broken

1. 5min.test.yml uses the wrong variable name — test will not render correctly

src/tests/multi-server/tests/prof-ldap/5min.test.yml:17 defines:

test_verify_timeout: 340

but template.yml.j2:40 references {{ test_state1_verify_timeout }}. Jinja will render that as the empty string (or fail with StrictUndefined if configured), so the rendered template gets timeout: with no value. Either rename to test_state1_verify_timeout in the param file, or rename the variable in the template — but the two must agree. short.ci.test.yml already uses test_state1_verify_timeout, so the param-file side is the more consistent fix.

Fix this →

2. short.ci.test.yml defines test_state2_verify_timeout: 30 but there is no state_2

template.yml.j2 only declares state_1. The state_order: sequence field and unused state2 variable suggest the original intent was a two-state test. Either add the second state to the template or drop the dead variable from short.ci.test.yml:18.

3. Missing packet.conf for the proto_load listener

environments/profiling-ldap.yml.j2:33 bind-mounts:

${DATA_PATH}/freeradius/profiling-ldap/common/packet.conf:/etc/freeradius/common/packet.conf

and the rendered radiusd.conf.j2:38 overrides packet_conf_path to /etc/freeradius/common/packet.conf. But this PR does not add configs/freeradius/profiling-ldap/common/packet.conf (or anything under configs/freeradius/common/packet.conf). Other suites ship one at configs/freeradius/load-generator/load-generator-packets/packet.conf. Without this file the volume mount will create an empty file/dir on the host (or the config-builder's --process-volumes step will fail when scanning for the source). Add the missing packet.conf or change the mount to point at an existing source.

4. Files-module mount is mounted but module is never loaded

environments/profiling-ldap.yml.j2:35 mounts mods-config/files/authorize, but radiusd.conf.j2 only loads ldap, pap, and always — no files module. The recv Access-Request section also doesn't reference it. The authorize file (testuser Password.Cleartext := "testpass") is dead — authentication is happening via LDAP. Either drop the mount + file, or load the files module and add files to recv Access-Request if it was meant to be a fallback.

Workflow issues

5. .github/workflows/ci-profiling.yml:12 — paths glob does not match the prof tests

paths:
  - 'src/tests/multi-server/prof-**'

The actual suite directory is src/tests/multi-server/tests/prof-ldap/ — the glob is missing the tests/ segment, so PR pushes that only touch the new suite files won't trigger this workflow. Also, changes to src/tests/multi-server/all.mk, configs/freeradius/profiling-ldap/**, environments/profiling-ldap.yml.j2, scripts/profiling/**, and scripts/docker/build/Dockerfile.multi-server-prof should arguably trigger it too. Suggest:

paths:
  - '.github/workflows/ci-profiling.yml'
  - 'src/tests/multi-server/**'

6. Workflow push: trigger is very broad

branches-ignore with just three branches means every push to every branch on the main repo runs the full profiling build + tests on the self-hosted runner. Given the cost (crossbuild image rebuild, profiling builds), consider scoping to master + tags, or moving to PR-only + scheduled.

7. PR-from-fork interaction

The job condition if: github.repository_owner == 'FreeRADIUS' will gate the workflow correctly for forks, but secrets.DOCKER_REPO_USERNAME and DOCKER_REPO_PASSWORD should still be reviewed for pull_request events (vs. pull_request_target) — by default fork PRs don't have access to secrets, which is fine and means the workflow simply won't run on this PR. Worth confirming this matches the maintainer's intent for "tested in CI".

start_valgrind_profiling.sh

8. PROFILE_DURATION line is broken shell and unused

src/tests/multi-server/scripts/profiling/start_valgrind_profiling.sh:30:

PROFILE_DURATION=$SEND_DURATION-$PROFILE_DURATION_BUFFER

This is a literal string assignment (e.g. 60-15), not arithmetic. It should be PROFILE_DURATION=$(( SEND_DURATION - PROFILE_DURATION_BUFFER )). But the variable is never referenced anywhere after this line — either delete it, or compute it correctly and use it (presumably instead of sleep ${SEND_DURATION} on line 71).

9. SEND_DURATION calc only valid when start_pps == max_pps

Line 28: SEND_DURATION=$(( TEST_LOADGEN_NUM_MESSAGES / TEST_LOADGEN_START_PPS )). The NUM_MESSAGES calc in template.yml.j2:22-24 walks from start_pps to max_pps by step, so this divisor is wrong as soon as the ramp is non-trivial. Today both test files happen to have start_pps == max_pps, so it works — but it's a footgun. Either compute a true duration in the template and pass it in, or sum the per-step durations here.

10. Commented-out block (lines 73-89) should be removed

Dead code committed verbatim — please delete rather than leaving an alternate ordering of the shutdown sequence in the file.

11. Gratuitous eval on lines 128-130

cmd='callgrind_annotate $(find /etc/prof-results -name "callgrind.out.*" -size +0c | sort) > /etc/prof-results/callgrind_report.txt'
echo "$cmd"
eval "$cmd"

If the intent was to log the command before running it, simpler is:

files=$(find /etc/prof-results -name "callgrind.out.*" -size +0c | sort)
echo "callgrind_annotate $files > /etc/prof-results/callgrind_report.txt"
callgrind_annotate $files > /etc/prof-results/callgrind_report.txt

Avoids eval and the unintuitive variable-stored-command pattern. Internal paths only so this isn't a security issue, but eval is worth avoiding by reflex.

12. .profiling_complete marker is referenced in the trap comment but never created

The trap '' SIGTERM comment on line 13 says it protects the script from being killed "before it can touch .profiling_complete", and line 6 removes a stale .profiling_complete — but nothing ever creates it. Either drop the comment + the stale-file removal, or add the touch at the end of the script (presumably what something downstream is polling for).

Smaller items

13. README mentions four prof suites but only prof-ldap ships

README.md lists prof-accept, prof-pap-auth, prof-ldap, prof-mysql and gives prof-mysql as the example invocation, but only prof-ldap is present. Either reword to reflect what's actually here, or note that the others are pending.

14. client localhost { ipaddr = * } (radiusd.conf.j2:89-93)

The shortname says "localhost" but ipaddr = * permits any IP (gated by the listener networks allow-list). Not wrong, just confusing. Either set ipaddr = 127.0.0.1 or rename the shortname.

15. Dockerfile.multi-server-prof — trailing blank lines and no --no-install-recommends-style cleanup

Minor: drop the two trailing blank lines. The flags in the CFLAGS comment are excellent, by the way — that's the kind of comment that earns its keep.

16. build_image.sh lacks set -euo pipefail

If docker build fails or arg parsing goes sideways, the script silently continues with empty state. Add set -euo pipefail near the top.

17. freeradius-prof.image target — duplicated branches

src/tests/multi-server/all.mk:321-334: the FORCE_IMAGE_REBUILD and ! docker images -q $(FREERADIUS_PROF_IMAGE) branches run identical command sequences. Could collapse to one branch with [ -n "$(FORCE_IMAGE_REBUILD)" ] || [ -z "$$(docker images -q $(FREERADIUS_PROF_IMAGE) 2>/dev/null)" ]. Cosmetic.

18. openldap.image target tags openldap:latest unconditionally

all.mk:367 does docker tag $(OPENLDAP_PROF_IMAGE) openldap:latest outside any conditional, so it runs every invocation even when no rebuild was needed. Probably harmless but it would clobber any unrelated openldap:latest already present on the host (e.g. an upstream image a developer pulled separately). Worth either guarding behind the rebuild branch or using a more specific tag in the compose file (freeradius4/openldap-prof:latest directly).

---

Summary

The biggest items to fix before merge:

• (1) param-file/template variable mismatch in 5min.test.yml — actually broken
• (3) missing packet.conf source file — the prof-ldap test will not be runnable as-is
• (5) workflow paths glob doesn't actually match the prof tests
• (8) broken PROFILE_DURATION arithmetic (dead code; remove or fix)
• (10) commented-out shutdown block — delete

The rest are polish: dead mounts (4), missing state_2 (2), README scope (13), shell hygiene (11, 16), workflow scope (6).
• Branch: new-prof-ldap-test

[marc-casavant]

Looking into the conflict.

[arr2036]

Hmm, could you see what parts of this still need to be merged in. The reworked commits are now on master.