ndmp-bareos ndmp-native improvements and fixes#2384
Draft
bruno-at-bareos wants to merge 46 commits intobareos:masterfrom
Draft
ndmp-bareos ndmp-native improvements and fixes#2384bruno-at-bareos wants to merge 46 commits intobareos:masterfrom
bruno-at-bareos wants to merge 46 commits intobareos:masterfrom
Conversation
8631106 to
7c223bf
Compare
4c65568 to
7c223bf
Compare
Contributor
Author
|
nobuild setup until I rework the concurrency of testing |
7c223bf to
d54f88d
Compare
9 tasks
d54f88d to
ac4da8f
Compare
231d074 to
440f0b8
Compare
d734746 to
dcde3f3
Compare
3bda1ce to
8eb71b9
Compare
Contributor
📝 WalkthroughWalkthroughAdds NDMP test support and templates, propagates backup format during job reschedule, tightens NDMP header parsing, extends systemtest scripts and environment for NDMP, replaces/creates many NDMP-native and NDMP-bareos test runners and configs, and removes several autochanger/block-size test scripts. CMake now optionally loads an NDMP config. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
systemtests/scripts/invalidate_tapes.sh (1)
68-93:⚠️ Potential issue | 🟠 Major
exit 1inside piped subshell won't terminate the main script.The
whileloop starting at Line 69 runs inside a subshell created by the pipe (mtx ... | { while ... }). Theexit 1on Line 81 will only exit the subshell, not the main script. The function will continue executing after the subshell completes, potentially masking the failure.🐛 Suggested fix using process substitution
- mtx -f "${changer_device}" status | { - while read -r line && [ "${i}" -le "$LAST_SLOT_NUMBER" ]; do + while read -r line && [ "${i}" -le "$LAST_SLOT_NUMBER" ]; do if echo "${line}" | grep "$(printf 'Storage Element %d:Full\n' ${i})"; then set -x if mtx -f "${changer_device}" load "${i}" "${USE_TAPE_DEVICE}" \ && mt -f "${tape_device}" rewind \ && mt -f "${tape_device}" weof \ && mtx -f "${changer_device}" unload "${i}" "${USE_TAPE_DEVICE}"; then set +x echo ((i = i + 1)) else echo "error $?" exit 1 fi fi - done + done < <(mtx -f "${changer_device}" status) - slots_ready=$((i - 1)) + slots_ready=$((i - 1)) - if [ ${slots_ready} -eq 0 ]; then - echo "Could not invalidate any tape" - else - echo "Invalidated ${slots_ready} tapes of autochanger ${changer_device}." - fi - } + if [ ${slots_ready} -eq 0 ]; then + echo "Could not invalidate any tape" + else + echo "Invalidated ${slots_ready} tapes of autochanger ${changer_device}." + fisystemtests/scripts/functions (1)
509-515:⚠️ Potential issue | 🟠 MajorFix AWK program quoting; current command treats pattern as filename instead of program.
The current code splits the AWK program across separate quoted strings. AWK interprets the second string as a file to read, resulting in empty output instead of the expected job ID. Combine into a single program and pass the jobid with
-v.🐛 Proposed fix
- RET="$("${AWK}" -F: "BEGIN { jobid=$JOBID } "'/Prev Backup JobId/ { cjid=$2 } /New Backup JobId/ { if (cjid == jobid) { print $2 } }' "$LOG")" + RET="$("${AWK}" -F: -v jobid="${JOBID}" '/Prev Backup JobId/ { cjid=$2 } /New Backup JobId/ { if (cjid == jobid) { print $2 } }' "$LOG")"
🤖 Fix all issues with AI agents
In `@CTestNDMPConfig.cmake.template`:
- Line 36: The ndmp_tape_and_robot_agent_tape_device string contains a typo
(missing slash) for the third device; update the value set in
ndmp_tape_and_robot_agent_tape_device so the third entry reads "/dev/rmt/12n"
instead of "/dev/rmt12n" (ensure entries remain semicolon-separated, e.g. in the
set(ndmp_tape_and_robot_agent_tape_device ...) line).
In `@systemtests/tests/ndmp-bareos/test-cleanup`:
- Line 35: The script calls cleanup_remote but that function is not defined in
the sourced helper scripts; fix by either adding a definition for cleanup_remote
(e.g., implement a function named cleanup_remote in one of the helpers such as
systemtests/scripts/functions or systemtests/scripts/cleanup) or replace the
call with the correct existing function name (for example use cleanup or
cleanup_remote_host if that’s the intended helper). Ensure the chosen function
handles the same cleanup steps (remote host cleanup, file removal, service stop)
and export or source the file so the test script can invoke the symbol
cleanup_remote successfully.
In `@systemtests/tests/ndmp-bareos/testrunner-00-full-fail-rerun`:
- Around line 99-101: The unconditional cancel of the rerun job (the lines that
echo "cancel jobid=2 yes" into "${tmp}/bconcmds" and call run_bconsole)
undermines the test; make the cancel optional by adding a guard around that
echo+run_bconsole sequence (e.g., a conditional based on an environment variable
or a flag like SKIP_CANCEL or CANCEL_RERUN) so the default behavior lets the
rerun proceed and the cancel only occurs when explicitly requested; update the
logic that writes to "${tmp}/bconcmds" and the invocation of run_bconsole to be
executed only when the guard is true.
In `@systemtests/tests/ndmp-bareos/testrunner-05-restore-full`:
- Line 48: The restore command string 'restore jobid=1 client=ndmp-fd
fileset=${NDMP_CLIENT}-fileset restorejob=NDMPRestoreDump all done yes"'
contains a stray trailing double-quote; remove the unmatched '"' at the end so
the command becomes properly terminated and bconsole can parse it (locate and
edit the restore command in testrunner-05-restore-full).
In `@systemtests/tests/ndmp-bareos/testrunner-06-restore-full-relative`:
- Around line 56-58: The grep invocation in the command string that pipes
tail/xargs to grep (${SSH} "tail -c 200
\"${NDMP_TEST_PATH}/${restore_path}/full\" | xargs | grep
${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}") should use fixed-string matching to
avoid '.' being treated as a regex; change that grep to use fixed-strings (e.g.,
grep -F or --fixed-strings) so the IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT is
matched literally.
In `@systemtests/tests/ndmp-bareos/testrunner-07-restore-full-absolute`:
- Around line 41-48: The restore_path is built incorrectly by prefixing
${NDMP_MOUNTPOINT}/data/ to ${NDMP_TEST_PATH} (which is already an absolute
path); update the assignment of restore_path to use NDMP_TEST_PATH directly so
it becomes
restore_path="${NDMP_TEST_PATH}/bareos-restores-${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}"
(locate the restore_path variable in the testrunner-07-restore-full-absolute
script and remove the extra ${NDMP_MOUNTPOINT}/data/ prefix).
In `@systemtests/tests/ndmp-bareos/testrunner-10-copy`:
- Around line 59-85: The SQL queries that populate copy_full_jobid and
copy_inc_jobid can return empty strings and should fail fast; after assigning
copy_full_jobid and again after assigning copy_inc_jobid add guard clauses that
test for empty values (e.g., test -z "$copy_full_jobid") and on failure print a
clear error to stderr (including which variable/query failed) and exit with
non‑zero status so subsequent calls to run_bconsole, restore jobid=...,
check_log, etc. are not executed with an empty job id. Ensure both guards
reference the exact variable names copy_full_jobid and copy_inc_jobid and abort
before any restore/cleanup steps.
In `@systemtests/tests/ndmp-bareos/testrunner-11-compare-with-copy`:
- Around line 58-61: The script currently uses set -e so a non-zero diff exit
will abort before capturing the status; change the diff invocation to prevent
the non-zero exit from propagating and still record its exit code, e.g. replace
the line with the diff command so it reads: diff -u
"${tmp}/bscan_FileStorage.out" "${tmp}/bscan_CopyStorage.out" >
"${tmp}/diff.out" || dstat=$? (this ensures dstat captures diff's exit code and
allows end_test to run), keeping the existing dstat variable and end_test call.
In `@systemtests/tests/ndmp-native/CMakeLists.txt`:
- Around line 66-71: The REGEX REPLACE that extracts
IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT from ROUTE_INFO_TO_ACCESS_NDMP_DATA_AGENT
can yield empty/invalid values on different platforms; after the existing
string(REGEX REPLACE ".* src ([^ ]+).*" \\1 IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT
${ROUTE_INFO_TO_ACCESS_NDMP_DATA_AGENT}) add validation using string(REGEX MATCH
...) to ensure IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT matches a valid IPv4 (or
IPv6) pattern and reset it (set to empty or fallback) if it does not; this
ensures IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT is only used when it contains a
syntactically valid address and prevents silent failures.
In
`@systemtests/tests/ndmp-native/etc/bareos/bareos-dir.d/jobdefs/DefaultNDMPRestoreJob.conf`:
- Around line 4-11: The DefaultNDMPRestoreJob jobdef hardcodes FileSet =
"isilon-fileset" (JobDefs -> Name = "DefaultNDMPRestoreJob") which will break
omnios tests that expect "omnios-fileset"; either make the jobdef variant-aware
or add a separate jobdef for the omnios variant: update DefaultNDMPRestoreJob to
reference the correct fileset based on the test variant (or create Name =
"DefaultNDMPRestoreJob-omnios" with FileSet = "omnios-fileset"), or if
omnios-fileset is unused remove that dead configuration; ensure references to
"isilon-fileset" and "omnios-fileset" are reconciled so each variant uses the
matching FileSet.
In `@systemtests/tests/ndmp-native/testrunner-05-restore-full`:
- Around line 46-49: The heredoc written to "${tmp}/bconcmds" contains a stray
double-quote at the end of the restore command which will break bconsole
parsing; edit the heredoc (the block created by cat <<END_OF_DATA) and remove
the trailing double-quote from the line beginning with restore jobid=1 (the
restore command referencing ${NDMP_CLIENT}-fileset and
restorejob=NDMPRestoreDump) so the line ends with yes instead of yes".
In `@systemtests/tests/ndmp-native/testrunner-07-restore-full-absolute`:
- Around line 44-61: The restore_path is constructed incorrectly by prefixing
"${NDMP_MOUNTPOINT}/data/" before ${NDMP_TEST_PATH}, duplicating the mountpoint;
change the assignment of restore_path to use
"${NDMP_TEST_PATH}/bareos-restores-${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}"
(remove the leading ${NDMP_MOUNTPOINT}/data/), then update the analogous cleanup
variable uses that follow the same pattern in the NDMP cleanup code (the entries
that similarly build paths from NDMP_TEST_PATH) so both restore and cleanup
target the same correct directory.
🧹 Nitpick comments (18)
core/src/dird/jobq.cc (1)
676-679: Free existingbackup_formatbefore overwrite to avoid a leak.
SetJcrDefaultscan already setnjcr->dir_impl->backup_format, so assigning a newstrdupwithout freeing risks a small but repeatable leak on reschedule.♻️ Proposed fix
- if (jcr->dir_impl->backup_format) { - njcr->dir_impl->backup_format = strdup(jcr->dir_impl->backup_format); - } + if (jcr->dir_impl->backup_format) { + if (njcr->dir_impl->backup_format) { + free(njcr->dir_impl->backup_format); + } + njcr->dir_impl->backup_format = strdup(jcr->dir_impl->backup_format); + }systemtests/tests/ndmp-bareos/testrunner-11-compare-with-copy (2)
46-48: Consider validatingcopy_volumeis non-empty.If
list volumes pool=ndmp-copyreturns no volumes,copy_volumewill be empty, causingrun_bscan_dbto receive an empty--volumes ""argument which may fail silently or produce confusing errors.🛡️ Proposed validation
copy_volume=$(bin/bconsole <<< ".api 2 list volumes pool=ndmp-copy " | awk '/"volumename":/ {gsub("\"","",$2);gsub(",","",$2);print $2}' | head -n1) + +if [[ -z "${copy_volume}" ]]; then + echo "ERROR: No copy volume found in pool ndmp-copy" + exit 1 +fi
59-61:dstatis captured but never used.The variable
dstatcaptures the diff exit status but is never checked or reported. If the intent is to fail the test when differences exist, the exit status should be propagated. If differences are expected (per the TODO on lines 41-44), consider logging the result.💡 Suggested usage
If differences should fail the test:
+if [[ ${dstat} -ne 0 ]]; then + echo "WARNING: bscan outputs differ between source and copy (see ${tmp}/diff.out)" + # Uncomment to fail: exit ${dstat} +fi + end_testsystemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/pool/ndmp-copy.conf (2)
6-6: VerifyMaximumVolumeBytesformat:10 mvs10M.The value
10 mhas a space between the number and unit. Other pool configs in this PR use10Mwithout a space (e.g.,ndmp.confLine 9). Verify this is intentional or standardize the format.Suggested fix for consistency
- MaximumVolumeBytes = 10 m + MaximumVolumeBytes = 10M
9-9: Minor: Trailing whitespace on Line 9.There appears to be trailing whitespace after
LocalCopyStorage.Suggested fix
- Storage = LocalCopyStorage + Storage = LocalCopyStorageCMakeLists.txt (1)
205-209: Consider validatingndmp_configpath for clearer failures.
A missing or mistyped path currently fails at include-time without a tailored message.💡 Optional improvement
-if(ndmp_config) - message(INFO "\n\"ndmp_config:\" loaded ${ndmp_config}") - include("${ndmp_config}") -endif() +if(ndmp_config) + if(EXISTS "${ndmp_config}") + message(STATUS "\n\"ndmp_config:\" loaded ${ndmp_config}") + include("${ndmp_config}") + else() + message(FATAL_ERROR "ndmp_config file not found: ${ndmp_config}") + endif() +endif()systemtests/scripts/create_autochanger_configs.sh.in (2)
99-102: Inconsistent indentation in generated Device block.Line 99 uses a tab character while surrounding lines use spaces, and Line 102 has a leading space before the closing brace. Since this is a heredoc that generates configuration files, the inconsistent whitespace will be written directly to the output.
🔧 Suggested fix for consistent indentation
- Drive Tape Alert Enabled = yes - Drive Crypto Enabled = Yes - Query Crypto Status = Yes - } + Drive Tape Alert Enabled = yes + Drive Crypto Enabled = Yes + Query Crypto Status = Yes +}
75-75: Cryptic comment lacks context.The comment
# Ex nanmigis unclear. Consider expanding it to explain the naming convention change or removing it if it was debug/temporary.systemtests/tests/autochanger/test-setup (1)
37-45: Unquoted argument may cause issues with spaces.The
${1:-}is unquoted in both script invocations. If an argument containing spaces is passed, it could be split incorrectly.🔧 Suggested fix to quote arguments
-if ! "${BAREOS_SCRIPTS_DIR}"/invalidate_tapes.sh ${1:-}; then +if ! "${BAREOS_SCRIPTS_DIR}"/invalidate_tapes.sh "${1:-}"; then echo "Could not invalidate tapes" exit 1 fi -if ! "${BAREOS_SCRIPTS_DIR}"/create_autochanger_configs.sh ${1:-}; then +if ! "${BAREOS_SCRIPTS_DIR}"/create_autochanger_configs.sh "${1:-}"; then echo "Could not create autochanger configs" exit 1 fisystemtests/scripts/functions-ndmp.in (1)
112-113: SSH variable defined after functions that use it.The
SSHvariable is defined at the end of the file (Line 112), but functions likecleanup_remote,make_remote_data_full, etc. reference it starting from Line 86. While bash's late binding allows this to work when functions are called after sourcing, it reduces readability. Consider moving the SSH definition before the functions that use it.systemtests/tests/ndmp-native/CMakeLists.txt (1)
100-107: Hardcoded test names are fragile.The test names
system:ndmp-native:04-restore-single-file-with-fileandsystem:ndmp-native:09-restore-incremental-with-fileare hardcoded. If test naming conventions change or tests are renamed, theseset_tests_propertiescalls will silently fail.Consider using variables or comments documenting why these specific tests are disabled, and what conditions would allow re-enabling them.
systemtests/scripts/functions (1)
52-62: Localizeoutputto avoid leaking state.
outputis newly introduced and global; making itlocalprevents clobbering callers that also useoutput.♻️ Suggested tweak
- output=$( + local output + output=$( "${BAREOS_BCONSOLE_BINARY}" -c "${BAREOS_CONFIG_DIR}" <<EOF .api 2 .sql query="SELECT CASE WHEN is_called THEN last_value ELSE 0 END FROM job_jobid_seq;" EOF )systemtests/tests/scsicrypto/testrunner-backup-bscrypto (1)
254-276: Guard against an emptybareos_tape_deviceand quote it in calls.If the config format changes or the pattern doesn’t match, this becomes empty and the bls/bextract calls fail in a non-obvious way. A small guard and quoting improves resilience.
♻️ Suggested hardening
bareos_tape_device=$(awk -F '=' '/^ .*Name.*=.*tapedrive.*-0.*/ {gsub(" ","",$2);gsub("\"","",$2);print $2}' etc/bareos/bareos-sd.d/device/tape_devices.conf) +if [ -z "${bareos_tape_device}" ]; then + echo "Failed to resolve tapedrive device from tape_devices.conf" >&2 + exit 1 +fi -sbin/bls-"${TestName}" -d10 -v -c "$(pwd)/etc/bareos" --director "bareos-dir" -L -j -V ${volume} ${bareos_tape_device} >"${bls_lj_log}" +sbin/bls-"${TestName}" -d10 -v -c "$(pwd)/etc/bareos" --director "bareos-dir" -L -j -V "${volume}" "${bareos_tape_device}" >"${bls_lj_log}" ... -sbin/bls-"${TestName}" -d10 -v -c "$(pwd)/etc/bareos" --director "bareos-dir" -V ${volume} ${bareos_tape_device} >"${bls_log}" +sbin/bls-"${TestName}" -d10 -v -c "$(pwd)/etc/bareos" --director "bareos-dir" -V "${volume}" "${bareos_tape_device}" >"${bls_log}" ... -sbin/bextract-"${TestName}" -d10 -v -c "$(pwd)/etc/bareos" --director "bareos-dir" -V ${volume} ${bareos_tape_device} "${restore_directory}" >"${bextract_log}" +sbin/bextract-"${TestName}" -d10 -v -c "$(pwd)/etc/bareos" --director "bareos-dir" -V "${volume}" "${bareos_tape_device}" "${restore_directory}" >"${bextract_log}"systemtests/tests/ndmp-native/testrunner-01-full-backup (1)
41-52: Ensure setdebug is reset even when the backup fails.♻️ Suggested adjustment
start_test + +reset_debug() { + set +e + echo "setdebug level=0 trace=0 timestamp=0 dir" >"${tmp}/bconcmds" + run_bconsole "${tmp}/bconcmds" || true +} +trap reset_debug EXIT cat <<END_OF_DATA >"${tmp}/bconcmds" setdebug level=250 trace=1 timestamp=1 dir @$out ${log_home}/full.out run job=ndmp-native fileset=${NDMP_CLIENT}-fileset level=Full yes wait END_OF_DATA run_bconsole "${tmp}/bconcmds" -echo "setdebug level=0 trace=0 timestamp=0 dir" >"${tmp}/bconcmds" -run_bconsole "${tmp}/bconcmds" - check_log "${log_home}/full.out"systemtests/tests/ndmp-bareos/CMakeLists.txt (1)
59-69: Optional: document why these tests are disabled.
Consider adding a brief comment or reference to the tracking issue so the disablement rationale is preserved.systemtests/tests/ndmp-bareos/testrunner-04-restore-single-file-with-file (1)
56-57: Follow up on the zombie-job check TODO.
If you want, I can help wire this back in or open a tracking issue.systemtests/tests/ndmp-bareos/testrunner-05-restore-full (1)
60-61: Follow up on the zombie-job check TODO.
If you’d like, I can help enable this check or open a tracking issue.systemtests/tests/ndmp-bareos/testrunner-06-restore-full-relative (1)
60-61: Track the disabled zombie check with an issue/flag.Consider annotating with a ticket ID or gating via an env flag to avoid a permanent coverage gap.
systemtests/tests/ndmp-bareos/testrunner-06-restore-full-relative
Outdated
Show resolved
Hide resolved
systemtests/tests/ndmp-native/etc/bareos/bareos-dir.d/jobdefs/DefaultNDMPRestoreJob.conf
Show resolved
Hide resolved
8eb71b9 to
86db5da
Compare
- modularize ndmp-bareos ndmp-native
- ndmp can be now tested with omnios
- cmake: ndmp introduce -Dndmp_config parameter
Based on CTestNDMPConfig.cmake.template you can create a file
with all the parameter set for being able to build and run systemtest
ndmp-bareos ndmp-native on either remote isilon or omnios.
new variable in Ctest config replace environment.local.in, which is
dropped
`Auth Type` parameter is now configurable
In your CTestNDMPConfig.cmake you can set the two used auth type
`ndmp_data_agent_auth_type` and `ndmp_tape_and_robot_auth_type`
- introduce "DOFULLFAILED"
Setting DOFULLFAILED allow to trigger a first full without data
preparation which then will failed, and will be rerun automatically.
Checking for bug no Level in rerun
printf was introduce to limit error exit due to null byte warning in
grep command: to be checked if really printf is needed
run copy, client and fileset were added due to misused of fileset id
when the selection is created on first run.
(fileset isilon is added to database while not used for omnios jobs)
- use printf on all restore check so it filter the null byte warning
on omnios
- factorize all common ndmp function in scripts/function-ndmp
- add test comparing with bscan original and copy
- systemtest function: use .api2 and .sql for last_job_id
- use add_alphabetic_requirements
- in restore when= make date format FreeBSD friendly
wip: check_for_zombie_jobs is not able to handle ndmp status client
do be uncommented once fixed
wip: ndmp-native references issues internal/issues#445
internal/issues#446 internal/issues#447
wip: ndmp-bareos add internal/issues#453 internal/issues#447
wip: internal/issues#349 copy loose data
Make all tests with autochanger and tapes using same scripts - replace tape-config by uniformized test-config in block-size test - fix logging in redirect_output by using tmp dir - fix bareos tape device naming - add select to the restore command Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
- add debugging to functions - ndmp-bareos remove unused local function
- add NDMP_TEST_PATH to environment.in file with the following path
${NDMP_MOUNTPOINT}/home/regress/${NDMP_FILESYSTEM}/${TestName}
The chosen order should allow multiconcurrency tests from several
hosts at the same time.
- adapt all fileset to use NDMP_TEST_PATH
86db5da to
ffb20ec
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (15)
systemtests/scripts/redirect_output (1)
3-3: Quote${0}insidebasenameto handle paths with spaces.The argument to
basenameis unquoted, which could cause issues if the script path contains spaces or special characters.Suggested fix
-logfile="${tmp}/$(basename ${0}).log" +logfile="${tmp}/$(basename "${0}").log"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/scripts/redirect_output` at line 3, The logfile assignment uses an unquoted ${0} inside basename which breaks for script paths with spaces; update the assignment so basename receives a quoted parameter and preserves spaces—i.e., change the right-hand side to use $(basename "${0}") (keeping the surrounding "${tmp}/... .log" quoting) so the script name is handled safely (reference: the logfile variable assignment and use of basename "${0}").systemtests/scripts/create_autochanger_configs.sh.in (1)
94-102: Minor indentation inconsistency on closing brace.The new device settings (
Drive Tape Alert Enabled,Drive Crypto Enabled,Query Crypto Status) are good additions for tape monitoring and encryption support. However, the closing brace on line 102 has inconsistent indentation (single space) compared to the 4-space indentation used for other lines in the Device block.🔧 Suggested fix for consistent indentation
Drive Tape Alert Enabled = Yes Drive Crypto Enabled = Yes Query Crypto Status = Yes - } +}Or align with the block's 4-space convention:
Drive Tape Alert Enabled = Yes Drive Crypto Enabled = Yes Query Crypto Status = Yes - } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/scripts/create_autochanger_configs.sh.in` around lines 94 - 102, The closing brace for the Device block has inconsistent indentation; adjust the closing "}" to match the block's 4-space indentation so it aligns with the other Device settings (the block containing AutoChanger, AutomaticMount, MaximumFileSize, AlwaysOpen, Maximum Concurrent Jobs, Drive Tape Alert Enabled, Drive Crypto Enabled, Query Crypto Status) to maintain consistent formatting.systemtests/tests/scsicrypto/testrunner-backup-bscrypto (1)
254-255: Add validation for extractedbareos_tape_deviceto fail fast with a clear error.If the awk pattern doesn't match (e.g., config file format changes or file is missing),
bareos_tape_devicewill be empty, causing downstreamblsandbextractcommands to fail with unclear errors.🛡️ Proposed fix to add validation
bareos_tape_device=$(awk -F '=' '/^ .*Name.*=.*tapedrive.*-0.*/ {gsub(" ","",$2);gsub("\"","",$2);print $2}' etc/bareos/bareos-sd.d/device/tape_devices.conf) +if [ -z "${bareos_tape_device}" ]; then + printf "Failed to extract bareos_tape_device from config\n" + export estat=1 + exit 1 +fi sbin/bls-"${TestName}" -d10 -v -c "$(pwd)/etc/bareos" --director "bareos-dir" -L -j -V ${volume} ${bareos_tape_device} >"${bls_lj_log}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/scsicrypto/testrunner-backup-bscrypto` around lines 254 - 255, The extracted variable bareos_tape_device can be empty if the awk pattern fails; add a validation immediately after its assignment in the script to check that bareos_tape_device is non-empty and points to an existing device (or at least is non-empty), and if the check fails print a clear error message (including the attempted pattern and file path) to stderr and exit with a non-zero status before calling sbin/bls-"${TestName}" -d10 ...; this ensures bls-"${TestName}" and downstream bextract invocations fail fast with a clear diagnostic.systemtests/tests/ndmp-bareos/etc/bareos/bareos-sd.d/device/CopyStorage.conf (1)
4-4: Consider parameterizingArchive Devicefor parallel test isolation.Given concurrent NDMP test execution goals, a static
Archive Device = storagecan become a shared resource hotspot. Making this test-instance-specific would reduce collision risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-bareos/etc/bareos/bareos-sd.d/device/CopyStorage.conf` at line 4, Replace the hard-coded "Archive Device = storage" entry with a parameterized value so each test instance gets its own device name; update the code that renders CopyStorage.conf to inject a unique value (for example using an environment/template variable like ARCHIVE_DEVICE or derived from TEST_INSTANCE_ID) and ensure the test harness sets that variable per-parallel-run, leaving the config to reference the variable instead of the literal "storage".systemtests/tests/block-size/testrunner (1)
64-64: Quote forwarded optional arg for shell safety and consistency.Lines 64 and 69 pass
${1:-}unquoted to shell scripts, creating potential word-splitting risk if the argument contains spaces. The sibling test filesystemtests/tests/autochanger/test-setupquotes this same pattern for robustness.♻️ Proposed fix
-if ! "${BAREOS_SCRIPTS_DIR}"/invalidate_tapes.sh ${1:-}; then +if ! "${BAREOS_SCRIPTS_DIR}"/invalidate_tapes.sh "${1:-}"; then @@ -if ! "${BAREOS_SCRIPTS_DIR}"/create_autochanger_configs.sh ${1:-}; then +if ! "${BAREOS_SCRIPTS_DIR}"/create_autochanger_configs.sh "${1:-}"; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/block-size/testrunner` at line 64, The shell invocations of the helper script invalidate_tapes.sh use an unquoted parameter ${1:-}, which risks word-splitting; update both calls (the one at the existing if test and the sibling on line 69) to pass the argument quoted as "${1:-}" so the script receives the whole optional argument safely and consistently with the other test (systemtests/tests/autochanger/test-setup).systemtests/tests/ndmp-bareos/testrunner-07-restore-full-absolute (1)
56-58: Consider handling grep failures gracefully.With
set -eactive (line 22), if thegrepat line 56 doesn't find a match, the entire test will fail immediately. This may be intentional (to catch missing IP marker), but if the test data could legitimately not contain the IP address in some scenarios, this could cause false failures.If this is the expected behavior, the code is fine as-is. If you want more descriptive error handling, consider:
-${SSH} "tail -c 200 \"${restore_path}/full\" | xargs | grep ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}" +${SSH} "tail -c 200 \"${restore_path}/full\" | xargs | grep ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}" || { echo "ERROR: IP marker not found in restored file"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-bareos/testrunner-07-restore-full-absolute` around lines 56 - 58, The tail+grep pipeline that searches for ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT} against the last 200 bytes of \"${restore_path}/full\" should not cause an uninformative immediate exit under set -e; replace the bare grep with a controlled check (e.g., use grep -q and test its exit code, or append || { echo "Descriptive error: IP marker not found in ${restore_path}/full"; exit 1; } or use || true if non-fatal) so failures are handled with a clear message or allowed when expected instead of letting set -e abort the entire test unexpectedly.systemtests/tests/ndmp-native/testrunner-07-restore-full-absolute (1)
59-59: Use fixed-string grep for IP verification.Line 59 currently treats the IP as a regex; dots can overmatch. Use fixed-string matching for a stricter assertion.
♻️ Suggested change
-${SSH} "printf \"%s\" \$(tail -c 200 \"${restore_path}/full\") | grep ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}" +${SSH} "printf \"%s\" \$(tail -c 200 \"${restore_path}/full\") | grep -F -- \"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}\""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-native/testrunner-07-restore-full-absolute` at line 59, The grep invocation treats ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT} as a regex and can overmatch; change the check in the command using tail+grep (the line invoking ${SSH} "printf \"%s\" \$(tail -c 200 \"${restore_path}/full\") | grep ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}") to use fixed-string matching (e.g., grep -F or --fixed-strings) so dots in the IP are matched literally; keep the same pipeline and variable names but add the -F flag to grep to tighten the assertion.systemtests/tests/ndmp-native/testrunner-09-restore-incremental-with-file (1)
26-29: Minor: Comment block style.The comment block at lines 26-28 is missing the trailing
#line that other scripts use. This is purely stylistic.🧹 Optional consistency fix
# # make sure we can restore a specific incremental backup # by passing file= on restore cmd line +#🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-native/testrunner-09-restore-incremental-with-file` around lines 26 - 29, Add the missing trailing comment delimiter to the existing comment block that documents restoring a specific incremental backup (the three-line comment starting with "make sure we can restore a specific incremental backup" and "by passing file= on restore cmd line") so it matches the style used in other scripts—i.e., append a line containing only "#" after the block to maintain consistency.systemtests/tests/ndmp-bareos/testrunner-10-copy (3)
44-44: Unused variable assignment.The variable
idis assigned but never used in this script. Remove it to avoid confusion.🧹 Proposed fix
-id=last_jobid_or_zero # first run the copy jobs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-bareos/testrunner-10-copy` at line 44, Remove the unused variable assignment "id=last_jobid_or_zero" by deleting that line; ensure no other code references the variable "id" or relies on this assignment (keep the existing "last_jobid_or_zero" value usage where needed) so the script no longer contains the redundant "id" binding.
107-107: Use fixed-string grep here as well.Same issue as above—use
grep -Ffor the IP address match.🔧 Proposed fix
-${SSH} "tail -c 200 \"${NDMP_TEST_PATH}/incremental\" | xargs | tail -n 1 | grep \"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-9\"" +${SSH} "tail -c 200 \"${NDMP_TEST_PATH}/incremental\" | xargs | tail -n 1 | grep -F -- \"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-9\""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-bareos/testrunner-10-copy` at line 107, The tail/xargs/grep command that checks the incremental file uses a regex-style grep; change it to fixed-string matching by adding the -F flag to grep (i.e., use grep -F) when searching for the literal "${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-9" in the command that currently reads ${SSH} "tail -c 200 \"${NDMP_TEST_PATH}/incremental\" | xargs | tail -n 1 | grep \"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-9\"" so the IP address is matched as a literal string.
80-80: Use fixed-string grep to avoid regex false positives.IP addresses contain dots that are regex wildcards. Use
grep -Ffor literal matching, consistent with the fix applied intestrunner-06-restore-full-relative.🔧 Proposed fix
-${SSH} "tail -c 200 \"${NDMP_TEST_PATH}/full\" | xargs | head -n 1 | grep \"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}\"" +${SSH} "tail -c 200 \"${NDMP_TEST_PATH}/full\" | xargs | head -n 1 | grep -F -- \"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}\""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-bareos/testrunner-10-copy` at line 80, The grep invocation in the remote command can match dots as regex wildcards and should use fixed-string matching; update the pipeline command that currently ends with | grep "${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}" to use grep -F (or --fixed-strings) instead (i.e., | grep -F "${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}") so the IP is matched literally, consistent with the change in testrunner-06-restore-full-relative.systemtests/tests/ndmp-native/testrunner-05-restore-full (1)
56-58: Use fixed-string grep for IP address matching.Line 58 uses
grepwith an IP address pattern. The dots in IP addresses act as regex wildcards. Usegrep -Ffor literal matching.🔧 Proposed fix
-${SSH} "printf \"%s\" \$(tail -c 200 \"${NDMP_TEST_PATH}/full\") | grep ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}" +${SSH} "printf \"%s\" \$(tail -c 200 \"${NDMP_TEST_PATH}/full\") | grep -F -- \"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}\""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-native/testrunner-05-restore-full` around lines 56 - 58, The grep invocation in the SSH command is treating the IP variable ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT} as a regex, so dots act as wildcards; update the command that runs printf "$(tail -c 200 "${NDMP_TEST_PATH}/full")" | grep ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT} to use fixed-string matching (e.g., change grep to grep -F or grep -F --) so the IP address is matched literally; ensure you update the command where ${SSH} executes the printf/tail pipeline.systemtests/tests/ndmp-bareos/testrunner-05-restore-full (1)
56-56: Use fixed-string grep for IP address matching.Same issue as in other test scripts—use
grep -Fto avoid regex interpretation of dots in the IP address.🔧 Proposed fix
-${SSH} "tail -c 200 \"${NDMP_TEST_PATH}/full\" | xargs | grep ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}" +${SSH} "tail -c 200 \"${NDMP_TEST_PATH}/full\" | xargs | grep -F -- \"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}\""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-bareos/testrunner-05-restore-full` at line 56, The grep invocation in the SSH command pipeline (the line using ${SSH} "tail -c 200 \"${NDMP_TEST_PATH}/full\" | xargs | grep ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}") can treat the IP as a regex; change it to use fixed-string matching by adding -F (i.e., use grep -F ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}) so dots in the IP are matched literally and no regex interpretation occurs.systemtests/tests/ndmp-bareos/testrunner-00-full-fail-rerun (1)
83-92: Reasonable handling of "Waiting on FD" edge case.The conditional check for
Status=Waiting on FDwith a fallback to cancel all jobs is a pragmatic workaround for the known issue. Thesleep 5gives time for storage resource release before proceeding.Consider making the sleep duration configurable or adding a comment explaining why 5 seconds was chosen.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-bareos/testrunner-00-full-fail-rerun` around lines 83 - 92, The handling of the "Status=Waiting on FD" edge case currently cancels jobs and uses a hardcoded sleep 5 to wait for resource release; update the testrunner-00-full-fail-rerun logic (around the status_storage check and run_bconsole calls) to either read a configurable wait timeout (e.g., an environment variable or test config like NDMP_WAIT_SECONDS) or add a clear inline comment explaining why 5 seconds was chosen and that it is a pragmatic backoff for FD release; ensure you reference the status_storage variable, the run_bconsole invocation that issues "cancel all yes", and the sleep command so the change is applied in that block.systemtests/tests/ndmp-native/testrunner-02-incremental-backups (1)
63-71: Consider parameterizing the job name for consistency.The job name
ndmp-nativeis hardcoded while the fileset uses the parameterized${NDMP_CLIENT}-fileset. This inconsistency could cause issues if the test is run against different NDMP configurations.💡 Suggested improvement
cat <<END_OF_DATA >"${tmp}/bconcmds" @$out ${log_home}/incr-${ID}.out -run job=ndmp-native fileset=${NDMP_CLIENT}-fileset level=Incremental yes +run job=${NDMP_CLIENT} fileset=${NDMP_CLIENT}-fileset level=Incremental yes wait END_OF_DATA🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-native/testrunner-02-incremental-backups` around lines 63 - 71, The hardcoded job name "ndmp-native" should be parameterized to match the fileset pattern; update the test to use a job variable (e.g., JOB_NAME) instead of the literal string so it becomes run job=${JOB_NAME} fileset=${NDMP_CLIENT}-fileset level=Incremental yes (or derive JOB_NAME as "${NDMP_CLIENT}-job" / the correct job naming convention used elsewhere) and ensure any references (logs or expected names) use that same variable to keep the script consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@systemtests/scripts/functions-ndmp.in`:
- Around line 97-99: The redirection using `<$(readlink sbin/bareos_dir-*)` and
`<$(readlink sbin/bareos_sd-*)` is unsafe for multiple matches and paths with
spaces; change to first resolve the glob to a single, quoted pathname before the
SSH redirection (e.g., expand sbin/bareos_dir-* and sbin/bareos_sd-* into
variables like dir_bin and sd_bin, verify there is exactly one match, run
readlink -f on that single path, and fail with a clear error if zero or multiple
matches), then use the quoted variables in the existing SSH "cat - >
\"${NDMP_TEST_PATH}/testfile\"" and "testfile2" redirections so filenames with
spaces are handled correctly.
In `@systemtests/tests/ndmp-bareos/CMakeLists.txt`:
- Around line 69-81: The three tests currently unconditionally disabled
(system:ndmp-bareos:00-full-fail-rerun,
system:ndmp-bareos:04-restore-single-file-with-file,
system:ndmp-bareos:09-restore-incremental-last-with-file) should be gated behind
a CMake option instead of hardcoding DISABLED TRUE; add a boolean option (e.g.,
OPTION ENABLE_NDMP_BAREOS_SYSTEM_TESTS OFF) near the top of the CMakeLists, then
wrap each set_tests_properties(...) DISABLED TRUE call inside an if(NOT
ENABLE_NDMP_BAREOS_SYSTEM_TESTS) ... endif() (or invert logic to set DISABLED
only when the option is OFF), and add a short comment/documentation string for
the option so enabling these tests is explicit and reversible.
In `@systemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/storage/ndmp.conf.in`:
- Around line 43-49: The Password placeholder is quoted inconsistently in the
Storage block — ensure the Password line uses consistent quoting so values with
spaces/special characters are handled; update the Storage block's Password =
`@sd_password`@ entry to match the other occurrence (Password = "@sd_password@")
so both uses of `@sd_password`@ are quoted, and verify the Device = CopyStorage /
Media Type = Copy entries remain unchanged.
In
`@systemtests/tests/ndmp-bareos/testrunner-09-restore-incremental-last-with-file`:
- Line 47: The restore command hard-codes a prior job id (jobid=10) and a marker
like "-9", making restores flaky; replace these fixed values by dynamically
resolving the correct job/marker at runtime (e.g., query the backup catalog for
the most recent successful job for client/ fileset and set a variable such as
NDMP_LAST_JOB_ID or NDMP_TARGET_MARKER), then use that variable in the restore
invocation (the line containing "restore jobid=10 client=ndmp-fd
fileset=${NDMP_CLIENT}-fileset restorejob=NDMPRestoreDump file=... yes" and the
later usage of "-9"); ensure the lookup logic picks the intended
incremental/last job by filtering on client/fileset and success state so reruns
and parallel tests are stable.
In `@systemtests/tests/ndmp-bareos/testrunner-11-compare-with-copy`:
- Line 71: The diff command writes output to "${tmp}/diff.out" and captures its
exit code into dstat but never enforces it; update the test to check dstat after
the diff invocation (the diff between "${tmp}/bscan_FileStorage.out" and
"${tmp}/bscan_CopyStorage.out") and fail the test when dstat is non-zero by
printing the diff content (from "${tmp}/diff.out") and exiting with dstat so
both mismatch (1) and error (2) cases are propagated; ensure the check
immediately follows the diff invocation that sets dstat.
- Around line 56-59: The existence check uses the wrong variable name: replace
the condition that checks source_volumes with copy_volumes so the "no copy
volume" error path is triggered correctly; update the if test to use
"${copy_volumes}" (and keep the echo and exit 1 behavior) so the script
correctly detects an empty copy_volumes variable.
In `@systemtests/tests/ndmp-native/CMakeLists.txt`:
- Around line 99-105: The CMake cache variable ndmp_tapes_devices is being set
without FORCE, so stale values persist and the foreach over
ndmp_tape_and_robot_agent_tape_device can append to old cache contents; update
the set call for ndmp_tapes_devices to include the CACHE entry with FORCE so the
cache is overwritten on reconfigure (modify the set invocation that defines
ndmp_tapes_devices) and keep the foreach/device append logic as-is.
In `@systemtests/tests/ndmp-native/etc/bareos/bareos-dir.d/storage/ndmp.conf.in`:
- Line 6: The ndmp_tape_and_robot_agent_auth_type placeholder is not defined in
CMake, so `@ndmp_tape_and_robot_agent_auth_type`@ will not be substituted; add a
CMake variable definition in systemtests/tests/ndmp-native/CMakeLists.txt such
as set(ndmp_tape_and_robot_agent_auth_type "Clear") following the pattern used
for ndmp_tape_and_robot_agent_address/user/password/changer_device so the
placeholder is replaced during configure; ensure the variable name exactly
matches ndmp_tape_and_robot_agent_auth_type and choose the appropriate default
("Clear" or "MD5") consistent with other ndmp configs.
In `@systemtests/tests/ndmp-native/test-setup`:
- Line 92: The for-loop is calling the hardcoded awk binary; replace the
invocation `awk` in the command `for V in $(awk -F ":" '/volumename/
{gsub("\"","",$2);gsub(",","",$2);gsub(" ","",$2); print $2}'
${tmp}/volumes.out)` with the environment-configurable `"${AWK}"` (preserving
all flags, script and quoting) so the test honors the AWK binary exported by the
test environment; also update the same pattern in the NDMP tests mentioned
(testrunner-10-copy and testrunner-11-compare-with-copy) for consistency.
In `@systemtests/tests/ndmp-native/testrunner-03-restore-single-file-with-mark`:
- Around line 45-51: The restore command in the generated bconcmds here places
the confirmation token "yes" inline with the restore invocation; update the
heredoc so that the "restore jobid=1 client=ndmp-fd
fileset=${NDMP_CLIENT}-fileset restorejob=NDMPRestoreDump" line does not include
"yes" and instead add a separate line containing only "yes" immediately after
the "done" line (preserving the existing "mark ${NDMP_TEST_PATH}/testfile",
"done" and "wait" lines), so the sequence is: restore..., mark..., done, yes,
wait.
In `@systemtests/tests/ndmp-native/testrunner-08-restore-incremental-with-mark`:
- Line 47: The test currently hardcodes the incremental job id (jobid=2) in the
restore command and expects `-1` later, making it order-dependent; replace the
static `jobid=2` usage in the restore step and the `-1` expectation by
dynamically resolving the incremental job id for the target client/fileset
before restore (e.g., run an API call or `.sql` query to fetch the latest
incremental job for client `ndmp-fd` / `${NDMP_CLIENT}-fileset`, store that id
in a variable) then use that variable in the `restore jobid=...` invocation and
in the subsequent validation instead of `-1`; ensure the lookup logic is
implemented where `restore jobid=2` is invoked and where the `-1` check occurs
so the test works reliably in reruns/concurrent runs.
---
Nitpick comments:
In `@systemtests/scripts/create_autochanger_configs.sh.in`:
- Around line 94-102: The closing brace for the Device block has inconsistent
indentation; adjust the closing "}" to match the block's 4-space indentation so
it aligns with the other Device settings (the block containing AutoChanger,
AutomaticMount, MaximumFileSize, AlwaysOpen, Maximum Concurrent Jobs, Drive Tape
Alert Enabled, Drive Crypto Enabled, Query Crypto Status) to maintain consistent
formatting.
In `@systemtests/scripts/redirect_output`:
- Line 3: The logfile assignment uses an unquoted ${0} inside basename which
breaks for script paths with spaces; update the assignment so basename receives
a quoted parameter and preserves spaces—i.e., change the right-hand side to use
$(basename "${0}") (keeping the surrounding "${tmp}/... .log" quoting) so the
script name is handled safely (reference: the logfile variable assignment and
use of basename "${0}").
In `@systemtests/tests/block-size/testrunner`:
- Line 64: The shell invocations of the helper script invalidate_tapes.sh use an
unquoted parameter ${1:-}, which risks word-splitting; update both calls (the
one at the existing if test and the sibling on line 69) to pass the argument
quoted as "${1:-}" so the script receives the whole optional argument safely and
consistently with the other test (systemtests/tests/autochanger/test-setup).
In
`@systemtests/tests/ndmp-bareos/etc/bareos/bareos-sd.d/device/CopyStorage.conf`:
- Line 4: Replace the hard-coded "Archive Device = storage" entry with a
parameterized value so each test instance gets its own device name; update the
code that renders CopyStorage.conf to inject a unique value (for example using
an environment/template variable like ARCHIVE_DEVICE or derived from
TEST_INSTANCE_ID) and ensure the test harness sets that variable
per-parallel-run, leaving the config to reference the variable instead of the
literal "storage".
In `@systemtests/tests/ndmp-bareos/testrunner-00-full-fail-rerun`:
- Around line 83-92: The handling of the "Status=Waiting on FD" edge case
currently cancels jobs and uses a hardcoded sleep 5 to wait for resource
release; update the testrunner-00-full-fail-rerun logic (around the
status_storage check and run_bconsole calls) to either read a configurable wait
timeout (e.g., an environment variable or test config like NDMP_WAIT_SECONDS) or
add a clear inline comment explaining why 5 seconds was chosen and that it is a
pragmatic backoff for FD release; ensure you reference the status_storage
variable, the run_bconsole invocation that issues "cancel all yes", and the
sleep command so the change is applied in that block.
In `@systemtests/tests/ndmp-bareos/testrunner-05-restore-full`:
- Line 56: The grep invocation in the SSH command pipeline (the line using
${SSH} "tail -c 200 \"${NDMP_TEST_PATH}/full\" | xargs | grep
${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}") can treat the IP as a regex; change it
to use fixed-string matching by adding -F (i.e., use grep -F
${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}) so dots in the IP are matched literally
and no regex interpretation occurs.
In `@systemtests/tests/ndmp-bareos/testrunner-07-restore-full-absolute`:
- Around line 56-58: The tail+grep pipeline that searches for
${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT} against the last 200 bytes of
\"${restore_path}/full\" should not cause an uninformative immediate exit under
set -e; replace the bare grep with a controlled check (e.g., use grep -q and
test its exit code, or append || { echo "Descriptive error: IP marker not found
in ${restore_path}/full"; exit 1; } or use || true if non-fatal) so failures are
handled with a clear message or allowed when expected instead of letting set -e
abort the entire test unexpectedly.
In `@systemtests/tests/ndmp-bareos/testrunner-10-copy`:
- Line 44: Remove the unused variable assignment "id=last_jobid_or_zero" by
deleting that line; ensure no other code references the variable "id" or relies
on this assignment (keep the existing "last_jobid_or_zero" value usage where
needed) so the script no longer contains the redundant "id" binding.
- Line 107: The tail/xargs/grep command that checks the incremental file uses a
regex-style grep; change it to fixed-string matching by adding the -F flag to
grep (i.e., use grep -F) when searching for the literal
"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-9" in the command that currently reads
${SSH} "tail -c 200 \"${NDMP_TEST_PATH}/incremental\" | xargs | tail -n 1 | grep
\"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-9\"" so the IP address is matched as a
literal string.
- Line 80: The grep invocation in the remote command can match dots as regex
wildcards and should use fixed-string matching; update the pipeline command that
currently ends with | grep "${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}" to use grep
-F (or --fixed-strings) instead (i.e., | grep -F
"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}") so the IP is matched literally,
consistent with the change in testrunner-06-restore-full-relative.
In `@systemtests/tests/ndmp-native/testrunner-02-incremental-backups`:
- Around line 63-71: The hardcoded job name "ndmp-native" should be
parameterized to match the fileset pattern; update the test to use a job
variable (e.g., JOB_NAME) instead of the literal string so it becomes run
job=${JOB_NAME} fileset=${NDMP_CLIENT}-fileset level=Incremental yes (or derive
JOB_NAME as "${NDMP_CLIENT}-job" / the correct job naming convention used
elsewhere) and ensure any references (logs or expected names) use that same
variable to keep the script consistent.
In `@systemtests/tests/ndmp-native/testrunner-05-restore-full`:
- Around line 56-58: The grep invocation in the SSH command is treating the IP
variable ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT} as a regex, so dots act as
wildcards; update the command that runs printf "$(tail -c 200
"${NDMP_TEST_PATH}/full")" | grep ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT} to use
fixed-string matching (e.g., change grep to grep -F or grep -F --) so the IP
address is matched literally; ensure you update the command where ${SSH}
executes the printf/tail pipeline.
In `@systemtests/tests/ndmp-native/testrunner-07-restore-full-absolute`:
- Line 59: The grep invocation treats ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT} as
a regex and can overmatch; change the check in the command using tail+grep (the
line invoking ${SSH} "printf \"%s\" \$(tail -c 200 \"${restore_path}/full\") |
grep ${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}") to use fixed-string matching
(e.g., grep -F or --fixed-strings) so dots in the IP are matched literally; keep
the same pipeline and variable names but add the -F flag to grep to tighten the
assertion.
In `@systemtests/tests/ndmp-native/testrunner-09-restore-incremental-with-file`:
- Around line 26-29: Add the missing trailing comment delimiter to the existing
comment block that documents restoring a specific incremental backup (the
three-line comment starting with "make sure we can restore a specific
incremental backup" and "by passing file= on restore cmd line") so it matches
the style used in other scripts—i.e., append a line containing only "#" after
the block to maintain consistency.
In `@systemtests/tests/scsicrypto/testrunner-backup-bscrypto`:
- Around line 254-255: The extracted variable bareos_tape_device can be empty if
the awk pattern fails; add a validation immediately after its assignment in the
script to check that bareos_tape_device is non-empty and points to an existing
device (or at least is non-empty), and if the check fails print a clear error
message (including the attempted pattern and file path) to stderr and exit with
a non-zero status before calling sbin/bls-"${TestName}" -d10 ...; this ensures
bls-"${TestName}" and downstream bextract invocations fail fast with a clear
diagnostic.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
CMakeLists.txtCTestNDMPConfig.cmake.templatecore/src/dird/jobq.cccore/src/ndmp/ndml_conn.csystemtests/cmake/BareosSystemtestFunctions.cmakesystemtests/environment.insystemtests/scripts/create_autochanger_configs.sh.insystemtests/scripts/functionssystemtests/scripts/functions-ndmp.insystemtests/scripts/invalidate_tapes.shsystemtests/scripts/redirect_outputsystemtests/tests/autochanger/create_autochanger_configs.sh.insystemtests/tests/autochanger/invalidate_tapes.shsystemtests/tests/autochanger/redirect_outputsystemtests/tests/autochanger/test-setupsystemtests/tests/block-size/create_autochanger_configs.sh.insystemtests/tests/block-size/invalidate_tapes.shsystemtests/tests/block-size/redirect_outputsystemtests/tests/block-size/test-config.insystemtests/tests/block-size/testrunnersystemtests/tests/ndmp-bareos/CMakeLists.txtsystemtests/tests/ndmp-bareos/environment.localsystemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/client/ndmp.conf.insystemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/fileset/isilon-fileset.conf.insystemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/fileset/omnios-fileset.conf.insystemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/job/NDMPBackupDump.confsystemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/job/isilondump.confsystemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/job/ndmp-copy.confsystemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/jobdefs/DefaultNDMPJob.confsystemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/jobdefs/DefaultNDMPRestoreJob.confsystemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/pool/isilon-copy.confsystemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/pool/ndmp-copy.confsystemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/pool/ndmp.confsystemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/storage/ndmp.conf.insystemtests/tests/ndmp-bareos/etc/bareos/bareos-sd.d/device/CopyStorage.confsystemtests/tests/ndmp-bareos/etc/bareos/bareos-sd.d/device/FileStorage.confsystemtests/tests/ndmp-bareos/etc/bareos/bareos-sd.d/device/FileStorage2.confsystemtests/tests/ndmp-bareos/etc/bareos/bareos-sd.d/ndmp/ndmp.conf.insystemtests/tests/ndmp-bareos/test-cleanupsystemtests/tests/ndmp-bareos/test-setupsystemtests/tests/ndmp-bareos/testrunner-00-full-fail-rerunsystemtests/tests/ndmp-bareos/testrunner-01-full-backupsystemtests/tests/ndmp-bareos/testrunner-02-incremental-backupssystemtests/tests/ndmp-bareos/testrunner-03-restore-single-file-with-marksystemtests/tests/ndmp-bareos/testrunner-04-restore-single-file-with-filesystemtests/tests/ndmp-bareos/testrunner-05-restore-fullsystemtests/tests/ndmp-bareos/testrunner-06-restore-full-relativesystemtests/tests/ndmp-bareos/testrunner-07-restore-full-absolutesystemtests/tests/ndmp-bareos/testrunner-08-copysystemtests/tests/ndmp-bareos/testrunner-08-restore-incremental-first-with-marksystemtests/tests/ndmp-bareos/testrunner-09-restore-incremental-last-with-filesystemtests/tests/ndmp-bareos/testrunner-10-copysystemtests/tests/ndmp-bareos/testrunner-11-compare-with-copysystemtests/tests/ndmp-native/CMakeLists.txtsystemtests/tests/ndmp-native/environment.localsystemtests/tests/ndmp-native/etc/bareos/bareos-dir.d/client/ndmp.conf.insystemtests/tests/ndmp-native/etc/bareos/bareos-dir.d/fileset/isilon-fileset.conf.insystemtests/tests/ndmp-native/etc/bareos/bareos-dir.d/fileset/omnios-fileset.conf.insystemtests/tests/ndmp-native/etc/bareos/bareos-dir.d/job/ndmp-native.confsystemtests/tests/ndmp-native/etc/bareos/bareos-dir.d/jobdefs/DefaultNDMPJob.confsystemtests/tests/ndmp-native/etc/bareos/bareos-dir.d/jobdefs/DefaultNDMPRestoreJob.confsystemtests/tests/ndmp-native/etc/bareos/bareos-dir.d/storage/ndmp.conf.insystemtests/tests/ndmp-native/test-cleanupsystemtests/tests/ndmp-native/test-setupsystemtests/tests/ndmp-native/testrunnersystemtests/tests/ndmp-native/testrunner-01-full-backupsystemtests/tests/ndmp-native/testrunner-02-incremental-backupssystemtests/tests/ndmp-native/testrunner-03-restore-single-file-with-marksystemtests/tests/ndmp-native/testrunner-04-restore-single-file-with-filesystemtests/tests/ndmp-native/testrunner-05-restore-fullsystemtests/tests/ndmp-native/testrunner-06-restore-full-relativesystemtests/tests/ndmp-native/testrunner-07-restore-full-absolutesystemtests/tests/ndmp-native/testrunner-08-restore-incremental-with-marksystemtests/tests/ndmp-native/testrunner-09-restore-incremental-with-filesystemtests/tests/scsicrypto/testrunner-backup-bscrypto
💤 Files with no reviewable changes (12)
- systemtests/tests/block-size/redirect_output
- systemtests/tests/block-size/invalidate_tapes.sh
- systemtests/tests/autochanger/create_autochanger_configs.sh.in
- systemtests/tests/ndmp-native/etc/bareos/bareos-dir.d/job/ndmp-native.conf
- systemtests/tests/ndmp-bareos/etc/bareos/bareos-sd.d/device/FileStorage2.conf
- systemtests/tests/block-size/create_autochanger_configs.sh.in
- systemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/job/isilondump.conf
- systemtests/tests/autochanger/invalidate_tapes.sh
- systemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/pool/isilon-copy.conf
- systemtests/tests/autochanger/redirect_output
- systemtests/tests/ndmp-native/testrunner
- systemtests/tests/ndmp-bareos/testrunner-08-copy
systemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/storage/ndmp.conf.in
Show resolved
Hide resolved
| echo "################ restore incremental by file #####################" | ||
| cat <<END_OF_DATA >"${tmp}/bconcmds" | ||
| @$out ${log_home}/restore-file.out | ||
| restore jobid=10 client=ndmp-fd fileset=${NDMP_CLIENT}-fileset restorejob=NDMPRestoreDump file="${NDMP_TEST_PATH}/incremental" yes |
Contributor
There was a problem hiding this comment.
Hard-coded JobId/marker assumptions will cause flaky restores.
Line [47] (jobid=10) and Line [55] (-9) depend on a fixed prior job history. This is brittle and can fail on reruns or parallelized NDMP test execution.
Also applies to: 55-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@systemtests/tests/ndmp-bareos/testrunner-09-restore-incremental-last-with-file`
at line 47, The restore command hard-codes a prior job id (jobid=10) and a
marker like "-9", making restores flaky; replace these fixed values by
dynamically resolving the correct job/marker at runtime (e.g., query the backup
catalog for the most recent successful job for client/ fileset and set a
variable such as NDMP_LAST_JOB_ID or NDMP_TARGET_MARKER), then use that variable
in the restore invocation (the line containing "restore jobid=10 client=ndmp-fd
fileset=${NDMP_CLIENT}-fileset restorejob=NDMPRestoreDump file=... yes" and the
later usage of "-9"); ensure the lookup logic picks the intended
incremental/last job by filtering on client/fileset and success state so reruns
and parallel tests are stable.
Comment on lines
+99
to
+105
| set(ndmp_tapes_devices | ||
| "" | ||
| CACHE STRING "builded string of Devices for ndmp storage" | ||
| ) | ||
| foreach(device ${ndmp_tape_and_robot_agent_tape_device}) | ||
| string(APPEND ndmp_tapes_devices "Device = \"${device}\"\n") | ||
| endforeach(device) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CMake cache entry set command FORCE flag behavior when cache already exists
💡 Result:
set(... CACHE ... FORCE) will overwrite an existing cache entry’s value (even if the user previously set it via -D/ccmake/cmake-gui). Without FORCE, set(... CACHE ...) generally does not overwrite an already-existing cache entry, because cache entries are intended to be user-editable defaults. [1]
Two important nuances when a cache entry already exists:
- Normal variable vs cache visibility: If a normal (non-cache) variable of the same name exists in the current scope,
${VAR}resolves to the normal variable first, so the cache value may be “hidden” from immediate use. [1] - Whether
set(CACHE{VAR} ... FORCE ...)removes a normal variable binding depends on policyCMP0126:CMP0126=OLD: usingFORCE(orINTERNAL) can cause CMake to remove the normal variable binding in the current scope (historical behavior). [2]CMP0126=NEW:set(CACHE{VAR} ...)does not remove the normal variable binding; the normal variable continues to “win” for${VAR}lookups. [2]
Also: setting a cache entry with type INTERNAL implies FORCE. [1]
Sources: Debian’s rendered CMake command docs for set(CACHE{...} ... FORCE ...) and the CMP0126 policy text. [1] [2]
🏁 Script executed:
cd systemtests/tests/ndmp-native && head -120 CMakeLists.txt | tail -30Repository: bareos/bareos
Length of output: 1260
Add FORCE to the cache set() call to prevent stale values across reconfigurations.
Without FORCE, CMake cache entries persist and are not overwritten by subsequent set(... CACHE ...) calls when the entry already exists. This causes the foreach loop to append entries to stale cache state from previous configuration runs, resulting in duplicated or incorrect Device = lines.
Suggested fix
set(ndmp_tapes_devices
""
- CACHE STRING "builded string of Devices for ndmp storage"
+ CACHE STRING "built string of devices for ndmp storage"
+ FORCE
)
- foreach(device ${ndmp_tape_and_robot_agent_tape_device})
+ foreach(device IN LISTS ndmp_tape_and_robot_agent_tape_device)
string(APPEND ndmp_tapes_devices "Device = \"${device}\"\n")
endforeach(device)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@systemtests/tests/ndmp-native/CMakeLists.txt` around lines 99 - 105, The
CMake cache variable ndmp_tapes_devices is being set without FORCE, so stale
values persist and the foreach over ndmp_tape_and_robot_agent_tape_device can
append to old cache contents; update the set call for ndmp_tapes_devices to
include the CACHE entry with FORCE so the cache is overwritten on reconfigure
(modify the set invocation that defines ndmp_tapes_devices) and keep the
foreach/device append logic as-is.
systemtests/tests/ndmp-native/etc/bareos/bareos-dir.d/storage/ndmp.conf.in
Show resolved
Hide resolved
systemtests/tests/ndmp-native/testrunner-03-restore-single-file-with-mark
Show resolved
Hide resolved
| echo "################ restore incremental by mark #####################" | ||
| cat <<END_OF_DATA >"${tmp}/bconcmds" | ||
| @$out ${log_home}/restore-mark.out | ||
| restore jobid=2 client=ndmp-fd fileset=${NDMP_CLIENT}-fileset restorejob=NDMPRestoreDump |
Contributor
There was a problem hiding this comment.
Avoid static job sequencing in restore verification.
Line [47] (jobid=2) and Line [60] (expecting -1) make this test order-dependent and fragile for reruns/concurrent execution. Please resolve the incremental job dynamically (e.g., via .api 2/.sql query) before restore and validation.
Also applies to: 60-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@systemtests/tests/ndmp-native/testrunner-08-restore-incremental-with-mark` at
line 47, The test currently hardcodes the incremental job id (jobid=2) in the
restore command and expects `-1` later, making it order-dependent; replace the
static `jobid=2` usage in the restore step and the `-1` expectation by
dynamically resolving the incremental job id for the target client/fileset
before restore (e.g., run an API call or `.sql` query to fetch the latest
incremental job for client `ndmp-fd` / `${NDMP_CLIENT}-fileset`, store that id
in a variable) then use that variable in the `restore jobid=...` invocation and
in the subsequent validation instead of `-1`; ensure the lookup logic is
implemented where `restore jobid=2` is invoked and where the `-1` check occurs
so the test works reliably in reruns/concurrent runs.
The exit 1 on Line 81 will only exit the subshell, not the main script. The function will continue executing after the subshell completes, potentially masking the failure. fix using process substitution.
avoid command to treats pattern as filename instead of program.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
systemtests/tests/ndmp-native/CMakeLists.txt (1)
102-108:⚠️ Potential issue | 🟠 MajorMissing
FORCEon cache variable and suboptimal foreach syntax.Without
FORCE, thendmp_tapes_devicescache entry persists across reconfigurations, causing the foreach loop to append to stale values. Additionally, usingforeach(device IN LISTS ...)is the recommended CMake idiom for iterating over list variables.Suggested fix
set(ndmp_tapes_devices "" - CACHE STRING "builded string of Devices for ndmp storage" + CACHE STRING "built string of devices for ndmp storage" + FORCE ) - foreach(device ${ndmp_tape_and_robot_agent_tape_device}) + foreach(device IN LISTS ndmp_tape_and_robot_agent_tape_device) string(APPEND ndmp_tapes_devices "Device = \"${device}\"\n") endforeach(device)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-native/CMakeLists.txt` around lines 102 - 108, The cached variable ndmp_tapes_devices is being appended to across reconfigurations and the foreach uses the older syntax; update the set() call for ndmp_tapes_devices to include FORCE so the cache is overwritten on reconfigure, and change the loop to the modern CMake idiom using foreach(device IN LISTS ndmp_tape_and_robot_agent_tape_device) to iterate the list variable (refer to ndmp_tapes_devices and ndmp_tape_and_robot_agent_tape_device to locate the code).systemtests/tests/ndmp-native/testrunner-08-restore-incremental-with-mark (2)
45-52:⚠️ Potential issue | 🟠 MajorHardcoded
jobid=2makes test order-dependent.The restore command uses a static job ID which will fail if tests run in different order, are rerun, or run concurrently. Consider resolving the incremental job ID dynamically using
.api 2and.sqlqueries (similar to other tests in this PR that uselast_job_idpatterns).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-native/testrunner-08-restore-incremental-with-mark` around lines 45 - 52, The restore command in the generated bconcmds uses a hardcoded "jobid=2" which makes the test order-dependent; change the test to dynamically obtain the incremental backup job id (use the same pattern other tests use: call the API via ".api 2" and run a ".sql" query to fetch last_job_id or the correct job id for ${NDMP_CLIENT}-fileset/incremental) and substitute that variable into the restore line in the bconcmds (target the created bconcmds block and the "restore jobid=2 client=ndmp-fd fileset=..." line to replace the hardcoded id with the dynamically resolved last_job_id).
58-63:⚠️ Potential issue | 🟡 MinorValidation pattern
-1is coupled to hardcoded job ordering.The grep pattern
${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-1assumes a specific incremental sequence number. This validation will break if the dynamic job resolution is implemented, as the expected marker content should align with the actually restored job.Consider extracting the expected marker from the job metadata or making the pattern resilient to reordering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-native/testrunner-08-restore-incremental-with-mark` around lines 58 - 63, The grep uses a hardcoded suffix "-1" which assumes job ordering; instead determine the expected marker dynamically and check against that value in the "${NDMP_TEST_PATH}/incremental" file. Modify the test to fetch the actual restored job identifier (e.g. from the restore/job metadata or restore output) into a variable (replace the literal "${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-1"), then run the SSH tail/grep against that variable; update the conditional that sets estat=1 to use this dynamically computed expected marker so the check in testrunner-08-restore-incremental-with-mark is resilient to job reordering.
🧹 Nitpick comments (3)
systemtests/tests/ndmp-native/testrunner-08-restore-incremental-with-mark (1)
65-65: Commentedcheck_for_zombie_jobsshould be tracked.If this is intentionally disabled due to a known issue with NDMP handling (as mentioned in the PR objectives), consider adding a TODO comment with a reference to the issue tracker so it can be re-enabled once resolved.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-native/testrunner-08-restore-incremental-with-mark` at line 65, The test currently has the line invoking check_for_zombie_jobs commented out; either re-enable that call or explicitly document why it's disabled by adding a TODO/NOTE above the commented line referencing the issue tracker ID and a short rationale so it can be revisited; locate the commented invocation of check_for_zombie_jobs in testrunner-08-restore-incremental-with-mark and either uncomment the call to run the check or add a clear TODO with the external issue reference and expected condition for re-enabling.systemtests/tests/ndmp-native/CMakeLists.txt (1)
91-96: Redundant default assignments for cache variables.These defaults (
ndmpadmin,test,Clear) are already set at lines 27-32, 30-33, and the cache declarations. Theif(NOT ...)checks here are only triggered if the cache variables are explicitly set to empty strings, which differs from not being set at all.If the intent is to handle empty-string cases, consider documenting this explicitly. Otherwise, these conditionals are unreachable for the password/user cases since they default to non-empty values in the cache.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-native/CMakeLists.txt` around lines 91 - 96, The conditionals setting defaults for ndmp_data_agent_user and ndmp_data_agent_password are redundant because those cache variables already have non-empty defaults; either remove these if(NOT ndmp_data_agent_user) / if(NOT ndmp_data_agent_password) blocks, or explicitly handle empty-string cases by switching to an existence test (e.g., use if(NOT DEFINED ndmp_data_agent_user) / if(NOT DEFINED ndmp_data_agent_password)) and add a comment clarifying intent; update the block that mentions Clear similarly or document why empty-string handling is required.systemtests/tests/ndmp-bareos/testrunner-11-compare-with-copy (1)
46-59: Extract duplicated volume-list parsing into a helper.The source/copy volume discovery blocks are duplicated and already drifted once in this PR history. A tiny helper reduces future divergence risk.
♻️ Proposed refactor
+get_pool_volumes() { + local pool="$1" + bin/bconsole <<<".api 2 +list volumes pool=${pool} +" | "${AWK}" '/"volumename":/ {gsub("\"","",$2);gsub(",","",$2);print $2}' \ + | xargs | sed -e "s, ,|,g" +} + -source_volumes=$(bin/bconsole <<<".api 2 -list volumes pool=ndmp -" | "${AWK}" '/"volumename":/ {gsub("\"","",$2);gsub(",","",$2);print $2}' | xargs | sed -e "s, ,|,g") +source_volumes="$(get_pool_volumes ndmp)" if [[ -z "${source_volumes}" ]]; then echo "ERROR: No source volume found in pool ndmp" exit 1 fi -copy_volumes=$(bin/bconsole <<<".api 2 -list volumes pool=ndmp-copy -" | "${AWK}" '/"volumename":/ {gsub("\"","",$2);gsub(",","",$2);print $2}' | xargs | sed -e "s, ,|,g") +copy_volumes="$(get_pool_volumes ndmp-copy)" if [[ -z "${copy_volumes}" ]]; then echo "ERROR: No copy volume found in pool ndmp-copy" exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@systemtests/tests/ndmp-bareos/testrunner-11-compare-with-copy` around lines 46 - 59, Extract the duplicated volume-discovery block into a small helper function (e.g., get_pool_volumes) that accepts a pool name and returns the pipe-separated volume list; replace the two duplicated blocks that set source_volumes and copy_volumes with calls to get_pool_volumes "ndmp" and get_pool_volumes "ndmp-copy", and keep the same error check logic (if [[ -z "${...}" ]] then echo error; exit 1) so the behavior of the existing bin/bconsole + "${AWK}" pipeline and the AWK/ sed processing is preserved inside the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@systemtests/scripts/invalidate_tapes.sh`:
- Around line 55-59: The current parsing uses sed substitution which returns the
original line on no-match, so changer_status can be non-empty and produce
invalid dte/se for the subsequent mtx unload; change the parsing so it only
emits a value on a successful regex match (e.g. use sed -n with the s///p form
or switch to an awk extraction that only prints when the pattern matches) so
that changer_status is empty on non-matching input and the existing if [ -n
"${changer_status}" ] guard prevents invalid dte/se values (update the lines
that set changer_status, dte, and se accordingly).
- Around line 68-91: The loop advances the slot index i only on successful
invalidation which skips later full slots and then computes slots_ready as i-1;
change to iterate over fixed slot indices and track successful invalidations
separately: obtain the mtx status output once (mtx -f "${changer_device}"
status), loop over slot numbers from 1 to LAST_SLOT_NUMBER (or parse each status
line but always increment the slot index), for each slot check whether the
corresponding status line indicates "Full" and if so run mtx/mt commands
(keeping set -x only around the command group) and increment a new success
counter (e.g., success_count) on success; do not advance i conditionally, and
set slots_ready=${success_count} (and keep existing error handling when the
mtx/mt/unload group fails).
In
`@systemtests/tests/ndmp-bareos/testrunner-08-restore-incremental-first-with-mark`:
- Line 60: The current SSH check uses grep
"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-1" which can false-match e.g. "...-10"
and treats dots as regex; update the check to require an exact fixed-string
whole-line match by using grep with fixed-string and exact-match flags (e.g.,
grep -F -x) against the constructed marker
"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-1" so the line read from
"${NDMP_TEST_PATH}/incremental" must exactly equal that marker.
- Line 47: The restore line hardcodes jobid=2 which is brittle; replace it with
a dynamic job-id lookup and use that variable in the restore invocation (e.g.,
compute the first incremental job id into a variable like
NDMP_FIRST_INCREMENTAL_JOBID or call the existing helper that finds incremental
jobs) and then change the restore command from "restore jobid=2 client=ndmp-fd
fileset=${NDMP_CLIENT}-fileset restorejob=NDMPRestoreDump" to use that variable
(jobid=${NDMP_FIRST_INCREMENTAL_JOBID} or equivalent). Locate the restore
invocation string and the test setup where incremental jobs are created (look
for the test name testrunner-08-restore-incremental-first-with-mark and any
helpers that list/query job ids) and wire the computed job id into the restore
step so the test always picks the correct first incremental.
---
Duplicate comments:
In `@systemtests/tests/ndmp-native/CMakeLists.txt`:
- Around line 102-108: The cached variable ndmp_tapes_devices is being appended
to across reconfigurations and the foreach uses the older syntax; update the
set() call for ndmp_tapes_devices to include FORCE so the cache is overwritten
on reconfigure, and change the loop to the modern CMake idiom using
foreach(device IN LISTS ndmp_tape_and_robot_agent_tape_device) to iterate the
list variable (refer to ndmp_tapes_devices and
ndmp_tape_and_robot_agent_tape_device to locate the code).
In `@systemtests/tests/ndmp-native/testrunner-08-restore-incremental-with-mark`:
- Around line 45-52: The restore command in the generated bconcmds uses a
hardcoded "jobid=2" which makes the test order-dependent; change the test to
dynamically obtain the incremental backup job id (use the same pattern other
tests use: call the API via ".api 2" and run a ".sql" query to fetch last_job_id
or the correct job id for ${NDMP_CLIENT}-fileset/incremental) and substitute
that variable into the restore line in the bconcmds (target the created bconcmds
block and the "restore jobid=2 client=ndmp-fd fileset=..." line to replace the
hardcoded id with the dynamically resolved last_job_id).
- Around line 58-63: The grep uses a hardcoded suffix "-1" which assumes job
ordering; instead determine the expected marker dynamically and check against
that value in the "${NDMP_TEST_PATH}/incremental" file. Modify the test to fetch
the actual restored job identifier (e.g. from the restore/job metadata or
restore output) into a variable (replace the literal
"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-1"), then run the SSH tail/grep against
that variable; update the conditional that sets estat=1 to use this dynamically
computed expected marker so the check in
testrunner-08-restore-incremental-with-mark is resilient to job reordering.
---
Nitpick comments:
In `@systemtests/tests/ndmp-bareos/testrunner-11-compare-with-copy`:
- Around line 46-59: Extract the duplicated volume-discovery block into a small
helper function (e.g., get_pool_volumes) that accepts a pool name and returns
the pipe-separated volume list; replace the two duplicated blocks that set
source_volumes and copy_volumes with calls to get_pool_volumes "ndmp" and
get_pool_volumes "ndmp-copy", and keep the same error check logic (if [[ -z
"${...}" ]] then echo error; exit 1) so the behavior of the existing
bin/bconsole + "${AWK}" pipeline and the AWK/ sed processing is preserved inside
the helper.
In `@systemtests/tests/ndmp-native/CMakeLists.txt`:
- Around line 91-96: The conditionals setting defaults for ndmp_data_agent_user
and ndmp_data_agent_password are redundant because those cache variables already
have non-empty defaults; either remove these if(NOT ndmp_data_agent_user) /
if(NOT ndmp_data_agent_password) blocks, or explicitly handle empty-string cases
by switching to an existence test (e.g., use if(NOT DEFINED
ndmp_data_agent_user) / if(NOT DEFINED ndmp_data_agent_password)) and add a
comment clarifying intent; update the block that mentions Clear similarly or
document why empty-string handling is required.
In `@systemtests/tests/ndmp-native/testrunner-08-restore-incremental-with-mark`:
- Line 65: The test currently has the line invoking check_for_zombie_jobs
commented out; either re-enable that call or explicitly document why it's
disabled by adding a TODO/NOTE above the commented line referencing the issue
tracker ID and a short rationale so it can be revisited; locate the commented
invocation of check_for_zombie_jobs in
testrunner-08-restore-incremental-with-mark and either uncomment the call to run
the check or add a clear TODO with the external issue reference and expected
condition for re-enabling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c849b43-2bd9-469a-957f-8b5d73238708
📒 Files selected for processing (11)
systemtests/scripts/functionssystemtests/scripts/functions-ndmp.insystemtests/scripts/invalidate_tapes.shsystemtests/tests/ndmp-bareos/etc/bareos/bareos-dir.d/storage/ndmp.conf.insystemtests/tests/ndmp-bareos/testrunner-08-restore-incremental-first-with-marksystemtests/tests/ndmp-bareos/testrunner-09-restore-incremental-last-with-filesystemtests/tests/ndmp-bareos/testrunner-10-copysystemtests/tests/ndmp-bareos/testrunner-11-compare-with-copysystemtests/tests/ndmp-native/CMakeLists.txtsystemtests/tests/ndmp-native/test-setupsystemtests/tests/ndmp-native/testrunner-08-restore-incremental-with-mark
🚧 Files skipped from review as they are similar to previous changes (4)
- systemtests/tests/ndmp-bareos/testrunner-10-copy
- systemtests/scripts/functions-ndmp.in
- systemtests/scripts/functions
- systemtests/tests/ndmp-native/test-setup
Comment on lines
+55
to
+59
| changer_status=$(echo "${line}" \ | ||
| | sed -e 's/Data Transfer Element \([0-9]\):Full (Storage Element \([0-9]*\).*/\1:\2/') | ||
| if [ -n "$changer_status" ]; then | ||
| dte=$(echo "${changer_status}" | awk -F: '{print $1}') | ||
| se=$(echo "${changer_status}" | awk -F: '{print $2}') | ||
| if [ -n "${changer_status}" ]; then | ||
| dte=$(echo "${changer_status}" | "${AWK}" -F: '{print $1}') | ||
| se=$(echo "${changer_status}" | "${AWK}" -F: '{print $2}') |
Contributor
There was a problem hiding this comment.
Make changer-status parsing fail-closed before unload.
At Line [55]–Line [59], substitution-only sed can return non-parsed input while still passing the non-empty check. That can feed invalid dte/se into mtx unload on unexpected status formats.
Proposed fix
- changer_status=$(echo "${line}" \
- | sed -e 's/Data Transfer Element \([0-9]\):Full (Storage Element \([0-9]*\).*/\1:\2/')
- if [ -n "${changer_status}" ]; then
+ changer_status=$(echo "${line}" \
+ | sed -n -e 's/^Data Transfer Element \([0-9]\+\):Full (Storage Element \([0-9]\+\).*/\1:\2/p')
+ if [ -n "${changer_status}" ]; then
dte=$(echo "${changer_status}" | "${AWK}" -F: '{print $1}')
se=$(echo "${changer_status}" | "${AWK}" -F: '{print $2}')
mtx -f "${changer_device}" unload "${se}" "${dte}"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| changer_status=$(echo "${line}" \ | |
| | sed -e 's/Data Transfer Element \([0-9]\):Full (Storage Element \([0-9]*\).*/\1:\2/') | |
| if [ -n "$changer_status" ]; then | |
| dte=$(echo "${changer_status}" | awk -F: '{print $1}') | |
| se=$(echo "${changer_status}" | awk -F: '{print $2}') | |
| if [ -n "${changer_status}" ]; then | |
| dte=$(echo "${changer_status}" | "${AWK}" -F: '{print $1}') | |
| se=$(echo "${changer_status}" | "${AWK}" -F: '{print $2}') | |
| changer_status=$(echo "${line}" \ | |
| | sed -n -e 's/^Data Transfer Element \([0-9]\+\):Full (Storage Element \([0-9]\+\).*/\1:\2/p') | |
| if [ -n "${changer_status}" ]; then | |
| dte=$(echo "${changer_status}" | "${AWK}" -F: '{print $1}') | |
| se=$(echo "${changer_status}" | "${AWK}" -F: '{print $2}') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@systemtests/scripts/invalidate_tapes.sh` around lines 55 - 59, The current
parsing uses sed substitution which returns the original line on no-match, so
changer_status can be non-empty and produce invalid dte/se for the subsequent
mtx unload; change the parsing so it only emits a value on a successful regex
match (e.g. use sed -n with the s///p form or switch to an awk extraction that
only prints when the pattern matches) so that changer_status is empty on
non-matching input and the existing if [ -n "${changer_status}" ] guard prevents
invalid dte/se values (update the lines that set changer_status, dte, and se
accordingly).
Comment on lines
+68
to
+91
| while read -r line && [ "${i}" -le "$LAST_SLOT_NUMBER" ]; do | ||
| if echo "${line}" | grep "$(printf 'Storage Element %d:Full\n' ${i})"; then | ||
| set -x | ||
| if mtx -f "${changer_device}" load "${i}" "${USE_TAPE_DEVICE}" \ | ||
| && mt -f "${tape_device}" rewind \ | ||
| && mt -f "${tape_device}" weof \ | ||
| && mtx -f "${changer_device}" unload "${i}" "${USE_TAPE_DEVICE}"; then | ||
| set +x | ||
| echo | ||
| ((i = i + 1)) | ||
| else | ||
| echo "error $?" | ||
| exit 1 | ||
| fi | ||
| done | ||
| fi | ||
| done < <(mtx -f "${changer_device}" status) | ||
|
|
||
| slots_ready=$((i - 1)) | ||
| slots_ready=$((i - 1)) | ||
|
|
||
| if [ ${slots_ready} -eq 0 ]; then | ||
| echo "Could not invalidate any tape" | ||
| else | ||
| echo "Invalidated ${slots_ready} tapes of autochanger ${changer_device}." | ||
| fi | ||
| } | ||
| if [ ${slots_ready} -eq 0 ]; then | ||
| echo "Could not invalidate any tape" | ||
| else | ||
| echo "Invalidated ${slots_ready} tapes of autochanger ${changer_device}." | ||
| fi |
Contributor
There was a problem hiding this comment.
Slot loop can skip valid tapes and misreport invalidation count.
At Line [68]–Line [83], i advances only after a successful match for the current slot. If slot i is empty but later slots are full, those later full slots are never processed. Also, Line [85] derives slots_ready from slot index (i - 1) instead of successful operations.
Proposed fix
- i="${FIRST_SLOT_NUMBER}"
- while read -r line && [ "${i}" -le "$LAST_SLOT_NUMBER" ]; do
- if echo "${line}" | grep "$(printf 'Storage Element %d:Full\n' ${i})"; then
+ invalidated_slots=0
+ while read -r line; do
+ i=$(echo "${line}" | sed -n 's/^Storage Element \([0-9]\+\):Full.*/\1/p')
+ [ -z "${i}" ] && continue
+ if [ "${i}" -lt "${FIRST_SLOT_NUMBER}" ] || [ "${i}" -gt "${LAST_SLOT_NUMBER}" ]; then
+ continue
+ fi
+ if echo "${line}" | grep "$(printf 'Storage Element %d:Full\n' "${i}")"; then
set -x
if mtx -f "${changer_device}" load "${i}" "${USE_TAPE_DEVICE}" \
&& mt -f "${tape_device}" rewind \
&& mt -f "${tape_device}" weof \
&& mtx -f "${changer_device}" unload "${i}" "${USE_TAPE_DEVICE}"; then
set +x
echo
- ((i = i + 1))
+ ((invalidated_slots = invalidated_slots + 1))
else
echo "error $?"
exit 1
fi
fi
done < <(mtx -f "${changer_device}" status)
- slots_ready=$((i - 1))
+ slots_ready="${invalidated_slots}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@systemtests/scripts/invalidate_tapes.sh` around lines 68 - 91, The loop
advances the slot index i only on successful invalidation which skips later full
slots and then computes slots_ready as i-1; change to iterate over fixed slot
indices and track successful invalidations separately: obtain the mtx status
output once (mtx -f "${changer_device}" status), loop over slot numbers from 1
to LAST_SLOT_NUMBER (or parse each status line but always increment the slot
index), for each slot check whether the corresponding status line indicates
"Full" and if so run mtx/mt commands (keeping set -x only around the command
group) and increment a new success counter (e.g., success_count) on success; do
not advance i conditionally, and set slots_ready=${success_count} (and keep
existing error handling when the mtx/mt/unload group fails).
| echo "################ restore incremental by mark #####################" | ||
| cat <<END_OF_DATA >"${tmp}/bconcmds" | ||
| @$out ${log_home}/restore-mark.out | ||
| restore jobid=2 client=ndmp-fd fileset=${NDMP_CLIENT}-fileset restorejob=NDMPRestoreDump |
Contributor
There was a problem hiding this comment.
Avoid hardcoding jobid=2 for the restore target.
Line 47 assumes the first incremental is always JobId 2. That becomes brittle once reruns/additional jobs are introduced (for example, full-job rerun flows), and this test may restore the wrong job while still appearing healthy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@systemtests/tests/ndmp-bareos/testrunner-08-restore-incremental-first-with-mark`
at line 47, The restore line hardcodes jobid=2 which is brittle; replace it with
a dynamic job-id lookup and use that variable in the restore invocation (e.g.,
compute the first incremental job id into a variable like
NDMP_FIRST_INCREMENTAL_JOBID or call the existing helper that finds incremental
jobs) and then change the restore command from "restore jobid=2 client=ndmp-fd
fileset=${NDMP_CLIENT}-fileset restorejob=NDMPRestoreDump" to use that variable
(jobid=${NDMP_FIRST_INCREMENTAL_JOBID} or equivalent). Locate the restore
invocation string and the test setup where incremental jobs are created (look
for the test name testrunner-08-restore-incremental-first-with-mark and any
helpers that list/query job ids) and wire the computed job id into the restore
step so the test always picks the correct first incremental.
|
|
||
| # we deterministically use one jobid and look that we found the corresponding incremental | ||
| # here the restore should be the first incremental | ||
| if ! ${SSH} "tail -c 200 \"${NDMP_TEST_PATH}/incremental\" | tail -n 1 | grep \"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-1\""; then |
Contributor
There was a problem hiding this comment.
Tighten the marker check to an exact fixed-string match.
Line 60 can false-pass: grep "${...}-1" also matches ...-10, and the IP dots are treated as regex wildcards. This weakens the core assertion of “first incremental”.
Proposed fix
-if ! ${SSH} "tail -c 200 \"${NDMP_TEST_PATH}/incremental\" | tail -n 1 | grep \"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-1\""; then
+if ! ${SSH} "tail -n 1 \"${NDMP_TEST_PATH}/incremental\" | grep -Fx \"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-1\""; then
echo "Restored file is not the expected first Incremental"
estat=1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ! ${SSH} "tail -c 200 \"${NDMP_TEST_PATH}/incremental\" | tail -n 1 | grep \"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-1\""; then | |
| if ! ${SSH} "tail -n 1 \"${NDMP_TEST_PATH}/incremental\" | grep -Fx \"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-1\""; then | |
| echo "Restored file is not the expected first Incremental" | |
| estat=1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@systemtests/tests/ndmp-bareos/testrunner-08-restore-incremental-first-with-mark`
at line 60, The current SSH check uses grep
"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-1" which can false-match e.g. "...-10"
and treats dots as regex; update the check to require an exact fixed-string
whole-line match by using grep with fixed-string and exact-match flags (e.g.,
grep -F -x) against the constructed marker
"${IP_ADDRESS_TO_ACCESS_NDMP_DATA_AGENT}-1" so the line read from
"${NDMP_TEST_PATH}/incremental" must exactly equal that marker.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ndmp: renew systemtest ndmp-bareos ndmp-native
-Dndmp_config parameterBased on CTestNDMPConfig.cmake.template you can create a file
with all the parameter set for being able to build and run systemtest
ndmp-bareos ndmp-native on either remote isilon or omnios.
new variable in Ctest config replace environment.local.in, which is
dropped
Setting DOFULLFAILED allow to trigger a first full without data
preparation which then will failed, and will be rerun automatically.
Checking for bug no Level in rerun
run copy, client and fileset were added due to misused of fileset id when the selection is created on first run. (fileset isilon is added to database while not used for omnios jobs)
systemtests/scripts/scripts/function-ndmpPlease check
If you have any questions or problems, please give a comment in the PR.
Helpful documentation and best practices
Checklist for the reviewer of the PR (will be processed by the Bareos team)
Make sure you check/merge the PR using
devtools/pr-toolto have some simple automated checks run and a proper changelog record added.General
Source code quality
Tests