SEP-2048: Bind NOMAD_VERSION to the feature build it was read from

[yyyyyyyan]

Summary

sidecar/pmm-fb/Containerfile.mysql asserts that the PMM Client copied into
sep-mysql ships the Nomad version compose.yaml names. Whether that assertion
means anything depends on which client was copied, and PMM_CLIENT_IMAGE
decides:

• empty — the client is the amd64 feature build at PMM_FB_TAG, so the
  assertion binds NOMAD_VERSION to the artifact a repin moves.
• non-empty — an arm64 engine, where bootstrap.sh selects the released
  percona/pmm-client:3.9.1. That client's Nomad cannot move with PMM_FB_TAG,
  so the assertion compares 2.0.5 against 2.0.5 and a repin that left
  NOMAD_VERSION stale still builds. The client/server mismatch then ships
  undetected: registration succeeds and only raw_exec placement misbehaves.

Two layers close that, keyed on PMM_CLIENT_IMAGE rather than on architecture —
the blind spot is "the copied client is not the feature-build client", which
today coincides with arm64 but is not the same property.

NOMAD_VERSION_FB_TAG and a second RUN (Containerfile.mysql:53-64). The
new build arg records which feature build NOMAD_VERSION was read from, and the
guard refuses a build that supplies a client image while pinning a different
PMM_FB_TAG. It abstains when PMM_CLIENT_IMAGE is empty, so lines 49-51 stay
the sole authority on the amd64 path and are unchanged byte for byte. The
existing NOMAD_VERSION="" opt-out still bypasses both checks. The three ARG
re-declarations at :59-61 are load-bearing: PMM_FB_TAG and
PMM_CLIENT_IMAGE are declared before the first FROM, which Docker scopes to
FROM lines only, so omitting a re-declaration expands them to empty and the
guard would pass every build while looking installed.

.github/workflows/pmm-fb-nomad-pin.yaml. The guard can distinguish a
witness left behind from one restated, but not an honestly restated witness from
one bumped to silence the error — and bumping it is the minimum edit that
silences the message. A pull_request job on pmm pulls the feature-build
client on a native amd64 runner and holds its tools/nomad version to
NOMAD_VERSION. Nothing runs on the arm64 build host, which is what keeps that
path free of the amd64 pull it exists to avoid. The same job runs
tests/sidecar/test_pmm_fb_nomad_pin.py, so the guard's truth table and the
ARG re-declarations are gated rather than merely covered locally; the module
reaches app/__init__.py and tests/conftest.py and nothing else, so pytest,
PyYAML and cryptography are its whole requirement.

The one case that may skip is narrow, deliberately: a pin the PR inherited
whose feature build has since been collected. The job resolves the base
revision's PMM_FB_TAG and compares it to the one under test, so a pin this PR
introduces that cannot be pulled — a typo, an unpublished build — fails red.
That case is not otherwise catchable by the agreement check, which proves the
three slots match each other and is satisfied by setting all three to the same
nonexistent value. Rate limiting, an auth change and registry outages fail red
too. A green check that verified nothing would be the same defect this ticket is
about, one layer up. The path filter covers scripts/check_pmm_fb_pins.py, the
test module and the workflow file as well as sidecar/pmm-fb/**, so a PR
editing only the reader the gate depends on cannot merge unrun.

scripts/check_pmm_fb_pins.py is the single reader both the new test and the
job use, so the two cannot drift. Its default mode also makes the
pmm-server image / PMM_FB_TAG agreement enforced — that pairing was
documented as "a repin edits both" and nothing checked it. Each slot is held to
the variable it is keyed by, not merely to carrying some ${NAME:-default}:
a slot quietly rewired to another variable agrees on committed defaults while an
exported PMM_FB_TAG moves only its siblings, which is the same mismatch
reached without changing a literal. A slot is also required to resolve to
something: three empty defaults agree with each other, which would have been the
same vacuous comparison one layer up, and a build.args node carrying no
mapping at all is refused by name rather than raising out of the reader. The
same check runs in pre-commit, keyed on the compose file and the reader, so a
local repin is answered before the push.

tests/sidecar/test_pmm_fb_nomad_pin.py covers the guard's truth table by
extracting the shipped RUN body and running it under sh, asserts statically
that the final stage declares every argument either guard reads (NOMAD_VERSION
included — it is read by both guards before either can fail, so moving its
declaration up would retire both at once), holds the job's image reference and
binary path to the ones the Containerfile names, and checks the pin reader
against the committed file and against tampered copies. Every value it uses is
read from the artifact rather than restated, so a repin cannot leave it passing
against an old tag.

Docs updated to separate the three checks rather than describe them as one:
README.md, mysql-target.md (a repin now moves three values, not two), and
two bootstrap.sh comments that described the vacuous comparison as if it were
a real check. Both docs state the skip condition as the job enforces it — both
pin values inherited unchanged, and that build since collected — rather than as
"collected tags are waved through", which a repin would have read as covering
itself.

The job this PR adds has been observed running on it

It declares no needs:, so the label gate does not hold it back, and it ran on
this PR's own head rather than only being read as YAML. It took the verifying
branch, not the warn-and-skip one:

docker.io/perconalab/pmm-client-fb:PR-4500-6632e5e ships Nomad v2.0.5,
matching the committed NOMAD_VERSION

So the gate pulled the feature-build client on a native amd64 runner and held
the committed NOMAD_VERSION to its real tools/nomad — which is the half of
this change that verifies rather than attests. The pytest step reported
29 passed in the same run. Re-check both after any further push, since a later
head has not been observed.

Known limitations

• The build-time guard establishes that the pin was restated against this
  feature build, not that anyone re-read the client's Nomad. Only the CI job
  establishes the value itself.
• When a pin the PR inherited names a feature build since collected from
  perconalab, the job warns and skips rather than failing: this harness pins
  throwaway artifacts, so failing red there would block every later PR touching
  these paths behind an unrelated repin. That is the one unverified path, it is
  reported as a warning naming the tag, and it cannot cover a pin the PR itself
  introduced.
• ci.yml is scoped to main, where sidecar/pmm-fb/ does not exist, so the
  project's pytest tier still never fires for the PRs these tests protect. The
  job added here runs the module directly instead, which covers it for this base
  branch but leaves the tests outside the suite every other test in the repo
  belongs to.
• The guard's RUN sits beside the existing assertion, so a tag-only repin on
  the arm64 path now invalidates the cached layers below it, including the
  test_db archive fetch. Keeping the two guards adjacent was deliberate — they
  are one mechanism and read as a pair — and the cost is one archive re-fetch per
  repin, not a rebuild of the package layers, which stay above the guard.

Unrelated local failure

tests/sidecar/test_encryption_key.py::test_a_peer_holding_the_state_lock_defers_then_refuses
times out locally. It exercises sidecar/encryption_key.py and
app/core/encryption.py; this diff touches neither. The failure is a lock-wait
timeout, not an assertion. The only workflow running on this base branch is the
one added here, which is scoped to the pin and does not exercise that module, so
there is no CI result to corroborate against.

Tested

• On an arm64 engine with the harness bootstrapped, move PMM_FB_TAG
  (in compose.yaml or the environment) without touching
  NOMAD_VERSION_FB_TAG, and rebuild sep-mysql forcing a rebuild of the
  image: the build is refused and the message names both tags and points at
  the amd64 remedy.
• Same engine, committed pins unchanged: sep-mysql builds to completion
  and the node registers with pmm-server as before.
• On an amd64 engine with the client-image slot empty, move PMM_FB_TAG and
  leave NOMAD_VERSION stale: the original assertion still fails the build,
  with its original message.
• With NOMAD_VERSION set to the empty string on either engine, both
  build-time checks are skipped, as before this change.
• The PMM FB Nomad pin job on this PR is green, and its log shows the
  feature-build client's Nomad version matching the committed
  NOMAD_VERSION.

Checklist

• New/modified functions have type hints and rST docstrings
• New tests added for new features or bug fixes
• Database migrations generated if models changed (make makemigrations) — N/A, no models changed
• User-facing changes documented (README, inline help, UI text)
• Configuration changes documented with examples

[Copilot]

🟡 Changes recommended

The guard tests are not automated on pmm pull requests, and malformed build-argument mappings are not normalized.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds layered safeguards binding NOMAD_VERSION to its PMM feature-build artifact.

Changes:

• Adds build-time witness validation and CI artifact verification.
• Introduces pin parsing and regression tests.
• Updates operator documentation and bootstrap messaging.

File summaries

File	Description
.github/workflows/pmm-fb-nomad-pin.yaml	Verifies Nomad against the feature-build client.
scripts/check_pmm_fb_pins.py	Validates committed compose pins.
sidecar/pmm-fb/Containerfile.mysql	Adds the witness guard.
sidecar/pmm-fb/compose.yaml	Declares the witness pin.
sidecar/pmm-fb/bootstrap.sh	Clarifies arm64 behavior.
sidecar/pmm-fb/README.md	Documents layered verification.
sidecar/pmm-fb/mysql-target.md	Updates repinning guidance.
tests/sidecar/test_pmm_fb_nomad_pin.py	Tests guard and checker behavior.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 10
• Review effort level: Balanced

---

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

[yyyyyyyan]

Follow-up from this review's own pass, deliberately not taken in this PR.

tests/sidecar/test_pmm_fb_nomad_pin.py holds five tests that exercise scripts/check_pmm_fb_pins.py, while tests/scripts/ already carries 17 modules for scripts/*.py with a shared load_script() loader and the main(argv) -> int plus capsys shape — tests/scripts/test_check_nomad_payload_size.py is the structural twin. The mirror is by package under test rather than by scenario topic, so those five belong at tests/scripts/test_check_pmm_fb_pins.py.

Left here because moving them splits a module that currently reads as one mechanism, and the workflow's path filter and pytest invocation would both have to name two paths. That trade is worth deciding on its own rather than inside a change about the Nomad pin.

[yyyyyyyan]

Follow-up from this review's own pass: a sibling of the empty-pin refusal added in this PR, deliberately not changed.

sidecar/pmm-fb/Containerfile.mysql:63 passes when PMM_FB_TAG and NOMAD_VERSION_FB_TAG are both blank, which test_guard_verdicts pins as the both-tags-blank case. The refusal added to the pin checker covers committed state, so a fresh clone cannot reach it; an operator exporting both as empty at build time still gets a pass with NOMAD_VERSION unvalidated.

Narrow — a blank PMM_FB_TAG also leaves pmm-server on a tagless image reference, so the stack breaks elsewhere — and changing it would alter a truth table the tests deliberately fix. Raising it rather than widening the guard unasked.