Feature/rgw integration and monitoring tools

[jrse]

feat(contrib): add RGW integration and developer observability tools

Add three developer CLI tools for testing and monitoring the external
arbiter setup:

• arbiter-health: architecture and health dashboard showing
  CephCluster status, mon quorum, RGW object store (optional), RemoteArbiter
  conditions, and Kubernetes-level fallback assessment
• arbiter-viz: Hubble-based traffic topology visualization for
  arbiter mon communication (msgr1/msgr2 flows, ASCII diagram)
• arbiter-s3-bench: S3 benchmarking tool for Ceph RGW with write,
  read, verify, loop, and cleanup modes (for Development tests only!)

Supporting changes:

• Add CephObjectStore and CephObjectStoreUser example manifests (setup rgw)
• Add second OSD disk and awscli provisioning to Lima VM (for osd out tests)
• Document RGW deployment and S3 bench usage in README

[Copilot]

🟡 Changes recommended

Unresolved moderate and critical issues remain in the health and S3 tooling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds RGW integration and developer tooling for testing and observing external Ceph arbiter deployments.

Changes:

• Adds health, Hubble visualization, and S3 benchmarking tools.
• Adds RGW manifests and S3 usage documentation.
• Extends Lima VM provisioning with an additional OSD disk and AWS CLI.

File summaries

File	Description
README.md	Documents RGW setup and S3 benchmarking
contrib/vm.yaml	Adds OSD storage and AWS CLI provisioning
contrib/tools/arbiter-viz	Visualizes Hubble traffic
contrib/tools/arbiter-s3-bench	Generates and verifies S3 traffic
contrib/tools/arbiter-health	Displays Ceph and arbiter health
contrib/k8s/examples/object-store.yaml	Defines the RGW object store
contrib/k8s/examples/object-store-user.yaml	Defines S3 test credentials

Review details

Suppressed comments (10)

README.md:103

• Waiting for the CephObjectStore does not wait for the CephObjectStoreUser controller to create rook-ceph-object-user-my-store-test-user. The immediately following bench invocation fetches that secret only once, so a normal reconciliation delay can make the documented setup fail with missing credentials. Wait for the generated secret (or the user resource's Ready condition) before starting the benchmark.

# wait for RGW to be ready
kubectl wait --for=jsonpath='{.status.phase}'=Ready cephobjectstore/my-store -n rook-ceph --timeout=300s
# run S3 bench (write + read + verify 50 objects)
limactl shell k8s bash ./contrib/tools/arbiter-s3-bench --verify

contrib/tools/arbiter-health:220

• The service lookup has the same source-context problem: kget queries the operator's current cluster, not the RemoteCluster referenced by the CR. In the supported independent-cluster topology this leaves the service section empty or incorrectly says the service is not found. Use the remote cluster client/context for this query.

  if [[ -n "$rc_ns" ]]; then
    arbiter_svc_info=$(kget get svc -n "$rc_ns" \
      -l "ceph.cobaltcore.sap.com/lookup=$ARBITER_NAME" \
      -o jsonpath='{range .items[*]}{.metadata.name}{" "}{.spec.type}{" "}{.spec.clusterIP}{"\n"}{end}')

contrib/tools/arbiter-health:182

• quorum_status returns quorum_names after the monmap object, so the mons array is not the final JSON member. This sed expression therefore produces no names; with set -euo pipefail, the following grep -o also makes render abort instead of showing the dashboard whenever Ceph quorum data is available. Parse the JSON with a real parser or extract the bounded monmap.mons object without requiring end-of-document placement.

    ceph_monmap_names=$(printf '%s' "$ceph_quorum" \
      | sed -n 's/.*"mons":\[\(.*\)\]\s*}\s*}$/\1/p' \
      | grep -o '"name":"[^"]*"' \
      | sed 's/"name":"//g; s/"//g' \
      | tr '\n' ',' | sed 's/,$//')

contrib/tools/arbiter-health:294

• msgr2 is not inherently encrypted; Ceph encryption depends on the configured messenger mode. This unconditionally reports encryption even for configurations using the default CRC mode, which misstates the security posture. Describe secure mode as configurable or query the cluster's configured mode.

  printf "  |         msgr2 :3300  ${DIM}(v2 protocol, encrypted, preferred)${RESET}\n"
  printf "  |         msgr1 :6789  ${DIM}(v1 legacy, unencrypted, fallback)${RESET}\n"

contrib/tools/arbiter-health:436

• This fallback is entered for any failed or empty kubectl exec, including a missing tools pod, authentication failure, or timeout; none of those proves quorum loss or that the cluster is read-only. The warning can therefore lead to the wrong remediation. Label this as Ceph status unavailable/Kubernetes-only and reserve a quorum-loss claim for parsed Ceph data.

    printf "  ${YELLOW}[WARN]  Ceph cluster unreachable (quorum likely lost, cluster is read-only)${RESET}\n"

contrib/tools/arbiter-health:204

• Rook RGW deployments use the object-store label (for example rook_object_store=my-store); ceph_daemon_id is the daemon ID such as rgw.my-store.a, not the store name. This selector therefore returns no RGW deployments, so the dashboard silently omits their readiness. Select by rook_object_store (or the actual ceph_daemon_id prefix).

    rgw_deploy_info=$(kget get deploy -n "$ra_ceph_ns" \
      -l "app=rook-ceph-rgw,ceph_daemon_id=${rgw_store_name}" \
      -o jsonpath='{range .items[*]}{.metadata.name}{" "}{.status.readyReplicas}{"/"}{.status.replicas}{"\n"}{end}')

contrib/tools/arbiter-health:399

• When Ceph reports an empty quorum, grep -c . prints 0 but exits with status 1, so the || echo 0 appends a second 0 to quorum_count. The subsequent numeric tests then receive 0\n0 and the no-quorum dashboard can emit an arithmetic/printf error instead of an assessment. Make the fallback preserve the single zero value.

    quorum_count=$(echo "$ceph_mon_list" | tr ',' '\n' | grep -c . || echo 0)

contrib/tools/arbiter-s3-bench:326

• Under set -e, a non-loop cycle that returns any read/write error exits at this command before reaching the --cleanup block. Therefore --cleanup is skipped exactly when a failed benchmark most needs cleanup. Capture the cycle status, perform cleanup, then return the captured status.

  run_cycle 1

contrib/tools/arbiter-s3-bench:244

• --read-only skips generate_objects, so no ${TMPDIR}/obj-*.sha256 files exist. Combining it with --verify therefore compares every downloaded object against an empty expected digest and reports false mismatches instead of verifying integrity. Reject this incompatible combination or provide expected digests for read-only inputs.

  if [[ "$VERIFY" == "true" && "$errors" -eq 0 ]]; then
    printf "\n${BOLD}--- VERIFY ---${RESET}\n"
    for i in $(seq 1 "$NUM_OBJECTS"); do
      local expected actual
      expected=$(cat "${TMPDIR}/obj-${i}.sha256")
      actual=$(sha256sum "${TMPDIR}/read/obj-${i}" | awk '{print $1}')

contrib/tools/arbiter-viz:290

• Because all Ceph pods share the rook-ceph namespace, this condition classifies RGW, OSD, toolbox, and other non-monitor nodes as mons. The connection diagram will therefore render unrelated Ceph traffic as monitor connections. Match the monitor pod/deployment name or a monitor-specific label instead of the namespace.

    if (index(nd, "mon") > 0 || index(nd, "rook-ceph") > 0) {
      mons[mon_count] = nd; mon_count++

• Files reviewed: 7/7 changed files
• Comments generated: 9
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate issues affect health reporting, S3 benchmark correctness, and tool behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (17)

Previously missed (1) — in code that hasn't changed since the last review.

contrib/tools/arbiter-s3-bench:244

• --read-only skips generate_objects, so no ${TMPDIR}/obj-*.sha256 files exist. Combining read-only with --verify therefore makes cat fail under set -e and terminates the benchmark before its summary; reject this combination up front or provide expected digests for existing objects.

README.md:104

• This loop command blocks until Ctrl+C, so the Hubble enable/port-forward commands below are not run while traffic is being generated; after Ctrl+C there is no loop traffic left to observe. Set up Hubble before starting the loop or explicitly tell the user to run it in a second terminal.

# run S3 bench in loop mode (continuous traffic until Ctrl+C)

contrib/tools/arbiter-health:399

• When quorum_names is empty, grep -c . prints 0 and returns 1, so the || echo 0 appends a second 0. quorum_count then contains a newline and the arithmetic comparisons below cannot reliably render the quorum-lost assessment. Use a counter that emits exactly one value for an empty list.

    quorum_count=$(echo "$ceph_mon_list" | tr ',' '\n' | grep -c . || echo 0)

contrib/tools/arbiter-health:213

• kget always invokes the current kubectl context, but rc_ns is the namespace from the target RemoteCluster and the operator reaches that cluster through the kubeconfig secret. With an independent target cluster, these calls query the source cluster and falsely report the arbiter deployment as missing, which also corrupts the fallback assessment. Use the target kubeconfig/context for these remote resources or make that requirement explicit.

  if [[ -n "$rc_ns" ]]; then
    arbiter_deploy_info=$(kget get deploy -n "$rc_ns" \
      -l "ceph.cobaltcore.sap.com/lookup=$ARBITER_NAME" \
      -o jsonpath='{range .items[*]}{.metadata.name}{" "}{.status.readyReplicas}{"/"}{.status.replicas}{" "}{.spec.template.spec.containers[0].image}{"\n"}{end}')
  fi

contrib/tools/arbiter-health:51

• The unknown-option branch calls usage, whose unconditional exit 0 makes invalid arguments return success after printing an error. Exit nonzero for this path while keeping --help successful so automation can detect a bad invocation.

    -h|--help)         usage ;;
    *)                 echo "Unknown option: $1" >&2; usage ;;

contrib/tools/arbiter-health:220

• This service lookup has the same context problem: kget queries the source cluster even though the labeled service is created in the remote cluster. In the documented split-cluster architecture this will always fall through to the misleading "configured ... but not found"/not-found output. Query the remote cluster context rather than the current kubectl context.

    arbiter_svc_info=$(kget get svc -n "$rc_ns" \
      -l "ceph.cobaltcore.sap.com/lookup=$ARBITER_NAME" \
      -o jsonpath='{range .items[*]}{.metadata.name}{" "}{.spec.type}{" "}{.spec.clusterIP}{"\n"}{end}')

contrib/tools/arbiter-health:108

• The ra_json/awk block is dead: ra_state is assigned again on line 112, so this response and parser are discarded. In watch mode that adds a redundant API request on every refresh and leaves misleading parsing logic. Remove the unused fetch/parser and keep the single jsonpath assignment.

  ra_state=$(echo "$ra_json" | awk -v Q='"' 'BEGIN{RS=""; FS="\n"} {
    needle = Q "state" Q ":" Q
    pos = index($0, needle)
    if (pos == 0) exit
    rest = substr($0, pos + length(needle))

contrib/tools/arbiter-health:436

• An empty ceph_quorum here only means both kubectl exec attempts produced no stdout; that also happens when the tools/mon pod is missing, the command is unavailable, or RBAC denies access. It does not establish that quorum is lost or that the cluster is read-only, so this warning can report a false outage; label the result as unavailable and keep the Kubernetes data as an estimate.

    printf "  ${YELLOW}[WARN]  Ceph cluster unreachable (quorum likely lost, cluster is read-only)${RESET}\n"

contrib/tools/arbiter-s3-bench:174

• --read-only is documented as reading existing objects, but a missing bucket enters this branch and executes s3 mb, mutating RGW. In read-only mode, treat a missing bucket as an error and only create it for normal/write modes.

  else
    printf "Creating bucket: %s\n" "$BUCKET"
    aws --endpoint-url "$S3_ENDPOINT" --no-verify-ssl \
      s3 mb "s3://${BUCKET}" 2>&1 | grep -v "InsecureRequestWarning" || true
  fi

contrib/tools/arbiter-s3-bench:60

• The parser accepts --read-only and --write-only together. That combination skips both the write and read blocks in run_cycle, then reports a successful cycle without performing any benchmark operation. Reject these mutually exclusive options after parsing.

    --read-only)       READ_ONLY=true; shift ;;
    --write-only)      WRITE_ONLY=true; shift ;;
    --verify)          VERIFY=true; shift ;;

contrib/tools/arbiter-s3-bench:317

• The SIGINT handler exits directly, bypassing the cleanup block at lines 327-329. Since --loop is documented to end with Ctrl+C, --loop --cleanup never deletes the bucket or its objects; invoke cleanup_bucket from this termination path when cleanup was requested.

  trap 'printf "\n${BOLD}Stopped after %d cycles (%d total errors)${RESET}\n" "$cycle" "$total_cycles_errors"; exit 0' INT

contrib/tools/arbiter-s3-bench:261

• This read/verify path has the same shell-status overflow: 256 or more failures wraps to status 0 and is treated as success by run_cycle. Keep the full count in data returned to the caller rather than using it directly as a Bash function exit status.

  return "$((errors + verify_errors))"

contrib/tools/arbiter-s3-bench:67

• The unknown-option branch calls usage, whose unconditional exit 0 makes invalid arguments return success after printing an error. Exit nonzero for this path while keeping --help successful so automation can detect a bad invocation.

    -h|--help)         usage ;;
    *)                 echo "Unknown option: $1" >&2; usage ;;

contrib/tools/arbiter-s3-bench:88

• The documented sequence creates the CephObjectStoreUser and immediately runs this one-shot secret lookup after waiting only for the store. Rook creates the user secret asynchronously, so this can fail transiently even though the manifests are correct; wait/retry for the generated secret (or wait on the user resource) before exiting.

AWS_ACCESS_KEY_ID=$(kubectl get secret "$SECRET_NAME" -n "$CEPH_NS" \
  -o jsonpath='{.data.AccessKey}' 2>/dev/null | base64 -d) || true
AWS_SECRET_ACCESS_KEY=$(kubectl get secret "$SECRET_NAME" -n "$CEPH_NS" \
  -o jsonpath='{.data.SecretKey}' 2>/dev/null | base64 -d) || true

contrib/tools/arbiter-viz:80

• The one-shot path explicitly collects both --to-label and --from-label flows, but follow mode subscribes only to --to-label. Live mode therefore omits arbiter-to-monitor responses and cannot show the bidirectional monitor topology advertised by this tool; use concurrent directional subscriptions or an equivalent endpoint filter.

if [[ "$FOLLOW" == "true" ]]; then
  run_hubble observe \
    --namespace "$NAMESPACE" \
    --to-label "$LABEL_SELECTOR" \
    -o json \
    --follow 2>/dev/null

contrib/tools/arbiter-viz:44

• The unknown-option branch calls usage, whose unconditional exit 0 makes a typo such as --cont return success even though an error was printed. Exit nonzero for invalid arguments while keeping --help successful so callers can detect configuration errors.

    -h|--help)       usage ;;
    *)               echo "Unknown option: $1" >&2; usage ;;

contrib/vm.yaml:9

• The setup instructions still create only the osd Lima disk (README.md:61), but this new disk is also declared with format: false. A fresh limactl create therefore has no provisioned osd2 disk for Rook and may fail before the VM starts. Add creation of osd2 to the documented setup (or use a self-provisioning disk declaration).

  - name: osd2
    format: false

• Files reviewed: 7/7 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved moderate issues affect health detection, visualization filtering, benchmark behavior, and VM provisioning.

Review details

Suppressed comments (19)

Previously missed (3) — in code that hasn't changed since the last review.

contrib/tools/arbiter-s3-bench:243

• --read-only --verify skips generate_objects, so no obj-*.sha256 files are created. After a successful download this unconditional cat therefore aborts the script under set -e instead of completing the cycle; reject this incompatible combination or add an explicit source for expected checksums.
  contrib/tools/arbiter-s3-bench:329
• If a non-loop cycle has any write, read, or verification error, run_cycle returns nonzero and set -e exits at this command before the --cleanup block below is reached. A failed or partial benchmark therefore leaves its bucket behind despite requesting cleanup; capture the status, run cleanup, and then return the saved status.
  contrib/tools/arbiter-viz:80
• One-shot mode explicitly collects both directions, but follow mode applies only --to-label. While following, arbiter-to-mon flows are therefore omitted, so the live topology is not bidirectional as the tool's normal collection path is. Add an equivalent merged --from-label stream (or an OR filter) to follow mode.

README.md:101

• Waiting for the CephObjectStore to become Ready does not guarantee that the CephObjectStoreUser controller has created rook-ceph-object-user-my-store-test-user. The next command fetches that secret immediately and can fail during normal reconciliation; wait for the user secret as well (or make the benchmark retry).

kubectl wait --for=jsonpath='{.status.phase}'=Ready cephobjectstore/my-store -n rook-ceph --timeout=300s

contrib/tools/arbiter-health:191

• These mon lists are filtered only by ceph_daemon_type=mon, so a namespace containing more than one CephCluster will mix unrelated monitor deployments into this cluster's readiness and Kubernetes fallback counts. The reconciler scopes the same lookup with app.kubernetes.io/part-of (pkg/controller/remotearbiter_controller.go:1015-1019); apply the same cluster selector here.

  rook_mons=$(kget get deploy -n "$ra_ceph_ns" -l ceph_daemon_type=mon \
    -o jsonpath='{range .items[*]}{.metadata.name}{" "}{.status.readyReplicas}{"/"}{.status.replicas}{"\n"}{end}')
  # Get mon IDs and readiness for quorum fallback assessment
  rook_mon_details=$(kget get deploy -n "$ra_ceph_ns" -l ceph_daemon_type=mon \
    -o jsonpath='{range .items[*]}{.metadata.labels.ceph_daemon_id}{" "}{.status.readyReplicas}{" "}{.status.replicas}{"\n"}{end}')

contrib/tools/arbiter-health:399

• When quorum_names is empty, grep -c . already prints 0 and exits nonzero; the || echo 0 adds a second 0, leaving quorum_count as a multi-line value. This is the path needed to report a lost quorum, but the subsequent numeric comparisons and output then receive a non-numeric count.

    quorum_count=$(echo "$ceph_mon_list" | tr ',' '\n' | grep -c . || echo 0)

contrib/tools/arbiter-health:159

• The fallback monitor pod lookup has the same cross-cluster issue: when the tools deployment is unavailable, it can execute ceph quorum_status in a monitor belonging to a different CephCluster in the namespace. Include the target cluster label in this selector.

    mon_pod=$(kget get pod -n "$ra_ceph_ns" -l ceph_daemon_type=mon \

contrib/tools/arbiter-health:182

• ceph quorum_status -f json places quorum_names after the monmap.mons array, so the array is followed by ,"quorum_names"..., not by the end of the JSON object. This sed expression therefore produces no monmap names, and a monitor that is present in the monmap but absent from quorum is incorrectly reported as not in the monmap.

    ceph_monmap_names=$(printf '%s' "$ceph_quorum" \
      | sed -n 's/.*"mons":\[\(.*\)\]\s*}\s*}$/\1/p' \
      | grep -o '"name":"[^"]*"' \
      | sed 's/"name":"//g; s/"//g' \
      | tr '\n' ',' | sed 's/,$//')

contrib/tools/arbiter-health:212

• rc_ns is the namespace in the referenced remote cluster, but kget always calls the current kubectl context. In a genuine separate-cluster deployment, this lookup therefore cannot see the arbiter Deployment and the dashboard/fallback assessment reports it as missing even when it is healthy. Load and use the RemoteCluster kubeconfig (or an explicit remote context) for this query.

    arbiter_deploy_info=$(kget get deploy -n "$rc_ns" \
      -l "ceph.cobaltcore.sap.com/lookup=$ARBITER_NAME" \
      -o jsonpath='{range .items[*]}{.metadata.name}{" "}{.status.readyReplicas}{"/"}{.status.replicas}{" "}{.spec.template.spec.containers[0].image}{"\n"}{end}')

contrib/tools/arbiter-health:220

• This Service lookup has the same context problem as the Deployment lookup above: rc_ns belongs to the remote cluster, while kget queries only the current cluster. With separate clusters, the dashboard will incorrectly show the arbiter service as missing. Use the RemoteCluster kubeconfig or an explicit remote context for this call.

    arbiter_svc_info=$(kget get svc -n "$rc_ns" \
      -l "ceph.cobaltcore.sap.com/lookup=$ARBITER_NAME" \
      -o jsonpath='{range .items[*]}{.metadata.name}{" "}{.spec.type}{" "}{.spec.clusterIP}{"\n"}{end}')

contrib/tools/arbiter-health:204

• Rook names RGW deployments with an instance suffix (for example, rook-ceph-rgw-my-store-a) and uses that full daemon ID in ceph_daemon_id; the CephObjectStore name here is only my-store. This exact selector therefore returns no gateway deployment and the dashboard omits RGW readiness even when the store is running. Match the generated deployment name/prefix (or otherwise filter the RGW daemon IDs) instead of this exact store name.

    rgw_deploy_info=$(kget get deploy -n "$ra_ceph_ns" \
      -l "app=rook-ceph-rgw,ceph_daemon_id=${rgw_store_name}" \
      -o jsonpath='{range .items[*]}{.metadata.name}{" "}{.status.readyReplicas}{"/"}{.status.replicas}{"\n"}{end}')

contrib/tools/arbiter-health:51

• Invalid options are reported as errors, but this branch calls usage, which exits with status 0. Scripts cannot distinguish an invalid invocation from successful --help; return a non-zero usage error here, as the S3 tool does.

    *)                 echo "Unknown option: $1" >&2; exit 2 ;;

contrib/tools/arbiter-s3-bench:173

• A failed bucket creation is hidden by the pipeline and || true, so the script proceeds as if setup succeeded and only reports per-object failures later. Return the aws s3 mb status instead of discarding it; otherwise an unavailable RGW or invalid bucket can make the benchmark's setup result misleading.

  if aws --endpoint-url "$S3_ENDPOINT" --no-verify-ssl \
    s3api head-bucket --bucket "$BUCKET" >/dev/null 2>&1; then

contrib/tools/arbiter-s3-bench:317

• The loop-mode interrupt trap exits immediately, so the cleanup block below the loop is unreachable. Consequently --loop --cleanup never deletes the bucket when the documented Ctrl+C stop is used. Run cleanup_bucket from this trap when cleanup was requested before exiting.

if [[ "$READ_ONLY" != "true" ]]; then

contrib/tools/arbiter-s3-bench:312

• --read-only is documented as only reading existing objects, but the unconditional ensure_bucket call can create the bucket when head-bucket fails. This mutates RGW state before any read. Skip bucket creation in read-only mode (while still allowing an informative missing-bucket error).

# Main

contrib/tools/arbiter-s3-bench:133

• The capability check only tests the exit status of date; BSD/macOS date accepts the format but emits a literal %N, so this branch can return a timestamp such as ...%N. The later arithmetic on end - start then fails even though the function claims to have a fallback. Validate that the output is numeric before using the nanosecond form.

}

contrib/tools/arbiter-viz:289

• The rook-ceph substring is also present in RGW, OSD, toolbox, and other Rook pod names, so this classifies every such node as a monitor. For example, RGW traffic will render rook-ceph-rgw-my-store-a in the monitor list. Match the monitor pod pattern or daemon identity instead.

    if (index(nd, "mon") > 0 || index(nd, "rook-ceph") > 0) {

contrib/tools/arbiter-viz:44

• Invalid options are reported as errors, but this branch calls usage, which exits with status 0. Scripts cannot distinguish an invalid invocation from successful --help (the S3 tool already uses status 2); return a non-zero usage error here.

    *)               echo "Unknown option: $1" >&2; exit 2 ;;

contrib/vm.yaml:9

• additionalDisks now references osd2, but the documented setup only creates the osd Lima disk before limactl create (README.md:61). Lima does not create a named additional disk from this entry, so a fresh VM setup will fail or lack the second disk needed by the OSD tests. Add and document a matching limactl disk create osd2 --size=... step before creating the VM.

  - name: osd2
    format: false

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved moderate issues affect setup reliability, health reporting, benchmarking correctness, cleanup safety, and VM provisioning.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (14)

Previously missed (2) — in code that hasn't changed since the last review.

contrib/tools/arbiter-health:203

• Rook's RGW deployment ceph_daemon_id is an instance identifier such as rgw.my-store.a, not just the object-store name. This selector therefore returns no RGW deployments, so the dashboard never reports pod readiness. Select the store using Rook's rook_object_store label instead.
  contrib/tools/arbiter-viz:80
• Normal mode collects both directions, but follow mode only applies --to-label. Because monitor traffic is bidirectional, live mode drops arbiter-to-monitor flows and cannot show the complete topology. Run both direction streams (or use a combined filter) before rendering.

README.md:103

• Creating a CephObjectStoreUser is asynchronous and creates the credentials Secret separately. The instructions wait only for the object store, then immediately run the benchmark, so a fresh deployment can fail at credential lookup even though both manifests were applied. Wait for rook-ceph-object-user-my-store-test-user (or the user Ready condition) before invoking the bench.

kubectl apply -f ./contrib/k8s/examples/object-store-user.yaml
# wait for RGW to be ready
kubectl wait --for=jsonpath='{.status.phase}'=Ready cephobjectstore/my-store -n rook-ceph --timeout=300s
# run S3 bench (write + read + verify 50 objects)
limactl shell k8s bash ./contrib/tools/arbiter-s3-bench --verify

contrib/tools/arbiter-health:212

• These lookups use the current kubectl context, but the controller creates the arbiter deployment and service through RemoteCluster's remote client, which may target a different API server. In that normal remote-cluster case the dashboard always shows the arbiter as missing/unready and the fallback quorum assessment is wrong; query the target cluster with its kubeconfig/context or explicitly report that remote data is unavailable.

    arbiter_deploy_info=$(kget get deploy -n "$rc_ns" \
      -l "ceph.cobaltcore.sap.com/lookup=$ARBITER_NAME" \
      -o jsonpath='{range .items[*]}{.metadata.name}{" "}{.status.readyReplicas}{"/"}{.status.replicas}{" "}{.spec.template.spec.containers[0].image}{"\n"}{end}')

contrib/tools/arbiter-health:179

• \s is a GNU sed extension, not a portable expression. On the macOS/BSD sed commonly used with this Lima-based developer setup, this pattern does not match the end of the mons array, leaving ceph_monmap_names empty and causing the dashboard to report an arbiter that is present in the monmap as absent. Use a POSIX character class such as [[:space:]] or a JSON parser.

      | sed -n 's/.*"mons":\[\(.*\)\]\s*}\s*}$/\1/p' \

contrib/tools/arbiter-health:182

• This regex only allows one closing brace after the mons array (]}), but mons is nested under monmap, so quorum_status closes both the monmap and top-level objects (]}}). The match therefore fails and ceph_monmap_names stays empty; an arbiter that is in the monmap but not in quorum is then falsely reported as not in the monmap. Extract the nested object structurally or match both closures.

    ceph_monmap_names=$(printf '%s' "$ceph_quorum" \
      | sed -n 's/.*"mons":\[\(.*\)\]\s*}\s*}$/\1/p' \
      | grep -o '"name":"[^"]*"' \
      | sed 's/"name":"//g; s/"//g' \
      | tr '\n' ',' | sed 's/,$//')

contrib/tools/arbiter-health:220

• This service lookup has the same context problem as the deployment lookup above: rc_ns belongs to the remote cluster, but this kubectl command uses the source/current context. It will show no arbiter service for a genuinely separate target cluster. Run it with the remote cluster's kubeconfig/context.

    arbiter_svc_info=$(kget get svc -n "$rc_ns" \
      -l "ceph.cobaltcore.sap.com/lookup=$ARBITER_NAME" \
      -o jsonpath='{range .items[*]}{.metadata.name}{" "}{.spec.type}{" "}{.spec.clusterIP}{"\n"}{end}')

contrib/tools/arbiter-s3-bench:178

• The aws s3 mb status is discarded by the pipeline and unconditional || true. A permission error or unavailable RGW is therefore treated as successful bucket setup, and the benchmark proceeds to report secondary upload failures instead of failing at initialization. Preserve the bucket-creation exit status here.

    aws --endpoint-url "$S3_ENDPOINT" --no-verify-ssl \
      s3 mb "s3://${BUCKET}" 2>&1 | grep -v "InsecureRequestWarning" || true

contrib/tools/arbiter-s3-bench:277

• The cleanup path recursively deletes every object in the caller-selected bucket and then removes the bucket. Since ensure_bucket also accepts an existing bucket, --cleanup can destroy data that this run did not create; use a run-specific prefix/owned bucket or require an explicit confirmation for pre-existing buckets.

  printf "Deleting all objects in s3://%s/...\n" "$BUCKET"
  aws --endpoint-url "$S3_ENDPOINT" --no-verify-ssl \
    s3 rm "s3://${BUCKET}" --recursive >/dev/null 2>&1 || true
  printf "Deleting bucket s3://%s...\n" "$BUCKET"
  aws --endpoint-url "$S3_ENDPOINT" --no-verify-ssl \
    s3 rb "s3://${BUCKET}" >/dev/null 2>&1 || true

contrib/tools/arbiter-s3-bench:212

• Bash return statuses are limited to 0–255. If 256 or more uploads fail, return "$errors" becomes status 0, so the caller's write_objects || ... branch is skipped and the cycle can be reported as successful despite failures. Return a boolean failure status and expose the full error count separately.

  return "$errors"

contrib/tools/arbiter-s3-bench:266

• The same exit-status overflow occurs for reads and verification errors: a total of 256 or more makes this return status 0. run_cycle can consequently lose all of those errors and print a successful summary; use a separate count variable and keep the function return value within the shell status range.

  return "$((errors + verify_errors))"

contrib/tools/arbiter-s3-bench:277

• Both cleanup commands ignore failures and Done is printed unconditionally. A network/permission error or a versioned/non-empty bucket can therefore leave data behind while the requested cleanup appears successful; preserve and report the command status instead of swallowing it.

  aws --endpoint-url "$S3_ENDPOINT" --no-verify-ssl \
    s3 rm "s3://${BUCKET}" --recursive >/dev/null 2>&1 || true
  printf "Deleting bucket s3://%s...\n" "$BUCKET"
  aws --endpoint-url "$S3_ENDPOINT" --no-verify-ssl \
    s3 rb "s3://${BUCKET}" >/dev/null 2>&1 || true

contrib/tools/arbiter-s3-bench:93

• The object-user controller creates the credentials Secret asynchronously, independently of the CephObjectStore phase. Waiting only for the store to become Ready does not guarantee that rook-ceph-object-user-my-store-test-user exists before the next command invokes this one-shot credential lookup, so the documented sequence can fail intermittently. Wait for the generated Secret or retry until both keys are present.

AWS_ACCESS_KEY_ID=$(kubectl get secret "$SECRET_NAME" -n "$CEPH_NS" \
  -o jsonpath='{.data.AccessKey}' 2>/dev/null | base64 -d) || true
AWS_SECRET_ACCESS_KEY=$(kubectl get secret "$SECRET_NAME" -n "$CEPH_NS" \
  -o jsonpath='{.data.SecretKey}' 2>/dev/null | base64 -d) || true

contrib/vm.yaml:9

• With format: false, this entry expects a pre-created Lima disk named osd2. The documented setup only creates osd, so creating the VM as documented will not provision the second OSD (and may fail when attaching it). Add limactl disk create osd2 --size=8G to the setup or otherwise provision this disk before creation.

  - name: osd2
    format: false

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Correctness and cleanup issues remain in the health and S3 tools.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (7)

Previously missed (1) — in code that hasn't changed since the last review.

contrib/tools/arbiter-health:492

• When Ceph reports an empty quorum_names array, grep -c . prints 0 and exits with status 1, so the || echo 0 branch prints a second 0. quorum_count then contains two lines and is not a valid numeric value for the comparisons below, breaking the quorum-lost assessment. Preserve the first count while neutralizing grep's no-match status instead.

README.md:105

• Applying a CephObjectStoreUser only creates the CR; Rook creates rook-ceph-object-user-my-store-test-user asynchronously. Waiting for the object store alone does not guarantee that the next command can fetch credentials, so this documented sequence can fail intermittently. Wait for the generated user secret (or have the benchmark retry the lookup) before starting it.

# run S3 bench (write + read + verify 50 objects)
limactl shell k8s bash ./contrib/tools/arbiter-s3-bench --verify

contrib/tools/arbiter-health:297

• The selector equates the RGW ceph_daemon_id with the object-store name, but Rook daemon IDs include the RGW/zone components (for example, rgw.my-store.a). Consequently this query returns no deployments for the supplied my-store example, so the dashboard omits RGW readiness even when the gateway is healthy. Select by Rook's object-store label or otherwise query the store's actual generated deployment name.

    rgw_deploy_info=$(kget get deploy -n "$ra_ceph_ns" \
      -l "app=rook-ceph-rgw,ceph_daemon_id=${rgw_store_name}" \
      -o jsonpath='{range .items[*]}{.metadata.name}{" "}{.status.readyReplicas}{"/"}{.status.replicas}{"\n"}{end}')

contrib/tools/arbiter-s3-bench:79

• --read-only --verify is currently accepted even though read-only mode skips generate_objects, so there are no local .sha256 files to compare with the downloaded objects. The later cat/sha256sum failures are hidden by the surrounding || true, allowing this mode to report verification success without verifying anything. Reject this incompatible option combination during argument validation.

if [[ "$READ_ONLY" == "true" && "$WRITE_ONLY" == "true" ]]; then
  echo "Error: --read-only and --write-only are mutually exclusive." >&2
  exit 2
fi

contrib/tools/arbiter-s3-bench:183

• This pipeline masks every aws s3 mb failure with || true. If credentials, the endpoint, or bucket creation is invalid, the benchmark continues into the object loop instead of failing setup, so the reported result is misleading. Check the create command's status and return an error.

    aws --endpoint-url "$S3_ENDPOINT" --no-verify-ssl \
      s3 mb "s3://${BUCKET}" 2>&1 | grep -v "InsecureRequestWarning" || true

contrib/tools/arbiter-s3-bench:353

• In non-loop mode, run_cycle returns nonzero on any object error, and set -e exits at this call before reaching cleanup_bucket. Thus --cleanup does not delete the bucket when the benchmark has a failed operation, contrary to the option's documented behavior; capture the cycle status, run cleanup, then exit with the original status.

  run_cycle 1

contrib/tools/arbiter-s3-bench:53

• The help extractor includes line 24, which is set -euo pipefail rather than a commented usage line. As a result, arbiter-s3-bench --help prints an implementation command after the options; stop the range at the blank comment line like the other two tools.

  sed -n '8,24p' "$0" | sed 's/^# \?//'

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved moderate issues affect health reporting, traffic visualization, benchmarking correctness, and VM provisioning.

Review details

Suppressed comments (9)

Previously missed (1) — in code that hasn't changed since the last review.

contrib/tools/arbiter-s3-bench:98

• The documented setup applies the CephObjectStoreUser and waits only for the object store before invoking the benchmark, but this secret lookup happens only once. User reconciliation and secret creation are asynchronous, so a fresh setup can race here and exit even though the user will become ready shortly; retry the lookup until the secret exists or document an explicit user-readiness wait.

contrib/tools/arbiter-health:153

• In watch mode, render calls resolve_remote_kubeconfig every five seconds, and each call creates a new temp file here. RESOLVED_REMOTE_KUBECONFIG is then overwritten, so cleanup can remove only the last file; the earlier files retain copies of the remote kubeconfig and accumulate in /tmp. Reuse the existing file or remove it before replacing it.

    rm -f "$RESOLVED_REMOTE_KUBECONFIG"

contrib/tools/arbiter-health:529

• This fallback is entered whenever the Ceph CLI query returns no output, which can also be caused by a missing toolbox, an RBAC denial, or a transient exec/network failure. The script cannot conclude from that alone that quorum is lost or that the cluster is read-only; this message can misdiagnose a healthy cluster. Report Ceph quorum as unknown and label the Kubernetes readiness result as a proxy.

    fi

contrib/tools/arbiter-health:492

• When quorum_names is empty, grep -c . already prints 0 before returning non-zero, so the || echo 0 appends a second 0. In the quorum-lost case this makes quorum_count a multi-line value, causing the numeric comparisons below to fail instead of rendering the fallback assessment. Count non-empty names without appending a second result.

  if [[ -n "$ceph_mon_list" || -n "$ceph_monmap_count" ]]; then

contrib/tools/arbiter-s3-bench:272

• read_objects keeps the read and verification failures in local variables but never updates the global READ_ERRORS declared for this purpose. Because run_cycle invokes this function with || true and then adds READ_ERRORS, failed downloads and checksum mismatches are omitted from the cycle total, so a failed read/verify run is reported as successful and exits with status 0. Assign the aggregate before returning.

  READ_ERRORS=$((errors + verify_errors))

contrib/tools/arbiter-s3-bench:250

• --read-only skips generate_objects, which is the only code that creates ${TMPDIR}/obj-*.sha256; therefore --read-only --verify reaches this block and tries to read checksum files that do not exist. Reject this combination during option validation or provide a persistent source of expected checksums, otherwise the advertised integrity check cannot work.

  if [[ "$VERIFY" == "true" && "$errors" -eq 0 ]]; then

contrib/tools/arbiter-s3-bench:125

• The fresh Lima VM has no AWS region configured; this script exports credentials but never sets a region, so the AWS CLI commonly exits with You must specify a region before sending any request to RGW. Set a default such as us-east-1 (or pass --region) for every AWS invocation.

S3_ENDPOINT="http://${RGW_IP}:${RGW_PORT}"

contrib/tools/arbiter-viz:80

• The non-follow path explicitly collects both --to-label and --from-label flows, but follow mode only subscribes to --to-label. Live responses from the arbiter back to the monitors are therefore omitted, so --follow does not provide a complete view of arbiter traffic. Stream or merge both directions in follow mode (or document that it is intentionally one-way).

if [[ "$FOLLOW" == "true" ]]; then
  run_hubble observe \
    --namespace "$NAMESPACE" \
    --to-label "$LABEL_SELECTOR" \
    -o json \
    --follow 2>/dev/null

contrib/vm.yaml:118

• This install block is nested inside the earlier if ! command -v kubeadm bootstrap guard, so reprovisioning an existing VM that already has kubeadm skips installing awscli. The newly documented S3 benchmark then fails even though the VM provisioning completed; install this dependency in an independent idempotent block.

    if ! command -v aws >/dev/null 2>&1; then
      export DEBIAN_FRONTEND=noninteractive
      apt-get update
      apt-get install -y awscli
    fi

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved correctness issues remain in the health, visualization, and S3 benchmarking tools.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (8)

Previously missed (3) — in code that hasn't changed since the last review.

contrib/tools/arbiter-health:523

• This monmap check has the same substring-matching problem as the quorum check: grep -qF can treat a different monitor whose name merely contains ra_monid as a match. Use exact-line matching so the health report does not claim the arbiter is in the monmap incorrectly.
  contrib/tools/arbiter-health:532
• An empty result from kexec is treated as Ceph being unreachable, but kexec suppresses all errors and returns empty output for unrelated failures such as a missing tools pod, authorization error, or CLI failure. This warning therefore can falsely claim quorum loss and read-only operation; report that quorum data is unavailable unless the command failure is classified.
  contrib/tools/arbiter-viz:80
• The non-follow path explicitly gathers both --to-label and --from-label flows, but follow mode subscribes only to traffic headed to the arbiter. Continuous output therefore omits all arbiter response flows and cannot show the same bidirectional monitor topology; use a bidirectional stream/filter strategy here as well.

README.md:105

• The documented sequence waits for the CephObjectStore but not for the CephObjectStoreUser secret that the benchmark reads immediately afterward. Since the user controller creates rook-ceph-object-user-my-store-test-user asynchronously, following these commands can race and produce the tool's credential error; wait for that Secret to be created (or have the tool retry) before invoking the benchmark.

kubectl wait --for=jsonpath='{.status.phase}'=Ready cephobjectstore/my-store -n rook-ceph --timeout=300s
# run S3 bench (write + read + verify 50 objects)
limactl shell k8s bash ./contrib/tools/arbiter-s3-bench --verify

contrib/tools/arbiter-health:287

• This output does not preserve an empty readyReplicas field. Kubernetes commonly omits status.readyReplicas when it is zero, which makes a line such as mon-a 1 collapse to mon-a 1; the parser then treats the total replica count as the ready count and can report an unready monitor as healthy in the fallback assessment. Emit a stable delimiter/marker and parse missing ready counts as zero.

  rook_mon_details=$(kget get deploy -n "$ra_ceph_ns" -l ceph_daemon_type=mon \
    -o jsonpath='{range .items[*]}{.metadata.labels.ceph_daemon_id}{" "}{.status.readyReplicas}{" "}{.status.replicas}{"\n"}{end}')

contrib/tools/arbiter-health:186

• The JSON fetched and parsed here is immediately discarded because ra_state is assigned again on the next line. This adds an unnecessary Kubernetes API call and awk parse to every render (including every five-second watch iteration); remove the dead ra_json extraction block.

  local ra_json=""
  ra_json=$(kget get remotearbiter "$ARBITER_NAME" -n "$OP_NAMESPACE" -o json)
  local ra_state="" ra_msg="" ra_monid="" ra_ceph_name="" ra_ceph_ns="" ra_svc_type="" ra_rc_name=""
  ra_state=$(echo "$ra_json" | awk -v Q='"' 'BEGIN{RS=""; FS="\n"} {
    needle = Q "state" Q ":" Q
    pos = index($0, needle)
    if (pos == 0) exit
    rest = substr($0, pos + length(needle))
    end = index(rest, Q)
    print substr(rest, 1, end - 1)
  }')

contrib/tools/arbiter-viz:256

• edge is constructed with | separators, but the third argument to awk split is an ERE; an unescaped | is alternation rather than a literal delimiter. As a result parts[1..4] can be parsed incorrectly and the displayed source, destination, port, and verdict are unreliable. Use a literal separator expression such as [|].

    split(e, parts, "|")

contrib/tools/arbiter-viz:335

• The diagram's pair-counting loop repeats the same unescaped | regular expression, so it cannot reliably recover the four fields from edge and will often report zero flows for monitor connections. Use the same literal separator as in the flow table parser.

        split(e, parts, "|")

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved moderate issues affect setup sequencing, cluster scoping, quorum handling, S3 verification/configuration, and traffic visualization.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

contrib/tools/arbiter-health:287

• These selectors match every monitor deployment in the namespace, not just the selected CephCluster. The operator itself scopes monitors with app.kubernetes.io/part-of=<cluster> (pkg/controller/remotearbiter_controller.go:1015-1019); without the same constraint, a namespace containing multiple CephClusters produces mixed monitor counts and an incorrect Kubernetes fallback assessment.

  rook_mons=$(kget get deploy -n "$ra_ceph_ns" -l ceph_daemon_type=mon \
    -o jsonpath='{range .items[*]}{.metadata.name}{" "}{.status.readyReplicas}{"/"}{.status.replicas}{"\n"}{end}')
  # Get mon IDs and readiness for quorum fallback assessment
  rook_mon_details=$(kget get deploy -n "$ra_ceph_ns" -l ceph_daemon_type=mon \
    -o jsonpath='{range .items[*]}{.metadata.labels.ceph_daemon_id}{" "}{.status.readyReplicas}{" "}{.status.replicas}{"\n"}{end}')

contrib/tools/arbiter-health:240

• The fallback mon pod lookup has the same unscoped ceph_daemon_type=mon selector. If the tools deployment cannot answer and multiple CephClusters share this namespace, the command can run against an unrelated cluster and make this dashboard report the wrong quorum. Scope this lookup with app.kubernetes.io/part-of=${ra_ceph_name} as well.

    mon_pod=$(kget get pod -n "$ra_ceph_ns" -l ceph_daemon_type=mon \
      -o jsonpath='{.items[0].metadata.name}')
    if [[ -n "$mon_pod" ]]; then
      ceph_quorum=$(kexec "$ra_ceph_ns" "$mon_pod" ceph quorum_status -f json --connect-timeout 5)

contrib/tools/arbiter-s3-bench:115

• On a fresh Lima VM, the newly installed AWS CLI has no region configured, and none of the benchmark's aws invocations supplies --region. AWS CLI S3 operations then fail with the region configuration error before any RGW traffic is generated. Set a default region (or pass one through every invocation) alongside the credentials.

export AWS_ACCESS_KEY_ID
export AWS_SECRET_ACCESS_KEY

contrib/tools/arbiter-viz:165

• The --from-label query returns reply flows from the arbiter to a monitor, where the monitor's service port is in source_port and destination_port is usually an ephemeral client port. Inspecting only destination_port therefore mislabels half of the traffic and omits those flows from the msgr1/msgr2 breakdown. Select a known Ceph port from either source or destination (and normalize the direction) before building the edge.

  # try destination_port first (hubble v1+), then port
  pneedle = Q "destination_port" Q ":"
  ppos = index(rest, pneedle)
  if (ppos == 0) {
    pneedle = Q "port" Q ":"

• Files reviewed: 7/7 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved moderate findings remain in the health, benchmarking, and visualization tools.

Review details

Suppressed comments (12)

Previously missed (1) — in code that hasn't changed since the last review.

contrib/tools/arbiter-health:304

• This always selects .items[0], so a namespace with multiple CephObjectStores can display and assess an arbitrary store rather than the store the operator/user is interested in. Please add a store selector option or render all stores instead of silently choosing the first item.

contrib/tools/arbiter-health:165

• On BSD/macOS, mktemp requires the six X placeholders to form the template suffix. With .kubeconfig after the placeholders, automatic remote-kubeconfig extraction fails before any remote queries run; keep the extension before the Xs or create the temporary file without an extension.

  tmpkubeconfig=$(mktemp "${TMPDIR:-/tmp}/arbiter-health-remote-XXXXXX.kubeconfig")

contrib/tools/arbiter-health:173

• This auto-detection silently chooses the first RemoteArbiter when multiple CRs exist in the operator namespace. All subsequent Ceph, condition, and remote-deployment queries then use an arbitrary arbiter and can produce a plausible but wrong dashboard. Require --arbiter for an ambiguous list or select deterministically with a clear indication.

  if [[ -z "$ARBITER_NAME" ]]; then
    ARBITER_NAME=$(kget get remotearbiter -n "$OP_NAMESPACE" -o jsonpath='{.items[0].metadata.name}')

contrib/tools/arbiter-health:308

• This hard-coded rook-ceph prefix makes the optional RGW endpoint disappear whenever the referenced CephCluster is in a non-default namespace. The dashboard already supports ra_ceph_ns; construct the Rook service name from that namespace instead.

    rgw_endpoint=$(kget get svc "rook-ceph-rgw-${rgw_store_name}" -n "$ra_ceph_ns" \

contrib/tools/arbiter-health:560

• A failed ceph quorum_status query does not establish that the cluster is read-only: the optional toolbox, Ceph credentials/CLI, or connectivity can be the failing component while Ceph remains writable. Present this as an inability to verify quorum/read-write state; the current assertion can lead to an incorrect operational response.

    printf "  ${YELLOW}[WARN]  Ceph cluster unreachable (quorum likely lost, cluster is read-only)${RESET}\n"

contrib/tools/arbiter-health:390

• This marks an RGW deployment OK as soon as one replica is ready, so a scaled store such as 1/3 is reported healthy despite two unavailable gateways. Compare ready replicas with the deployment's desired replica count when rendering health status.

        if [[ "${rgw_ready_num:-0}" -ge 1 ]] 2>/dev/null; then

contrib/tools/arbiter-s3-bench:364

• Because run_cycle is invoked in an || context at both call sites, Bash disables errexit for the function body. generate_objects is not checked here, so a failed dd can be followed by uploads of empty/partial files while the cycle is reported successful. Check each generation command and stop or record the cycle before calling write_objects when generation fails.

    generate_objects

contrib/tools/arbiter-s3-bench:139

• On the freshly provisioned Lima VM no AWS region is configured, so the first aws s3api/aws s3 call can fail with You must specify a region before any benchmark traffic is generated. Set a default RGW-compatible region here (or pass --region on every invocation) so the documented command works without an external aws configure step.

export AWS_ACCESS_KEY_ID
export AWS_SECRET_ACCESS_KEY

contrib/tools/arbiter-s3-bench:122

• --ceph-ns is configurable, but this generated Rook Secret name hard-codes the default rook-ceph namespace prefix. With a cluster in another namespace, the script queries the wrong Secret and exits before the benchmark; derive the prefix from CEPH_NS (and use the same prefix for the RGW service below).

SECRET_NAME="rook-ceph-object-user-${STORE_NAME}-${USER_NAME}"

contrib/tools/arbiter-s3-bench:142

• The --ceph-ns option is also ignored when constructing the RGW service name. Rook names this service with the Ceph namespace prefix, so a non-default namespace makes endpoint discovery fail even when the store exists.

RGW_SVC="rook-ceph-rgw-${STORE_NAME}"

contrib/tools/arbiter-s3-bench:323

• Because --run-id explicitly allows reusing an existing prefix, this recursive delete can remove objects from earlier runs or other users sharing that prefix; for example, --run-id old --write-only --cleanup deletes all of old/, not just the objects created by this invocation. Reject cleanup for reused prefixes or delete only the exact object keys written by this run.

  if ! aws --endpoint-url "$S3_ENDPOINT" --no-verify-ssl \
    s3 rm "s3://${BUCKET}/${RUN_ID}/" --recursive >/dev/null 2>&1; then

contrib/tools/arbiter-viz:58

• This picks the first RemoteArbiter across all namespaces but only keeps its name; the flow queries still use the separately configured namespace (default external-arbiter). With multiple arbiters or a different target namespace, auto-detection can show no flows or the wrong arbiter's flows. Resolve the selected CR's target namespace or require explicit selectors when ambiguous.

if [[ -z "$ARBITER_NAME" ]]; then
  ARBITER_NAME=$(kubectl get remotearbiter -A -o jsonpath='{.items[0].metadata.name}' 2>/dev/null || true)

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved S3 cleanup and Hubble label-selector issues need correction.

Review details

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

contrib/tools/arbiter-viz:66

• Hubble label filters are source-qualified; Kubernetes pod labels must be passed as k8s:<key>=<value>. This selector omits the k8s: source prefix, so the generated arbiter pod label will not match and both flow queries can return no traffic. Prefix the selector (or accept a fully qualified label input).
  README.md:111
• The Hubble CLI expects Kubernetes labels in the k8s:<key>=<value> form. Without the k8s: source prefix, this command does not select the ceph.cobaltcore.sap.com/lookup pod label and will generally show no arbiter traffic.

contrib/tools/arbiter-s3-bench:103

• --run-id is accepted for write runs, but cleanup_objects recursively deletes the entire ${RUN_ID} prefix. Reusing an earlier ID with --cleanup therefore removes objects from the earlier run too, contrary to the claim that cleanup only removes this run's objects. Reject --cleanup with an explicit run ID (or track and delete only keys created by this invocation).

if [[ "$READ_ONLY" == "true" && "$CLEANUP" == "true" ]]; then
  echo "Error: --read-only and --cleanup are mutually exclusive." >&2
  echo "Cleanup only removes objects written by this run." >&2
  exit 2
fi

contrib/tools/arbiter-s3-bench:201

• This trap only removes the local temporary directory. If a one-shot run is interrupted during an upload or download, objects already written remain in RGW even when --cleanup was requested; the aws calls are also inside if ! blocks, so SIGINT can be treated as an object failure instead of stopping the run. Add an interrupt/termination handler that performs cleanup before exiting.

trap 'rm -rf "$TMPDIR"' EXIT

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[senolcolak]

P1 — qualify the Kubernetes label for Hubble. Hubble label filters are source-qualified, so this selector needs the prefix. As written, the generated pod label may not match and both observations can return no traffic. Please build here and update the equivalent README example. I don't know hubble so this totally came from codex

[senolcolak]

P2 — --follow bypasses the visualization. Both observers write raw JSON directly to stdout, and this branch exits before the AWK renderer below. The follow mode therefore never shows the advertised topology, summary, or port breakdown. Please feed the merged stream through a live renderer, or explicitly document this as a raw-output mode.

[senolcolak]

P2 — do not infer quorum loss from a failed diagnostic command. Empty output can also mean the toolbox is missing, RBAC denied exec, the Ceph CLI failed, or the request timed out. None of those establishes that quorum is lost or that the cluster is read-only. Please report the quorum assessment as unavailable and reserve the outage diagnosis for valid Ceph evidence.

[senolcolak]

P2 — surface partial Hubble collection failures. Both directional queries discard stderr and status. If one succeeds and the other fails, the tool renders an incomplete capture as if it were the full bidirectional topology. Please track both statuses and warn or fail when either direction could not be collected.

[senolcolak]

P2 — do not depend on ownerReferences[0]. Owner references are not ordered, so the RemoteArbiter owner may not be the first entry. In that case inline RemoteCluster discovery silently fails and the later remote assessment becomes unreliable. Please match across all owner references, ideally by UID and kind.

[senolcolak]

The new shell tooling needs a small targeted test suite before we rely on it. This does not need exhaustive coverage: mocked command tests for Hubble success/partial failure and follow mode, quorum-command failure, RemoteCluster discovery, CLI argument validation, and S3 cleanup/error propagation would cover the high-risk branches.

[Copilot]

🟡 Changes recommended

Unresolved moderate issues remain in arbiter selection, watch/error handling, and S3 region configuration.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

contrib/tools/arbiter-health:195

• This full JSON fetch and AWK parse is immediately discarded when ra_state is assigned again on line 196. In watch mode it adds an unnecessary API request and leaves dead parsing logic that can diverge from the actual JSONPath source; remove the unused ra_json block.

  local ra_json=""
  ra_json=$(kget get remotearbiter "$ARBITER_NAME" -n "$OP_NAMESPACE" -o json)
  local ra_state="" ra_msg="" ra_monid="" ra_ceph_name="" ra_ceph_ns="" ra_svc_type="" ra_rc_name=""
  ra_state=$(echo "$ra_json" | awk -v Q='"' 'BEGIN{RS=""; FS="\n"} {
    needle = Q "state" Q ":" Q
    pos = index($0, needle)
    if (pos == 0) exit
    rest = substr($0, pos + length(needle))
    end = index(rest, Q)
    print substr(rest, 1, end - 1)
  }')

contrib/tools/arbiter-health:670

• With set -e, an unset (or unsupported) TERM makes clear exit non-zero, so --watch terminates after the first render in non-interactive shells. tput is already guarded above; guard this refresh command as well (for example, clear 2>/dev/null || true).

    clear

contrib/tools/arbiter-health:173

• Auto-detection takes .items[0] without checking whether the operator namespace contains multiple RemoteArbiter resources. In a multi-arbiter deployment this silently reports an arbitrary arbiter's health; fail with an ambiguity error and require --arbiter, or otherwise select deterministically.

    ARBITER_NAME=$(kget get remotearbiter -n "$OP_NAMESPACE" -o jsonpath='{.items[0].metadata.name}')

contrib/tools/arbiter-s3-bench:144

• The Lima provisioning only installs awscli and does not configure a region, and none is set before the S3 calls below. AWS CLI S3 operations can therefore fail with You must specify a region before contacting RGW; set a default RGW-compatible region (or expose a region option) before invoking aws.

export AWS_ACCESS_KEY_ID
export AWS_SECRET_ACCESS_KEY

contrib/tools/arbiter-viz:58

• Auto-detection takes .items[0] without checking whether the namespace contains multiple RemoteArbiter resources. In a multi-arbiter deployment this silently visualizes an arbitrary arbiter while the requested/default namespace and label may target another one; fail with an ambiguity error and require --arbiter, or otherwise select deterministically.

  ARBITER_NAME=$(kubectl get remotearbiter -A -o jsonpath='{.items[0].metadata.name}' 2>/dev/null || true)

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved moderate issues remain in the CLI tools and should be fixed before approval.

Review details

Suppressed comments (8)

Previously missed (1) — in code that hasn't changed since the last review.

contrib/tools/arbiter-s3-bench:73

• These numeric options are used by seq and arithmetic without validation. For example, --num-objects -1 runs no upload loop but produces a successful summary with negative object/throughput values; non-numeric input instead leaks a shell error. Reject invalid or non-positive values while parsing the arguments.

contrib/tools/arbiter-health:324

• Selecting .items[0] makes the dashboard report an arbitrary object store when the Ceph namespace contains multiple CephObjectStore resources; Kubernetes list ordering is not a stable indication of the store users intend. The endpoint and deployment status below can therefore describe a different RGW. Add an explicit store selector (or otherwise make the choice deterministic) and use it for all RGW queries.

  rgw_store_name=$(kget get cephobjectstore -n "$ra_ceph_ns" \
    -o jsonpath='{.items[0].metadata.name}')

contrib/tools/arbiter-health:414

• This marks an RGW deployment OK whenever at least one replica is ready, so a configured 1/2 deployment is reported as healthy instead of degraded. Compare ready replicas with the desired replica count (and distinguish partial readiness from full readiness) before emitting OK.

        if [[ "${rgw_ready_num:-0}" -ge 1 ]] 2>/dev/null; then
          printf "  |   %-35s %s [$(ok)]\n" "$rgw_dep_name" "$rgw_dep_ready"
        else
          printf "  |   %-35s %s [$(err)]\n" "$rgw_dep_name" "$rgw_dep_ready"
        fi

contrib/tools/arbiter-health:166

• The mktemp template does not end in the required X run on BSD/macOS (...XXXXXX.kubeconfig), so automatic remote-kubeconfig extraction fails on those platforms even though the helper is otherwise written for portable macOS/Linux use. The file extension is not needed; use a template ending in XXXXXX (or create then rename it).

  tmpkubeconfig=$(mktemp "${TMPDIR:-/tmp}/arbiter-health-remote-XXXXXX.kubeconfig")
  echo "$kubeconfig_b64" | b64decode > "$tmpkubeconfig"

contrib/tools/arbiter-viz:106

• The EXIT trap always finishes with exit 0, so a Hubble follow process that terminates with an error causes wait/set -e to enter this trap and the CLI still reports success. Preserve the child/wait failure status and only return zero for an intentional Ctrl+C termination; otherwise monitoring failures are silently hidden.

  trap 'kill $to_pid $from_pid 2>/dev/null; rm -f "$TMPFILE"; exit 0' INT TERM EXIT
  wait
  exit 0

contrib/tools/tests/arbiter-health.bats:40

• _extract_mons_json is reimplemented inside the test instead of exercising arbiter-health's parser. These tests can therefore pass even if the production parsing at contrib/tools/arbiter-health:281-308 regresses; drive the tool with mocked ceph quorum_status output or move the production parser into a sourceable unit.

# Normalises input to a single line to handle both compact and pretty-printed
# output, then uses awk bracket-matching with a whitespace-tolerant needle.
_extract_mons_json() {
  local flat

contrib/tools/tests/arbiter-s3-bench.bats:93

• This test defines a new cleanup_objects function instead of invoking the implementation under test. It can pass while the real cleanup at contrib/tools/arbiter-s3-bench:323-334 deletes the wrong prefix or mishandles AWS failures; make the test execute the tool's function or run the CLI with mocked dependencies.

  export NUM_OBJECTS=5

  # Define a minimal cleanup_objects function matching the tool's
  cleanup_objects() {
    local errors=0

contrib/tools/tests/arbiter-viz.bats:138

• The port-resolution tests copy the production AWK logic rather than invoking arbiter-viz, so changes or regressions in the actual parser at contrib/tools/arbiter-viz:199-248 will not be detected. Feed fixture flows through the tool with mocked Hubble output and assert the rendered port breakdown/edges.

# ---------------------------------------------------------------------------

@test "arbiter-viz: request flow resolves dst_port 3300 as msgr2" {
  local result
  result=$(echo '{"l4":{"TCP":{"source_port":49152,"destination_port":3300}}}' | awk -v Q='"' '

• Files reviewed: 18/18 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review details

Suppressed comments (6)

Previously missed (1) — in code that hasn't changed since the last review.

contrib/tools/arbiter-s3-bench:73

• These values are used by seq, arithmetic expansion, and dd without validation. Inputs such as --num-objects 0, --object-size -1, or a non-numeric value either run an empty benchmark or terminate with a shell error instead of a clear CLI validation error; reject non-positive/non-numeric values here (and return status 2) before proceeding.

contrib/tools/arbiter-health:109

• The remote queries bypass kget and invoke kubectl without a client-side request timeout. If the remote API endpoint is unreachable, a render (and therefore --watch) can block indefinitely instead of reaching the dashboard's fallback assessment; pass a bounded --request-timeout in the shared Kubernetes query paths, including rget and kexec.

    result=$(kubectl --kubeconfig "$RESOLVED_REMOTE_KUBECONFIG" "${ctx_args[@]}" "${args[@]}" 2>/dev/null) || true
    echo "${result:-}"

contrib/tools/arbiter-health:675

• The watch loop runs with set -e, so an unguarded clear makes --watch exit when TERM is unset or the output is not attached to a terminal (clear returns nonzero in that case). This prevents the advertised periodic dashboard from continuing in common SSH/CI or piped invocations; guard the terminal refresh command's failure.

    clear

contrib/tools/arbiter-s3-bench:416

• When --cleanup is used after a successful cycle, cleanup_objects runs as the condition of an if, so its nonzero status is intentionally ignored and the final printf makes the tool exit 0 even when s3 rm failed. Please propagate the cleanup failure (while still printing the cleanup hint) so callers do not mistake leftover test objects for a successful cleanup.

if [[ "$CLEANUP" == "true" ]]; then
  cleanup_objects
fi

contrib/tools/arbiter-s3-bench:403

• The loop-mode SIGINT trap also unconditionally exits 0 after cleanup_objects; when --cleanup is enabled and s3 rm fails, the command still reports a successful stop even though its objects remain. Preserve the cleanup return status in the trap instead of always exiting 0.

  trap 'printf "\n${BOLD}Stopped after %d cycles (%d total errors)${RESET}\n" "$cycle" "$total_cycles_errors"; if [[ "$CLEANUP" == "true" ]]; then cleanup_objects; fi; print_cleanup_hint; exit 0' INT

contrib/tools/arbiter-s3-bench:201

• When a non-loop invocation is interrupted (for example, Ctrl+C during an upload), this is the only exit trap, so --cleanup is never applied and the already-written run prefix is left in RGW. Add an INT/TERM handler after cleanup_objects is defined that attempts the requested prefix cleanup before exiting, while retaining the temporary-directory cleanup.

TMPDIR=$(mktemp -d)
trap 'rm -rf "$TMPDIR"' EXIT

• Files reviewed: 18/18 changed files
• Comments generated: 0 new
• Review effort level: Lite