PMM-7 Fix PMM-T2227 tarball upgrade version check - #1137
Closed
travagliad wants to merge 3 commits into
Closed
Conversation
Use CLIENT_IMAGE version instead of pmm-submodules VERSION file, quote tarball URLs, and harden agent restart after upgrade. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: davi.travaglia <davi.travaglia@percona.com>
Fix docker run invocation on pmm-client images and wait for tarball_client container to be running before setup. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: davi.travaglia <davi.travaglia@percona.com>
Use the client docker image version for --version checks when CLIENT_VERSION is a tarball URL, matching PMM-T2227 expectations. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: davi.travaglia <davi.travaglia@percona.com>
WalkthroughThe CLI tests now read expected versions from the configured client image when available. The tarball upgrade test validates startup, agent replacement, connectivity, version, and container cleanup. ChangesCLI version and upgrade validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/tests/generic.spec.ts`:
- Line 600: Validate the result from the oldPid docker exec call before
comparing restart PIDs: assert the command succeeded and that oldPid.stdout
contains exactly one PID. Update the restart comparison assertions around oldPid
and the corresponding new PID checks so an empty or malformed initial PID fails
the test instead of allowing any new PID to pass.
- Around line 612-623: The newAdminStatus command is executed once before
polling starts, so the subsequent assertion checking for 'Connected' uses stale
output captured before the agent had time to fully reconnect. Move the pmm-admin
status command execution and the outContains('Connected') assertion inside the
toPass() callback alongside the PID validation. This ensures the connectivity
status is checked on each poll interval rather than using the initial stale
capture, allowing the test to wait for the agent to actually reconnect after
restart.
- Line 625: The docker rm cleanup command at line 625 does not execute if an
earlier installation or assertion fails, leaving the container running and
affecting subsequent tests. Wrap the code section from container creation
through all assertions and test logic in a try block, and move the cli.exec
docker rm command into a finally block that ensures cleanup runs regardless of
test success or failure.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 83948907-6de1-4fac-8397-233fa8e249e4
📒 Files selected for processing (1)
cli/tests/generic.spec.ts
| const adminStatus = await cli.exec(`docker exec ${containerName} pmm-admin status`); | ||
| const oldVersion = await cli.exec(`docker exec ${containerName} pmm-admin version | grep "Version:"`); | ||
| const oldVersion = await cli.exec(`docker exec ${containerName} pmm-admin version | grep "^Version:"`); | ||
| const oldPid = await cli.exec(`docker exec ${containerName} ps -C pmm-agent -o pid=`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the old PID before you compare restart PIDs.
The test does not assert that oldPid succeeded or contained exactly one PID. If oldPid.stdout is empty, any new PID passes the restart comparison without proving that the original agent was replaced.
Proposed validation
const oldPid = await cli.exec(`docker exec ${containerName} ps -C pmm-agent -o pid=`);
+await oldPid.assertSuccess();
+const oldPids = oldPid.getStdOutLines().map((pid) => pid.trim()).filter(Boolean);
+expect(oldPids, 'Exactly one PMM Agent must run before upgrade').toHaveLength(1);
...
-const oldPidValue = oldPid.stdout.trim();
+const oldPidValue = oldPids[0];Also applies to: 614-620
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/tests/generic.spec.ts` at line 600, Validate the result from the oldPid
docker exec call before comparing restart PIDs: assert the command succeeded and
that oldPid.stdout contains exactly one PID. Update the restart comparison
assertions around oldPid and the corresponding new PID checks so an empty or
malformed initial PID fails the test instead of allowing any new PID to pass.
Comment on lines
612
to
+623
| const newAdminStatus = await cli.exec(`docker exec ${containerName} pmm-admin status`); | ||
| const newVersion = await cli.exec(`docker exec ${containerName} pmm-admin version | grep "Version:"`); | ||
| const newVersion = await cli.exec(`docker exec ${containerName} pmm-admin version | grep "^Version:"`); | ||
| const oldPidValue = oldPid.stdout.trim(); | ||
|
|
||
| await newPid.outNotContains(oldPid.stdout); | ||
| await expect(async () => { | ||
| const newPid = await cli.exec(`docker exec ${containerName} ps -C pmm-agent -o pid=`); | ||
| const pids = newPid.getStdOutLines().map((pid) => pid.trim()); | ||
| expect(pids, 'PMM Agent should be running after upgrade').toHaveLength(1); | ||
| expect(pids[0], `PMM Agent was not restarted. Old PID: ${oldPidValue}, New PID: ${pids[0]}`).not.toBe(oldPidValue); | ||
| }).toPass({ intervals: [1_000], timeout: 30_000 }); | ||
| await newAdminStatus.outContains('Connected'); | ||
| await newVersion.outContains(latestVersion); | ||
| await newVersion.outContains(expectedUpgradeVersion); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Poll live connectivity after the agent restart.
newAdminStatus is captured immediately after the detached start command. The PID poll does not refresh this result or prove that the agent is connected. A normal startup delay can therefore fail the test with stale status output.
Run pmm-admin status inside toPass(). Assert Connected on each current result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/tests/generic.spec.ts` around lines 612 - 623, The newAdminStatus command
is executed once before polling starts, so the subsequent assertion checking for
'Connected' uses stale output captured before the agent had time to fully
reconnect. Move the pmm-admin status command execution and the
outContains('Connected') assertion inside the toPass() callback alongside the
PID validation. This ensures the connectivity status is checked on each poll
interval rather than using the initial stale capture, allowing the test to wait
for the agent to actually reconnect after restart.
| await newVersion.outContains(latestVersion); | ||
| await newVersion.outContains(expectedUpgradeVersion); | ||
|
|
||
| await cli.exec(`docker rm -f ${containerName}`); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Move container cleanup into a finally block.
If installation or an assertion fails, line 625 does not execute. The detached container remains active and can affect later tests. Wrap the flow after container creation in try/finally, and remove the container in finally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/tests/generic.spec.ts` at line 625, The docker rm cleanup command at line
625 does not execute if an earlier installation or assertion fails, leaving the
container running and affecting subsequent tests. Wrap the code section from
container creation through all assertions and test logic in a try block, and
move the cli.exec docker rm command into a finally block that ensures cleanup
runs regardless of test success or failure.
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
2 participants
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.
Why
FB
CLI / Integration / Genericfailed on PMM-T2227 because the test compared the post-upgradepmm-admin versionagainstpmm-submodules/v3/VERSION(e.g.3.9.0/3.9.1) instead of the tarball/client image actually under test. FB and dev tarballs report feature-branch versions (e.g.3.9.0-v3-*or3.9.0-PMM-*), so the assertion failed even when upgrade succeeded. The same VERSION-file mismatch also brokepmm-admin --versionandsummary --versionin the Generic suite.How
CLIENT_IMAGEviadocker run --entrypoint pmm-admin ... --version --json.pmm3_client_install_tarball.sh.docker network connectargument order and harden pmm-agent restart/PID checks after upgrade.tarball_clientto be running before install steps; remove the container on completion.Triggered by FB failure on Percona-Lab/pmm-submodules#4505 (
CLI tests Generic).CI: FB integration run 30909007760 — PMM-T2227 passed; follow-up commit fixes two related version-string tests.
Summary by CodeRabbit
Bug Fixes
Tests