Regression tests for swadm

[cfzimmerman]

"A network switch is only as good as its CLI"

- Probably someone important at some point

swadm is a great tool, and I would like it to continuously improve. While working on #145, I observed how easy (and tempting!) it is to make breaking changes. However, a quick search suggests breaking changes would not be well received by scripts and docs. Such changes may be needed someday, but that should be a careful decision and certainly not an accident.

This PR adds infrastructure for regression testing swadm changes in CI. These diffs focus on the tx eq settings I'll soon be modifying, but hopefully the structure is easy to extend as other swadm projects arise.

[cfzimmerman]

🤖 Claude review

Review: dendrite#350 — swadm CLI integration tests

Nice cleanup — folding the loose swadm/tests/*.rs binaries into one cli
target, killing the dead port-link.rs, and giving the CLI a real
output-matching helper is the right shape. Pattern/expect_line/pat! is a
good abstraction and the output_txeq unit test that exercises the matcher
without a dpd is a nice touch.

Most of what follows is about when these tests run and how they
synchronize with dpd, not about the matcher itself. Two of them I think will
bite CI.

---

1. .github/buildomat/packet-test-common.sh:106 — swadm CLI tests now gate the packet tests

Dropping --test counters means the swadm block runs three link-mutating
tests instead of one read-only one, and it runs them before the packet
tests, with set -o errexit still in effect (it's re-enabled right after the
dpd startup poll and not cleared again until banner "Packet Tests").

Two consequences:

• --no-fail-fast only affects cargo's within-run behavior, not its exit code.
  Any swadm CLI regression now aborts the whole job before dpd-client's
  integration tests run at all, so you lose all packet/multicast signal for an
  unrelated string-formatting change. That's a big blast radius for a test suite
  whose stated purpose is catching CLI drift.
• The tests mutate rear0/0, which dpd-client also depends on.
  dpd-client/tests/integration_tests/common.rs:445 registers rear0 in the
  port table, and Switch::init() (same file, line 498) panics —
  failed to get mac for port rear0/0 — if link_mac_get fails for any
  registered port. So a swadm test that leaves rear0/0 deleted takes down
  every packet test with a completely unrelated error message.

Suggest moving the pushd swadm block to after the dpd-client block. It
costs nothing and makes the swadm tests strictly additive to the job.

2. swadm/tests/cli/tx_eq.rs:31 (also link_apply.rs:24 and :51) — read-back races the reconciler

link serdes set txeq and link apply do not program the ASIC
synchronously. Switch::link_tx_eq_set (dpd/src/link.rs:1277) and the
port-settings modify path (dpd/src/port_settings.rs:457) both just set
link.tx_eq, clear plumbed.tx_eq_pushed, and trigger() the reconciler. The
read side goes the other way: link_tx_eq_get (dpd/src/api_server.rs:2604)
reads the SDE via serdes::port_tx_eq_get, not dpd's config.

And the reconciler doesn't just push the taps — dpd/src/link.rs:1856:

} else if link.config.enabled && !link.plumbed.tx_eq_pushed {
    debug!(log, "tx-eq needs an update, tearing down link",);
    true

so a tx-eq change unplumbs and re-plumbs the link (port del, port add, MAC
reprogram, tx-eq push, enable) before the new value is visible.

The tests issue the set/apply and then immediately spawn a second swadm
process to get. In the window, serdes::port_tx_eq_get → lane_count() runs
against a port that has been torn down (error → non-zero exit → cmd::swadm
bails), or it returns the previous taps (assertion failure). It'll usually
win the race because process spawn is slower than the reconciler, but "usually"
is how you get a test that fails once a month in buildomat and nobody can
reproduce.

Worth wrapping the read-back in a poll-with-timeout helper on Out /
cmd::swadm — something like expect_line_within(Duration) that retries the
command. Every other integration suite in this repo that touches link state
polls rather than reading once.

3. swadm/tests/cli/tx_eq.rs:78 — create_link deletes rear0/0 with no way back

let delete_cmd = format!("link del {link}");
if let Err(e) = cmd::swadm(&delete_cmd) { println!("Delete failed. ..."); }

cmd::swadm(format!("link create {port} -s 100g --fec rs"))?

If the del succeeds and the create on the next line fails — or the test
process is killed between them — rear0/0 is gone for the rest of the job.
dpd/misc/model_config.toml is only read at dpd startup, so nothing puts it
back. See finding 1 for what that does to the packet tests.

A Drop guard (or a scopeguard-style teardown) that re-creates the link from
model_config.toml's values (fec = RS, speed = 100G) would make these tests
safe to run anywhere in the job.

Minor related note: create_link recreates the link but never restores the
tx-eq state, so set_all_taps leaves rear0/0 with {-1, 0, 10, 5, 2}
programmed. Harmless for the packet tests today since rear0 has no veth, but
it's the kind of leftover that makes a future failure confusing.

4. swadm/tests/cli/link_apply.rs:79 — tx_eq_exclusive only asserts "exit code != 0"

cmd::swadm bails on any non-zero exit, so expect_err can't tell "clap
rejected the conflict" from "--lane got renamed", "the speed string is no
longer accepted", or "dpd isn't reachable". The error already carries stderr —
asserting it contains clap's cannot be used with would make the test actually
pin the conflicts_with = "tx_eq" attributes at swadm/src/link.rs:460-476.

Second point on this one: this test needs no dpd at all — clap rejects the args
before any network I/O. Because it's #[ignore]d it only ever runs in the
Linux packet-test job. Un-ignoring it would get it running in the illumos
test.sh job for free, which is where a swadm-only change is most likely to
be caught early. (#[serial] is also unnecessary on it.)

5. swadm/src/link.rs:2151 — link apply ignores --link's link id and --lane (pre-existing)

Not introduced here, but the new tests are the reason to mention it: the handler
does

body.links.insert(String::from("0"), types::LinkSettings { ... });

hardcoding link id 0 and discarding link.link_id from the parsed LinkPath.
--lane is likewise dropped on the dpd side — LinkSpec has no lane field
and add_link (dpd/src/port_settings.rs:325) carries a TODO saying so.

Concretely, swadm link apply --link rear0/1 --lane 1 --speed 100g --tag t
reconfigures rear0/0 and, because calculate_links
(dpd/src/port_settings.rs:192) computes links_to_del = switch_links - settings_links, deletes rear0/1. Since every new test uses rear0/0 and
--lane 0, the suite bakes in the blind spot. A case using a non-zero link id
would be a good addition — either as a test of the fix, or as an #[ignore]d
known-failure with a comment.

6. swadm/tests/cli/cmd.rs:157 — a trailing re::ANY can never match

make_regex joins every pattern with \s+, but expect_line matches against
line.trim(). So pat![.., ANY] at the end of a pattern list requires trailing
whitespace that trim() has already removed:

out.expect_line(pat!["Speed", "100G", ANY])  // never matches `Speed  100G`

It happens to work at tx_eq.rs:74 only because link ls has more columns
after Media. Since this helper is explicitly meant to be reused by future
tests, either document that ANY must not be last, or have make_regex emit
\s* when the next/previous element can match empty.

7. swadm/tests/cli/cmd.rs:41 — hardcoded --host [::1] makes the CI env vars dead

packet-test-common.sh still exports DENDRITE_TEST_HOST='[::1]' and
DENDRITE_TEST_VERBOSITY=3, and nothing reads them any more on the swadm side.
Reading DENDRITE_TEST_HOST (default [::1]) and DENDRITE_TEST_PORT (default
12224) would keep the script honest and let these tests run against the
split illumos-dpd / Linux-model rig the new README describes — which is exactly
the setup someone will reach for when the tofino-sde#21 blocker is lifted.

8. swadm/tests/cli/cmd.rs:42 — whitespace splitting is a one-way door

split_ascii_whitespace() makes the multi-line format! strings read well, but
it silently mangles any argument containing a space (a --tag with a space, a
description, a comma-free list). Worth an escape hatch — swadm_args(&[&str])
with the current function as a thin wrapper — before someone hits it and spends
an afternoon on it.

---

Smaller things

• swadm/tests/cli/counters.rs:17: .expect("Failed to execute swadm counters list") now also fires when the command ran fine and returned non-zero.
  Wording nit, but the message will mislead whoever reads the CI log.
• swadm/tests/cli/link_apply.rs:22: --tx-eq=-1 sets main = -1, which is
  not a physically sensible main tap (it's normally a large positive value).
  Fine against the model, which accepts anything, but if these tests ever run on
  hardware the SDE may clamp or reject it. A comment saying the value is
  deliberately arbitrary would save a future reader the detour.
• swadm/Cargo.toml:29: stray trailing blank line after [dev-dependencies].
• README wording: "These tests are run in Linux CI but currently ignored in
  Illumos CI" is accurate, but worth adding why an individual test is
  #[ignore]d — the attribute means "needs a live dpd", and finding 4 shows at
  least one test that doesn't.

Verified

• cargo check -p swadm --tests is clean on illumos; cargo picks up
  swadm/tests/cli/main.rs as a single cli test target as intended.
• tools/check_copyrights.sh only inspects .sh|.xml|.rs|.p4, so the new
  README.md needs no header, and all four new .rs files have one.
• The tag passed to port_settings_apply only lands on IP-address entries, so
  applying with --tag test over a link created by link create (or by
  model_config.toml) is not an ownership conflict — good.
• dpd's create_link handles the delete_me case by replacing the link and
  keeping its MAC (dpd/src/link.rs:583-605), so the link del → link create
  sequence in create_link is not itself racy. It's only the failure path
  (finding 3) that's a problem.

[cfzimmerman]

1. Accepted, but I don't feel great about this. If swadm tests really grow, I'd consider making them their own CI job. But that's probably its own PR.
2. Added a cmd::repeat helper for retries.
3. I think this is unwarranted complexity. swadm tests should assume the switch is in an undefined config and handle that accordingly (see tx_eq::create_100g_link). And per (1), other tests are now unaffected because swadm tests run last.
4. I removed serial and ignored. Now we're checking for a "Usage" message in stderr as well.
5. Seems legit but out of scope for this PR.
6. Now we expect ANY between entries instead of SPACE. This makes the tests less precise but also more accommodating.

The remaining items are indeed smaller things. Leaving as-is for now.

[cfzimmerman]

🤖 Claude Review

Re-reviewed at c7dc0ee. Thanks — the responses all land, and most of the
previous round is closed out. One new finding, one nit, and one thing I want to
write down rather than argue.

Verified on a live rig (helios dpd + tofino-model) before writing this:

• cargo fmt --check -p swadm, cargo check -p swadm --all-targets clean.
• cargo test -p swadm — 2 passed, 5 ignored.
• cargo test -p swadm --test cli -- --ignored — 5 passed, run 12 times, no
  flakes. 0.75–1.13s idle; 1.49–4.48s with the box loaded 2x over its 16 CPUs.

New: TIMEOUT is now too short, for the same reason it was too long

swadm/tests/cli/cmd.rs:100

const TIMEOUT: Duration = Duration::from_millis(500);
const SLEEP:   Duration = Duration::from_millis(100);

TIMEOUT bounds the whole loop, not one attempt, and each attempt spawns a
swadm process and does an HTTP round trip. So 500 ms buys about four attempts
— and only if every attempt is fast.

Measured on the rig just now:

what	time
swadm link ls, swadm link serdes get txeq	30–35 ms
link delete → link get returns 404	78 ms
link apply → tx-eq visible in get txeq	80 ms
link serdes set txeq → new value visible, 3 runs	109 ms, 539 ms, 122 ms

That 539 ms is the problem. A single swadm invocation occasionally takes about
half a second on an idle machine, which is the entire budget. When that happens
retry silently degrades to one attempt and the test becomes exactly the
read-once race the helper exists to prevent — and it fails as an assertion
mismatch, which reads as a dpd bug rather than as a timeout.

It doesn't bite today because convergence is fast enough that the first read
almost always succeeds, so the retry path is nearly dead code. It gets exercised
precisely when CI is slow, which is when the budget is smallest.

Suggest seconds rather than milliseconds — from_secs(5) still fails fast at
roughly 50 attempts. The in-repo precedent for this same class of wait is
dpd-client/tests/integration_tests/counters.rs:42: 20 iterations at 100 ms,
2 s total, for SDE lag after a counter write.

Nit in the same block: the doc comment at cmd.rs:88 says "reasonable linear
backoff", but SLEEP is a constant 100 ms. Either drop the word or make it one.

Closed

1. from_secs(500) typo — fixed.

2. Setup before link apply — delete_link() is the right fix, and for a
slightly better reason than I gave. link apply is idempotent for settings,
but not for enabled: add_link sets link.config.enabled = true
(dpd/src/port_settings.rs:338), while modify_link can't — LinkSpec has no
enabled field at all. Both tx-eq push sites gate on link.config.enabled
(dpd/src/link.rs:1856, :1965). Deleting first forces the add branch every
time, so the tests no longer depend on who enabled rear0/0 last.

4. ANY separators — agreed, and SPACE fixes more than it looks like.
With mandatory \s+ between members, interior tokens are now self-bounding:
main\s+-1\s+ can't match -13, because 3 isn't whitespace. That was the
substance of my "10 matches 100" complaint and it's resolved. Only the first and
last members are still unbounded, which needs a pat! whose outermost member is
a bare number to matter — worth remembering, not worth code today.

For the record, the 404 failure you hit was the trailing ANY demanding
whitespace after Not Found where the output has ;. Dropping the ANYs was
right: expect_line uses unanchored is_match, so a leading or trailing ANY
can only ever be a no-op or a false negative.

5. swadm_exact escape hatch — the disclaimer at cmd.rs:54 is the right
call. Agreed the API quality is worth more than a problem nobody has.

6. Error::Proc — fine as is.

7. Fail-fast on dpd-client — agreed, and it's the better default. One
consequence worth holding in your head: a red packet-test job now means "swadm
untested", not "swadm fine".

8. README wording — fair, dropped.

Not a request, just a note

On teardown (3): agreed that setup is the contract and that both swadm suites
now honor it. The asymmetry to be aware of is that dpd-client does not —
Switch::init reads the MAC of every configured port and panics on failure
(dpd-client/tests/integration_tests/common.rs:493-505), with no setup of its
own, so it inherits whatever the last writer left behind.

In CI that's safe, because dpd-client now runs first. On a dev rig it means
swadm-then-packet-tests panics with failed to get mac for port rear0/0 while
packet-tests-then-swadm is fine. That's a legitimate constraint to accept — I'd
just rather it be a known one than a surprise someone debugs from scratch.

[taspelund]

It looks like this is pre-existing, but do you have any idea why this is mixed case?

[cfzimmerman]

This was new to me as well. Seems imposed by Cargo: https://doc.rust-lang.org/cargo/reference/environment-variables.html#:~:text=cleaned%20between%20builds.-,CARGO_BIN_EXE_,-%3Cname%3E%20%E2%80%94%20The

[taspelund]

You could always move sleep/timeout into args and expose a thin wrapper (fn or macro) that supplies the default. Not something I feel strongly about though, so feel free to ignore me if you disagree

[cfzimmerman]

Haha I often write comments trying to exonerate suspicious code only to realize later the comment could just be replaced by less suspicious code. Seems the case here. Thx!

retry now just calls a function retry_with with these defaults.

[taspelund]

This matches 0 or more characters, right? Is that worth calling out in the comment, since the others call out 1 being the minimum match length?

[taspelund]

this is slick, I didn't know you could do this inside of let destructuring

[taspelund]

This feels like it would be a good use case for expectorate

[cfzimmerman]

I agree that this particular case is a good use of expectorate. However since it's a test for the actual matcher, using that instead of expectorate is the whole point.

That begs whether expectorate is a better tool than expect_line. I'm putting a comment in the next commit, but my guess is not.

Tx eq is a good motivating case because it's arbitrarily structured and exposes uncontrolled state.

In this case, the values outside parentheses are what we have commanded, and afaik the values inside parentheses are what the hardware itself has decided to do. This occurs in the case of self optimizing tx eq. So we only want to assert the values outside parentheses controlled by swadm command. Hence the PARENS matches asserting that something exists there, but idk what's inside.

In my understanding, expectorate does exact matching, which makes it a poor fit for a fuzzy assert like this. Is that accurate?

[cfzimmerman]

Here's another plausible design:

• Get swadm output into a machine readable format. Either by adding formatted output options or building parsing infra.
• Allow tests to restructure that output to extract unnecessary tokens.
• Store known good versions of that output in files, and use expectorate for diffing.

However, unless someone feels strongly for a solution of that form, I'm disinclined because it seems like more required code per each new test.

[taspelund]

Similar feedback here about expectorate. As someone unfamiliar with the output of swadm link serdes get txeq, I have no idea what this test is looking for. I think being able to look at some output that's checked into get as a comparison would make it much simpler to reason about the test.

[cfzimmerman]

I'm not yet convinced about expectorate, but this is a good point. I'll add an EXAMPLE comment.

[taspelund]

Sure, no worries. I don't know expectorate super well, but I've seen it used for command output checking, both structured and unstructured, so I thought it would be worth evaluating. If you don't feel like it's a good fit, that's fine by me

[cfzimmerman]

Sorry for the repeated pings on this. Looks like we use expectorate in a lot of other crates. If I want people to contribute tests, probably good to use familiar tooling. I'll look for a way to make it work conveniently. Thx for suggesting.

[taspelund]

what are tap flags? I'm not sure what this test is doing without digging into swadm to figure out what the tap flags are. Maybe a reference to a command, source file, or type name would be helpful?

[bnaecker]

This is referring to the flags like --main or --pre1.

The term "tap" comes from signal processing, and refers to one shifted copy of the signal we're operating on. So --pre1 is the gain on a copy of the signal shifted backward in time by one sample. It's one of the gains in the filter we apply to the signal to try to improve quality.

[cfzimmerman]

I'm generally against unwarranted jargon. But tap seems pretty well standardized in tx eq literature, and it's a super convenient name 😅

[bnaecker]

Yeah, the term tap holds roughly the same position in signal processing as process does in software. If you're mucking with it, you know what it means :)

[taspelund]

Overall, the changes LGTM. I would say that the main bit of feedback I have is that it's hard to track what the tests are doing without already knowing that area of the code (what is a tap flag? what does the expected output look like?).

I'd like you to take a look at expectorate and see if that would be a good fit for some of these tests. If it doesn't make sense in this case, then maybe just a multi-line comment showing what swadm output the test is comparing against would be helpful.