lablup / lablup/bssh

Add network-ping command for fast ICMP connectivity testing

Open
#75 0 comments 0 reactions 1 assignee Claimed by @inureyes View on GitHub
priority:low status:backlog type:enhancement
Dominant language
Rust
Stars
65
Forks
7
Avg merge
1h 30m
Merged PRs (30d)
25

Description

## Problem

Currently, the `bssh ping` command tests SSH connectivity by establishing a full SSH connection, authenticating, and executing a no-op command (`true`, see `src/commands/ping.rs`). While this is useful for verifying SSH service availability, it's relatively slow (2-5 seconds per host due to SSH handshake and authentication overhead).

Users often want to quickly check basic network connectivity across all cluster nodes without the SSH overhead, similar to the traditional `ping` command that uses ICMP packets and responds in milliseconds.

## Proposed Solution

Add a new `network-ping` command that performs fast ICMP ping tests across all cluster nodes to verify basic network connectivity.

## Implementation Details

### CLI Changes

Add a new subcommand to the `Commands` enum in `src/cli/bssh.rs` (the old flat `src/cli.rs` was split into `src/cli/{mod,bssh,pdsh}.rs` in #105):

```rust
#[derive(Debug, Subcommand)]
pub enum Commands {
// ... existing commands ...

#[command(
visible_alias = "nping",
about = "Test network connectivity using ICMP ping (fast)",
long_about = "Sends ICMP echo requests to all target hosts in parallel.\nReports per-host packet loss and round-trip time statistics.\nDoes not open an SSH connection, so it does not verify SSH service availability or authentication.\n\nExit codes: 0 (all hosts answered), 1 (some hosts answered), 255 (no host answered, or bssh failed before sending)"
)]
NetworkPing {
#[arg(
short = 'c',
long = "count",
default_value = "4",
help = "Number of echo requests to send per host"
)]
count: u32,

#[arg(
short = 'W',
long = "timeout",
default_value = "2",
help = "Time to wait for a reply to each request, in seconds"
)]
timeout: u64,

#[arg(
long = "interval",
default_value = "0.2",
help = "Interval between requests, in seconds (minimum 0.2)"
)]
interval: f64,
},
}
```

#### Flag naming rationale (OpenSSH compatibility)

The original draft of this issue proposed `-t` for the timeout. That is rejected: the root `Cli` already binds `-t` to `--tty` (`src/cli/bssh.rs:266`), which is OpenSSH's "force pseudo-terminal allocation". Because no root argument is declared `global = true`, clap would not error, but `-t` would mean "force a TTY" before the subcommand and "timeout" after it. That is exactly the kind of positional ambiguity a drop-in SSH replacement must not introduce.

The rule adopted here: **a subcommand short flag must not reuse a letter that the root `Cli` binds to a different meaning.** Where the conventional `ping` short flag collides with an OpenSSH flag, the option becomes long-only.

Short flags already taken by the root `Cli`, nearly all with their OpenSSH meanings: `-4 -6 -A -b -C -D -F -H -J -L -N -Q -R -S -T -f -i -k -l -o -p -q -t -v -x`.

| Option | Choice | Reason |
|---|---|---|
| Packet count | `-c, --count` | Matches `ping -c`. `-c` is free at the root (`-C` is `--cluster`, and clap is case sensitive). |
| Reply timeout | `-W, --timeout` | Matches iputils `ping -W` ("time to wait for a response"). `-W` is free at the root. Avoids the `-t`/`--tty` collision. |
| Interval | `--interval` (long only) | `ping -i` would shadow OpenSSH's `-i` (IdentityFile) at the root, so no short form is offered. |
| Address family | none; inherits root `-4`/`-6` | OpenSSH treats `AddressFamily` as a client-global setting, not a per-operation one. See the blocking dependency below. |
| Parallelism | none; inherits root `--parallel` | Same as `ping`, `upload`, and `download`, which all read `ctx.max_parallel` (default 10). |

Two further constraints verified in the current code:

- Existing variants (`List`, `Ping`, `Upload`, `Download`, `Interactive`, `CacheStats`) all use `#[command(about = ..., long_about = ...)]` prose rather than doc comments, so the new variant follows that style.
- No subcommand in the codebase currently declares a clap alias, so `visible_alias = "nping"` would be the first. This is accepted deliberately: `nping` is short enough to be worth it, and the alias does not shadow anything.

### Core Implementation

Create `src/commands/network_ping.rs` and register it in `src/commands/mod.rs`:
- Use a Rust ICMP ping library (see options below)
- Perform parallel ping tests across all nodes
- Display results with latency statistics (min/avg/max/stddev)
- Show packet loss percentage
- Color-coded output (see thresholds below)

Reuse `crate::ui::OutputFormatter::{format_command_header, format_summary}` so the output matches `ping`, `upload`, and `download`.

#### Settled behavior

These points were unspecified in the original draft. They are now decided, following OpenSSH and iputils/BSD `ping` precedent.

**Packet interval.** Default 0.2 seconds, minimum 0.2 seconds. iputils `ping` permits intervals below 0.2s only for the superuser, so 0.2s is the safe unprivileged floor. With the default 4 packets this is roughly 0.6 seconds of wall clock per host, and hosts are probed in parallel.

**Concurrency.** No subcommand flag. The command reuses the global `--parallel` value (`ctx.max_parallel`, default 10), consistent with every other multi-node command.

**Output modes.** Normal output only. No TUI, no `--stream`, no `--output-dir`. This mirrors `ping`, whose implementation states "Use normal execution (no TUI, no streaming) for ping". A fixed-size result table gains nothing from streaming, and the run is short enough that progress monitoring is unnecessary.

**pdsh compatibility mode.** Not exposed. pdsh has no ICMP subcommand, and `src/cli/pdsh.rs` maps pdsh *options* onto bssh behavior rather than exposing bssh subcommands. `network-ping` stays bssh-only.

**Latency color thresholds.** There is no OpenSSH precedent here, so these are set as a starting point and may be tuned: green under 10 ms, yellow from 10 ms to 100 ms, red above 100 ms. Any packet loss forces red regardless of latency.

**Statistics fields.** `min/avg/max/stddev`, matching BSD `ping`'s summary line. The original sample output omitted stddev; it is included below. (iputils prints `mdev` instead; `stddev` is chosen because it is the more widely understood label and the issue's own text already said stddev.)

**Exit codes.** `0` when every targeted host answered at least one echo request. `1` when at least one host answered and at least one did not. `255` when no host answered, or when bssh failed before it could send anything (ICMP socket creation denied, no hosts resolved, bad arguments). The `255` case follows OpenSSH's convention that 255 means "ssh itself failed" rather than "the remote operation failed". This must stay consistent with #245, which settles the same question for the sibling `ping` command.

### Suggested Libraries

**Option 1: `surge-ping` (Recommended)**
- Pure Rust, async-friendly with tokio
- Cross-platform (Linux, macOS, Windows)
- No external dependencies
- Supports both privileged and unprivileged ICMP

```toml
[dependencies]
surge-ping = "0.9"
```

**Option 2: `fastping-rs`**
- Simple API, battle-tested
- Requires raw sockets (may need elevated privileges)

**Option 3: `pnet` (lower-level)**
- Full network protocol suite
- More complex but very flexible
- Requires privileged access

### Example Implementation Skeleton

```rust
use anyhow::Result;
use surge_ping::{Client, Config, ICMP, PingIdentifier, PingSequence};
use std::net::IpAddr;
use std::time::Duration;
use tokio::time::timeout;

pub async fn network_ping_nodes(
nodes: Vec,
count: u32,
ping_timeout: u64,
interval: f64,
max_parallel: usize,
) -> Result<()> {
// Create ICMP client
let client = Client::new(&Config::default())?;

// Create tasks for each node
let tasks: Vec<_> = nodes.iter().map(|node| {
let client = client.clone();
let host = node.host.clone();
tokio::spawn(async move {
ping_host(&client, &host, count, ping_timeout).await
})
}).collect();

// Execute with concurrency limit
// ... parallel execution logic ...

// Display results with statistics
// ... formatting and output ...

Ok(())
}

async fn ping_host(
client: &Client,
host: &str,
count: u32,
timeout_secs: u64,
) -> Result {
// NOTE (2026-08-02): `Node.host` is a hostname string, not necessarily a
// literal IP. Nothing in the current SSH path parses it as `IpAddr` (russh
// and tokio resolve `(host, port)` for us), so this line must become a DNS
// lookup (e.g. `tokio::net::lookup_host`) or every hostname-based node
// fails to parse. Which resolved address to pick is an open decision, see
// "Decisions still required" below.
let addr: IpAddr = host.parse()?;
let mut pinger = client.pinger(addr, PingIdentifier(rand::random())).await;

let mut latencies = Vec::new();
let mut lost = 0;

for seq in 0..count {
match timeout(
Duration::from_secs(timeout_secs),
pinger.ping(PingSequence(seq as u16), &[])
).await {
Ok(Ok((_, duration))) => latencies.push(duration),
_ => lost += 1,
}
}

Ok(PingStats::from_latencies(latencies, lost, count))
}
```

Note that `Config::default()` selects ICMPv4. Honoring the root `-4`/`-6` flags requires selecting `ICMP::V4` or `ICMP::V6` explicitly, which is why the `ICMP` import above is currently unused.

### Output Format

```
▶ Network Ping Test Results (12 nodes)

● 10.100.64.101 4/4 packets min/avg/max/stddev = 0.5/1.2/2.1/0.6 ms
● 10.100.64.102 4/4 packets min/avg/max/stddev = 0.8/1.5/2.3/0.5 ms
● 10.100.64.103 3/4 packets min/avg/max/stddev = 1.2/2.1/3.0/0.8 ms (25% loss)
● 10.100.64.104 0/4 packets - Host unreachable

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Summary: 3 reachable, 1 unreachable (25% success rate)
Average latency: 1.6 ms (mean of per-host averages)
```

## Usage Examples

```bash
# Basic network ping to all cluster nodes
bssh -C production network-ping

# Custom packet count and reply timeout
bssh -C production network-ping -c 10 -W 1

# Slower probing
bssh -C production network-ping -c 10 --interval 1.0

# Force IPv6 (requires the -4/-6 wiring issue to land first)
bssh -6 -C production network-ping

# Quick alias
bssh -C production nping

# With specific hosts
bssh -H "host1,host2,host3" network-ping
```

## Comparison: `ping` vs `network-ping`

| Command | Purpose | Speed | Tests |
|---------|---------|-------|-------|
| `ping` | SSH connectivity test | 2-5s per node | SSH service + auth |
| `network-ping` | Network connectivity test | ~0.6s total at defaults (4 packets at 0.2s interval, hosts probed in parallel), plus up to `-W` per unresponsive host | ICMP reachability |

The original draft claimed "<100ms per node", which is not achievable with a multi-packet probe: the floor is `count * interval`. The corrected figure reflects the settled defaults.

## Files to Modify/Create

```
src/
├── cli/
│ └── bssh.rs # Add NetworkPing variant to Commands enum
├── app/
│ └── dispatcher.rs # Route NetworkPing (see match sites below)
└── commands/
├── mod.rs # Export network_ping module
└── network_ping.rs # New: ICMP ping implementation

Cargo.toml # Add surge-ping dependency
README.md # Document under "Built-in Commands"
docs/man/bssh.1 # Document under .SH COMMANDS
```

Command routing moved out of `src/main.rs` into `src/app/dispatcher.rs` in `5f3c320` (main.rs split, Phase 2, 2025-10-20). A new variant must be handled at four match sites in `dispatcher.rs`, not one:

1. the main `match &cli.command` dispatch arm
2. `subcommand_name()` (diagnostics label)
3. `sudo_password_is_applicable()` (should return `false`; ICMP has no sudo hook)
4. `ssh_password_is_applicable()` (should return `false`; ICMP opens no SSH connection)

## Dependencies to Add

```toml
[dependencies]
surge-ping = "0.9" # ICMP ping library (0.9.0 released 2026-06-29)
```

`rand` is already a direct dependency (`rand = "0.10"`), so the `rand::random()` call in the skeleton needs no new dependency.

## Security Considerations

**ICMP Raw Sockets:**
- On Linux: May require `CAP_NET_RAW` capability or root privileges
- On macOS: Generally works without special permissions
- ~~On Windows: Requires administrator privileges~~ (not applicable: bssh does not build for Windows. `nix` is an unconditional dependency in `Cargo.toml`, and the release matrix ships only linux-gnu/musl x86_64+aarch64 and aarch64-apple-darwin.)

**Solutions:**
1. Use `surge-ping` with unprivileged mode (SOCK_DGRAM) when possible
2. Document privilege requirements in README
3. Fall back to TCP ping if ICMP is unavailable
4. Provide clear error messages if permissions are insufficient

Note that items 1, 3, and 4 are not yet a chosen strategy; see "Decisions still required".

## Blocking dependency

Honoring `-4`/`-6` requires those flags to actually work. They are currently parsed and then ignored: `src/cli/bssh.rs:281-296` declares them, `docs/man/bssh.1` documents them, and no code in the repository reads `cli.ipv4` or `cli.ipv6`. #246 tracks wiring them into the connection path (along with the `AddressFamily` SSH config keyword, which is parsed and resolved but likewise never consumed). Until #246 lands, `network-ping` either hardcodes ICMPv4 or becomes the first consumer of the preference, which is a scheduling decision between the two issues.

## Testing Plan

1. **Unit Tests**:
- Ping parsing and statistics calculation (min/avg/max/stddev)
- Timeout handling
- Packet loss detection
- Exit code mapping for the all-success, partial-failure, and total-failure cases

2. **Integration Tests**:
- Single host ping
- Multi-host parallel ping
- Timeout scenarios
- Unreachable hosts
- Note: whether these can run in CI is unresolved, see "Decisions still required"

3. **Manual Testing**:
- Test on Linux (various distros)
- Test on macOS
- Test with different privilege levels
- Compare with system `ping` command

## Alternative Implementations

If ICMP proves problematic, consider:
1. **TCP Ping**: Connect to SSH port without authentication
2. **HTTP Ping**: If nodes have web services
3. **Hybrid**: Try ICMP first, fall back to TCP

## Decisions still required

Nothing below is settled. These need answering before or during implementation. They are recorded here so the decision can be made later from the issue alone.

1. **Whether to build this at all.** The issue is `priority:low` / `status:backlog` and its own closing paragraph notes that `ping` already covers SSH connectivity testing. No one has committed to the feature.
2. **Which ICMP library.** Three candidates are listed above with `surge-ping` marked "Recommended", but no evaluation has been done. Deciding factors: unprivileged SOCK_DGRAM support on both Linux and macOS, IPv6 support (needed for `-6`), and maintenance activity.
3. **What happens when ICMP is unavailable.** The four "Solutions" above are parallel options, not a strategy. The choice is material: adding an automatic TCP fallback changes what the command measures, so the comparison table's claim of "ICMP reachability" would no longer hold. Options: hard error with a clear message and exit 255; automatic fallback with the transport labeled per host in the output; or an explicit opt-in flag.
4. **When, if ever, the TCP/HTTP/Hybrid alternatives trigger.** Related to 3, but broader: "if ICMP proves problematic" has no defined trigger.
5. **Jump host behavior.** ICMP cannot traverse `-J`/`--jump-host`, which bssh supports. For a node reachable only through a jump host, `network-ping` must either report it unreachable (accurate for ICMP, misleading for the user), skip it with a notice, or fall back to a TCP probe tunneled through the jump host. This is the largest open question.
6. **Which resolved address to probe.** A hostname may resolve to several addresses. `ping` probes one. Options: first address in resolver order (matching the existing SSH connect loop's preference order), all addresses, or the address family forced by `-4`/`-6`. Interacts with #246.
7. **CI feasibility.** The integration tests need an ICMP socket. Whether GitHub's `ubuntu-latest` runners permit unprivileged ICMP (via `net.ipv4.ping_group_range`) has not been checked. If they do not, the integration tests must be feature-gated or moved to manual testing.
8. **Final exit code mapping.** The 0/1/255 scheme above is proposed, not ratified. #245 asks the same question for `ping` and frames it as Option A (0/1 only, preserving the currently documented text) versus Option B (0/1/255, OpenSSH-aligned). Whichever option #245 adopts governs here too; `network-ping` should not diverge from its sibling.

## Related Issues

- Complements existing `ping` command (SSH connectivity test)
- Blocked on #246 (`-4`/`-6` address family flags are parsed but ignored) for address family support
- Exit code semantics must agree with #245 (`ping` exit code contract)
- Part of broader cluster management features

## Priority

Low - Nice to have feature for quick network checks, though `ping` command already provides SSH connectivity testing.

---
**Refresh log**

- 2026-08-02 - Refreshed against `main` at `c3b8ac2`. Feature is still entirely unimplemented: no `network_ping` module, no `NetworkPing` variant, no `surge-ping` dependency. All items remain open.
- Renamed references: 3. `src/cli.rs` to `src/cli/bssh.rs` (split in #105, 2025-12-17, after this issue was filed); `src/main.rs` routing to `src/app/dispatcher.rs` (`5f3c320`, 2025-10-20, which already predated this issue, so the original body was pointing at a stale path from the start); `src/commands/mod.rs` path re-nested under the tree.
- Corrected facts: 3. `ping` executes `true`, not `echo 'pong'`; `surge-ping` bumped 0.8 to 0.9 (0.9.0, 2026-06-29); `rand 0.10` is already a direct dependency.
- Marked obsolete: 1. The Windows administrator-privileges bullet. bssh does not build for Windows (unconditional `nix` dependency) and ships no Windows release artifact.
- Added implementation notes: dispatcher requires four match arms, not one; `-t` already means `--tty` at the root; no existing subcommand uses a clap alias; `Node.host` is a hostname so the skeleton's `IpAddr` parse needs a DNS lookup; docs targets are README "Built-in Commands" and `docs/man/bssh.1` `.SH COMMANDS`.
- Surfaced (not added to scope): ICMP cannot traverse `-J/--jump-host`, which bssh supports. What `network-ping` should do for jump-host-reachable-only nodes is undecided and may deserve its own discussion before implementation.

- 2026-08-02 (second pass, verified against `main` at `0eb3fac`; only `Cargo.lock` moved since `c3b8ac2`, so every `Cargo.toml` claim above still holds) - Resolved the flag collision and the previously unspecified behaviors, and separated what is now settled from what still needs a decision.
- Flag set reworked for OpenSSH compatibility: `-t` rejected because the root `Cli` binds it to `--tty`. Timeout becomes `-W, --timeout` (iputils `ping -W`), interval becomes long-only `--interval` because `ping -i` would shadow OpenSSH's `-i` (IdentityFile), count stays `-c`. Address family and parallelism inherit the root flags rather than adding subcommand duplicates. The governing rule is stated in the body: a subcommand short flag must not reuse a root letter with a different meaning.
- Settled 7 previously unspecified points: packet interval (0.2s default and floor, per iputils' unprivileged limit), concurrency (inherits `--parallel`), output modes (normal only, mirroring `ping`), pdsh exposure (none), color thresholds (10ms / 100ms), statistics fields (min/avg/max/stddev, sample output corrected), and exit codes (0/1/255 with 255 following OpenSSH's "ssh itself failed" convention).
- Corrected the comparison table: "<100ms per node" is unachievable for a multi-packet probe whose floor is `count * interval`. Replaced with the actual figure at the settled defaults.
- Added a "Blocking dependency" section: `-4`/`-6` are declared at `src/cli/bssh.rs:281-296` and read by nothing, so address family support depends on #246.
- Added a "Decisions still required" section with 8 numbered items, so the remaining choices can be made later from the issue alone rather than rediscovered.
- Filed two issues found during this pass: #245 (`ping` documents exit codes 0/1 but always exits 0) and #246 (`-4`/`-6` parsed but never consumed, along with the `AddressFamily` config keyword). Both govern decisions listed here.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.