Take pmm-admin version reference from client, not server - #1234
Closed
travagliad wants to merge 1 commit into
Closed
Take pmm-admin version reference from client, not server#1234travagliad wants to merge 1 commit into
travagliad wants to merge 1 commit into
Conversation
The Generic CLI's PMM_VERSION derivation on the feature-build / RC path resolved to `pmm_agent_status.server_version` (introduced in #1175). The pmm-admin --version / summary --version tests then asserted that string appeared in the client's own stdout. Server_version can legitimately drift from the client's stamp on submodules-only FB pushes. The pmm-managed RPM's S3 cache key (build-server-rpm's get_rpm_version) strips the build timestamp and is derived from the pmm submodule commit only, so a PR that never moves that pointer (e.g. Percona-Lab/pmm-submodules#4486, which only edits ci.py) keeps hitting the cached RPM stamped at first build. Every re-push then gets: fresh client tarball with the new full_pmm_version, cached pmm-managed with the initial one. Same tag, same code, two stamps. Compare client to itself instead: take PMM_VERSION from pmm_admin_version in the same status --json call. It is still "the artifact under test" per the #1175 rationale, just the client's own binary rather than the server it happens to be pointed at. Assertion stays exact-match, no loosening. Reproduced on a Linode VM against perconalab/pmm-server-fb:PR-4486-8abaa26 + pmm-client-PR-4486-8abaa26.tar.gz: - pmm-admin status --json.pmm_agent_status.server_version = 3.10.0-fix-switch-branch-slash-names-86ad073d8 - pmm-admin status --json.pmm_admin_version = 3.10.0-fix-switch-branch-slash-names-8abaa26f6 - pmm-admin --version prints 3.10.0-fix-switch-branch-slash-names-8abaa26f6 ## Failures fixed (investigator) - source: [Percona-Lab/pmm-submodules#4486](Percona-Lab/pmm-submodules#4486) -- run [32742352894](https://github.com/Percona-Lab/pmm-submodules/actions/runs/32742352894), check `CLI / Integration tests / CLI / Integration / Generic` (job [97479577166](https://github.com/Percona-Lab/pmm-submodules/actions/runs/32742352894/job/97479577166)) - tests: - `cli/tests/generic.spec.ts:109` / `@generic` -- run pmm-admin --version - `cli/tests/generic.spec.ts:136` / `@generic` -- run pmm-admin summary --version Signed-off-by: Claude (Investigator) <claude-bot@percona.com>
|
Warning Review limit reachedNext included review available in 13 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Failures fixed (investigator)
CLI / Integration tests / CLI / Integration / Generic(job 97479577166; same failure re-observed on the newer PR-4486-8abaa26 run)cli/tests/generic.spec.ts:109/@generic—run pmm-admin --versioncli/tests/generic.spec.ts:136/@generic—run pmm-admin summary --versionWhat failed
Both tests derive
PMM_VERSIONon the FB / RC path frompmm-admin status --json→pmm_agent_status.server_version(added in #1175), then assert that string appears verbatim in the client's ownpmm-admin --versionstdout. On PR-4486 the two stamps disagreed:Root cause
FB
perconalab/pmm-server-fb:PR-4486-1e7d532ships a pmm-managed binary stamped-86ad073d8(an earlier pmm-submodules commit), while the fresh client tarball at the same tag stamps-1e7d53200. They are the same source snapshot with two different build-time strings, so any equality check between them fails.Why they drift on a submodules-only PR:
build/scripts/build-server-rpm'sget_rpm_version()strips the build timestamp (sed -re 's/\.[0-9]{10}\././'), leaving the RPM cache key aspmm-managed-3.10.0-21.<pmm-submodule-short-commit>. That short commit isfull_commitfromprepare_specs, which without agit_patharg falls back to the pmm submodule's HEAD. PR-4486 (Percona-Lab/pmm-submodules#4486) only touches pmm-submodules's ownci.py— the pmm submodule pointer never moves — so every re-push hits the same S3 cache key and pulls the RPM built on the first attempt (Aug 21, HEAD86ad073d8). The client tarball is uploaded under a per-push key (pmm-client-${BRANCH_NAME}-${SHORTENED_COMMIT}.tar.gz), so it is always rebuilt and stamps the current pmm-submodules HEAD.Result: same tag, same functional code, two version strings. Server_version is no longer a reliable reference for what the client's stdout will contain.
Fix
Take
PMM_VERSIONfrompmm_admin_versionin the samepmm-admin status --jsonpayload instead ofpmm_agent_status.server_version. It is the client binary's own compile-time stamp — the exact same string thatpmm-admin --versionprints — so the check remains a strict equality test of client output vs. its own reported version, keeping the #1175 rationale ("track the artifact under test") but pointing at the client artifact these tests actually exercise. No assertion loosening.Repro & verification
Provisioned a throwaway Linode VM (
heal-4486), startedperconalab/pmm-server-fb:PR-4486-8abaa26(readyz), installedpmm-client-PR-4486-8abaa26.tar.gz, provisionedpdpgsql=16viapmm-framework. The two failing FB tests reproduced identically:pmm_agent_status.server_version=3.10.0-fix-switch-branch-slash-names-86ad073d8pmm_admin_version=3.10.0-fix-switch-branch-slash-names-8abaa26f6pmm-admin --versionprints-8abaa26f6After syncing this branch onto the same VM and re-running
npx playwright test --grep @generic tests/generic.spec.ts, bothrun pmm-admin --versionandrun pmm-admin summary --versionpass. (The remaining local failures — T1219, T2193, T2227 — are unrelated env deps: FB exportsENCRYPTED_CLIENT_CONFIG=trueinWIZARD_ARGS, setsPMM_CLIENT_VERSION, and provisions additional services; they pass green in the FB job itself, visible in the run 32742352894 log for the same tests.) The next FB re-push on PR-4486 (or any submodules-only PR) is the real acceptance check.Generated by Claude Code