From 3aa3259c111c6cbbc0a88d2fdc27bf87f6d825a9 Mon Sep 17 00:00:00 2001 From: Kautilya Tripathi Date: Wed, 29 Jul 2026 14:21:39 +0530 Subject: [PATCH] ceph-perf: fix classic compare crash and crimson SeaStore Drop cpu_cycles_per_op from injected acceptable rules so CBT compare does not TypeError on float(None) when perf is unavailable. Use {workload} in the GitHub report paths so HTML does not strip tags into "//". For crimson SeaStore without spare NVMes, attach sparse images via losetup and pass /dev/loop* (vstart rejects plain .img). Wipe only device contents; reattach detached loops when BACK-FILE is missing; tear down images only at job end. Signed-off-by: Kautilya Tripathi --- ceph-perf-pull-requests/README.rst | 14 ++- ceph-perf-pull-requests/build/cleanup | 1 + ceph-perf-pull-requests/build/common.sh | 110 +++++++++++++++--- ceph-perf-pull-requests/build/compare | 4 +- .../build/prepare-workloads.py | 45 ++++++- ceph-perf-pull-requests/build/run-cbt | 5 + ceph-perf-pull-requests/build/setup-pre.sh | 3 +- 7 files changed, 156 insertions(+), 26 deletions(-) diff --git a/ceph-perf-pull-requests/README.rst b/ceph-perf-pull-requests/README.rst index 9f792b3c2..d4432ff1a 100644 --- a/ceph-perf-pull-requests/README.rst +++ b/ceph-perf-pull-requests/README.rst @@ -40,7 +40,8 @@ The ``ceph-perf`` project generates two freestyle jobs: (pilot node ``o02``, see https://tracker.ceph.com/issues/78071 — so three 7.3T drives, never OS ``sda`` / small mounted disks). If fewer than 3 spare NVMes exist, creates three ~32GiB sparse images under - ``$WORKSPACE/seastore-imgs/`` and still runs SeaStore. + ``$WORKSPACE/seastore-imgs/`` and attaches them with ``losetup`` (vstart + requires writable **block** devices — plain ``.img`` files are rejected). Both run on ``performance`` nodes, build ``ceph-main`` and the PR merge ref (``WITH_CRIMSON=ON``, ``vstart-base`` + ``crimson-osd``; compiler selection via @@ -89,8 +90,9 @@ What the job does build console), so “View more details” keep working after the fact. On failure/success the job drops partial CBT archives (so they are never reused), -stops cluster processes, and wipes the job’s SeaStore NVMes (``wipefs`` + leading -zeros; nvme-only, never if mounted). Compare regressions fail both the GitHub +stops cluster processes, and wipes the job’s SeaStore block devices (``wipefs`` + +leading zeros on ``/dev/nvme*`` / ``/dev/loop*``; never if mounted). Loop-backed +images are detached only at job teardown. Compare regressions fail both the GitHub check **and** the Jenkins build. An archive is treated as complete (reusable, and accepted after a run) only if @@ -169,7 +171,11 @@ the crimson CBT YAML upstream) to reduce single-shot false positives:: iops_avg: (or (greater) (near 0.10)) latency_avg: (or (less) (near 0.10)) iops_stddev: (or (less) (near 2.00)) - cpu_cycles_per_op:(or (less) (near 0.10)) + +``cpu_cycles_per_op`` is intentionally **not** injected: classic/perf-basic uses +collectl (not perf), and on these nodes ``perf_event_paranoid>=1`` leaves that +metric as ``None``. CBT ``compare.py`` then crashes with ``TypeError: float(None)`` +while walking ``acceptable``. Jenkins console log order (easy to misread) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/ceph-perf-pull-requests/build/cleanup b/ceph-perf-pull-requests/build/cleanup index 9734cce17..771be4eeb 100644 --- a/ceph-perf-pull-requests/build/cleanup +++ b/ceph-perf-pull-requests/build/cleanup @@ -6,3 +6,4 @@ set +e # Best-effort after the WORKSPACE guard (wipe/pkill must not fail the publisher). kill_cluster_procs wipe_seastore_devs +teardown_seastore_devs diff --git a/ceph-perf-pull-requests/build/common.sh b/ceph-perf-pull-requests/build/common.sh index 78faf9f6c..2776e41d9 100644 --- a/ceph-perf-pull-requests/build/common.sh +++ b/ceph-perf-pull-requests/build/common.sh @@ -9,24 +9,22 @@ kill_cluster_procs() { done } -# Wipe SeaStore targets listed in $WORKSPACE/seastore-devs.txt: -# - /dev/nvme*: wipefs + leading zeros (never if mounted) -# - $WORKSPACE/seastore-imgs/* sparse images: delete -# Anything else is refused. +# Wipe *contents* of SeaStore block devices listed in $WORKSPACE/seastore-devs.txt. +# Accepts only /dev/nvme* and /dev/loop* (vstart requires writable block devices). +# Never detaches loop devices or deletes backing images here — those stay for the +# whole job; see teardown_seastore_devs for final cleanup. Pre-CBT wipe used to +# `rm` sparse .img paths, which broke SeaStore (ceph-perf-crimson #41). wipe_seastore_devs() { - # Require WORKSPACE so we never operate on /seastore-imgs or similar. : "${WORKSPACE:?}" local devs_file="${WORKSPACE}/seastore-devs.txt" - local img_dir="${WORKSPACE}/seastore-imgs" local dev mountpoints if test ! -s "$devs_file"; then - rm -rf "$img_dir" return 0 fi while IFS= read -r dev || test -n "$dev"; do test -n "$dev" || continue case "$dev" in - /dev/nvme*) + /dev/nvme*|/dev/loop*) if test ! -b "$dev"; then echo "skip wipe, not a block device: $dev" >&2 continue @@ -35,7 +33,6 @@ wipe_seastore_devs() { echo "REFUSING to wipe (lsblk not found to verify mounts): $dev" >&2 continue fi - # Fail closed: if lsblk cannot report mount state, do not wipe. if ! mountpoints=$(lsblk -n -o MOUNTPOINT "$dev" 2>/dev/null); then echo "REFUSING to wipe (lsblk failed to verify mounts): $dev" >&2 continue @@ -48,19 +45,106 @@ wipe_seastore_devs() { sudo wipefs -a "$dev" || true sudo dd if=/dev/zero of="$dev" bs=1M count=100 status=none conv=fsync || true ;; - "${WORKSPACE}/seastore-imgs"/*) - echo "Removing SeaStore sparse image $dev" - rm -f "$dev" || true - ;; *) echo "REFUSING to wipe unexpected SeaStore path: $dev" >&2 continue ;; esac done < "$devs_file" +} + +# Detach loop devices and remove sparse backing images (end of job / next job start). +teardown_seastore_devs() { + : "${WORKSPACE:?}" + local devs_file="${WORKSPACE}/seastore-devs.txt" + local img_dir="${WORKSPACE}/seastore-imgs" + local dev + if test -s "$devs_file"; then + while IFS= read -r dev || test -n "$dev"; do + test -n "$dev" || continue + case "$dev" in + /dev/loop*) + echo "Detaching SeaStore loop device $dev" + sudo losetup -d "$dev" || true + ;; + esac + done < "$devs_file" + fi rm -rf "$img_dir" } +# True if $1 is a writable block device. For /dev/loop*, also require an +# attached BACK-FILE: a detached loop node still passes `[ -b ] && [ -w ]` +# (vstart's check) after chmod a+rw, but SeaStore would see an empty device. +seastore_dev_ready() { + local dev=$1 + local back + test -b "$dev" && test -w "$dev" || return 1 + case "$dev" in + /dev/loop*) + back=$(sudo losetup -n -O BACK-FILE "$dev" 2>/dev/null || true) + # losetup prints a blank BACK-FILE line when the node is free. + printf '%s' "$back" | grep -q '[^[:space:]]' + ;; + *) + return 0 + ;; + esac +} + +# Ensure every seastore-devs entry is a writable block device (vstart requirement). +# Re-attaches workspace images to free loop devices if a prior wipe/teardown left +# them detached. Fails the job loudly if SeaStore cannot run. +ensure_seastore_devs() { + : "${WORKSPACE:?}" + local devs_file="${WORKSPACE}/seastore-devs.txt" + local img_dir="${WORKSPACE}/seastore-imgs" + local dev img i new_devs="" replaced=0 + if test ! -s "$devs_file"; then + return 0 + fi + i=0 + while IFS= read -r dev || test -n "$dev"; do + test -n "$dev" || continue + if seastore_dev_ready "$dev"; then + new_devs="${new_devs}${dev}"$'\n' + i=$((i + 1)) + continue + fi + img="${img_dir}/osd-${i}.img" + if test ! -f "$img"; then + echo "ERROR: SeaStore device $dev missing and no backing image $img" >&2 + return 1 + fi + echo "Re-attaching SeaStore image $img (was $dev)" + # detach stale association if any + sudo losetup -d "$dev" 2>/dev/null || true + dev=$(sudo losetup --show -f "$img") || { + echo "ERROR: losetup failed for $img" >&2 + return 1 + } + sudo chmod a+rw "$dev" || true + if ! seastore_dev_ready "$dev"; then + echo "ERROR: $dev is not a ready SeaStore block device after losetup" >&2 + return 1 + fi + new_devs="${new_devs}${dev}"$'\n' + replaced=1 + i=$((i + 1)) + done < "$devs_file" + if test "$replaced" -eq 1; then + printf '%s' "$new_devs" > "$devs_file" + # Refresh the run-cbt wrapper seastore-devs list if present. + if test -f "${WORKSPACE}/run-cbt-seastore.sh"; then + local csv + csv=$(printf '%s' "$new_devs" | paste -sd, -) + # Replace --seastore-devs with current csv (single occurrence). + sed -i -E "s|--seastore-devs [^ \\\]+|--seastore-devs ${csv}|" \ + "${WORKSPACE}/run-cbt-seastore.sh" + fi + fi +} + cleanup_vstart() { # Prefer stopping from the active tree if present. local tree diff --git a/ceph-perf-pull-requests/build/compare b/ceph-perf-pull-requests/build/compare index c713085fa..50f59e6c4 100644 --- a/ceph-perf-pull-requests/build/compare +++ b/ceph-perf-pull-requests/build/compare @@ -31,8 +31,8 @@ result=success echo "store: $store_tag" echo "baseline_sha: $sha_main" echo "pr_sha: $sha_pr" - echo "baseline_archives: ${ARCHIVE_MAIN}//$store_tag/$sha_main/" - echo "pr_archives: ${ARCHIVE_PR}//$store_tag/$sha_pr/" + echo "baseline_archives: ${ARCHIVE_MAIN}/{workload}/$store_tag/$sha_main/" + echo "pr_archives: ${ARCHIVE_PR}/{workload}/$store_tag/$sha_pr/" if test -f "${WORKSPACE}/perf-meta.txt"; then cat "${WORKSPACE}/perf-meta.txt" fi diff --git a/ceph-perf-pull-requests/build/prepare-workloads.py b/ceph-perf-pull-requests/build/prepare-workloads.py index e86267e76..629569b3a 100644 --- a/ceph-perf-pull-requests/build/prepare-workloads.py +++ b/ceph-perf-pull-requests/build/prepare-workloads.py @@ -96,12 +96,15 @@ def main() -> int: out.mkdir(parents=True, exist_ok=True) main_src = ws / "ceph-main" # 10% near-tolerance to reduce single-shot false positives vs upstream 5%. + # Do NOT inject cpu_cycles_per_op: classic/perf-basic uses collectl (not perf), + # and on performance nodes perf_event_paranoid>=1 leaves that metric as None. + # CBT compare.py then crashes with TypeError: float(None) while evaluating + # acceptable rules (see ceph-perf-classic #8710 / ceph#68408). acceptable = { "bandwidth": "(or (greater) (near 0.10))", "iops_avg": "(or (greater) (near 0.10))", "iops_stddev": "(or (less) (near 2.00))", "latency_avg": "(or (less) (near 0.10))", - "cpu_cycles_per_op": "(or (less) (near 0.10))", } meta: list[str] = [] devs_file = ws / "seastore-devs.txt" @@ -131,7 +134,10 @@ def main() -> int: meta.append(f"{name}_source={src}") meta.append("workloads=read,write") # Crimson always uses SeaStore (never CyanStore). Prefer 3 spare NVMes; - # otherwise create workspace sparse images so the job still runs SeaStore. + # otherwise create sparse images attached via losetup (vstart requires + # writable *block* devices — plain .img files are rejected, see + # ceph-perf-crimson #41: "All --seastore-devs must refer to writable + # block devices"). store_tag = "seastore" nvmes: list[tuple[int, str]] = [] for line in os.popen( @@ -168,16 +174,43 @@ def main() -> int: chosen = [] for i in range(3): img = img_dir / f"osd-{i}.img" - # Create/resize sparse image without allocating full size. with open(img, "wb") as fh: fh.truncate(sparse_bytes) - chosen.append(str(img)) - meta.append("seastore_backend=sparse-file") + try: + loop = subprocess.check_output( + ["sudo", "losetup", "--show", "-f", str(img)], + text=True, + ).strip() + except (subprocess.CalledProcessError, FileNotFoundError) as exc: + raise SystemExit( + f"losetup failed for {img} (SeaStore needs block devices): {exc}" + ) from exc + if not loop.startswith("/dev/loop"): + raise SystemExit(f"unexpected losetup output for {img}: {loop!r}") + chosen.append(loop) + meta.append(f"seastore_loop={loop}:{img}") + meta.append("seastore_backend=loop") meta.append(f"seastore_sparse_bytes={sparse_bytes}") meta.append( f"seastore_note=only {len(nvmes)} unmounted NVMe(s); " - "using workspace sparse images" + "using losetup-backed sparse images" ) + # vstart checks `[ -b ] && [ -w ]` without sudo. + import stat as stat_mod + + for path in chosen: + subprocess.run(["sudo", "chmod", "a+rw", path], check=False) + try: + mode = os.stat(path).st_mode + except OSError as exc: + raise SystemExit(f"SeaStore path missing after setup: {path}: {exc}") from exc + if not stat_mod.S_ISBLK(mode): + raise SystemExit(f"SeaStore path is not a block device: {path}") + if not os.access(path, os.W_OK): + raise SystemExit( + f"SeaStore path is not writable by this user (vstart requires " + f"-w): {path}" + ) devs = ",".join(chosen) src_sh = (main_src / "src/script/run-cbt.sh").read_text() old = ( diff --git a/ceph-perf-pull-requests/build/run-cbt b/ceph-perf-pull-requests/build/run-cbt index dc2b77bac..16a0af892 100644 --- a/ceph-perf-pull-requests/build/run-cbt +++ b/ceph-perf-pull-requests/build/run-cbt @@ -79,6 +79,11 @@ cleanup_all() { trap cleanup_all EXIT cleanup_vstart_local +# vstart requires writable block devices. Re-attach loop-backed images if needed, +# then wipe contents (never delete/detach here — that broke crimson #41). +if test "${OSD_FLAVOR}" != "classic"; then + ensure_seastore_devs || fail_and_drop_archive 1 +fi wipe_seastore_devs sleep 2 diff --git a/ceph-perf-pull-requests/build/setup-pre.sh b/ceph-perf-pull-requests/build/setup-pre.sh index ce5958424..86d123c8b 100644 --- a/ceph-perf-pull-requests/build/setup-pre.sh +++ b/ceph-perf-pull-requests/build/setup-pre.sh @@ -49,7 +49,8 @@ fedora) esac kill_cluster_procs -# Previous build may have left SeaStore devices dirty. +# Previous build may have left SeaStore devices dirty / loop attachments around. +teardown_seastore_devs wipe_seastore_devs mkdir -p "${WORKSPACE}/perf-workloads" -- 2.47.3